fix(dashboard): prevent undo crash on new dashboard opened in edit mode#41252
fix(dashboard): prevent undo crash on new dashboard opened in edit mode#41252ayushanand13 wants to merge 6 commits into
Conversation
Opening a brand-new dashboard directly in edit mode (`?edit=true`) could
leave a stale or empty layout in the redux-undo history, so the Undo
control was enabled before any user action. Clicking it reverted the
layout to an empty state, throwing
`TypeError: Cannot read properties of undefined (reading 'type')` and
leaving the UI broken until reload.
Root cause: `HYDRATE_DASHBOARD` is a tracked action, and redux-undo's
`CLEAR_HISTORY` resets `_latestUnfiltered` to the current `present`
(non-null). When history is cleared while the layout is still the
pre-hydration `{}`, the next hydration captures that empty layout as an
undoable baseline.
Harden the undoable dashboard-layout reducer:
- Reset the undo history on every `HYDRATE_DASHBOARD` so hydration (not a
user edit) is never undoable and no layout from a previously edited
dashboard lingers after SPA navigation between dashboards.
- Never let undo/redo replace a valid layout with an invalid (rootless)
one; drop the corrupt history and keep the last valid layout instead,
so clicking Undo can never crash the dashboard.
Add reducer unit tests covering the stale-baseline scenario, SPA
navigation, the undo guard, and normal undo of a real edit.
Fixes apache#41246
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| } from 'src/dashboard/util/componentTypes'; | ||
|
|
||
| // Cast reducer to accept partial mock data and inspect history fields in tests | ||
| const reducer = undoableLayoutReducer as (state: any, action: any) => any; |
There was a problem hiding this comment.
Suggestion: Replace the reducer cast that uses any with concrete reducer-derived types (for state, action, and return) so the test keeps type safety without broad any usage. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The file is TypeScript and the cast explicitly uses any for both the reducer state and action parameters, which violates the custom rule against new or modified TS/TSX code using any. The suggestion correctly identifies a real rule violation in the current file.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts
**Line:** 37:37
**Comment:**
*Custom Rule: Replace the reducer cast that uses `any` with concrete reducer-derived types (for state, action, and return) so the test keeps type safety without broad `any` usage.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The use of Proposed FixUpdate the reducer cast in // Replace the existing cast with concrete types
import { DashboardLayoutState } from 'src/dashboard/types';
import { AnyAction } from 'redux';
const reducer = undoableLayoutReducer as (state: DashboardLayoutState | undefined, action: AnyAction) => DashboardLayoutState;(Note: Ensure There are no other comments on this PR to address. Would you like me to proceed with any other tasks? superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #9a6e31
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts - 1
- Naming inconsistency with related modules · Line 21-21
Review Details
-
Files reviewed - 2 · Commit Range:
4b2138d..4b2138d- superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts
- superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| // eslint-disable-next-line import/named | ||
| import undoable, { StateWithHistory } from 'redux-undo'; | ||
| import { UNDO_LIMIT } from '../util/constants'; | ||
| import undoable, { ActionCreators, StateWithHistory } from 'redux-undo'; |
There was a problem hiding this comment.
Rename the imported ActionCreators from 'redux-undo' to UndoActionCreators and update all references (e.g., UndoActionCreators.clearHistory()) to match the alias used elsewhere in the dashboard module.
Code Review Run #9a6e31
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if ( | ||
| state && | ||
| isValidLayout(state.present) && | ||
| !isValidLayout(nextState.present) | ||
| ) { | ||
| return baseUndoableReducer(state, ActionCreators.clearHistory()); |
There was a problem hiding this comment.
Suggestion: Clearing history in the invalid-transition guard can incorrectly mark a dashboard as having no unsaved changes after an Undo. undoLayoutAction treats past.length === 0 as "fully reverted" and sets hasUnsavedChanges to false, but this branch empties history even when the current layout is still edited. Preserve enough history state (or signal this recovery path separately) so Undo recovery from corrupt history does not fake a clean state. [logic error]
Severity Level: Major ⚠️
- ⚠️ Dashboard header may hide unsaved-changes indicator incorrectly.
- ⚠️ Navigation away can skip UnsavedChangesModal despite edits.
- ⚠️ Users may believe edits saved when history was just cleared.Steps of Reproduction ✅
1. In the dashboard header
(`superset-frontend/src/dashboard/components/Header/index.tsx:95-105, 215-217`), the Undo
button and Ctrl+Z shortcut call `boundActionCreators.onUndo()`, which is bound to
`undoLayoutAction` from `src/dashboard/actions/dashboardLayout.ts:12-29`.
2. Consider a dashboard editing session where `dashboardState.hasUnsavedChanges` is `true`
(set by layout edit thunks via `setUnsavedChangesAfterAction` in
`src/dashboard/actions/dashboardLayout.ts:99-127,130-205` or by header actions like
`handleOnPropertiesChange` in `Header/index.tsx:139-157`), and the `dashboardLayout` undo
history contains a corrupt last past entry whose layout is missing `DASHBOARD_ROOT_ID` (an
invalid/rootless layout the new guard is designed to protect against).
3. When the user triggers Undo, `undoLayoutAction` dispatches `UndoActionCreators.undo()`
(`src/dashboard/actions/dashboardLayout.ts:16-17`), which is handled by `undoableReducer`
in `src/dashboard/reducers/undoableDashboardLayout.ts:135-153`. `baseUndoableReducer`
first computes `nextState`; because the last past entry is invalid,
`isValidLayout(state.present)` is `true` and `!isValidLayout(nextState.present)` is
`true`, so the guard at lines 145-149 runs and returns `baseUndoableReducer(state,
ActionCreators.clearHistory())` (line 150), clearing `dashboardLayout.past` while keeping
`state.present` (the edited layout) unchanged.
4. Still inside the same Undo flow, `undoLayoutAction` then calls `getState()` and reads
`dashboardLayout` and `dashboardState` (`src/dashboard/actions/dashboardLayout.ts:19-20`).
Since the guard has just cleared history, `dashboardLayout.past.length === 0` even though
the current layout is still edited, and assuming `dashboardState.maxUndoHistoryExceeded`
and `dashboardState.updatedColorScheme` are both false (their normal values for a typical
edit session, initialized in `hydrate` at `src/dashboard/actions/hydrate.ts:26-31`), the
condition at `dashboardLayout.ts:21-25` passes and `dispatch(setUnsavedChanges(false))`
executes. As a result, `dashboardState.hasUnsavedChanges` becomes `false`, and the
header's unsaved-changes handling (`useUnsavedChangesPrompt` wired at
`Header/index.tsx:87-95`) stops warning on navigation and no longer indicates unsaved
edits, even though the user's layout changes are still present.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts
**Line:** 145:150
**Comment:**
*Logic Error: Clearing history in the invalid-transition guard can incorrectly mark a dashboard as having no unsaved changes after an Undo. `undoLayoutAction` treats `past.length === 0` as "fully reverted" and sets `hasUnsavedChanges` to false, but this branch empties history even when the current layout is still edited. Preserve enough history state (or signal this recovery path separately) so Undo recovery from corrupt history does not fake a clean state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left one functional concern that looks worth addressing before merge. The type-safety note is minor, but it does line up with the repo's "no new any" guidance.
Functional - worth addressing
-
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts:145-150The invalid-transition guard keeps
state.presentwhen undo would move to a rootless layout, but it also clears the entire undo history.undoLayoutAction()then reads the post-dispatch state and treatsdashboardLayout.past.length === 0as "fully reverted", which dispatchessetUnsavedChanges(false)whenmaxUndoHistoryExceededandupdatedColorSchemeare false.That means the recovery path can leave the edited layout visible while marking the dashboard clean, so the header/nav-away protection may stop warning even though there are still unsaved edits. Could we preserve enough history state, or otherwise signal this recovery path so
undoLayoutAction()does not fake a clean state after dropping corrupt history?
Minor
-
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts:37Small type-safety gap: the test casts the reducer through
(state: any, action: any) => any. Since the repo guidance explicitly avoids newanyin TS, would it be worth usingDashboardLayoutState/StateWithHistory<DashboardLayout>plusAnyActionor a narrowly typed test helper instead?
- Reject invalid undo/redo transitions by keeping the current valid state instead of clearing history. Clearing it could let undoLayoutAction() read the emptied `past` as a fully-reverted, clean dashboard and drop the unsaved-changes / nav-away guard while edits were still visible. The HYDRATE_DASHBOARD reset remains the primary fix that prevents a stale/empty baseline from entering history in the first place. - Type the reducer unit tests with DashboardLayout / AnyAction instead of `any`, per the repo's no-new-any guidance.
…ators Match the import alias used in dashboardState.ts and dashboardLayout.ts for naming consistency across the dashboard module (review nit).
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left one functional note below. The reducer-level reset looks directionally good, but this test setup appears to disagree with the redux-undo behavior locked in the frontend package.
Functional - worth addressing before merge
-
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts:128I don't think this setup creates the corrupt history that the assertion expects. The frontend lockfile resolves
redux-undoto1.1.0, and in that implementationclearHistory()withignoreInitialState: trueresets_latestUnfilteredtonull; after line 124, theUPDATE_COMPONENTSat line 126 makes the layout valid but does not push the empty{}intopast. A minimal reducer using the locked implementation leavesstate.past.length === 0here, so this expectation should fail in Jest.WDYT - could we either seed the corrupt history explicitly for the guard test, or adjust the test/root-cause explanation to match the locked
redux-undobehavior?
Code Review Agent Run #e569fcActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…new-dashboard-crash
The frontend lockfile resolves redux-undo to 1.1.0, where clearHistory() under ignoreInitialState resets _latestUnfiltered to null. The previous guard test seeded its corrupt-history precondition via a premature clearHistory + tracked action, which does not push a rootless layout onto `past` in 1.1.0 — so that assertion would fail in Jest. Rewrite the reducer tests to seed history states explicitly (mirroring redux-undo's StateWithHistory shape) so the guard test exercises a real rootless-undo precondition, add coverage that a normal valid->valid undo still works, and correct the root-cause comment to match 1.1.0 behavior. No production-logic change.
|
@richardfogaca thanks for the careful review — you were right about the locked version. The lockfile resolves Pushed
Context on 1.1.0: hydration leaves |
Code Review Agent Run #14ed50Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Thanks @ayushanand13, this looks solid. The reducer-level reset on hydration plus the rootless-layout guard reads cleanly, and returning state untouched on rejection nicely sidesteps the undoLayoutAction false-clean concern Richard raised. The valid -> valid undo test is a nice touch. Code LGTM; approving, happy to merge once CI's green, unless @richardfogaca feels differently.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41252 +/- ##
=======================================
Coverage 64.36% 64.37%
=======================================
Files 2653 2653
Lines 144867 144880 +13
Branches 33421 33427 +6
=======================================
+ Hits 93250 93265 +15
+ Misses 49946 49944 -2
Partials 1671 1671
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:
|
|
@rusackas waiting for your approval as "Merge branch 'master' into fix/dashboard-undo-new-dashboard-crash" |
3 participants
SUMMARY
The reported crash is
TypeError: Cannot read properties of undefined (reading 'type'), thrown when Undo reverts the dashboard layout to a rootless/empty state and rendering code readscomponent.typeon a missing component.Version note. The report is against 6.1.0. The frontend now locks
redux-undoto 1.1.0, whoseclearHistory()underignoreInitialStateresets_latestUnfilteredtonull— so a freshly hydrated dashboard already keeps Undo disabled and the exact reproduction does not trigger on current master. This PR hardens the undo-history handling so the crash class is impossible regardless of the redux-undo version or how a rootless layout might reach the stack.Changes (
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts):HYDRATE_DASHBOARDat the reducer level — hydration is not a user edit and must never be undoable; this guarantees the Undo control starts disabled and drops a previous dashboard's layout on SPA navigation (complementing the component-levelclearDashboardHistory()from fix(dashboard): clear undo history #40569).presentwith a rootless one; the transition is rejected and the valid layout kept, leaving history intact soundoLayoutAction()doesn't misread an emptied stack as a clean, fully-reverted dashboard.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
See the screen recording in #41246.
TypeError: Cannot read properties of undefined (reading 'type').TESTING INSTRUCTIONS
Reducer unit tests (seeded against the locked redux-undo 1.1.0 behavior) cover: hydration leaves Undo disabled; a layout edit flows through the wrapped reducer; re-hydrating a different dashboard clears the previous dashboard's stack (SPA navigation); undo never reverts to a rootless layout (guard) and preserves history; and a normal valid -> valid undo still works.
Manual: create a dashboard via + Dashboard, confirm Undo is disabled before any change, then make an edit and confirm Undo reverts it without error.
ADDITIONAL INFORMATION
?edit=true) causesTypeErroron click #41246