fix(explore): migrate from window.history to React Router history API (#38887)

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,
     };
 

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<typeof createMemoryHistory>;
 } = {}) => {
   const path = overridePathname ?? defaultPath;
   Object.defineProperty(window, 'location', {
@@ -136,14 +139,18 @@ const renderWithRouter = ({
       return { pathname: path, search };
     },
   });
-  return render(
-    <MemoryRouter initialEntries={[`${path}${search}`]}>
+  const history =
+    existingHistory ??
+    createMemoryHistory({ initialEntries: [`${path}${search}`] });
+  const result = render(
+    <Router history={history}>
       <Route path={path}>
         <ExploreViewContainer />
       </Route>
-    </MemoryRouter>,
+    </Router>,
     { 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 () => {

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) {

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 () => {

superset-frontend/src/explore/components/SaveModal.tsx

@@ -196,10 +196,7 @@ class SaveModal extends Component<SaveModalProps, SaveModalState> {
 
   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<SaveModalProps, SaveModalState> {
         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();

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);

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();
 });

superset-frontend/src/pages/Chart/Chart.test.tsx

@@ -260,7 +260,11 @@ describe('ChartPage', () => {
       const { getByTestId } = render(
         <>
           <Link
-            to={`/?${URL_PARAMS.dashboardPageId.name}=${dashboardPageId}&${URL_PARAMS.saveAction.name}=overwrite`}
+            to={{
+              pathname: '/',
+              search: `?${URL_PARAMS.dashboardPageId.name}=${dashboardPageId}`,
+              state: { saveAction: 'overwrite' },
+            }}
           >
             Change route
           </Link>
@@ -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(
+        <>
+          <Link to="/?slice_id=99">Navigate away</Link>
+          <ChartPage />
+        </>,
+        {
+          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),
+      );
+    });
   });
 });

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<string, unknown>)?.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 <Loading />;