From a0e63faf62c0dad147054da4a677d969d0a03dcf Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Fri, 28 Nov 2025 20:44:41 +0300 Subject: [PATCH] fix: add a fallback to chart state callback (#36327) --- .../components/gridComponents/Chart/Chart.jsx | 78 +++++++---- .../gridComponents/Chart/Chart.test.jsx | 124 ++++++++++++++++++ 2 files changed, 179 insertions(+), 23 deletions(-) diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx index 2775a38f4fa..37e24b99f63 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx @@ -114,6 +114,33 @@ const SliceContainer = styled.div` const EMPTY_OBJECT = {}; const EMPTY_ARRAY = []; +// Helper function to get chart state with fallback +const getChartStateWithFallback = (chartState, formData, vizType) => { + if (!hasChartStateConverter(vizType)) { + return null; + } + + return ( + chartState?.state || formData.table_state || formData.pivot_table_state + ); +}; + +// Helper function to create own state with chart state conversion +const createOwnStateWithChartState = (baseOwnState, chartState, vizType) => { + const state = getChartStateWithFallback(chartState, {}, vizType); + + if (!state) { + return baseOwnState; + } + + const convertedState = convertChartStateToOwnState(vizType, state); + return { + ...baseOwnState, + ...convertedState, + chartState: state, + }; +}; + const Chart = props => { const dispatch = useDispatch(); const descriptionRef = useRef(null); @@ -383,16 +410,11 @@ const Chart = props => { const ownState = useMemo(() => { const baseOwnState = dataMask[props.id]?.ownState || EMPTY_OBJECT; - - if (hasChartStateConverter(slice.viz_type) && chartState?.state) { - return { - ...baseOwnState, - ...convertChartStateToOwnState(slice.viz_type, chartState.state), - chartState: chartState.state, - }; - } - - return baseOwnState; + return createOwnStateWithChartState( + baseOwnState, + chartState, + slice.viz_type, + ); }, [ dataMask[props.id]?.ownState, props.id, @@ -486,19 +508,19 @@ const Chart = props => { const safeChartName = chartName.replace(/[^a-zA-Z0-9_-]/g, '_'); filename = `${safeChartName}${timestamp}.csv`; } - let ownState = dataMask[props.id]?.ownState || {}; + const baseOwnState = dataMask[props.id]?.ownState || {}; + const state = getChartStateWithFallback( + chartState, + formData, + slice.viz_type, + ); - // Convert chart-specific state to backend format using registered converter - if (hasChartStateConverter(slice.viz_type) && chartState?.state) { - const convertedState = convertChartStateToOwnState( - slice.viz_type, - chartState.state, - ); - ownState = { - ...ownState, - ...convertedState, - }; - } + const ownState = state + ? { + ...baseOwnState, + ...convertChartStateToOwnState(slice.viz_type, state), + } + : baseOwnState; exportChart({ formData: exportFormData, @@ -671,7 +693,17 @@ const Chart = props => { formData={formData} labelsColor={labelsColor} labelsColorMap={labelsColorMap} - ownState={ownState} + ownState={createOwnStateWithChartState( + dataMask[props.id]?.ownState || EMPTY_OBJECT, + { + state: getChartStateWithFallback( + chartState, + formData, + slice.viz_type, + ), + }, + slice.viz_type, + )} filterState={dataMask[props.id]?.filterState} queriesResponse={chart.queriesResponse} timeout={timeout} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.jsx index bb49952601f..158f9072fea 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.jsx @@ -21,6 +21,7 @@ import { FeatureFlag, VizType } from '@superset-ui/core'; import * as redux from 'redux'; import * as exploreUtils from 'src/explore/exploreUtils'; +import * as chartStateConverter from '../../../util/chartStateConverter'; import { sliceEntitiesForChart as sliceEntities } from 'spec/fixtures/mockSliceEntities'; import mockDatasource from 'spec/fixtures/mockDatasource'; import chartQueries, { @@ -116,6 +117,10 @@ beforeAll(() => { })); }); +afterEach(() => { + jest.clearAllMocks(); +}); + test('should render a SliceHeader', () => { const { getByTestId, container } = setup(); expect(getByTestId('slice-header')).toBeInTheDocument(); @@ -297,3 +302,122 @@ test('should re-render when cacheBusterProp changes', () => { rerender(); expect(getByTestId('chart-container')).toBeInTheDocument(); }); + +test('should handle chart state conversion when converter exists', () => { + const mockChartState = { sortColumn: 'column1', sortOrder: 'asc' }; + const mockConvertedState = { sort: [{ columnId: 'column1', order: 'asc' }] }; + + jest + .spyOn(chartStateConverter, 'hasChartStateConverter') + .mockReturnValue(true); + jest + .spyOn(chartStateConverter, 'convertChartStateToOwnState') + .mockReturnValue(mockConvertedState); + + const { getByTestId } = setup( + {}, + { + ...defaultState, + dashboardState: { + ...defaultState.dashboardState, + chartStates: { + [queryId]: { state: mockChartState }, + }, + }, + }, + ); + + expect(getByTestId('chart-container')).toBeInTheDocument(); + expect(chartStateConverter.hasChartStateConverter).toHaveBeenCalledWith( + VizType.Table, + ); +}); + +test('should fallback to formData state when runtime state not available', () => { + const mockFormDataState = { sortColumn: 'column2', sortOrder: 'desc' }; + const mockConvertedState = { sort: [{ columnId: 'column2', order: 'desc' }] }; + + jest + .spyOn(chartStateConverter, 'hasChartStateConverter') + .mockReturnValue(true); + jest + .spyOn(chartStateConverter, 'convertChartStateToOwnState') + .mockReturnValue(mockConvertedState); + + const { getByTestId } = setup( + {}, + { + ...defaultState, + charts: { + ...defaultState.charts, + [queryId]: { + ...defaultState.charts[queryId], + form_data: { + ...defaultState.charts[queryId].form_data, + table_state: mockFormDataState, + }, + }, + }, + }, + ); + + expect(getByTestId('chart-container')).toBeInTheDocument(); +}); + +test('should handle chart state when no converter exists', () => { + jest + .spyOn(chartStateConverter, 'hasChartStateConverter') + .mockReturnValue(false); + jest.spyOn(chartStateConverter, 'convertChartStateToOwnState'); + + const { getByTestId } = setup( + {}, + { + ...defaultState, + dashboardState: { + ...defaultState.dashboardState, + chartStates: { + [queryId]: { state: { someState: 'value' } }, + }, + }, + }, + ); + + expect(getByTestId('chart-container')).toBeInTheDocument(); + expect( + chartStateConverter.convertChartStateToOwnState, + ).not.toHaveBeenCalled(); +}); + +test('should merge base ownState with converted chart state', () => { + const baseOwnState = { existingProp: 'value' }; + const mockChartState = { sortColumn: 'column1', sortOrder: 'asc' }; + const mockConvertedState = { sort: [{ columnId: 'column1', order: 'asc' }] }; + + jest + .spyOn(chartStateConverter, 'hasChartStateConverter') + .mockReturnValue(true); + jest + .spyOn(chartStateConverter, 'convertChartStateToOwnState') + .mockReturnValue(mockConvertedState); + + const { getByTestId } = setup( + {}, + { + ...defaultState, + dataMask: { + [queryId]: { + ownState: baseOwnState, + }, + }, + dashboardState: { + ...defaultState.dashboardState, + chartStates: { + [queryId]: { state: mockChartState }, + }, + }, + }, + ); + + expect(getByTestId('chart-container')).toBeInTheDocument(); +});