feat(versioning): version-history UI - #41551
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
23a1f1e to
f704fee
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41551 +/- ##
==========================================
+ Coverage 65.57% 65.77% +0.19%
==========================================
Files 2818 2840 +22
Lines 160023 161762 +1739
Branches 36556 37079 +523
==========================================
+ Hits 104940 106400 +1460
- Misses 53038 53313 +275
- Partials 2045 2049 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f704fee to
6621e18
Compare
The post-restore rehydration fetch dispatched hydrateExplore with no cancellation: resolving after the user navigated away — or after a save-as swapped the slice in place — it would rewrite the explore store with the old chart's payload over the newly loaded chart. The effect now cancels its in-flight fetch on cleanup, same pattern as the uuid fetch above it. Test confirmed to fail without the cancellation. Raised by codeant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…matic Swapping a chart's datasource derives a new granularity_sqla as a side effect; the session log recorded it as a user edit of the time column. Same class as the axis-margin marks: the user's gesture was the swap, not a control change. Raised by codeant's sweep for unmarked programmatic writes; the other call sites were re-audited and remain user-initiated (dialog confirms, clear-form click, temporal-filter reset, annotation edits, axis-title drag edits). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SET_VERSION_PREVIEW re-entered the applying state unconditionally, but the appliers' effects key on versionUuid and never re-run for an identical value — so clicking a change row inside the group already being previewed (every row is clickable) wedged isPreviewApplying permanently: the banner reported "Loading historical version" over a fully applied snapshot and Restore was withheld until the preview was exited. The dashboard hook had a second route to the same wedge via its appliedVersionRef early-return. Guarding at the reducer closes both call sites at once. Found by kgabryje in review, with the regression test contributed there; its assertions are flipped to pin the fixed behaviour and it fails against the pre-fix reducer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both found by kgabryje in review. A chart rename travels through UPDATE_CHART_TITLE, not SET_FIELD_VALUE, so it never reached the session log: the restore gate saw a clean page and a restore silently discarded the rename. The middleware now logs renames, with the inlined constant pinned against the real explore module like its siblings. And Control's reset-on-hide effect dispatched an unmarked setControlValue: changing control A hides control B, and B's reset was attributed to the user as an edit of B — a phantom entry that would also spuriously block restores. Marked programmatic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four smaller confirmed items from the same review: - A stale `?edit=true` in the URL no longer flips the dashboard into edit mode as a side effect of closing a preview or reloading after a restore: both exit-path hydrations now pass an explicit editMode override, matching the entry path that already did. - The newest-self probe gets its own staleness counter: loadMore bumps the shared fetch id, and a click landing between a reset's page-0 response and its probe's response discarded the probe permanently, leaving the newest save untagged. - The empty-tab call to action into edit mode is withheld during a version preview, as DashboardBuilder's empty state already is — it rendered as a live-looking link the grid gate silently swallowed. - ActivityOperation gains the server's documented 'update' verb, so narrowing on the union accounts for collapsed-record summaries. Also documents preview fidelity honestly: charts render with current definitions and non-layout properties show live values; restore applies the full captured state. The docs page previously overclaimed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All raised by codeant against the previous batch — fair catches on fresh code: - The dashboard preview's unmount cleanup now invalidates the fetch id, so an apply() settling after navigation cannot dispatch completion into the global slice (or, on its failure path, clear a preview the next page just requested). The unmount test now pins the slice too. - The newest-self probe is invalidated when fetchPage starts rather than when the probe launches: switching entities immediately supersedes the old entity's in-flight probe, which could otherwise land after the switch and write the previous entity's newest save into the new page's Current marker. - A pending restore confirmation is dropped when the page's entity changes underneath it, rather than surviving to combine the new uuid with the old version uuid (the server would refuse the mismatch, but the user would see a confusing failure). The two race tests were confirmed to fail without their guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he effect 308f5cb cancelled the post-restore rehydration from the effect's own cleanup. That cleanup runs on every re-run, and the effect depends on `refreshActivity` -> `fetchPage` -> the debounced search term, so typing in the panel's search box during the rehydration round trip cancelled the fetch — and nothing re-issued it, because `lastRestoreCountRef` was already synced. The timeline showed the restore while the chart still rendered its pre-restore state, with the failure toast suppressed too. Replace it with a dedicated invalidation token bumped only by unmount, a slice/uuid swap, or a newer restore — the same pattern the newest-self probe and the dashboard preview's unmount cleanup already use. The regression test fails against the pre-fix source. Found by kgabryje's round-2 review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d704f6a marked the derived `granularity_sqla` write programmatic. That is right for a dataset *swap*, where ChangeDatasourceModal's `onChange` emits a recorded control change of its own. Edit Dataset has no such change: it dispatches `SET_DATASOURCE` + `UPDATE_FORM_DATA_BY_DATASOURCE`, neither of which the session-log middleware recorded. So editing dataset metadata that invalidated the chart's time column rewrote `granularity_sqla` and left the log empty — a restore then discarded the reconciled value with no warning. Before that commit the path blocked restore. Record `UPDATE_FORM_DATA_BY_DATASOURCE` instead. Both routes reconcile the chart's form data against the new columns, and both report under `controlName: 'datasource'`, so the swap's two entries collapse into one. The derived write stays programmatic. Also mark AnnotationLayerControl's validation-sync write, which re-publishes the same value with fresh errors after an annotation query resolves: a failing query was manufacturing a phantom edit on an untouched chart. Found by kgabryje's round-2 review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…board The earlier refutation was wrong: `dashboards/api.py:613` sets `result["extra_editors"]` after `schema.dump(dash)`, so the `columns` projection never strips it, and it is gated on `EXTRA_EDITORS_RESOLVER` — present in exactly the deployment where the omission bites. `useDashboard` and `hydrate.ts` both spread the payload wholesale, so the field is already in scope where `canUserEditDashboard` runs. Widen the shared predicate to `editors` union `extra_editors`, matching the server's `is_editor`. Because `dash_edit_perm` is this function's result, that corrects the Edit button, the version-history menu entry and the dashboard restore gate together. `canOverwriteSlice` had the same normalisation inline; it now reuses the shared helper. Note: this is NOT behind VERSION_HISTORY. It changes the Edit-button gate for any deployment with a resolver configured — a widening toward what the API already permits, not a narrowing, but called out deliberately. Found by kgabryje's round-2 review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The snapshot endpoint has always projected the full scalar set (`versioning/queries.py:491-501`); the preview applied only title, css, metadata and layout, leaving the live certification badge, draft/published pill, description and slug sitting over historical content. Only the *membership* half of preview fidelity needs a backend contract change. Theme is the one scalar needing resolution — the version table stores `theme_id`, hydration wants the object — so resolve it with a single lookup, skipped when it matches the live theme, and falling back to the live theme rather than dropping the preview if the lookup fails. Also corrects the exit-preview comment: the entry gate tests for unsaved changes, not for edit mode, so "previews can only be entered from view mode" was never the invariant. Forcing view mode on exit is still right. Found by kgabryje's round-2 review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ds it Every version-table column is nullable — the tables mirror `slices` and `dashboards`, whose columns are nullable, and a row written for a delete carries nulls throughout. The client typed `slice_name`, `viz_type`, `datasource_id`, `datasource_type`, `uuid` and `dashboard_title` non-null, so the compiler could not see the cases below. Model the whole shape rather than the two examples named earlier, and handle what that exposes: the chart preview explains a version that records no viz type or dataset instead of requesting `undefined__undefined`; `createChartFromSnapshot` fails with an actionable message instead of posting a payload the API rejects on validation; "open as new" falls back to an untitled name. No runtime behaviour changes for well-formed snapshots. Docs: "Open as new" no longer claims to copy "the selected version" without qualification. A new chart is built from the version, but a new dashboard is a copy of the live dashboard with only the version's title, CSS, metadata and layout applied — DashboardCopySchema accepts nothing else. The preview fidelity caveat is updated for the scalars the preview now applies. Found by kgabryje's round-2 review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The theme lookup added in 34764a2 awaited between committing liveDataRef and setting appliedVersionRef. That stretch has to stay synchronous: the save-signal effect nulls the cached live copy whenever no preview is applied yet, so a save landing inside the await left a preview applied over a null cache. Exit-preview only rehydrates `if (liveData)`, so closing the preview did nothing — the page kept rendering the historical snapshot after the banner disappeared, presenting it as live, until a reload. Reachable whenever the previewed version's theme_id differs from the live theme (so the lookup actually fetches) and a save lands during that round trip. Resolve the theme with the other pre-check awaits instead. The regression test fails against the previous placement (hydrate called once, exit never rehydrates) and passes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Explore pushes its chart state into history entries, so a back/forward between them is an undo/redo: ExploreViewContainer dispatches setExploreControls, and the reducer rebuilds the entire control map without emitting a single control change. The session-log middleware recorded only SET_FIELD_VALUE, UPDATE_CHART_TITLE and UPDATE_FORM_DATA_BY_DATASOURCE, so that whole rewrite was invisible to the restore dirty gate. Reachable sequence: edit a chart, save (which hydrates and clears the log), press Back to the pre-save controls, then restore a version. The gate saw an empty log while the form had moved, so the restore proceeded and discarded the popped state with no warning — the same fail-open direction as the Edit Dataset gap, one writer over. The trade is deliberate and worth stating: recording this means stepping back to a state that matches the saved chart also latches an entry, and the gate is a hard block whose only discard affordance in Explore is a page reload. That is the append-only log's known failure direction, and it fails closed where the missing entry failed open. Baseline diffing replaces both. Note the constant: explore names it SET_EXPLORE_CONTROLS but its literal is 'UPDATE_EXPLORE_CONTROLS'; the pinning test now covers it. All three new assertions fail against the middleware without this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Panel of clean-code / tidy-first / continuous-delivery / react lenses over
the full branch; the confirmed, small-fix findings land here, the
structural and known-deferred ones stay on the follow-up list.
- openRelatedEntity called window.open after awaiting the id lookup — the
silent Safari popup-block failure navigationUtils' own docstring
documents, and which openAsNew already avoids. It now claims the tab
synchronously and navigates or closes it after the resolve. (Found
independently by two lenses.)
- The restore/fork in-flight locks were per-hook-instance refs, but the
preview banner and the history panel each mount their own
useVersionActions for the same entity — the banner's "Open as new" plus
the panel kebab's mid-flight forked two copies. The lock is now a
module-scope set keyed restore:{entity}:{uuid} (entity-wide: concurrent
restores to different versions would race each other's rehydration) and
fork:{entity}:{uuid}:{version} (per-version: forking two different
versions concurrently is legitimate). Control run: the new two-instance
test forks twice against the old code.
- The ?version_history=true effects re-ran on every canRestore move and
re-read the persistent URL param, so a late false→true flip (info
refetch after a properties save) re-opened a panel the user had closed.
Both the dashboard and explore sides now honour the param once per
mount.
- Save groups whose versionUuid the server did not provide still offered
Restore/Open-as-new kebabs whose clicks silently no-op'd in the
container guards. The group kebab and the expanded action-row kebabs
are now hidden for such groups.
Also tags the restore toast probe with the TODO(version-history) marker
its sibling workarounds carry, naming the backend contract fix
(created: boolean on the restore response) that removes it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test added in 266f676 was committed without a prettier pass; CI's pre-commit lane caught it. No behavioural change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`clearVersionPreview()` took no argument and cleared whatever preview the global slice held, while `versionRestored(uuid)` dispatched on the very next line carries the uuid precisely so consumers can check identity. So a restore of entity A settling after the user had navigated to entity B and opened a preview there silently cleared B's preview. The guard goes in the reducer rather than at the call site: the action now requires an entity uuid and the reducer no-ops when it doesn't match the preview in flight, so all five dispatchers — including any future one — inherit it. A required (not optional) parameter is the point: it makes an asynchronous dispatcher decide whose preview it is. Reported by @rusackas from codeant's thread on apache#41551. The two new reducer tests fail against the unscoped action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ame type Post-rebase housekeeping for two things that landed on master: - apache#42434 migrated the repo from Prettier to Oxfmt. Only three of this branch's files needed reformatting (the rest are new files Oxfmt had never seen); the changes are wrapping-only. - apache#42711 removed `username` from the version payload's `changed_by`, so `VersionChangedBy` — which existed solely to add that field to `ActivityChangedBy` and had no other consumer — is deleted. `VersionMeta` now uses `ActivityChangedBy` directly, which is what the server-side schema-parity test enforces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Generated by Claude (AI) on behalf of @mikebridge. Rebased onto master — head is now 73 commits replayed over 79 of master's. What it absorbed:
Verification after the rebase: 168/168 in the feature suite, 267/267 across the affected areas, oxfmt/oxlint/stylelint/custom-rules all clean. One note for anyone type-checking locally: |
- `createDashboardFromSnapshot` only set `metadata.positions` when the snapshot had a `position_json`. The copy endpoint rebuilds the new dashboard's layout and chart associations from that key, so forking a version whose layout was empty produced a copy carrying the *source's current* charts — today's dashboard under a historical name. It now always sends `positions`, empty when the snapshot had none. The existing empty-layout test asserted the buggy shape and now pins the fix. - The explore restore-rehydration was invalidated by unmount, a slice/uuid swap and a newer restore, but not by a save. A save committing while the fetch was in flight left the payload in hand older than the store, and hydrating it rolled the chart back over the newer save — the guard its dashboard sibling already had via `saveSignalAtStart`. Both new tests fail against the previous code. Refuted in the same round, with evidence on the threads: the `can_write` asymmetry in `canOverwriteSlice` (the shipped master predicate never checked it either — pre-existing chart/dashboard divergence, not introduced here) and the entity-level scope of `clearVersionPreview` (a restore rehydrates the whole page, so every preview of that entity must exit — version-level scoping would strand a banner over live content). The browser-history session-log entry is the deliberate fail-closed trade documented in 9197772, tracked as sc-115766. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rusackas
left a comment
There was a problem hiding this comment.
As flagged, clearVersionPreview's scoping race is fixed now. LGTM!
SUMMARY
The frontend for entity version history: timeline rendering, server-side search, version preview, open-as-new, and restore — roughly 7,700 lines including Playwright e2e coverage.
Ships behind the
VERSION_HISTORYfeature flag (default off). With the flag on, an Explore chart's actions menu gains View version history, opening a panel that lists each captured version with its author, timestamp, and a human-readable summary of what changed, with search and an all-changes filter. Restoring a version rewrites the entity to that state and is itself recorded as a new version, so history stays append-only.All backend prerequisites are now merged, so this branch carries only the UI work plus the
VERSION_HISTORYflag declaration inconfig.pyand the generated feature-flag docs:POST /api/v1/{chart,dashboard}/<uuid>/versions/<version_uuid>/restoreroute this PR's Restore action callsReviewers should look at this deliberately, because it affects every deployment on merge.
The Save-modal overwrite gate moved into a shared
canOverwriteSlicehelper, so the modal, the history menu, the panel's URL-param gate and the preview's restore control stop disagreeing — they previously had four subtly different rules. The substantive change is that the gate now also honoursextra_editors.The server authorizes overwrite through
UpdateChartCommand→raise_for_editorship→is_editor, which resolves to admin OR (the user's subjects ∩ (editors∪extra_editors)). Master's client-side gate ignoredextra_editorsentirely, so a user granted editorship through a deployment'sEXTRA_EDITORS_RESOLVERsaw Save (Overwrite) disabled on a chart the API would have let them overwrite. That is the case this fixes, and it is a widening: nobody loses an affordance.The same extraction also drops the chart-
ownersbranch from #41352. That is not a behaviour change:ownerswas deleted from theSlicemodel by #38831 five days before #41352 landed, soslice.ownersis never populated and that branch has always evaluated tofalse. It type-checked becauseSaveModaltypes the prop asRecord<string, any>, and its test passed only because the test injectedownersinto a mock store. This was reported as a blocker in review and withdrawn once the model history was checked — details in the correction comment below.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: no version history surface exists — the actions menu has no entry, and edits are unrecoverable once saved over.
After (captured on a live stack with
VERSION_HISTORY+ENABLE_VERSIONING_CAPTUREon):The panel, opened from Explore's … → View version history: each save with author, timestamp, and descriptive change rows; the live version tagged Current.
Previewing an older version: the banner marks the page historical, the chart shows the version's state, and Restore / Open as new are offered once the snapshot is on screen.
The restore confirmation:
After restoring: the chart reverts, and the restore itself heads the timeline as the new Current version — history stays append-only.
TESTING INSTRUCTIONS
Enable the flag and capture, then exercise the panel:
baselineentry plus one row per edit, each showing the author and a summary of the change.Worth exercising specifically, since these were review findings fixed here:
?edit=truein the URL does not leave it available.?version_history=truedoes not offer Restore (the menu already hid it; the panel did not).Automated coverage: unit/RTL tests plus Playwright e2e, all included in this PR.
ADDITIONAL INFORMATION
design:proposal). This PR is UI only and ships behindVERSION_HISTORY, which defaults toFalse.VERSION_HISTORY(plusENABLE_VERSIONING_CAPTUREfor versions to be recorded)Review findings addressed on this branch (from the two review panels posted above)
Each fix has a control run behind it — the new test was confirmed to fail against the pre-fix source and pass with it — except the i18n plural, which no English assertion can distinguish (noted in that commit).
5468199965canOverwriteSliceowner change above, with its test rewritten (the old one passed via the fixture'seditors: [{id: 1}]and would have stayed green either way). Dashboard restore gate now honoursis_managed_externally, via oneselectCanRestoreDashboardshared by the panel and the banner.userCanSaveAsgated on preview; preview hydration forceseditMode: falserather than re-deriving it from theeditURL param.fa637138636f1771cff9bf7149696b36fa6444cfChartVersionPreview, which had none.e9474c0c44tn()for the shared-dataset plural; one message instead of two assembled fragments.2c275d34d0DashboardContentWrapper, so the grid gate did not reach it — previewing a version whose layout is empty still offered a way into edit mode over the snapshot. Gated like the Header'suserCanEdit; the empty-state message itself stays, since it describes what is on screen.87c98e8e13@noble/hasheslockfile entries added by11bce141bbunder a@braintree/sanitize-urltitle; neither is on master, and a plainnpm installremoves both.One finding was withdrawn rather than fixed: the
canOverwriteSlicechange was reported as reverting #41352's chart-owner branch, and it does not.ownerswas deleted fromSliceby #38831 five days before #41352 landed, so that branch has never been reachable — the helper drops dead code and addsextra_editors, which makes the client gate match the server'sis_editorfor the first time. Evidence is in the correction comment above.|
aa76d865b3| A preview request still in flight when the user navigated away hydrated a dashboard into the global store over whatever page mounted next — the fetch-id guard only covered requests superseded by another preview, not unmount. Also bounded the/api/v1/explore/lookups for charts missing from a snapshot, which ran unbatched throughPromise.all(observed 20 concurrent against a cap of 6). ||
92eea36e67|canRestoredefaulted totruein bothPreviewBannerandVersionHistoryPanel. Every call site passes it, so no behaviour changes — but a permission term should fail closed for the next one. ||
dc473b6d2a|recordKeyomitted the record values, so two genuinely distinct changes sharing kind/operation/path within one save collapsed on page merge and the timeline under-reported it.SaveGroupItemalready documented that this collision occurs. ||
68ade085a8| The activity-log e2e renamed the three most recently changed charts, making it order-dependent, unsafe on parallel workers, and leaving append-only version records behind that its revert could not remove. It now creates and deletes its own chart, and no longer requiresbirth_namesto be loaded. Verified against a live stack: passes repeatedly and leaves nothing behind. ||
e33ba0cc27| Chart previews never left the "applying" state — only the dashboard hook dispatchedversionPreviewApplied, so a chart preview's banner read "Loading historical version" forever and Restore never appeared. The chart path now announces when its load settles (on failure too — the error alert replaces the chart, and a version whose data can't render is still a valid restore target). Tests previously masked this by hand-craftingisPreviewApplying: false; they now start from the real post-dispatch state. ||
40e5fb7b26| The session log recorded programmatic control writes (transferred-control cleanup after load, derived axis margins, the tooltip_template rewrite), so an untouched chart could list "unsaved changes" the user never made.setControlValuegains an explicitprogrammaticmark; the six effect call sites set it and the middleware skips them, with the mark pinned across modules by a test built on the real action creator. ||
6f37cf732f| Restore now has the same unsaved-changes gate preview already had — a panel-kebab restore previously rehydrated the page and silently wiped in-progress edits and undo history. The gate lives inrequestRestore, which every entry point funnels through; dashboards usehasUnsavedChanges, explore uses the session log (trustworthy after the fix above). ||
def8bbb7a7| The server'struncatedresponse field ("count is a floor") was dropped at the frontend boundary, so a clipped history paged to the clamped end and read as the beginning of time. The activity hook now surfaces it and the panel shows a terminal notice. ||
a1b03953a5| Timeline meta text usedcolorTextQuaternary(disabled-level, ~1.9:1) for informative content; nowcolorTextTertiary, the codebase convention (WCAG 1.4.3). ||
0dc70e639c|VERSION_RESTOREDwas an unscoped broadcast: a restore confirmed on entity A that resolved after navigating to entity B made B rehydrate — clearing B's filters for someone else's restore. The action now carries the restored entity's uuid and all three consumers ignore foreign bumps. ||
2fbfab3695| A save resolving while a preview apply was in flight let the apply cache a pre-save copy of the dashboard — exit-preview could then resurrect pre-save state. The apply now detects save-signal movement and refetches before caching. (Raised by codeant; fixing it also surfaced a test-harness bug — an unstableuseToastsmock re-ran the apply on every store update, masking staleness bugs.) ||
3b7a1fb944| The "Current" marker derived from the first visible activity page, which is search-filtered, include-scoped, and can be filled by newer related records — so it could freeze on a stale save after a filtered restore, or drop entirely.newestGroupnow comes from a dedicated one-recordinclude=selfprobe. (Two codeant findings + a deferred capstone item, one root cause.) ||
075241a2f7| The unsaved-changes gate ran only when the restore modal opened; work turning dirty while it sat open was still wiped on confirm. Re-validated at the moment of mutation. (Codex capstone, react lens.) |A 10-lens capstone review of the versioning feature as a whole (merged backend + this branch) is posted above; the seven fixes ending at
0dc70e639cclose every frontend High it raised. Its backend findings are follow-ups against merged code, not asks on this PR.Known remaining, not addressed here (all structural/polish): four preview-gate wrappers could be one component; the two page containers share ~120 duplicated lines that want a common hook;
describeRecordis a long conditional dispatch; timeline rows are unmemoized behind the search input; docs page added in50eb210534.