This is an automated email from the ASF dual-hosted git repository. EnxDev pushed a commit to branch test/chatbot-local in repository https://gitbox.apache.org/repos/asf/superset.git
commit 23d4574caf498f4470472b7dca91cf1e2bac642f Author: Enzo Martellucci <[email protected]> AuthorDate: Thu May 28 00:12:30 2026 +0200 fix(extensions): address context-sharing PR review — blockers, quality, and tests - Add CREATE_NEW_SLICE and SLICE_UPDATED to exploreChangePredicate so onDidChangeChart fires when a chart is saved for the first time - Remove dataset namespace: no producer exists yet; ships it once a caller is wired in DatasetCreation or equivalent - Remove ...supersetCore spread from window.superset assignment so un-contracted symbols from @apache-superset/core are not leaked onto the global object; list namespaces explicitly instead - Add defensive array copy for filter values in buildDashboardContext so extension mutations cannot affect Redux state - Lazy-initialize currentPageType in navigation to avoid module-load window.location access (throws in non-browser test environments) - Fix /sqllab exact-match missing from derivePageType - Add unit tests: navigation (7), explore (9), dashboard (11) — 27 tests covering page-type gating, dispose semantics, predicate coverage, and defensive copy invariant Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- superset-frontend/src/core/dashboard/index.test.ts | 168 +++++++++++++++++++++ superset-frontend/src/core/dashboard/index.ts | 13 +- superset-frontend/src/core/explore/index.test.ts | 157 +++++++++++++++++++ superset-frontend/src/core/explore/index.ts | 6 +- superset-frontend/src/core/index.ts | 1 - .../src/core/navigation/index.test.ts | 89 +++++++++++ superset-frontend/src/core/navigation/index.ts | 16 +- .../src/extensions/ExtensionsStartup.tsx | 15 +- 8 files changed, 443 insertions(+), 22 deletions(-) diff --git a/superset-frontend/src/core/dashboard/index.test.ts b/superset-frontend/src/core/dashboard/index.test.ts new file mode 100644 index 00000000000..f5078c2e7c1 --- /dev/null +++ b/superset-frontend/src/core/dashboard/index.test.ts @@ -0,0 +1,168 @@ +/** + * 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. + */ + +// --------------------------------------------------------------------------- +// Captured listeners — allows tests to trigger action notifications manually. +// --------------------------------------------------------------------------- +type ListenerEntry = { + predicate: (action: { type: string }) => boolean; + effect: (action: { type: string }) => void; +}; + +const capturedListeners: ListenerEntry[] = []; + +// Declared before jest.mock so the factory closure can reference it. +let mockState: Record<string, unknown>; + +jest.mock('src/views/store', () => ({ + store: { getState: () => mockState, dispatch: jest.fn() }, + listenerMiddleware: { + startListening: (opts: { + predicate: (action: { type: string }) => boolean; + effect: (action: { type: string }) => void; + }) => { + const entry = { predicate: opts.predicate, effect: opts.effect }; + capturedListeners.push(entry); + return () => { + const idx = capturedListeners.indexOf(entry); + if (idx !== -1) capturedListeners.splice(idx, 1); + }; + }, + }, +})); + +jest.mock('../navigation', () => ({ + navigation: { getPageType: jest.fn(() => 'dashboard') }, +})); + +function dispatch(actionType: string) { + const action = { type: actionType }; + capturedListeners + .filter(e => e.predicate(action)) + .forEach(e => e.effect(action)); +} + +// Imported after mocks +// eslint-disable-next-line import/first +import { dashboard } from './index'; + +function makeState( + overrides: Partial<{ + dashboardInfo: unknown; + nativeFilters: unknown; + dataMask: unknown; + }> = {}, +) { + return { + dashboardInfo: { id: 1, dashboard_title: 'Sales', slug: 'sales' }, + nativeFilters: { filters: { 'filter-1': { name: 'Region' } } }, + dataMask: { 'filter-1': { filterState: { value: ['West'] } } }, + ...overrides, + }; +} + +beforeEach(() => { + mockState = makeState(); +}); + +afterEach(() => { + capturedListeners.length = 0; + jest.restoreAllMocks(); +}); + +test('getCurrentDashboard returns undefined when not on dashboard page', () => { + const { navigation } = jest.requireMock('../navigation'); + (navigation.getPageType as jest.Mock).mockReturnValueOnce('explore'); + expect(dashboard.getCurrentDashboard()).toBeUndefined(); +}); + + +test('getCurrentDashboard returns undefined when dashboardInfo is absent', () => { + mockState = makeState({ dashboardInfo: undefined }); + expect(dashboard.getCurrentDashboard()).toBeUndefined(); +}); + +test('getCurrentDashboard returns dashboard context with active filters', () => { + expect(dashboard.getCurrentDashboard()).toEqual({ + dashboardId: 1, + title: 'Sales', + filters: [{ filterId: 'filter-1', label: 'Region', value: ['West'] }], + }); +}); + +test('getCurrentDashboard excludes filters with null value', () => { + mockState = makeState({ + dataMask: { 'filter-1': { filterState: { value: null } } }, + }); + expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0); +}); + +test('getCurrentDashboard excludes dataMask entries not in nativeFilters', () => { + mockState = makeState({ + dataMask: { 'chart-filter': { filterState: { value: 'foo' } } }, + }); + expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0); +}); + +test('filter array value is a defensive copy — mutation does not affect Redux state', () => { + const ctx = dashboard.getCurrentDashboard(); + const original = [...(mockState as any).dataMask['filter-1'].filterState.value]; + (ctx!.filters[0].value as string[]).push('East'); + expect((mockState as any).dataMask['filter-1'].filterState.value).toEqual(original); +}); + +// Action type strings match the constants in src/dashboard/actions/hydrate +// and src/dataMask/actions — kept as literals so this test file has no +// import dependency on those modules. +test.each([ + 'HYDRATE_DASHBOARD', + 'UPDATE_DATA_MASK', + 'SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE', +])('onDidChangeDashboard fires on action type %s', actionType => { + const listener = jest.fn(); + const disposable = dashboard.onDidChangeDashboard(listener); + + dispatch(actionType); + + expect(listener).toHaveBeenCalledWith( + expect.objectContaining({ dashboardId: 1, title: 'Sales' }), + ); + disposable.dispose(); +}); + +test('onDidChangeDashboard does not fire when not on dashboard page', () => { + const { navigation } = jest.requireMock('../navigation'); + (navigation.getPageType as jest.Mock).mockReturnValue('explore'); + + const listener = jest.fn(); + const disposable = dashboard.onDidChangeDashboard(listener); + dispatch('HYDRATE_DASHBOARD'); + + expect(listener).not.toHaveBeenCalled(); + (navigation.getPageType as jest.Mock).mockReturnValue('dashboard'); + disposable.dispose(); +}); + +test('disposed listener is not called', () => { + const listener = jest.fn(); + const disposable = dashboard.onDidChangeDashboard(listener); + disposable.dispose(); + dispatch('HYDRATE_DASHBOARD'); + expect(listener).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/core/dashboard/index.ts b/superset-frontend/src/core/dashboard/index.ts index 611a4514f0b..dbfcdfac85c 100644 --- a/superset-frontend/src/core/dashboard/index.ts +++ b/superset-frontend/src/core/dashboard/index.ts @@ -54,11 +54,14 @@ function buildDashboardContext(): DashboardContext | undefined { const value = mask?.filterState?.value; return value !== null && value !== undefined; }) - .map(([id, mask]: [string, any]) => ({ - filterId: id, - label: nativeFilters[id]?.name ?? id, - value: mask.filterState.value, - })); + .map(([id, mask]: [string, any]) => { + const raw = mask.filterState.value; + return { + filterId: id, + label: nativeFilters[id]?.name ?? id, + value: Array.isArray(raw) ? [...raw] : raw, + }; + }); return { dashboardId: info.id as number, diff --git a/superset-frontend/src/core/explore/index.test.ts b/superset-frontend/src/core/explore/index.test.ts new file mode 100644 index 00000000000..744478bc956 --- /dev/null +++ b/superset-frontend/src/core/explore/index.test.ts @@ -0,0 +1,157 @@ +/** + * 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. + */ + +// --------------------------------------------------------------------------- +// Captured listeners — allows tests to trigger action notifications manually. +// --------------------------------------------------------------------------- +type ListenerEntry = { + predicate: (action: { type: string }) => boolean; + effect: (action: { type: string }) => void; +}; + +const capturedListeners: ListenerEntry[] = []; + +// Declared before jest.mock so the factory closure can reference it. +let mockState: Record<string, unknown>; + +jest.mock('src/views/store', () => ({ + store: { getState: () => mockState, dispatch: jest.fn() }, + listenerMiddleware: { + startListening: (opts: { + predicate: (action: { type: string }) => boolean; + effect: (action: { type: string }) => void; + }) => { + const entry = { predicate: opts.predicate, effect: opts.effect }; + capturedListeners.push(entry); + return () => { + const idx = capturedListeners.indexOf(entry); + if (idx !== -1) capturedListeners.splice(idx, 1); + }; + }, + }, +})); + +jest.mock('../navigation', () => ({ + navigation: { getPageType: jest.fn(() => 'explore') }, +})); + +function dispatch(actionType: string) { + const action = { type: actionType }; + capturedListeners + .filter(e => e.predicate(action)) + .forEach(e => e.effect(action)); +} + +// Imported after mocks +// eslint-disable-next-line import/first +import { explore } from './index'; + +beforeEach(() => { + mockState = { + explore: { + slice: { slice_id: 42, slice_name: 'My Chart' }, + datasource: { id: 7, table_name: 'orders' }, + controls: { viz_type: { value: 'bar' } }, + sliceName: 'My Chart', + form_data: {}, + }, + }; +}); + +afterEach(() => { + capturedListeners.length = 0; + jest.restoreAllMocks(); +}); + +test('getCurrentChart returns undefined when not on explore page', () => { + const { navigation } = jest.requireMock('../navigation'); + (navigation.getPageType as jest.Mock).mockReturnValueOnce('dashboard'); + expect(explore.getCurrentChart()).toBeUndefined(); +}); + +test('getCurrentChart returns undefined when explore state is absent', () => { + mockState = {}; + expect(explore.getCurrentChart()).toBeUndefined(); +}); + +test('getCurrentChart returns chart context from Redux state', () => { + expect(explore.getCurrentChart()).toEqual({ + chartId: 42, + chartName: 'My Chart', + vizType: 'bar', + datasourceId: 7, + datasourceName: 'orders', + }); +}); + +test('getCurrentChart returns null chartId for unsaved chart', () => { + mockState = { + explore: { + slice: null, + datasource: { id: 1, table_name: 'events' }, + controls: { viz_type: { value: 'line' } }, + sliceName: null, + form_data: { viz_type: 'line' }, + }, + }; + expect(explore.getCurrentChart()?.chartId).toBeNull(); +}); + +// Action type strings match the constants in src/explore/actions/exploreActions +// and src/explore/actions/datasourcesActions — kept as literals so this test +// file has no import dependency on those modules. +test.each([ + 'HYDRATE_EXPLORE', + 'UPDATE_FORM_DATA', // SET_FORM_DATA constant resolves to this string + 'UPDATE_CHART_TITLE', + 'SET_DATASOURCE', + 'CREATE_NEW_SLICE', + 'SLICE_UPDATED', +])('onDidChangeChart fires on action type %s', actionType => { + const listener = jest.fn(); + const disposable = explore.onDidChangeChart(listener); + + dispatch(actionType); + + expect(listener).toHaveBeenCalledWith( + expect.objectContaining({ chartId: 42, vizType: 'bar' }), + ); + disposable.dispose(); +}); + +test('onDidChangeChart does not fire when page type is not explore', () => { + const { navigation } = jest.requireMock('../navigation'); + (navigation.getPageType as jest.Mock).mockReturnValue('dashboard'); + + const listener = jest.fn(); + const disposable = explore.onDidChangeChart(listener); + dispatch('HYDRATE_EXPLORE'); + + expect(listener).not.toHaveBeenCalled(); + (navigation.getPageType as jest.Mock).mockReturnValue('explore'); + disposable.dispose(); +}); + +test('disposed listener is not called', () => { + const listener = jest.fn(); + const disposable = explore.onDidChangeChart(listener); + disposable.dispose(); + dispatch('HYDRATE_EXPLORE'); + expect(listener).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/core/explore/index.ts b/superset-frontend/src/core/explore/index.ts index 732ae975caa..2e09ffd34cc 100644 --- a/superset-frontend/src/core/explore/index.ts +++ b/superset-frontend/src/core/explore/index.ts @@ -27,7 +27,9 @@ import type { explore as exploreApi } from '@apache-superset/core'; import { HYDRATE_EXPLORE } from 'src/explore/actions/hydrateExplore'; import { + CREATE_NEW_SLICE, SET_FORM_DATA, + SLICE_UPDATED, UPDATE_CHART_TITLE, } from 'src/explore/actions/exploreActions'; import { SET_DATASOURCE } from 'src/explore/actions/datasourcesActions'; @@ -64,7 +66,9 @@ const exploreChangePredicate: AnyListenerPredicate<RootState> = action => action.type === HYDRATE_EXPLORE || action.type === SET_FORM_DATA || action.type === UPDATE_CHART_TITLE || - action.type === SET_DATASOURCE; + action.type === SET_DATASOURCE || + action.type === CREATE_NEW_SLICE || + action.type === SLICE_UPDATED; const getCurrentChart: typeof exploreApi.getCurrentChart = () => buildChartContext(); diff --git a/superset-frontend/src/core/index.ts b/superset-frontend/src/core/index.ts index d259597457c..bc170079057 100644 --- a/superset-frontend/src/core/index.ts +++ b/superset-frontend/src/core/index.ts @@ -29,7 +29,6 @@ export const core: typeof coreType = { export * from './authentication'; export * from './commands'; export * from './dashboard'; -export * from './dataset'; export * from './editors'; export * from './explore'; export * from './extensions'; diff --git a/superset-frontend/src/core/navigation/index.test.ts b/superset-frontend/src/core/navigation/index.test.ts new file mode 100644 index 00000000000..a8c570153fd --- /dev/null +++ b/superset-frontend/src/core/navigation/index.test.ts @@ -0,0 +1,89 @@ +/** + * 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. + */ + +// Reset module state between tests so currentPageType is re-initialized. +beforeEach(() => { + jest.resetModules(); + Object.defineProperty(window, 'location', { + writable: true, + value: { pathname: '/' }, + }); +}); + +async function importNavigation() { + const mod = await import('./index'); + return mod; +} + +test('getPageType returns "other" for unknown pathname', async () => { + const { navigation } = await importNavigation(); + expect(navigation.getPageType()).toBe('other'); +}); + +test('getPageType derives page type from window.location.pathname', async () => { + window.location.pathname = '/superset/dashboard/42/'; + const { navigation } = await importNavigation(); + expect(navigation.getPageType()).toBe('dashboard'); +}); + +test('notifyPageChange updates the current page type', async () => { + const { navigation, notifyPageChange } = await importNavigation(); + notifyPageChange('/explore/?form_data={}'); + expect(navigation.getPageType()).toBe('explore'); +}); + +test('notifyPageChange fires listeners on page type change', async () => { + const { navigation, notifyPageChange } = await importNavigation(); + const listener = jest.fn(); + const disposable = navigation.onDidChangePage(listener); + + notifyPageChange('/superset/dashboard/1/'); + expect(listener).toHaveBeenCalledWith('dashboard'); + + disposable.dispose(); +}); + +test('notifyPageChange does not fire listeners when page type is unchanged', async () => { + window.location.pathname = '/superset/dashboard/1/'; + const { navigation, notifyPageChange } = await importNavigation(); + const listener = jest.fn(); + navigation.onDidChangePage(listener); + + notifyPageChange('/superset/dashboard/2/'); + expect(listener).not.toHaveBeenCalled(); +}); + +test('onDidChangePage listener is removed after dispose', async () => { + const { navigation, notifyPageChange } = await importNavigation(); + const listener = jest.fn(); + const disposable = navigation.onDidChangePage(listener); + + disposable.dispose(); + notifyPageChange('/superset/dashboard/1/'); + expect(listener).not.toHaveBeenCalled(); +}); + +test('sqllab path is matched with and without trailing slash', async () => { + const { notifyPageChange, navigation } = await importNavigation(); + notifyPageChange('/sqllab'); + expect(navigation.getPageType()).toBe('sqllab'); + notifyPageChange('/explore/'); + notifyPageChange('/sqllab/history'); + expect(navigation.getPageType()).toBe('sqllab'); +}); diff --git a/superset-frontend/src/core/navigation/index.ts b/superset-frontend/src/core/navigation/index.ts index 01859ba6fed..fb9cf9d2b1e 100644 --- a/superset-frontend/src/core/navigation/index.ts +++ b/superset-frontend/src/core/navigation/index.ts @@ -36,23 +36,31 @@ function derivePageType(pathname: string): PageType { if (pathname.startsWith('/explore/')) return 'explore'; if (pathname.startsWith('/superset/explore/')) return 'explore'; if (pathname.startsWith('/chart/add')) return 'explore'; - if (pathname === '/sqllab' || pathname.startsWith('/sqllab/')) return 'sqllab'; + if (pathname === '/sqllab' || pathname.startsWith('/sqllab/')) + return 'sqllab'; if (pathname.startsWith('/dataset/')) return 'dataset'; if (pathname.startsWith('/superset/welcome/')) return 'home'; return 'other'; } -let currentPageType: PageType = derivePageType(window.location.pathname); +let currentPageType: PageType | undefined; + +function getOrInitPageType(): PageType { + if (currentPageType === undefined) { + currentPageType = derivePageType(window.location.pathname); + } + return currentPageType; +} /** Called by ExtensionsStartup whenever the React Router location changes. */ export const notifyPageChange = (pathname: string): void => { const next = derivePageType(pathname); - if (next === currentPageType) return; + if (next === getOrInitPageType()) return; currentPageType = next; listeners.forEach(fn => fn(next)); }; -const getPageType: typeof navigationApi.getPageType = () => currentPageType; +const getPageType: typeof navigationApi.getPageType = () => getOrInitPageType(); const onDidChangePage: typeof navigationApi.onDidChangePage = ( listener: (pageType: PageType) => void, diff --git a/superset-frontend/src/extensions/ExtensionsStartup.tsx b/superset-frontend/src/extensions/ExtensionsStartup.tsx index 6b7dfd45e88..26ba1f928de 100644 --- a/superset-frontend/src/extensions/ExtensionsStartup.tsx +++ b/superset-frontend/src/extensions/ExtensionsStartup.tsx @@ -18,8 +18,6 @@ */ import { useEffect, useRef, useState } from 'react'; import { useLocation } from 'react-router-dom'; -// eslint-disable-next-line no-restricted-syntax -import * as supersetCore from '@apache-superset/core'; import { logging } from '@apache-superset/core/utils'; import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; import { @@ -27,7 +25,6 @@ import { core, commands, dashboard, - dataset, editors, explore, extensions, @@ -48,7 +45,6 @@ declare global { core: typeof core; commands: typeof commands; dashboard: typeof dashboard; - dataset: typeof dataset; editors: typeof editors; explore: typeof explore; extensions: typeof extensions; @@ -84,10 +80,7 @@ const ExtensionsStartup: React.FC<{ children?: React.ReactNode }> = ({ // browser's default error surfacing so host error reporting is unaffected. useEffect(() => { const handleUnhandledRejection = (event: PromiseRejectionEvent) => { - logging.error( - '[extensions] Unhandled rejection:', - event.reason, - ); + logging.error('[extensions] Unhandled rejection:', event.reason); }; window.addEventListener('unhandledrejection', handleUnhandledRejection); return () => { @@ -107,14 +100,14 @@ const ExtensionsStartup: React.FC<{ children?: React.ReactNode }> = ({ return; } - // Provide the implementations for @apache-superset/core + // Provide the implementations for @apache-superset/core. + // Namespaces are listed explicitly — do not spread the core package here, + // as that would leak un-contracted symbols onto window.superset. window.superset = { - ...supersetCore, authentication, core, commands, dashboard, - dataset, editors, explore, extensions,
