feat(reporters): add minimap sidebar navigator to HTML report#5494
feat(reporters): add minimap sidebar navigator to HTML report#5494
Conversation
Add a collapsible minimap sidebar for navigating large test suites (200+ test classes). Shows colored status bars proportional to test count per group, with scroll-spy highlighting the currently visible group. Closes #5488
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
TIP This summary will be updated as you push new changes. Give us feedback
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — feat: minimap sidebar navigator
This is a well-scoped, thoughtful feature. The accessibility baseline (role/aria on the sidebar itself, keyboard shortcut guard against input focus, localStorage persistence, print/forced-colors support) is solid. A few design concerns worth considering:
1. Minimap rows are not keyboard accessible (accessibility gap)
Individual minimap rows are bare <div> elements with a click handler. A user who opened the minimap with the m shortcut cannot then navigate or activate rows using a keyboard — there's no tabindex or role on them.
The outer container has role="navigation" which is good, but the interactive rows need:
h += '<div class=minimap-row role=button tabindex=0 data-dgi=+item.dgIdx+ title=+tip+>';And in the event listener, handle keydown (Enter/Space) in addition to click:
mmContent.addEventListener('keydown', function(e) {
if (e.key !== 'Enter' && e.key !== ' ') return;
var row = e.target.closest('.minimap-row');
if (row) { e.preventDefault(); row.click(); }
});Without this, keyboard-only users open a panel they cannot effectively use.
2. Rebuilding the entire minimap on every render() invocation
updateMinimap is called at the end of every render() call. Each invocation:
- Re-filters all groups
- Rebuilds the entire
mmContent.innerHTML - Disconnects and reconnects the
IntersectionObserver
For 200+ test classes, this is O(n) DOM churn on every keystroke in the search box. The scroll-spy teardown/rebuild is especially expensive since IntersectionObserver forces a layout pass for each new target.
A better architecture separates the two concerns:
// Called once per logical state change (filter/group mode change)
function rebuildMinimap(allDisplayGroups) { /* sets innerHTML, reconnects observer */ }
// Called only when the filter/search changes visible groups
function syncMinimapVisibility() { /* show/hide rows based on current matchesFilter */ }This way, scroll-spy isn't recreated on every search keystroke.
3. Multiple simultaneous active highlights
The IntersectionObserver uses threshold: 0 + rootMargin: '0px 0px -75% 0px', so any group intersecting the top 25% of the viewport is marked active. When the page loads or after a scroll, several groups can be simultaneously highlighted in the minimap — reducing the "you are here" clarity.
Consider only activating the topmost intersecting group:
spyObs = new IntersectionObserver(function(entries) {
entries.forEach(function(en) {
var gi = en.target.dataset.gi;
var mmRow = mmContent.querySelector('.minimap-row[data-dgi=+gi+]');
if (mmRow) mmRow.classList.toggle('active', en.isIntersecting);
});
// After processing batch, keep only topmost active row
var allActive = Array.from(mmContent.querySelectorAll('.minimap-row.active'));
if (allActive.length > 1) {
allActive.slice(1).forEach(function(r) { r.classList.remove('active'); });
}
}, {rootMargin: '0px 0px -75% 0px', threshold: 0});4. parseInt without radix
// Line ~2240
var dgi = parseInt(row.dataset.dgi);Should be:
var dgi = parseInt(row.dataset.dgi, 10);Without the radix, inputs starting with 0x are parsed as hex. Low real-world risk here (group indices are generated integers), but it's a lint/style best practice.
5. var usage in a 'use strict' module
The file sets 'use strict' at line 1260. The new updateMinimap function and its IIFE use var throughout, while all surrounding features use const/let. This isn't a bug, but it's inconsistent. Using const/let also avoids variable hoisting surprises.
Summary
| Issue | Severity |
|---|---|
| Minimap rows not keyboard-navigable | Medium (a11y) |
| Full rebuild + observer reconnect on every render | Medium (perf) |
| Multiple simultaneous active highlights | Low (UX) |
parseInt without radix |
Low |
var vs const/let inconsistency |
Low |
The core logic (index alignment between data-gi / data-dgi, lazy-load expansion on click, scroll-spy, localStorage persistence, responsive backdrop) is correct. The feature delivers real value for large test suites.
- Add role="button" tabindex="0" to minimap rows for keyboard accessibility - Add Enter/Space keydown handler for keyboard navigation of rows - Fix parseInt without radix (add radix 10) - Replace var with const/let for consistency with rest of file - Hoist openMinimap/closeMinimap/toggleMinimap to outer scope so keyboard shortcut uses cached ref instead of redundant getElementById
Review responseAddressed items 1, 4, 5 in 7b1b9c5. Pushing back on 2 and 3: 1. Minimap rows not keyboard accessible — FixedAdded 2. Rebuilding minimap on every render() — No change neededThe search input is already debounced, so 3. Multiple simultaneous active highlights — IntentionalMultiple active rows show the "viewport window" — which section of the report is currently visible. This is the same behavior as VS Code's minimap highlighting a range, not a single line. Collapsing to a single row would lose that spatial context. 4. parseInt without radix — FixedAdded radix 10. 5. var → const/let — FixedAll minimap code now uses |
There was a problem hiding this comment.
Code Review: Minimap Sidebar Navigator
This is a well-scoped feature with good attention to UX details — keyboard shortcuts, mobile backdrop, print suppression, forced-colors support, and localStorage persistence all handled. The countStatuses() extraction is a clean refactor that removes real duplication. A few issues worth addressing before merge:
Issues
1. Development artifact comments should be removed
The /* ── Feature 11: Minimap Sidebar Navigator ─────────── */ comments in both CSS and JS reference an internal feature numbering scheme that has no meaning to contributors. These should be replaced with descriptive comments or removed entirely.
2. Multiple active minimap rows (IntersectionObserver bug)
The scroll-spy uses mmRow.classList.toggle('active', en.isIntersecting) on each entry individually. With rootMargin: '0px 0px -75% 0px', multiple groups can be simultaneously intersecting (e.g., when a tall group spans the top 25% of the viewport, and the next group's header is also visible). This causes multiple bars to light up at once, which defeats the purpose of a single-position indicator.
A better approach tracks all currently visible groups and highlights only the topmost one:
const visibleGis = new Set();
spyObs = new IntersectionObserver(function(entries) {
entries.forEach(function(en) {
if (en.isIntersecting) visibleGis.add(en.target.dataset.gi);
else visibleGis.delete(en.target.dataset.gi);
});
const topGi = [...visibleGis].sort((a, b) => +a - +b)[0];
mmContent.querySelectorAll('.minimap-row').forEach(function(row) {
const isActive = row.dataset.dgi === topGi;
row.classList.toggle('active', isActive);
if (isActive && minimapOpen) row.scrollIntoView({block: 'nearest', behavior: 'auto'});
});
}, {rootMargin: '0px 0px -75% 0px', threshold: 0});This ensures exactly one bar is highlighted at a time, reflecting the user's actual position in the list.
3. Missing aria-expanded on toggle button
The minimapToggle button opens/closes the minimap panel but never sets aria-expanded. Screen readers won't know the current state of the panel. Add it to openMinimap() and closeMinimap():
function openMinimap() {
minimapOpen = true;
if (mm) mm.classList.add('open');
if (mmToggle) { mmToggle.classList.add('active'); mmToggle.setAttribute('aria-expanded', 'true'); }
if (mmBackdrop) mmBackdrop.classList.add('open');
localStorage.setItem('tunit-minimap', 'open');
}
function closeMinimap() {
minimapOpen = false;
if (mm) mm.classList.remove('open');
if (mmToggle) { mmToggle.classList.remove('active'); mmToggle.setAttribute('aria-expanded', 'false'); }
if (mmBackdrop) mmBackdrop.classList.remove('open');
localStorage.setItem('tunit-minimap', 'closed');
}4. var in countStatuses() is inconsistent with the rest of the file
var c = {p:0,f:0,s:0} should be const c = {p:0,f:0,s:0}. The new function also uses function(t) callbacks while the rest of the file uses arrow functions. This isn't critical but creates style inconsistency in an otherwise uniform file.
Minor
- The
data-gi(group index on DOM elements) vsdata-dgi(display group index on minimap rows) naming is subtle. The mapping works correctly because both iteratedisplayGroupsfrom 0, but a brief comment explaining the invariant would help future maintainers. - Minimap shows only bars, no text labels — labels are tooltip-only. This is fine for a density visualization, but it means users must hover to identify a group. Worth considering abbreviated labels below bars for very wide viewports.
Summary
The core feature is solid and well-tested per the test plan. The IntersectionObserver multi-active bug is the most impactful issue and worth fixing before merge. The aria-expanded gap is a real accessibility miss since the button explicitly controls a panel. The rest are polish items.
- Fix var→const in countStatuses (missed in previous sweep) - Extract ensureGroupOpen helper to deduplicate group-expand logic between navigateToRow and scrollToTest; also fixes latent bug where scrollToTest was not setting aria-expanded when opening a group - Replace per-entry querySelector in IntersectionObserver callback with pre-built Map for O(1) minimap row lookups during scroll-spy
There was a problem hiding this comment.
Code Review — Round 3 (Follow-up)
Good iteration. The fixes from the last round are in and the refactors in the third commit (95493cf6) are clean improvements — particularly ensureGroupOpen eliminating the latent bug where scrollToTest wasn't setting aria-expanded.
Two items from Round 2 are still unaddressed:
1. aria-expanded missing from toggle button — still open
openMinimap() / closeMinimap() update the visual active class but never set aria-expanded on mmToggle. The button controls a panel; without this attribute screen readers report static state.
function openMinimap() {
minimapOpen = true;
if (mm) mm.classList.add('open');
if (mmToggle) { mmToggle.classList.add('active'); mmToggle.setAttribute('aria-expanded', 'true'); }
if (mmBackdrop) mmBackdrop.classList.add('open');
localStorage.setItem('tunit-minimap', 'open');
}
function closeMinimap() {
minimapOpen = false;
if (mm) mm.classList.remove('open');
if (mmToggle) { mmToggle.classList.remove('active'); mmToggle.setAttribute('aria-expanded', 'false'); }
if (mmBackdrop) mmBackdrop.classList.remove('open');
localStorage.setItem('tunit-minimap', 'closed');
}This is a one-line fix per function and the only remaining a11y gap.
2. "Feature 11" artifact comments — still present
Three locations still carry the internal feature numbering scheme that has no meaning outside this PR:
- C# comment (line 89):
// Feature 11: Minimap sidebar navigator - CSS comment (line 1184):
/* ── Feature 11: Minimap Sidebar Navigator ─────────── */ - JS comment (line 2073):
// ── Feature 11: Minimap Sidebar Navigator ──────────
These can either be removed entirely (the function/selector names are self-documenting) or replaced with a plain description like // Minimap sidebar navigator. Leaving numbered feature annotations in the shipped source adds noise without benefit.
Accepted pushback
Happy with the reasoning on:
- Multi-active scroll-spy: "viewport window" rather than single-point cursor is a valid UX model (analogous to VS Code's minimap range highlight). No change needed.
- Full minimap rebuild on render: The debounced search path means this doesn't fire per-keystroke, and the IO reconnect is required because
container.innerHTMLreplaces all observed nodes. Accepted.
Summary
The feature is complete and well-engineered. Two remaining items: the aria-expanded attribute (small fix, real a11y value) and the artifact comments (cleanup). Once those are cleared this is ready to merge.
- Set aria-expanded on mmToggle in openMinimap/closeMinimap so screen readers report panel state - Remove internal "Feature 11:" numbering from comments
There was a problem hiding this comment.
Code Review — Round 4 (Final)
All outstanding items from Round 3 are now resolved. This is ready to merge.
Resolved since last review
aria-expanded on toggle button — Fixed ✓
openMinimap() and closeMinimap() now both set aria-expanded on mmToggle:
if (mmToggle) { mmToggle.classList.add('active'); mmToggle.setAttribute('aria-expanded','true'); }Screen readers will now correctly announce the open/closed state.
"Feature 11" artifact comments — Fixed ✓
All three locations updated:
- C# (line 90):
// Minimap sidebar navigator - CSS:
/* ── Minimap Sidebar Navigator ─────────── */ - JS:
// ── Minimap Sidebar Navigator ──────────
Previously accepted (no action needed)
- Multi-active scroll-spy: Viewport-range highlight is a valid UX model. Accepted in Round 3.
- Full minimap rebuild on
render(): Debounced search mitigates the concern; IO reconnect is necessary becausecontainer.innerHTMLreplaces observed nodes. Accepted in Round 3.
Remaining minor (non-blocking)
countStatusesusesfunction(t)callbacks instead of arrow functions, inconsistent with the surrounding file style. Too minor to block.
Overall
The feature is complete, correct, and well-engineered. Keyboard navigation (role=button, tabindex=0, Enter/Space handler), accessibility (aria-expanded, aria-label), responsive behavior (mobile backdrop), print suppression, and localStorage persistence are all handled properly. The countStatuses() extraction is a clean improvement that removes real duplication. LGTM.
1 participant
Summary
mto toggle; responsive with backdrop overlay on mobileDetails
updateMinimap()builds bar-based minimap from all display groups (not just lazy-loaded subset),countStatuses()extracted as shared helper, IntersectionObserver for scroll-spy withrootMargin: '0px 0px -75% 0px'renderLimitand re-renders firstCloses #5488
Test plan
m— sidebar slides in from right with colored bars?— help overlay shows minimap shortcut