Skip to content

Commit b6e572c

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 ea3d247 commit b6e572c

File tree

3 files changed

+239
-1
lines changed

3 files changed

+239
-1
lines changed

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,4 +332,75 @@ describe('Dashboard', () => {
332332
expect(mockTriggerQuery).not.toHaveBeenCalled();
333333
});
334334
});
335+
336+
// Tests for ownDataCharts updates (issue #36595)
337+
// getRelevantDataMask strips clientView from ownState before Dashboard receives it
338+
test('should NOT call refresh when ownDataCharts content is unchanged', () => {
339+
// When only clientView changes, getRelevantDataMask strips it,
340+
// so Dashboard receives identical ownDataCharts and should not refresh
341+
const initialOwnDataCharts = {
342+
1: { pageSize: 10, currentPage: 0 },
343+
};
344+
345+
const { rerender } = renderDashboard({
346+
ownDataCharts: initialOwnDataCharts,
347+
dashboardState: {
348+
...dashboardState,
349+
editMode: false,
350+
},
351+
});
352+
353+
// Rerender with same ownDataCharts (simulates clientView-only change after stripping)
354+
rerender(
355+
<PluginContext.Provider value={{ loading: false }}>
356+
<Dashboard
357+
{...props}
358+
ownDataCharts={initialOwnDataCharts}
359+
dashboardState={{
360+
...dashboardState,
361+
editMode: false,
362+
}}
363+
>
364+
<ChildrenComponent />
365+
</Dashboard>
366+
</PluginContext.Provider>,
367+
);
368+
369+
expect(mockTriggerQuery).not.toHaveBeenCalled();
370+
});
371+
372+
test('should call refresh when ownDataCharts pageSize changes', () => {
373+
const initialOwnDataCharts = {
374+
1: { pageSize: 10, currentPage: 0 },
375+
};
376+
377+
const { rerender } = renderDashboard({
378+
ownDataCharts: initialOwnDataCharts,
379+
dashboardState: {
380+
...dashboardState,
381+
editMode: false,
382+
},
383+
});
384+
385+
const updatedOwnDataCharts = {
386+
1: { pageSize: 20, currentPage: 0 },
387+
};
388+
389+
rerender(
390+
<PluginContext.Provider value={{ loading: false }}>
391+
<Dashboard
392+
{...props}
393+
ownDataCharts={updatedOwnDataCharts}
394+
dashboardState={{
395+
...dashboardState,
396+
editMode: false,
397+
}}
398+
>
399+
<ChildrenComponent />
400+
</Dashboard>
401+
</PluginContext.Provider>,
402+
);
403+
404+
expect(mockTriggerQuery).toHaveBeenCalledWith(true, '1');
405+
});
335406
});

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: 17 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,22 @@ 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+
const value = item[prop];
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+
// Only clone when clientView exists to avoid unnecessary allocations
41+
if (
42+
prop === 'ownState' &&
43+
value &&
44+
typeof value === 'object' &&
45+
'clientView' in value
46+
) {
47+
return [item.id, omit(value, ['clientView'])];
48+
}
49+
return [item.id, value];
50+
}),
3551
);
3652

3753
interface LayerInfo {

0 commit comments

Comments
 (0)