mikebridge commented on PR #41551:
URL: https://github.com/apache/superset/pull/41551#issuecomment-5132088867
## Consolidated review — version-history UI
Two independent review panels were run over `master...HEAD` (57 files,
+8,459/−98) at `0d2e568fc6`: a Codex panel (6 lenses) and a second panel (react
/ preset / committer). Findings below are merged and de-duplicated; **⚑ marks
items two or more independent lenses found separately**, which is the strongest
signal in here.
Worth stating plainly: the two panels disagreed sharply in places. The Codex
run's `preset-review` returned *no findings and approved*, while the second run
of the same lens returned 20 findings including 4 High. Treat neither panel's
silence as evidence.
### Already resolved — please disregard from the Codex report
Two of that report's three Blockers no longer apply:
- **"PR description omits testing instructions / checklist"** — the
description has been rewritten and now carries testing steps and a completed
checklist. *(The BEFORE/AFTER screenshot gap does still stand.)*
- **"Required frontend validation has not run"** — CI on this revision is
**63 passing / 0 failing**.
---
### High — worth fixing before a committer pass
**1. ⚑ The extracted `canOverwriteSlice` silently reverts the chart-owner
overwrite path — and it is the one change in this PR not behind the feature
flag**
`superset-frontend/src/explore/exploreUtils/canOverwriteSlice.ts:46` ·
`SaveModal.tsx:228`
SaveModal's previous local helper was `can_overwrite || isUserAdmin ||
canEditSlice || isCurrentUserOwner()`. The shared helper has no `owners`
handling, so a chart **owner** who is not an editor and not an Admin now gets
"Save (Overwrite)" disabled and the default flips to Save-As. That owner branch
was added deliberately in #41352 ("let chart owners overwrite when owners come
through as objects").
Two aggravating factors:
- Every other change here is gated by `VERSION_HISTORY`; **this one alters
the Save-modal gate for all deployments on merge**.
- The existing regression test `SaveModal.test.tsx:361` still passes
**vacuously** — its fixture slice carries `editors: [{id: 1}]`, so it now
succeeds via the editors branch and no longer exercises owners at all.
The direction may well be correct (the server's `is_editor` does not consult
`owners` either, so the old client gate was over-permissive and owners would
have hit a 403). But it should be a stated decision, not a side effect: either
restore the owner route, or note the revert in the PR description and rewrite
that test to assert the new behaviour.
**2. ⚑ Dashboard restore gate omits `is_managed_externally`, so the URL
param bypasses what the menu hides**
`DashboardVersionHistory.tsx:57` · `DashboardBuilder.tsx:438,770`
`canOverwriteSlice.ts:55` deliberately refuses externally-managed slices.
The dashboard side does not mirror it: the header menu correctly ANDs
`!is_managed_externally`, but `canRestore` uses `dash_edit_perm` alone. So on
an externally-managed dashboard the menu entry is hidden while
`?version_history=true` opens the panel **with Restore enabled**, and the
preview banner's Restore button renders. The server's restore endpoint has no
`is_managed_externally` check, so the UI is the only guard. This is the same
class of gate divergence the `canOverwriteSlice` extraction set out to
eliminate.
**3. `window.open` is called after `await`, so "Open as new" is
popup-blocked outside Chrome**
`useVersionActions.tsx:157` · `openRelated.ts:49`
`openAsNew` awaits the snapshot fetch and the create call before opening the
tab. Safari blocks any `window.open` outside the synchronous gesture task;
Firefox/Chrome block once transient activation lapses, which two sequential
round-trips will routinely exceed. It fails **silently** — the success toast
fires and the entity *is* created — so users see no tab, click again, and fork
duplicates. Open a blank tab synchronously on click and set its location, or
replace it with a toast containing a real link.
Related, and found independently by both panels: the in-flight guard reads
render-time `isCreating` before `setIsCreating(true)`, so two fast activations
both observe `false`. Use a ref-based lock cleared in `finally`, and propagate
the disabled/loading state to the kebab triggers, not just the banner button.
**4. ⚑ The restore path discards both the server error and the server's
partial-success message**
`useVersionActions.tsx:106,120`
The `restoreVersion` return value is dropped and the catch emits a fixed
string with no `getClientErrorObject`, so a 403 and a 422 carrying a real
reason are indistinguishable to the user. More seriously, the endpoint returns
`"OK; N chart(s) referenced by the snapshot no longer exist and were not
reattached"` on partial success — the frontend never reads it, so **a dashboard
restore that silently drops chart associations reports an unqualified
success**. `useVersionActivity.ts:131` already uses `getClientErrorObject`, so
this is an inconsistency within the feature.
**5. ⚑ The restore orchestration and the chart preview have no tests at all**
`useVersionActions.tsx` · `ChartVersionPreview.tsx`
Neither file has a test. Untested: no-op detection, the re-entrancy guard,
both failure toasts, and the whole `openAsNew` flow. Both container suites mock
`useVersionActions` to a stub, so the kebab restore path is unexercised
end-to-end, and `ChartVersionPreview` (236 lines, one of the four
`canOverwriteSlice` call sites) has no coverage of snapshot fetch, datasource
switching, stale-response protection, or error rendering.
**6. Preview lifecycle is one ~145-line effect**
`useDashboardVersionPreview.ts:178`
Fetch orchestration, snapshot resolution, store hydration and teardown are
encoded in a single effect, which is where the concurrency bugs in this feature
will keep coming from. Related: `DashboardVersionHistory` and
`ExploreVersionHistory` independently reimplement URL-triggered opening,
cleanup, debounced fetching and refresh detection — fixes to one will drift
from the other.
---
### Medium
- **Preview gate does not actually disable descendants.** `aria-disabled` is
descriptive only, and `pointer-events: none` plus keydown suppression do not
block programmatic or AT activation — focusable chart menus and links stay live
during historical preview. Consider `inert`. Separately, the key handler passes
through only Tab/Escape, so it also swallows Space, PageUp/Down, Home/End and
arrows — a keyboard user cannot scroll a long previewed dashboard (WCAG 2.1.1).
- **Two rollout switches with hidden coordination.** `VERSION_HISTORY` can
be on while `ENABLE_VERSIONING_CAPTURE` is off, giving a visible panel that
misleadingly reports "No history yet."
- **Flag-off behaviour is untested for four of seven gated surfaces**
(dashboard header menu, DashboardBuilder panel, PreviewBanner,
ExploreViewContainer). The two new DashboardBuilder tests never set
`window.featureFlags`, so they assert the preview gate with the flag *off* —
the opposite of the configuration they model.
- **Some layout DOM/CSS ships unconditionally**, outside the flag: the
dashboard grid template changes for everyone, and four new wrapper `<div>`s are
inserted around FilterBar / DataSourcePanel / control panels in deployments
that have the flag off.
- **`canRestore` is an authorization prop that defaults to `true`** in both
`VersionHistoryPanel` and `PreviewBanner`, and no test pins it — removing the
gate term leaves the suite green.
- **The URL-param authorization gate has no test at either call site** —
dropping `&& canRestore` from both leaves everything green. This is the
security-relevant fix in the PR and the least covered.
- **A missing effect cleanup** in `useDashboardVersionPreview.ts:313` lets
an in-flight `apply()` hydrate the global store after unmount, clobbering
whatever page mounted next. Every other async site in this feature invalidates
on teardown.
- **Restore fires two activity refetches** — the "sync so it does not
refetch" line runs before the value it means to absorb changes.
- **Dedupe key omits `from_value`/`to_value`**, so genuinely distinct
records sharing kind/operation/path within one transaction are dropped on page
merge — the collision condition the code documents elsewhere.
- **Groups with no visible records can be restored but not previewed**
(restores, imports, clones, scaffolding-only saves), which inverts the desired
risk profile: preview matters most *before* a destructive restore.
- **i18n**: sentences assembled from independently translated fragments, a
hardcoded English plural (`'%s charts updated'`) two functions below a correct
`tn()` usage, and an entity link produced by `lastIndexOf` string surgery on a
translated headline — which silently degrades or links the wrong substring
under translation.
- **No user or operator documentation** for a new panel on two of the
most-used pages: the two-flag setup, retention, who may view or restore, and
the preview/restore/open-as-new model are all undocumented.
- **`describeRecord` is a ~95-line conditional dispatch table** mixing
classification, value extraction, templates and fallback at one level.
### Low
Nested interactive controls under `role="button"`; previewed row signalled
by colour alone with no `aria-pressed`; no focus management or Escape-to-close
on the panel; session-log middleware dispatching on every explore control
keystroke whether or not the panel is ever opened; unguarded `JSON.parse` of
snapshot `params` surfacing as a generic failure; two timestamp-parsing
conventions in one feature plus lexicographic ordering of `issued_at`;
hardcoded en-US date formats; `editors: []` fabricated for preview charts when
the real value is in hand; deprecated antd `Alert message` prop; search input
missing `maxLength` against a documented 1024-char server cap;
`window.featureFlags` mutated without restore; unrelated `package-lock.json`
churn (two `@noble/hashes` entries).
### Open process question
The Codex committer lens raised that no approved SIP is linked for this
feature. That one is a judgement call for a committer rather than something to
fix in code — worth resolving explicitly, either by linking the accepted
versioning SIP as covering this UI and restore behaviour, or establishing that
on `dev@`.
---
### Worth saying — what is good here
The fetch-invalidation discipline is the strongest thing in this diff.
`useVersionActivity` bumps its fetch id *before* the `!uuid` guard specifically
so clearing the uuid invalidates an in-flight response, and
`useDashboardVersionPreview` bumps unconditionally on preview-close so
historical data cannot land after the interaction gates disappear — that is the
failure mode a preview feature actually dies of, and both are commented with
the *reason* rather than the mechanism. The session-log middleware inlines its
action constants to keep explore out of every page's bundle and then exports
them so a test can pin them against the real constants, turning a silent
death-by-rename into a failing test. And the preview key gate cites WCAG 2.1.2
for the Tab/Escape carve-out, which normally only surfaces in an audit.
_This review was generated by Claude (AI) on behalf of @mikebridge._
--
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]
