This is an automated email from the ASF dual-hosted git repository.
msyavuz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new e053418c97 fix(Matrixify): Do not clear values when saving (#37090)
e053418c97 is described below
commit e053418c9700dd75629f3d15f8cd5bd972e7ed09
Author: Mehmet Salih Yavuz <[email protected]>
AuthorDate: Tue Jan 13 23:22:51 2026 +0300
fix(Matrixify): Do not clear values when saving (#37090)
---
.../components/ControlPanelsContainer.test.tsx | 101 +++++++++++++++++
.../explore/components/ControlPanelsContainer.tsx | 15 +++
.../controls/MatrixifyDimensionControl.test.tsx | 119 +++++++++++++++++++++
.../controls/MatrixifyDimensionControl.tsx | 36 +++----
4 files changed, 249 insertions(+), 22 deletions(-)
diff --git
a/superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx
b/superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx
index 5739496f77..a5b1665056 100644
--- a/superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx
+++ b/superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx
@@ -27,6 +27,8 @@ import { t } from '@apache-superset/core';
import {
DatasourceType,
getChartControlPanelRegistry,
+ isFeatureEnabled,
+ FeatureFlag,
} from '@superset-ui/core';
import { defaultControls, defaultState } from 'src/explore/store';
import { ExplorePageState } from 'src/explore/types';
@@ -36,6 +38,13 @@ import {
ControlPanelsContainerProps,
} from 'src/explore/components/ControlPanelsContainer';
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(),
+}));
+
+const mockIsFeatureEnabled = isFeatureEnabled as jest.Mock;
+
const FormDataMock = () => {
const formData = useSelector(
(state: ExplorePageState) => state.explore.form_data,
@@ -90,10 +99,14 @@ describe('ControlPanelsContainer', () => {
beforeEach(() => {
getChartControlPanelRegistry().registerValue('table', defaultTableConfig);
+ jest.clearAllMocks();
+ // Default: feature disabled
+ mockIsFeatureEnabled.mockReturnValue(false);
});
afterEach(() => {
getChartControlPanelRegistry().remove('table');
+ jest.clearAllMocks();
});
afterAll(() => {
@@ -277,4 +290,92 @@ describe('ControlPanelsContainer', () => {
expect(screen.getByText('Shift start date')).not.toBeVisible();
expect(screen.getByText('Calculation type')).not.toBeVisible();
});
+
+ test('should stay on Matrixify tab when matrixify is enabled', async () => {
+ // Enable Matrixify feature flag
+ mockIsFeatureEnabled.mockImplementation(
+ (featureFlag: FeatureFlag) => featureFlag === FeatureFlag.Matrixify,
+ );
+
+ const props = getDefaultProps();
+ props.form_data = {
+ ...props.form_data,
+ matrixify_enable_vertical_layout: true,
+ };
+
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+
+ // Check that Matrixify tab exists and is active
+ await waitFor(() => {
+ const matrixifyTab = screen.getByRole('tab', { name: /matrixify/i });
+ expect(matrixifyTab).toBeInTheDocument();
+ expect(matrixifyTab).toHaveAttribute('aria-selected', 'true');
+ });
+
+ // Simulate saving with updated dimension values
+ const updatedProps = {
+ ...props,
+ form_data: {
+ ...props.form_data,
+ matrixify_enable_vertical_layout: true,
+ matrixify_dimension_columns: {
+ dimension: 'country',
+ values: ['USA', 'Canada'],
+ },
+ },
+ };
+
+ rerender(<ControlPanelsContainer {...updatedProps} />);
+
+ // Matrixify tab should still be active after rerender
+ await waitFor(() => {
+ const matrixifyTabAfterSave = screen.getByRole('tab', {
+ name: /matrixify/i,
+ });
+ expect(matrixifyTabAfterSave).toHaveAttribute('aria-selected', 'true');
+ });
+ });
+
+ test('should automatically switch to Matrixify tab when matrixify becomes
enabled', async () => {
+ // Enable Matrixify feature flag
+ mockIsFeatureEnabled.mockImplementation(
+ (featureFlag: FeatureFlag) => featureFlag === FeatureFlag.Matrixify,
+ );
+
+ const props = getDefaultProps();
+
+ const { rerender } = render(<ControlPanelsContainer {...props} />, {
+ useRedux: true,
+ });
+
+ // Initially, Data tab should be active
+ const dataTab = screen.getByRole('tab', { name: /data/i });
+ expect(dataTab).toHaveAttribute('aria-selected', 'true');
+
+ // Enable matrixify
+ const updatedProps = {
+ ...props,
+ form_data: {
+ ...props.form_data,
+ matrixify_enable_horizontal_layout: true,
+ },
+ };
+
+ rerender(<ControlPanelsContainer {...updatedProps} />);
+
+ // Matrixify tab should now be active
+ await waitFor(() => {
+ const matrixifyTab = screen.getByRole('tab', { name: /matrixify/i });
+ expect(matrixifyTab).toBeInTheDocument();
+ expect(matrixifyTab).toHaveAttribute('aria-selected', 'true');
+ });
+
+ // Data tab should no longer be active
+ expect(screen.getByRole('tab', { name: /data/i })).toHaveAttribute(
+ 'aria-selected',
+ 'false',
+ );
+ });
});
diff --git
a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
index 057a381cad..6f9f3e4f3f 100644
--- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
+++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
@@ -290,6 +290,7 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
const prevChartStatus = usePrevious(props.chart.chartStatus);
const [showDatasourceAlert, setShowDatasourceAlert] = useState(false);
+ const [activeTabKey, setActiveTabKey] = useState<string>(TABS_KEYS.DATA);
const containerRef = useRef<HTMLDivElement>(null);
@@ -795,6 +796,18 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
const showCustomizeTab = customizeSections.length > 0;
const showMatrixifyTab = isFeatureEnabled(FeatureFlag.Matrixify);
+ // Check if matrixify is enabled in form_data
+ const matrixifyIsEnabled =
+ form_data.matrixify_enable_vertical_layout ||
+ form_data.matrixify_enable_horizontal_layout;
+
+ // Auto-switch to Matrixify tab when it's enabled
+ useEffect(() => {
+ if (showMatrixifyTab && matrixifyIsEnabled) {
+ setActiveTabKey(TABS_KEYS.MATRIXIFY);
+ }
+ }, [showMatrixifyTab, matrixifyIsEnabled]);
+
// Check if matrixify sections have validation errors
const matrixifyHasErrors = useMemo(() => {
if (!showMatrixifyTab) return false;
@@ -882,6 +895,8 @@ export const ControlPanelsContainer = (props:
ControlPanelsContainerProps) => {
id="controlSections"
data-test="control-tabs"
allowOverflow={false}
+ activeKey={activeTabKey}
+ onChange={(key: string) => setActiveTabKey(key)}
items={[
{
key: TABS_KEYS.DATA,
diff --git
a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx
b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx
index 0acf0fd89f..961ea5e729 100644
---
a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx
+++
b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx
@@ -430,3 +430,122 @@ test('should clear values when switching from topn to
members mode', async () =>
});
});
});
+
+test('should preserve dimension values when rerendering with same mode', async
() => {
+ // Regression test for bug where values disappear on save
+ const onChange = jest.fn();
+ const value: MatrixifyDimensionControlValue = {
+ dimension: 'country',
+ values: ['USA', 'Australia'],
+ };
+
+ (SupersetClient.get as jest.Mock).mockResolvedValue({
+ json: { result: ['USA', 'Canada', 'Australia', 'Mexico'] },
+ });
+
+ const { rerender } = render(
+ <MatrixifyDimensionControl
+ {...defaultProps}
+ value={value}
+ onChange={onChange}
+ selectionMode="members"
+ />,
+ );
+
+ // Simulate a rerender that would happen during save
+ rerender(
+ <MatrixifyDimensionControl
+ {...defaultProps}
+ value={value}
+ onChange={onChange}
+ selectionMode="members"
+ />,
+ );
+
+ // Values should not be cleared when mode hasn't changed
+ expect(onChange).not.toHaveBeenCalledWith({
+ dimension: 'country',
+ values: [],
+ topNValues: [],
+ });
+
+ // The values should still be present in the UI
+ expect(
+ screen.getByRole('combobox', { name: 'Select dimension values' }),
+ ).toBeInTheDocument();
+});
+
+test('should not clear values on initial render with members mode', async ()
=> {
+ // Regression test: ensure values are not cleared on initial mount
+ const onChange = jest.fn();
+ const value: MatrixifyDimensionControlValue = {
+ dimension: 'country',
+ values: ['USA', 'Australia'],
+ };
+
+ (SupersetClient.get as jest.Mock).mockResolvedValue({
+ json: { result: ['USA', 'Canada', 'Australia', 'Mexico'] },
+ });
+
+ render(
+ <MatrixifyDimensionControl
+ {...defaultProps}
+ value={value}
+ onChange={onChange}
+ selectionMode="members"
+ />,
+ );
+
+ // onChange should not be called to clear values on initial render
+ expect(onChange).not.toHaveBeenCalledWith({
+ dimension: 'country',
+ values: [],
+ topNValues: [],
+ });
+
+ // The values selector should be visible
+ expect(
+ screen.getByRole('combobox', { name: 'Select dimension values' }),
+ ).toBeInTheDocument();
+});
+
+test('should preserve values when other props change but mode stays the same',
async () => {
+ // Regression test for save scenario where form_data updates trigger
rerenders
+ const onChange = jest.fn();
+ const value: MatrixifyDimensionControlValue = {
+ dimension: 'country',
+ values: ['USA', 'Australia'],
+ };
+
+ (SupersetClient.get as jest.Mock).mockResolvedValue({
+ json: { result: ['USA', 'Canada', 'Australia', 'Mexico'] },
+ });
+
+ const { rerender } = render(
+ <MatrixifyDimensionControl
+ {...defaultProps}
+ value={value}
+ onChange={onChange}
+ selectionMode="members"
+ formData={{ some_field: 'initial' }}
+ />,
+ );
+
+ // Simulate form_data change (like what happens during save)
+ rerender(
+ <MatrixifyDimensionControl
+ {...defaultProps}
+ value={value}
+ onChange={onChange}
+ selectionMode="members"
+ formData={{ some_field: 'updated', another_field: 'new' }}
+ />,
+ );
+
+ // Values should not be cleared when only form_data changes
+ expect(onChange).not.toHaveBeenCalledWith({
+ dimension: 'country',
+ values: [],
+ topNValues: [],
+ });
+});
diff --git
a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx
b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx
index 87a1efe291..35face1e70 100644
---
a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx
+++
b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx
@@ -82,17 +82,21 @@ export default function MatrixifyDimensionControl(
// Reset values when selection mode changes
useEffect(() => {
- if (prevSelectionMode.current !== selectionMode) {
- prevSelectionMode.current = selectionMode;
- if (value?.values && value.values.length > 0) {
- onChange({
- dimension: value.dimension,
- values: [],
- topNValues: [],
- });
- }
+ // Only clear values when actually switching between modes, not on initial
load or re-render
+ if (
+ prevSelectionMode.current !== selectionMode &&
+ prevSelectionMode.current !== undefined &&
+ value?.dimension
+ ) {
+ // Clear values when switching between members and topn modes
+ onChange({
+ dimension: value.dimension,
+ values: [],
+ topNValues: [],
+ });
}
- }, [selectionMode, value]);
+ prevSelectionMode.current = selectionMode;
+ }, [selectionMode]);
// Initialize dimension options from datasource
useEffect(() => {
@@ -175,18 +179,6 @@ export default function MatrixifyDimensionControl(
// Load TopN values when in TopN mode
useEffect(() => {
if (!value?.dimension || !datasource || selectionMode !== 'topn') {
- // Clear values when switching away from topn mode
- if (
- selectionMode !== 'topn' &&
- value?.values &&
- value.values.length > 0
- ) {
- onChange({
- dimension: value.dimension,
- values: [],
- topNValues: [],
- });
- }
return undefined;
}
