Repository navigation
fix(ui): follow-ups to #255 — mobile group dead end, nodesEsc leak, test gaps (#259) - #260
Conversation
Tests first, red on master 3bb2cb8 where they describe a behaviour change. 1. test-packets.js: an expanded group across the 600 px breakpoint. At 390 px it must render no child rows, no `expanded` class and no down caret, while the hash stays in expandedHashes so the children return at 1400 px. _getRowCount must agree with the rendered rows on both sides (Kpa-clawbot#424). 4 of the 7 new cases fail on master. 2. test-issue-259-nodes-esc-listener.js: the node page's document keydown handler (`nodesEsc`). After the router's destroy/init cycle for node A -> B -> C at most one listener may survive, Escape may write the hash once, and destroy() must leave none behind. 4 of 7 fail on master, where three listeners stack and Escape navigates three times. 3. test-issue-254-affinity-debug-toggle.js: three source assertions so the fast layer proves the loadFullNode template goes through renderAffinityDebugCard() and holds no inline on*= handler. Green on master by design — these close review finding F1, where the mutant that restores the old inline card passed the unit test 9/9. 4. test-issue-254-affinity-toggle-mobile-aria-e2e.js: press Space, not only Enter, on the 390 px group row (review finding F3). New E2E test-issue-259-mobile-expanded-group-e2e.js registered in deploy.yml next to the #254 line; the new unit file registered in test-all.sh. Relates to #259 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…259) A group expanded at desktop width kept `class="expanded"` and its child rows after the layout crossed to <= 600 px. There the expand column is hidden and the row only selects (Kpa-clawbot#1461 #7, #255), so nothing was left to collapse it: the dead end upstream Kpa-clawbot#1461 #7 describes, and finding F2 of the #255 review. Keep the hash in `expandedHashes` and leave the children out of the rendered slice while the mobile mode is active, rather than clearing the set on the flip. Two reasons: - the 600 px line is crossed in both directions by a phone rotating (390x844 is mobile, 844x390 is not), so clearing would discard the user's expansion on every rotation; suppressing the rows restores it on the way back; - the dead end also appears on a *first* render at a narrow width — the Kpa-clawbot#866 deep link #/packets/<hash>/<obs> adds the hash to `expandedHashes` before any row is built — which a mode-flip hook alone would not catch. One render-time helper, `groupIsExpandedInView()`, is used by both `buildGroupRowHtml` and `_getRowCount`, so the virtual-scroll row counts and the rendered rows cannot diverge (Kpa-clawbot#424). The mode-flip resize handler now also invalidates the cached counts, because an expanded group's row count changes with the mode. Desktop rendering is untouched: carets, `aria-expanded` and `expandedHashes` behave exactly as in #248/#255. `_setDisplayGrouped` joins the existing test hooks so the unit sandbox can exercise `_getRowCount` in grouped mode. Relates to #259 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`init()` added a fresh `nodesEsc` closure to `document` on every full-screen node-page init and only unhooked it when Escape actually fired. Navigating node A -> B -> C therefore left three live listeners, and one Escape wrote the same hash three times. Finding F4 of the #255 review. The handler is now a single module-level `_nodesEsc`, so a repeat `addEventListener` is a DOM no-op, and `destroy()` removes it — which covers the router's destroy/init cycle and the in-module `navigateToNode` path. Relates to #259 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The counting document.addEventListener in the new test pushed every registration, so it reported three listeners even for a stable handler reference that the real DOM would have registered once. It now deduplicates on (type, handler), which makes the count the listener count rather than the number of add() calls. Both mutants are still killed: master's per-init closure stacks three listeners, and keeping the stable reference while dropping the destroy() removal fails the "destroy() removes the Escape listener" case. Relates to #259 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rapport — CS-Minimax PR#260 #259 — head 0ed1a3dStatus: All four items of #259 are implemented on a draft PR; every item has a test and a mutant, the local suites are green, and the PR stays a draft. Evidence tags: [T] test or run output, [A] assessment or inference, [K] checked in code, diff, git or CI. Branch
The plan is on #259. No closing keywords in the title, body or commit messages; Requirements
Why option 2 for item 1The issue offered clearing
One helper, Red on master
|
| Test | On master | On the branch |
|---|---|---|
test-packets.js (#259 block) |
142 passed, 4 failed | 146 passed, 0 failed |
test-issue-259-nodes-esc-listener.js |
3 passed, 4 failed (three stacked listeners, Escape navigated three times) | 7 passed, 0 failed |
test-issue-259-mobile-expanded-group-e2e.js |
see M1 below — 2 failed, the dead end reproduced | 9 passed, 0 failed |
| the 3 new F1 source assertions | green on master by design — they close a mutant gap, not a current bug; M3 is their proof | green |
| the new Space step (F3) | green on master — the handler already accepts Space; M4 is its proof | green |
Mutants [T]
Each ran against the relevant layers, with a server restart per mutant that needed one, then git checkout -- public/ and a clean tree.
| Mutant | What | test-packets.js |
nodes-esc unit | #254 unit | #259 E2E | #254 E2E | Verdict |
|---|---|---|---|---|---|---|---|
| M1 | item 1 reverted: buildGroupRowHtml and _getRowCount back to the plain expandedHashes.has() check (master) |
4 failed | — | — | 2 failed | — | killed |
| M1b | the _invalidateRowCounts() call dropped from the mode-flip handler |
green | — | — | green | — | survives, see below |
| M2 | item 2 reverted: per-init closure, no removal in destroy() (master) |
— | 4 failed | — | — | — | killed |
| M2b | stable handler reference kept, but destroy() no longer removes it |
— | 1 failed | — | — | — | killed |
| M3 | master's inline onclick card back in the loadFullNode template, renderer exported but unused (the review's M1) |
— | — | 3 failed | — | — | killed, now by the fast layer |
| M4 | ' ' dropped from the row keydown branch in packets.js |
— | — | — | — | 1 failed (only the new Space step; Enter still passed) | killed |
M1's E2E failure is the dead end itself, verbatim from the run:
{"action":"select-hash","aria":null,"expandedClass":true,"children":3,"visibleChildren":3,"expandCellVisible":false,"carets":["ph-caret-down"]} — three child rows on screen, the row still marked expanded, and the expand column hidden. [T]
M1b survives and is reported as such. Dropping _invalidateRowCounts() leaves the cached per-entry row counts stale after a mode flip, which affects only the virtual-scroll spacer heights and the incremental row-removal arithmetic for a long, scrolled list. Every layer here reaches the seeded group through ?hash=…, so the table holds a single entry and never scrolls, and the rendered rows are correct either way. The call is kept because the stale count is a real inconsistency, but no test observes it. [T][A]
Suites (local Go server on a copy of e2e-fixture.db, prepared as in CI: freshen, the deploy.yml seed SQL, corescope-migrate, seeds 2073 and 199; a free local port, stopped by port) [T]
| Suite | Result |
|---|---|
sh test-all.sh |
220/220 files |
node test-frontend-helpers.js |
707 passed, 0 failed |
node test-packets.js |
146 passed, 0 failed |
node test-issue-254-affinity-debug-toggle.js |
12/12 |
node test-issue-259-nodes-esc-listener.js |
7/7 |
node test-issue-259-mobile-expanded-group-e2e.js (new) |
9/9 |
node test-issue-254-affinity-toggle-mobile-aria-e2e.js |
13/13 (12 before, plus Space) |
node test-issue-189-group-caret-e2e.js |
3/3 |
node test-issue-1461-mobile-page-actions.js |
6/6 |
node test-test-all.js |
10/10 |
Screenshots (local Chromium; described, not attached — the CLI cannot upload images) [T]
- 1400 px, light, expanded: the seeded 3-observation group sits at the top of the Latest Packets table with a down caret in the expand column, an eye badge "3" in the RPT column and its three child rows below it (DUBLIN Obs, GY889 Repeater, Kennedy Repeater, hop chips cc000000 / bb000000 / aa000000). The detail panel on the right shows "Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 fixture · observation 1 of 3".
- 1400 px, dark, expanded: identical layout on the dark card tokens; the same single down caret and the same three children. No colour value was added, so both themes render from the existing variables.
- 390 px, light, after resizing from 1400 px: one row left — Time / Type / Details, no expand column, no child rows, and the mobile bottom nav below. The group row carries
class="group-header "with noexpanded,data-action="select-hash"and noaria-expanded. - 390 px, dark, after the same resize: the same single row on the dark background.
- 390 px touch, detail sheet: a tap opens the bottom detail sheet ("Observations (3)") and the row behind it stays collapsed with no children.
CI [K][T]
Run on head 0ed1a3df, attempt 1, conclusion success. No reruns, and neither known flake fired (#250 TestStatsFileHasNoCredentials, #256 Hash Stats sort — the Hash Stats URL-state cases all passed).
| Job | Result | Evidence from the log |
|---|---|---|
| ✅ Go Build & Test | success | test-all.sh 220 passed, 0 failed (220 files), including test-issue-259-nodes-esc-listener.js; the PR-only XSS --diff origin/master preflight ran |
| ✅ Playwright E2E Tests | success | new test-issue-259-mobile-expanded-group-e2e.js 9/9; test-issue-254-affinity-toggle-mobile-aria-e2e.js 13/13, Space step included; test-issue-189-group-caret-e2e.js 3/3 |
| ✅ Build & Publish Docker Image | success | — |
| 📦 Release Artifacts | skipped | fork-guarded |
| 🚀 Deploy Staging | skipped | fork-guarded |
| 📝 Publish Badges & Summary | skipped | fork-guarded |
Remainder
- M1b is not killed (above): the cached row counts after a mode flip are not observed by any layer. [T][A]
- While a group is expanded, every data refresh still fetches
/packets/<hash>for each hash inexpandedHashes, including in mobile mode where the children are not rendered. That is unchanged pre-existing behaviour, bounded by the number of expanded groups, and was left alone. [K] - The Tufte v2: mobile packets view — kill chrome, kill duplication (follow-up to #1458 / #1459) Kpa-clawbot/CoreScope#1461 API: harden configured-scope contract for Local Mesh consumers #7 click redirect in
mobile-page-actions.jsis still in place and now only matters for rows rendered before a breakpoint crossing, inside the 150 ms debounce. Carried over from the fix(ui): Affinity Debug toggle and mobile group-row aria-expanded (#254) #255 remainder; deliberately not touched. [A] aria-expandedonrole="row"in a plain table has still not been checked with a real screen reader (carried over from fix(packets): collapsed groups show a right caret, expanded a down caret (#189) #248 and fix(ui): Affinity Debug toggle and mobile group-row aria-expanded (#254) #255). [A]- Not verified: Firefox, Safari/WebKit, a real phone, a real screen reader, staging or production (by design). No customizer values were added.
Review — CS-pve-agent1 PR#260 — head 0ed1a3dDom: REQUEST CHANGES Evidence tags: [T] test or run output, [A] assessment or inference, [K] checked in code, diff, git or CI. The production changes for items 1 and 2 are correct. I checked them in a browser on the merged tree and with my own mutants. One test gap blocks the merge: item 1's render rule hides a group's expansion at ≤ 600 px, so every 390 px "does not expand" assertion can no longer see an expansion. That includes the new Space step for item 4 and the existing #254 tap and Enter steps. A mutant that silently toggles the group on mobile survives every layer on this branch, but the #254 E2E kills it on master. The fix is small and only touches tests (see F1). Findings
The author's M1b (the The review points
Always-checks
Mutants (mine, against the merged tree; server restarted and fixture re-copied per mutant) [T]
Tests run (merged tree
|
| Suite | Result |
|---|---|
sh test-all.sh |
220/220 files on a serial run. A first run, in parallel with both Go suites, had 1 failure in test-channels-client-state-152.js (R4-3 S2, a timing case). It was 67/67 on 3 isolated reruns and on master; channels.js is untouched. |
node test-frontend-helpers.js |
707 passed, 0 failed |
cmd/server go test ./... |
ok (912 s, -timeout 20m). A first parallel run hit the default 10 min timeout under load. |
cmd/ingestor go test ./... |
ok (973 s, -timeout 20m). Same as above for the first run. |
node test-packets.js / nodes-esc / #254 unit |
146/146, 7/7, 12/12 |
E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the deploy.yml seed SQL, corescope-migrate, seeds 2073, 199 and 245), stopped by port |
#259 9/9, #254 13/13, #189 3/3, Kpa-clawbot#1461 6/6 |
| Own browser script (above) | 18/18 on the branch's packets.js |
CI on head 0ed1a3df: run 37323250705, attempt 1, success. Go Build & Test ✅, Playwright E2E ✅, Docker ✅; Release, Deploy Staging and Badges skipped (fork-guarded). Neither known flake fired: the Hash Stats sort (#256) and the backfill write-hold (#267) did not fail. [K]
Not verified
- Firefox, WebKit/Safari, a real phone, a real screen reader. [A]
- The
-racevariant of the Go server suite that CI runs; I ran it without-race. No Go code changed. [A] - CI on the merged tree against the current
origin/master(c6b356de). Only my local runs cover that. [A] - Staging and production, by design.
- Screenshots were taken locally and inspected, not attached.
Head was 0ed1a3df65575fc08b2aa58d9b69ca9623a72bf5 on git ls-remote before and after the review. Nothing was pushed or changed on the PR.
Review F1 on #260: since groupIsExpandedInView(), a hash that *is* in expandedHashes renders at <= 600 px exactly like a collapsed row -- no `expanded` class, no child rows. The 390 px tap, Enter and Space steps asserted only on those two, so they could no longer fail on an expansion at that width: a mutant whose select-hash row toggles the group *and* opens the sheet passed every layer on this branch, while the same mutant on master's packets.js failed the #254 E2E twice. Two independent ways to see it again: - packets.js exports `_isExpanded(hash)` on the existing test API, so the steps read expandedHashes itself. A missing hook yields the string 'NO-HOOK', which fails the assertion rather than passing it. - both E2Es now resize back to 1400 px after the mobile activations and assert the group is still collapsed -- the render-level view, and what a user rotating a phone to landscape would actually see. The #259 E2E's 390 px resize step also asserts the other half of the fix at the state level: the expansion is kept in expandedHashes, not cleared. Review F4: the touch block's comment called ?hash= the Kpa-clawbot#866 deep link. Kpa-clawbot#866 is #/packets/<hash>?obs=<id>, which does expand before the first render; ?hash= only filters. The comment now says which case the block covers and where the first-render-at-narrow-width case is covered.
Review F2 on #260: "the nodes list view adds no Escape listener" passed for the wrong reason. The list view does add one -- nodesPanelEsc in renderLeft() -- but only once the asynchronous loadNodes() resolves, and the case asserted synchronously before that. It stated something untrue about the page and could not fail. Replaced with cases that await the render (the sandbox's api() resolves immediately, so draining the microtask queue is enough) and then hold the list view to one listener. Review F3 is the leak they expose: - a region change re-runs loadNodes() -> renderLeft() inside one visit; - list -> node -> list -> node -> list across five router cycles; - destroy() must leave none behind. Supporting sandbox changes: loadNodes()'s catch is silent, so console.error is now recorded and every list-view case asserts it stayed empty -- a missing stub would otherwise read as "no listener". history.replaceState is recorded (nodesPanelEsc's observable effect), elements get a real Set-backed classList and a dataset/value, and #nodesRight starts .empty as the real markup renders it, so the handler's "only while a panel is open" guard is the browser's guard. Red on the parent commit: 2 listeners after two renders, 3 after the five cycles, 1 left after destroy().
Review F3 on #260: nodesPanelEsc had the same leak class as the item-2 fix for _nodesEsc. It was a fresh named closure registered inside renderLeft(), and renderLeft() runs on every load of the list -- a visit, a region change, a filter change -- so the listeners stacked on document and nothing ever removed them. Same shape as the item-2 fix: _nodesPanelEsc is a module-level reference, so a repeat addEventListener is a DOM no-op, and destroy() takes it off alongside _nodesEsc. Behaviour is unchanged -- it still only acts while #nodesRight is not .empty. Pre-existing, but small and in the same file as item 2.
Rapport — CS-Minimax PR#260 runde 2 — head 35f184bReview feedback addressed (commit Evidence tags: [T] test or run output, [A] assessment or inference, [K] checked in code, diff, git or CI. 1. F1 (blocking) — the 390 px "does not expand" assertions bite againAgreed, and the diagnosis was exactly right: with
Concretely:
Your RM5b now dies. I rebuilt it as M1 (
Verbatim from the run, the hook assertion and the render assertion each firing on their own: One detail worth recording: under M1 the Enter step still passes, because the preceding tap already toggled the group on and Enter toggles it back off. That is the same alternation your RM5 hit. It is why the round-trip step at the end of the block matters — it is the one assertion that is independent of how many times the mutant toggled. [T][A]
2. F2 — the nodes list-view case no longer claims something untrueAgreed. "The nodes list view adds no Escape listener" asserted synchronously, before Supporting sandbox work, because Mutant M2 — the list view registers no Escape listener at all (the 3. F3 —
|
| Case | Before the fix | After |
|---|---|---|
| the list view registers exactly one listener once its load resolves | 1 | 1 |
| a second render inside the same visit (a region change) does not add a second | 2 | 1 |
| list → node → list → node → list does not stack | 3 | 1 |
| one Escape closes the detail panel exactly once | 1 | 1 |
destroy() removes the list view's listener |
1 left | 0 |
Your Chromium count of 3 after node A → list → node B → list reproduces in the sandbox as 3. [T]
I kept "one Escape closes the detail panel exactly once" even though it passes before the fix: with the real guard modelled, handler 1 adds .empty and handlers 2 and 3 bail, so the leak is latent rather than visible — your "harmless today" is right, and the listener count is the honest observable. It is there as a regression guard on the behaviour, not as proof of the leak. [A]
Mutant M3a — renderLeft() registers a fresh wrapper function (e) { _nodesPanelEsc(e); } (the pre-existing bug): 8 passed, 3 failed (2 after two renders, 3 after five cycles, 1 left after destroy()). Mutant M3b — the stable reference kept but destroy() no longer removes it: 10 passed, 1 failed. [T]
The file is now 11 cases, up from 7.
4. F4 — the Kpa-clawbot#866 deep link
Corrected. #866 is #/packets/<hash>?obs=<id>, not #/packets/<hash>/<obs>; ?hash= only filters and expands nothing. [K]
- The PR body now names the right URL and says where the first-render-at-narrow-width case is actually covered (the
test-packets.jsunit cases, which seedexpandedHashesand render at 390 px). test-issue-259-mobile-expanded-group-e2e.js's touch-block comment now describes what the block covers — a context that starts at 390 px and never renders the group at desktop width first — and says explicitly that?hash=is not the bug: /#/packets/<hash> full-page — clicking a different observation doesn't update hex payload or path details Kpa-clawbot/CoreScope#866 link.
5. F5 and F6 — no change
- F5 (
test-packets.js:1326is red on master only because the_setDisplayGroupedhook is missing). Accurate, and your own RM1 result is the reason the case earns its place: it is the only layer that kills a revert of_getRowCountalone. Left as it is. [A] - F6 (pre-existing, identical on master's
packets.js: an?obs=deep link at 1400 px renders the header expanded with 0 children until a later refresh; the desktop detail pane survives a flip to 390 px and must be closed with ×). Both are outside Follow-ups to #255: expanded group dead end on mobile, nodesEsc listener leak, two test gaps #259's four items and neither is a dead end. Not touched; recorded here so they are not lost. [A]
Your acknowledgement of M1b (the _invalidateRowCounts() call in the mode-flip handler is unobserved by any layer) still stands, and the call is still kept. [A]
Branch
codex/issue-259-followups-255, head 35f184b4. Three new commits, tests before fixes, then a merge commit for origin/master c6b356de. All four authored and committed by dborup <kontakt@meshview.dk>. No rebase, amend or force-push; only explicit git add. [K]
8fd2be92test — F1's assertions and the_isExpandedhook, plus F4's comment.09ba33b4test — F2's rewrite and F3's cases (red on8fd2be92).f6abeef1fix —_nodesPanelEsc(green).35f184b4mergeorigin/master.
Clean merge, no conflicts. Master's only touch to public/nodes.js in that range is the advertIntervals argument on the two NodeAdverts.render(...) calls (#245), nowhere near this change. No closing keywords in the title, body or commits; closingIssuesReferences is empty. The PR stays a draft. [K]
Production diff for this round is 5 lines in public/packets.js (the test hook) and the _nodesPanelEsc move in public/nodes.js. Everything else is tests. [K]
Mutants [T]
Against the merged tree, one per finding, server restarted between the E2E ones, source restored from a scratchpad snapshot (not git checkout) and the tree verified clean after each.
| Mutant | Finding | What | test-packets.js |
nodes-esc unit | #254 E2E | #259 E2E | #189 E2E | Verdict |
|---|---|---|---|---|---|---|---|---|
| M1 | F1 | your RM5b: a select-hash group-header row calls pktToggleGroup(value) then pktSelectHash(value) |
146/146 | — | 3 failed | 2 failed | 3/3 | killed |
| M2 | F2 | renderLeft() registers no Escape listener at all |
— | 4 failed | — | — | — | killed |
| M3a | F3 | renderLeft() registers a fresh wrapper closure per render (the pre-existing bug) |
— | 3 failed | — | — | — | killed |
| M3b | F3 | stable reference kept, destroy() no longer removes it |
— | 1 failed | — | — | — | killed |
F4 is documentation only, so it has no mutant. F5 and F6 need no change. [A]
Suites (local, merged tree) [T]
A Go server built from this tree on a copy of e2e-fixture.db, prepared as in deploy.yml (freshen, the Kpa-clawbot#1486 and Kpa-clawbot#1791 seed SQL, corescope-migrate, then seeds 2073, 199 and 245), on port 13700, stopped by port afterwards.
| Suite | Result |
|---|---|
sh test-all.sh |
220 passed, 0 failed (220 files) — serial run, no flakes |
node test-frontend-helpers.js |
707 passed, 0 failed |
node test-packets.js |
146 passed, 0 failed |
node test-issue-259-nodes-esc-listener.js |
11 passed, 0 failed (was 7) |
node test-issue-254-affinity-toggle-mobile-aria-e2e.js |
14 passed, 0 failed (was 13) |
node test-issue-259-mobile-expanded-group-e2e.js |
10 passed, 0 failed (was 9) |
node test-issue-189-group-caret-e2e.js |
3 passed, 0 failed |
node test-issue-1461-mobile-page-actions.js |
6 passed, 0 failed (a unit file, no server needed — it is in test-all.sh, not deploy.yml) |
sh scripts/check-xss-sinks.sh --diff origin/master |
exit 0, no output |
CI [K][T]
Run 37363814293 on head 35f184b4, attempt 1, conclusion success. No reruns, and neither known flake fired — #267 (the backfill write-hold) and #271 did not fail, and nothing in the run was rerun.
| Job | Result | Evidence from the log |
|---|---|---|
| ✅ Go Build & Test | success | test-all.sh 220 passed, 0 failed (220 files); test-issue-259-nodes-esc-listener.js ran with all four new list-view cases green (the list view registers exactly one Escape listener once its load resolves, a second render inside the same visit does not add a second listener, list -> node -> list -> node -> list does not stack listeners, destroy() removes the list view's Escape listener); the PR-only XSS --diff origin/master preflight ran |
| ✅ Playwright E2E Tests | success | test-issue-254-affinity-toggle-mobile-aria-e2e.js 14/14 (the new round-trip step included), test-issue-259-mobile-expanded-group-e2e.js 10/10, test-issue-189-group-caret-e2e.js 3/3 |
| ✅ Build & Publish Docker Image | success | — |
| 📦 Release Artifacts | skipped | fork-guarded |
| 🚀 Deploy Staging | skipped | fork-guarded |
| 📝 Publish Badges & Summary | skipped | fork-guarded |
One log line is worth pre-empting, because it looks alarming on a grep: [FAIL] STALE ALLOWLIST: 1 entry matched nothing this run in the Go job. That is test-a11y-axe-1668-selftest.js deliberately printing its own gate's failure text for a synthetic entry, and it ends PASS: a11y-axe-1668 selftest. Present on master too, unrelated to this PR. [K]
Not verified
- Firefox, WebKit/Safari, a real phone, a real screen reader. [A]
- The Go suites were not re-run locally this round; no
.gofile is touched by any of the three commits, and CI's Go job covers the merge. [A][K] aria-expandedonrole="row"in a plain table with a real screen reader, carried over 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. [A]- Staging and production, by design. No staging or production access was used, and no API key.
- Screenshots were not retaken this round; the two new steps are state and attribute assertions, not visual ones. [A]
Review — CS-pve-agent1 PR#260 — head 35f184bDom: APPROVE with nits Evidence tags: [T] test or run output, [A] assessment or inference, [K] checked in code, diff, git or CI. This is round 2. The blocking finding from round 1 (F1) is fixed: the 390 px "does not expand" assertions bite again, at two independent layers. That includes RM5b, which survived round 1. All five of my packets mutants die, among them one that blinds the new test hook, so the render-level round trip carries it alone. F2 and F3 are fixed, and the fixes are proven by tests that are red on master and on the round-1 head. In a real browser, after A → list → B → list, there is exactly one Findings
The author's M1b (the The review points
Always-checks
Tests run (merged tree
|
| Suite | Result |
|---|---|
sh test-all.sh |
221 passed, 0 failed (221 files) (master added one file since c6b356de) |
node test-frontend-helpers.js |
707 passed, 0 failed |
node test-packets.js / nodes-esc / #254 unit / Kpa-clawbot#1461 unit |
146/146, 11/11, 12/12, 6/6 |
cmd/server go test ./... |
ok on a serial rerun (370 s). The first run, in parallel with my E2E and mutant runs, had 1 failure: TestHandleAnalyticsSubpathDetailWithStore (coverage_test.go:2700, expected 200, got 503). The test requests /api/analytics/subpath-detail without waiting for the async subpath index, and the handler answers 503 until SubpathIndexReady() (routes.go:3021). It passed 20/20 in isolation (-count=20). The flake class is the same as the closed #227, in a different test. No Go code is in this PR, so it is master's code [T][K]. |
cmd/ingestor go test ./... |
ok (920 s) |
E2E against a local Go server built from the merged tree on a copy of e2e-fixture.db, prepared as in deploy.yml (freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL, corescope-migrate, seeds 2073, 199 and 245), restarted with a fresh fixture per mutant, stopped by port |
#259 10/10, #254 14/14, #189 3/3 |
Own Chromium scripts (CDP listener counts, Escape on the list, pktEsc) |
as reported above |
CI on head 35f184b4: run 37363814293, attempt 1, success. Jobs:
- ✅ Go Build & Test:
test-all.sh220/220, nodes-esc 11/11. - ✅ Playwright E2E: test: 13 red orphan unit tests + 4 orphan E2E files left after #187; collapsed packet groups show an up-caret #189 3/3, Follow-up to #248: Affinity Debug toggle broken + reversed carets; group-row aria-expanded on mobile #254 14/14 with the round-trip step, Follow-ups to #255: expanded group dead end on mobile, nodesEsc listener leak, two test gaps #259 10/10.
- ✅ Docker.
- Release, Deploy Staging and Badges: skipped (fork-guarded).
Neither known flake fired: the Hash Stats sort (#256) passed, and the backfill write-hold (#267) did not fail. [K]
Not verified
- CI on the merged tree against the current
origin/masterf91339f2; master moved after the branch's merge ofc6b356de, and only my local runs cover that. [A] - The
-racevariant of the Go suites that CI runs. No Go code changes in this PR. [A] - Firefox, WebKit/Safari, a real phone, a real screen reader. [A]
- Staging and production, by design. No API key was used.
- No screenshots this round; every check was an attribute, state or listener assertion.
Head was 35f184b4dc8bd314b5a5a6a9ae53972a014b5946 on git ls-remote before and after the review. Nothing was pushed or changed on the PR.
#268 (issue #258, column widths) and this branch both appended a line to test-all.sh's runner list. Kept both, in issue order next to #254: run test-issue-258-column-widths.js run test-issue-259-nodes-esc-listener.js Everything else auto-merged and was verified to be the exact union of both sides: .github/workflows/deploy.yml keeps the #258 and #259 E2E lines, public/packets.js keeps #258's TableResponsive.unhidden() and #259's groupIsExpandedInView(), and test-packets.js keeps both sets of cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rapport — CS-Minimax PR#260 merge-konflikt — head bd1e9e5
Evidence tags: [T] test or run output, [A] assessment or inference, [K] checked in code, diff, git or CI. 1. The conflict —
|
merged test-all.sh vs |
added | removed |
|---|---|---|
branch 35f184b4 |
run test-app-api-bust-inflight-243.js, run test-issue-258-column-widths.js |
— |
master 406f8481 |
run test-issue-259-nodes-esc-listener.js |
— |
test-test-all.js independently confirms the resolution is well-formed: every registered file exists, no file is registered twice, every root test-*.js runs in test-all.sh or deploy.yml, 10/10. [T]
The merge commit carries no content of its own beyond that resolution. git show --cc bd1e9e5e — which lists only what differs from both parents — shows test-all.sh and nothing else: [T][K]
diff --cc test-all.sh
@@@ -58,7 -59,7 +59,8 @@@
run test-issue-254-affinity-debug-toggle.js
+ run test-issue-258-column-widths.js
+run test-issue-259-nodes-esc-listener.js
run test-perf-go-runtime.js
2. The auto-merges — checked, both sets of changes intact
public/packets.js — the two PRs touch disjoint regions, so there was nothing to reconcile: #258 adds unhidden() and extends the window.TableResponsive export in the TableResponsive IIFE around lines 32-242; #259 adds groupIsExpandedInView(), the _invalidateRowCounts() call on the breakpoint crossing, its two call sites in buildGroupRowHtml() / _getRowCount(), and the _isExpanded / _setDisplayGrouped test hooks, from line ~2431 down. Diffing the merged file against each parent gives exactly the other parent's hunks, with no deletions: [K]
- vs branch:
+unhidden()(18 lines) andsweep: sweepDetached→sweep: sweepDetached, unhidden; - vs master:
+groupIsExpandedInView(),+_invalidateRowCounts(), the twoexpandedHashes.has(p.hash)→groupIsExpandedInView(p.hash)call sites,+_isExpanded,+_setDisplayGrouped.
.github/workflows/deploy.yml — the union of both E2E lines, each still inside the Playwright step and each still on BASE_URL=http://localhost:13581: [K]
BASE_URL=... node test-issue-1122-details-row-clamp-e2e.js
BASE_URL=... node test-issue-258-column-widths-e2e.js # from master
...
BASE_URL=... node test-issue-254-affinity-toggle-mobile-aria-e2e.js
BASE_URL=... node test-issue-259-mobile-expanded-group-e2e.js # from this branch
test-packets.js — purely additive both ways: the merged file adds master's 39 lines over the branch and the branch's 95 lines over master, with 0 removed lines in either direction. [T]
3. Tests
Node suites, on the merge commit: [T]
| Suite | Result |
|---|---|
sh test-all.sh |
222 passed, 0 failed (222 files) |
node test-frontend-helpers.js |
707 passed, 0 failed |
node test-packets.js |
148 passed, 0 failed, 0 known bugs still failing |
node test-issue-259-nodes-esc-listener.js |
11 passed, 0 failed |
test-packets.js is 148 here against the 146 reported in round 2 — the +2 are #258's cases arriving from master, not a change on this branch. [A][K]
E2E against a local corescope-server on port 13700 (cmd/server built from the merge commit, serving public/), with the fixture prepared the way deploy.yml does it — a scratch copy of test-fixtures/e2e-fixture.db, tools/freshen-fixture.sh, the inline Kpa-clawbot#1486/Kpa-clawbot#1791 seed, corescope-migrate, then the Kpa-clawbot#2073, #199 and #245 seed files. All four exit 0: [T]
| E2E | Result |
|---|---|
test-issue-254-affinity-toggle-mobile-aria-e2e.js |
14 passed, 0 failed |
test-issue-259-mobile-expanded-group-e2e.js |
10 passed, 0 failed |
test-issue-258-column-widths-e2e.js |
22 passed, 0 failed |
test-issue-1122-details-row-clamp-e2e.js |
18 passed, 0 failed |
That is the answer to the question the merge actually raised — whether #258's column widths and #259's mobile/nodesEsc behaviour both still hold in the same tree. They do, from both directions: #258's "Details is >= 15% and expand <= 10% of the table" passes at 1200 px and 900 px across both fixture windows, and #259's "resize 1400 -> 390: no child row is left visible" / "resize 390 -> 1400: the children and the state come back" pass in light and dark, while #254's 390 px tap/Enter/Space cases and the 1400 px round trip still assert nothing expanded the group. [T]
4. CI
Run 37398668886. Attempt 2 is green on every job. Per job: [K]
| Job | Attempt 1 | Attempt 2 |
|---|---|---|
| ✅ Go Build & Test | failure — cmd/server hit the 20-minute go test timeout |
success (22m 45s) |
| 🎭 Playwright E2E Tests | skipped (needs: go-test) |
success |
| 🏗️ Build & Publish Docker Image | skipped (needs: e2e-test) |
success |
| 📦 Release Artifacts | skipped | skipped |
| 🚀 Deploy Staging | skipped | skipped — gated to push on refs/heads/master of Kpa-clawbot/CoreScope |
| 📝 Publish Badges & Summary | skipped | skipped |
The four E2Es this merge put at risk all pass in CI with exactly the counts I measured locally: #254 14 passed, 0 failed; #259 10 passed, 0 failed; #258 22 passed, 0 failed; Kpa-clawbot#1122 Details clamp passed 18 failed 0. [T]
The attempt-1 failure was the cmd/server time budget, not this merge
I re-ran it rather than changing anything, and the same commit passed. The evidence that it is a pre-existing master-side budget problem: [K][T][A]
-
The merge adds no Go code at all.
git diff 406f8481 bd1e9e5e -- '*.go' go.mod go.sumis empty — the merged tree differs from master only inpublic/nodes.js,public/packets.js, fourtest-*.jsfiles, onetest-all.shline and onedeploy.ymlE2E line. Nothing in the timed-out package is reachable from any of them. [K] -
It was a wall-clock timeout, not a failing assertion or a deadlock.
go test -timeout 20m -race -coverprofileon./cmd/server:panic: test timed out after 20m0s running tests: TestTrackedBytesTracksTheHeap_113 (33s) FAIL github.com/corescope/server 1200.263sOne test was running, 33 s in; every other goroutine in the dump is an idle
database/sql.(*DB).connectionOpenerorhealSchemaFlagsselect. The package had already printedcoverage: 88.9% of statements— it ran out of total budget, nothing was stuck. [T] -
The same commit, same flags, 177 s apart across the two attempts — and the package was already at ~91 % of budget on the fix(ui): column widths ignore colspan rows and empty first renders (#258) #268 side before the merge:
Run Commit cmd/servervs the 1200 s limit37329171525 — PR fix(ui): column widths ignore colspan rows and empty first renders (#258) #268's own green run da436cd61090.204 s (91 %) 37323250705 — this branch, green 0ed1a3df1061.690 s (88 %) 37363814293 — this branch at 35f184b4, green35f184b4869.232 s (72 %) 37398668886 attempt 1 — the merged tree bd1e9e5etimed out at 1200 s 37398668886 attempt 2 — the merged tree bd1e9e5e1023.028 s (85 %), coverage: 90.4%The run-to-run spread on identical or near-identical trees is ~220 s, larger than the ~110 s of headroom PR fix(ui): column widths ignore colspan rows and empty first renders (#258) #268 left. Merging the two sides puts fix(ui): column widths ignore colspan rows and empty first renders (#258) #268's new
cmd/servertests (api_fallback_test.go,paths_confirm_deferred_test.go,pathlen_fast_test.go, plus the extendedresolved_path_backfill_188_test.go/reach_rank_test.go/coverage_test.go) into a package that had little room for them — this branch at35f184b4did not have those tests, which is why its own runs were the fastest of the set. So the merge did not break the Go job, but it does leave it closer to the edge, and the next few points of growth incmd/serverwill time out on master too. [A][T] -
Master has no green datapoint at
406f8481to compare against: its own CI/CD run there (37398092159) was cancelled by the next push, and master has since moved on to34f672c6. [K]
Per the task I made no other changes, so I did not touch -timeout or the slow tests. Raising the budget or splitting cmd/server is a master-side call and worth adding to the follow-up issue next to the round-2 nits. [A]
2 participants
Relates to #259
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 close two gaps in #255's own tests.1. An expanded group no longer leaves orphan children on mobile
A group expanded at desktop width kept
class="expanded"and its child rows after the layout crossed to ≤ 600 px. There the expand column is hidden and the row only selects (Kpa-clawbot#1461 #7, #255), so nothing was left to collapse it — the dead end upstream#1461#7 describes, and finding F2 of the #255 review.Chosen option: keep the hash in
expandedHashesand leave the children out of the rendered slice while the mobile mode is active, rather than clearing the set on the flip. Two reasons:#/packets/<hash>?obs=<id>adds the hash toexpandedHashesbefore any row is built — which a mode-flip hook alone would not catch. That first-render case is covered by thetest-packets.jsunit cases, which seedexpandedHashesand render at 390 px; the E2E's 390 px touch context is reached by?hash=, which only filters and expands nothing.One render-time helper,
groupIsExpandedInView(), is used by bothbuildGroupRowHtmland_getRowCount, so the virtual-scroll row counts and the rendered rows cannot diverge (Kpa-clawbot#424). The mode-flip resize handler now also invalidates the cached counts, because an expanded group's row count changes with the mode.Desktop rendering is untouched: carets,
aria-expandedandexpandedHashesbehave exactly as in #248/#255, over live updates, sorting and filtering.2. One Escape listener per node page, not one per visit
nodes.jsinit()added a freshnodesEscclosure todocumenton every full-screen node-page init and only unhooked it when Escape actually fired, so node A → B → C left three live listeners and one Escape wrote the same hash three times (finding F4).The handler is now a single module-level
_nodesEsc— so a repeataddEventListeneris a DOM no-op — anddestroy()removes it, which covers both the router's destroy/init cycle and the in-modulenavigateToNodepath.The nodes list view had the same leak in
nodesPanelEsc(Escape closes the detail panel), registered insiderenderLeft().renderLeft()runs on every load of the list — a visit, a region change, a filter change — so the listeners stacked there too, and nothing ever removed them. Fixed the same way, as_nodesPanelEsc(finding F3 of the #260 review; pre-existing, but small and in the same file).3. Test gap F1: the fast layer now proves the node page uses the renderer
test-issue-254-affinity-debug-toggle.jsexercisedrenderAffinityDebugCard()in isolation, so the review's mutant — master's inlineonclickcard back in theloadFullNodetemplate, the renderer exported but unused — passed it 9/9 and only the E2E caught it. Three source assertions close that: the template interpolates${renderAffinityDebugCard()},id="node-affinity-debug"occurs exactly once inpublic/nodes.js, and no inlineon*=handler sits anywhere near the card. That mutant now fails the unit test 3 cases.4. Test gap F3: Space on a mobile group row
The #254 E2E pressed Enter on the 390 px group row but not Space, although the handler treats them the same. A Space step was added: the detail sheet opens, the group does not expand, and the row is still
select-hashwithoutaria-expanded.Item 1's render rule makes the "does not expand" half invisible in the DOM: at ≤ 600 px a hash that is in
expandedHashesrenders exactly like a collapsed row. So the 390 px tap, Enter and Space steps now also readexpandedHashesitself, through a new_isExpanded(hash)hook on_packetsTestAPI, and both mobile E2Es resize back to 1400 px afterwards and assert the group is still collapsed — the render-level view, and what a user rotating a phone to landscape would see (finding F1 of the #260 review).Tests
test-packets.js_getRowCount, 600/601 px, state survives the round trip)test-issue-259-nodes-esc-listener.js(new, registered intest-all.sh)keydownlisteners and the hash writes — A → B → C for the full-screen view, and the list view over a region change and five router cyclestest-issue-254-affinity-debug-toggle.jstest-issue-259-mobile-expanded-group-e2e.js(new, registered indeploy.ymlnext to the #254 line)test-issue-254-affinity-toggle-mobile-aria-e2e.jsexpandedHashesassertions on tap/Enter/Space, and a 390 → 1400 round trip_setDisplayGroupedand_isExpandedjoin the existing_packetsTestAPIhooks: the first so the unit sandbox can exercise_getRowCountin grouped mode, the second so the mobile E2E steps can see an expansion the mobile render deliberately hides.The mutants, the suite results and the screenshots are in the report comment below.
Not changed
public/mobile-page-actions.jsis left as it is.deploy.ymlgains exactly one line and the fork guards stay at 9 and 1.🤖 Generated with Claude Code