From 1f46360d75e0673b79b38b7a13508e3acbc60e3e Mon Sep 17 00:00:00 2001 From: yousoph Date: Thu, 18 Jun 2026 01:31:08 +0000 Subject: [PATCH 1/3] fix(explore): pre-populate SaveModal dashboard from chart metadata When a user opens the save modal for a chart that is already on one or more dashboards, the Dashboard field was blank unless the user had arrived from that dashboard (URL param) or had saved there before (sessionStorage). The Explore API already returns metadata.dashboards listing every dashboard the chart belongs to, but SaveModal never consulted it. The fix adds a fallback in componentDidMount: if no dashboardId is found from the URL prop or sessionStorage, iterate metadata.dashboards, load each one, and pre-select the first dashboard the user can edit. This makes "Save & go to dashboard" work correctly when navigating directly to a chart's Explore page. Also wires explore.metadata into SaveModal via mapStateToProps so the component does not need any new parent-level prop. Co-Authored-By: Claude Sonnet 4.6 --- .../src/explore/components/SaveModal.test.tsx | 164 ++++++++++++++++++ .../src/explore/components/SaveModal.tsx | 33 +++- 2 files changed, 196 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/explore/components/SaveModal.test.tsx b/superset-frontend/src/explore/components/SaveModal.test.tsx index ae01446affb4..7733e2dd956e 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.tsx +++ b/superset-frontend/src/explore/components/SaveModal.test.tsx @@ -615,6 +615,170 @@ test('onTabChange correctly updates selectedTab via forceUpdate', () => { }); }); +const ownerUser = { + userId: 1, + username: 'testuser', + firstName: 'Test', + lastName: 'User', + isActive: true, + isAnonymous: false, + permissions: {}, + roles: { Alpha: [['can_write', 'Dashboard']] as [string, string][] }, + groups: [], +}; + +const makeMetadataDashboard = (id: number, title: string) => ({ + id, + dashboard_title: title, + owners: [{ id: 1, first_name: 'Test', last_name: 'User' }], + extra_owners: [], + roles: [], + url: `/superset/dashboard/${id}/`, + slug: null, + thumbnail_url: null, + published: true, + changed_by_name: 'Test User', + changed_by: { id: 1, first_name: 'Test', last_name: 'User' }, + changed_on: '2024-01-01', + charts: [], +}); + +test('pre-populates dashboard from metadata.dashboards when dashboardId prop is absent', async () => { + const dashboardId = 5; + const dashboardTitle = 'Chart Dashboard'; + + const myProps = { + ...defaultProps, + dashboardId: null, + metadata: { + dashboards: [{ id: dashboardId, dashboard_title: dashboardTitle }], + owners: ['Test User'], + created_on_humanized: '2 days ago', + changed_on_humanized: '1 day ago', + }, + user: ownerUser, + slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] }, + dispatch: jest.fn(), + addDangerToast: jest.fn(), + }; + + const component = new TestSaveModal(myProps); + const mockFull = makeMetadataDashboard(dashboardId, dashboardTitle); + + component.loadDashboard = jest.fn().mockResolvedValue(mockFull); + component.loadTabs = jest.fn().mockResolvedValue([]); + + const stateUpdates: any[] = []; + component.setState = jest.fn((update: any) => { + stateUpdates.push(update); + }); + + try { + sessionStorage.clear(); + } catch (_) { + // ignore + } + + await component.componentDidMount(); + + expect(component.loadDashboard).toHaveBeenCalledWith(dashboardId); + expect(stateUpdates).toContainEqual({ + dashboard: { label: dashboardTitle, value: dashboardId }, + }); + expect(component.loadTabs).toHaveBeenCalledWith(dashboardId); +}); + +test('skips non-editable dashboards and picks the first editable one from metadata', async () => { + const editableId = 7; + const editableTitle = 'Editable Dashboard'; + + const myProps = { + ...defaultProps, + dashboardId: null, + metadata: { + dashboards: [ + { id: 6, dashboard_title: 'Not Mine' }, + { id: editableId, dashboard_title: editableTitle }, + ], + owners: ['Test User'], + created_on_humanized: '2 days ago', + changed_on_humanized: '1 day ago', + }, + user: ownerUser, + slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] }, + dispatch: jest.fn(), + addDangerToast: jest.fn(), + }; + + const component = new TestSaveModal(myProps); + + const notMine = makeMetadataDashboard(6, 'Not Mine'); + notMine.owners = [{ id: 99, first_name: 'Other', last_name: 'Owner' }]; + const editable = makeMetadataDashboard(editableId, editableTitle); + + component.loadDashboard = jest.fn().mockImplementation((id: number) => + Promise.resolve(id === 6 ? notMine : editable), + ); + component.loadTabs = jest.fn().mockResolvedValue([]); + + const stateUpdates: any[] = []; + component.setState = jest.fn((update: any) => { + stateUpdates.push(update); + }); + + try { + sessionStorage.clear(); + } catch (_) { + // ignore + } + + await component.componentDidMount(); + + expect(stateUpdates).toContainEqual({ + dashboard: { label: editableTitle, value: editableId }, + }); + expect(component.loadTabs).toHaveBeenCalledWith(editableId); +}); + +test('does not use metadata fallback when dashboardId prop is set', async () => { + const propDashboardId = 3; + const propDashboardTitle = 'Prop Dashboard'; + + const myProps = { + ...defaultProps, + dashboardId: propDashboardId, + metadata: { + dashboards: [{ id: 99, dashboard_title: 'Should Not Be Used' }], + owners: ['Test User'], + created_on_humanized: '2 days ago', + changed_on_humanized: '1 day ago', + }, + user: ownerUser, + slice: { slice_id: 1, slice_name: 'My Chart', owners: [1] }, + dispatch: jest.fn(), + addDangerToast: jest.fn(), + }; + + const component = new TestSaveModal(myProps); + const mockFull = makeMetadataDashboard(propDashboardId, propDashboardTitle); + + component.loadDashboard = jest.fn().mockResolvedValue(mockFull); + component.loadTabs = jest.fn().mockResolvedValue([]); + + const stateUpdates: any[] = []; + component.setState = jest.fn((update: any) => { + stateUpdates.push(update); + }); + + await component.componentDidMount(); + + expect(component.loadDashboard).toHaveBeenCalledWith(propDashboardId); + expect(component.loadDashboard).not.toHaveBeenCalledWith(99); + expect(stateUpdates).toContainEqual({ + dashboard: { label: propDashboardTitle, value: propDashboardId }, + }); +}); + test('chart placement logic finds row with available space', () => { // Test case 1: Row has space (8 + 4 = 12 <= 12) const positionJson1 = { diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 9d6cbeab6c08..b64eb4b5aa94 100755 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -54,7 +54,7 @@ import { isUserAdmin, } from 'src/dashboard/util/permissionUtils'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; -import { SaveActionType, ChartStatusType } from 'src/explore/types'; +import { SaveActionType, ChartStatusType, ExplorePageInitialData } from 'src/explore/types'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import { removeChartState, @@ -81,6 +81,7 @@ interface SaveModalProps extends RouteComponentProps { isVisible: boolean; dispatch: Dispatch; theme: SupersetTheme; + metadata?: ExplorePageInitialData['metadata']; } type SaveModalState = { @@ -162,6 +163,34 @@ class SaveModal extends Component { t('An error occurred while loading dashboard information.'), ); } + } else { + const metadataDashboards = this.props.metadata?.dashboards; + if (metadataDashboards?.length) { + // Fallback: the chart is already on one or more dashboards (from Explore API + // metadata). Pre-populate with the first dashboard the user can edit so the + // "Save & go to dashboard" button works out of the box. + try { + const loaded = await Promise.all( + metadataDashboards.map(({ id }) => + this.loadDashboard(id).catch(() => null), + ), + ); + const editable = (loaded as (Dashboard | null)[]).find( + d => d && canUserEditDashboard(d, this.props.user), + ) as Dashboard | undefined; + if (editable) { + this.setState({ + dashboard: { + label: editable.dashboard_title, + value: editable.id, + }, + }); + await this.loadTabs(editable.id); + } + } catch (error) { + logging.warn(error); + } + } } } @@ -826,6 +855,7 @@ interface StateProps { dashboards: any; alert: any; isVisible: boolean; + metadata?: ExplorePageInitialData['metadata']; } function mapStateToProps({ @@ -841,6 +871,7 @@ function mapStateToProps({ dashboards: saveModal.dashboards, alert: saveModal.saveModalAlert, isVisible: saveModal.isVisible, + metadata: explore.metadata, }; } From 026f9d799f62a0558d017c4bb563ccc32ac4b252 Mon Sep 17 00:00:00 2001 From: yousoph Date: Thu, 18 Jun 2026 08:47:15 +0000 Subject: [PATCH 2/3] style: prettier formatting --- .../src/explore/components/SaveModal.test.tsx | 8 +++++--- superset-frontend/src/explore/components/SaveModal.tsx | 6 +++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.test.tsx b/superset-frontend/src/explore/components/SaveModal.test.tsx index 7733e2dd956e..1b809dde07cd 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.tsx +++ b/superset-frontend/src/explore/components/SaveModal.test.tsx @@ -716,9 +716,11 @@ test('skips non-editable dashboards and picks the first editable one from metada notMine.owners = [{ id: 99, first_name: 'Other', last_name: 'Owner' }]; const editable = makeMetadataDashboard(editableId, editableTitle); - component.loadDashboard = jest.fn().mockImplementation((id: number) => - Promise.resolve(id === 6 ? notMine : editable), - ); + component.loadDashboard = jest + .fn() + .mockImplementation((id: number) => + Promise.resolve(id === 6 ? notMine : editable), + ); component.loadTabs = jest.fn().mockResolvedValue([]); const stateUpdates: any[] = []; diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index b64eb4b5aa94..df57540d8260 100755 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -54,7 +54,11 @@ import { isUserAdmin, } from 'src/dashboard/util/permissionUtils'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; -import { SaveActionType, ChartStatusType, ExplorePageInitialData } from 'src/explore/types'; +import { + SaveActionType, + ChartStatusType, + ExplorePageInitialData, +} from 'src/explore/types'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import { removeChartState, From 8b9ed696f4ed68b2f339f7531f07f25c87ea69ef Mon Sep 17 00:00:00 2001 From: yousoph Date: Thu, 18 Jun 2026 09:18:24 +0000 Subject: [PATCH 3/3] perf(explore): use sequential early-exit loop instead of Promise.all in SaveModal Promise.all fired N concurrent dashboard API requests even though only the first editable dashboard is needed. Switch to a sequential for-of loop that breaks as soon as it finds an editable dashboard, bounding requests to the minimum needed. Co-Authored-By: Claude Sonnet 4.6 --- .../src/explore/components/SaveModal.tsx | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index df57540d8260..fce7c0cf5937 100755 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -174,14 +174,15 @@ class SaveModal extends Component { // metadata). Pre-populate with the first dashboard the user can edit so the // "Save & go to dashboard" button works out of the box. try { - const loaded = await Promise.all( - metadataDashboards.map(({ id }) => - this.loadDashboard(id).catch(() => null), - ), - ); - const editable = (loaded as (Dashboard | null)[]).find( - d => d && canUserEditDashboard(d, this.props.user), - ) as Dashboard | undefined; + let editable: Dashboard | undefined; + for (const { id } of metadataDashboards) { + // eslint-disable-next-line no-await-in-loop + const result = await this.loadDashboard(id).catch(() => null); + if (result && canUserEditDashboard(result, this.props.user)) { + editable = result as Dashboard; + break; + } + } if (editable) { this.setState({ dashboard: {