Skip to content

Commit 0770786

Browse files
sadpandajoeclaude
andcommitted
fix(dashboard): prevent table chart infinite reload loop
Strip clientView from ownState in getRelevantDataMask to prevent infinite reload loops in dashboard context. TableChart writes clientView to ownState on every filtered-row change for export functionality, but clientView changes should NOT trigger chart re-queries. This matches the behavior already present in ExploreViewContainer which strips clientView before comparing ownState changes. Fixes #36595 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 76f1b5e commit 0770786

File tree

3 files changed

+235
-1
lines changed

3 files changed

+235
-1
lines changed

superset-frontend/src/dashboard/components/Dashboard.test.jsx

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,4 +332,80 @@ describe('Dashboard', () => {
332332
expect(mockTriggerQuery).not.toHaveBeenCalled();
333333
});
334334
});
335+
336+
// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
337+
describe('ownDataCharts updates', () => {
338+
test('should NOT call refresh when ownDataCharts content is unchanged (clientView stripped upstream)', () => {
339+
// This test verifies Dashboard behavior when ownDataCharts stays the same.
340+
// In the real app, getRelevantDataMask strips clientView from dataMask.ownState,
341+
// so when only clientView changes, Dashboard receives identical ownDataCharts.
342+
const initialOwnDataCharts = {
343+
1: { pageSize: 10, currentPage: 0 },
344+
};
345+
346+
const { rerender } = renderDashboard({
347+
ownDataCharts: initialOwnDataCharts,
348+
dashboardState: {
349+
...dashboardState,
350+
editMode: false,
351+
},
352+
});
353+
354+
// Rerender with same ownDataCharts (this is what happens after clientView is stripped)
355+
rerender(
356+
<PluginContext.Provider value={{ loading: false }}>
357+
<Dashboard
358+
{...props}
359+
ownDataCharts={initialOwnDataCharts}
360+
dashboardState={{
361+
...dashboardState,
362+
editMode: false,
363+
}}
364+
>
365+
<ChildrenComponent />
366+
</Dashboard>
367+
</PluginContext.Provider>,
368+
);
369+
370+
// No refresh should occur since ownDataCharts content is unchanged
371+
expect(mockTriggerQuery).not.toHaveBeenCalled();
372+
});
373+
374+
test('should call refresh when real ownDataCharts changes occur', () => {
375+
const initialOwnDataCharts = {
376+
1: { pageSize: 10, currentPage: 0 },
377+
};
378+
379+
const { rerender } = renderDashboard({
380+
ownDataCharts: initialOwnDataCharts,
381+
dashboardState: {
382+
...dashboardState,
383+
editMode: false,
384+
},
385+
});
386+
387+
// Real change: pageSize changed
388+
const updatedOwnDataCharts = {
389+
1: { pageSize: 20, currentPage: 0 },
390+
};
391+
392+
rerender(
393+
<PluginContext.Provider value={{ loading: false }}>
394+
<Dashboard
395+
{...props}
396+
ownDataCharts={updatedOwnDataCharts}
397+
dashboardState={{
398+
...dashboardState,
399+
editMode: false,
400+
}}
401+
>
402+
<ChildrenComponent />
403+
</Dashboard>
404+
</PluginContext.Provider>,
405+
);
406+
407+
// pageSize changed, so chart should be refreshed
408+
expect(mockTriggerQuery).toHaveBeenCalledWith(true, '1');
409+
});
410+
});
335411
});

superset-frontend/src/dashboard/util/activeAllDashboardFilters.test.ts

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,3 +165,154 @@ test('should preserve the structure of the property value', () => {
165165
},
166166
});
167167
});
168+
169+
test('should strip clientView from ownState to prevent infinite reload loops', () => {
170+
// This test verifies the fix for issue #36595
171+
// TableChart writes clientView to ownState on every filtered-row change
172+
// If clientView is not stripped, Dashboard treats it as a chart-state change
173+
// and re-triggers queries, causing an infinite reload loop
174+
const mockDataMaskWithClientView: DataMaskStateWithId = {
175+
chart1: {
176+
id: 'chart1',
177+
ownState: {
178+
pageSize: 10,
179+
currentPage: 0,
180+
// clientView is added by TableChart for export functionality
181+
// but should NOT trigger chart re-queries
182+
clientView: {
183+
rows: [{ id: 1, name: 'Test' }],
184+
columns: [{ key: 'id' }, { key: 'name' }],
185+
count: 1,
186+
},
187+
},
188+
},
189+
chart2: {
190+
id: 'chart2',
191+
ownState: {
192+
sortBy: [{ id: 'col1', desc: true }],
193+
clientView: {
194+
rows: [{ id: 2, name: 'Test2' }],
195+
columns: [{ key: 'id' }, { key: 'name' }],
196+
count: 1,
197+
},
198+
},
199+
},
200+
};
201+
202+
const result = getRelevantDataMask(mockDataMaskWithClientView, 'ownState');
203+
204+
// clientView should be stripped from ownState
205+
// Only pageSize, currentPage, sortBy etc should remain
206+
expect(result).toEqual({
207+
chart1: {
208+
pageSize: 10,
209+
currentPage: 0,
210+
},
211+
chart2: {
212+
sortBy: [{ id: 'col1', desc: true }],
213+
},
214+
});
215+
});
216+
217+
test('should return equal results when only clientView changes', () => {
218+
// When TableChart updates clientView (on every filtered-row change),
219+
// the selector output should remain equal, so Dashboard.jsx's
220+
// areObjectsEqual comparison returns true and doesn't trigger re-queries
221+
222+
const dataMaskBefore: DataMaskStateWithId = {
223+
chart1: {
224+
id: 'chart1',
225+
ownState: {
226+
pageSize: 10,
227+
currentPage: 0,
228+
clientView: {
229+
rows: [{ id: 1, name: 'Original' }],
230+
count: 1,
231+
},
232+
},
233+
},
234+
};
235+
236+
const dataMaskAfter: DataMaskStateWithId = {
237+
chart1: {
238+
id: 'chart1',
239+
ownState: {
240+
pageSize: 10,
241+
currentPage: 0,
242+
// clientView changed (simulating TableChart updating filtered rows)
243+
clientView: {
244+
rows: [
245+
{ id: 1, name: 'Updated' },
246+
{ id: 2, name: 'New Row' },
247+
],
248+
count: 2,
249+
},
250+
},
251+
},
252+
};
253+
254+
const resultBefore = getRelevantDataMask(dataMaskBefore, 'ownState');
255+
const resultAfter = getRelevantDataMask(dataMaskAfter, 'ownState');
256+
257+
// Both results should be equal since clientView is stripped
258+
// This is what prevents the infinite reload loop in Dashboard
259+
expect(resultBefore).toEqual(resultAfter);
260+
expect(resultBefore).toEqual({
261+
chart1: { pageSize: 10, currentPage: 0 },
262+
});
263+
});
264+
265+
test('should return extraFormData unchanged (clientView stripping only applies to ownState)', () => {
266+
// Verify extraFormData is passed through without modification
267+
const mockDataMask: DataMaskStateWithId = {
268+
filter1: {
269+
id: 'filter1',
270+
extraFormData: {
271+
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
272+
granularity_sqla: 'ds',
273+
},
274+
},
275+
filter2: {
276+
id: 'filter2',
277+
extraFormData: {
278+
time_range: 'Last 7 days',
279+
},
280+
},
281+
};
282+
283+
const result = getRelevantDataMask(mockDataMask, 'extraFormData');
284+
285+
// extraFormData should be returned unchanged
286+
expect(result).toEqual({
287+
filter1: {
288+
filters: [{ col: 'country', op: 'IN', val: ['USA'] }],
289+
granularity_sqla: 'ds',
290+
},
291+
filter2: {
292+
time_range: 'Last 7 days',
293+
},
294+
});
295+
});
296+
297+
test('should return filterState unchanged (clientView stripping only applies to ownState)', () => {
298+
// Verify filterState is passed through without modification
299+
const mockDataMask: DataMaskStateWithId = {
300+
filter1: {
301+
id: 'filter1',
302+
filterState: {
303+
value: ['A', 'B'],
304+
label: 'Categories A and B',
305+
},
306+
},
307+
};
308+
309+
const result = getRelevantDataMask(mockDataMask, 'filterState');
310+
311+
// filterState should be returned unchanged
312+
expect(result).toEqual({
313+
filter1: {
314+
value: ['A', 'B'],
315+
label: 'Categories A and B',
316+
},
317+
});
318+
});

superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
JsonObject,
2323
PartialFilters,
2424
} from '@superset-ui/core';
25+
import { omit } from 'lodash';
2526
import { ActiveFilters, ChartConfiguration } from '../types';
2627

2728
export const getRelevantDataMask = (
@@ -31,7 +32,13 @@ export const getRelevantDataMask = (
3132
Object.fromEntries(
3233
Object.values(dataMask)
3334
.filter(item => item[prop])
34-
.map(item => [item.id, item[prop]]),
35+
.map(item => [
36+
item.id,
37+
// Strip clientView from ownState to prevent infinite reload loops (issue #36595)
38+
// TableChart writes clientView to ownState on every filtered-row change for export
39+
// but clientView changes should NOT trigger chart re-queries
40+
prop === 'ownState' ? omit(item[prop], ['clientView']) : item[prop],
41+
]),
3542
);
3643

3744
interface LayerInfo {

0 commit comments

Comments
 (0)