This is an automated email from the ASF dual-hosted git repository. asoare pushed a commit to branch alexandrusoare/fix/matrixify-data-zoom in repository https://gitbox.apache.org/repos/asf/superset.git
commit c1c4082e005652e876777a766041f74b07a3bfc4 Author: alexandrusoare <[email protected]> AuthorDate: Wed Jan 14 15:25:42 2026 +0200 fix(data-zoom): data-zoom in matrixify --- .../src/chart/components/StatefulChart.test.tsx | 248 +++++++++++++++++++++ .../src/chart/components/StatefulChart.tsx | 26 ++- 2 files changed, 273 insertions(+), 1 deletion(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx index 35b65ac772..086e35f043 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.test.tsx @@ -471,3 +471,251 @@ test('should handle chartId changes', async () => { expect(mockChartClient.loadFormData).toHaveBeenCalledTimes(2); }); }); + +test('should NOT refetch data when string-based renderTrigger control (zoomable) changes', async () => { + // Control panel with zoomable as a string reference (like ['zoomable'] in control panels) + const controlPanelConfig = { + controlPanelSections: [ + { + controlSetRows: [ + ['zoomable'], // String reference to shared control + [ + { + name: 'metrics', + config: { + renderTrigger: false, + }, + }, + ], + ], + }, + ], + }; + + (getChartControlPanelRegistry as any).mockReturnValue({ + get: jest.fn().mockReturnValue(controlPanelConfig), + }); + + const formDataWithZoom = { + ...mockFormData, + zoomable: false, + }; + + const { rerender, getByTestId } = render( + <StatefulChart formData={formDataWithZoom} chartType="test_chart" />, + ); + + await waitFor(() => { + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + }); + + // Toggle zoomable (string-based shared control with renderTrigger: true) + const updatedFormData = { + ...formDataWithZoom, + zoomable: true, + }; + + rerender(<StatefulChart formData={updatedFormData} chartType="test_chart" />); + + await waitFor(() => { + // Should NOT refetch data - zoomable is a renderTrigger control + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + // But should re-render with new formData + expect(getByTestId('super-chart')).toHaveTextContent( + JSON.stringify(updatedFormData), + ); + }); +}); + +test('should NOT refetch data when other string-based renderTrigger controls change', async () => { + // Test other controls in RENDER_TRIGGER_SHARED_CONTROLS set + const controlPanelConfig = { + controlPanelSections: [ + { + controlSetRows: [ + ['color_scheme'], // String reference + ['y_axis_format'], // String reference + ['currency_format'], // String reference + ['time_shift_color'], // String reference + ], + }, + ], + }; + + (getChartControlPanelRegistry as any).mockReturnValue({ + get: jest.fn().mockReturnValue(controlPanelConfig), + }); + + const { rerender, getByTestId } = render( + <StatefulChart formData={mockFormData} chartType="test_chart" />, + ); + + await waitFor(() => { + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + }); + + // Change multiple string-based renderTrigger controls + const updatedFormData = { + ...mockFormData, + color_scheme: 'new_scheme', + y_axis_format: '.2f', + }; + + rerender(<StatefulChart formData={updatedFormData} chartType="test_chart" />); + + await waitFor(() => { + // Should NOT refetch data + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + // But should re-render + expect(getByTestId('super-chart')).toHaveTextContent( + JSON.stringify(updatedFormData), + ); + }); +}); + +test('should refetch when string control is NOT in RENDER_TRIGGER_SHARED_CONTROLS', async () => { + // Control panel with a string control that is NOT in the renderTrigger set + const controlPanelConfig = { + controlPanelSections: [ + { + controlSetRows: [ + ['some_unknown_control'], // Not in RENDER_TRIGGER_SHARED_CONTROLS + ], + }, + ], + }; + + (getChartControlPanelRegistry as any).mockReturnValue({ + get: jest.fn().mockReturnValue(controlPanelConfig), + }); + + const { rerender } = render( + <StatefulChart formData={mockFormData} chartType="test_chart" />, + ); + + await waitFor(() => { + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + }); + + // Change the unknown control + const updatedFormData = { + ...mockFormData, + some_unknown_control: 'new_value', + }; + + rerender(<StatefulChart formData={updatedFormData} chartType="test_chart" />); + + await waitFor(() => { + // Should refetch because the control is not recognized as renderTrigger + expect(mockChartClient.client.post).toHaveBeenCalledTimes(2); + }); +}); + +test('should handle mixed string and object controls correctly', async () => { + // Control panel with both string references and object definitions + const controlPanelConfig = { + controlPanelSections: [ + { + controlSetRows: [ + ['zoomable'], // String reference (in RENDER_TRIGGER_SHARED_CONTROLS) + [ + { + name: 'minorTicks', + config: { + renderTrigger: true, + }, + }, + ], // Object definition + ], + }, + ], + }; + + (getChartControlPanelRegistry as any).mockReturnValue({ + get: jest.fn().mockReturnValue(controlPanelConfig), + }); + + const formDataWithControls = { + ...mockFormData, + zoomable: false, + minorTicks: false, + }; + + const { rerender, getByTestId } = render( + <StatefulChart formData={formDataWithControls} chartType="test_chart" />, + ); + + await waitFor(() => { + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + }); + + // Change both string-based and object-based renderTrigger controls + const updatedFormData = { + ...formDataWithControls, + zoomable: true, + minorTicks: true, + }; + + rerender(<StatefulChart formData={updatedFormData} chartType="test_chart" />); + + await waitFor(() => { + // Should NOT refetch - both are renderTrigger controls + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + // But should re-render + expect(getByTestId('super-chart')).toHaveTextContent( + JSON.stringify(updatedFormData), + ); + }); +}); + +test('should refetch when mixing renderTrigger string control with non-renderTrigger change', async () => { + const controlPanelConfig = { + controlPanelSections: [ + { + controlSetRows: [ + ['zoomable'], // String reference (renderTrigger) + [ + { + name: 'metrics', + config: { + renderTrigger: false, + }, + }, + ], + ], + }, + ], + }; + + (getChartControlPanelRegistry as any).mockReturnValue({ + get: jest.fn().mockReturnValue(controlPanelConfig), + }); + + const formDataWithZoom = { + ...mockFormData, + zoomable: false, + metrics: ['metric1'], + }; + + const { rerender } = render( + <StatefulChart formData={formDataWithZoom} chartType="test_chart" />, + ); + + await waitFor(() => { + expect(mockChartClient.client.post).toHaveBeenCalledTimes(1); + }); + + // Change both zoomable (renderTrigger) and metrics (non-renderTrigger) + const updatedFormData = { + ...formDataWithZoom, + zoomable: true, + metrics: ['metric2'], + }; + + rerender(<StatefulChart formData={updatedFormData} chartType="test_chart" />); + + await waitFor(() => { + // Should refetch because metrics changed (non-renderTrigger) + expect(mockChartClient.client.post).toHaveBeenCalledTimes(2); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx index ba22e64808..fbc94b0f4c 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx @@ -36,6 +36,24 @@ import SuperChart from './SuperChart'; // Using more specific states that align with chart loading process type LoadingState = 'uninitialized' | 'loading' | 'loaded' | 'error'; +/** + * Known shared controls that have renderTrigger: true. + * These are controls defined in sharedControls that only affect rendering, + * not data fetching. When these controls change, we should re-render + * without refetching data. + * + * This list is needed because string-based control references (e.g., ['zoomable']) + * cannot be introspected for their renderTrigger property without importing + * sharedControls, which would create a circular dependency. + */ +const RENDER_TRIGGER_SHARED_CONTROLS = new Set([ + 'zoomable', + 'color_scheme', + 'time_shift_color', + 'y_axis_format', + 'currency_format', +]); + /** * Helper function to determine if data should be refetched based on formData changes * @param prevFormData Previous formData @@ -72,7 +90,13 @@ function shouldRefetchData( if (section.controlSetRows) { section.controlSetRows.forEach((row: any) => { row.forEach((control: any) => { - if (control && typeof control === 'object') { + // Handle string references to shared controls with renderTrigger + if ( + typeof control === 'string' && + RENDER_TRIGGER_SHARED_CONTROLS.has(control) + ) { + renderTriggerControls.add(control); + } else if (control && typeof control === 'object') { const controlName = control.name || control.config?.name; if (controlName && control.config?.renderTrigger === true) { renderTriggerControls.add(controlName);
