Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions superset-frontend/src/dashboard/components/Dashboard.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -332,4 +332,73 @@ describe('Dashboard', () => {
expect(mockTriggerQuery).not.toHaveBeenCalled();
});
});

test('should NOT call refresh when ownDataCharts content is unchanged', () => {
// When only clientView changes, getRelevantDataMask strips it,
// so Dashboard receives identical ownDataCharts and should not refresh
const initialOwnDataCharts = {
1: { pageSize: 10, currentPage: 0 },
};

const { rerender } = renderDashboard({
ownDataCharts: initialOwnDataCharts,
dashboardState: {
...dashboardState,
editMode: false,
},
});

// Rerender with same ownDataCharts (simulates clientView-only change after stripping)
rerender(
<PluginContext.Provider value={{ loading: false }}>
<Dashboard
{...props}
ownDataCharts={initialOwnDataCharts}
dashboardState={{
...dashboardState,
editMode: false,
}}
>
<ChildrenComponent />
</Dashboard>
</PluginContext.Provider>,
);

expect(mockTriggerQuery).not.toHaveBeenCalled();
});

test('should call refresh when ownDataCharts pageSize changes', () => {
const initialOwnDataCharts = {
1: { pageSize: 10, currentPage: 0 },
};

const { rerender } = renderDashboard({
ownDataCharts: initialOwnDataCharts,
dashboardState: {
...dashboardState,
editMode: false,
},
});

const updatedOwnDataCharts = {
1: { pageSize: 20, currentPage: 0 },
};

rerender(
<PluginContext.Provider value={{ loading: false }}>
<Dashboard
{...props}
ownDataCharts={updatedOwnDataCharts}
dashboardState={{
...dashboardState,
editMode: false,
}}
>
<ChildrenComponent />
</Dashboard>
</PluginContext.Provider>,
);

expect(mockTriggerQuery).toHaveBeenCalledWith(true, '1');
});
});
150 changes: 150 additions & 0 deletions superset-frontend/src/dashboard/util/activeAllDashboardFilters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,3 +165,153 @@ test('should preserve the structure of the property value', () => {
},
});
});

test('should strip clientView from ownState to prevent infinite reload loops', () => {
// TableChart writes clientView to ownState on every filtered-row change
// If clientView is not stripped, Dashboard treats it as a chart-state change
// and re-triggers queries, causing an infinite reload loop
const mockDataMaskWithClientView: DataMaskStateWithId = {
chart1: {
id: 'chart1',
ownState: {
pageSize: 10,
currentPage: 0,
// clientView is added by TableChart for export functionality
// but should NOT trigger chart re-queries
clientView: {
rows: [{ id: 1, name: 'Test' }],
columns: [{ key: 'id' }, { key: 'name' }],
count: 1,
},
},
},
chart2: {
id: 'chart2',
ownState: {
sortBy: [{ id: 'col1', desc: true }],
clientView: {
rows: [{ id: 2, name: 'Test2' }],
columns: [{ key: 'id' }, { key: 'name' }],
count: 1,
},
},
},
};

const result = getRelevantDataMask(mockDataMaskWithClientView, 'ownState');

// clientView should be stripped from ownState
// Only pageSize, currentPage, sortBy etc should remain
expect(result).toEqual({
chart1: {
pageSize: 10,
currentPage: 0,
},
chart2: {
sortBy: [{ id: 'col1', desc: true }],
},
});
});

test('should return equal results when only clientView changes', () => {
// When TableChart updates clientView (on every filtered-row change),
// the selector output should remain equal, so Dashboard.jsx's
// areObjectsEqual comparison returns true and doesn't trigger re-queries

const dataMaskBefore: DataMaskStateWithId = {
chart1: {
id: 'chart1',
ownState: {
pageSize: 10,
currentPage: 0,
clientView: {
rows: [{ id: 1, name: 'Original' }],
count: 1,
},
},
},
};

const dataMaskAfter: DataMaskStateWithId = {
chart1: {
id: 'chart1',
ownState: {
pageSize: 10,
currentPage: 0,
// clientView changed (simulating TableChart updating filtered rows)
clientView: {
rows: [
{ id: 1, name: 'Updated' },
{ id: 2, name: 'New Row' },
],
count: 2,
},
},
},
};

const resultBefore = getRelevantDataMask(dataMaskBefore, 'ownState');
const resultAfter = getRelevantDataMask(dataMaskAfter, 'ownState');

// Both results should be equal since clientView is stripped
// This is what prevents the infinite reload loop in Dashboard
expect(resultBefore).toEqual(resultAfter);
expect(resultBefore).toEqual({
chart1: { pageSize: 10, currentPage: 0 },
});
});

test('should return extraFormData unchanged (clientView stripping only applies to ownState)', () => {
// Verify extraFormData is passed through without modification
const mockDataMask: DataMaskStateWithId = {
filter1: {
id: 'filter1',
extraFormData: {
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
granularity_sqla: 'ds',
},
},
filter2: {
id: 'filter2',
extraFormData: {
time_range: 'Last 7 days',
},
},
};

const result = getRelevantDataMask(mockDataMask, 'extraFormData');

// extraFormData should be returned unchanged
expect(result).toEqual({
filter1: {
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
granularity_sqla: 'ds',
},
filter2: {
time_range: 'Last 7 days',
},
});
});

test('should return filterState unchanged (clientView stripping only applies to ownState)', () => {
// Verify filterState is passed through without modification
const mockDataMask: DataMaskStateWithId = {
filter1: {
id: 'filter1',
filterState: {
value: ['A', 'B'],
label: 'Categories A and B',
},
},
};

const result = getRelevantDataMask(mockDataMask, 'filterState');

// filterState should be returned unchanged
expect(result).toEqual({
filter1: {
value: ['A', 'B'],
label: 'Categories A and B',
},
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
JsonObject,
PartialFilters,
} from '@superset-ui/core';
import { omit } from 'lodash';
import { ActiveFilters, ChartConfiguration } from '../types';

export const getRelevantDataMask = (
Expand All @@ -31,7 +32,21 @@ export const getRelevantDataMask = (
Object.fromEntries(
Object.values(dataMask)
.filter(item => item[prop])
.map(item => [item.id, item[prop]]),
.map(item => {
const value = item[prop];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be handled by the chart component instead of getRelevantDataMask? I'm wondering if clientView might be useful for another caller of getRelevantDataMask and should just be discarded by the chart when re-rendering.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think modifying the table chart plugin (separate package) to conditionally write clientView based on context could potentially be more complex and could introduce regressions. Also this mirrors how Explore does it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have two options:

  1. Given the "relevant" word on getRelevantDataMask, we might consider that clientView is not relevant and your solution would be correct. This means that clientView is never relevant for any caller of this function which might be true given that there's only one caller currently (DashboardPage.tsx).
  2. If clientView might be useful for other callers, we could change DashboardPage.tsx and omit the property there:
const selectRelevantDatamask = createSelector(
  (state: RootState) => state.dataMask,
  dataMask => omit(getRelevantDataMask(dataMask, 'ownState'), ['clientView']),
);

I'm good with either solution, so I'm approving the PR.

// TableChart writes clientView to ownState on every filtered-row change for export
// but clientView changes should NOT trigger chart re-queries
// Only clone when clientView exists to avoid unnecessary allocations
if (
prop === 'ownState' &&
value &&
typeof value === 'object' &&
'clientView' in value
) {
return [item.id, omit(value, ['clientView'])];
}
return [item.id, value];
}),
);

interface LayerInfo {
Expand Down
Loading