fix: render report summary charts after the table-of-contents rebuild#15195
Open
stevewallone wants to merge 1 commit into
Open
fix: render report summary charts after the table-of-contents rebuild#15195stevewallone wants to merge 1 commit into
stevewallone wants to merge 1 commit into
Conversation
In every *_pdf_report.html template the Executive-Summary charts render from a jQuery ready handler, while the table-of-contents builder runs on window.onload and reassigns #contents.innerHTML. The chart containers live inside #contents, so whenever the charts paint before the TOC rebuild, the innerHTML round-trip re-parses the subtree and silently discards the painted canvases (Chart.js instances in the new UI, Flot canvases in the classic UI), leaving blank chart boxes with no console error. The two orderings race each other: on a warm local cache the async ready callback can land after onload and the charts survive, but with realistic network latency the charts paint first and are destroyed. The symptom was reported as far back as DefectDojo#2294 and DefectDojo#3253 (2020, Flot era) and never fixed - both were auto-closed by the stale bot. Wrap the chart rendering in renderReportCharts() in all seven report templates (engagement, product, product type, test, endpoint, finding, product endpoint) in both template trees (dojo/templates and dojo/templates_classic) and invoke it after the TOC builder has finished (or on a ready handler when the TOC is disabled), so the charts are always created into the final DOM. In five of the seven templates (product, test, endpoint, finding, product endpoint) the window.onload TOC builder was also missing the {% if include_table_of_contents %} guard the other templates have, so it ran even with the table of contents disabled. Because the #contents wrapper is itself rendered only when the TOC is enabled, the ungated handler dereferences a null #contents on its first line and throws an uncaught TypeError before it can rewrite anything (the finding template null-checks #contents and stays silent) - so the charts survive in that configuration, but every such report logs a console error. Those five now carry the same guard as their siblings, so the handler is not emitted when the TOC is off. In the test template the chart code was split between a ready handler and top-level script code; both pieces are consolidated into renderReportCharts() preserving their order, and its call sites stay inside {% if include_executive_summary %} because the function definition is only rendered with the executive summary. Add a regression assertion to the Selenium report tests that the summary charts end up painted when executive summary and table of contents are enabled together (factored into a helper and applied to the product type, product, engagement, test, and product-endpoint report tests). The check is chart-library agnostic (painted canvas pixels), so it holds on both the Chart.js and classic Flot report pages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes a long-standing bug where the Executive-Summary charts on generated HTML reports
render as empty boxes. It was reported as far back as #2294 and #3253 (2020, Flot era);
both were auto-closed by the stale bot and the cause was never identified.
The charts are not failing to build — they are built and then silently destroyed. In
every
*_pdf_report.htmltemplate the charts render from a jQueryreadyhandler, whilethe table-of-contents builder runs on
window.onloadand rebuilds the TOC by reassigning#contents.innerHTML. The chart containers live inside#contents, so aninnerHTMLround-trip re-serializes and re-parses that subtree — and a
<canvas>'s painted bitmapand its Chart.js/Flot instance registration do not survive serialization. Whenever the
charts paint before the TOC rebuild, they are replaced with blank canvases. No exception
is thrown, which is why this reads as a data/charting problem.
The two orderings race. On a warm local cache the async
readycallback can occasionallyland after
onloadand the charts survive; with realistic static-asset latencyonloadfires late, the charts paint first, and they are destroyed near-deterministically — which
matches how #2294/#3253 described it as always-blank in real deployments. Throttling
static assets by a few hundred ms reproduces it every time locally; toggling only the
Table of Contents flag flips the failure on and off.
The fix: wrap the chart-rendering code in a
renderReportCharts()function and invokeit after the TOC builder finishes (or from the
readyhandler when the TOC is disabled),so the charts are always created into the final DOM. Applied to all seven report types
(engagement, product, product type, test, endpoint, finding, product endpoint) in both
template trees —
dojo/templates/(new UI, Chart.js) anddojo/templates_classic/(classic UI, Flot) — 14 templates in total.
Two template-specific notes:
window.onloadTOC builder was missing the{% if include_table_of_contents %}guardits siblings have, so it ran even with the TOC disabled. Because the
#contentswrapperis itself rendered only when the TOC is enabled, the ungated handler dereferences a null
#contentson its first line and throws an uncaughtTypeErrorbefore it can rewriteanything (the finding template null-checks
#contentsand stays silent) — so the chartssurvive in that configuration, but every such report logs a console error. These five now
carry the same guard, so the handler is not emitted when the TOC is off.
readyhandler and top-levelscript code; both pieces are consolidated into
renderReportCharts()preserving theirorder. Its call sites stay inside
{% if include_executive_summary %}because thefunction definition is only rendered with the executive summary.
Test results
Added a regression assertion to the Selenium report tests (
tests/report_builder_test.py):with Executive Summary and Table of Contents both enabled, the summary charts end up with
painted (non-blank) canvas pixels. The check is chart-library-agnostic — it inspects
rendered pixels, not the DOM — so a blank
<canvas>element (which exists in both thebroken and fixed states) does not pass it, and it holds on both the Chart.js and classic
Flot report pages. Factored into a helper and applied to the product type, product,
engagement, test, and product-endpoint report tests.
Manually verified on a live 3.0.0 instance across both UI trees: with static-asset
throttling forcing the race, the URL-routable report types render their charts with the
TOC enabled (previously blank) and with it disabled (regression check), with no console
errors.
ruffclean.A note on the secondary (ungated-
onload) fix: I verified it manually (a load probeacross the report types confirms no console error with the TOC disabled) rather than with
a dedicated assertion, because the test harness globally whitelists
Uncaught TypeError: Cannot read properties of null (reading 'innerHTML')inassertNoConsoleErrors()— which is exactly the error these ungated handlers throw. Thatwhitelist entry can't simply be removed, because
dojo/templates/dojo/custom_html_toc.html(the report builder's TOC, exercised by
generate_HTML_report) has an independent,still-unfixed null-dereference of the same shape. I've left the whitelist in place and
kept this PR scoped to the per-report-type templates; tightening the whitelist and fixing
custom_html_toc.htmlis noted as follow-up below.Known trade-offs and follow-ups (disclosed for reviewers; not addressed here to keep the change minimal)
window.onloadinstead of DOM-ready in the TOC-enabled path.Rendering after the TOC rebuild is what fixes the race, but it does mean charts appear
slightly later on very large reports (
onloadwaits for all subresources, e.g. embeddedfinding images). This is a deliberate trade of a little latency for correctness. The
only way to keep charts on the fast ready handler and avoid the race would be to change
the TOC builder itself (next point).
#contents.innerHTML, and that serialize/re-parse is what destroys live canvases. Amore thorough fix would build the TOC by walking the DOM instead of round-tripping
innerHTML, which would also eliminate the latency trade-off above. That's a largerchange to long-standing code; I kept this PR to the minimal ordering fix.
seven report types in both trees, which is why this bug existed in seven places and why
five copies had drifted to the ungated form. Extracting the shared logic into a single
static JS module would prevent future drift; out of scope here but worth doing.
Documentation
No documentation changes — this is a rendering bugfix with no user-facing feature or
settings change.
Checklist
bugfixbranch.