From c30edaf075cb18a2a36a792cc56ef8336fc5e8a2 Mon Sep 17 00:00:00 2001 From: SBIN2010 Date: Fri, 16 Jan 2026 17:55:27 +0300 Subject: [PATCH] feat: add tab select with save chart to dashboard (#36332) Co-authored-by: Evan Rusackas Co-authored-by: Enzo Martellucci --- superset-frontend/src/dashboard/constants.ts | 3 + .../src/explore/components/SaveModal.test.jsx | 383 ++++++++++++++++++ .../src/explore/components/SaveModal.tsx | 287 ++++++++++++- superset-frontend/src/explore/types.ts | 19 + 4 files changed, 684 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/dashboard/constants.ts b/superset-frontend/src/dashboard/constants.ts index 60c88475132..526b28ae1e5 100644 --- a/superset-frontend/src/dashboard/constants.ts +++ b/superset-frontend/src/dashboard/constants.ts @@ -48,3 +48,6 @@ export const DEFAULT_CROSS_FILTER_SCOPING: NativeFilterScope = { rootPath: [DASHBOARD_ROOT_ID], excluded: [], }; + +export const CHART_WIDTH = 4; +export const CHART_HEIGHT = 50; diff --git a/superset-frontend/src/explore/components/SaveModal.test.jsx b/superset-frontend/src/explore/components/SaveModal.test.jsx index 9ed680c3687..3d5d34ec190 100644 --- a/superset-frontend/src/explore/components/SaveModal.test.jsx +++ b/superset-frontend/src/explore/components/SaveModal.test.jsx @@ -31,6 +31,8 @@ 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'; +import { CHART_WIDTH, CHART_HEIGHT } from 'src/dashboard/constants'; +import { GRID_COLUMN_COUNT } from 'src/dashboard/util/constants'; jest.mock('@superset-ui/core/components/Select', () => ({ ...jest.requireActual('@superset-ui/core/components/Select/AsyncSelect'), @@ -42,6 +44,18 @@ jest.mock('@superset-ui/core/components/Select', () => ({ ), })); +jest.mock('@superset-ui/core/components/TreeSelect', () => ({ + TreeSelect: ({ onChange, disabled }) => { + return ( + onChange(value)} + /> + ); + }, +})); + const middlewares = [thunk]; const mockStore = configureStore(middlewares); const initialState = { @@ -429,3 +443,372 @@ test('dispatches removeChartState when saving and going to dashboard', async () // Clean up removeChartStateSpy.mockRestore(); }); + +test('disables tab selector when no dashboard selected', () => { + const { getByRole, getByTestId } = setup(); + fireEvent.click(getByRole('radio', { name: 'Save as...' })); + const tabSelector = getByTestId('mock-tree-select'); + expect(tabSelector).toBeInTheDocument(); + expect(tabSelector).toBeDisabled(); +}); + +test('renders tab selector when saving as', async () => { + const { getByRole, getByTestId } = setup(); + fireEvent.click(getByRole('radio', { name: 'Save as...' })); + const selection = getByTestId('mock-async-select'); + fireEvent.change(selection, { target: { value: '1' } }); + const tabSelector = getByTestId('mock-tree-select'); + expect(tabSelector).toBeInTheDocument(); + expect(tabSelector).toBeDisabled(); +}); + +test('onDashboardChange triggers tabs load for existing dashboard', async () => { + const dashboardId = mockEvent.value; + + fetchMock.get(`glob:*/api/v1/dashboard/${dashboardId}/tabs`, { + json: { + result: { + tab_tree: [ + { value: 'tab1', title: 'Main Tab' }, + { value: 'tab2', title: 'Tab' }, + ], + }, + }, + }); + const component = new PureSaveModal(defaultProps); + const loadTabsMock = jest + .fn() + .mockResolvedValue([{ value: 'tab1', title: 'Main Tab' }]); + component.loadTabs = loadTabsMock; + await component.onDashboardChange({ + value: dashboardId, + label: 'Test Dashboard', + }); + expect(loadTabsMock).toHaveBeenCalledWith(dashboardId); +}); + +test('onTabChange correctly updates selectedTab via forceUpdate', () => { + const component = new PureSaveModal(defaultProps); + + component.state = { + ...component.state, + tabsData: [ + { + value: 'tab1', + title: 'Main Tab', + key: 'tab1', + children: [ + { + value: 'tab2', + title: 'Analytics Tab', + key: 'tab2', + }, + ], + }, + ], + }; + + component.setState = function (stateUpdate) { + if (typeof stateUpdate === 'function') { + this.state = { ...this.state, ...stateUpdate(this.state) }; + } else { + this.state = { ...this.state, ...stateUpdate }; + } + }.bind(component); + + component.onTabChange('tab2'); + + expect(component.state.selectedTab).toEqual({ + value: 'tab2', + label: 'Analytics Tab', + }); +}); + +test('chart placement logic finds row with available space', () => { + // Test case 1: Row has space (8 + 4 = 12 <= 12) + const positionJson1 = { + tab1: { + type: 'TABS', + id: 'tab1', + children: ['row1'], + }, + row1: { + type: 'ROW', + id: 'row1', + children: ['CHART-1'], + meta: {}, + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { width: 8 }, + }, + }; + + // Test case 2: Row is full (12 + 4 = 16 > 12) + const positionJson2 = { + ...positionJson1, + 'CHART-1': { + ...positionJson1['CHART-1'], + meta: { width: 12 }, + }, + }; + + // Test case 3: Multiple charts in row + const positionJson3 = { + tab1: { + type: 'TABS', + id: 'tab1', + children: ['row1'], + }, + row1: { + type: 'ROW', + id: 'row1', + children: ['CHART-1', 'CHART-2'], + meta: {}, + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { width: 6 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { width: 4 }, + }, + }; + + const findRowWithSpace = (positionJson, tabChildren) => { + for (const childKey of tabChildren) { + const child = positionJson[childKey]; + if (child?.type === 'ROW') { + const rowChildren = child.children || []; + const totalWidth = rowChildren.reduce((sum, key) => { + const component = positionJson[key]; + return sum + (component?.meta?.width || 0); + }, 0); + + if (totalWidth + CHART_WIDTH <= GRID_COLUMN_COUNT) { + return childKey; + } + } + } + return null; + }; + + // Test case 1: Should find row with space + expect(findRowWithSpace(positionJson1, ['row1'])).toBe('row1'); + + // Test case 2: Should not find row (full) + expect(findRowWithSpace(positionJson2, ['row1'])).toBeNull(); + + // Test case 3: Should not find row (6 + 4 = 10, adding 4 = 14 > 12) + expect(findRowWithSpace(positionJson3, ['row1'])).toBeNull(); +}); + +test('addChartToDashboardTab successfully adds chart to existing row with space', async () => { + const dashboardId = 123; + const chartId = 456; + const tabId = 'TABS_ID'; + const sliceName = 'Test Chart'; + + const positionJson = { + [tabId]: { + type: 'TABS', + id: tabId, + children: ['row1'], + }, + row1: { + type: 'ROW', + id: 'row1', + children: ['CHART-1'], + meta: {}, + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { width: 8, height: 50, chartId: 100 }, + }, + }; + + const mockDashboard = { + id: dashboardId, + position_json: JSON.stringify(positionJson), + }; + + const SupersetClient = require('@superset-ui/core').SupersetClient; + const originalGet = SupersetClient.get; + const originalPut = SupersetClient.put; + + SupersetClient.get = jest.fn().mockResolvedValueOnce({ + json: { result: mockDashboard }, + }); + + SupersetClient.put = jest.fn().mockResolvedValueOnce({ + json: { result: mockDashboard }, + }); + + const component = new PureSaveModal(defaultProps); + + const mockNanoid = jest.spyOn(require('nanoid'), 'nanoid'); + mockNanoid.mockReturnValue('test-id'); + + try { + const response = await component.addChartToDashboardTab( + dashboardId, + chartId, + tabId, + sliceName, + ); + + expect(SupersetClient.get).toHaveBeenCalledWith({ + endpoint: `/api/v1/dashboard/${dashboardId}`, + }); + + expect(SupersetClient.put).toHaveBeenCalledWith({ + endpoint: `/api/v1/dashboard/${dashboardId}`, + headers: { 'Content-Type': 'application/json' }, + body: expect.stringContaining('position_json'), + }); + + const putCall = SupersetClient.put.mock.calls[0][0]; + const body = JSON.parse(putCall.body); + const updatedPositionJson = JSON.parse(body.position_json); + + expect(updatedPositionJson[`CHART-${chartId}`]).toBeDefined(); + expect(updatedPositionJson[`CHART-${chartId}`].meta.chartId).toBe(chartId); + expect(updatedPositionJson.row1.children).toContain(`CHART-${chartId}`); + } finally { + SupersetClient.get = originalGet; + SupersetClient.put = originalPut; + mockNanoid.mockRestore(); + } +}); + +test('addChartToDashboardTab creates new row when no existing row has space', async () => { + const dashboardId = 123; + const chartId = 456; + const tabId = 'TABS_ID'; + const sliceName = 'Test Chart'; + + const positionJson = { + [tabId]: { + type: 'TABS', + id: tabId, + children: ['row1'], + }, + row1: { + type: 'ROW', + id: 'row1', + children: ['CHART-1'], + parents: ['ROOT_ID', 'GRID_ID', tabId], + meta: {}, + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + children: [], + parents: ['ROOT_ID', 'GRID_ID', tabId, 'row1'], + meta: { + width: GRID_COLUMN_COUNT, + height: 50, + chartId: 100, + sliceName: 'Existing Chart', + }, + }, + }; + + const mockDashboard = { + id: dashboardId, + position_json: JSON.stringify(positionJson), + }; + + const SupersetClient = require('@superset-ui/core').SupersetClient; + const originalGet = SupersetClient.get; + const originalPut = SupersetClient.put; + + SupersetClient.get = jest.fn().mockResolvedValueOnce({ + json: { result: mockDashboard }, + }); + + let putRequestBody = null; + SupersetClient.put = jest.fn().mockImplementationOnce(request => { + putRequestBody = request; + return Promise.resolve({ + json: { result: mockDashboard }, + }); + }); + + const component = new PureSaveModal(defaultProps); + + const mockRowId = 'test-row-id'; + const mockNanoid = jest.spyOn(require('nanoid'), 'nanoid'); + mockNanoid.mockReturnValueOnce(mockRowId); + + try { + await component.addChartToDashboardTab( + dashboardId, + chartId, + tabId, + sliceName, + ); + + expect(SupersetClient.put).toHaveBeenCalled(); + const body = JSON.parse(putRequestBody.body); + const updatedPositionJson = JSON.parse(body.position_json); + + expect(updatedPositionJson[`ROW-${mockRowId}`]).toBeDefined(); + expect(updatedPositionJson[`ROW-${mockRowId}`].type).toBe('ROW'); + + expect(updatedPositionJson[tabId].children).toContain(`ROW-${mockRowId}`); + + expect(updatedPositionJson[`CHART-${chartId}`]).toBeDefined(); + expect(updatedPositionJson[`ROW-${mockRowId}`].children).toContain( + `CHART-${chartId}`, + ); + } finally { + SupersetClient.get = originalGet; + SupersetClient.put = originalPut; + mockNanoid.mockRestore(); + } +}); + +test('addChartToDashboardTab handles empty position_json', async () => { + const dashboardId = 123; + const chartId = 456; + const tabId = 'TABS_ID'; + const sliceName = 'Test Chart'; + + const mockDashboard = { + id: dashboardId, + position_json: null, + }; + + const SupersetClient = require('@superset-ui/core').SupersetClient; + const originalGet = SupersetClient.get; + const originalPut = SupersetClient.put; + + SupersetClient.get = jest.fn().mockResolvedValueOnce({ + json: { result: mockDashboard }, + }); + + SupersetClient.put = jest.fn().mockResolvedValueOnce({ + json: { result: mockDashboard }, + }); + + const component = new PureSaveModal(defaultProps); + + const mockNanoid = jest.spyOn(require('nanoid'), 'nanoid'); + mockNanoid.mockReturnValue('test-id'); + + try { + await expect( + component.addChartToDashboardTab(dashboardId, chartId, tabId, sliceName), + ).rejects.toThrow(`Tab ${tabId} not found in positionJson`); + } finally { + SupersetClient.get = originalGet; + SupersetClient.put = originalPut; + mockNanoid.mockRestore(); + } +}); diff --git a/superset-frontend/src/explore/components/SaveModal.tsx b/superset-frontend/src/explore/components/SaveModal.tsx index 91410448e7e..67284fcd97b 100644 --- a/superset-frontend/src/explore/components/SaveModal.tsx +++ b/superset-frontend/src/explore/components/SaveModal.tsx @@ -19,6 +19,7 @@ /* eslint camelcase: 0 */ import { ChangeEvent, FormEvent, Component } from 'react'; import { Dispatch } from 'redux'; +import { nanoid } from 'nanoid'; import rison from 'rison'; import { connect } from 'react-redux'; import { withRouter, RouteComponentProps } from 'react-router-dom'; @@ -32,20 +33,24 @@ import { Input, Loading, Divider, + TreeSelect, } from '@superset-ui/core/components'; import { t, logging } from '@apache-superset/core'; import { DatasourceType, isDefined, SupersetClient } from '@superset-ui/core'; import { css, styled, Alert } from '@apache-superset/core/ui'; import { Radio } from '@superset-ui/core/components/Radio'; +import { GRID_COLUMN_COUNT } from 'src/dashboard/util/constants'; import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; -import { SaveActionType } from 'src/explore/types'; +import { SaveActionType, ChartStatusType } from 'src/explore/types'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; import { removeChartState, updateChartState, } from 'src/dashboard/actions/dashboardState'; import { Dashboard } from 'src/types/Dashboard'; +import { TabNode, TabTreeNode } from '../types'; +import { CHART_WIDTH, CHART_HEIGHT } from 'src/dashboard/constants'; // Session storage key for recent dashboard const SK_DASHBOARD_ID = 'save_chart_recent_dashboard'; @@ -71,6 +76,8 @@ type SaveModalState = { isLoading: boolean; saveStatus?: string | null; dashboard?: { label: string; value: string | number }; + selectedTab?: { label: string; value: string | number }; + tabsData: TabTreeNode[]; }; export const StyledModal = styled(Modal)` @@ -90,9 +97,13 @@ class SaveModal extends Component { this.state = { newSliceName: props.sliceName, datasetName: props.datasource?.name, - action: this.canOverwriteSlice() ? 'overwrite' : 'saveas', + action: this.canOverwriteSlice() + ? ChartStatusType.overwrite + : ChartStatusType.saveas, isLoading: false, dashboard: undefined, + tabsData: [], + selectedTab: undefined, }; this.onDashboardChange = this.onDashboardChange.bind(this); this.onSliceNameChange = this.onSliceNameChange.bind(this); @@ -132,6 +143,7 @@ class SaveModal extends Component { this.setState({ dashboard: { label: result.dashboard_title, value: result.id }, }); + await this.loadTabs(dashboardId); } } catch (error) { logging.warn(error); @@ -151,10 +163,20 @@ class SaveModal extends Component { this.setState({ newSliceName: event.target.value }); } - onDashboardChange(dashboard: { label: string; value: string | number }) { - this.setState({ dashboard }); - } + onDashboardChange = async (dashboard: { + label: string; + value: string | number; + }) => { + this.setState({ + dashboard, + tabsData: [], + selectedTab: undefined, + }); + if (typeof dashboard.value === 'number') { + await this.loadTabs(dashboard.value); + } + }; changeAction(action: SaveActionType) { this.setState({ action }); } @@ -210,6 +232,7 @@ class SaveModal extends Component { delete formData.url_params; let dashboard: DashboardGetResponse | null = null; + let selectedTabId: string | undefined; if (this.state.dashboard) { let validId = this.state.dashboard.value; if (this.isNewDashboard()) { @@ -231,6 +254,12 @@ class SaveModal extends Component { ? sliceDashboards : [...sliceDashboards, dashboard.id]; formData.dashboards = sliceDashboards; + if ( + this.state.action === ChartStatusType.saveas && + this.state.selectedTab?.value !== 'OUT_OF_TAB' + ) { + selectedTabId = this.state.selectedTab?.value as string; + } } } @@ -262,6 +291,21 @@ class SaveModal extends Component { } : null, ); + if (dashboard && selectedTabId) { + try { + await this.addChartToDashboardTab( + dashboard.id, + value.id, + selectedTabId, + this.state.newSliceName, + ); + } catch (error) { + logging.error('Error adding chart to dashboard tab:', error); + this.props.addDangerToast( + t('Chart was saved but could not be added to the selected tab.'), + ); + } + } } try { @@ -276,11 +320,14 @@ class SaveModal extends Component { // Go to new dashboard url if (gotodash && dashboard) { + let url = dashboard.url; + if (this.state.selectedTab?.value) { + url += `#${this.state.selectedTab.value}`; + } this.props.dispatch(removeChartState(value.id)); - this.props.history.push(dashboard.url); + this.props.history.push(url); return; } - const searchParams = this.handleRedirect(window.location.search, value); this.props.history.replace(`/explore/?${searchParams.toString()}`); @@ -291,6 +338,114 @@ class SaveModal extends Component { } } + /* Adds a chart to the specified dashboard tab. If an existing row has space, the chart is added there; otherwise, a new row is created. + * @param {number} dashboardId - ID of the dashboard. + * @param {number} chartId - ID of the chart to add. + * @param {string} tabId - ID of the dashboard tab where the chart is added. + * @param {string | undefined} sliceName - Chart name + */ + addChartToDashboardTab = async ( + dashboardId: number, + chartId: number, + tabId: string, + sliceName: string | undefined, + ) => { + try { + const dashboardResponse = await SupersetClient.get({ + endpoint: `/api/v1/dashboard/${dashboardId}`, + }); + + const dashboard = dashboardResponse.json.result; + + let positionJson = dashboard.position_json; + if (typeof positionJson === 'string') { + positionJson = JSON.parse(positionJson); + } + positionJson = positionJson || {}; + + const chartKey = `CHART-${chartId}`; + + // Find a row in the tab with available space + const tabChildren = positionJson[tabId]?.children || []; + let targetRowKey: string | null = null; + + for (const childKey of tabChildren) { + const child = positionJson[childKey]; + if (child?.type === 'ROW') { + const rowChildren = child.children || []; + const totalWidth = rowChildren.reduce((sum: number, key: string) => { + const component = positionJson[key]; + return sum + (component?.meta?.width || 0); + }, 0); + + if (totalWidth + CHART_WIDTH <= GRID_COLUMN_COUNT) { + targetRowKey = childKey; + break; + } + } + } + + const updatedPositionJson = { ...positionJson }; + + // Create a new row if no existing row has space + if (!targetRowKey) { + targetRowKey = `ROW-${nanoid()}`; + updatedPositionJson[targetRowKey] = { + type: 'ROW', + id: targetRowKey, + children: [], + parents: ['ROOT_ID', 'GRID_ID', tabId], + meta: { + background: 'BACKGROUND_TRANSPARENT', + }, + }; + + if (positionJson[tabId]) { + updatedPositionJson[tabId] = { + ...positionJson[tabId], + children: [...(positionJson[tabId].children || []), targetRowKey], + }; + } else { + throw new Error(`Tab ${tabId} not found in positionJson`); + } + } + + updatedPositionJson[chartKey] = { + type: 'CHART', + id: chartKey, + children: [], + parents: ['ROOT_ID', 'GRID_ID', tabId, targetRowKey], + meta: { + width: CHART_WIDTH, + height: CHART_HEIGHT, + chartId, + sliceName: sliceName ?? `Chart ${chartId}`, + }, + }; + + // Add chart to the target row + updatedPositionJson[targetRowKey] = { + ...updatedPositionJson[targetRowKey], + children: [ + ...(updatedPositionJson[targetRowKey].children || []), + chartKey, + ], + }; + + const response = await SupersetClient.put({ + endpoint: `/api/v1/dashboard/${dashboardId}`, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + position_json: JSON.stringify(updatedPositionJson), + }), + }); + + return response; + } catch (error) { + throw new Error(`Error adding chart to dashboard tab: ${error}`); + } + }; + loadDashboard = async (id: number) => { const response = await SupersetClient.get({ endpoint: `/api/v1/dashboard/${id}`, @@ -332,6 +487,101 @@ class SaveModal extends Component { totalCount: count, }; }; + // Loads dashboard tabs and returns the tab hierarchy for display. + loadTabs = async (dashboardId: number) => { + try { + const response = await SupersetClient.get({ + endpoint: `/api/v1/dashboard/${dashboardId}/tabs`, + }); + + const { result } = response.json; + if (!result || !Array.isArray(result.tab_tree)) { + logging.warn('Invalid tabs response format'); + this.setState({ tabsData: [] }); + return []; + } + const tabTree = result.tab_tree; + const gridTabIds = new Set(); + const convertToTreeData = (nodes: TabNode[]): TabTreeNode[] => + nodes.map(node => { + const isGridTab = + Array.isArray(node.parents) && node.parents.includes('GRID_ID'); + if (isGridTab) { + gridTabIds.add(node.value); + } + return { + value: node.value, + title: node.title, + key: node.value, + children: + node.children && node.children.length > 0 + ? convertToTreeData(node.children) + : undefined, + }; + }); + + const treeData = convertToTreeData(tabTree); + + // Add "Out of tab" option at the beginning + if (gridTabIds.size > 0) { + const tabsDataWithOutOfTab = [ + { + value: 'OUT_OF_TAB', + title: 'Out of tab', + key: 'OUT_OF_TAB', + children: undefined, + }, + ...treeData, + ]; + + this.setState({ + tabsData: tabsDataWithOutOfTab, + selectedTab: { value: 'OUT_OF_TAB', label: 'Out of tab' }, + }); + } else { + const firstTab = treeData[0]; + this.setState({ + tabsData: treeData, + selectedTab: { value: firstTab.value, label: firstTab.title }, + }); + } + + return treeData; + } catch (error) { + logging.error('Error loading tabs:', error); + this.setState({ tabsData: [] }); + return []; + } + }; + + onTabChange = (value: string) => { + if (value) { + const findTabInTree = (data: TabTreeNode[]): TabTreeNode | null => { + for (const item of data) { + if (item.value === value) { + return item; + } + if (item.children) { + const found = findTabInTree(item.children); + if (found) return found; + } + } + return null; + }; + + const selectedTab = findTabInTree(this.state.tabsData); + if (selectedTab) { + this.setState({ + selectedTab: { + value: selectedTab.value, + label: selectedTab.title, + }, + }); + } + } else { + this.setState({ selectedTab: undefined }); + } + }; renderSaveChartModal = () => { const info = this.info(); @@ -350,7 +600,7 @@ class SaveModal extends Component { this.changeAction('saveas')} > {t('Save as...')} @@ -404,6 +654,27 @@ class SaveModal extends Component { } /> + {this.state.action === ChartStatusType.saveas && ( + + + + )} {info && } {this.props.alert && (