Follow-ups to #255: expanded group dead end on mobile, nodesEsc listener leak, two test gaps

[dborup]

Follow-ups from the review of #255 (#254, merged as 3bb2cb89). Items 1 and 2 are pre-existing behaviour that #255 did not cause. Items 3 and 4 are test gaps in #255's own tests. Line references are to master 3bb2cb89.

1. An expanded packet group has no collapse path after crossing to mobile width

At 1400 px, expand a multi-observation group in the grouped packets view, then resize to ≤ 600 px. The row re-renders as select-hash without aria-expanded, as #255 intends. It keeps class="expanded", though, and its child rows stay visible. The expand column is hidden at that width, so nothing can collapse the group.

Before #255 the same row announced a stale aria-expanded="true", so this is not a regression. It is the dead end that upstream #1461 #7 describes.

Expected: a group expanded on desktop does not leave visible children on mobile without a way back. For example:

• clear the hash from expandedHashes when the layout mode flips to mobile; or
• keep the state but omit the child rows from the visible slice while in mobile mode, so they come back on desktop.

Test: an E2E that expands a group at 1400 px, resizes to 390 px and asserts that no child rows are visible, or that the group can be collapsed. Add a mutant without the fix.

2. nodes.js adds a document keydown listener (nodesEsc) on every node-page init

init() adds the listener on each visit, and it is only removed when Escape is pressed. Navigating between nodes A → B → C stacks three listeners, and Escape then sets the same hash three times. That is harmless today, but the listeners leak.

Expected: at most one listener. Remove it in the page's destroy(), or register it once. Add a test that counts the listeners, or the hash writes, after repeated navigation.

3. Test gap: the unit test does not prove that the node page uses the new renderer

test-issue-254-affinity-debug-toggle.js exercises renderAffinityDebugCard() in isolation. A mutant that puts the old inline onclick card back into the loadFullNode template, and leaves the renderer exported but unused, passes the unit test 9/9; only the E2E catches it. The "no inline event handler anywhere in the card" case is also green on master, because it scans an empty string there.

Expected: a fast-layer assertion that the loadFullNode template uses renderAffinityDebugCard(), or that nodes.js has no onclick= in the #node-affinity-debug card.

4. Test gap: Space on a mobile group row

The #254 E2E presses Enter on a 390 px group row but not Space. The handler treats both keys the same, and the review checked Space by hand: the detail sheet opens and the group does not expand. Add Space to the E2E.

Evidence

• From the fix(ui): Affinity Debug toggle and mobile group-row aria-expanded (#254) #255 review comment (findings F1–F4): items 1 and 3 were reproduced there with a test or mutant; item 4 was checked by hand there.
• Item 2 is based on reading the code.
• Nothing here has been re-run on 3bb2cb89.

[dborup]

Plan — CS-Minimax #259

Branch codex/issue-259-followups-255 off origin/master 3bb2cb89, in a dedicated worktree. Tests first (red on master), then the fix. No rebase, amend or force-push; explicit git add only. Draft PR, Relates to #259, no closing keywords.

1. Expanded group has no collapse path on mobile

Choice: keep expandedHashes, drop the children from the rendered slice while in mobile mode (the second option in the issue), rather than clearing the set on the flip.

Why:

• Rotating a phone crosses the 600 px breakpoint in both directions (390x844 portrait is mobile, 844x390 landscape is not). Clearing the set would silently throw away the user's expansion on every rotation; suppressing the rows restores it on the way back.
• The dead end also exists on a first render at 390 px, not only after a resize: the deep link #/packets/<hash>/<obs> adds the hash to expandedHashes before the first renderTableRows() (packets.js around the directPacketHash branch). Clearing on the mode flip alone would not catch that; a render-time rule catches both.
• It is render-time only, so nothing in the desktop path from fix(packets): collapsed groups show a right caret, expanded a down caret (#189) #248/fix(ui): Affinity Debug toggle and mobile group-row aria-expanded (#254) #255 changes: carets, aria-expanded and expandedHashes keep behaving the same across live updates, sorting and filtering.

Implementation sketch (public/packets.js):

• one helper, groupIsExpandedInView(hash) = expandedHashes.has(hash) && !groupRowSelectsOnActivate();
• buildGroupRowHtml uses it for the expanded class, the caret and the child rows, so a mobile row renders exactly like a collapsed one (no expanded, caret-right, no tr.group-child);
• _getRowCount uses it too, so the virtual-scroll row counts and the rendered rows cannot diverge (debt: _lastRenderedRowCounts computed independently from buildGroupRowHtml — divergence risk (PR #402) Kpa-clawbot/CoreScope#424);
• the existing mode-flip resize handler (_onGroupRowModeResize) also calls _invalidateRowCounts(), because the per-entry row count changes when the mode flips.

Tests:

• test-packets.js: at 390 px an expanded group renders no expanded class, a caret-right and no child rows, and _getRowCount is 1; at 1400 px the same group still renders expanded, caret-down, its children and _getRowCount = 1 + children; the hash stays in expandedHashes across 1400 -> 390 -> 1400.
• new E2E test-issue-259-mobile-expanded-group-e2e.js: expand the seeded 3-observation group at 1400 px, resize to 390 px, assert no child rows are visible (and that the row claims no expanded state), then resize back to 1400 px and assert the children and aria-expanded="true" are back. Registered in deploy.yml next to the Follow-up to #248: Affinity Debug toggle broken + reversed carets; group-row aria-expanded on mobile #254 line.

2. nodesEsc listener leak

init() adds an anonymous-per-call nodesEsc on every node-page init and only removes it when Escape fires. Fix: lift it to one module-level named handler, register it in the direct-node branch of init(), and remove it in destroy() (which also runs on the in-module navigateToNode path and from the router). A stable reference also makes a repeat addEventListener a DOM no-op.

Test: new test-issue-259-nodes-esc-listener.js — a vm sandbox with a counting document.addEventListener/removeEventListener, run init/destroy for node A -> B -> C, assert the net number of document keydown listeners is at most 1, then fire Escape and assert exactly one location.hash write.

3. Test gap F1 — the fast layer does not prove the template uses the renderer

Add to test-issue-254-affinity-debug-toggle.js source-level assertions on public/nodes.js:

• the loadFullNode template interpolates ${renderAffinityDebugCard()};
• id="node-affinity-debug" occurs exactly once in the file (i.e. only inside the renderer);
• no onclick=/on*= attribute anywhere near a node-affinity-debug occurrence.

That kills Macmini's M1 ("old inline card back in the template, renderer exported but unused", which passes 9/9 today) in the fast layer, and stops the "no inline handler" case from being vacuously green on an empty string.

4. Test gap F3 — Space on a mobile group row

Add a Space step to the 390 px block of test-issue-254-affinity-toggle-mobile-aria-e2e.js: focus the group row, press Space, assert the detail sheet opens, the group does not expand and the row is still select-hash without aria-expanded.

Mutants (at least one per item)

1. buildGroupRowHtml/_getRowCount back to the plain expandedHashes.has() check (the master behaviour) — must fail test-packets.js and the new E2E.
2. destroy() no longer removes the listener and the handler is re-created per init — must fail the new nodes-esc test.
3. Master's inline onclick card put back into the loadFullNode template with the renderer left exported but unused — must now fail test-issue-254-affinity-debug-toggle.js, not only the E2E.
4. Space handled differently from Enter in packets.js (drop ' ' from the keydown branch) — must fail the new Space step.

Each mutant is reverted with git checkout -- and the tree verified clean afterwards.

Suites and gates

sh test-all.sh, node test-frontend-helpers.js, node test-packets.js, node test-issue-254-affinity-debug-toggle.js, plus the #254, #189 and Kpa-clawbot#1461 E2Es and the new ones against a local Go server on a copy of e2e-fixture.db, prepared as in CI (freshen, the seed SQL, corescope-migrate, seeds 2073 and 199), on a free local port, stopped by port.

Gates: no hardcoded colours, no per-item API calls, scripts/check-xss-sinks.sh --diff origin/master clean, fork guards unchanged (9 in deploy.yml, 1 in release-fast-path.yml), the new E2E registered beside the #254 line.

Screenshots at 390 px and 1400 px, light and dark, described in the report comment on the PR.

[dborup]

Fixed by #260 (merged as edec4253). Remaining review nits are tracked in #282.