Skip to content

Commit 3d3357e

Browse files
Updated per CodeRabbit review
1 parent 60ace64 commit 3d3357e

4 files changed

Lines changed: 143 additions & 2 deletions

File tree

‎components/report-view.jsx‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ import {
2020
enhanceReport,
2121
bindPopupHandlers,
2222
stashStatefulDom,
23-
graftStatefulDom
23+
graftStatefulDom,
24+
destroyBubbleCharts
2425
} from '../lib/report-view';
2526
import { enhanceTables } from '../lib/table-enhancer';
2627
import { renderErrorPage, logError } from '../lib/error-handler';
@@ -176,6 +177,10 @@ export default function ReportView({
176177
if (cancelled) return;
177178

178179
const payloadChanged = lastEnhancedPayloadRef.current !== payload;
180+
// The replacement render throws the old canvases away with the
181+
// markup: destroy their Chart.js instances first so none outlives
182+
// its canvas (enhanceReport builds fresh charts for the new DOM).
183+
if (payloadChanged) destroyBubbleCharts();
179184
// Stash live chart canvases and rendered graphs before the innerHTML
180185
// swap throws the old DOM away (no-op on first render — there is
181186
// nothing stateful in the loading placeholder).

‎lib/report-view.js‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,20 +420,37 @@ const bubbleCharts = new Map();
420420
/**
421421
* Redraws every live bubble chart so legend text re-resolves against the
422422
* newly active theme. Entries whose canvas left the DOM (report re-rendered
423-
* or torn down) are pruned instead of redrawn.
423+
* or torn down) are destroyed — releasing their Chart.js resources — and
424+
* pruned instead of redrawn.
424425
*
425426
* @returns {void}
426427
*/
427428
function redrawChartsForTheme() {
428429
for (const [canvas, chart] of bubbleCharts) {
429430
if (!canvas.isConnected) {
431+
if (chart && typeof chart.destroy === 'function') chart.destroy();
430432
bubbleCharts.delete(canvas);
431433
continue;
432434
}
433435
if (chart && typeof chart.update === 'function') chart.update();
434436
}
435437
}
436438

439+
/**
440+
* Destroys every live bubble chart and clears the registry. The report
441+
* component calls this before a payload change replaces the report markup
442+
* (container.innerHTML), so no Chart.js instance outlives its removed
443+
* canvas; enhanceReport then builds fresh charts for the new canvases.
444+
*
445+
* @returns {void}
446+
*/
447+
export function destroyBubbleCharts() {
448+
for (const chart of bubbleCharts.values()) {
449+
if (chart && typeof chart.destroy === 'function') chart.destroy();
450+
}
451+
bubbleCharts.clear();
452+
}
453+
437454
/**
438455
* Binds one delegated listener redrawing charts on theme changes: the
439456
* rf-theme radios (components/theme-toggle.jsx) for explicit switches and

‎tests/integration/report-view.test.jsx‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,53 @@ describe('enhanced report tables: stateful widgets survive interactions', () =>
693693
expect(document.getElementById('overlay').style.display).toBe('block');
694694
hideAllPopups();
695695
});
696+
697+
test('a payload change destroys the previous charts before the markup swap and re-creates them', async () => {
698+
// Every Chart.js construction/teardown, in order, so the test can pin
699+
// that the old instances are released before the replacement render
700+
// builds fresh charts on the new canvases.
701+
const events = [];
702+
const instances = [];
703+
window.Chart = function () {
704+
const chart = {
705+
destroyed: false,
706+
update() {},
707+
destroy() { chart.destroyed = true; events.push('destroy'); }
708+
};
709+
instances.push(chart);
710+
events.push('create');
711+
return chart;
712+
};
713+
markWidgetReady('chart');
714+
respondJsonFor(sampleJson);
715+
const utils = await renderReport();
716+
const firstCanvas = utils.container.querySelector('canvas#chart_GOD');
717+
const firstRenderCreates = instances.length;
718+
expect(firstRenderCreates).toBeGreaterThan(0);
719+
720+
// Switching branches fetches a new payload: the replacement render must
721+
// tear the old Chart.js instances down (they would otherwise outlive
722+
// their removed canvases) and then re-create charts for the fresh DOM.
723+
utils.rerender(_jsx(ReportView, {
724+
username: 'junit-team',
725+
repository: 'junit4',
726+
branch: 'develop',
727+
widgetSettleMs: 25
728+
}));
729+
// The rerender passes through loading/graft phases that reuse or drop
730+
// the old canvas node, so wait on the destruction itself settling.
731+
await waitFor(() => {
732+
expect(events.filter(e => e === 'destroy').length).toBe(firstRenderCreates);
733+
});
734+
expect(utils.container.querySelector('canvas#chart_GOD')).not.toBe(firstCanvas);
735+
736+
expect(instances.slice(0, firstRenderCreates).every(c => c.destroyed)).toBe(true);
737+
// All destroys precede every replacement construction.
738+
const destroys = events.slice(firstRenderCreates, instances.length);
739+
expect(destroys.every(e => e === 'destroy')).toBe(true);
740+
// enhanceReport still builds charts for the replacement canvases.
741+
expect(instances.length).toBe(2 * firstRenderCreates);
742+
}, SLOW_TEST_MS);
696743
});
697744

698745
async function hideAllPopups() {

‎tests/unit/report-view.test.js‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
enhanceReport,
1313
resolveLegendTextColor,
1414
bindThemeChartRedraw,
15+
destroyBubbleCharts,
1516
statefulElementIds,
1617
stashStatefulDom,
1718
graftStatefulDom
@@ -263,6 +264,77 @@ describe('theme-change chart redraw', () => {
263264
expect(god.updates.length).toBeGreaterThan(godRedraws); // still live
264265
expect(brain.updates.length).toBe(brainRedraws); // stale entry pruned, not redrawn
265266
});
267+
268+
it('destroys the chart of a canvas that left the DOM before pruning it', () => {
269+
installThemeRadios('system');
270+
bindThemeChartRedraw();
271+
document.body.innerHTML = `
272+
<input type="radio" name="rf-theme" id="rf-theme-light">
273+
<input type="radio" name="rf-theme" id="rf-theme-dark" checked>
274+
<input type="radio" name="rf-theme" id="rf-theme-system">
275+
<canvas id="chart_GOD"></canvas>`;
276+
const chart = {
277+
destroyed: false,
278+
update() {},
279+
destroy() { chart.destroyed = true; }
280+
};
281+
window.Chart = function () { return chart; };
282+
initBubbleChart(document.getElementById('chart_GOD'), 'God Classes', { bubbles: [] });
283+
expect(chart.destroyed).toBe(false);
284+
285+
// The re-render threw the canvas away: the next theme change must
286+
// release the Chart.js instance, not merely drop the registry entry.
287+
document.getElementById('chart_GOD').remove();
288+
const radio = document.getElementById('rf-theme-light');
289+
radio.checked = true;
290+
radio.dispatchEvent(new window.Event('change', { bubbles: true }));
291+
292+
expect(chart.destroyed).toBe(true);
293+
});
294+
});
295+
296+
describe('destroyBubbleCharts', () => {
297+
afterEach(() => { delete window.Chart; });
298+
299+
it('destroys every live chart and clears the registry', () => {
300+
installThemeRadios('light');
301+
const instances = [];
302+
window.Chart = function () {
303+
const chart = {
304+
destroyed: false,
305+
update() {},
306+
destroy() { chart.destroyed = true; }
307+
};
308+
instances.push(chart);
309+
return chart;
310+
};
311+
initBubbleChart(document.getElementById('chart_GOD'), 'God Classes', { bubbles: [] });
312+
const second = document.createElement('canvas');
313+
second.id = 'chart_BRAIN';
314+
document.body.appendChild(second);
315+
initBubbleChart(second, 'Brain Methods', { bubbles: [] });
316+
expect(instances.length).toBe(2);
317+
318+
destroyBubbleCharts();
319+
expect(instances.map(c => c.destroyed)).toEqual([true, true]);
320+
321+
// Registry cleared: a theme change must not touch the destroyed charts.
322+
const updates = [];
323+
instances.forEach(c => (c.update = () => updates.push('redraw')));
324+
const dark = document.getElementById('rf-theme-dark');
325+
dark.checked = true;
326+
dark.dispatchEvent(new window.Event('change', { bubbles: true }));
327+
expect(updates.length).toBe(0);
328+
});
329+
330+
it('tolerates chart stubs without a destroy method', () => {
331+
installThemeRadios('light');
332+
window.Chart = function () {
333+
return { update() {} }; // no destroy: must not throw
334+
};
335+
initBubbleChart(document.getElementById('chart_GOD'), 'God Classes', { bubbles: [] });
336+
expect(() => destroyBubbleCharts()).not.toThrow();
337+
});
266338
});
267339

268340
describe('initDisharmonyCharts', () => {

0 commit comments

Comments
 (0)