mikebridge commented on PR #41548:
URL: https://github.com/apache/superset/pull/41548#issuecomment-5359810920
## Review β dashboard Zustand + TanStack Query migration
> π€ This review was generated by Claude (AI) on behalf of @mikebridge, who
directed and verified it.
**Reviewed at HEAD** `af7b90b` (296 files, +15,026/β12,860). Method: four
parallel deep passes (Zustand stores; TanStack Query layer; ReduxβZustand
bridges + hydration; migrated consumers + tests), cross-checked against a
second independent AI review, with every warning below re-verified manually at
the cited lines. All 20 new Jest suites pass locally (162 tests).
Reducer-parity spot checks were run against the deleted Redux code on `master`.
### Summary
This is an unusually faithful, well-engineered port β atomic single-commit
hydration behind a closed render gate, a sound and well-tested discard-snapshot
design, factory-driven query keys with zero inline literals, mutations with
`retry: 0`, and no unstable store selectors anywhere (`useShallow` never
needed). No critical issues found. The warnings concentrate in two areas:
mutation-layer write races and the convention-based lifecycle of the
module-scope singleton stores.
### Warnings
**W1 β Last-writer-wins between the three metadata mutations** *(suggest
fixing before merge)*
`useSaveCrossFiltersSetting.ts:37-52`,
`useSaveFilterBarOrientation.ts:37-42`, and
`useSaveChartConfiguration.ts:47-54` each snapshot `metadata` at `mutationFn`
start and PUT the entire `json_metadata`; the store learns the other mutation's
field only in `onSuccess`. From one menu (`FilterBarSettings/index.tsx:89-91`):
change orientation β toggle cross-filters within one round trip β the second
PUT reverts orientation server-side, and `applyMetadataSaveResult` writes the
regression back into the store, visibly flipping the UI.
**Fix**: shared TanStack mutation `scope: { id: 'dashboard-metadata' }` on
all three so they execute serially. (Re-reading the store just before
serializing is not sufficient β two in-flight mutations still read the same
baseline.)
**W2 β Publish response landing after entering edit mode bakes unsaved edits
into the discard baseline** *(suggest fixing before merge)*
`usePublishDashboard.ts:51` calls `rebaselineHydrationSnapshot`, which
(`rebaselineHydrationDashboardInfo.ts:64-78`) rewrites the whole snapshot β
layout, slices, state seed β from live stores, though only `published` changed.
Toggle publish β click Edit β drag a chart while the PUT is in flight β a later
"Discard" restores the unsaved edits and clears `hasUnsavedChanges`, disarming
the unsaved-changes prompt.
**Fix**: update only the persisted `published` value in the snapshot; don't
rebaseline layout/slices/state. (Skipping the rebaseline in edit mode would be
incomplete β discard would then revert `published` even though the server save
succeeded.)
**W3 β Mid-hydration rebuild of the active-filters cache reads pre-HYDRATE
Redux state** (new ordering bug)
Verified lifecycle: on AβB navigation the render gate
(`DashboardPage.tsx:414`) reopens β `hasDashboardInfoInitiated` is satisfied by
dashboard A's never-reset `dashboardInfo` store β and mounts
`SyncDashboardState` (`:416`); child effects run before parent effects, so its
subscriptions (`SyncDashboardState/index.tsx:159-168`) install before the
hydrate effect. `hydrate.ts` then seeds the layout store (~`:286`) *before*
dispatching `HYDRATE_DASHBOARD` (~`:362`), so the layout subscription fires
synchronously and `rebuildActiveFilters` reads A's `dashboardFilters` against
B's layout, overwriting the `getActiveFilters()` module cache (feeds
`getAppliedFilterValues`, `Chart.tsx:441`). Legacy filter-box only (empty on
modern dashboards), hence not critical. Note `useDiscardChanges` has the order
right.
**Fix**: dispatch `HYDRATE_DASHBOARD` before seeding the Zustand stores, or
re-run `buildActiveFilters` after the dispatch β plus a focused AβB navigation
test on the cache contents.
**W4 β Stale sticky-header width after resizing the vertical filter bar**
(new user-visible bug) *(suggest fixing before merge)*
`DashboardBuilder.tsx:744` parks the new width in `pendingFilterBarWidthRef`
during `ResizableSidebar`'s child render; the syncing no-dep `useLayoutEffect`
(`:546-550`) only runs when DashboardBuilder re-renders β and a sidebar drag
re-renders only the sidebar subtree (`ResizableSidebar/index.tsx:79-82`, width
is sidebar-local state). Drag the resize handle β the panel resizes but the
sticky header keeps the old width offset until an unrelated re-render.
**Fix**: lift the width into DashboardBuilder via an explicit
`onWidthChange` callback from `ResizableSidebar`.
**W5 β The zundo port dropped the rootless-layout undo guard**
Master's `undoableDashboardLayout.ts` refused to let undo/redo restore a
layout missing `DASHBOARD_ROOT_ID` (with a comment naming the crash it
prevented); the zundo store (`useDashboardLayoutStore.ts:152-156`) has no
equivalent β `undo()` blindly restores any snapshot. Reaching it needs a bad
snapshot in history (a tracked action between `setLayout` and
`temporal.clear()`, or pre-hydration) β the races the old guard existed for.
**Fix**: validate in `undoLayout()`/`redoLayout()`
(`dashboardLayout/actions.ts:49-53`) or a `wrapTemporal` guard.
### Suggestions
Inherited from `master` (verbatim ports β worth fixing while the code is
being touched):
1. `useSaveChartCustomization.ts:96-98` clears deleted customizations' data
masks *before* the PUT; failure leaves values dropped. Move after the
successful response. Also `:176` returns a fabricated `last_modified_time:
Date.now()` β a trap if ever wired into overwrite protection.
2. Pre-existing duplicate-submit gap exposed by the new Header guard:
`OverwriteConfirmModal.tsx:109-124` dispatches the save thunk outside the new
`saveInFlightRef`/`isPending` protection and the confirm button isn't disabled
on `dashboardIsSaving` β double-clicking "Overwrite" fires duplicate PUTs.
Cheap close: `loading`/`disabled` on `dashboardIsSaving`.
3. `hydrateDashboardInfo` (`coreSlice.ts:133-152`) merges and resets only
`pendingChartCustomizations`;
`chartCustomizationData`/`chartCustomizationLoading` leak across dashboards
(ids collide for exported/imported dashboards).
Structural hardening:
4. `hydrate.ts:239-282` seeds `dashboardState` via a hand-enumerated field
list (currently complete β verified key-by-key) while the exported
`dashboardStateInitialState` has zero importers; a future slice field would
silently survive AβB navigation. Seed with `{ ...dashboardStateInitialState,
...seed }` here and in `useDiscardChanges.ts:82`.
5. `setLayout` pushes the previous layout into zundo history and relies on
every caller pairing it with `temporal.getState().clear()` (all three current
sites do; the redux-undo wrapper did it atomically). Clear inside `setLayout`.
6. No shared store-reset helper for tests; merge-mode `setState(partial)`
seeding leaks keys across tests within a file (e.g. `Header.test.tsx:140-158`
hand-rolls `temporal.clear()`). An `afterEach` `resetAllDashboardStores()` in
`spec/helpers/setup.ts` would close the class.
7. `dataMask/actions.ts:61-101`: action creators mutate the Zustand store as
a side effect and return actions no reducer handles β calling the store actions
directly (as `embedded/api.tsx:113-115` does) and deleting the dead types would
remove the trap; the `dispatch({ type: ON_REFRESH })` at
`actions/dashboardState.ts:576` is also dead.
8. `setNativeFiltersConfig` (`dashboardInfo/slices/coreSlice.ts:89-116`)
re-implements `preserveScopes` (`slices/helpers.ts:38-68`) minus the
`.filter(Boolean)` β a null config entry (anticipated by `hydrate.ts:203`)
throws. Call the helper.
9. `src/queries/queryClient.ts:30`: global `retry: 1` re-issues 4xx
requests; a predicate skipping client errors avoids doomed retries.
### What looks good
Atomic hydration (all store seeds + the Redux dispatch in one synchronous
block β single React commit); the discard-snapshot design (`gcTime: Infinity`,
released on unmount, reload fallback, extensive tests); complete query-key
coverage via the factories; every queryFn through SupersetClient; line-faithful
reducer ports including the quirks (`isPersistent === undefined`, `UNDO_LIMIT +
2`, `maxUndoHistoryExceeded` thresholds); clean deletion β no orphaned action
constants, no `state.<migrated slice>` reads anywhere; one-directional bridge
inventory with no loop pairs; ASF headers on all new files; zero production
`any`; accurate UPDATING.md. Several genuine small fixes along the way (stale
filter badge on Clear All, embedded string-vs-number slice key,
deleted-filter-id guard).
### Limits
Click sequences above were derived from code reading and local Jest runs,
not reproduced in a browser; no Playwright/profiling/embedded exercise.
### Verdict
Needs changes, but close: W1, W2, and W4 are the new user-reachable defects
and worth fixing before merge; W3 and W5 could ride an immediate follow-up. One
process question for the committers: SIP-207 (#39209) was still labeled
`design:proposal`/OPEN as of 2026-08-20, and the migration ships unflagged with
no rollback switch β is the plan to conclude the SIP vote before merging?
--
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]
