mikebridge commented on PR #41551: URL: https://github.com/apache/superset/pull/41551#issuecomment-5132150291
### Addendum — third lens A committer-perspective lens finished after the review above. Most of it converged on what is already posted; below is only what was **new**, plus one item that now has a third independent confirmation. Everything here I checked against the code on `0d2e568fc6` rather than taking the lens at its word. **Item 1 above is now three-for-three.** The committer lens reached `canOverwriteSlice` independently and rated it a merge blocker, on the same reasoning — the owner branch from #41352 is dropped, it is the one change outside `VERSION_HISTORY`, and `SaveModal.test.tsx:361` passes via the `editors` path so CI cannot see it. Setting that fixture to `editors: []` is the one-line change that makes the test mean something again. #### New — High **A. Preview does not force edit mode off, and the Save toolbar is not gated on it** `useDashboardVersionPreview.ts:272,305` · `Header/index.tsx:555` · `DashboardBuilder.tsx:785,856` `hydrateWith` passes no `editMode`, and `hydrate.ts:118,399` derives it as `canEdit && reservedUrlParams.edit === 'true'`. Both branches misbehave: - **With `?edit=true`**: edit mode survives into preview. `Header/index.tsx:549-554` adds `&& !isVersionPreviewActive` to `userCanEdit` *and* to the share permission, but `userCanSaveAs` on line 555 is bare `!!dashboardInfo.dash_save_perm` — so the Save/Discard toolbar at 679-681 stays live over the snapshot's `position_data`. `VersionHistoryColumn` (DashboardBuilder:856) renders regardless of edit mode, so the two states are reachable together. The empty-state "Edit the dashboard" button at `DashboardBuilder.tsx:785-789` is also outside the gated `DashboardContentWrapper` (the gate starts at 794) and dispatches `setEditMode(true)`. - **Without it**: a user who entered edit mode via the toggle has `editMode` silently flipped to `false` by the preview hydrate, alongside `hasUnsavedChanges: false` — in-progress layout edits vanish with no prompt. The first is a path to saving historical layout over the live dashboard; the second is data loss. Simplest fix is an explicit `editMode: false` in the preview hydrate plus the missing `!isVersionPreviewActive` on `userCanSaveAs`, and refusing to open preview while `hasUnsavedChanges`. **B. No loading state while a dashboard preview is being applied** `useDashboardVersionPreview.ts:189-291` There is no loading, pending or spinner state anywhere in the hook. The banner and every interaction gate engage on activation, but `apply()` then runs three-plus serialized round trips (dashboard, charts, snapshot, plus one `/api/v1/explore/` per missing chart). Until they resolve the user is looking at the **live** dashboard labelled "Previewing historical version", with no way to tell it apart from a preview that has finished applying. On a slow instance that is a correctness hazard, not polish. **C. The SIP question, concretely** The open process item I raised above has a specific answer: [SIP-210 — Entity Version History for Dashboards, Charts, and Datasets](https://github.com/apache/superset/issues/39492) exists, is **OPEN** and labelled `design:proposal` with no `[VOTE]` outcome, and is not referenced in this PR body. The mitigating fact is that `VERSION_HISTORY` defaults false and is marked development, so this lands dark. The concrete ask is to link SIP-210 under ADDITIONAL INFORMATION and state that the flag stays default-off until it passes a `dev@` vote — that converts a process blocker into a documented posture. #### New — Medium - **Unbounded fan-out.** One `/api/v1/explore/` request per chart present in the snapshot layout but missing from the live dashboard, with no batching and no concurrency cap — while `api.ts:209-231` (`fetchReachableChartIds`) explicitly batches the same class of lookup at 100. - **The one e2e test mutates shared example charts.** `playwright/tests/version-history/activity-log.spec.ts:120-189` renames the three most-recently-changed charts (`listCharts` orders by `changed_on_delta_humanized`) and grants the admin subject editorship, reverting in a `finally`. That is order-dependent and hostile to parallel workers, and the version records the renames create outlive the revert. Line 157's unscoped `getByText('Current')` is also a strict-mode-violation risk once `CurrentVersionSection` renders next to a group tagged "Current". More to the point: the PR's own testing instructions name preview → restore as the primary flow, and there is no e2e for preview, restore or open-as-new. - **The preview-gate wrapper is copy-pasted four times** (`ExploreViewContainer/index.tsx:1096,1166` · `DashboardBuilder.tsx:587,684`), and inconsistently — the two filter-bar gates apply `inert` with a comment explaining that it blocks keyboard focus, while the dashboard grid gate at 802 uses only `aria-disabled` + `pointer-events: none` and then compensates with the blanket `onKeyDownCapture`. Extracting one `<PreviewGate active>` would resolve the Medium about the gate not disabling descendants at the same time. `Meta` is likewise duplicated verbatim four times and `KebabButton` twice. #### New — Low - `appliedVersionRef.current = null` at `useDashboardVersionPreview.ts:302` runs before the `if (liveData)` guard at 304, so a null `liveDataRef` leaves snapshot content on screen with the banner and all gates gone. - `Slice` (`types/Chart.ts:66-75`) gains `uuid`/`changed_on` but not `extra_editors`, which `canOverwriteSlice.ts:27` reads via its own structural type and the backend emits (`models/slice.py:270`) — the dependency is invisible to the next reader of the canonical type. - The version-history slice is a hand-rolled `switch` reducer with hand-written action constants and creators; `views/store.ts` already uses `configureStore`, so `createSlice` would remove roughly 90 lines. Separately, `[key: string]: unknown` on `ChartVersionSnapshot`/`DashboardVersionSnapshot` (`types.ts:102,114`) makes any property access type-check, so a typo'd field reads as `unknown` instead of erroring. #### One practical note on the merge gate `unit-tests-required`, `cypress-matrix-required`, `playwright-tests-required` and `test-postgres-required` currently report FAILURE. Those are six **zero-duration** jobs from a superseded duplicate workflow trigger — the surviving run's equivalents all pass, which is where the "63 passing" above comes from. Not a code defect, but the merge button will show red until those workflows are re-run. _This addendum 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]
