Rework the HTML report - #133
Conversation
The rendered report was hard to read and leaked internals. Move HTML rendering out of render.go into an html/template with embedded CSS and JS, and rework what it puts on the page. Markdown output is unchanged. - Heading is the scanned scope from _meta.json, not the run directory name; the run id and a human-readable generated stamp move to the footer. - Severity counts become tiles that are also the severity filter, so the counts are no longer printed twice. - Sidebar lists rules grouped by their cat-NN prefix with counts, a filter box, and jump-to-severity links, so no rule name has to be remembered. - Cards carry a severity ribbon, fill the column, and hide description, provenance, subject and rule id behind a Details toggle. - Subject line is a location: org/repo/file, or the scope plus the subject kind when the finding is not anchored to a file. Built from canonical fields, so fingerprints do not change. - Search matches titles only. Light and dark themes with an explicit toggle. html/template's contextual escaping also closes a gap: Rule.URL was previously interpolated into href after only HTML-escaping, so a non-http scheme survived. The document stays a single self-contained file with no external requests, which is what makes it usable offline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review
Critical Issues
internal/report/assets/report.html:129dropsRule.ScenarioIDfrom HTML output. The previous renderer displayed it alongside the rule ID, so scenario-linked findings now lose meaningful traceability. Render the scenario when non-empty and add regression coverage.
Security
No security concerns flagged.
Suggestions
No suggestions.
Reviewed by Codex (gpt-5.6-sol)
There was a problem hiding this comment.
Claude Review
Critical issues
None. The rework is clean: renderHTML now returns ([]byte, error) and both the html and all cases in report.go are updated to propagate it; the new reportMeta fields (Platform, Scope, Generated) map to real engine.State fields; and all template funcs (location, ruleID, codeCaption, provRows, hasDetails, etc.) are registered and back existing helpers. hasDetails gates on exactly the four fields the Details block renders.
Security
- Net improvement, not a regression: replacing hand-rolled
html.EscapeStringstring-building withhtml/templatemeansRule.URLis now URL-sanitized inhrefcontext, so ajavascript:scheme is neutered (previously it survived escaping).report.js/report.cssare inlined viatemplate.JS/template.CSSfrom trusted//go:embedassets, and all finding-derived text goes through auto-escaping. No new network calls or secrets handling.
Test coverage
Present and strong — html_test.go covers nil-field tolerance, the XSS/non-http URL case, empty-report shape, rule grouping, scope-vs-runID heading, and locationOf edge cases against concrete inputs rather than implementation shape.
No critical issues — LGTM pending human review.
WalkthroughThe report package now renders a complete HTML report with metadata, findings, filters, navigation, evidence, remediation, provenance, and run details. It embeds CSS and JavaScript assets. Client-side behavior supports themes, filtering, searching, navigation, detail expansion, printing, and empty states. Report metadata now stores platform, scope, and generation time. HTML rendering errors are returned before output. Tests cover rendering, formatting, grouping, sanitization, fallbacks, and empty reports. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/report/assets/report.html (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive both search inputs an explicit accessible name.
Both inputs rely on the placeholder alone. The other controls in this template carry
aria-label. Match them.♿ Proposed change
- <input id="ruleQ" class="side-input" type="search" placeholder="Filter rules…" autocomplete="off" spellcheck="false"> + <input id="ruleQ" class="side-input" type="search" aria-label="Filter rules" placeholder="Filter rules…" autocomplete="off" spellcheck="false">- <input id="q" type="search" placeholder="Search finding titles…" autocomplete="off" spellcheck="false"> + <input id="q" type="search" aria-label="Search finding titles" placeholder="Search finding titles…" autocomplete="off" spellcheck="false">Also applies to: 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/report/assets/report.html` at line 63, Update both search inputs, including the controls identified by ruleQ and the corresponding input at the other referenced location, to include explicit aria-label attributes matching their filtering purpose; preserve their existing placeholder and other attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/report/assets/report.css`:
- Line 12: Darken the --faint CSS variable in both its definitions, including
the matching value used by the dark blocks, so small text reaches at least 4.5:1
contrast on the relevant light and dark surfaces while preserving all existing
token usages.
- Around line 26-27: Update the --sans and --mono font-family declarations to
quote all multi-word and proper-name families, including existing names such as
"Segoe UI" and "SF Mono", so every family flagged by value-keyword-case follows
the stylesheet’s quoting convention.
In `@internal/report/assets/report.js`:
- Around line 126-127: Update the beforeprint handler to synchronize the expand
control after calling setDetails(true): set expand.dataset.open to the open
state and update the button text to match all details being expanded, reusing
the existing symbols and control-update logic.
---
Nitpick comments:
In `@internal/report/assets/report.html`:
- Line 63: Update both search inputs, including the controls identified by ruleQ
and the corresponding input at the other referenced location, to include
explicit aria-label attributes matching their filtering purpose; preserve their
existing placeholder and other attributes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58cf8493-637f-40c2-8919-3f0abb859764
📒 Files selected for processing (7)
internal/report/assets/report.cssinternal/report/assets/report.htmlinternal/report/assets/report.jsinternal/report/html.gointernal/report/html_test.gointernal/report/render.gointernal/report/report.go
| --raised: #ffffff; | ||
| --ink: #11151a; | ||
| --muted: #5b6673; | ||
| --faint: #8a94a1; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Darken --faint; it fails WCAG AA for small text.
--faint is about 3.1:1 on white and about 3.8:1 on the dark surface. AA needs 4.5:1. The token drives 10.5–12px text: .eyebrow, .side-h, .side-n, .fid, .lbl-sec, and the footer. The same value at line 62 needs the matching change.
♿ Proposed token values
- --faint: `#8a94a1`;
+ --faint: `#6b7480`;Dark blocks at lines 40 and 62:
- --faint: `#6d7783`;
+ --faint: `#8b95a1`;Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/report/assets/report.css` at line 12, Darken the --faint CSS
variable in both its definitions, including the matching value used by the dark
blocks, so small text reaches at least 4.5:1 contrast on the relevant light and
dark surfaces while preserving all existing token usages.
| --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Helvetica, Arial, sans-serif; | ||
| --mono: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Quote the multi-word and proper-name font families to clear stylelint.
Stylelint flags six unquoted family names under value-keyword-case. Quoting matches the already-quoted "Segoe UI" and "SF Mono".
🎨 Proposed fix
- --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Helvetica, Arial, sans-serif;
- --mono: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace;
+ --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", "Roboto", "Helvetica", "Arial", sans-serif;
+ --mono: ui-monospace, "SFMono-Regular", "SF Mono", "Menlo", "Consolas", monospace;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Helvetica, Arial, sans-serif; | |
| --mono: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace; | |
| --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", "Roboto", "Helvetica", "Arial", sans-serif; | |
| --mono: ui-monospace, "SFMono-Regular", "SF Mono", "Menlo", "Consolas", monospace; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 26-26: Expected "Roboto" to be "roboto" (value-keyword-case)
(value-keyword-case)
[error] 26-26: Expected "Helvetica" to be "helvetica" (value-keyword-case)
(value-keyword-case)
[error] 26-26: Expected "Arial" to be "arial" (value-keyword-case)
(value-keyword-case)
[error] 27-27: Expected "SFMono-Regular" to be "sfmono-regular" (value-keyword-case)
(value-keyword-case)
[error] 27-27: Expected "Menlo" to be "menlo" (value-keyword-case)
(value-keyword-case)
[error] 27-27: Expected "Consolas" to be "consolas" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/report/assets/report.css` around lines 26 - 27, Update the --sans
and --mono font-family declarations to quote all multi-word and proper-name
families, including existing names such as "Segoe UI" and "SF Mono", so every
family flagged by value-keyword-case follows the stylesheet’s quoting
convention.
Source: Linters/SAST tools
| // Printing a report should not silently drop whatever sits behind a toggle. | ||
| addEventListener("beforeprint", function () { setDetails(true); }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize the expand control before printing.
beforeprint opens every details.more element. It does not update expand.dataset.open or the button text. After printing, the button can display Expand all while all details are open. The first click then has no visible effect.
Proposed fix
- addEventListener("beforeprint", function () { setDetails(true); });
+ addEventListener("beforeprint", function () {
+ setDetails(true);
+ expand.dataset.open = "true";
+ expand.textContent = "Collapse all";
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Printing a report should not silently drop whatever sits behind a toggle. | |
| addEventListener("beforeprint", function () { setDetails(true); }); | |
| // Printing a report should not silently drop whatever sits behind a toggle. | |
| addEventListener("beforeprint", function () { | |
| setDetails(true); | |
| expand.dataset.open = "true"; | |
| expand.textContent = "Collapse all"; | |
| }); |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 126-126: React's useState should not be directly called
Context: setDetails(true)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/report/assets/report.js` around lines 126 - 127, Update the
beforeprint handler to synchronize the expand control after calling
setDetails(true): set expand.dataset.open to the open state and update the
button text to match all details being expanded, reusing the existing symbols
and control-update logic.
There was a problem hiding this comment.
Gemini Review
Critical Issues
- Nil pointer dereference on unreadable
_meta.json: Ininternal/report/report.go, the call toengine.LoadState(runDir)ignores its returned error. While a missing file is handled safely byLoadState, a corrupted or unreadable_meta.jsoncauses it to return anilstate and an error. Ignoring this error and immediately accessingstate.Platformandstate.Scope(introduced in this PR) will trigger a runtime panic. Initialize a fallbackStatestruct ifstate == nil.
Security
- No security concerns flagged. The migration to
html/templatenatively handles contextual HTML escaping and safely neuters potentially harmful links (e.g.,javascript:URIs inRule.URLbecoming#ZgotmplZ), acting as a robust replacement for the previous manualhtml.EscapeStringapproach.
Suggestions
- Dynamic dark mode listener: The JS toggle in
internal/report/assets/report.jsevaluates theprefers-color-schememedia query once on load. Consider attaching an event listener (matchMedia("(prefers-color-scheme: dark)").addEventListener("change", ...)) to automatically update the theme if the user's OS preference changes dynamically while the report is open (assuming no manual override exists inlocalStorage). emptyInlinescope in tests: Ininternal/report/html_test.go, theemptyInlineutility only targets and strips the first occurrence of a tag. This works perfectly for the current single-asset document shape, but using a loop or regex replacement would be more robust against future changes that introduce multiple<style>or<script>blocks.
Reviewed by Gemini (gemini-3.1-pro-preview)
2 participants
HTML report format updates.