feat(html-report): add category/tag filter pills to toolbar#5496
feat(html-report): add category/tag filter pills to toolbar#5496
Conversation
There was a problem hiding this comment.
Code Review: Category/Tag Filter Pills
Overall this is a solid, well-thought-out feature. The OR-based multi-select model, URL hash persistence, collapsible overflow, and zero-overhead when no categories exist are all good design choices. The usage in the HTML detail panel and the API in both safely handle special characters. A few issues worth addressing:
1. Missing aria-pressed on category pill buttons (Accessibility — Should Fix)
In buildCatPills(), the buttons are toggle controls but never receive aria-pressed:
btn.className = 'pill cat-pill' + (activeCategories.has(c) ? ' active' : '');
btn.setAttribute('data-category', c);
// Missing: btn.setAttribute('aria-pressed', activeCategories.has(c) ? 'true' : 'false');The status filter buttons in filterBtns correctly set aria-pressed (lines ~1804, 1912–1914), so the pattern is established. Screen readers can't communicate the toggled state without this attribute.
2. Double render on page load when hash contains categories (Design Concern)
buildCatPills() is called at initialisation time (unconditionally when categories exist), then called again inside loadFromHash() when activeCategories.size > 0. The first render always has no active state and is immediately thrown away on a hash-restore. Consider restructuring so buildCatPills() is always called after loadFromHash() runs:
// Current (two renders when hash has categories):
if(catNames.length > 0) buildCatPills(); // first render, no active state
// ... later ...
loadFromHash(); // populates activeCategories, then calls buildCatPills() again
// Better:
loadFromHash(); // populate activeCategories
if(catNames.length > 0) buildCatPills(); // single render with correct stateThis also simplifies loadFromHash() — the trailing if(activeCategories.size > 0) buildCatPills(); can be removed.
3. Comma in a category name breaks URL round-trip (Correctness)
Categories are joined with a bare comma:
p.push('category=' + encodeURIComponent(Array.from(activeCategories).join(',')));encodeURIComponent encodes the entire string including the commas, but the value is then decoded before splitting:
const v = decodeURIComponent(pair.substring(eq + 1));
// ...
v.split(',') // splits on commas in category *names* tooA category name like "Integration, Smoke" (legal in C# [Category] attributes) will be corrupted on reload. Either split before decoding, or use a safer separator like |:
// Encoding:
Array.from(activeCategories).map(encodeURIComponent).join(',')
// Decoding:
v.split(',').map(decodeURIComponent)4. Hash-restored unknown categories silently show zero results (UX)
If the URL contains #category=Nonexistent, the name is added to activeCategories but never appears in catNames, so no pill renders and the filter shows 0 results with no explanation. Consider filtering restored values against known categories:
else if (k === 'category') {
v.split(',').forEach(function(c){ if(c && catCounts[c]) activeCategories.add(c); });
}5. Category tag in detail panel only adds, never toggles (UX Asymmetry)
Clicking .cat-link inside an expanded test always adds the category to the filter, even if it was already active. This is a one-way affordance. A small note in the title attribute or a toggle would be more consistent with the pill behaviour — though this is minor since the pill is the primary toggle surface.
Summary
| # | Severity | Description |
|---|---|---|
| 1 | Should Fix | Missing aria-pressed on toggle pills |
| 2 | Minor | Double render on page load |
| 3 | Should Fix | Comma in category name breaks URL round-trip |
| 4 | Minor | Unknown hash categories silently show 0 results |
| 5 | Nit | .cat-link is add-only, not a toggle |
The feature implementation is clean and the zero-cost-when-empty design is particularly good. Issues 1 and 3 are the ones most worth addressing before merge.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Code Review: Category/Tag Filter Pills
Overall this is a solid, well-thought-out feature. The OR-based multi-select model, URL hash persistence, collapsible overflow, and zero-overhead when no categories exist are all good design choices. The esc() usage in the HTML detail panel and the setAttribute API in buildCatPills both safely handle special characters. A few issues worth addressing:
1. Missing aria-pressed on category pill buttons (Accessibility - Should Fix)
In buildCatPills(), the buttons are toggle controls but never receive aria-pressed. The status filter buttons in filterBtns correctly set aria-pressed (lines ~1804, 1912-1914), so the pattern is established. Screen readers cannot communicate the toggled state without this attribute.
btn.className = 'pill cat-pill' + (activeCategories.has(c) ? ' active' : '');
btn.setAttribute('data-category', c);
// Missing: btn.setAttribute('aria-pressed', activeCategories.has(c) ? 'true' : 'false');2. Double render on page load when hash contains categories (Design Concern)
buildCatPills() is called at initialisation time (before loadFromHash), then called again inside loadFromHash() when activeCategories.size > 0. The first render always has no active state and is immediately thrown away. Consider restructuring so buildCatPills() is always called after loadFromHash() runs:
// Better: single render with correct state
loadFromHash(); // populate activeCategories first
if(catNames.length > 0) buildCatPills(); // then render with correct active stateThis also lets you remove the trailing if(activeCategories.size > 0) buildCatPills(); from loadFromHash().
3. Comma in a category name breaks URL round-trip (Correctness - Should Fix)
Categories are joined with a bare comma, then the whole string is URL-encoded:
p.push('category=' + encodeURIComponent(Array.from(activeCategories).join(',')));But the value is decoded before splitting:
const v = decodeURIComponent(pair.substring(eq + 1));
v.split(',') // splits on literal commas in category names tooA category name like "Integration, Smoke" (legal in C# [Category] attributes) will be corrupted on reload. Fix by encoding each value individually:
// Encoding:
p.push('category=' + Array.from(activeCategories).map(encodeURIComponent).join(','));
// Decoding (split the raw encoded string, then decode each piece):
pair.substring(eq + 1).split(',').map(decodeURIComponent)4. Hash-restored unknown categories silently show zero results (UX)
If the URL contains #category=Nonexistent, the name is added to activeCategories but never appears in catNames, so no pill renders and the filter shows 0 results with no feedback. Consider filtering restored values against known categories:
else if (k === 'category') {
v.split(',').forEach(function(c){ if(c && catCounts[c]) activeCategories.add(c); });
}5. Category tag in detail panel only adds, never toggles (Nit)
Clicking .cat-link inside an expanded test always adds the category to the filter, even if already active. This asymmetry with pill toggling is minor since the pill is the primary toggle surface, but updating title to reflect state would help.
Summary
| # | Severity | Description |
|---|---|---|
| 1 | Should Fix | Missing aria-pressed on toggle pills |
| 2 | Minor | Double render on page load |
| 3 | Should Fix | Comma in category name breaks URL round-trip |
| 4 | Minor | Unknown hash categories silently show 0 results |
| 5 | Nit | .cat-link is add-only, not a toggle |
The feature implementation is clean and the zero-cost-when-empty design is particularly good. Issues 1 and 3 are the ones most worth addressing before merge.
- Add aria-pressed to category pill toggle buttons for screen readers - Eliminate double render on page load by deferring buildCatPills() until after loadFromHash() populates activeCategories - Fix URL round-trip for category names containing commas by encoding each value individually rather than encoding the joined string - Filter hash-restored categories against known categories to prevent silent zero-result state from stale/invalid URLs - Make category tags in test detail panels toggle (not add-only) to match the pill button behavior
There was a problem hiding this comment.
Follow-up Review: All Previous Issues Addressed
The two follow-up commits (05a4b50 and c43afbfc) have resolved all 5 issues from the prior review:
| # | Issue | Status |
|---|---|---|
| 1 | Missing aria-pressed on toggle pills |
Fixed — btn.setAttribute('aria-pressed', isActive ? 'true' : 'false') added |
| 2 | Double render on page load | Fixed — loadFromHash() now runs before buildCatPills(), single render |
| 3 | Comma in category name breaks URL round-trip | Fixed — per-value map(encodeURIComponent) encoding and per-value decode |
| 4 | Unknown hash categories show silent zero results | Fixed — if(c && catCounts[c]) guards stale/invalid hash values |
| 5 | .cat-link was add-only |
Fixed — now delegates to toggleCategory() which properly toggles |
Final review notes
The implementation is clean and correct:
- XSS is properly handled:
textContentinbuildCatPills()andesc()in the detail panel HTML string - Dark/light theme variants use appropriately contrasted violet values (
#a78bfadark /#7c3aedlight) - Event delegation on
catRowis efficient — a single listener handles all pill clicks - The zero-overhead guard (
catRowstays hidden until categories exist) is a nice touch
No new concerns. The feature is ready to merge.
Summary
Closes #5486
ReportTestResult.Categories)#category=Integration,Smoke) alongside existing filter/sort/search/group params.pillbase classTest plan
[Category]attributes — verify category pills appear below the toolbarcategory=...param and survives page reload