From c61be6da7674a4e814765a76ad13b7102297b384 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 3 Dec 2025 09:54:50 +0300 Subject: [PATCH 1/4] fix(SaveModal): reset chart state when saving and going to a dashboard --- superset-frontend/src/explore/components/SaveModal.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 759241224d88..27908dfb39dc 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -46,6 +46,7 @@ import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; import { SaveActionType } from 'src/explore/types'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { removeChartState } from 'src/dashboard/actions/dashboardState'; import { Dashboard } from 'src/types/Dashboard'; // Session storage key for recent dashboard @@ -276,6 +277,7 @@ class SaveModal extends Component { // Go to new dashboard url if (gotodash && dashboard) { + this.props.dispatch(removeChartState(value.id)); this.props.history.push(dashboard.url); return; } From 2d4f0edc3d65a6f5db7ae684247d5864a25f076a Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 3 Dec 2025 10:05:50 +0300 Subject: [PATCH 2/4] chore: add tests for the fix --- .../src/explore/components/SaveModal.test.jsx | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 43f4b5739e75..b7229ca55035 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -30,6 +30,7 @@ import fetchMock from 'fetch-mock'; import * as saveModalActions from 'src/explore/actions/saveModalActions'; import SaveModal, { PureSaveModal } from 'src/explore/components/SaveModal'; +import * as dashboardStateActions from 'src/dashboard/actions/dashboardState'; jest.mock('@superset-ui/core/components/Select', () => ({ ...jest.requireActual('@superset-ui/core/components/Select/AsyncSelect'), @@ -345,3 +346,82 @@ test('removes form_data_key from URL parameters after save', () => { expect(result.get('slice_id')).toEqual('1'); expect(result.get('save_action')).toEqual('overwrite'); }); + +test('dispatches removeChartState when saving and going to dashboard', async () => { + // Spy on the removeChartState action creator + const removeChartStateSpy = jest.spyOn(dashboardStateActions, 'removeChartState'); + + // Mock the dashboard API response + const dashboardId = 123; + const dashboardUrl = '/superset/dashboard/test-dashboard/'; + fetchMock.get( + `glob:*/api/v1/dashboard/${dashboardId}*`, + { + result: { + id: dashboardId, + dashboard_title: 'Test Dashboard', + url: dashboardUrl + }, + }, + { overwriteRoutes: true } + ); + + const mockDispatch = jest.fn(); + const mockHistory = { + push: jest.fn(), + replace: jest.fn(), + }; + const chartId = 42; + const mockUpdateSlice = jest.fn(() => Promise.resolve({ id: chartId })); + const mockSetFormData = jest.fn(); + + const myProps = { + ...defaultProps, + slice: { slice_id: 1, slice_name: 'title', owners: [1] }, + actions: { + setFormData: mockSetFormData, + updateSlice: mockUpdateSlice, + getSliceDashboards: jest.fn(() => Promise.resolve([])), + saveSliceFailed: jest.fn(), + }, + user: { userId: 1 }, + history: mockHistory, + dispatch: mockDispatch, + }; + + const saveModal = new PureSaveModal(myProps); + saveModal.state = { + action: 'overwrite', + newSliceName: 'test chart', + datasetName: 'test dataset', + dashboard: { label: 'Test Dashboard', value: dashboardId }, + saveStatus: null, + }; + + // Mock onHide to prevent errors + saveModal.onHide = jest.fn(); + + // Trigger save and go to dashboard (gotodash = true) + await saveModal.saveOrOverwrite(true); + + // Wait for async operations + await waitFor(() => { + expect(mockUpdateSlice).toHaveBeenCalled(); + expect(mockSetFormData).toHaveBeenCalled(); + }); + + // Verify removeChartState was called with the correct chart ID + expect(removeChartStateSpy).toHaveBeenCalledWith(chartId); + + // Verify the action was dispatched + expect(mockDispatch).toHaveBeenCalled(); + expect(mockDispatch).toHaveBeenCalledWith( + dashboardStateActions.removeChartState(chartId) + ); + + // Verify navigation happened + expect(mockHistory.push).toHaveBeenCalled(); + + // Clean up + removeChartStateSpy.mockRestore(); +}); From 5a12520b47eb64e91c97cd98f5872be978a16167 Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 3 Dec 2025 11:34:24 +0300 Subject: [PATCH 3/4] fix: remove spy pollution from test assertion The test was incorrectly calling removeChartState inside the assertion, which would cause the spy to record the call and make the test pass even if the actual implementation didn't call it. --- .../src/explore/components/SaveModal.test.jsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index b7229ca55035..647a084e611b 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -413,11 +413,12 @@ test('dispatches removeChartState when saving and going to dashboard', async () // Verify removeChartState was called with the correct chart ID expect(removeChartStateSpy).toHaveBeenCalledWith(chartId); - // Verify the action was dispatched + // Verify the action was dispatched (check the action object directly) expect(mockDispatch).toHaveBeenCalled(); - expect(mockDispatch).toHaveBeenCalledWith( - dashboardStateActions.removeChartState(chartId) - ); + expect(mockDispatch).toHaveBeenCalledWith({ + type: 'REMOVE_CHART_STATE', + chartId, + }); // Verify navigation happened expect(mockHistory.push).toHaveBeenCalled(); From c6be7e060f6702d51e0002789ba8522b3418057d Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Wed, 3 Dec 2025 14:13:42 +0300 Subject: [PATCH 4/4] fix: ci --- .../src/explore/components/SaveModal.test.jsx | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 647a084e611b..9ed680c36873 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -349,21 +349,24 @@ test('removes form_data_key from URL parameters after save', () => { test('dispatches removeChartState when saving and going to dashboard', async () => { // Spy on the removeChartState action creator - const removeChartStateSpy = jest.spyOn(dashboardStateActions, 'removeChartState'); - + const removeChartStateSpy = jest.spyOn( + dashboardStateActions, + 'removeChartState', + ); + // Mock the dashboard API response const dashboardId = 123; const dashboardUrl = '/superset/dashboard/test-dashboard/'; fetchMock.get( `glob:*/api/v1/dashboard/${dashboardId}*`, { - result: { - id: dashboardId, + result: { + id: dashboardId, dashboard_title: 'Test Dashboard', - url: dashboardUrl + url: dashboardUrl, }, }, - { overwriteRoutes: true } + { overwriteRoutes: true }, ); const mockDispatch = jest.fn(); @@ -374,7 +377,7 @@ test('dispatches removeChartState when saving and going to dashboard', async () const chartId = 42; const mockUpdateSlice = jest.fn(() => Promise.resolve({ id: chartId })); const mockSetFormData = jest.fn(); - + const myProps = { ...defaultProps, slice: { slice_id: 1, slice_name: 'title', owners: [1] }, @@ -412,7 +415,7 @@ test('dispatches removeChartState when saving and going to dashboard', async () // Verify removeChartState was called with the correct chart ID expect(removeChartStateSpy).toHaveBeenCalledWith(chartId); - + // Verify the action was dispatched (check the action object directly) expect(mockDispatch).toHaveBeenCalled(); expect(mockDispatch).toHaveBeenCalledWith({ @@ -422,7 +425,7 @@ test('dispatches removeChartState when saving and going to dashboard', async () // Verify navigation happened expect(mockHistory.push).toHaveBeenCalled(); - + // Clean up removeChartStateSpy.mockRestore(); });