codeant-ai-for-open-source[bot] commented on code in PR #41252:
URL: https://github.com/apache/superset/pull/41252#discussion_r3445601931
##########
superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts:
##########
@@ -107,4 +107,50 @@ const undoableReducer: Reducer<
ignoreInitialState: true,
});
+/*
+ * A valid dashboard layout always contains the root component. The undo
history
+ * can otherwise capture a stale or empty layout as an undoable baseline — most
+ * notably the pre-hydration `{}` layout left behind when a brand-new dashboard
+ * is opened directly in edit mode via `?edit=true`. Reverting to such a state
+ * renders the dashboard with no components and throws
+ * `TypeError: Cannot read properties of undefined (reading 'type')`.
+ */
+const isValidLayout = (layout?: DashboardLayout): boolean =>
+ Boolean(layout && layout[DASHBOARD_ROOT_ID]);
+
+/*
+ * Wraps the redux-undo reducer to keep the dashboard layout undo history
sound:
+ *
+ * 1. Hydration establishes the baseline for the dashboard being opened. It is
+ * not a user edit and must never be undoable, so the history is reset on
+ * every HYDRATE_DASHBOARD. Doing this in the reducer — rather than relying
+ * solely on a follow-up clearDashboardHistory() dispatch from the page
+ * component — guarantees the Undo control starts disabled and that no
layout
+ * from a previously edited dashboard lingers in the stack after navigation.
+ * 2. As defense in depth, undo/redo is never allowed to replace a valid layout
+ * with an invalid one. If that would happen the corrupt history is dropped
+ * and the last valid layout is kept, so clicking Undo can never crash the
+ * dashboard.
+ */
+const undoableReducer: Reducer<StateWithHistory<DashboardLayout>, AnyAction> =
(
+ state,
+ action,
+) => {
+ const nextState = baseUndoableReducer(state, action);
+
+ if (action.type === HYDRATE_DASHBOARD) {
+ return baseUndoableReducer(nextState, ActionCreators.clearHistory());
+ }
+
+ if (
+ state &&
+ isValidLayout(state.present) &&
+ !isValidLayout(nextState.present)
+ ) {
+ return baseUndoableReducer(state, ActionCreators.clearHistory());
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=79cf7fafe2c241f583effc8a49df5326&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=79cf7fafe2c241f583effc8a49df5326&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41252&comment_hash=1e44d199b4df2a546dde09d98b1cb4a37c429510d29bfe8702e79dc0915e40b3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41252&comment_hash=1e44d199b4df2a546dde09d98b1cb4a37c429510d29bfe8702e79dc0915e40b3&reaction=dislike'>👎</a>
--
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]
