mirror of
https://github.com/apache/superset.git
synced 2026-06-03 14:49:23 +00:00
fix(dashboard): prevent table chart infinite reload loop (#36686)
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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',
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
@@ -22,6 +22,7 @@ import {
|
||||
JsonObject,
|
||||
PartialFilters,
|
||||
} from '@superset-ui/core';
|
||||
import { omit } from 'lodash';
|
||||
import { ActiveFilters, ChartConfiguration } from '../types';
|
||||
|
||||
export const getRelevantDataMask = (
|
||||
@@ -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];
|
||||
// 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 {
|
||||
|
||||
Reference in New Issue
Block a user