From 4b2138d91ef10c54bec1c0bd16d0c3431756d707 Mon Sep 17 00:00:00 2001 From: Ayush Date: Sat, 20 Jun 2026 12:11:58 +0530 Subject: [PATCH 1/4] fix(dashboard): prevent undo crash on new dashboard opened in edit mode 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 #41246 --- .../reducers/undoableDashboardLayout.test.ts | 154 ++++++++++++++++++ .../reducers/undoableDashboardLayout.ts | 52 +++++- 2 files changed, 203 insertions(+), 3 deletions(-) create mode 100644 superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts new file mode 100644 index 000000000000..2f43b6123a7e --- /dev/null +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts @@ -0,0 +1,154 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ActionCreators } from 'redux-undo'; + +import undoableLayoutReducer from 'src/dashboard/reducers/undoableDashboardLayout'; +import { UPDATE_COMPONENTS } from 'src/dashboard/actions/dashboardLayout'; +import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate'; +import { + DASHBOARD_ROOT_ID, + DASHBOARD_GRID_ID, + DASHBOARD_HEADER_ID, +} from 'src/dashboard/util/constants'; +import { + DASHBOARD_ROOT_TYPE, + DASHBOARD_GRID_TYPE, + DASHBOARD_HEADER_TYPE, + CHART_TYPE, +} 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; + +// A minimal but valid dashboard layout always contains the root component. +const makeValidLayout = () => ({ + [DASHBOARD_ROOT_ID]: { + id: DASHBOARD_ROOT_ID, + type: DASHBOARD_ROOT_TYPE, + children: [DASHBOARD_GRID_ID], + }, + [DASHBOARD_GRID_ID]: { + id: DASHBOARD_GRID_ID, + type: DASHBOARD_GRID_TYPE, + parents: [DASHBOARD_ROOT_ID], + children: [], + }, + [DASHBOARD_HEADER_ID]: { + id: DASHBOARD_HEADER_ID, + type: DASHBOARD_HEADER_TYPE, + meta: { text: '[ untitled dashboard ]' }, + }, +}); + +const hydrate = (present: Record) => ({ + type: HYDRATE_DASHBOARD, + data: { dashboardLayout: { present } }, +}); + +const updateComponents = (nextComponents: Record) => ({ + type: UPDATE_COMPONENTS, + payload: { nextComponents }, +}); + +const init = () => reducer(undefined, { type: '@@INIT' }); + +test('hydrating a dashboard leaves an empty, disabled undo history', () => { + const state = reducer(init(), hydrate(makeValidLayout())); + + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + // Hydration is not a user edit, so Undo (past) and Redo (future) start empty + expect(state.past).toHaveLength(0); + expect(state.future).toHaveLength(0); +}); + +test('a stale/empty baseline left by a premature clearHistory does not survive hydration', () => { + // Reproduces the issue: clearing history while the layout is still the + // pre-hydration `{}` arms redux-undo to capture that empty layout on the next + // tracked action (it resets `_latestUnfiltered` to the current, empty present). + let state = init(); + state = reducer(state, ActionCreators.clearHistory()); + + // Without the fix, HYDRATE_DASHBOARD would now push the empty `{}` into `past`, + // enabling an Undo that reverts to an empty layout and crashes the dashboard. + state = reducer(state, hydrate(makeValidLayout())); + + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + expect(state.past).toHaveLength(0); +}); + +test('re-hydrating with a new dashboard drops the previous dashboard from the undo stack', () => { + // Simulates SPA navigation between dashboards without a full reload. + let state = reducer(init(), hydrate(makeValidLayout())); + + // A genuine edit on the first dashboard creates real undo history. + state = reducer( + state, + updateComponents({ + 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [] }, + }), + ); + expect(state.past.length).toBeGreaterThan(0); + + // Opening another dashboard re-hydrates and must reset the history so the + // previous dashboard's layout is no longer undoable. + const nextLayout = makeValidLayout(); + state = reducer(state, hydrate(nextLayout)); + + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + expect(state.past).toHaveLength(0); + expect(state.future).toHaveLength(0); +}); + +test('undo never reverts the layout to an invalid state and drops corrupt history', () => { + // Build a corrupt history (valid present, empty baseline in `past`) the same + // way it arises in the wild, then exercise Undo. + let state = init(); + state = reducer(state, ActionCreators.clearHistory()); // arms the empty baseline + // A tracked action other than hydrate captures the empty `{}` into `past`. + state = reducer(state, updateComponents(makeValidLayout())); + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + expect(state.past).toHaveLength(1); // the corrupt, empty baseline + + state = reducer(state, ActionCreators.undo()); + + // The guard keeps a valid layout instead of exposing the empty one... + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + // ...and clears the corrupt history so Undo becomes disabled. + expect(state.past).toHaveLength(0); +}); + +test('undo still reverts a genuine layout edit', () => { + let state = reducer(init(), hydrate(makeValidLayout())); + + state = reducer( + state, + updateComponents({ + 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [] }, + }), + ); + expect(state.present['CHART-abc']).toBeDefined(); + expect(state.past).toHaveLength(1); + + state = reducer(state, ActionCreators.undo()); + + // The added chart is undone, and the layout is still valid. + expect(state.present['CHART-abc']).toBeUndefined(); + expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + expect(state.past).toHaveLength(0); +}); diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts index 2854afea2e5e..2579857818b9 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts @@ -18,8 +18,8 @@ */ import { AnyAction, Reducer } from 'redux'; // 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'; +import { DASHBOARD_ROOT_ID, UNDO_LIMIT } from '../util/constants'; import { UPDATE_COMPONENTS, DELETE_COMPONENT, @@ -97,7 +97,7 @@ const layoutOnlyReducer: Reducer = ( return dashboardLayout(state || {}, action); }; -const undoableReducer: Reducer< +const baseUndoableReducer: Reducer< StateWithHistory, AnyAction > = undoable(layoutOnlyReducer, { @@ -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, 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()); + } + + return nextState; +}; + export default undoableReducer; From d5e23b627b3cc3029358aa8cfcf5e98b36f25f23 Mon Sep 17 00:00:00 2001 From: Ayush Date: Sat, 20 Jun 2026 21:47:04 +0530 Subject: [PATCH 2/4] refactor(dashboard): address review on undo-history guard - 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. --- .../reducers/undoableDashboardLayout.test.ts | 47 ++++++++++--------- .../reducers/undoableDashboardLayout.ts | 10 ++-- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts index 2f43b6123a7e..0246a06734c9 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts @@ -16,11 +16,13 @@ * specific language governing permissions and limitations * under the License. */ +import type { AnyAction } from 'redux'; import { ActionCreators } from 'redux-undo'; import undoableLayoutReducer from 'src/dashboard/reducers/undoableDashboardLayout'; import { UPDATE_COMPONENTS } from 'src/dashboard/actions/dashboardLayout'; import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate'; +import type { DashboardLayout } from 'src/dashboard/types'; import { DASHBOARD_ROOT_ID, DASHBOARD_GRID_ID, @@ -33,35 +35,41 @@ import { CHART_TYPE, } 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; +const reducer = undoableLayoutReducer; // A minimal but valid dashboard layout always contains the root component. -const makeValidLayout = () => ({ +const makeValidLayout = (): DashboardLayout => ({ [DASHBOARD_ROOT_ID]: { id: DASHBOARD_ROOT_ID, type: DASHBOARD_ROOT_TYPE, children: [DASHBOARD_GRID_ID], + meta: {}, }, [DASHBOARD_GRID_ID]: { id: DASHBOARD_GRID_ID, type: DASHBOARD_GRID_TYPE, parents: [DASHBOARD_ROOT_ID], children: [], + meta: {}, }, [DASHBOARD_HEADER_ID]: { id: DASHBOARD_HEADER_ID, type: DASHBOARD_HEADER_TYPE, + children: [], meta: { text: '[ untitled dashboard ]' }, }, }); -const hydrate = (present: Record) => ({ +const chartComponent = (): DashboardLayout => ({ + 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [], meta: {} }, +}); + +const hydrate = (present: DashboardLayout): AnyAction => ({ type: HYDRATE_DASHBOARD, data: { dashboardLayout: { present } }, }); -const updateComponents = (nextComponents: Record) => ({ +const updateComponents = (nextComponents: DashboardLayout): AnyAction => ({ type: UPDATE_COMPONENTS, payload: { nextComponents }, }); @@ -97,25 +105,19 @@ test('re-hydrating with a new dashboard drops the previous dashboard from the un let state = reducer(init(), hydrate(makeValidLayout())); // A genuine edit on the first dashboard creates real undo history. - state = reducer( - state, - updateComponents({ - 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [] }, - }), - ); + state = reducer(state, updateComponents(chartComponent())); expect(state.past.length).toBeGreaterThan(0); // Opening another dashboard re-hydrates and must reset the history so the // previous dashboard's layout is no longer undoable. - const nextLayout = makeValidLayout(); - state = reducer(state, hydrate(nextLayout)); + state = reducer(state, hydrate(makeValidLayout())); expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); expect(state.past).toHaveLength(0); expect(state.future).toHaveLength(0); }); -test('undo never reverts the layout to an invalid state and drops corrupt history', () => { +test('undo never reverts the layout to an invalid state', () => { // Build a corrupt history (valid present, empty baseline in `past`) the same // way it arises in the wild, then exercise Undo. let state = init(); @@ -125,23 +127,22 @@ test('undo never reverts the layout to an invalid state and drops corrupt histor expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); expect(state.past).toHaveLength(1); // the corrupt, empty baseline + const before = state.present; state = reducer(state, ActionCreators.undo()); - // The guard keeps a valid layout instead of exposing the empty one... + // The undo is rejected: the valid layout is kept instead of exposing the + // empty one, so rendering can't throw `undefined.type`. expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); - // ...and clears the corrupt history so Undo becomes disabled. - expect(state.past).toHaveLength(0); + expect(state.present).toBe(before); + // History is left untouched, so undoLayoutAction() won't read an emptied + // stack as a fully-reverted, clean dashboard. + expect(state.past).toHaveLength(1); }); test('undo still reverts a genuine layout edit', () => { let state = reducer(init(), hydrate(makeValidLayout())); - state = reducer( - state, - updateComponents({ - 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [] }, - }), - ); + state = reducer(state, updateComponents(chartComponent())); expect(state.present['CHART-abc']).toBeDefined(); expect(state.past).toHaveLength(1); diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts index 2579857818b9..0f80bf21776b 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts @@ -128,9 +128,11 @@ const isValidLayout = (layout?: DashboardLayout): boolean => * 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. + * with an invalid (rootless) one. Such a transition is rejected and the + * current valid layout is kept, so clicking Undo can never crash the + * dashboard. History is left untouched on rejection so callers that inspect + * it (e.g. undoLayoutAction) don't misread an emptied stack as a clean, + * fully-reverted dashboard and silently drop the unsaved-changes guard. */ const undoableReducer: Reducer, AnyAction> = ( state, @@ -147,7 +149,7 @@ const undoableReducer: Reducer, AnyAction> = ( isValidLayout(state.present) && !isValidLayout(nextState.present) ) { - return baseUndoableReducer(state, ActionCreators.clearHistory()); + return state; } return nextState; From a6e57f8e7c727916c45f42b4aafaf32f706747bc Mon Sep 17 00:00:00 2001 From: Ayush Date: Sat, 20 Jun 2026 21:51:17 +0530 Subject: [PATCH 3/4] refactor(dashboard): alias redux-undo ActionCreators as UndoActionCreators Match the import alias used in dashboardState.ts and dashboardLayout.ts for naming consistency across the dashboard module (review nit). --- .../dashboard/reducers/undoableDashboardLayout.test.ts | 10 +++++----- .../src/dashboard/reducers/undoableDashboardLayout.ts | 7 +++++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts index 0246a06734c9..7a6520c6cf9a 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts @@ -17,7 +17,7 @@ * under the License. */ import type { AnyAction } from 'redux'; -import { ActionCreators } from 'redux-undo'; +import { ActionCreators as UndoActionCreators } from 'redux-undo'; import undoableLayoutReducer from 'src/dashboard/reducers/undoableDashboardLayout'; import { UPDATE_COMPONENTS } from 'src/dashboard/actions/dashboardLayout'; @@ -90,7 +90,7 @@ test('a stale/empty baseline left by a premature clearHistory does not survive h // pre-hydration `{}` arms redux-undo to capture that empty layout on the next // tracked action (it resets `_latestUnfiltered` to the current, empty present). let state = init(); - state = reducer(state, ActionCreators.clearHistory()); + state = reducer(state, UndoActionCreators.clearHistory()); // Without the fix, HYDRATE_DASHBOARD would now push the empty `{}` into `past`, // enabling an Undo that reverts to an empty layout and crashes the dashboard. @@ -121,14 +121,14 @@ test('undo never reverts the layout to an invalid state', () => { // Build a corrupt history (valid present, empty baseline in `past`) the same // way it arises in the wild, then exercise Undo. let state = init(); - state = reducer(state, ActionCreators.clearHistory()); // arms the empty baseline + state = reducer(state, UndoActionCreators.clearHistory()); // arms the empty baseline // A tracked action other than hydrate captures the empty `{}` into `past`. state = reducer(state, updateComponents(makeValidLayout())); expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); expect(state.past).toHaveLength(1); // the corrupt, empty baseline const before = state.present; - state = reducer(state, ActionCreators.undo()); + state = reducer(state, UndoActionCreators.undo()); // The undo is rejected: the valid layout is kept instead of exposing the // empty one, so rendering can't throw `undefined.type`. @@ -146,7 +146,7 @@ test('undo still reverts a genuine layout edit', () => { expect(state.present['CHART-abc']).toBeDefined(); expect(state.past).toHaveLength(1); - state = reducer(state, ActionCreators.undo()); + state = reducer(state, UndoActionCreators.undo()); // The added chart is undone, and the layout is still valid. expect(state.present['CHART-abc']).toBeUndefined(); diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts index 0f80bf21776b..5ad2964b1afb 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts @@ -18,7 +18,10 @@ */ import { AnyAction, Reducer } from 'redux'; // eslint-disable-next-line import/named -import undoable, { ActionCreators, StateWithHistory } from 'redux-undo'; +import undoable, { + ActionCreators as UndoActionCreators, + StateWithHistory, +} from 'redux-undo'; import { DASHBOARD_ROOT_ID, UNDO_LIMIT } from '../util/constants'; import { UPDATE_COMPONENTS, @@ -141,7 +144,7 @@ const undoableReducer: Reducer, AnyAction> = ( const nextState = baseUndoableReducer(state, action); if (action.type === HYDRATE_DASHBOARD) { - return baseUndoableReducer(nextState, ActionCreators.clearHistory()); + return baseUndoableReducer(nextState, UndoActionCreators.clearHistory()); } if ( From 15ede2dc78e1cad25e68eb096f82f725d58b45d9 Mon Sep 17 00:00:00 2001 From: Ayush Date: Sun, 21 Jun 2026 11:24:42 +0530 Subject: [PATCH 4/4] test(dashboard): align undo-history tests with locked redux-undo 1.1.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../reducers/undoableDashboardLayout.test.ts | 129 +++++++++--------- .../reducers/undoableDashboardLayout.ts | 13 +- 2 files changed, 74 insertions(+), 68 deletions(-) diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts index 7a6520c6cf9a..4900179a27c6 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.test.ts @@ -17,7 +17,11 @@ * under the License. */ import type { AnyAction } from 'redux'; -import { ActionCreators as UndoActionCreators } from 'redux-undo'; +// eslint-disable-next-line import/named +import { + ActionCreators as UndoActionCreators, + StateWithHistory, +} from 'redux-undo'; import undoableLayoutReducer from 'src/dashboard/reducers/undoableDashboardLayout'; import { UPDATE_COMPONENTS } from 'src/dashboard/actions/dashboardLayout'; @@ -38,7 +42,9 @@ import { const reducer = undoableLayoutReducer; // A minimal but valid dashboard layout always contains the root component. -const makeValidLayout = (): DashboardLayout => ({ +const makeValidLayout = ( + title = '[ untitled dashboard ]', +): DashboardLayout => ({ [DASHBOARD_ROOT_ID]: { id: DASHBOARD_ROOT_ID, type: DASHBOARD_ROOT_TYPE, @@ -56,100 +62,99 @@ const makeValidLayout = (): DashboardLayout => ({ id: DASHBOARD_HEADER_ID, type: DASHBOARD_HEADER_TYPE, children: [], - meta: { text: '[ untitled dashboard ]' }, + meta: { text: title }, }, }); -const chartComponent = (): DashboardLayout => ({ - 'CHART-abc': { id: 'CHART-abc', type: CHART_TYPE, children: [], meta: {} }, -}); +// The frontend locks redux-undo to 1.1.0, whose `clearHistory()` under +// `ignoreInitialState` resets `_latestUnfiltered` to null. That makes a rootless +// layout impossible to push onto `past` through normal layout actions, so the +// guard's corrupt-history precondition is seeded directly. `makeHistory` mirrors +// redux-undo's `StateWithHistory` shape — `past`/`present`/`future` is all that +// `undo()` needs to compute the previous state. +const makeHistory = ( + past: DashboardLayout[], + present: DashboardLayout, + future: DashboardLayout[] = [], +): StateWithHistory => ({ past, present, future }); const hydrate = (present: DashboardLayout): AnyAction => ({ type: HYDRATE_DASHBOARD, data: { dashboardLayout: { present } }, }); -const updateComponents = (nextComponents: DashboardLayout): AnyAction => ({ - type: UPDATE_COMPONENTS, - payload: { nextComponents }, -}); - -const init = () => reducer(undefined, { type: '@@INIT' }); - test('hydrating a dashboard leaves an empty, disabled undo history', () => { - const state = reducer(init(), hydrate(makeValidLayout())); + const initial = reducer(undefined, { type: '@@INIT' }); + const state = reducer(initial, hydrate(makeValidLayout())); expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); - // Hydration is not a user edit, so Undo (past) and Redo (future) start empty + // Hydration is not a user edit, so Undo (past) and Redo (future) start empty. expect(state.past).toHaveLength(0); expect(state.future).toHaveLength(0); }); -test('a stale/empty baseline left by a premature clearHistory does not survive hydration', () => { - // Reproduces the issue: clearing history while the layout is still the - // pre-hydration `{}` arms redux-undo to capture that empty layout on the next - // tracked action (it resets `_latestUnfiltered` to the current, empty present). - let state = init(); - state = reducer(state, UndoActionCreators.clearHistory()); - - // Without the fix, HYDRATE_DASHBOARD would now push the empty `{}` into `past`, - // enabling an Undo that reverts to an empty layout and crashes the dashboard. - state = reducer(state, hydrate(makeValidLayout())); - +test('a layout edit is applied through the wrapped reducer', () => { + const hydrated = reducer( + reducer(undefined, { type: '@@INIT' }), + hydrate(makeValidLayout()), + ); + + const update: AnyAction = { + type: UPDATE_COMPONENTS, + payload: { + nextComponents: { + 'CHART-1': { id: 'CHART-1', type: CHART_TYPE, children: [], meta: {} }, + }, + }, + }; + const state = reducer(hydrated, update); + + expect(state.present['CHART-1']).toBeDefined(); expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); - expect(state.past).toHaveLength(0); }); -test('re-hydrating with a new dashboard drops the previous dashboard from the undo stack', () => { - // Simulates SPA navigation between dashboards without a full reload. - let state = reducer(init(), hydrate(makeValidLayout())); - - // A genuine edit on the first dashboard creates real undo history. - state = reducer(state, updateComponents(chartComponent())); - expect(state.past.length).toBeGreaterThan(0); +test('re-hydrating a different dashboard clears the previous dashboard from the undo stack', () => { + // Simulates SPA navigation: dashboard A already has undo history when B opens. + const dashboardA = makeHistory( + [makeValidLayout('A v1')], + makeValidLayout('A v2'), + ); - // Opening another dashboard re-hydrates and must reset the history so the - // previous dashboard's layout is no longer undoable. - state = reducer(state, hydrate(makeValidLayout())); + const state = reducer(dashboardA, hydrate(makeValidLayout('B'))); expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); expect(state.past).toHaveLength(0); expect(state.future).toHaveLength(0); }); -test('undo never reverts the layout to an invalid state', () => { - // Build a corrupt history (valid present, empty baseline in `past`) the same - // way it arises in the wild, then exercise Undo. - let state = init(); - state = reducer(state, UndoActionCreators.clearHistory()); // arms the empty baseline - // A tracked action other than hydrate captures the empty `{}` into `past`. - state = reducer(state, updateComponents(makeValidLayout())); - expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); - expect(state.past).toHaveLength(1); // the corrupt, empty baseline +test('undo never reverts the layout to an invalid (rootless) state', () => { + // A rootless `{}` baseline sits at the head of `past`; a plain redux-undo + // undo() here would move it into `present` and crash rendering with + // `Cannot read properties of undefined (reading 'type')`. + const corrupt = makeHistory([{}], makeValidLayout()); + const before = corrupt.present; - const before = state.present; - state = reducer(state, UndoActionCreators.undo()); + const state = reducer(corrupt, UndoActionCreators.undo()); - // The undo is rejected: the valid layout is kept instead of exposing the - // empty one, so rendering can't throw `undefined.type`. + // The guard rejects the transition: the valid layout is kept unchanged... expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); expect(state.present).toBe(before); - // History is left untouched, so undoLayoutAction() won't read an emptied - // stack as a fully-reverted, clean dashboard. + // ...and history is left intact, so undoLayoutAction() won't misread an + // emptied stack as a fully-reverted, clean dashboard. expect(state.past).toHaveLength(1); }); -test('undo still reverts a genuine layout edit', () => { - let state = reducer(init(), hydrate(makeValidLayout())); +test('the guard does not interfere with a normal undo between valid layouts', () => { + const previous = makeValidLayout('previous'); + const current = makeValidLayout('current'); - state = reducer(state, updateComponents(chartComponent())); - expect(state.present['CHART-abc']).toBeDefined(); - expect(state.past).toHaveLength(1); - - state = reducer(state, UndoActionCreators.undo()); + const state = reducer( + makeHistory([previous], current), + UndoActionCreators.undo(), + ); - // The added chart is undone, and the layout is still valid. - expect(state.present['CHART-abc']).toBeUndefined(); - expect(state.present[DASHBOARD_ROOT_ID]).toBeDefined(); + // A valid -> valid undo proceeds normally. + expect(state.present).toBe(previous); expect(state.past).toHaveLength(0); + expect(state.future).toHaveLength(1); }); diff --git a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts index 5ad2964b1afb..98ece3c581e8 100644 --- a/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts +++ b/superset-frontend/src/dashboard/reducers/undoableDashboardLayout.ts @@ -111,12 +111,13 @@ const baseUndoableReducer: Reducer< }); /* - * 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')`. + * A valid dashboard layout always contains the root component. Undo/redo must + * never leave `present` without it: a rootless layout renders the dashboard + * with no components and throws + * `TypeError: Cannot read properties of undefined (reading 'type')`. Such a + * state can arise whenever a rootless or empty layout reaches the undo history — + * e.g. an empty or partial hydration, or a tracked layout action dispatched + * before the dashboard has hydrated. */ const isValidLayout = (layout?: DashboardLayout): boolean => Boolean(layout && layout[DASHBOARD_ROOT_ID]);