mikebridge commented on PR #41548: URL: https://github.com/apache/superset/pull/41548#issuecomment-5360234619
## Follow-up review — additional lenses (incremental to [my earlier review](https://github.com/apache/superset/pull/41548#issuecomment-5359810920)) > 🤖 Generated by Claude (AI) on behalf of @mikebridge, who directed and verified it. Five further review passes (delivery-practices, code-quality ×3, domain-modelling) ran over the same HEAD (`af7b90b`), findings deduplicated against the earlier review and re-verified at the cited lines. Only **new** material below — plus one correction to my own earlier comment. ### Correction to my earlier review My suggestion #2 ("no shared Zustand store-reset helper for tests; state leaks across tests") was **wrong about the reset**: `superset-frontend/__mocks__/zustand.ts` is a root Jest automock that resets every store — including zundo temporal history — between tests. Nice design. What survives of that finding is only a *seeding* gap: `spec/fixtures/mockStore.js` seeds 3 of the 6 migrated stores and `getMockStore` seeds none, which is why ~38 test files hand-roll their own seeding. ### Critical — stale Redux reads that now dereference removed slices The strongest findings of this round are two consumers that still read the migrated slices from Redux. Both compile because they declare their own local state types with optional fields instead of importing the store's `RootState`, and both have tests that hand-roll the old state shape, so CI stays green. **C1 — Every dashboard log event silently loses its attribution.** `src/middleware/loggerMiddleware.ts:162` destructures `dashboardInfo` and `dashboardLayout` from `store.getState()`, but the root reducer now registers only `charts`/`datasources`/`dashboardFilters` — so `dashboardInfo?.id` is always undefined. Consequences: dashboard events lose `source`/`source_id`/`dashboard_id`, `sendBeacon` drops `&dashboard_id=`, the `else if (explore?.slice)` fall-through can mislabel dashboard events as explore events, and the `dashboardLayout?.present?.[target_id]` target-name enrichment is dead. **Fix**: read `useDashboardInfoStore.getState()` / `useDashboardLayoutStore.getState()`, and type the middleware against the real `RootState` so the next slice removal is a compile error. **C2 — Dashboard report toggles take the chart branch.** `src/features/reports/ReportModal/actions.ts:132` destructures `dashboardInfo` from Redux; `isEmpty(undefined)` is true, so the code falls into `else if (!isEmpty(charts))` (charts intentionally stayed in Redux) and fetches a report for `Object.keys(charts)[0]` with `creationMethod: 'charts'`. Reachable via `toggleActive(...).finally(structureFetchAction)` from the dashboard header — after toggling a dashboard report, `reports.dashboards[id]` never refreshes and a wrong-resource API call fires. `ReportRootState` still declares `dashboardInfo`, masking the break. **C3 (preventive, same class)** — `src/components/Chart/chartAction.ts:89` still declares `dashboardInfo`/`dataMask` on its local Redux state type. No live reader today, but this is the exact hole that produced C1/C2. Worth a one-time sweep for locally-declared state types naming removed slices; `src/dashboard/types.ts` already exports a narrowed `RootState` that makes such reads compile errors. ### Warnings (new) 1. **Copy-save failures resolve as mutation success.** `actions/dashboardState.ts:489-490` ends the copy path with `.catch(response => onError(response))` — no rethrow (the overwrite path rethrows) — so `useSaveDashboard`'s mutation fires `onSuccess(undefined)`, the same sentinel that means "overwrite precheck deferred". `isError` is never true for a failed copy. Rethrow, and model the precheck deferral as an explicit result (`{ status: 'saved' | 'deferred' }`). 2. **Undo back to the saved baseline no longer clears `hasUnsavedChanges`** (master's `undoLayoutAction` did, guarded by the still-present escape flags), while redo still sets it — undo-everything leaves Save enabled and the beforeunload prompt armed. The code comment and `Header.test.tsx:441` show it's deliberate, but the PR body says "behavior-preserving" — worth surfacing as an explicit decision in the description. 3. **Emptied titles leave the dashboard permanently dirty.** The new per-keystroke title dirty-flagging clears only on exact revert to `savedTitle`, but `EditableTitle.handleBlur` refuses to commit empty/trim-unchanged values — so typing a title to empty and blurring strands the flag with no undo entry to clear it. Compare against `value.trim()`. 4. **The five `invalidateQueries(dashboardKeys.detail(id))` calls are no-ops on the dashboard route** — the page still fetches via the legacy `useApiV1Resource` (`DashboardPage.tsx:110-118`); the TanStack detail query's only consumer is PropertiesModal, and `DASHBOARD_GET_COLUMNS` + the JSON parse are duplicated from `hooks/apiResources/dashboards.ts` with no pinning test. Migrate the page fetch or drop the duplicate; if the page moves to TanStack later, the `[readyToRender]` hydration effect needs a rehydration guard first (the unused `isDashboardHydrated` ref is already there). 5. **`setInScopeStatusOfFilters` lacks the missing-filter guard its customization sibling has** (`util/inScopeStatus.ts:36-40`): a scope for a filter not yet in the store spreads `undefined`, misses the metadata merge, and writes a store entry keyed `"undefined"`. 6. **Test-infra details**: `spec/helpers/testing-library.tsx:91` allocates `new QueryClient` inside the wrapper component body, wiping the query cache on every re-render (hoist with `useState(() => …)`); `spec/fixtures/mockState.js` still ships the removed slices into Redux `preloadedState` (combineReducers warning on every dev store construction). 7. **Rollout/consistency posture**: extensions reading `state.dashboardState` etc. now get silent `undefined` (documented in UPDATING.md, but no shim or dev-mode warning — worth an explicit supported/internal call); global `refetchOnWindowFocus: true` is the wrong default polarity for an edit surface (every current query defuses it locally; the next one won't); and QueryClient access is split between `useQueryClient()` and a directly-imported singleton — identical in prod, divergent under the test wrapper, so the `getCachedSlice` wiring (drag-from-panel) is effectively untestable. Standardizing one access path would close it. ### Suggestions (new, brief) - The scope-preservation rule ("client-computed `chartsInScope`/`tabsInScope` survive a server refresh") now exists in **4-5 places with diverging predicates** (`preserveScopes`, its inline copy in `setNativeFiltersConfig`, `mergeFilterChanges` — which requires *both* fields non-null unlike the others, `hydrateNativeFilters`, and the merge in `useSaveChartCustomization`) — one named function, five call sites. - `useSaveChartCustomization`: post-save store writes/invalidation live inside `mutationFn`'s try, so a post-processing throw after a successful PUT toasts "Failed to save" — move effects to `onSuccess`, the optimistic mask-deletes to `onMutate` with rollback. - The two metadata settings PUTs also omit `last_modified_time`, so they bypass the overwrite-confirm guard on top of the last-writer-wins race already reported. - zundo details: `limit: 52` duplicates `UNDO_LIMIT + 2` as a literal (and caps one entry differently than redux-undo did); no `partialize`, so history snapshots the whole store — `partialize: s => ({ layout: s.layout })` fixes both. - Naming: two exported interfaces are both `CoreSlice` (dashboardInfo + dashboardState) with twin `coreInitialState` consts; `stores/dashboardSlices/` (charts) vs `stores/*/slices/` (Zustand units) overloads "slice"; the `metadataSlice` in the *state* store holds session UI state while `dashboardInfo.metadata` is the persisted blob; `useDashboardData.ts` exports `useDashboardQuery`. - Dead code: `queries/relatedOptions.ts` has zero consumers (and interpolates `query` into the URL unencoded — fix before ever wiring it up); `fillNativeFilters`' `currentFilters` param is undefined at its only call site (its compare-branch is unreachable); `getUndoLength` is a dead export (the vestige of the dropped clear-dirty-on-baseline logic in warning 2); two empty section banners in `actions/dashboardState.ts`. - Two inherited quirks gained new comments that overpromise (both verified as master parity, so the fix is the comment or a small guard, not urgent): `setRefreshFrequency` writes `undefined` into `shouldPersistRefreshFrequency` on the omit path while its comment claims the flag is untouched; `buildDashboardLayout` is documented "Pure build" but mutates the aliased `position_data` (and `hydrateDashboard` writes into `dashboard.metadata` in place, with the same reference landing in the cached discard snapshot). - Typed contracts worth adding while the shapes are fresh: derive `zustandStateSeed`'s type from the store (it's `Record<string, unknown>`, so a new store field silently changes discard semantics), and declare the hydration-attached permission flags (`superset_can_explore` …) as a real interface instead of an index signature recovered by casts. - `ReactQueryDevtools`: add an explicit `NODE_ENV === 'development' &&` guard and move the package to devDependencies (currently prod deps, safe only via the package's self-nulling); the embedded provider omits it inconsistently. - `applyMetadataSaveResult`'s unguarded `JSON.parse` is the shared post-save contract of four mutations and has no test — the most valuable of the ~13 currently-untested new modules. - Per Conventional Commits, a self-described behavior-preserving migration is `refactor(dashboard):`, not `feat(dashboard):`. ### What looks good (this round) The `__mocks__/zustand.ts` reset design; the pure `operations.ts` layout transforms behind domain-named actions (the modelling lens called it the best-modelled piece — worth propagating to the other stores); the oxlint `no-restricted-imports` boundary making the stores/queries layering pipeline-enforced rather than conventional; `spec/helpers/reducerIndex.ts` trimmed to match the production store; and an independent local run of the new suites came back green again (194 tests / 25 suites in this round's count). ### Updated verdict Same as before plus two: the earlier W1/W2/W4 stand, and **C1/C2 above join them as pre-merge fixes** — they're the "code that still thinks the Redux slices exist" class, and C3's sweep is the cheap way to make sure these two are the last of it. -- 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]
