mikebridge commented on PR #41550:
URL: https://github.com/apache/superset/pull/41550#issuecomment-5166719110
# Frontend self-review pass at `1816052493`
An AI-assisted frontend-only pass over `preset/master...HEAD` (49 commits,
85 files, +17,348 −5,829 — the 24 `.ts`/`.tsx` files of it), run against our
React/Redux review checklist. Every `file:line` below was re-read at that SHA
before posting, and anything that didn't survive the check was dropped rather
than hedged.
Prior threads from @kgabryje and codeant were read first; the five
substantive frontend findings are all genuinely resolved in the tree, so they
aren't repeated here. **No Critical issues.** Two type-contract weaknesses
follow — neither is reachable as a user-visible bug today, both are one-line
fixes.
## Warning 1 — bulk delete discriminates on the optional `source_type`,
while every other semantic-view branch uses the required `kind`
`superset-frontend/src/pages/DatasetList/index.tsx:1278`, `:1281`, `:1590`
`Dataset` declares `kind: 'physical' | 'virtual' | 'semantic_view'`
(required) but `source_type?: 'database' | 'semantic_layer'` (**optional**,
`:153`). The new bulk-delete routing and the semantic-view count driving the
modal's recoverability promise both key off the optional one:
```ts
d => d.source_type === 'semantic_layer' // :1281, and :1590 for the
count
```
whereas row rendering (`:844`), the row-action branch, and export (`:615`)
all use `kind === 'semantic_view'`.
Were `source_type` ever absent, `undefined` classifies a semantic view as a
regular dataset, so `pendingBulkSemanticCount === 0` → `recoverable={softDelete
&& true}` at `:1549` → the modal drops the type-DELETE gate and promises *"will
be moved to Recently Archived. You can recover them there"* — then sends a
semantic-view id to `DELETE /api/v1/dataset/`. This file's own export comment
at `:607-613` spells out what follows: that endpoint *"looks rows up by bare
numeric id against `tables` only — passing a semantic-view id silently returns
whatever SqlaTable happens to share that id."*
**Not reachable today** — `superset/datasource/schemas.py:45` and `:96` both
emit `source_type` via `fields.Constant`, so it is always present. That is what
keeps this a Warning rather than a Critical. But the TS type says otherwise, so
nothing in the compiler or the tests binds that guarantee, and the failure mode
is deleting an unrelated object under an "Archive" promise.
**Fix:** use `kind === 'semantic_view'` at all three sites, matching the
path that already documents the hazard. `kind` is the stronger discriminator —
`SemanticViewListSchema.kind` is a `fields.Constant("semantic_view")`
(`schemas.py:95`), so a dataset can never produce that value, while
`DatasetListSchema.kind` is a plain `fields.String()`. Making `source_type`
required in the TS type would also close it, but leaves two fields encoding one
fact.
## Warning 2 — `ArchivedItem` doesn't declare the field the Archived column
renders
`superset-frontend/src/pages/ArchivedList/types.ts:84-95`, rendered at
`superset-frontend/src/pages/ArchivedList/index.tsx:325`
The type declares `deleted_at?: string | null` (`:88`), used only as a sort
id. The value actually rendered is `deleted_at_delta_humanized`, which is **not
declared** — it resolves through the `[key: string]: unknown` index signature
at `:95`, so `String(original.deleted_at_delta_humanized ?? '')` compiles
regardless.
The server attaches it post-`dump` in `superset/views/filters.py:437`
(`_inject_deleted_at`), and it is genuinely nullable —
`deleted_at_map.get(row_id, (None, None))` yields `None` for any row missing
from the projection. A rename or a null gives a silently blank Archived column
with no compile error, and the unit fixtures supply the field
(`ArchivedList.test.tsx:73`), so they would keep passing against a renamed API.
The sibling convention already handles this: `src/views/CRUD/types.ts`
declares `changed_on_delta_humanized` in three places.
**Fix:** declare `deleted_at_delta_humanized?: string | null` on
`ArchivedItem`. Worth considering narrowing the index signature to just the
three per-type name fields, since that signature is what let this through.
## Suggestion — `config.deletedRecencyOperator as FilterOperator`
`superset-frontend/src/pages/ArchivedList/index.tsx:414`. Both operators are
typed as bare `string` in the config (`types.ts:44,46`), so this cast is
load-bearing rather than cosmetic. Typing those fields as `FilterOperator` (or
a union including the soft-delete operators) removes the cast and turns an
operator typo into a compile error instead of a rison request the API rejects.
## What looks good
- **The `inFlight` guard is the right shape.** `inFlightRef` as the
authority with `inFlight` state mirroring it for rendering
(`ArchivedList/index.tsx:183-198`) handles the "two fast clicks both pass a
state check" race, and `await refreshData()` before `finally` keeps the guard
alive past the stale row. Both paths clear in `finally` — no way to enter and
not exit.
- **`useAppSelector` is used** (`:21`, `:450`) — the typed hook from
`views/store.ts` that only ~23 files have adopted. The selector reads `roles`
without constructing a new object, so it stays referentially stable.
- **Type selection is derived, not stored** (`:489-493`), with the reasoning
for why a `useState` initializer would strand the Select on a value its options
no longer contain.
- **The recoverability promise matches the effect on the bulk path** —
`pendingBulkSemanticCount` and `handleBulkDatasetDelete` apply the identical
predicate to the identical array, and React 18 batches the two `setState`s in
`onSelect` (`:1589-1593`), so the count can't be read stale.
- **The flag-off path is genuinely unchanged** in `CRUD/utils.tsx` (each
ternary preserves the original msgid verbatim), and the route is gated in
`routes.tsx` behind a `lazy()` import, so flag-off deployments don't even fetch
the chunk.
## Checked and clean
Recording these so the next pass doesn't re-spend the effort.
- **Retracted mid-review:** I suspected the single-row dataset delete would
promise "Archive" for a semantic view, since `recoverable={softDelete}` at
`:1379` has no semantic-view guard where the bulk path at `:1549` does. It
doesn't — the Cell branches on `kind === 'semantic_view'` at `:844` and routes
semantic views to `handleSemanticViewDelete` → `/api/v1/semantic_view/{id}`,
whose modal at `:1503` passes no `recoverable` (full danger treatment) and
toasts "Deleted:", not "Archived:". The mirror is covered.
- **The type-to-confirm gate is armed by default.**
`DeleteModal/index.tsx:42-49` defaults `requireConfirmationText = true` /
`recoverable = false`, so the row purge (`ArchivedList:119-137`, passing
neither) gets the gate; `hide()` and `confirm()` both reset `disableChange`
(`:63-73`).
- No unstable `useSelector` results, no reducer mutation (the branch adds no
reducers), no dispatch-during-render.
**Not verified:** no browser walkthrough, no Playwright or Jest run, no
profiling, no flag-on/flag-off visual diff. The three Playwright specs and five
test files were read from the diff, not executed. Backend behaviour was
inferred from the Marshmallow schemas and `views/filters.py`, not observed
against a running instance. The Python portion of the diff
(`commands/purge.py`, `purge_cascade.py`, the migration) was out of scope for
this pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
