This is an automated email from the ASF dual-hosted git repository.
msyavuz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 7999b7410f2 fix(explore): restore Back button undo for chart changes
(#42473)
7999b7410f2 is described below
commit 7999b7410f2c6e4fb0e31b18b47eb21dec9c65f3
Author: Mehmet Salih Yavuz <[email protected]>
AuthorDate: Wed Jul 29 13:55:57 2026 +0300
fix(explore): restore Back button undo for chart changes (#42473)
---
.../ExploreChartHeader/ExploreChartHeader.test.tsx | 29 ++++
.../components/ExploreChartHeader/index.tsx | 17 ++
.../ExploreViewContainer.test.tsx | 191 ++++++++++++++++++++-
.../components/ExploreViewContainer/index.tsx | 70 +++++++-
.../explore/exploreUtils/exploreHistory.test.ts | 71 ++++++++
.../src/explore/exploreUtils/exploreHistory.ts | 77 +++++++++
.../src/hooks/useUnsavedChangesPrompt/index.ts | 13 +-
superset-frontend/src/pages/Chart/Chart.test.tsx | 149 ++++++++++++++++
superset-frontend/src/pages/Chart/index.tsx | 21 ++-
9 files changed, 630 insertions(+), 8 deletions(-)
diff --git
a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
index 81aa12fe758..9e0ee9feedf 100644
---
a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
+++
b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
@@ -31,9 +31,11 @@ import * as downloadAsImage from 'src/utils/downloadAsImage';
import * as exploreUtils from 'src/explore/exploreUtils';
import {
FeatureFlag,
+ QueryFormData,
VizType,
getChartMetadataRegistry,
} from '@superset-ui/core';
+import { toChartStateHistoryState } from
'src/explore/exploreUtils/exploreHistory';
import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt';
import ExploreHeader, { ExploreChartHeaderProps } from '.';
import fs from 'fs';
@@ -388,6 +390,33 @@ describe('ExploreChartHeader', () => {
);
});
+ test('treats chart states of the same chart as in place transitions', async
() => {
+ const formData = {
+ viz_type: VizType.Histogram,
+ datasource: '49__table',
+ slice_id: 318,
+ } as QueryFormData;
+ render(<ExploreHeader {...createProps({ formData })} />, {
+ useRedux: true,
+ });
+
+ const [{ isInPlaceTransition }] = (useUnsavedChangesPrompt as jest.Mock)
+ .mock.lastCall;
+
+ expect(
+ isInPlaceTransition(
+ toChartStateHistoryState({ ...formData, row_limit: 10 }),
+ ),
+ ).toBe(true);
+ expect(
+ isInPlaceTransition(
+ toChartStateHistoryState({ ...formData, slice_id: 42 }),
+ ),
+ ).toBe(false);
+ expect(isInPlaceTransition({ fromDashboard: true })).toBe(false);
+ expect(isInPlaceTransition(undefined)).toBe(false);
+ });
+
test('Save chart', async () => {
const setSaveChartModalVisibilitySpy = jest.spyOn(
saveModalActions,
diff --git
a/superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
b/superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
index 679f25c0b18..7f3c734d255 100644
--- a/superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
+++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.tsx
@@ -18,6 +18,7 @@
*/
import { FC, memo, useCallback, useEffect, useMemo, useState } from 'react';
import { useHistory } from 'react-router-dom';
+import type { Location } from 'history';
import { useDispatch } from 'react-redux';
import {
Tooltip,
@@ -45,6 +46,10 @@ import { applyColors, resetColors } from
'src/utils/colorScheme';
import ReportModal from 'src/features/reports/ReportModal';
import { deleteActiveReport } from 'src/features/reports/ReportModal/actions';
import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt';
+import {
+ getChartStateFromHistoryState,
+ isSameChartState,
+} from 'src/explore/exploreUtils/exploreHistory';
import { getChartFormDiffs } from 'src/utils/getChartFormDiffs';
import { StreamingExportModal } from 'src/components/StreamingExportModal';
import { Tag } from 'src/components/Tag';
@@ -243,6 +248,17 @@ const ExploreChartHeader: FC<ExploreChartHeaderProps> = ({
[originalFormData, currentFormData],
);
+ // Explore's own chart-state entries (see updateHistory) keep the user on the
+ // chart, so stepping through them isn't leaving unsaved changes behind.
+ const isChartStateTransition = useCallback(
+ (state: Location['state']) =>
+ isSameChartState(getChartStateFromHistoryState(state), {
+ ...formData,
+ slice_id: formData?.slice_id ?? slice?.slice_id,
+ }),
+ [formData, slice?.slice_id],
+ );
+
const {
showModal: showUnsavedChangesModal,
setShowModal: setShowUnsavedChangesModal,
@@ -256,6 +272,7 @@ const ExploreChartHeader: FC<ExploreChartHeaderProps> = ({
},
isSaveModalVisible,
manualSaveOnUnsavedChanges: true,
+ isInPlaceTransition: isChartStateTransition,
});
const showModal = useCallback(() => {
diff --git
a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
index 24edcc44617..65378889e34 100644
---
a/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
+++
b/superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
@@ -27,9 +27,11 @@ import {
VizType,
} from '@superset-ui/core';
import { QUERY_MODE_REQUISITES } from 'src/explore/constants';
-import { Router, Route } from 'react-router-dom';
+import { MemoryRouter, Route, Router } from 'react-router-dom';
+import type { RouteComponentProps } from 'react-router-dom';
import { createMemoryHistory } from 'history';
import {
+ act,
render,
screen,
userEvent,
@@ -140,15 +142,20 @@ jest.mock('../ControlPanelsContainer', () => ({
onQuery,
buttonErrorMessage,
errorMessage,
+ chartIsStale,
}: {
onQuery: () => void;
buttonErrorMessage?: ReactNode;
errorMessage?: ReactNode;
+ chartIsStale?: boolean;
}) => {
const message = buttonErrorMessage ?? errorMessage;
return (
- <div data-test="control-panels-container">
+ <div
+ data-test="control-panels-container"
+ data-stale={String(!!chartIsStale)}
+ >
<button type="button" onClick={onQuery}>
Update chart
</button>
@@ -327,6 +334,186 @@ test('reuses the same form_data param when updating',
async () => {
getChartControlPanelRegistry().remove('table');
});
+test('pushes a history entry when the chart changed, so Back undoes it', async
() => {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [],
+ });
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: { ...reduxState.explore.controls, row_limit: { value: 100 } },
+ },
+ };
+ const store = createStore(initialState, reducerIndex);
+ const history = createMemoryHistory({
+ initialEntries: [`${defaultPath}${SEARCH}`],
+ });
+ const pushSpy = jest.spyOn(history, 'push');
+ renderWithRouter({
+ search: SEARCH,
+ history,
+ initialState,
+ store: store as Store,
+ });
+ // the entry Back should return to
+ await waitFor(() =>
+ expect(history.location.state).toEqual(
+ expect.objectContaining({ row_limit: 100 }),
+ ),
+ );
+ act(() => {
+ store.dispatch(exploreActions.setControlValue('row_limit', 200));
+ });
+ await userEvent.click(screen.getByText('Update chart'));
+ await waitFor(() =>
+ expect(pushSpy).toHaveBeenCalledWith(
+ expect.stringMatching('form_data_key'),
+ // the chart id is stamped on, the controls don't produce one
+ expect.objectContaining({ row_limit: 200, slice_id: 1 }),
+ ),
+ );
+ pushSpy.mockRestore();
+ getChartControlPanelRegistry().remove('table');
+});
+
+const renderWithMemoryRouter = ({
+ initialState,
+ store,
+}: {
+ initialState: object;
+ store: Store;
+}) => {
+ // MemoryRouter builds the history react-router itself ships, the one Explore
+ // sees at runtime - the top-level `history` package is a different major
+ let routerHistory: RouteComponentProps['history'] | undefined;
+ const result = render(
+ <MemoryRouter initialEntries={[`${defaultPath}${SEARCH}`]}>
+ <Route
+ render={({ history }) => {
+ routerHistory ??= history;
+ return <ExploreViewContainer />;
+ }}
+ />
+ </MemoryRouter>,
+ { useRedux: true, useDnd: true, initialState, store },
+ );
+ return {
+ ...result,
+ routerHistory: routerHistory as RouteComponentProps['history'],
+ };
+};
+
+test('restores the state of a popped entry without leaving the chart stale',
async () => {
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [
+ { label: 'Options', expanded: true, controlSetRows: [['row_limit']] },
+ ],
+ });
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: { ...reduxState.explore.controls, row_limit: { value: 100 } },
+ },
+ };
+ const store = createStore(initialState, reducerIndex);
+ const { routerHistory } = renderWithMemoryRouter({
+ initialState,
+ store: store as Store,
+ });
+ await waitFor(() =>
+ expect(routerHistory.location.state).toEqual(
+ expect.objectContaining({ row_limit: 100 }),
+ ),
+ );
+ act(() => {
+ store.dispatch(exploreActions.setControlValue('row_limit', 200));
+ });
+ expect(screen.getByTestId('control-panels-container')).toHaveAttribute(
+ 'data-stale',
+ 'true',
+ );
+ await userEvent.click(screen.getByText('Update chart'));
+ await waitFor(() =>
+ expect(routerHistory.location.state).toEqual(
+ expect.objectContaining({ row_limit: 200 }),
+ ),
+ );
+
+ act(() => {
+ routerHistory.goBack();
+ });
+ await waitFor(() =>
+ expect(screen.getByTestId('control-panels-container')).toHaveAttribute(
+ 'data-stale',
+ 'false',
+ ),
+ );
+ getChartControlPanelRegistry().remove('table');
+});
+
+test('doesnt push an entry for the state a popped entry restored', async () =>
{
+ getChartControlPanelRegistry().registerValue('table', {
+ controlPanelSections: [
+ { label: 'Options', expanded: true, controlSetRows: [['y_axis_format']]
},
+ ],
+ });
+ const initialState = {
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ form_data: {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ metrics: [],
+ },
+ controls: {
+ ...reduxState.explore.controls,
+ y_axis_format: { value: ',d', renderTrigger: true },
+ },
+ },
+ };
+ const store = createStore(initialState, reducerIndex);
+ const { routerHistory } = renderWithMemoryRouter({
+ initialState,
+ store: store as Store,
+ });
+ await waitFor(() =>
+ expect(routerHistory.location.state).toEqual(
+ expect.objectContaining({ y_axis_format: ',d' }),
+ ),
+ );
+ const pushSpy = jest.spyOn(routerHistory, 'push');
+ // a render-trigger change gets its own entry
+ act(() => {
+ store.dispatch(exploreActions.setControlValue('y_axis_format', '.2f'));
+ });
+ await waitFor(() => expect(pushSpy).toHaveBeenCalledTimes(1));
+
+ act(() => {
+ routerHistory.goBack();
+ });
+ await waitFor(() =>
+ expect(routerHistory.location.state).toEqual(
+ expect.objectContaining({ y_axis_format: ',d' }),
+ ),
+ );
+ expect(pushSpy).toHaveBeenCalledTimes(1);
+ pushSpy.mockRestore();
+ getChartControlPanelRegistry().remove('table');
+});
+
test('doesnt call replace when pathname is not /explore', async () => {
getChartMetadataRegistry().registerValue(
'table',
diff --git
a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
index cd131a97d1b..fa52e944e58 100644
--- a/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
+++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.tsx
@@ -23,10 +23,11 @@ import {
useCallback,
useEffect,
useMemo,
+ useRef,
useState,
} from 'react';
import { bindActionCreators, Dispatch } from 'redux';
-import { connect } from 'react-redux';
+import { connect, shallowEqual, useSelector } from 'react-redux';
import {
useChangeEffect,
useComponentDidMount,
@@ -48,6 +49,7 @@ import { logging } from '@apache-superset/core/utils';
import { debounce, isEqual, isObjectLike, omit, pick } from 'lodash-es';
import { Resizable } from 're-resizable';
import { useHistory } from 'react-router-dom';
+import type { Action, Location } from 'history';
import { ActionButton, Tooltip } from '@superset-ui/core/components';
import { usePluginContext } from 'src/components';
import { Global } from '@emotion/react';
@@ -74,6 +76,12 @@ import { mergeExtraFormData } from
'src/dashboard/components/nativeFilters/utils
import { postFormData, putFormData } from 'src/explore/exploreUtils/formData';
import { datasourcesActions } from 'src/explore/actions/datasourcesActions';
import { mountExploreUrl } from 'src/explore/exploreUtils';
+import {
+ getChartStateFromHistoryState,
+ isSameChartState,
+ selectRestoreTarget,
+ toChartStateHistoryState,
+} from 'src/explore/exploreUtils/exploreHistory';
import { getFormDataFromControls } from 'src/explore/controlUtils';
import * as exploreActions from 'src/explore/actions/exploreActions';
import * as saveModalActions from 'src/explore/actions/saveModalActions';
@@ -173,6 +181,7 @@ const updateHistory = debounce(
title,
tabId,
history,
+ sliceId,
) => {
const payload = { ...formData };
const chartId = formData.slice_id;
@@ -227,7 +236,20 @@ const updateHistory = debounce(
force,
false,
);
- history.replace(url, payload);
+ const previousChartState = getChartStateFromHistoryState(
+ history.location.state,
+ );
+ const state = toChartStateHistoryState(payload, sliceId);
+ if (
+ isReplace ||
+ !previousChartState ||
+ isEqual(previousChartState, getChartStateFromHistoryState(state))
+ ) {
+ history.replace(url, state);
+ } else {
+ // one entry per chart state is what makes the Back button undo it
+ history.push(url, state);
+ }
}
} catch (e) {
logging.warn('Failed at altering browser history', e);
@@ -382,6 +404,9 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
const [lastQueriedControls, setLastQueriedControls] = useState(
props.controls,
);
+ /** set while the chart state of a popped history entry is being applied */
+ const restoringFromHistory = useRef(false);
+ const restoreTarget = useSelector(selectRestoreTarget, shallowEqual);
const [isCollapsed, setIsCollapsed] = useState(false);
const [width, setWidth] = useState(
@@ -479,6 +504,7 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
title,
tabId,
history,
+ restoreTarget.slice_id,
);
},
[
@@ -486,6 +512,7 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
props.form_data,
props.datasource.id,
props.datasource.type,
+ restoreTarget.slice_id,
props.standalone,
props.force,
tabId,
@@ -556,6 +583,9 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
);
});
+ // a pending update would run against whatever chart the user moved on to
+ useEffect(() => () => updateHistory.cancel(), []);
+
useChangeEffect(tabId, (previous, current) => {
if (current) {
addHistory({ isReplace: true });
@@ -600,7 +630,10 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
: getFormDataFromControls(props.controls);
props.actions.updateQueryFormData(newQueryFormData, props.chart.id);
props.actions.renderTriggered(new Date().getTime(), props.chart.id);
- addHistory();
+ if (!restoringFromHistory.current) {
+ // an entry for a state we just stepped back to would break the next
Back
+ addHistory();
+ }
},
[
addHistory,
@@ -611,6 +644,27 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
],
);
+ // Explore's own entries carry the chart state they were pushed with, so a
POP
+ // into one is an undo/redo: apply it and re-query in place, which is also
why
+ // ExplorePage skips its reload for them.
+ useEffect(
+ () =>
+ history.listen((loc: Location, action: Action) => {
+ const chartState = getChartStateFromHistoryState(loc.state);
+ if (
+ action !== 'POP' ||
+ !chartState ||
+ !isSameChartState(chartState, restoreTarget)
+ ) {
+ return;
+ }
+ restoringFromHistory.current = true;
+ props.actions.setExploreControls(chartState);
+ props.actions.triggerQuery(true, props.chart.id);
+ }),
+ [history, restoreTarget, props.actions, props.chart.id],
+ );
+
// effect to run when controls change
useEffect(() => {
if (
@@ -736,6 +790,16 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
}
}, [props.controls, props.ownState]);
+ // A restore queries the controls it just applied, so they are the last
+ // queried ones - otherwise the chart would be reported as stale.
+ // Runs after the effect above so that one still sees the restore.
+ useEffect(() => {
+ if (restoringFromHistory.current) {
+ restoringFromHistory.current = false;
+ setLastQueriedControls(props.controls);
+ }
+ }, [props.controls]);
+
const chartIsStale = useMemo(() => {
if (lastQueriedControls) {
const { controls } = props;
diff --git a/superset-frontend/src/explore/exploreUtils/exploreHistory.test.ts
b/superset-frontend/src/explore/exploreUtils/exploreHistory.test.ts
new file mode 100644
index 00000000000..142bc1fad55
--- /dev/null
+++ b/superset-frontend/src/explore/exploreUtils/exploreHistory.test.ts
@@ -0,0 +1,71 @@
+/**
+ * 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 { QueryFormData, VizType } from '@superset-ui/core';
+import {
+ getChartStateFromHistoryState,
+ isSameChartState,
+ toChartStateHistoryState,
+} from './exploreHistory';
+
+const formData = {
+ datasource: '1__table',
+ viz_type: VizType.Table,
+ slice_id: 7,
+} as QueryFormData;
+
+test('round trips form data through the history state', () => {
+ expect(
+ getChartStateFromHistoryState(toChartStateHistoryState(formData)),
+ ).toEqual(formData);
+});
+
+test('stamps the chart id on form data that carries none', () => {
+ const controlsFormData = { datasource: '1__table' } as QueryFormData;
+ expect(
+ getChartStateFromHistoryState(
+ toChartStateHistoryState(controlsFormData, 7),
+ ),
+ ).toEqual({ ...controlsFormData, slice_id: 7 });
+ expect(
+ getChartStateFromHistoryState(toChartStateHistoryState(formData, 8)),
+ ).toEqual(formData);
+});
+
+test('ignores history states that hold no chart state', () => {
+ expect(getChartStateFromHistoryState(undefined)).toBeUndefined();
+ expect(
+ getChartStateFromHistoryState({ saveAction: 'overwrite' }),
+ ).toBeUndefined();
+ expect(getChartStateFromHistoryState(formData)).toBeUndefined();
+});
+
+test('matches chart states of the same chart and dataset only', () => {
+ expect(isSameChartState(formData, { ...formData, row_limit: 10
})).toBe(true);
+ expect(isSameChartState(formData, { ...formData, slice_id: 8 })).toBe(false);
+ expect(
+ isSameChartState(formData, { ...formData, datasource: '2__table' }),
+ ).toBe(false);
+ expect(isSameChartState(formData, undefined)).toBe(false);
+});
+
+test('treats a missing slice id as the unsaved chart', () => {
+ const newChart = { datasource: '1__table' } as QueryFormData;
+ expect(isSameChartState(newChart, { ...newChart, slice_id: 0 })).toBe(true);
+ expect(isSameChartState(newChart, { ...newChart, slice_id: 7 })).toBe(false);
+});
diff --git a/superset-frontend/src/explore/exploreUtils/exploreHistory.ts
b/superset-frontend/src/explore/exploreUtils/exploreHistory.ts
new file mode 100644
index 00000000000..a43c17167e3
--- /dev/null
+++ b/superset-frontend/src/explore/exploreUtils/exploreHistory.ts
@@ -0,0 +1,77 @@
+/**
+ * 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 { JsonObject, QueryFormData } from '@superset-ui/core';
+import { omit } from 'lodash-es';
+import { UNSAVED_CHART_ID } from 'src/explore/constants';
+import type { ExplorePageState } from 'src/explore/types';
+
+/**
+ * Explore stores the chart's form data in the history entry so that stepping
+ * back through entries restores previous chart states. The marker tells those
+ * entries apart from regular navigation to another page or chart.
+ */
+const CHART_STATE_MARKER = '__exploreChartState';
+
+/**
+ * The chart id has to be stamped on: form data assembled from the controls
+ * carries none, so a state would otherwise not be attributable to its chart.
+ */
+export const toChartStateHistoryState = (
+ formData: QueryFormData,
+ chartId?: number,
+): JsonObject => ({
+ ...formData,
+ slice_id: formData.slice_id ?? chartId,
+ [CHART_STATE_MARKER]: true,
+});
+
+export const getChartStateFromHistoryState = (
+ state: unknown,
+): QueryFormData | undefined =>
+ state &&
+ typeof state === 'object' &&
+ (state as JsonObject)[CHART_STATE_MARKER] === true
+ ? (omit(state as JsonObject, CHART_STATE_MARKER) as QueryFormData)
+ : undefined;
+
+/**
+ * Chart states are only interchangeable within the same chart and dataset -
+ * applying one to another chart would mix the two.
+ */
+/**
+ * The chart a state can be restored into. ExplorePage decides whether to
reload
+ * and ExploreViewContainer whether to restore, so both have to read it here -
+ * disagreeing leaves a POP resolving to neither.
+ */
+export const selectRestoreTarget = (
+ state: ExplorePageState,
+): Partial<QueryFormData> => ({
+ slice_id:
+ state.explore?.slice?.slice_id ?? state.explore?.form_data?.slice_id,
+ datasource: state.explore?.controls?.datasource?.value as string | undefined,
+});
+
+export const isSameChartState = (
+ a?: Partial<QueryFormData>,
+ b?: Partial<QueryFormData>,
+): boolean =>
+ !!a &&
+ !!b &&
+ (a.slice_id ?? UNSAVED_CHART_ID) === (b.slice_id ?? UNSAVED_CHART_ID) &&
+ a.datasource === b.datasource;
diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
index 2301b2c515d..995a8007fed 100644
--- a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
+++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
@@ -28,6 +28,12 @@ type UseUnsavedChangesPromptProps = {
onSave: () => Promise<void> | void;
isSaveModalVisible?: boolean;
manualSaveOnUnsavedChanges?: boolean;
+ /**
+ * Transitions that keep the user on the current page - Explore, for one,
adds
+ * an entry per chart state so that Back undoes it - aren't navigation away
+ * and shouldn't prompt.
+ */
+ isInPlaceTransition?: (state: Location['state']) => boolean;
};
export const useUnsavedChangesPrompt = ({
@@ -35,6 +41,7 @@ export const useUnsavedChangesPrompt = ({
onSave,
isSaveModalVisible = false,
manualSaveOnUnsavedChanges = false,
+ isInPlaceTransition,
}: UseUnsavedChangesPromptProps) => {
const history = useHistory();
const [showModal, setShowModal] = useState(false);
@@ -88,6 +95,10 @@ export const useUnsavedChangesPrompt = ({
return undefined;
}
+ if (isInPlaceTransition?.(state)) {
+ return undefined;
+ }
+
if (manualSaveRef.current) {
manualSaveRef.current = false;
return undefined;
@@ -105,7 +116,7 @@ export const useUnsavedChangesPrompt = ({
setShowModal(true);
return false;
},
- [history],
+ [history, isInPlaceTransition],
);
useEffect(() => {
diff --git a/superset-frontend/src/pages/Chart/Chart.test.tsx
b/superset-frontend/src/pages/Chart/Chart.test.tsx
index 6f15faceb99..3ead8f44dc1 100644
--- a/superset-frontend/src/pages/Chart/Chart.test.tsx
+++ b/superset-frontend/src/pages/Chart/Chart.test.tsx
@@ -19,11 +19,14 @@
import fetchMock from 'fetch-mock';
import { Link } from 'react-router-dom';
import {
+ act,
+ createStore,
render,
waitFor,
screen,
fireEvent,
} from 'spec/helpers/testing-library';
+import reducerIndex from 'spec/helpers/reducerIndex';
import { getExploreFormData } from 'spec/fixtures/mockExploreFormData';
import { getDashboardFormData } from 'spec/fixtures/mockDashboardFormData';
import { LocalStorageKeys } from 'src/utils/localStorageHelpers';
@@ -32,6 +35,8 @@ import { URL_PARAMS } from 'src/constants';
import { JsonObject, VizType } from '@superset-ui/core';
import { useUnsavedChangesPrompt } from 'src/hooks/useUnsavedChangesPrompt';
import { getParsedExploreURLParams } from
'src/explore/exploreUtils/getParsedExploreURLParams';
+import { toChartStateHistoryState } from
'src/explore/exploreUtils/exploreHistory';
+import * as exploreActions from 'src/explore/actions/exploreActions';
import * as messageToastActions from 'src/components/MessageToasts/actions';
import ChartPage from '.';
@@ -358,6 +363,150 @@ describe('ChartPage', () => {
JSON.stringify({ show_cell_bars: true }).slice(1, -1),
);
});
+
+ test('restores the chart state held by the entry on back-button navigation
(POP)', async () => {
+ const exploreApiRoute = 'glob:*/api/v1/explore/*';
+ const formData = getExploreFormData({
+ viz_type: VizType.Table,
+ show_cell_bars: true,
+ });
+ fetchMock.get(exploreApiRoute, {
+ result: { dataset: { id: 1 }, form_data: formData },
+ });
+ fetchMock.post('glob:*/api/v1/chart/data*', { result: [] });
+ const setExploreControlsSpy = jest.spyOn(
+ exploreActions,
+ 'setExploreControls',
+ );
+ render(
+ <>
+ <Link
+ to={{
+ pathname: '/',
+ search: `?${URL_PARAMS.sliceId.name}=${formData.slice_id}`,
+ state: toChartStateHistoryState({
+ ...formData,
+ show_cell_bars: false,
+ }),
+ }}
+ >
+ Change the chart
+ </Link>
+ <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),
+ );
+
+ // an entry Explore pushed for a chart change, then navigation off it
+ fireEvent.click(screen.getByText('Change the chart'));
+ fireEvent.click(screen.getByText('Navigate away'));
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(2),
+ );
+ fetchMock.clearHistory();
+ setExploreControlsSpy.mockClear();
+
+ window.history.back();
+ await waitFor(() =>
+ expect(setExploreControlsSpy).toHaveBeenCalledWith(
+ expect.objectContaining({ show_cell_bars: false }),
+ ),
+ );
+ expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(0);
+ });
+
+ test('re-fetches when the entry holds the state of another chart', async
() => {
+ const exploreApiRoute = 'glob:*/api/v1/explore/*';
+ const formData = getExploreFormData({ viz_type: VizType.Table });
+ fetchMock.get(exploreApiRoute, {
+ result: { dataset: { id: 1 }, form_data: formData },
+ });
+ const setExploreControlsSpy = jest.spyOn(
+ exploreActions,
+ 'setExploreControls',
+ );
+ render(
+ <>
+ <Link
+ to={{
+ pathname: '/',
+ search: `?${URL_PARAMS.sliceId.name}=99`,
+ state: toChartStateHistoryState({ ...formData, slice_id: 99 }),
+ }}
+ >
+ Another chart
+ </Link>
+ <Link to="/?slice_id=100">Navigate away</Link>
+ <ChartPage />
+ </>,
+ { useRouter: true, useRedux: true, useDnd: true },
+ );
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1),
+ );
+
+ fireEvent.click(screen.getByText('Another chart'));
+ fireEvent.click(screen.getByText('Navigate away'));
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(2),
+ );
+ fetchMock.clearHistory();
+ setExploreControlsSpy.mockClear();
+
+ window.history.back();
+ await waitFor(() =>
+ expect(fetchMock.callHistory.calls(exploreApiRoute).length).toBe(1),
+ );
+ expect(setExploreControlsSpy).not.toHaveBeenCalled();
+ });
+
+ test('re-fetches when the dataset changed after the entry was pushed',
async () => {
+ const exploreApiRoute = 'glob:*/api/v1/explore/*';
+ const loads = () =>
+ fetchMock.callHistory.calls(exploreApiRoute, { method: 'GET' }).length;
+ const formData = getExploreFormData({ viz_type: VizType.Table });
+ fetchMock.get(exploreApiRoute, {
+ result: { dataset: { id: 1 }, form_data: formData },
+ });
+ const store = createStore({}, reducerIndex);
+ render(
+ <>
+ <Link
+ to={{
+ pathname: '/',
+ search: `?${URL_PARAMS.sliceId.name}=${formData.slice_id}`,
+ state: toChartStateHistoryState(formData),
+ }}
+ >
+ Change the chart
+ </Link>
+ <Link to="/?slice_id=99">Navigate away</Link>
+ <ChartPage />
+ </>,
+ { useRouter: true, useRedux: true, useDnd: true, store },
+ );
+ await waitFor(() => expect(loads()).toBe(1));
+ fireEvent.click(screen.getByText('Change the chart'));
+ fireEvent.click(screen.getByText('Navigate away'));
+ await waitFor(() => expect(loads()).toBe(2));
+ fetchMock.clearHistory();
+
+ // the entry predates the swap, so it can't be applied to the chart on
screen
+ act(() => {
+ store.dispatch(
+ exploreActions.setExploreControls({
+ ...formData,
+ datasource: '3__table',
+ }),
+ );
+ });
+ window.history.back();
+ await waitFor(() => expect(loads()).toBe(1));
+ });
});
test('does not show error toast when request is aborted on unmount', async
() => {
diff --git a/superset-frontend/src/pages/Chart/index.tsx
b/superset-frontend/src/pages/Chart/index.tsx
index 4de2b6b712e..99ea4d77bc8 100644
--- a/superset-frontend/src/pages/Chart/index.tsx
+++ b/superset-frontend/src/pages/Chart/index.tsx
@@ -17,7 +17,7 @@
* under the License.
*/
import { useCallback, useEffect, useRef, useState } from 'react';
-import { useDispatch } from 'react-redux';
+import { shallowEqual, useDispatch, useSelector } from 'react-redux';
import { useHistory } from 'react-router-dom';
import type { Location, Action } from 'history';
import { t } from '@apache-superset/core/translation';
@@ -36,6 +36,11 @@ import { URL_PARAMS } from 'src/constants';
import getFormDataWithExtraFilters from
'src/dashboard/util/charts/getFormDataWithExtraFilters';
import { getAppliedFilterValues } from
'src/dashboard/util/activeDashboardFilters';
import { getParsedExploreURLParams } from
'src/explore/exploreUtils/getParsedExploreURLParams';
+import {
+ getChartStateFromHistoryState,
+ isSameChartState,
+ selectRestoreTarget,
+} from 'src/explore/exploreUtils/exploreHistory';
import { hydrateExplore } from 'src/explore/actions/hydrateExplore';
import ExploreViewContainer from 'src/explore/components/ExploreViewContainer';
import { ExploreResponsePayload, SaveActionType } from 'src/explore/types';
@@ -138,6 +143,7 @@ export default function ExplorePage() {
const abortControllerRef = useRef<AbortController | null>(null);
const dispatch = useDispatch();
const history = useHistory();
+ const restoreTarget = useSelector(selectRestoreTarget, shallowEqual);
const loadExploreData = useCallback(
(
@@ -304,10 +310,21 @@ export default function ExplorePage() {
// 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).
+ // Entries holding a chart state of the loaded chart are skipped: Explore
+ // pushed them itself, and ExploreViewContainer restores a popped one in
place.
useEffect(() => {
const unlisten = history.listen((loc: Location, action: Action) => {
const saveAction = (loc.state as Record<string, unknown>)?.saveAction as
SaveActionType | undefined;
+ const chartState = getChartStateFromHistoryState(loc.state);
+ if (chartState) {
+ if (action === 'PUSH') {
+ return;
+ }
+ if (action === 'POP' && isSameChartState(chartState, restoreTarget)) {
+ return;
+ }
+ }
if (action === 'PUSH' || action === 'POP') {
setIsLoaded(false);
loadExploreData(loc, saveAction);
@@ -316,7 +333,7 @@ export default function ExplorePage() {
}
});
return unlisten;
- }, [history, loadExploreData]);
+ }, [history, loadExploreData, restoreTarget]);
if (!isLoaded) {
return <Loading />;
