fix(dashboard): render the full generic type name in card titles - #336
Conversation
The COLLECTIONS titles and `vs` baselines are trusted in-repo literals, but they are markup-shaped: concatenated straight into an innerHTML template, a generic parameter is parsed as a start tag and swallowed. Every card on the grid and every detail page therefore rendered a truncated heading — `IntDictionary` for `IntDictionary<int>`, and the same `vs Dictionary` on cards measured against `Dictionary<int, int>`, `Dictionary<long, int>` and `Dictionary<int, List<int>>` alike. `EnumSet<TEnum>` additionally materialized a stray <tenum> element in the DOM. Escape every label on its way to a sink: `col.title` / `col.vs` / the op label on the grid, `collection.title` / `collection.vs` in the detail page's shell and its no-data notice, and the hasher section's group head. index.html gains one shared `escapeHtml` for both of its scripts, replacing the narrower `esc` that only the hasher section had; detail.html already had the helper and just needed to use it. Guard it in CI. `scripts/check_dashboard_coverage.js` already fails on a card the page would silently drop for want of data; it now also fails on a label the page would silently truncate, by folding each file into logical statements and rejecting any label concatenated into a markup string outside a text sink. The check reports all four pre-fix sites. Closes #328
There was a problem hiding this comment.
Pull request overview
This PR fixes a rendering bug in the benchmark dashboard where generic type parameters (e.g., <int>) were being parsed as HTML and dropped from visible titles/labels. It does this by consistently escaping markup-shaped labels before they reach innerHTML, and adds a CI guard to prevent regressions.
Changes:
- Add/standardize
escapeHtmlusage on dashboard pages so collection titles, baselines (vs), and other label-like strings render verbatim (including generics). - Extend
scripts/check_dashboard_coverage.jswith a new structural rule to fail CI if markup-shaped labels are concatenated into markup without escaping. - Document the “write labels as plain text; escape at render-time” rule in CONTRIBUTING, and record the fix in CHANGELOG/ROADMAP.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/dev/bench/index.html | Introduces shared escapeHtml and applies it to card/hasher labels rendered via innerHTML. |
| web/dev/bench/detail.html | Escapes collection.title/collection.vs in the header and no-data notice to prevent generic truncation. |
| scripts/check_dashboard_coverage.js | Adds a structural CI check to detect unescaped label concatenation patterns. |
| CONTRIBUTING.md | Documents that dashboard labels must be plain text and are escaped at render time; references the CI guard. |
| CHANGELOG.md | Adds two [Unreleased] “Fixed” entries describing the rendering fix and the new CI guard. |
| ROADMAP.md | Marks the label-truncation silent-drop mode as closed and links it to #328. |
The guard keyed off "is this a label concatenation", with an exclusion list for the sinks known to be safe. That is the wrong way round: a label spliced into a document.title, a log line or a query string is fine and would have been reported. Require a markup context instead — the statement has to feed innerHTML / outerHTML / insertAdjacentHTML / document.write, or splice the label into a string literal that opens a tag. The safe-sink exclusion list goes away, since setAttribute and textContent no longer match on their own. Still reports all four pre-fix sites; a document.title assignment and a query-string build over the same labels now pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
scripts/check_dashboard_coverage.js:124
MARKUP_CONTEXTis meant to detect “string literal that opens a tag”, but the current['"]</branch only matches closing tags (e.g.'</div>') and will miss common markup fragments that start with an opening tag (e.g.'<div>','<em>'). That means check (3) can fail to flag unescaped label concatenation when markup is built in an intermediate variable and only later assigned toinnerHTML/etc.
const MARKUP_CONTEXT = /\.(?:inner|outer)HTML\b|insertAdjacentHTML\(|document\.write\(|['"]</;
|
Addressing the round-2 finding suppressed as low confidence ( The trailing const MARKUP_CONTEXT = /\.(?:inner|outer)HTML\b|insertAdjacentHTML\(|document\.write\(|['"]</;is the regex delimiter, not part of the pattern. Read back from the compiled object: The alternative is And end-to-end through the check itself, using exactly the scenario the finding describes — markup accumulated in an intermediate variable with only an opening tag, assigned to var frag = '<em>' + collection.title;
frag += 'closing markup added far away';
document.body.innerHTML = frag;Caught on the first statement, before the |
Closes #328.
The bug
col.titleandcol.vsare concatenated straight intoinnerHTML, so a markup-shaped label has its generic parameters parsed as a start tag and dropped. Rendered the pre-fix pages against a syntheticdata.jsand read the DOM back:Three different baselines (
Dictionary<int, int>,Dictionary<long, int>,Dictionary<int, List<int>>) all readvs Dictionary, andEnumSet<TEnum>materialized a stray<tenum>element — exactly as the issue described. Same DOM read after the fix:and on
detail.html?c=EnumSet&op=Union:h1=EnumSet<TEnum> · Union,vs-label=HashSet<TEnum>, no-data notice =No measurements recorded for EnumSet<TEnum>.Union.This is not a security issue — the labels are trusted in-repo literals, unlike the
opquery parameter fixed in #327. It is trusted-but-markup-shaped text reaching an HTML sink.What changed
web/dev/bench/index.html— one sharedescapeHtmlfor both scripts on the page, replacing the narrowerescthat only the hasher IIFE had (it missed"/', and the collections IIFE had no helper at all). Applied tocol.title,col.vs, the per-celloplabel, and the hasher group head (meta.title/meta.sub/meta.vs,r.method). The hasher head is not in the issue's list; it is the same sink taking the same kind of label, andmethodLabelalready returnsEqualityComparer<T>.Default, so leaving it raw would have re-created the bug the first time avsthere carried a generic.web/dev/bench/detail.html—escapeHtmlapplied tocollection.title/collection.vsinrenderShelland tocollection.titlein the no-data notice. The helper was already there and already used forparams.op.Deliberately left alone:
cell.setAttribute('aria-label', col.title + …)is a text sink and was already correct (which is why the accessible name has been more accurate than the visible heading all along), andstat.collection/stat.opcome out ofparseName's\w+captures and cannot contain markup.scripts/check_dashboard_coverage.js— new structural check (3): no markup-shaped label may be concatenated into a markup string outside a text sink. The file is folded into logical statements first (joining lines that end on a continuation token) so that a multi-linesetAttribute(call is recognised as the safe sink it is. Runs on every PR viaci.yml, alongside the existing structural checks; the report-only checks renumber to (4)/(5).CONTRIBUTING.md— "The dashboard" now says to writetitle/vsas plain text (they are escaped at render time, so a pre-escaped label renders its entities literally) and notes the guard.CHANGELOG.md— two### Fixedbullets under[Unreleased]: the render fix and the CI guard.ROADMAP.md— the 2.4.0 "No guard on the benchmark dashboard" bullet records this second silent-drop mode in the same page and marks itdone.Parity checklist
No C# changed, so most of the rollout does not apply, and here is why for each: no new collection or public API → no dedicated test files, no cross-collection shared-test rows, no benchmark class or
Program.csregistration, nodocs/api/*or README entry. Dashboard wiring is the subject of the fix rather than a follow-on: no collection was added, so neitherCOLLECTIONSarray nor theweb/index.htmlship cards gain a row (the ship cards were already written with</>and render correctly). The applicable facets — regression coverage, contributor docs, CHANGELOG, ROADMAP — are all in this PR.The regression test is
scripts/check_dashboard_coverage.jsrather than an xUnit case, because the defect is in the site, not the library. It fails onmain, reporting all four pre-fix sites:and passes on this branch.
Test plan
node scripts/check_dashboard_coverage.js— passes on this branch (131 cards / 41 collections)data.js; DOM read back for titles,vslabels, and stray elements (before/after above)dotnet build— 0 errorsdotnet test -f net9.0— 5088 passed, 0 failed (no C# changed; run to confirm no collateral)dashboard-coveragestep inci.ymlruns the new check on this PRmain,benchmarks.ymlrepublishesdata.jstogh-pagesand the live dashboard picks up the corrected titles — the HTML is served from the repo, so no data regeneration is needed for the fix itself