diff --git a/superset-frontend/src/explore/actions/hydrateExplore.ts b/superset-frontend/src/explore/actions/hydrateExplore.ts index 0639ff9f949..cb51686b955 100644 --- a/superset-frontend/src/explore/actions/hydrateExplore.ts +++ b/superset-frontend/src/explore/actions/hydrateExplore.ts @@ -211,7 +211,7 @@ export const hydrateExplore = sliceFormData, queryController: null, queriesResponse: null, - triggerQuery: false, + triggerQuery: !!saveAction, lastRendered: 0, }; diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx index 848e20f2f18..85d875aefb9 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx @@ -26,7 +26,8 @@ import { VizType, } from '@superset-ui/core'; import { QUERY_MODE_REQUISITES } from 'src/explore/constants'; -import { MemoryRouter, Route } from 'react-router-dom'; +import { Router, Route } from 'react-router-dom'; +import { createMemoryHistory } from 'history'; import { render, screen, @@ -124,11 +125,13 @@ const renderWithRouter = ({ overridePathname, initialState = reduxState, store, + history: existingHistory, }: { search?: string; overridePathname?: string; initialState?: object; store?: Store; + history?: ReturnType; } = {}) => { const path = overridePathname ?? defaultPath; Object.defineProperty(window, 'location', { @@ -136,14 +139,18 @@ const renderWithRouter = ({ return { pathname: path, search }; }, }); - return render( - + const history = + existingHistory ?? + createMemoryHistory({ initialEntries: [`${path}${search}`] }); + const result = render( + - , + , { useRedux: true, useDnd: true, initialState, store }, ); + return { ...result, history }; }; test('generates a new form_data param when none is available', async () => { @@ -155,19 +162,18 @@ test('generates a new form_data param when none is available', async () => { useLegacyApi: false, }), ); - const replaceState = jest.spyOn(window.history, 'replaceState'); - await waitFor(() => renderWithRouter()); - expect(replaceState).toHaveBeenCalledWith( - expect.anything(), - undefined, + const history = createMemoryHistory({ initialEntries: [defaultPath] }); + const replaceSpy = jest.spyOn(history, 'replace'); + await waitFor(() => renderWithRouter({ history })); + expect(replaceSpy).toHaveBeenCalledWith( expect.stringMatching('form_data_key'), - ); - expect(replaceState).toHaveBeenCalledWith( expect.anything(), - undefined, - expect.stringMatching('datasource_id'), ); - replaceState.mockRestore(); + expect(replaceSpy).toHaveBeenCalledWith( + expect.stringMatching('datasource_id'), + expect.anything(), + ); + replaceSpy.mockRestore(); }); test('renders chart in standalone mode', () => { @@ -180,40 +186,37 @@ test('renders chart in standalone mode', () => { expect(queryByTestId('standalone-app')).toBeInTheDocument(); }); -test('generates a different form_data param when one is provided and is mounting', async () => { - const replaceState = jest.spyOn(window.history, 'replaceState'); - await waitFor(() => renderWithRouter({ search: SEARCH })); - expect(replaceState).not.toHaveBeenLastCalledWith( - 0, - expect.anything(), - undefined, - expect.stringMatching(KEY), - ); - expect(replaceState).toHaveBeenCalledWith( - expect.anything(), - undefined, +test('generates a form_data param with datasource_id when mounting with existing key', async () => { + const history = createMemoryHistory({ + initialEntries: [`${defaultPath}${SEARCH}`], + }); + const replaceSpy = jest.spyOn(history, 'replace'); + await waitFor(() => renderWithRouter({ search: SEARCH, history })); + expect(replaceSpy).toHaveBeenCalledWith( expect.stringMatching('datasource_id'), + expect.anything(), ); - replaceState.mockRestore(); + replaceSpy.mockRestore(); }); test('reuses the same form_data param when updating', async () => { getChartControlPanelRegistry().registerValue('table', { controlPanelSections: [], }); - const replaceState = jest.spyOn(window.history, 'replaceState'); - const pushState = jest.spyOn(window.history, 'pushState'); - await waitFor(() => renderWithRouter({ search: SEARCH })); - expect(replaceState.mock.calls.length).toBe(1); + const history = createMemoryHistory({ + initialEntries: [`${defaultPath}${SEARCH}`], + }); + const replaceSpy = jest.spyOn(history, 'replace'); + await waitFor(() => renderWithRouter({ search: SEARCH, history })); + expect(replaceSpy.mock.calls.length).toBe(1); userEvent.click(screen.getByText('Update chart')); - await waitFor(() => expect(pushState.mock.calls.length).toBe(1)); - expect(replaceState.mock.calls[0]).toEqual(pushState.mock.calls[0]); - replaceState.mockRestore(); - pushState.mockRestore(); + await waitFor(() => expect(replaceSpy.mock.calls.length).toBe(2)); + expect(replaceSpy.mock.calls[0]).toEqual(replaceSpy.mock.calls[1]); + replaceSpy.mockRestore(); getChartControlPanelRegistry().remove('table'); }); -test('doesnt call replaceState when pathname is not /explore', async () => { +test('doesnt call replace when pathname is not /explore', async () => { getChartMetadataRegistry().registerValue( 'table', new ChartMetadata({ @@ -222,24 +225,29 @@ test('doesnt call replaceState when pathname is not /explore', async () => { useLegacyApi: false, }), ); - const replaceState = jest.spyOn(window.history, 'replaceState'); - await waitFor(() => renderWithRouter({ overridePathname: '/dashboard' })); - expect(replaceState).not.toHaveBeenCalled(); - replaceState.mockRestore(); + const history = createMemoryHistory({ initialEntries: ['/dashboard'] }); + const replaceSpy = jest.spyOn(history, 'replace'); + await waitFor(() => + renderWithRouter({ overridePathname: '/dashboard', history }), + ); + expect(replaceSpy).not.toHaveBeenCalled(); + replaceSpy.mockRestore(); }); test('preserves unknown parameters', async () => { - const replaceState = jest.spyOn(window.history, 'replaceState'); const unknownParam = 'test=123'; + const history = createMemoryHistory({ + initialEntries: [`${defaultPath}${SEARCH}&${unknownParam}`], + }); + const replaceSpy = jest.spyOn(history, 'replace'); await waitFor(() => - renderWithRouter({ search: `${SEARCH}&${unknownParam}` }), + renderWithRouter({ search: `${SEARCH}&${unknownParam}`, history }), ); - expect(replaceState).toHaveBeenCalledWith( - expect.anything(), - undefined, + expect(replaceSpy).toHaveBeenCalledWith( expect.stringMatching(unknownParam), + expect.anything(), ); - replaceState.mockRestore(); + replaceSpy.mockRestore(); }); test('retains query mode requirements when query_mode is enabled', async () => { diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx index 26110471a7f..73d732d0cce 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx @@ -46,6 +46,7 @@ import { t } from '@apache-superset/core/translation'; import { logging } from '@apache-superset/core/utils'; import { debounce, isEqual, isObjectLike, omit, pick } from 'lodash'; import { Resizable } from 're-resizable'; +import { useHistory } from 'react-router-dom'; import { Tooltip } from '@superset-ui/core/components'; import { usePluginContext } from 'src/components'; import { Global } from '@emotion/react'; @@ -63,7 +64,6 @@ import { LOG_ACTIONS_MOUNT_EXPLORER, LOG_ACTIONS_CHANGE_EXPLORE_CONTROLS, } from 'src/logger/LogUtils'; -import { ensureAppRoot } from 'src/utils/pathUtils'; import { getUrlParam } from 'src/utils/urlUtils'; import cx from 'classnames'; import * as chartActions from 'src/components/Chart/chartAction'; @@ -170,10 +170,11 @@ const updateHistory = debounce( force, title, tabId, + history, ) => { const payload = { ...formData }; const chartId = formData.slice_id; - const params = new URLSearchParams(window.location.search); + const params = new URLSearchParams(history.location.search); const additionalParam = Object.fromEntries(params); if (chartId) { @@ -192,7 +193,6 @@ const updateHistory = debounce( try { let key: string | null | undefined; - let stateModifier: 'replaceState' | 'pushState'; if (isReplace) { key = await postFormData( datasourceId, @@ -201,7 +201,6 @@ const updateHistory = debounce( chartId, tabId, ); - stateModifier = 'replaceState'; } else { key = getUrlParam(URL_PARAMS.formDataKey); if (key) { @@ -214,10 +213,9 @@ const updateHistory = debounce( tabId, ); } - stateModifier = 'pushState'; } // avoid race condition in case user changes route before explore updates the url - if (window.location.pathname.startsWith(ensureAppRoot('/explore'))) { + if (history.location.pathname.startsWith('/explore')) { const url = mountExploreUrl( standalone ? URL_PARAMS.standalone.name : 'base', { @@ -225,8 +223,9 @@ const updateHistory = debounce( ...additionalParam, }, force, + false, ); - window.history[stateModifier](payload, title, url); + history.replace(url, payload); } } catch (e) { logging.warn('Failed at altering browser history', e); @@ -387,6 +386,7 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { getSidebarWidths(LocalStorageKeys.DatasourceWidth), ); const tabId = useTabId(); + const history = useHistory(); const theme = useTheme(); @@ -431,6 +431,7 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { props.force, title, tabId, + history, ); }, [ @@ -441,22 +442,10 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { props.standalone, props.force, tabId, + history, ], ); - const handlePopstate = useCallback(() => { - const formData = window.history.state; - if (formData && Object.keys(formData).length) { - props.actions.setExploreControls(formData); - props.actions.postChartFormData( - formData, - props.force, - props.timeout, - props.chart.id, - ); - } - }, [props.actions, props.chart.id, props.timeout]); - const onQuery = useCallback(() => { props.actions.setForceQuery(false); @@ -526,17 +515,6 @@ function ExploreViewContainer(props: ExploreViewContainerProps) { } }); - const previousHandlePopstate = usePrevious(handlePopstate); - useEffect(() => { - if (previousHandlePopstate) { - window.removeEventListener('popstate', previousHandlePopstate); - } - window.addEventListener('popstate', handlePopstate); - return () => { - window.removeEventListener('popstate', handlePopstate); - }; - }, [handlePopstate, previousHandlePopstate]); - const previousHandleKeyDown = usePrevious(handleKeydown); useEffect(() => { if (previousHandleKeyDown) { diff --git a/superset-frontend/src/explore/components/SaveModal.test.tsx b/superset-frontend/src/explore/components/SaveModal.test.tsx index e04994b1a2a..1c79be556f5 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.tsx +++ b/superset-frontend/src/explore/components/SaveModal.test.tsx @@ -381,7 +381,7 @@ test('removes form_data_key from URL parameters after save', () => { // other parameters should remain expect(result.get('other_param')).toEqual('value'); expect(result.get('slice_id')).toEqual('1'); - expect(result.get('save_action')).toEqual('overwrite'); + expect(result.has('save_action')).toBe(false); }); test('dispatches removeChartState when saving and going to dashboard', async () => { diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index a78b4ce7dc1..5dff843dbbb 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -196,10 +196,7 @@ class SaveModal extends Component { handleRedirect = (windowLocationSearch: string, chart: any) => { const searchParams = new URLSearchParams(windowLocationSearch); - searchParams.set('save_action', this.state.action); - searchParams.delete('form_data_key'); - searchParams.set('slice_id', chart.id.toString()); return searchParams; }; @@ -343,7 +340,9 @@ class SaveModal extends Component { return; } const searchParams = this.handleRedirect(window.location.search, value); - this.props.history.replace(`/explore/?${searchParams.toString()}`); + this.props.history.replace(`/explore/?${searchParams.toString()}`, { + saveAction: this.state.action, + }); this.setState({ isLoading: false }); this.onHide(); diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts index 50204693aab..2301b2c515d 100644 --- a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts +++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts @@ -21,7 +21,7 @@ import { getClientErrorObject } from '@superset-ui/core'; import { useEffect, useRef, useCallback, useState } from 'react'; import { useHistory } from 'react-router-dom'; import { useBeforeUnload } from 'src/hooks/useBeforeUnload'; -import type { Location } from 'history'; +import type { Location, Action } from 'history'; type UseUnsavedChangesPromptProps = { hasUnsavedChanges: boolean; @@ -71,13 +71,23 @@ export const useUnsavedChangesPrompt = ({ }, [onSave]); const blockCallback = useCallback( - ({ - pathname, - state, - }: { - pathname: Location['pathname']; - state: Location['state']; - }) => { + ( + { + pathname, + search, + state, + }: { + pathname: Location['pathname']; + search: Location['search']; + state: Location['state']; + }, + action: Action, + ) => { + // REPLACE actions are URL sync (e.g. updating form_data_key), not navigation + if (action === 'REPLACE') { + return undefined; + } + if (manualSaveRef.current) { manualSaveRef.current = false; return undefined; @@ -85,7 +95,11 @@ export const useUnsavedChangesPrompt = ({ confirmNavigationRef.current = () => { unblockRef.current?.(); - history.push(pathname, state); + if (action === 'POP') { + history.go(-1); + } else { + history.push({ pathname, search }, state); + } }; setShowModal(true); diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx b/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx index ccd259f22f5..889ba89a91e 100644 --- a/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx +++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/useUnsavedChangesPrompt.test.tsx @@ -125,7 +125,7 @@ test('should close modal when handleConfirmNavigation is called', () => { expect(result.current.showModal).toBe(false); }); -test('should preserve pathname and state when confirming navigation', () => { +test('should preserve pathname, search, and state when confirming navigation', () => { const onSave = jest.fn(); const history = createMemoryHistory(); const wrapper = ({ children }: any) => ( @@ -134,6 +134,7 @@ test('should preserve pathname and state when confirming navigation', () => { const locationState = { fromDashboard: true, dashboardId: 123 }; const pathname = '/another-page'; + const search = '?slice_id=42&foo=bar'; const { result } = renderHook( () => useUnsavedChangesPrompt({ hasUnsavedChanges: true, onSave }), @@ -144,7 +145,7 @@ test('should preserve pathname and state when confirming navigation', () => { // Simulate a blocked navigation (the hook sets up history.block internally) act(() => { - history.push(pathname, locationState); + history.push({ pathname, search }, locationState); }); // Modal should now be visible @@ -158,8 +159,8 @@ test('should preserve pathname and state when confirming navigation', () => { // Modal should close expect(result.current.showModal).toBe(false); - // Verify correct call with pathname and state preserved - expect(pushSpy).toHaveBeenCalledWith(pathname, locationState); + // Verify correct call with pathname, search, and state preserved + expect(pushSpy).toHaveBeenCalledWith({ pathname, search }, locationState); pushSpy.mockRestore(); }); diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx b/superset-frontend/src/pages/Chart/Chart.test.tsx index de3787ebde5..58106375f0e 100644 --- a/superset-frontend/src/pages/Chart/Chart.test.tsx +++ b/superset-frontend/src/pages/Chart/Chart.test.tsx @@ -260,7 +260,11 @@ describe('ChartPage', () => { const { getByTestId } = render( <> Change route @@ -298,5 +302,60 @@ describe('ChartPage', () => { }).slice(1, -1), ); }); + + test('re-fetches explore data on back-button navigation (POP)', async () => { + const exploreApiRoute = 'glob:*/api/v1/explore/*'; + const initialFormData = getExploreFormData({ + viz_type: VizType.Table, + show_cell_bars: true, + }); + const updatedFormData = getExploreFormData({ + viz_type: VizType.Table, + show_cell_bars: false, + }); + fetchMock.get(exploreApiRoute, { + result: { dataset: { id: 1 }, form_data: initialFormData }, + }); + render( + <> + Navigate away + + , + { + useRouter: true, + useRedux: true, + useDnd: true, + }, + ); + await waitFor(() => + expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1), + ); + expect(screen.getByTestId('mock-explore-chart-panel')).toHaveTextContent( + JSON.stringify({ show_cell_bars: true }).slice(1, -1), + ); + + // Navigate forward (PUSH) then simulate back-button (POP) + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(exploreApiRoute, { + result: { dataset: { id: 1 }, form_data: updatedFormData }, + }); + fireEvent.click(screen.getByText('Navigate away')); + await waitFor(() => + expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1), + ); + + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(exploreApiRoute, { + result: { dataset: { id: 1 }, form_data: initialFormData }, + }); + // Simulate back button + window.history.back(); + await waitFor(() => + expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1), + ); + expect(screen.getByTestId('mock-explore-chart-panel')).toHaveTextContent( + JSON.stringify({ show_cell_bars: true }).slice(1, -1), + ); + }); }); }); diff --git a/superset-frontend/src/pages/Chart/index.tsx b/superset-frontend/src/pages/Chart/index.tsx index 948abdc47f9..3f8d28ce2dc 100644 --- a/superset-frontend/src/pages/Chart/index.tsx +++ b/superset-frontend/src/pages/Chart/index.tsx @@ -16,9 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { useDispatch } from 'react-redux'; -import { useLocation } from 'react-router-dom'; +import { useHistory } from 'react-router-dom'; +import type { Location, Action } from 'history'; import { t } from '@apache-superset/core/translation'; import { getLabelsColorMap, @@ -128,20 +129,28 @@ const getDashboardContextFormData = () => { export default function ExplorePage() { const [isLoaded, setIsLoaded] = useState(false); - const isExploreInitialized = useRef(false); + const fetchGeneration = useRef(0); const dispatch = useDispatch(); - const location = useLocation(); + const history = useHistory(); - useEffect(() => { - const exploreUrlParams = getParsedExploreURLParams(location); - const saveAction = getUrlParam( - URL_PARAMS.saveAction, - ) as SaveActionType | null; - const dashboardContextFormData = getDashboardContextFormData(); + const loadExploreData = useCallback( + ( + loc: { search: string; pathname: string }, + saveAction?: SaveActionType | null, + ) => { + fetchGeneration.current += 1; + const generation = fetchGeneration.current; + const exploreUrlParams = getParsedExploreURLParams(loc); + const dashboardContextFormData = getDashboardContextFormData(); + + const isStale = () => generation !== fetchGeneration.current; - if (!isExploreInitialized.current || !!saveAction) { fetchExploreData(exploreUrlParams) .then(({ result }) => { + if (isStale()) { + return; + } + const formData = dashboardContextFormData ? getFormDataWithDashboardContext( result.form_data, @@ -176,6 +185,10 @@ export default function ExplorePage() { }) .catch(err => Promise.all([getClientErrorObject(err), err])) .then(resolved => { + if (isStale()) { + return; + } + const [clientError, err] = resolved || []; if (!err) { return Promise.resolve(); @@ -209,6 +222,9 @@ export default function ExplorePage() { ) .then( ({ result: { id, url, owners, form_data: _, ...data } }) => { + if (isStale()) { + return; + } const slice = { ...data, datasource: err.extra?.datasource_name, @@ -225,6 +241,9 @@ export default function ExplorePage() { }, ) .catch(() => { + if (isStale()) { + return; + } dispatch(hydrateExplore(exploreData)); }); } @@ -232,12 +251,39 @@ export default function ExplorePage() { return Promise.resolve(); }) .finally(() => { - setIsLoaded(true); - isExploreInitialized.current = true; + if (!isStale()) { + setIsLoaded(true); + } }); - } + }, + [dispatch], + ); + + // Initial fetch on mount + useEffect(() => { + loadExploreData(history.location); getLabelsColorMap().source = LabelsColorMapSource.Explore; - }, [dispatch, location]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + // Re-fetch on navigation or post-save. + // PUSH/POP: full reload (unmount + re-fetch). + // REPLACE with saveAction state: re-fetch without unmount (keeps chart visible). + // Other REPLACE: ignored (URL sync from updateHistory). + useEffect(() => { + const unlisten = history.listen((loc: Location, action: Action) => { + const saveAction = (loc.state as Record)?.saveAction as + | SaveActionType + | undefined; + if (action === 'PUSH' || action === 'POP') { + setIsLoaded(false); + loadExploreData(loc, saveAction); + } else if (saveAction) { + loadExploreData(loc, saveAction); + } + }); + return unlisten; + }, [history, loadExploreData]); if (!isLoaded) { return ;