sadpandajoe commented on code in PR #44276: URL: https://github.com/apache/superset/pull/44276#discussion_r4305178822
########## superset-frontend/packages/superset-core/src/dashboard/index.ts: ########## @@ -0,0 +1,333 @@ +/** + * 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. + */ + +/** + * @fileoverview Dashboard API for Superset extensions. + * + * Exposes the dashboard currently active on the Dashboard surface (see + * `navigation.getPage() === 'dashboard'`) so extensions can identify it and + * read/apply its layout, custom CSS, and native filter values. + */ + +/** + * Gets the ID of the dashboard currently active on the Dashboard surface. + * + * @returns The current dashboard's ID, or undefined if none is active. + * + * @example + * ```typescript + * const dashboardId = dashboard.getDashboardId(); + * if (dashboardId != null) { + * console.log(`Dashboard ID: ${dashboardId}`); + * } + * ``` + */ +export declare function getDashboardId(): number | undefined; + +/** + * One component (row, column, chart holder, tab, markdown, etc.) in a + * dashboard's layout tree, as returned by {@link getLayout}. + */ +export interface LayoutNode { + /** IDs of this node's child components, in display order. */ + children: string[]; + + /** IDs of this node's ancestor components, root first. */ + parents?: string[]; + + /** Component type, e.g. `'CHART'`, `'ROW'`, `'TABS'`, `'MARKDOWN'`. */ + type: string; + + /** This node's own ID, e.g. `'CHART-abc123'`. */ + id: string; + + /** Grid size/position and other component-specific settings. */ + meta: { + chartId?: number; + width?: number; + height?: number; + [key: string]: unknown; + }; +} + +/** + * Gets the current dashboard's full layout tree — one entry per component + * (row, column, chart holder, tab, markdown, etc.), keyed by node ID. + * + * The returned map and every node (and its nested `meta`/`children`/ + * `parents`) are frozen: mutating them has no effect on the dashboard — + * use {@link updateLayoutNode} to apply a change instead. + * + * @returns A map of node ID to layout node. + * + * @example + * ```typescript + * const layout = dashboard.getLayout(); + * const chartNode = layout['CHART-abc123']; + * console.log(chartNode.meta.width); + * ``` + */ +export declare function getLayout(): Record<string, LayoutNode>; Review Comment: `getLayout()` returns the raw layout map, but every value is declared as a `LayoutNode` with required `children` and `meta`. The map also contains the `DASHBOARD_VERSION_KEY` version string, and the hydrated `HEADER_ID` entry has no `children`, so type-correct extension code like `Object.values(dashboard.getLayout()).map(n => n.children.length)` throws on an ordinary dashboard. Could this normalize what is returned, or declare the real heterogeneous shape (optional `children`/`meta`, excluding the version key)? ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,354 @@ +/** + * 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 { dashboard as dashboardApi } from '@apache-superset/core'; +import { isNativeFilter, makeApi, SupersetClient } from '@superset-ui/core'; +import type { + DataMask, + Divider, + Filter, + JsonObject, + QueryFormData, +} from '@superset-ui/core'; +import { matchPath } from 'react-router-dom'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { applySavedFilterChanges } from 'src/dashboard/actions/nativeFilters'; +import type { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; +import { updateDataMask } from 'src/dataMask/actions'; +import { + setChartFormData, + triggerQuery, +} from 'src/components/Chart/chartAction'; +import { invalidateChartFormDataCache } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { applyDefaultFormData } from 'src/explore/store'; +import extractUrlParams from 'src/dashboard/util/extractUrlParams'; +import { RoutePaths } from 'src/views/routePaths'; +import { store, RootState } from 'src/views/store'; +import { navigation } from '../navigation'; + +const getState = () => store.getState() as RootState; + +// The `:idOrSlug` the browser's current URL routes to, read directly off +// `window.location` rather than from React Router context, matching how +// `navigation`'s own page derivation works. +const getRoutedIdOrSlug = (): string | undefined => + matchPath<{ idOrSlug: string }>(window.location.pathname, { + path: RoutePaths.DASHBOARD, + exact: false, + })?.params.idOrSlug; + +// `dashboardInfo`/`dashboardLayout`/`nativeFilters`/`charts` are retained +// across an in-SPA navigation, and even once the browser has routed to a new +// dashboard, they keep the *previous* dashboard's data until that +// dashboard's HYDRATE_DASHBOARD completes. Comparing the URL's `idOrSlug` +// against `dashboardInfo`'s own id/slug — rather than trusting the page type +// alone — closes this window: they only match again once hydration has +// actually caught up, including for a same-surface dashboard-to-dashboard +// navigation. +const isDashboardActive = (): boolean => { + if (navigation.getPage() !== 'dashboard') return false; + const idOrSlug = getRoutedIdOrSlug(); + if (idOrSlug == null) return false; + const { id, slug } = getState().dashboardInfo; + return id != null && (String(id) === idOrSlug || slug === idOrSlug); +}; + +// True only while the dashboard identified by `dashboardId` is still the one +// on screen. An async API call captures its dashboard's id up front and +// rechecks it after each await, so a navigation that completes in between +// can't have a stale response applied to the dashboard now being shown. +const isStillActiveDashboard = (dashboardId: number): boolean => + isDashboardActive() && getState().dashboardInfo.id === dashboardId; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (!isDashboardActive() || id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + isDashboardActive() ? (getState().dashboardInfo.id ?? undefined) : undefined; + +// Every dashboardLayout reducer treats a node (and its `meta`/`children`/ +// `parents`) as immutable already — an update always replaces it with a new +// object rather than mutating it in place — so freezing the copy below only +// makes that existing contract explicit, without risking a reducer's own +// future in-place update on a node returned from here. +function deepFreeze<T>(value: T): T { + if (value !== null && typeof value === 'object' && !Object.isFrozen(value)) { + Object.freeze(value); + Object.values(value as Record<string, unknown>).forEach(deepFreeze); + } + return value; +} + +const getLayout: typeof dashboardApi.getLayout = () => + isDashboardActive() + ? deepFreeze({ ...getState().dashboardLayout.present }) + : {}; + +const getActiveTabs: typeof dashboardApi.getActiveTabs = () => + isDashboardActive() ? [...(getState().dashboardState.activeTabs ?? [])] : []; + +const updateLayoutNode: typeof dashboardApi.updateLayoutNode = async ( + nodeId: string, + meta: Record<string, unknown>, +) => { + requireDashboardId(); + const node = getState().dashboardLayout.present[nodeId]; + if (!node) { + throw new Error(`Layout node "${nodeId}" not found`); + } + // UPDATE_COMPONENTS replaces each keyed entry wholesale (it's not a deep + // merge), so the node's other fields must be carried through alongside + // the merged meta. + store.dispatch( + updateComponents({ + [nodeId]: { ...node, meta: { ...node.meta, ...meta } }, + }) as any, + ); +}; + +const getCss: typeof dashboardApi.getCss = () => + isDashboardActive() ? (getState().dashboardInfo.css ?? '') : ''; + +const setCss: typeof dashboardApi.setCss = async (css: string) => { + requireDashboardId(); + store.dispatch(dashboardInfoChanged({ css })); +}; + +const getFilters: typeof dashboardApi.getFilters = () => { + if (!isDashboardActive()) return []; + const { nativeFilters, dataMask } = getState(); + const filterElements = Object.values(nativeFilters.filters) as Array< + Filter | Divider + >; + return filterElements.filter(isNativeFilter).map(filter => { + const mask = dataMask[filter.id]; + return { + id: filter.id, + name: filter.name, + filterType: filter.filterType, + targets: filter.targets, + extraFormData: mask?.extraFormData, + filterState: mask?.filterState, + }; + }); +}; + +const updateFilters: typeof dashboardApi.updateFilters = async ( + updates: dashboardApi.FilterValueUpdate[], +) => { + requireDashboardId(); + const { filters: currentFilters } = getState().nativeFilters; + updates.forEach(({ filterId }) => { + if (!currentFilters[filterId]) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + }); + updates.forEach(({ filterId, extraFormData, filterState }) => { + const dataMask: DataMask = {}; + if (extraFormData !== undefined) { + dataMask.extraFormData = extraFormData; + } + if (filterState !== undefined) { + dataMask.filterState = filterState; + } + store.dispatch(updateDataMask(filterId, dataMask)); + }); +}; + +const saveFilters: typeof dashboardApi.saveFilters = async ( + updates: dashboardApi.FilterConfigUpdate[], + deletedFilterIds: string[] = [], +) => { + const dashboardId = requireDashboardId(); + const { filters: currentFilters } = getState().nativeFilters; + + const modified = updates.map( + ({ filterId, name, targets, defaultDataMask }) => { + const existing = currentFilters[filterId]; + if (!existing) { + throw new Error(`Filter "${filterId}" not found on this dashboard`); + } + return { + ...existing, + ...(name !== undefined && { name }), + ...(targets !== undefined && { targets }), + ...(defaultDataMask !== undefined && { defaultDataMask }), + }; + }, + ) as SaveFilterChangesType['modified']; + + if (modified.length === 0 && deletedFilterIds.length === 0) { + return; + } + + const filterChanges: SaveFilterChangesType = { + modified, + deleted: deletedFilterIds, + reordered: [], + }; + + const putFilters = makeApi<SaveFilterChangesType, { result: Filter[] }>({ + method: 'PUT', + endpoint: `/api/v1/dashboard/${dashboardId}/filters`, + }); + const response = await putFilters(filterChanges); + + // The save is already persisted, but the navigation that completed while + // it was in flight means the local state now belongs to another dashboard, + // which must not receive this one's saved filters. + if (!isStillActiveDashboard(dashboardId)) return; + + // Snapshot the live filter values only now, immediately before they are + // reset below, so a value the user changed while the request was pending + // isn't reverted to a stale one. + const { dataMask: currentDataMask } = getState(); + applySavedFilterChanges( + store.dispatch, + filterChanges, + response.result, + currentFilters, + ); + + // applySavedFilterChanges resets each modified filter's live extraFormData/ + // filterState back to its default unless the filter is required or + // defaultToFirstItem (see updateDataMaskForFilterChanges), even for an + // update that never touched the filter's value (e.g. a rename). Restore + // whatever was live immediately before that reset for any filter whose + // update didn't explicitly set a new one. + updates.forEach(({ filterId, defaultDataMask }) => { + if (defaultDataMask !== undefined) return; + const liveMask = currentDataMask[filterId]; + if (!liveMask) return; + store.dispatch( Review Comment: This restores the pre-save selection for every update that omits `defaultDataMask`, including one that changes `targets`. `updateDataMaskForFilterChanges` deliberately resets the mask when the targets differ, so e.g. a live `region IN ['US']` filter saved with a new `country` target comes back still applying the `region` predicate, and the new column ends up filtered by a value picked for the old one. Could the restore be skipped (or limited to rename-only updates) when `targets` is supplied and differs from the existing definition? ########## superset-frontend/src/core/dashboard/index.test.ts: ########## @@ -0,0 +1,818 @@ +/** + * 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 { SupersetClient, makeApi } from '@superset-ui/core'; +import type { QueryFormData } from '@superset-ui/core'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { applySavedFilterChanges } from 'src/dashboard/actions/nativeFilters'; +import { updateDataMask } from 'src/dataMask/actions'; +import { + setChartFormData, + triggerQuery, +} from 'src/components/Chart/chartAction'; +import { invalidateChartFormDataCache } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { applyDefaultFormData } from 'src/explore/store'; +import extractUrlParams from 'src/dashboard/util/extractUrlParams'; +import { store } from 'src/views/store'; +import { navigation } from '../navigation'; +import { dashboard } from './index'; + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { get: jest.fn() }, + makeApi: jest.fn(), +})); + +jest.mock('src/dashboard/actions/dashboardLayout', () => ({ + updateComponents: jest.fn(nextComponents => ({ + type: 'MOCK_UPDATE_COMPONENTS', + nextComponents, + })), +})); + +jest.mock('src/dashboard/actions/dashboardInfo', () => ({ + dashboardInfoChanged: jest.fn(newInfo => ({ + type: 'MOCK_DASHBOARD_INFO_CHANGED', + newInfo, + })), +})); + +jest.mock('src/dashboard/actions/nativeFilters', () => ({ + applySavedFilterChanges: jest.fn(), +})); + +jest.mock('src/dataMask/actions', () => ({ + updateDataMask: jest.fn((filterId, dataMask) => ({ + type: 'MOCK_UPDATE_DATA_MASK', + filterId, + dataMask, + })), +})); + +jest.mock('src/explore/store', () => ({ + applyDefaultFormData: jest.fn(formData => formData), +})); + +jest.mock('src/dashboard/util/extractUrlParams', () => jest.fn(() => ({}))); + +jest.mock('src/dashboard/util/charts/getFormDataWithExtraFilters', () => ({ + invalidateChartFormDataCache: jest.fn(), +})); + +jest.mock('src/views/store', () => ({ + store: { + getState: jest.fn(), + dispatch: jest.fn(), + subscribe: jest.fn(() => jest.fn()), + }, +})); + +jest.mock('../navigation', () => ({ + navigation: { getPage: jest.fn() }, +})); + +const mockGetPage = navigation.getPage as jest.Mock; +const mockGetState = store.getState as jest.Mock; +const mockDispatch = store.dispatch as jest.Mock; +const mockStoreSubscribe = store.subscribe as jest.Mock; +const mockClientGet = SupersetClient.get as jest.Mock; +const mockMakeApi = makeApi as jest.Mock; +const mockApplySavedFilterChanges = applySavedFilterChanges as jest.Mock; +const mockExtractUrlParams = extractUrlParams as jest.Mock; +const mockApplyDefaultFormData = applyDefaultFormData as jest.Mock; +const mockInvalidateChartFormDataCache = + invalidateChartFormDataCache as jest.Mock; + +// isDashboardActive() matches this against `window.location.pathname`'s +// `:idOrSlug`, so a test that activates dashboard N must also route the +// (mocked) browser to it, the way `DashboardPage`'s own URL would. +function activateDashboard( + overrides: Record<string, unknown> = {}, + pathname?: string, +) { + mockGetPage.mockReturnValue('dashboard'); + const dashboardInfo = (overrides.dashboardInfo as + | { id?: number; slug?: string } + | undefined) ?? { id: 7, css: '' }; + window.location.pathname = pathname ?? `/dashboard/${dashboardInfo.id}/`; + mockGetState.mockReturnValue({ + dashboardInfo: { id: 7, css: '' }, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + charts: {}, + ...overrides, + }); +} + +beforeEach(() => { + jest.clearAllMocks(); + // Reassigning window.location wholesale (rather than just .pathname) + // ensures it's a plain, freely writable object regardless of what a + // previous test left it as. + Object.defineProperty(window, 'location', { + writable: true, + value: { pathname: '/' }, + }); + mockGetPage.mockReturnValue('home'); + mockGetState.mockReturnValue({ + dashboardInfo: {}, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + charts: {}, + }); + mockExtractUrlParams.mockReturnValue({}); + mockApplyDefaultFormData.mockImplementation(formData => formData); + // clearAllMocks() only clears calls, not a mockImplementation override + // left over from a previous test, so restore the default here. + mockStoreSubscribe.mockImplementation(() => jest.fn()); +}); + +const existingFilter = { + id: 'NATIVE_FILTER-1', + type: 'NATIVE_FILTER', + name: 'Region', + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'region' } }], + scope: { rootPath: ['ROOT_ID'], excluded: [] }, + controlValues: {}, + cascadeParentIds: [], + defaultDataMask: {}, + description: '', +}; + +test('getDashboardId returns undefined when the dashboard is not active', () => { + expect(dashboard.getDashboardId()).toBeUndefined(); +}); + +test('getDashboardId returns the active dashboard id', () => { + activateDashboard({ dashboardInfo: { id: 42, css: '' } }); + expect(dashboard.getDashboardId()).toBe(42); +}); + +test('getDashboardId is undefined mid-navigation, when the route has moved on to a new dashboard but Redux hasn’t hydrated it yet', () => { + // The browser has already routed to dashboard 8, but dashboardInfo (and + // the other Redux slices) still hold dashboard 5's data until its + // HYDRATE_DASHBOARD completes. + mockGetPage.mockReturnValue('dashboard'); + window.location.pathname = '/dashboard/8/'; + mockGetState.mockReturnValue({ + dashboardInfo: { id: 5, css: '' }, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + charts: {}, + }); + + expect(dashboard.getDashboardId()).toBeUndefined(); + expect(dashboard.getCss()).toBe(''); + expect(dashboard.getFilters()).toEqual([]); +}); + +test('getDashboardId is active again once dashboardInfo catches up with the routed dashboard', () => { + mockGetPage.mockReturnValue('dashboard'); + window.location.pathname = '/dashboard/8/'; + mockGetState.mockReturnValue({ + dashboardInfo: { id: 8, css: '' }, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + charts: {}, + }); + + expect(dashboard.getDashboardId()).toBe(8); +}); + +test('getDashboardId matches the routed dashboard by slug', () => { + mockGetPage.mockReturnValue('dashboard'); + window.location.pathname = '/dashboard/my-dashboard-slug/'; + mockGetState.mockReturnValue({ + dashboardInfo: { id: 8, slug: 'my-dashboard-slug', css: '' }, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + charts: {}, + }); + + expect(dashboard.getDashboardId()).toBe(8); +}); + +test('getLayout returns {} when the dashboard is not active', () => { + expect(dashboard.getLayout()).toEqual({}); +}); + +test('getLayout returns a copy of the current layout when active', () => { + const present = { 'CHART-1': { id: 'CHART-1', meta: { width: 4 } } }; + activateDashboard({ dashboardLayout: { present } }); + const layout = dashboard.getLayout(); + expect(layout).toEqual(present); + expect(layout).not.toBe(present); +}); + +test('getLayout freezes the returned map and every node, including nested meta', () => { + const present = { + 'CHART-1': { + id: 'CHART-1', + children: ['CHART-1-content'], + meta: { width: 4, chartId: 99 }, + }, + }; + activateDashboard({ dashboardLayout: { present } }); + const layout = dashboard.getLayout(); + + expect(Object.isFrozen(layout)).toBe(true); + expect(Object.isFrozen(layout['CHART-1'])).toBe(true); + expect(Object.isFrozen(layout['CHART-1'].meta)).toBe(true); + expect(Object.isFrozen(layout['CHART-1'].children)).toBe(true); + + // A mutation attempt must not leak into the next call's result, and must + // not corrupt the live Redux state it's sharing node references with. + expect(() => { + (layout['CHART-1'].meta as Record<string, unknown>).width = 999; + }).toThrow(TypeError); + expect(present['CHART-1'].meta.width).toBe(4); + expect(dashboard.getLayout()['CHART-1'].meta.width).toBe(4); +}); + +test('getActiveTabs returns [] when the dashboard is not active', () => { + expect(dashboard.getActiveTabs()).toEqual([]); +}); + +test('getActiveTabs returns a copy of the current active tab IDs when active', () => { + const activeTabs = ['TAB-1', 'TAB-1-1']; + activateDashboard({ dashboardState: { activeTabs } }); + const result = dashboard.getActiveTabs(); + expect(result).toEqual(activeTabs); + expect(result).not.toBe(activeTabs); +}); + +test('updateLayoutNode throws when the dashboard is not active', async () => { + await expect( + dashboard.updateLayoutNode('CHART-1', { width: 6 }), + ).rejects.toThrow('No dashboard is currently active'); +}); + +test('updateLayoutNode throws when the node does not exist', async () => { + activateDashboard(); + await expect( + dashboard.updateLayoutNode('CHART-missing', { width: 6 }), + ).rejects.toThrow('Layout node "CHART-missing" not found'); +}); + +test('updateLayoutNode merges meta into the node and preserves other fields', async () => { + const present = { + 'CHART-1': { + id: 'CHART-1', + type: 'CHART', + meta: { width: 4, height: 50, chartId: 99 }, + }, + }; + activateDashboard({ dashboardLayout: { present } }); + + await dashboard.updateLayoutNode('CHART-1', { width: 6 }); + + expect(updateComponents).toHaveBeenCalledWith({ + 'CHART-1': { + id: 'CHART-1', + type: 'CHART', + meta: { width: 6, height: 50, chartId: 99 }, + }, + }); + expect(mockDispatch).toHaveBeenCalledWith({ + type: 'MOCK_UPDATE_COMPONENTS', + nextComponents: { + 'CHART-1': { + id: 'CHART-1', + type: 'CHART', + meta: { width: 6, height: 50, chartId: 99 }, + }, + }, + }); +}); + +test('getCss returns "" when the dashboard is not active', () => { + expect(dashboard.getCss()).toBe(''); +}); + +test('getCss returns the current dashboard css', () => { + activateDashboard({ dashboardInfo: { id: 1, css: '.foo {}' } }); + expect(dashboard.getCss()).toBe('.foo {}'); +}); + +test('setCss throws when the dashboard is not active', async () => { + await expect(dashboard.setCss('.foo {}')).rejects.toThrow( + 'No dashboard is currently active', + ); +}); + +test('setCss dispatches dashboardInfoChanged with the new css', async () => { + activateDashboard(); + await dashboard.setCss('.foo {}'); + expect(dashboardInfoChanged).toHaveBeenCalledWith({ css: '.foo {}' }); + expect(mockDispatch).toHaveBeenCalledWith({ + type: 'MOCK_DASHBOARD_INFO_CHANGED', + newInfo: { css: '.foo {}' }, + }); +}); + +test('getFilters returns [] when the dashboard is not active', () => { + expect(dashboard.getFilters()).toEqual([]); +}); + +test('getFilters returns only native filters, with their current mask values', () => { + activateDashboard({ + nativeFilters: { + filters: { + 'NATIVE_FILTER-1': { + id: 'NATIVE_FILTER-1', + type: 'NATIVE_FILTER', + name: 'Region', + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'region' } }], + }, + 'DIVIDER-1': { + id: 'DIVIDER-1', + type: 'DIVIDER', + title: 'Section', + }, + }, + }, + dataMask: { + 'NATIVE_FILTER-1': { + extraFormData: { filters: [{ col: 'region', op: 'IN', val: ['US'] }] }, + filterState: { value: ['US'] }, + }, + }, + }); + + expect(dashboard.getFilters()).toEqual([ + { + id: 'NATIVE_FILTER-1', + name: 'Region', + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'region' } }], + extraFormData: { filters: [{ col: 'region', op: 'IN', val: ['US'] }] }, + filterState: { value: ['US'] }, + }, + ]); +}); + +test('updateFilters throws when the dashboard is not active', async () => { + await expect( + dashboard.updateFilters([{ filterId: 'NATIVE_FILTER-1' }]), + ).rejects.toThrow('No dashboard is currently active'); +}); + +test('updateFilters throws when an update references a filter not on this dashboard, and dispatches nothing', async () => { + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + }); + + await expect( + dashboard.updateFilters([ + { filterId: 'NATIVE_FILTER-1', filterState: { value: ['US'] } }, + { filterId: 'NATIVE_FILTER-missing', filterState: { value: ['US'] } }, + ]), + ).rejects.toThrow( + 'Filter "NATIVE_FILTER-missing" not found on this dashboard', + ); + expect(updateDataMask).not.toHaveBeenCalled(); +}); + +test('updateFilters only includes the fields the caller supplied', async () => { + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + }); + + await dashboard.updateFilters([ + { filterId: 'NATIVE_FILTER-1', filterState: { value: ['US'] } }, + ]); + + // A filterState-only update must not clear extraFormData by sending it as + // undefined — that would erase the filter's query-modifying payload. + expect(updateDataMask).toHaveBeenCalledWith('NATIVE_FILTER-1', { + filterState: { value: ['US'] }, + }); +}); + +test('updateFilters applies both extraFormData and filterState when both are supplied', async () => { + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + }); + + const extraFormData = { filters: [{ col: 'region', op: 'IN', val: ['US'] }] }; + await dashboard.updateFilters([ + { + filterId: 'NATIVE_FILTER-1', + extraFormData, + filterState: { value: ['US'] }, + }, + ]); + + expect(updateDataMask).toHaveBeenCalledWith('NATIVE_FILTER-1', { + extraFormData, + filterState: { value: ['US'] }, + }); +}); + +test('refreshChart throws when the dashboard is not active', async () => { + await expect(dashboard.refreshChart(1)).rejects.toThrow( + 'No dashboard is currently active', + ); +}); + +test('refreshChart throws when the chart is not on the current dashboard', async () => { + activateDashboard({ charts: {} }); + await expect(dashboard.refreshChart(123)).rejects.toThrow( + 'Chart 123 is not on the current dashboard', + ); + expect(mockClientGet).not.toHaveBeenCalled(); +}); + +test("refreshChart throws when the chart's configuration can't be retrieved", async () => { + activateDashboard({ charts: { 123: { id: 123 } } }); + mockClientGet.mockResolvedValue({ json: { result: [] } }); + + await expect(dashboard.refreshChart(123)).rejects.toThrow( + "Could not load chart 123's current configuration", + ); +}); + +test('refreshChart re-fetches, merges url params, and re-queries the chart', async () => { + activateDashboard({ + charts: { 123: { id: 123, chartStatus: 'rendered' } }, + }); + mockClientGet.mockResolvedValue({ + json: { + result: [ + { + id: 123, + form_data: { + slice_id: 123, + viz_type: 'big_number', + url_params: { foo: 'bar' }, + }, + }, + ], + }, + }); + mockExtractUrlParams.mockReturnValue({ standalone: undefined, baz: 'qux' }); + + await dashboard.refreshChart(123); + + expect(mockClientGet).toHaveBeenCalledWith({ + endpoint: '/api/v1/dashboard/7/charts', + }); + const expectedFormData = { + slice_id: 123, + viz_type: 'big_number', + url_params: { foo: 'bar', standalone: undefined, baz: 'qux' }, + }; + expect(applyDefaultFormData).toHaveBeenCalledWith(expectedFormData); + expect(mockInvalidateChartFormDataCache).toHaveBeenCalledWith(123); + // setChartFormData/triggerQuery are plain, real (unmocked) action + // creators here, so calling them again produces the same comparable + // action object dispatch should have received. + expect(mockDispatch).toHaveBeenNthCalledWith( + 1, + setChartFormData(expectedFormData as unknown as QueryFormData, 123), + ); + expect(mockDispatch).toHaveBeenNthCalledWith(2, triggerQuery(true, 123)); + expect(mockDispatch).toHaveBeenCalledTimes(2); +}); + +test('refreshChart waits for the chart to reach a terminal status before resolving', async () => { + const charts: Record<number, { id: number; chartStatus?: string }> = { + 123: { id: 123, chartStatus: 'loading' }, + }; + activateDashboard({ charts }); + mockClientGet.mockResolvedValue({ + json: { + result: [ + { id: 123, form_data: { slice_id: 123, viz_type: 'big_number' } }, + ], + }, + }); + const unsubscribe = jest.fn(); + mockStoreSubscribe.mockImplementation(listener => { + // Redux never calls a listener synchronously from within subscribe() + // itself — only after some later dispatch — so defer this like a real + // store would, to also exercise the returned unsubscribe being called. + queueMicrotask(() => { + charts[123].chartStatus = 'rendered'; + listener(); + }); + return unsubscribe; + }); + + await dashboard.refreshChart(123); + + expect(mockStoreSubscribe).toHaveBeenCalled(); + expect(unsubscribe).toHaveBeenCalled(); +}); + +test('refreshChart resolves for a virtualized chart whose query succeeds without rendering', async () => { + const charts: Record<number, { id: number; chartStatus?: string }> = { + 123: { id: 123, chartStatus: 'loading' }, + }; + activateDashboard({ charts }); + mockClientGet.mockResolvedValue({ + json: { result: [{ id: 123, form_data: { slice_id: 123 } }] }, + }); + mockStoreSubscribe.mockImplementation(listener => { + queueMicrotask(() => { + charts[123].chartStatus = 'success'; + listener(); + }); + return jest.fn(); + }); + + await expect(dashboard.refreshChart(123)).resolves.toBeUndefined(); +}); + +test('refreshChart rejects and unsubscribes when the chart is removed mid-refresh', async () => { + const charts: Record<number, { id: number; chartStatus?: string }> = { + 123: { id: 123, chartStatus: 'loading' }, + }; + activateDashboard({ charts }); + mockClientGet.mockResolvedValue({ + json: { result: [{ id: 123, form_data: { slice_id: 123 } }] }, + }); + const unsubscribe = jest.fn(); + mockStoreSubscribe.mockImplementation(listener => { + queueMicrotask(() => { + delete charts[123]; + listener(); + }); + return unsubscribe; + }); + + await expect(dashboard.refreshChart(123)).rejects.toThrow( + 'Chart 123 was removed before it finished refreshing', + ); + expect(unsubscribe).toHaveBeenCalled(); +}); + +test('refreshChart rejects and unsubscribes when the user navigates away mid-refresh', async () => { + const charts: Record<number, { id: number; chartStatus?: string }> = { + 123: { id: 123, chartStatus: 'loading' }, + }; + activateDashboard({ charts }); + mockClientGet.mockResolvedValue({ + json: { result: [{ id: 123, form_data: { slice_id: 123 } }] }, + }); + const unsubscribe = jest.fn(); + mockStoreSubscribe.mockImplementation(listener => { + queueMicrotask(() => { + mockGetPage.mockReturnValue('home'); + listener(); + }); + return unsubscribe; + }); + + await expect(dashboard.refreshChart(123)).rejects.toThrow( + 'The dashboard changed before chart 123 finished refreshing', + ); + expect(unsubscribe).toHaveBeenCalled(); +}); + +test('refreshChart does not apply a fetched configuration after navigating to another dashboard', async () => { + activateDashboard({ charts: { 123: { id: 123, chartStatus: 'rendered' } } }); + let resolveGet: (value: unknown) => void = () => {}; + mockClientGet.mockReturnValue(new Promise(resolve => (resolveGet = resolve))); + + const pending = dashboard.refreshChart(123); + activateDashboard({ + dashboardInfo: { id: 8, css: '' }, + charts: { 123: { id: 123, chartStatus: 'rendered' } }, + }); + resolveGet({ + json: { result: [{ id: 123, form_data: { slice_id: 123 } }] }, + }); + + await expect(pending).rejects.toThrow( + 'The dashboard changed before chart 123 finished refreshing', + ); + expect(mockDispatch).not.toHaveBeenCalled(); +}); + +test('saveFilters throws when the dashboard is not active', async () => { + await expect( + dashboard.saveFilters([{ filterId: 'NATIVE_FILTER-1', name: 'EMEA' }]), + ).rejects.toThrow('No dashboard is currently active'); +}); + +test('saveFilters throws when an update references a filter not on this dashboard', async () => { + activateDashboard({ nativeFilters: { filters: {} } }); + + await expect( + dashboard.saveFilters([{ filterId: 'NATIVE_FILTER-missing', name: 'x' }]), + ).rejects.toThrow( + 'Filter "NATIVE_FILTER-missing" not found on this dashboard', + ); + expect(mockMakeApi).not.toHaveBeenCalled(); +}); + +test('saveFilters is a no-op when there are no updates or deletions', async () => { + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + }); + + await dashboard.saveFilters([]); + + expect(mockMakeApi).not.toHaveBeenCalled(); + expect(mockApplySavedFilterChanges).not.toHaveBeenCalled(); +}); + +test('saveFilters merges partial updates onto the existing filter and persists them', async () => { + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + }); + const savedFilter = { + ...existingFilter, + name: 'Region (EMEA)', + // The server's copy of scope fields, which the save call itself never + // sent — these must be stripped before the result reaches Redux so + // this session's own calculateScopes output isn't clobbered. + chartsInScope: [1, 2], + tabsInScope: ['TAB-1'], + }; + const mockRequest = jest.fn().mockResolvedValue({ result: [savedFilter] }); + mockMakeApi.mockReturnValue(mockRequest); + + await dashboard.saveFilters([ + { filterId: 'NATIVE_FILTER-1', name: 'Region (EMEA)' }, + ]); + + expect(mockMakeApi).toHaveBeenCalledWith({ + method: 'PUT', + endpoint: '/api/v1/dashboard/7/filters', + }); + const expectedFilterChanges = { + modified: [{ ...existingFilter, name: 'Region (EMEA)' }], + deleted: [], + reordered: [], + }; + expect(mockRequest).toHaveBeenCalledWith(expectedFilterChanges); + expect(mockApplySavedFilterChanges).toHaveBeenCalledWith( + store.dispatch, + expectedFilterChanges, + [savedFilter], + { 'NATIVE_FILTER-1': existingFilter }, + ); +}); + +test('saveFilters restores a filter’s live value after a save that only changed its name', async () => { + // A prior updateFilters() call left a session-only value live in + // dataMask, which applySavedFilterChanges would otherwise wipe back to + // the filter's default since this save includes it in `modified`. + activateDashboard({ + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + dataMask: { + 'NATIVE_FILTER-1': { + extraFormData: { filters: [{ col: 'region', op: 'IN', val: ['US'] }] }, + filterState: { value: ['US'] }, + }, + }, + }); + const mockRequest = jest + .fn() + .mockResolvedValue({ result: [{ ...existingFilter, name: 'EMEA' }] }); + mockMakeApi.mockReturnValue(mockRequest); + + await dashboard.saveFilters([{ filterId: 'NATIVE_FILTER-1', name: 'EMEA' }]); + + expect(updateDataMask).toHaveBeenCalledWith('NATIVE_FILTER-1', { + extraFormData: { filters: [{ col: 'region', op: 'IN', val: ['US'] }] }, + filterState: { value: ['US'] }, + }); +}); + +test('saveFilters restores the live value as of the response, not as of the call', async () => { + const state = { + dashboardInfo: { id: 7, css: '' }, + dashboardLayout: { present: {} }, + dashboardState: { activeTabs: [] }, + nativeFilters: { filters: { 'NATIVE_FILTER-1': existingFilter } }, + dataMask: { + 'NATIVE_FILTER-1': { filterState: { value: ['US'] } }, + } as Record<string, unknown>, + charts: {}, + }; + activateDashboard(state); + let resolveRequest: (value: unknown) => void = () => {}; + mockMakeApi.mockReturnValue( + jest.fn(() => new Promise(resolve => (resolveRequest = resolve))), + ); + + const pending = dashboard.saveFilters([ + { filterId: 'NATIVE_FILTER-1', name: 'EMEA' }, + ]); + // The user changes the filter while the request is in flight. Review Comment: This assigns `state.dataMask['NATIVE_FILTER-1']` on the same `dataMask` object the code under test receives from `getState()`. Capturing `currentDataMask` before the PUT would therefore still observe `['CA']`, so moving the snapshot back before the request would not fail this test. Real updates replace the mapping immutably. Could the test swap in a new `dataMask` object while the request is pending, so it distinguishes a pre-request snapshot from the post-response one? ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -0,0 +1,354 @@ +/** + * 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 { dashboard as dashboardApi } from '@apache-superset/core'; +import { isNativeFilter, makeApi, SupersetClient } from '@superset-ui/core'; +import type { + DataMask, + Divider, + Filter, + JsonObject, + QueryFormData, +} from '@superset-ui/core'; +import { matchPath } from 'react-router-dom'; +import { updateComponents } from 'src/dashboard/actions/dashboardLayout'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; +import { applySavedFilterChanges } from 'src/dashboard/actions/nativeFilters'; +import type { SaveFilterChangesType } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/types'; +import { updateDataMask } from 'src/dataMask/actions'; +import { + setChartFormData, + triggerQuery, +} from 'src/components/Chart/chartAction'; +import { invalidateChartFormDataCache } from 'src/dashboard/util/charts/getFormDataWithExtraFilters'; +import { applyDefaultFormData } from 'src/explore/store'; +import extractUrlParams from 'src/dashboard/util/extractUrlParams'; +import { RoutePaths } from 'src/views/routePaths'; +import { store, RootState } from 'src/views/store'; +import { navigation } from '../navigation'; + +const getState = () => store.getState() as RootState; + +// The `:idOrSlug` the browser's current URL routes to, read directly off +// `window.location` rather than from React Router context, matching how +// `navigation`'s own page derivation works. +const getRoutedIdOrSlug = (): string | undefined => + matchPath<{ idOrSlug: string }>(window.location.pathname, { + path: RoutePaths.DASHBOARD, + exact: false, + })?.params.idOrSlug; + +// `dashboardInfo`/`dashboardLayout`/`nativeFilters`/`charts` are retained +// across an in-SPA navigation, and even once the browser has routed to a new +// dashboard, they keep the *previous* dashboard's data until that +// dashboard's HYDRATE_DASHBOARD completes. Comparing the URL's `idOrSlug` +// against `dashboardInfo`'s own id/slug — rather than trusting the page type +// alone — closes this window: they only match again once hydration has +// actually caught up, including for a same-surface dashboard-to-dashboard +// navigation. +const isDashboardActive = (): boolean => { + if (navigation.getPage() !== 'dashboard') return false; + const idOrSlug = getRoutedIdOrSlug(); + if (idOrSlug == null) return false; + const { id, slug } = getState().dashboardInfo; + return id != null && (String(id) === idOrSlug || slug === idOrSlug); +}; + +// True only while the dashboard identified by `dashboardId` is still the one +// on screen. An async API call captures its dashboard's id up front and +// rechecks it after each await, so a navigation that completes in between +// can't have a stale response applied to the dashboard now being shown. +const isStillActiveDashboard = (dashboardId: number): boolean => + isDashboardActive() && getState().dashboardInfo.id === dashboardId; + +const requireDashboardId = (): number => { + const { id } = getState().dashboardInfo; + if (!isDashboardActive() || id == null) { + throw new Error('No dashboard is currently active'); + } + return id; +}; + +const getDashboardId: typeof dashboardApi.getDashboardId = () => + isDashboardActive() ? (getState().dashboardInfo.id ?? undefined) : undefined; + +// Every dashboardLayout reducer treats a node (and its `meta`/`children`/ +// `parents`) as immutable already — an update always replaces it with a new +// object rather than mutating it in place — so freezing the copy below only +// makes that existing contract explicit, without risking a reducer's own +// future in-place update on a node returned from here. +function deepFreeze<T>(value: T): T { + if (value !== null && typeof value === 'object' && !Object.isFrozen(value)) { + Object.freeze(value); + Object.values(value as Record<string, unknown>).forEach(deepFreeze); + } + return value; +} + +const getLayout: typeof dashboardApi.getLayout = () => + isDashboardActive() + ? deepFreeze({ ...getState().dashboardLayout.present }) Review Comment: `{ ...present }` is only a shallow copy, so `deepFreeze` freezes the node and `meta` objects that the hydration payload shares with the store. `hydrateDashboard` later writes `layout[layoutId].meta.sliceName = ...` into those same nodes, and `useDashboardVersionPreview` re-hydrates from its cached live payload. With Version History on: open and close a preview, call `getLayout()`, then open and close another preview, and the second restore can throw a `TypeError` on the frozen `meta`. Could this clone the nodes before freezing them? -- 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]
