From 20e6dfd37df8a4055fce609bb50eddb1f13ae2b0 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Thu, 16 Jul 2026 17:31:03 -0700 Subject: [PATCH] fix(dashboard): seed default active tab path at hydration (#42075) --- .../src/dashboard/actions/hydrate.test.ts | 276 ++++++++++++++++++ .../src/dashboard/actions/hydrate.ts | 16 +- .../components/nativeFilters/state.test.ts | 97 ++++-- .../components/nativeFilters/state.ts | 60 +--- .../dashboard/reducers/dashboardState.test.ts | 83 ++++++ .../util/getDefaultActiveTabs.test.ts | 112 +++++++ .../dashboard/util/getDefaultActiveTabs.ts | 54 ++++ 7 files changed, 612 insertions(+), 86 deletions(-) create mode 100644 superset-frontend/src/dashboard/actions/hydrate.test.ts create mode 100644 superset-frontend/src/dashboard/util/getDefaultActiveTabs.test.ts create mode 100644 superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts diff --git a/superset-frontend/src/dashboard/actions/hydrate.test.ts b/superset-frontend/src/dashboard/actions/hydrate.test.ts new file mode 100644 index 00000000000..797df346abe --- /dev/null +++ b/superset-frontend/src/dashboard/actions/hydrate.test.ts @@ -0,0 +1,276 @@ +/** + * 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 { HYDRATE_DASHBOARD, hydrateDashboard } from './hydrate'; +import { + DASHBOARD_ROOT_TYPE, + TABS_TYPE, + TAB_TYPE, +} from '../util/componentTypes'; +import { DASHBOARD_ROOT_ID } from '../util/constants'; + +/** + * Regression guard for the follow-up to PR #39417 / PR #41832: the default + * active-tab path must be seeded into `dashboardState.activeTabs` at hydration + * time, so every first-render consumer (filter bar, share menus, permalink + * utilities, screenshot download) reads the dashboard's real default tab path + * instead of an empty array. + * + * Before this change, hydrate seeded `activeTabs: activeTabs || + * dashboardState?.activeTabs || []` (hydrate.ts). With no permalink, no stored + * state and no directPathToChild, that resolved to `[]`, and the live `Tabs` + * component only populated the value from a post-mount effect. These tests + * assert the seeded default path and fail without the hydration-time seeding. + */ + +const layoutItem = ( + id: string, + type: string, + children: string[], + parents: string[], +) => ({ id, type, children, parents, meta: {} }); + +const buildDashboard = ( + positionData: Record, + title = 'Test dashboard', +) => ({ + id: 1, + dashboard_title: title, + css: '', + published: true, + changed_on: '2024-01-01T00:00:00.000Z', + owners: [], + metadata: {}, + position_data: positionData, +}); + +const hydrate = ( + positionData: Record, + overrides: { + activeTabs?: string[] | null; + dashboardState?: Record; + } = {}, +) => { + const dispatch = jest.fn((action: unknown) => action); + const getState = () => + ({ + user: { roles: {}, userId: 1 }, + common: { conf: {} }, + dashboardState: overrides.dashboardState ?? {}, + }) as any; + const action = ( + hydrateDashboard({ + history: { replace: jest.fn() }, + dashboard: buildDashboard(positionData), + charts: [], + dataMask: {}, + activeTabs: overrides.activeTabs ?? null, + chartStates: null, + } as any) as any + )(dispatch, getState); + return action; +}; + +test('seeds the default (first) tab path for a flat ROOT → TABS → TAB layout', () => { + const positionData = { + [DASHBOARD_ROOT_ID]: layoutItem( + DASHBOARD_ROOT_ID, + DASHBOARD_ROOT_TYPE, + ['TABS-1'], + [], + ), + 'TABS-1': layoutItem( + 'TABS-1', + TABS_TYPE, + ['TAB-1', 'TAB-2'], + [DASHBOARD_ROOT_ID], + ), + 'TAB-1': layoutItem('TAB-1', TAB_TYPE, [], [DASHBOARD_ROOT_ID, 'TABS-1']), + 'TAB-2': layoutItem('TAB-2', TAB_TYPE, [], [DASHBOARD_ROOT_ID, 'TABS-1']), + }; + + const action = hydrate(positionData); + + expect(action.type).toBe(HYDRATE_DASHBOARD); + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-1']); +}); + +test('seeds the recursive default path for nested TABS containers', () => { + const positionData = { + [DASHBOARD_ROOT_ID]: layoutItem( + DASHBOARD_ROOT_ID, + DASHBOARD_ROOT_TYPE, + ['TABS-1'], + [], + ), + 'TABS-1': layoutItem( + 'TABS-1', + TABS_TYPE, + ['TAB-1', 'TAB-2'], + [DASHBOARD_ROOT_ID], + ), + 'TAB-1': layoutItem( + 'TAB-1', + TAB_TYPE, + ['TABS-2'], + [DASHBOARD_ROOT_ID, 'TABS-1'], + ), + 'TAB-2': layoutItem('TAB-2', TAB_TYPE, [], [DASHBOARD_ROOT_ID, 'TABS-1']), + 'TABS-2': layoutItem( + 'TABS-2', + TABS_TYPE, + ['TAB-1-1', 'TAB-1-2'], + [DASHBOARD_ROOT_ID, 'TABS-1', 'TAB-1'], + ), + 'TAB-1-1': layoutItem( + 'TAB-1-1', + TAB_TYPE, + [], + [DASHBOARD_ROOT_ID, 'TABS-1', 'TAB-1', 'TABS-2'], + ), + 'TAB-1-2': layoutItem( + 'TAB-1-2', + TAB_TYPE, + [], + [DASHBOARD_ROOT_ID, 'TABS-1', 'TAB-1', 'TABS-2'], + ), + }; + + const action = hydrate(positionData); + + expect(action.type).toBe(HYDRATE_DASHBOARD); + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-1', 'TAB-1-1']); +}); + +test('seeds the default tab path for an embedded top-level-TABS layout (hideTab scenario)', () => { + // hideTab is a `uiConfig` property (read at DashboardBuilder.tsx, outside + // hydrated state), not a layout/position_data property, so it cannot and + // need not appear in this fixture. The seed derives purely from layout + // shape and is correct whether or not the top-level tab bar renders — this + // is an ordinary top-level-TABS layout mirroring state.test.ts's + // `embeddedLayout`, re-anchoring the #39417 embedded-filter-bar regression + // guard at the layer (hydration) that now owns the seeded value. + const positionData = { + [DASHBOARD_ROOT_ID]: layoutItem( + DASHBOARD_ROOT_ID, + DASHBOARD_ROOT_TYPE, + ['TABS-1'], + [], + ), + 'TABS-1': layoutItem( + 'TABS-1', + TABS_TYPE, + ['TAB-Company', 'TAB-Desktop'], + [DASHBOARD_ROOT_ID], + ), + 'TAB-Company': layoutItem( + 'TAB-Company', + TAB_TYPE, + [], + [DASHBOARD_ROOT_ID, 'TABS-1'], + ), + 'TAB-Desktop': layoutItem( + 'TAB-Desktop', + TAB_TYPE, + [], + [DASHBOARD_ROOT_ID, 'TABS-1'], + ), + }; + + const action = hydrate(positionData); + + expect(action.type).toBe(HYDRATE_DASHBOARD); + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-Company']); +}); + +// Precedence: the layout default only applies +// to a genuinely fresh load. A permalink `activeTabs`, a non-empty stored +// redux value, or a non-empty `directPathToChild` (deep link) must each +// suppress it. +const flatTabsPositionData = { + [DASHBOARD_ROOT_ID]: layoutItem( + DASHBOARD_ROOT_ID, + DASHBOARD_ROOT_TYPE, + ['TABS-1'], + [], + ), + 'TABS-1': layoutItem( + 'TABS-1', + TABS_TYPE, + ['TAB-1', 'TAB-2'], + [DASHBOARD_ROOT_ID], + ), + 'TAB-1': layoutItem('TAB-1', TAB_TYPE, [], [DASHBOARD_ROOT_ID, 'TABS-1']), + 'TAB-2': layoutItem('TAB-2', TAB_TYPE, [], [DASHBOARD_ROOT_ID, 'TABS-1']), +}; + +test('a permalink activeTabs param suppresses the layout default', () => { + const action = hydrate(flatTabsPositionData, { activeTabs: ['TAB-2'] }); + + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-2']); +}); + +test('a non-empty stored redux activeTabs value suppresses the layout default', () => { + const action = hydrate(flatTabsPositionData, { + dashboardState: { activeTabs: ['TAB-2'] }, + }); + + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-2']); +}); + +test('a non-empty directPathToChild (deep link) suppresses the layout default', () => { + const action = hydrate(flatTabsPositionData, { + dashboardState: { directPathToChild: ['TAB-2'] }, + }); + + expect(action.data.dashboardState.activeTabs).toEqual([]); +}); + +test('an empty stored redux activeTabs value still falls through to the layout default', () => { + // Pins the `.length` guard: a stored `activeTabs: []` is truthy but must + // not be treated as "already populated" — otherwise it would win over the + // layout default and regress to the pre-fix `[]`. + const action = hydrate(flatTabsPositionData, { + dashboardState: { activeTabs: [] }, + }); + + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-1']); +}); + +test('a non-empty stored redux value wins over a non-empty directPathToChild', () => { + // Pins the `||` operand order: stored value is checked before the + // directPathToChild-gated default branch. + const action = hydrate(flatTabsPositionData, { + dashboardState: { activeTabs: ['TAB-2'], directPathToChild: ['TAB-1'] }, + }); + + expect(action.data.dashboardState.activeTabs).toEqual(['TAB-2']); +}); + +test('a permalink activeTabs: [] (empty but present) wins and seeds []', () => { + // INTENTIONAL legacy-link behavior: unlike the stored-redux operand, the + // permalink param is not `?.length`-normalized, so a permalink that + // deliberately encoded "no active tabs" still wins over the layout + // default. This degrades to the pre-fix one-paint-late correction (the + // live Tabs component resolves the default after mount) rather than + // seeding the layout default — no regression, since that was already + // today's behavior for such links. + const action = hydrate(flatTabsPositionData, { activeTabs: [] }); + + expect(action.data.dashboardState.activeTabs).toEqual([]); +}); diff --git a/superset-frontend/src/dashboard/actions/hydrate.ts b/superset-frontend/src/dashboard/actions/hydrate.ts index 19f532f41eb..95b04ae452a 100644 --- a/superset-frontend/src/dashboard/actions/hydrate.ts +++ b/superset-frontend/src/dashboard/actions/hydrate.ts @@ -49,6 +49,7 @@ import { ROW_TYPE, } from 'src/dashboard/util/componentTypes'; import findFirstParentContainerId from 'src/dashboard/util/findFirstParentContainer'; +import getDefaultActiveTabs from 'src/dashboard/util/getDefaultActiveTabs'; import getEmptyLayout from 'src/dashboard/util/getEmptyLayout'; import getLocationHash from 'src/dashboard/util/getLocationHash'; import newComponentFactory, { @@ -327,6 +328,19 @@ export const hydrateDashboard = metadata.cross_filters_enabled as boolean | undefined, ); + // precedence: permalink param > stored redux value > layout default. + // The layout default only applies to a genuinely fresh load: no permalink + // activeTabs, no stored activeTabs, and no deep-link (directPathToChild), + // which the live Tabs component resolves on its own. + const seededActiveTabs = + activeTabs || + (dashboardState?.activeTabs?.length + ? dashboardState.activeTabs + : undefined) || + (directPathToChild.length + ? [] + : getDefaultActiveTabs(dashboardLayout.present as DashboardLayout)); + return dispatch({ type: HYDRATE_DASHBOARD, data: { @@ -390,7 +404,7 @@ export const hydrateDashboard = lastModifiedTime: dashboard.changed_on, isRefreshing: false, isFiltersRefreshing: false, - activeTabs: activeTabs || dashboardState?.activeTabs || [], + activeTabs: seededActiveTabs, datasetsStatus: dashboardState?.datasetsStatus || ResourceStatus.Loading, chartStates: chartStates || dashboardState?.chartStates || {}, diff --git a/superset-frontend/src/dashboard/components/nativeFilters/state.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/state.test.ts index dbb78c1167d..ec840142994 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/state.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/state.test.ts @@ -620,15 +620,15 @@ test('useChartCustomizationConfiguration ignores undefined items in metadata', ( ); }); -// --- Embedded / hideTab: activeTabs is empty --- +// --- Embedded / hideTab: seeded default tab path --- // When an embedded dashboard uses hideTab:true, the Tabs component never -// mounts, so setActiveTab never fires and activeTabs stays []. The same -// empty state occurs transiently on first render of any tabbed dashboard. -// -// useActiveDashboardTabs derives the default first tab from the layout when -// Redux activeTabs is empty, so scope evaluation uses the correct default -// tab instead of either "no tabs active" (blank filter bar) or "all tabs" -// (showing out-of-scope filters). +// mounts, so setActiveTab never fires. Previously this left activeTabs at its +// empty hydration default and useActiveDashboardTabs reconstructed the +// default tab path from the layout on every read. Now dashboard hydration +// seeds the default (first) tab path directly into Redux (see +// actions/hydrate.ts and util/getDefaultActiveTabs.ts), so +// useActiveDashboardTabs is a plain Redux read and these mocks inject the +// path hydration would have supplied. // Helper: build a layout with ROOT_ID → TABS container → TAB children function embeddedLayout(extras: Record> = {}) { @@ -672,10 +672,10 @@ function embeddedLayout(extras: Record> = {}) { }; } -test('useIsFilterInScope: filter scoped to default tab is in-scope when activeTabs is empty', () => { +test('useIsFilterInScope: filter scoped to default tab is in-scope for the seeded default tab', () => { (useSelector as jest.Mock).mockImplementation((selector: Function) => { const mockState = { - dashboardState: { activeTabs: [] }, + dashboardState: { activeTabs: ['TAB-Company'] }, dashboardLayout: { present: embeddedLayout() }, }; return selector(mockState); @@ -699,10 +699,10 @@ test('useIsFilterInScope: filter scoped to default tab is in-scope when activeTa expect(result.current(filter)).toBe(true); }); -test('useIsFilterInScope: filter scoped only to non-default tab is out-of-scope when activeTabs is empty', () => { +test('useIsFilterInScope: filter scoped only to non-default tab is out-of-scope for the seeded default tab', () => { (useSelector as jest.Mock).mockImplementation((selector: Function) => { const mockState = { - dashboardState: { activeTabs: [] }, + dashboardState: { activeTabs: ['TAB-Company'] }, dashboardLayout: { present: embeddedLayout() }, }; return selector(mockState); @@ -726,10 +726,10 @@ test('useIsFilterInScope: filter scoped only to non-default tab is out-of-scope expect(result.current(filter)).toBe(false); }); -test('useIsFilterInScope: filter with rootPath to default tab is in-scope when activeTabs is empty', () => { +test('useIsFilterInScope: filter with rootPath to default tab is in-scope for the seeded default tab', () => { (useSelector as jest.Mock).mockImplementation((selector: Function) => { const mockState = { - dashboardState: { activeTabs: [] }, + dashboardState: { activeTabs: ['TAB-Company'] }, dashboardLayout: { present: embeddedLayout() }, }; return selector(mockState); @@ -752,10 +752,10 @@ test('useIsFilterInScope: filter with rootPath to default tab is in-scope when a expect(result.current(filter)).toBe(true); }); -test('useSelectFiltersInScope: only default-tab filters are in scope when activeTabs is empty (embedded hideTab)', () => { +test('useSelectFiltersInScope: only default-tab filters are in scope for the seeded default tab (embedded hideTab)', () => { (useSelector as jest.Mock).mockImplementation((selector: Function) => { const mockState = { - dashboardState: { activeTabs: [] }, + dashboardState: { activeTabs: ['TAB-Company'] }, dashboardLayout: { present: embeddedLayout() }, }; return selector(mockState); @@ -801,10 +801,10 @@ test('useSelectFiltersInScope: only default-tab filters are in scope when active ); }); -test('useSelectFiltersInScope: dividers are always in scope even when activeTabs is empty', () => { +test('useSelectFiltersInScope: dividers are always in scope for the seeded default tab', () => { (useSelector as jest.Mock).mockImplementation((selector: Function) => { const mockState = { - dashboardState: { activeTabs: [] }, + dashboardState: { activeTabs: ['TAB-Company'] }, dashboardLayout: { present: embeddedLayout() }, }; return selector(mockState); @@ -958,6 +958,40 @@ test('useIsFilterInScope: missing dashboardLayout falls back without crashing', expect(() => result.current(filter)).not.toThrow(); }); +test('useIsFilterInScope: dashboardState with no activeTabs key falls back to [] without throwing', () => { + // DashboardStateShape.activeTabs is optional; on a partial store (e.g. + // pre-hydration render, a story, or a test that doesn't seed activeTabs at + // all) the plain Redux read can return undefined. useActiveDashboardTabs + // must normalize that to [] so downstream `.includes()` calls don't throw. + (useSelector as jest.Mock).mockImplementation((selector: Function) => { + const mockState = { + dashboardState: {}, + dashboardLayout: { present: embeddedLayout() }, + }; + return selector(mockState); + }); + + const filter: Filter = { + id: 'filter_no_active_tabs_key', + name: 'No activeTabs key', + filterType: 'filter_select', + type: NativeFilterType.NativeFilter, + chartsInScope: [1], + scope: { rootPath: ['TAB-Company'], excluded: [] }, + controlValues: {}, + defaultDataMask: {}, + cascadeParentIds: [], + targets: [{ column: { name: 'col' }, datasetId: 1 }], + description: 'Filter scoped to a tab, with no activeTabs key in state', + }; + + const { result } = renderHook(() => useIsFilterInScope()); + expect(() => result.current(filter)).not.toThrow(); + // With no activeTabs, a tab-scoped filter is out-of-scope, not "in scope + // by default" — falling back to [] must not be mistaken for "all tabs". + expect(result.current(filter)).toBe(false); +}); + // Shared fixture for the two nested-tabs tests below. Layout is identical; // only the redux activeTabs differs (empty for the default-path test, // inner-only for the hideTab ancestor-merge test). @@ -1015,12 +1049,14 @@ const mockNestedTabsState = (activeTabs: string[]) => ({ dashboardLayout: { present: nestedTabsLayout() }, }); -test('useIsFilterInScope: deeply nested tabs — default path includes inner-tab default', () => { +test('useIsFilterInScope: deeply nested tabs — scopes filters against an injected nested active-tab path', () => { // ROOT → TABS-1 → [TAB-Outer1, TAB-Outer2] // └─ TAB-Outer1 → TABS-2 → [TAB-Inner1, TAB-Inner2] - // Default path should be ['TAB-Outer1', 'TAB-Inner1']. + // activeTabs ['TAB-Outer1', 'TAB-Inner1'] mirrors the path hydration seeds + // into Redux (util/getDefaultActiveTabs.ts), injected here directly since + // this suite mocks useSelector rather than exercising hydration. (useSelector as jest.Mock).mockImplementation((selector: Function) => - selector(mockNestedTabsState([])), + selector(mockNestedTabsState(['TAB-Outer1', 'TAB-Inner1'])), ); const innerDefaultFilter: Filter = { @@ -1052,13 +1088,18 @@ test('useIsFilterInScope: deeply nested tabs — default path includes inner-tab expect(result.current(innerNonDefaultFilter)).toBe(false); }); -test('useIsFilterInScope: nested Tabs mounted under hideTab:true — outer ancestor merged so outer-tab scoping is preserved', () => { - // hideTab:true skips the top-level Tabs but a nested Tabs can still mount - // and dispatch setActiveTab. activeTabs holds only the inner id; without - // ancestor merging, filters whose charts have tabParents=[outer, inner] - // would be marked out-of-scope because the outer id is missing. +test('useIsFilterInScope: nested tabs — full Redux-supplied path keeps outer-tab scoping intact', () => { + // Formerly proved useActiveDashboardTabs reconstructed a missing outer + // ancestor from a partial Redux value (['TAB-Inner1'] only). That + // reconstruction responsibility no longer lives in the hook: hydrate + // seeding (util/getDefaultActiveTabs.ts) and the SET_ACTIVE_TAB reducer's + // ancestor-preservation property (see the mid-session regression guard in + // reducers/dashboardState.test.ts) now guarantee Redux always carries the + // full active-tab path, outer ancestor included. This test pins that a + // plain Redux read of that full path still scopes outer-tab filters + // correctly. (useSelector as jest.Mock).mockImplementation((selector: Function) => - selector(mockNestedTabsState(['TAB-Inner1'])), + selector(mockNestedTabsState(['TAB-Outer1', 'TAB-Inner1'])), ); const innerActiveFilter: Filter = { @@ -1090,7 +1131,7 @@ test('useIsFilterInScope: nested Tabs mounted under hideTab:true — outer ances }; const { result } = renderHook(() => useIsFilterInScope()); - // Outer ancestor TAB-Outer1 is merged into the active path → in scope. + // Outer ancestor TAB-Outer1 is present in the seeded path → in scope. expect(result.current(innerActiveFilter)).toBe(true); // TAB-Outer2 is not in the active path → out of scope, scoping preserved. expect(result.current(otherOuterFilter)).toBe(false); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/state.ts b/superset-frontend/src/dashboard/components/nativeFilters/state.ts index 046457f28c4..a2abf0bfda4 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/state.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/state.ts @@ -30,8 +30,7 @@ import { } from '@superset-ui/core'; import { FilterElement } from './FilterBar/FilterControls/types'; import { ActiveTabs, DashboardLayout, RootState } from '../../types'; -import { CHART_TYPE, TAB_TYPE, TABS_TYPE } from '../../util/componentTypes'; -import { DASHBOARD_ROOT_ID } from '../../util/constants'; +import { CHART_TYPE, TAB_TYPE } from '../../util/componentTypes'; import { isChartCustomizationId } from './FiltersConfigModal/utils'; import { migrateChartCustomizationArray, @@ -184,62 +183,9 @@ export function useDashboardHasTabs() { } function useActiveDashboardTabs(): ActiveTabs { - const reduxTabs = useSelector( - state => state.dashboardState?.activeTabs, + return useSelector( + state => state.dashboardState?.activeTabs ?? EMPTY_ACTIVE_TABS, ); - const dashboardLayout = useDashboardLayout(); - - return useMemo(() => { - const reduxList = reduxTabs ?? []; - const reduxFallback = reduxList.length ? reduxList : EMPTY_ACTIVE_TABS; - if (!dashboardLayout) return reduxFallback; - - // Tabbed dashboards always nest the top-level TABS container as the first - // child of ROOT. If that invariant doesn't hold (no-tabs layout), no - // fallback applies and we use reduxTabs as-is. - const root = dashboardLayout[DASHBOARD_ROOT_ID]; - if (!root?.children?.length) return reduxFallback; - const topContainer = dashboardLayout[root.children[0]]; - if (topContainer?.type !== TABS_TYPE || !topContainer.children?.length) { - return reduxFallback; - } - - // Walk every TABS container along the active path. For each container, - // pick the child Redux marked active; otherwise pick the first child (the - // default the live Tabs component would render). This handles: - // - empty reduxTabs (hideTab:true, no permalink) → full default path - // - reduxTabs missing an outer ancestor (hideTab:true skipped the - // top-level Tabs, but a nested Tabs dispatched setActiveTab) → fill - // in the missing ancestor so outer-tab scoping is preserved - // - fully populated reduxTabs (normal hydration) → same result - const reduxSet = new Set(reduxList); - const result: ActiveTabs = []; - const queue: string[] = [ - topContainer.children.find(c => reduxSet.has(c)) ?? - topContainer.children[0], - ]; - while (queue.length > 0) { - const tabId = queue.shift()!; - result.push(tabId); - const tab = dashboardLayout[tabId]; - if (!tab?.children) continue; - for (const childId of tab.children) { - const child = dashboardLayout[childId]; - if (child?.type !== TABS_TYPE || !child.children?.length) continue; - queue.push( - child.children.find(c => reduxSet.has(c)) ?? child.children[0], - ); - } - } - - // Preserve any reduxTabs entries that fell outside the traversed path so - // we never silently drop a redux-marked active tab id. - const resultSet = new Set(result); - for (const id of reduxList) { - if (!resultSet.has(id)) result.push(id); - } - return result; - }, [reduxTabs, dashboardLayout]); } function useSelectChartTabParents() { diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.test.ts b/superset-frontend/src/dashboard/reducers/dashboardState.test.ts index 24cf6287c9c..559aa6a7f37 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardState.test.ts +++ b/superset-frontend/src/dashboard/reducers/dashboardState.test.ts @@ -19,6 +19,7 @@ import configureMockStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import dashboardStateReducer from './dashboardState'; +import { HYDRATE_DASHBOARD } from '../actions/hydrate'; import { ADD_SLICE, ON_CHANGE, @@ -206,7 +207,89 @@ describe('DashboardState reducer', () => { }), ); }); + + test('preserves a hydrate-seeded outer ancestor across a nested-tab switch', () => { + const store = mockStore({ + dashboardState: { activeTabs: ['TAB-Outer1', 'TAB-Inner1'] }, + dashboardLayout: { + present: { + 'TAB-Outer1': { parents: [] }, + 'TAB-Outer2': { parents: [] }, + 'TAB-Inner1': { parents: ['TAB-Outer1', 'TABS-2'] }, + 'TAB-Inner2': { parents: ['TAB-Outer1', 'TABS-2'] }, + }, + }, + }); + const request = setActiveTab('TAB-Inner2', 'TAB-Inner1'); + const thunkAction = request( + store.dispatch, + store.getState as () => RootState, + ); + + const result = typedDashboardStateReducer( + createMockDashboardState({ + activeTabs: ['TAB-Outer1', 'TAB-Inner1'], + }), + thunkAction, + ); + + expect(result.activeTabs).toContain('TAB-Outer1'); + expect(result.activeTabs).toEqual( + expect.arrayContaining(['TAB-Outer1', 'TAB-Inner2']), + ); + }); + + // Pins the exact seam that replaced the deleted useActiveDashboardTabs + // layout-walk reconstruction: the Tabs component's initial-mount + // dispatch (no prevTabId) must not drop a hydrate-seeded outer ancestor + // either. The prior test only covers a later prev→next switch. + test('preserves a hydrate-seeded outer ancestor on the initial mount dispatch (no prevTabId)', () => { + const store = mockStore({ + dashboardState: { activeTabs: ['TAB-Outer1', 'TAB-Inner1'] }, + dashboardLayout: { + present: { + 'TAB-Outer1': { parents: [] }, + 'TAB-Outer2': { parents: [] }, + 'TAB-Inner1': { parents: ['TAB-Outer1', 'TABS-2'] }, + 'TAB-Inner2': { parents: ['TAB-Outer1', 'TABS-2'] }, + }, + }, + }); + const request = setActiveTab('TAB-Inner1', undefined); + const thunkAction = request( + store.dispatch, + store.getState as () => RootState, + ); + + const result = typedDashboardStateReducer( + createMockDashboardState({ + activeTabs: ['TAB-Outer1', 'TAB-Inner1'], + }), + thunkAction, + ); + + expect(result.activeTabs).toContain('TAB-Outer1'); + expect(result.activeTabs).toEqual( + expect.arrayContaining(['TAB-Outer1', 'TAB-Inner1']), + ); + }); }); + // Pins a side effect of seeding activeTabs at hydration (see + // actions/hydrate.ts / util/getDefaultActiveTabs.ts): a non-empty seeded + // activeTabs now reaches HYDRATE_DASHBOARD with entries, which the + // existing "Initialize tab activation times for initially active tabs" + // branch below was already written to handle — previously unreachable in + // practice on a fresh load because activeTabs was always seeded `[]`. + test('HYDRATE_DASHBOARD populates tabActivationTimes for a seeded activeTabs value', () => { + const result = typedDashboardStateReducer(undefined, { + type: HYDRATE_DASHBOARD, + data: { dashboardState: { activeTabs: ['TAB-1'] } }, + }); + + expect(result.activeTabs).toEqual(['TAB-1']); + expect(result.tabActivationTimes?.['TAB-1']).toEqual(expect.any(Number)); + }); + test('SET_ACTIVE_TABS', () => { expect( typedDashboardStateReducer( diff --git a/superset-frontend/src/dashboard/util/getDefaultActiveTabs.test.ts b/superset-frontend/src/dashboard/util/getDefaultActiveTabs.test.ts new file mode 100644 index 00000000000..2394bb2e960 --- /dev/null +++ b/superset-frontend/src/dashboard/util/getDefaultActiveTabs.test.ts @@ -0,0 +1,112 @@ +/** + * 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 getDefaultActiveTabs from './getDefaultActiveTabs'; +import { DASHBOARD_ROOT_ID } from './constants'; +import type { DashboardLayout } from '../types'; + +const layoutItem = ( + id: string, + type: string, + children: string[] = [], +): DashboardLayout[string] => + ({ id, type, children, meta: {} }) as DashboardLayout[string]; + +test('returns the first tab path for a flat ROOT → TABS → TAB layout', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', ['TAB-1', 'TAB-2']), + 'TAB-1': layoutItem('TAB-1', 'TAB'), + 'TAB-2': layoutItem('TAB-2', 'TAB'), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual(['TAB-1']); +}); + +test('recurses into nested TABS containers', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', ['TAB-1', 'TAB-2']), + 'TAB-1': layoutItem('TAB-1', 'TAB', ['TABS-2']), + 'TAB-2': layoutItem('TAB-2', 'TAB'), + 'TABS-2': layoutItem('TABS-2', 'TABS', ['TAB-1-1', 'TAB-1-2']), + 'TAB-1-1': layoutItem('TAB-1-1', 'TAB'), + 'TAB-1-2': layoutItem('TAB-1-2', 'TAB'), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual(['TAB-1', 'TAB-1-1']); +}); + +test('returns an empty path for a dashboard with no tabs', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['GRID_ID']), + GRID_ID: layoutItem('GRID_ID', 'GRID', ['CHART-1']), + 'CHART-1': layoutItem('CHART-1', 'CHART'), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual([]); +}); + +test('returns an empty path when TABS is nested under GRID rather than the first ROOT child', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['GRID_ID']), + GRID_ID: layoutItem('GRID_ID', 'GRID', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', ['TAB-1']), + 'TAB-1': layoutItem('TAB-1', 'TAB'), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual([]); +}); + +test('returns an empty path for a layout with no ROOT entry', () => { + const layout = {} as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual([]); +}); + +test('returns an empty path when the top-level TABS container has no children', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', []), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual([]); +}); + +test('returns the outer-only path when a nested TABS container has no children', () => { + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', ['TAB-1']), + 'TAB-1': layoutItem('TAB-1', 'TAB', ['TABS-2']), + 'TABS-2': layoutItem('TABS-2', 'TABS', []), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual(['TAB-1']); +}); + +test('terminates on a cyclic layout, emitting each id at most once', () => { + // TAB-1 → TABS-2 → TAB-1 (cycle back to the same tab) + const layout = { + [DASHBOARD_ROOT_ID]: layoutItem(DASHBOARD_ROOT_ID, 'ROOT', ['TABS-1']), + 'TABS-1': layoutItem('TABS-1', 'TABS', ['TAB-1']), + 'TAB-1': layoutItem('TAB-1', 'TAB', ['TABS-2']), + 'TABS-2': layoutItem('TABS-2', 'TABS', ['TAB-1']), + } as unknown as DashboardLayout; + + expect(getDefaultActiveTabs(layout)).toEqual(['TAB-1']); +}); diff --git a/superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts b/superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts new file mode 100644 index 00000000000..8e5fbd438df --- /dev/null +++ b/superset-frontend/src/dashboard/util/getDefaultActiveTabs.ts @@ -0,0 +1,54 @@ +/** + * 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 { DashboardLayout } from '../types'; +import { TABS_TYPE } from './componentTypes'; +import { DASHBOARD_ROOT_ID } from './constants'; + +// Default active-tab path a freshly-rendered dashboard shows: ROOT → first +// TABS → first TAB, recursing into nested TABS. Empty for non-tabbed layouts +// and for layouts whose top-level TABS is not the first ROOT child (e.g. TABS +// nested under GRID), matching the live Tabs component's default selection. +export default function getDefaultActiveTabs( + layout: DashboardLayout, +): string[] { + const root = layout?.[DASHBOARD_ROOT_ID]; + const top = root?.children?.length ? layout[root.children[0]] : undefined; + if (top?.type !== TABS_TYPE || !top.children?.length) return []; + + const result: string[] = []; + const visited = new Set(); + const queue: string[] = [top.children[0]]; + while (queue.length) { + const tabId = queue.shift() as string; + if (visited.has(tabId)) continue; + visited.add(tabId); + result.push(tabId); + layout[tabId]?.children?.forEach(childId => { + const child = layout[childId]; + if ( + child?.type === TABS_TYPE && + child.children?.length && + !visited.has(child.children[0]) + ) { + queue.push(child.children[0]); + } + }); + } + return result; +}