kgabryje commented on PR #41551:
URL: https://github.com/apache/superset/pull/41551#issuecomment-5144304941
# Independent review — three lenses over `075241a2f7`
Ran three independent reviewers over `master...HEAD` at
`075241a2f71a2009d1740cd65ca5252187c920c8` (merge base `7d2b184079`):
correctness/server-contract, frontend state-and-races, and a global
architecture pass. These are AI panels, same as the ones already posted above —
so I re-read and confirmed every `file:line` below against source myself before
posting. Where a reviewer's claim didn't survive that check, I've said so
rather than repeating it.
Given how much review this branch has already absorbed, I aimed them at
*verifying the landed fixes* and *reconciling the PR body against the code*
rather than another defect sweep. Findings the four prior panels already cover
aren't repeated here.
**Verdict: nothing here blocks merging it dark.** One real bug should be
fixed before `VERSION_HISTORY` is turned on anywhere, and one class of issue is
worth a look because it recurs.
---
## The unflagged change is clean
All three reviewers cleared `canOverwriteSlice` independently, and I checked
the history myself. Both load-bearing claims in the PR body hold:
- **`owners` was provably dead.** #38831 (`33f0fc93ed`, 2026-07-08) removed
`Slice.owners`; #41352 (`c0e5f5226d`, 2026-07-13) added the frontend owner
branch five days later, against a model that no longer had the field. `Slice`
today has `editors`/`viewers` and no `owners` attribute, and `Slice.data` emits
no `owners` key. Dropping the branch is a genuine no-op.
- **`extra_editors` is a strict widening.** Full case enumeration over
`{slice-null, is_managed_externally, can_overwrite, admin, editors,
extra_editors, owners}` gives NEW ⊇ OLD, with `extra_editors` the only
newly-true set. It moves the client toward `is_editor`
(`superset/security/manager.py:4935-4937` unions `get_extra_editor_subject_ids`
with `resource.editors`), and `EXTRA_EDITORS_RESOLVER` can't produce a wire
shape the helper mishandles — the server normalizes to `list[int]` at
`security/manager.py:127-167`.
The withdrawal was correct. Worth stating plainly so it doesn't get
re-litigated.
---
## 1. Re-selecting the version you're already previewing wedges the preview
permanently
This is the one I'd fix before the flag goes on anywhere.
`SET_VERSION_PREVIEW` re-enters the applying state unconditionally:
```ts
// reducer.ts:181-186
case SET_VERSION_PREVIEW:
return { ...state, preview: action.preview, isPreviewApplying: true };
```
Only `ChartVersionPreview`'s `.finally` or the dashboard hook's
`apply().then` can leave it, and both sit inside effects keyed on `versionUuid`
(`ChartVersionPreview.tsx:214` — `[dispatch, entityUuid, versionUuid]`).
Dispatch the **same** `versionUuid` again and the deps are unchanged, so the
effect never re-runs and nothing ever announces completion. The dashboard hook
has an independent second route to the same wedge —
`useDashboardVersionPreview.ts:286` early-returns on `appliedVersionRef.current
=== versionUuid` without dispatching `versionPreviewApplied()`.
**It's reachable by ordinary clicking.** `ActionRow.tsx:212` is
`onClick={onPreview}` on *every* change row, including rows of the group
currently being previewed. `previewIntent()` (`SaveGroupItem.tsx:261`) sees
`isCurrent === false` and calls through with an identical `versionUuid`.
**Result:** `isPreviewApplying` stays `true` for the rest of the page's
life. `PreviewBanner.tsx:82-83` renders *"Loading historical version"* over a
snapshot that is fully applied and on screen, and `PreviewBanner.tsx:101`
(`canRestore && !isApplying`) withholds Restore forever. Only exiting and
re-entering clears it. This is exactly the reading the comment at
`PreviewBanner.tsx:79-81` says a user must never be given, and `e33ba0cc27`
closed the failure path but not this one.
Reproduced empirically against the real reducer and store — a paste-in
regression test is at the end of this comment. The narrowest fix is at the
reducer, since it closes both call sites at once: make `SET_VERSION_PREVIEW` a
no-op when `action.preview.versionUuid === state.preview?.versionUuid`.
---
## 2. Preview is not a faithful preview of what restore will do
Three reviewers reached this from three unrelated directions, which is why
I'd rank it above anything else structural:
- **Chart membership.** Restore rebuilds it from the
`dashboard_slices_version` shadow, validity-windowed at the target transaction
(`superset/versioning/restore.py:190-249`). Preview rebuilds it by scanning
`position_json` for chart ids (`useDashboardVersionPreview.ts:97-103`). These
disagree whenever `dashboard.slices` and `position_json` disagree — e.g. a
chart still a member but dragged out of the layout. Preview shows it absent;
restore re-attaches it.
- **Scalar properties.** `Dashboard.__versioned__` excludes only
ownership/audit columns — so `description`, `theme_id`,
`certification_details`, `slug` and `published` *are* captured and *are*
restored. The client's `DashboardVersionSnapshot` (`types.ts:112-122`) names
none of them, and preview overrides only title, CSS, metadata and position.
Preview an old version and it still renders the live theme and description;
restore changes both.
- **Chart definitions.** `resolveSnapshotCharts` reuses `liveCharts`
verbatim, so each chart in a previewed dashboard renders with its *current*
definition, not the one it had at that version.
`docs/.../version-history.mdx:66-69` says preview *"reconstructs the layout as
it was"* without noting this.
For a feature whose safety story is "look before you destroy," the gap
between what preview shows and what restore does seems worth closing — or at
least documenting. The membership half needs the snapshot endpoint to return
the membership set; the scalar half is frontend-only.
Related: **"Open as new"** sends only title, CSS and JSON metadata to the
copy endpoint, so the new dashboard gets none of the version's description,
theme, certification, publication state or slug.
---
## 3. Several fixes landed on one entity type but not its mirror
Calling this out as a pattern rather than four tickets, because it predicts
where the rest are:
- **`extra_editors` fixed for charts, not dashboards.**
`canUserEditDashboard` → `isUserDashboardEditor` intersects `dashboard.editors`
only (`dashboard/util/permissionUtils.ts:55-63`), while the server's
`is_editor` unions `extra_editors`. A user granted editorship through
`EXTRA_EDITORS_RESOLVER` is refused the menu, the Restore control, and
`?version_history=true` on a dashboard the API would let them restore.
`Dashboard` doesn't type the field at all (`types/Dashboard.ts:22-42`).
- **Unmount guard fixed for dashboards, not charts.** `aa76d865b3` added
`isMountedRef` to the dashboard path. The chart path at
`ExploreVersionHistory.tsx:199-203` still dispatches `hydrateExplore`
unconditionally when the fetch resolves — the uuid check at `:186-191` runs
*before* the request and can't protect its completion. Restore chart A,
navigate to chart B, let A's `/api/v1/explore/` response land: B's Explore
state is replaced with A's payload.
- **Empty-state edit link fixed in one of two places.** `2c275d34d0` gated
`DashboardBuilder.tsx:816-821`; the identical control inside an empty *tab*
(`gridComponents/Tab/Tab.tsx:350-363`) was missed. Not exploitable —
`pointer-events: none` and the `onKeyDownCapture` handler both cover it — so it
renders as a live-looking link that silently does nothing.
- **`programmatic` mark missed a writer.** `40e5fb7b26` marked six sites;
`explore/components/Control.tsx:88` calls `setControlValue?.(name,
props.default)` inside an effect (the reset-on-hide path) with no mark.
Changing control A hides control B, and B's reset is attributed to the user as
*"Changed 'B'"*. Worth noting the mark is opt-out (`exploreActions.ts:116`
defaults `programmatic` to `false`), so a future `setControlValue` inside an
effect regresses this silently with nothing failing CI. Plugin writes route
through `ExploreViewContainer/index.tsx:994-998`, which passes no options
object, so they can't opt in at all today.
Also in this family, and the reason the last one matters: the chart restore
gate uses the session log as its dirty signal (`useVersionActions.tsx:98-102`),
which is wrong in both directions. **False negative** — renaming a chart
dispatches `UPDATE_CHART_TITLE`, not `SET_FIELD_VALUE`, so it never reaches the
middleware; restore proceeds and `hydrateExplore` resets `sliceName` from the
server, discarding the rename with no warning. **False positive** — the log is
append-only and never diffed against the original, so `row_limit` 100 → 200 →
100 leaves an entry, restore is refused with *"Save or discard your unsaved
changes…"*, and Explore has no discard affordance, so a page reload is the only
way out.
---
## 4. Smaller confirmed items
- **A `?edit=true` deep link can survive a preview round-trip into edit
mode.** Entry correctly forces `editMode: false` and explains why
(`useDashboardVersionPreview.ts:349-354`), but the exit path (`:385-389`) and
the post-restore path (`:275`) pass no override and fall through to the
URL-derived value at `hydrate.ts:126`. Arrive at `?edit=true`, leave edit mode
by saving (the URL keeps the param — only `discardChanges` strips it), preview,
then **Close preview** → the dashboard silently enters edit mode.
`hasUnsavedChanges` is reset by the hydrate so nothing is written without
further action.
- **Timeline re-renders on every keystroke.** `searchTerm` is controlled
state in the container; only the *fetch* is debounced, not the render. None of
`SaveGroupItem`, `ActionRow`, `RelatedUpdateRow`, `CurrentVersionSection` or
`VersionHistoryPanel` is memoized, and there's no virtualization. Per row per
keystroke: a dayjs UTC parse + `.local()` + `.format()`, `describeRecord`, and
emotion re-serialization of ~8 prop-dependent `styled` components. `loadMore`
chases up to `MAX_CHAINED_PAGES = 8` × `PAGE_SIZE = 25` per click with no
ceiling across clicks, so this grows without bound. `memo` on the four row
components plus lifting the search input would cover it.
- **A `loadMore` click can drop the "Current" probe.** The `include=self`
probe from `3b7a1fb944` is guarded on the same `fetchIdRef` that `loadMore`
bumps (`useVersionActivity.ts:143`, `:186`). A click landing between the page-0
response and the probe's response discards it permanently, so `newestGroup`
stays null and the newest save renders without its Current tag and with
preview/restore affordances. Restoring it is a server no-op
(`useVersionActions.tsx:173` toasts *"Already at this version"*), so this is a
labeling defect on a narrow window — a separate counter for the probe would
close it.
- **Wire types drift from the server in three places:** `ActivityOperation`
omits `"update"` (emitted when a field exceeds the record cap);
`ActivityRecord.path` is required but deleted related records are deliberately
redacted to `null`; `ChartVersionSnapshot.query_context` is required but
`Slice.__versioned__` excludes it and the chart version table has no such
column. All three are currently tolerated at runtime, but they're what let the
impossible test fixtures through.
- **i18n:** `describeRecord` assembles sentences from translated fragments —
`t('Added a %s', t('chart'))` at `display.ts:272-289` — which
`display.ts:394-397` itself argues against. Unfixable for languages that
inflect the noun or need article agreement, and it's the largest translatable
surface in the panel. Dates are hardcoded US formats (`display.ts:35`, `:44`)
on every row; dayjs `LLL`/`l LT` would be a drop-in.
- **`RelatedUpdateRow.tsx:151-154`** locates the link by
`headline.lastIndexOf(record.entity_name)`. When `headline` is a
server-provided summary, nothing guarantees the name appears at the intended
position — for a one-character entity name it will match an arbitrary character
and split the sentence around it.
- **The `include` filter persists across entities.**
`SET_VERSION_HISTORY_INCLUDE` is never reset on close or navigation. There's a
test asserting the persistence (`reducer.test.ts:77`), so this may be
deliberate — it reads as a bug from the user's side.
---
## 5. Two things that ship with the flag off
Flagging these only because they reach deployments that never opted in. Both
look benign; neither appears to have been visually verified.
- `DashboardWrapper.tsx:34` changes `grid-template-columns` from `auto 1fr`
to `auto 1fr auto` for **every** dashboard. The third column collapses to 0
when empty, so I'd expect no visual change.
- Four new wrapper `<div>`s ship unconditionally (two around the filter bars
in `DashboardBuilder.tsx`, two around
`DataSourcePanel`/`ControlPanelsContainer` in `ExploreViewContainer`), several
carrying `height: 100%` inside sticky/flex containers. The `inert` and
`pointer-events` attributes on them are conditional; the elements are not.
A screenshot pass with the flag off would close both.
---
## 6. Architectural note
Offering this as context for the roadmap rather than as an ask on this PR —
the feature works and is contained.
The PR implements preview twice, differently. Chart preview is a **detached
render**: 245 lines, local state, never writes the global store. Dashboard
preview **hydrates the snapshot into the live global store** and unwinds it:
404 lines, 7 refs, three interleaved state machines. Nearly every expensive
mechanism in the feature exists only for the dashboard half — the `liveDataRef`
cache of the whole payload and its three invalidation effects, the save-signal
listener, the `dataMask` save/replay, the `editMode` override on
`hydrateDashboard`, the store-level `isPreviewApplying` flag, and the gate
wrappers.
The consequence worth naming: because the *live* store holds historical
data, those gates are **safety-critical rather than cosmetic**. On the chart
side the equivalent code (`ExploreViewContainer/index.tsx:1109-1125`,
`1179-1198`) is pure visual dimming, because the store was never poisoned. The
four near-duplicate preview-gate wrappers noted in the PR body aren't cosmetic
duplication — they're the failure-mode surface of the mechanism. The unwind is
also lossy by construction: `hydrateWith` passes `activeTabs: null,
chartStates: null`, and the undo stack is simply gone.
An alternative the codebase already supports: a **nested Redux store**.
`setupStore()` (`views/store.ts:175-195`) is already parameterized on
`initialState` and already runs a second instance in production
(`views/menu.tsx:39`), and no component in `src/dashboard/` imports the
singleton — they all read the default react-redux context. A `<Provider
store={previewStore}>` around `DashboardContainer` would redirect the whole
tree. Preview becomes "mount a store seeded with the snapshot," exit becomes
"unmount," and the cache, the listener, the staleness check, the dataMask
replay, the `editMode` override and `isPreviewApplying` all disappear — along
with the possibility of a save writing historical layout to the live entity,
since there's no live store in scope.
The reason to weigh this *before* datasets rather than after: the current
shape doesn't extend. A dataset preview needs a third variant, and each new
entity kind multiplies the gate surface.
---
## Checked and clean
Stated explicitly so the next round doesn't re-spend budget on them:
- **`0dc70e639c`** — `VERSION_RESTORED` is properly scoped; all three
consumers compare uuid before reacting (`useDashboardVersionPreview.ts:251`,
`DashboardVersionHistory.tsx:150`, `ExploreVersionHistory.tsx:186`).
- **`aa76d865b3`** — the dashboard unmount guard is real, and
`EXPLORE_REHYDRATION_CONCURRENCY = 6` is applied. The other `Promise.all`s are
bounded too.
- **`2fbfab3695`** — effect ordering is correct: the save effect is declared
before the apply effect, so `saveSignalAtStart` reads an already-synced ref.
- **`dc473b6d2a`** — `recordKey` includes `from_value`/`to_value`.
- **`e33ba0cc27`** — the failure path *is* covered; only the re-selection
path above isn't.
- **`6f37cf732f` / `075241a2f7`** — the single-funnel claim holds:
`restoreVersion` has exactly one caller, reachable only through
`requestRestore` → `RestoreConfirmModal`, and all three UI entry points go
through it.
- **`87c98e8e13`** — `git diff <merge-base>..HEAD -- package-lock.json` is
empty; the lockfile is byte-identical to master.
- **`truncated`**, pagination, `include` and search semantics all match the
server parser.
- **Preview writes** — every write path during a dashboard preview was
enumerated (header Save/Edit/Share/SaveAs, kebab, inline title, both filter
bars, the grid, both `setEditMode(true)` sites). *"Preview never writes"*
holds, modulo the `?edit=true` exit path above.
- **Explore preview gates are symmetric by construction** — all seven are
pure functions of the selector read fresh each render, with no state latch, so
none can be left disabled after exit.
- **No authorization bypass and no XSS.** Read endpoints call resource-typed
`raise_for_access`; restore is route-mapped to `write` and separately calls
`raise_for_editorship`. The feature renders server strings as React text
children with no `dangerouslySetInnerHTML`.
- **The new Playwright is not dead code.** `activity-log.spec.ts` is under
the required tree, `playwright.config.ts` includes it in Chromium, and the
required workflow runs with
`SUPERSET_CONFIG=tests.integration_tests.superset_test_config`, which enables
both `VERSION_HISTORY` and capture.
- 274 existing Jest tests across the feature and the touched
explore/dashboard suites pass.
Two notes on scope: `?version_history=true` gating history on *restore*
permission makes the UI strictly narrower than the read API (`can_read` +
`raise_for_access`), which the code comment indicates is deliberate — flagging
it as a product decision worth stating, not a defect. And
`is_managed_externally` is enforced client-side only; the restore command
doesn't check it, so an authorized editor can call the endpoint directly.
That's a backend follow-up against already-merged code, not an ask here, but
`version-history.mdx:81-83` states it unconditionally.
Separately, and **not caused by this PR** (`exploreReducer.ts` isn't in the
diff): `ChartGetResponseSchema.editors` is a `SubjectResponseSchema`, i.e.
`{id, label, …}` with no `value` field, but `exploreReducer.ts:618-619`
normalizes with `typeof editor === 'number' ? editor : editor.value`. After the
Properties modal saves and refetches, that writes `undefined` ids into
`state.explore.slice.editors`. Worth its own issue.
What this review did **not** do: no manual walkthrough of the UI, no
Playwright run, no profiling of the panel.
---
<details>
<summary><b>Regression test for finding 1</b> — passes on current head,
which is the bug</summary>
Drop in as `src/features/versionHistory/previewReselect.test.tsx`. After the
fix, the final assertion should become `toBe(false)`.
```tsx
import { act } from 'react';
import { createStore, render, waitFor } from 'spec/helpers/testing-library';
import reducerIndex from 'spec/helpers/reducerIndex';
import { getChartDataRequest } from 'src/components/Chart/chartAction';
import type { VersionHistoryState } from './types';
import { setVersionPreview, type VersionHistoryRootState } from './reducer';
import { fetchDatasourceMetadata, fetchVersionSnapshot } from './api';
import ChartVersionPreview from './ChartVersionPreview';
jest.mock('./api', () => ({
fetchDatasourceMetadata: jest.fn(),
fetchVersionSnapshot: jest.fn(),
}));
jest.mock('./PreviewBanner', () => ({ __esModule: true, default: () => null
}));
jest.mock('src/components/Chart/chartAction', () => ({
getChartDataRequest: jest.fn(),
handleChartDataResponse: jest.fn(),
}));
jest.mock('src/explore/exploreUtils', () => ({
...jest.requireActual('src/explore/exploreUtils'),
getQuerySettings: () => [false],
}));
jest.mock('@superset-ui/core', () => ({
...jest.requireActual('@superset-ui/core'),
SuperChart: () => <div data-test="super-chart" />,
}));
const { handleChartDataResponse } = jest.requireMock(
'src/components/Chart/chartAction',
);
const PREVIEW = {
entityUuid: 'chart-uuid',
versionUuid: 'version-uuid',
transactionId: 1,
headline: 'A save',
issuedAt: '2026-07-01T00:00:00Z',
};
const previewState = (): VersionHistoryState => ({
isPanelOpen: true,
entityType: 'chart',
include: 'all',
isPreviewApplying: true,
preview: PREVIEW,
sessionLog: [],
restoreCount: 0,
lastRestoredEntityUuid: null,
});
beforeEach(() => {
jest.clearAllMocks();
(fetchVersionSnapshot as jest.Mock).mockResolvedValue({
params: '{"metric":"count"}',
viz_type: 'table',
datasource_id: 5,
datasource_type: 'table',
});
(fetchDatasourceMetadata as jest.Mock).mockResolvedValue({});
(getChartDataRequest as jest.Mock).mockResolvedValue({ response: {}, json:
{} });
(handleChartDataResponse as jest.Mock).mockResolvedValue([{ data: [] }]);
});
test('re-selecting the already-previewed version wedges isPreviewApplying',
async () => {
const store = createStore(
{
versionHistory: previewState(),
explore: {
datasource: { id: 5, type: 'table', columns: [], metrics: [] },
slice: { editors: [{ id: 1 }] },
},
user: { userId: 1, roles: {} },
},
reducerIndex,
);
render(<ChartVersionPreview />, { useTheme: true, store });
const applying = () =>
(store.getState() as unknown as VersionHistoryRootState).versionHistory
.isPreviewApplying;
await waitFor(() => expect(applying()).toBe(false)); // first apply settles
// The user clicks a change row inside the group they are already
previewing:
// ActionRow.tsx:212 -> SaveGroupItem.tsx:265 ->
ExploreVersionHistory.tsx:244.
act(() => {
store.dispatch(setVersionPreview({ ...PREVIEW }));
});
expect(applying()).toBe(true); // reducer.ts:186 re-enters unconditionally
// The effect deps ([dispatch, entityUuid, versionUuid]) are unchanged, so
it
// never re-runs and nothing dispatches versionPreviewApplied(). Flush to
show
// it is wedged, not slow.
await act(async () => {
await new Promise(resolve => {
setTimeout(resolve, 50);
});
});
expect(applying()).toBe(true); // BUG: stuck forever; Restore never
returns.
});
```
</details>
--
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]
