Skip to content

fix(packets): one pktEsc listener per page and #268/#260/#212 follow-ups (#282) - #313

Merged
dborup merged 2 commits into
masterfrom
codex/issue-282-pktesc-leak-followups
Oct 6, 2026
Merged

dborup merged 2 commits into
masterfrom
codex/issue-282-pktesc-leak-followups

Conversation

@dborup-agent

Copy link
Copy Markdown
Collaborator

Relates to #282

Follow-up nits from the reviews of #268, #260 and #212. All 8 points handled, in order. No behaviour change beyond the listed points; cmd/server stays read-only (only a new *_test.go added there).

Plan (as built)

Tests first, then the code (two commits). Every point has a test and at least one mutant; where a point is a pure comment/text fix the guard is on the source text, and the behaviour it describes is covered by existing tests (named below).

# Point Fix Test Mutant
1 pktEsc leaks a listener public/packets.js: _pktEsc is a stable module-level handler, added once in renderLeft() (repeat add = DOM no-op) and removed in destroy() — same shape as #260's _nodesPanelEsc test-issue-282-pktesc-listener.js drives init/destroy over several #/packets visits and counts document keydown listeners master stacks listeners / destroy() leaves one → red
2 stale #866 deep-link comment public/packets.js: #/packets/<hash>?obs=<id> test-issue-282-comment-guards.js (point 2) re-introduce /<obs> form → red
3 untested _nodesPanelEsc guard (mutant ND) n/a (guard already correct) test-issue-259-nodes-esc-listener.js: Escape while the panel is still .empty must do nothing dropping the .empty guard → red
4 wrong #258 E2E header comment test-issue-258-column-widths-e2e.js: describes dragging the Time handle right test-issue-282-comment-guards.js (point 4) re-introduce "Path handle … left" → red
5 wrong PR claim "analytics never create an observer" test-issue-258-column-widths.js header: the trigger is the first render's usable-row count, any table (analytics included) with < 5 does create the one-shot observer test-issue-282-comment-guards.js (point 5); behaviour already pinned by the "empty"/"near-empty first body" cases in test-issue-258-column-widths.js drop the sparse-body observer branch → those cases red
6 untested transport-route frame through GetChannelMessages n/a (read-only) cmd/server/channel_transport_hash_size_282_test.go: TRANSPORT frames (v2+v3, db+store) so substr(t.raw_hex, 1, 12) reaches the byte-5 path length shorten to substr(…, 1, 11) → red (FLOOD test stays green)
7 two offset sources in the Path Length row public/packets.js: the encoded hash size is read from the same path-length byte the row prints (offset from pkt.route_type), not re-derived inside senderPathHashSize(buf) from the header byte test-packets.js: a transport frame whose stored route_type and on-wire header route bits disagree — the printed byte and its hash_size label must describe the same byte revert to senderPathHashSize(buf) → red
8 exports used only by tests public/channels.js: removed renderObservedPathHashBadge + OBSERVED_PATH_HASH_TOOLTIP (no shipped reader) test-channels-observed-path-hash-size.js: badge-only cases dropped; normalize/union coverage retained — (dead-code removal)

Verification

  • sh test-all.sh: 224/224 files pass (incl. the two new node suites + the meta-registration test).
  • cd cmd/server && go build . && go vet ./... && go test ./...: green; channel-message suite also green under -race.
  • scripts/check-xss-sinks.sh --diff origin/master: clean.
  • Fork-guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml. No new map[string]interface{} outside tests. No hardcoded colours.
  • Browser check against a local Go server on the CI-prepared e2e-fixture.db:
    • test-issue-147-packets-url-obs-e2e.js (deep link + Esc): 12/12.
    • test-issue-180-packets-url-modal-e2e.js (Esc / detail sheet): 12/12.
    • test-issue-259-mobile-expanded-group-e2e.js (group expansion): 10/10.
    • transport-route packet detail: the Byte Breakdown renders Path Length 0x40 → hash_size=2 bytes, hash_count=0 with Transport Codes / Next Hop / Last Hop (point 7 live).

Draft until review.

dborup and others added 2 commits October 6, 2026 11:37
…212

Tests first, red on master where there is a behaviour/offset change to pin:

- #282 (1) test-issue-282-pktesc-listener.js: drives packets.js init/destroy
  over several #/packets visits and counts document keydown listeners. Red on
  master (listeners stack; destroy() leaves one behind), green once _pktEsc is a
  stable module-level reference removed in destroy().
- #282 (3) test-issue-259-nodes-esc-listener.js: adds the missing _nodesPanelEsc
  "only act while a detail panel is open" guard case (kills mutant ND).
- #282 (6) cmd/server/channel_transport_hash_size_282_test.go: sends TRANSPORT
  frames through GetChannelMessages so the substr(t.raw_hex, 1, 12) slice is
  exercised to byte 5; shortening it to 11 turns this red while the FLOOD test
  stays green.
- #282 (7) test-packets.js: buildFieldTable must read the Path Length width from
  the byte it prints (one offset source), not a second header-derived offset.
- #282 (8) test-channels-observed-path-hash-size.js: drops the badge-only cases
  for the removed renderObservedPathHashBadge export.
- #282 (2,4,5) test-issue-282-comment-guards.js: source-text guards for the
  comment/text corrections, red against the pre-#282 wording.
- test-all.sh: register the two new node suites.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/text fixes

- #282 (1) public/packets.js: _pktEsc is now a stable module-level handler,
  registered once in renderLeft() (a repeat add is a DOM no-op) and removed in
  destroy(), so #/packets no longer stacks a document keydown listener per
  visit. Same fix shape as nodes.js' #259 nodesPanelEsc.
- #282 (7) public/packets.js: the Path Length row in the hex breakdown derives
  the encoded hash size from the same path-length byte it prints (offset from
  pkt.route_type) instead of re-deriving the offset inside senderPathHashSize()
  from the raw_hex header byte. One offset source; for a well-formed frame the
  result is unchanged, including transport-route frames.
- #282 (2) public/packets.js: the Kpa-clawbot#866 deep-link comment now reads
  #/packets/<hash>?obs=<id>, the actual form.
- #282 (8) public/channels.js: remove renderObservedPathHashBadge and
  OBSERVED_PATH_HASH_TOOLTIP -- the only reader was the test export; no shipped
  code rendered the "Observed path hash" badge.
- #282 (4) test-issue-258-column-widths-e2e.js: header now describes dragging
  the Time handle right (what the test does), not the Path handle left.
- #282 (5) test-issue-258-column-widths.js: correct the claim that analytics
  tables never create an observer -- any table whose first render has < 5 usable
  rows creates the one-shot MutationObserver.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-MacBook PR#313 #282 — head 275296a

Status: All 8 points handled in order on a draft PR; every point has a test and a mutant, all local suites are green, the browser checks pass, and the single (fork-scoped) CI job is still running — results below will be updated when it finishes.

Evidence tags: [T] test/run output, [A] assessment/inference, [K] checked in code/diff/CI.

Branch

codex/issue-282-pktesc-leak-followups from origin/master 298609e0 (clean ancestor; origin/master has since advanced to 03f9a9d6 on unrelated work — no rebase). Two commits, tests before code, author+committer dborup <kontakt@meshview.dk>. Only explicit git add; no rebase/amend/force-push. No closing keywords in title/body/commits. [K]

  • 59fd2437 tests (red on master where a behaviour/offset changes)
  • 275296a1 the fixes + comment/text corrections

Requirements

# Point Fix Test Mutant Result
1 pktEsc listener leak public/packets.js: _pktEsc is module-level, added once in renderLeft() (repeat add = DOM no-op), removed in destroy() test-issue-282-pktesc-listener.js (7 cases): counts document keydown listeners over several init/destroy visits master stacks / destroy() leaves one → 3 cases red [T] ✅ [T]
2 stale Kpa-clawbot#866 deep-link comment public/packets.js: #/packets/<hash>?obs=<id> test-issue-282-comment-guards.js p2 re-add /<obs> → red [T] ✅ [T]
3 untested _nodesPanelEsc guard (ND) — (guard already correct) test-issue-259-nodes-esc-listener.js: Escape while panel .empty does nothing drop .empty guard → red (verified) [T] ✅ [T]
4 wrong #258 E2E header test-issue-258-column-widths-e2e.js: Time handle dragged right test-issue-282-comment-guards.js p4 re-add "Path … left" → red [T] ✅ [T]
5 wrong "analytics never create an observer" test-issue-258-column-widths.js header: row-count rule, analytics not exempt test-issue-282-comment-guards.js p5; behaviour already pinned by the "empty"/"near-empty first body" cases drop sparse-body observer branch → those cases red [A] ✅ [T]
6 untested transport frame via GetChannelMessages — (cmd/server read-only) cmd/server/channel_transport_hash_size_282_test.go: TRANSPORT frames, v2+v3, db+store substr(…,1,12)→,1,11) → transport test red, FLOOD test green (verified) [T] ✅ [T]
7 two offset sources in Path Length row public/packets.js: width read from the same byte the row prints (offset from pkt.route_type), not re-derived in senderPathHashSize(buf) test-packets.js: transport frame where stored route_type ≠ header route bits revert to senderPathHashSize(buf) → red (master prints 0x80 but labels hash_size=2) [T] ✅ [T]
8 exports only used by tests public/channels.js: removed renderObservedPathHashBadge + OBSERVED_PATH_HASH_TOOLTIP (no shipped reader; renderSenderPathHashBadge is the live one) test-channels-observed-path-hash-size.js: badge-only cases dropped, normalize/union retained — dead-code removal [K] ✅ [T]

Local checks [T]

  • sh test-all.sh: 224/224 files pass (incl. the two new node suites + the meta registration test).
  • cd cmd/server && go build . && go vet ./... && go test ./...: green; -race on the channel-message suite green.
  • scripts/check-xss-sinks.sh --diff origin/master: clean.
  • Fork-guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml. [K]
  • No new map[string]interface{} outside tests; no hardcoded colours. [K]
  • cmd/server production code untouched (only a new *_test.go). [K]

Browser check [T] — local Go server (my build) on CI-prepared e2e-fixture.db, port 13900

  • test-issue-147-packets-url-obs-e2e.js (deep link + Esc): 12/12.
  • test-issue-180-packets-url-modal-e2e.js (Esc, detail sheet): 12/12.
  • test-issue-259-mobile-expanded-group-e2e.js (group expansion): 10/10.
  • Transport-route packet detail (point 7): Byte Breakdown renders Path Length 0x40 → hash_size=2 bytes, hash_count=0 with Transport Codes / Next Hop / Last Hop. Only console noise was GET /api/nodes/<fake-pubkey> 404 from the synthetic packet's dummy key — unrelated.

CI [K]

Single fork-scoped job Go Build & Test (deploy/E2E jobs are fork-guarded off on this fork). In progress at report time; will update with the per-step outcome. Known-flaky #271 noted — if it is the only failure I will re-run the job once.

Rest / notes

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

CI update — green [T]

Run 37445133914 concluded success (head 275296a1), per job:

No failures; #271 did not need a re-run. PR stays a draft.

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent3 PR#313 — head 275296a

Dom: APPROVE med nits

Independent, read-only review. Evidence tags: [T] test/run output I produced, [A] assessment/inference, [K] checked in code/diff/CI.

Trees used: git archive of the head, and of the merged tree git merge-tree --write-tree origin/master 275296a1… = 09797cdd against origin/master 03f9a9d6. The merge is clean; the only master change since the merge base 298609e0 is test-issue-180-packets-url-modal-e2e.js, which this PR does not touch. [K] git ls-remote on the head gave 275296a1 both before and after the review. [K]

Findings

# Severity Where Finding Evidence
1 Nit (DRY) public/packets.js buildFieldTable (point 7) The fix inlines a second copy of senderPathHashSize()'s width rules: TRACE gives no width, 0b11 is invalid, and 0x00 on route 2/3 is the direct marker. AGENTS.md asks for one implementation. A small shared helper in app.js, e.g. pathHashSizeFromByte(pathByte, routeType, headerByte), could be called by both senderPathHashSize (with the header-derived offset) and the Path Length row (with the pkt.route_type offset). That would still give one offset source per caller. [K] diff; [A]
2 Nit (test gap) same lines Mutant (pkt.route_type === 2 || pkt.route_type === 3) → (pkt.route_type === 2) survives test-packets.js and test-frontend-helpers.js, so no test covers the TRANSPORT_DIRECT 0x00 marker in the hex breakdown. The same mutant in app.js senderPathHashSize also survives on master, so the gap is pre-existing, not new. But the PR now has two untested copies of the rule. One buildFieldTable case with raw_hex: '17aabbccdd00…', route_type: 3 would kill both mutants if the helper in finding 1 is shared. [T]
3 Nit (comment/test) test-issue-282-pktesc-listener.js header and case "a second render inside the same visit…" The header says the old closure stacked "on every filter/region change within a visit". It did not. renderLeft() returns at the filtersBuilt guard before reaching addEventListener, so the leak was once per visit. As a result, this case is also green on master; it documents behaviour but is not a regression guard. The other three leak cases are red on master and carry the fix. Suggest correcting the header or relabelling the case. [T] red run; [K] renderLeft() filtersBuilt
4 Nit (comment) public/channels.js #282 (8) comment The comment says "normalizeObservedPathHashSizes() stays: the union/merge path and the sender badge still use it". renderSenderPathHashBadge() does not call it; it reads message.senderPathHashSize. The real callers are union, the cached-message merge, the packet → message mapping and the dedup key. [K]
5 Info author report The report says deploy/E2E jobs are fork-guarded off. On this run, 🎭 Playwright E2E Tests does run (see CI below). [K]

None of these blocks the PR. 1+2 are best fixed together; 3+4 are one-line comment edits.

Answers to the review points

  1. pktEsc leak — fixed. _pktEsc is now a module-level function. It is added in renderLeft() and removed in destroy(), the same shape as _nodesEsc/_nodesPanelEsc in nodes.js. [K]
    • Unit test. test-issue-282-pktesc-listener.js against master's packets.js gives 3/7 red: 4 listeners after 4 visits, destroy() leaves 1, and the reopened page has 2. On the PR it is 7/7 green. [T]
    • Real browser (Chromium, CDP getEventListeners(document), local server). On the PR, document keydown listeners stay at 5 on the first visit to #/packets, after 5 more #/nodes ↔ #/packets round trips, and on #/nodes. On master's frontend they grow 8 → 13 → 14. [T]
    • Esc still works. Esc closes an open detail panel (#pktRight goes .empty). There were no page errors. [T]
    • Order (fix(packets): keep ?obs=/?viewPath= on packet detail URLs (#147) #167). The handler body is unchanged. It is a bubble-phase document listener, as before. Modal Esc handlers (packet-path-map.js, channel-color-picker.js) listen in the capture phase. A stable reference only removes the duplicates; it does not move the first one. [K]/[A]
    • E2E. test-issue-180-packets-url-modal-e2e.js (12/12) and test-issue-147-packets-url-obs-e2e.js (12/12) are green. [T]
  2. Points 3 and 6 — the new tests.
    • Point 3. The new _nodesPanelEsc .empty guard case passes. Mutant ND (if (!panel) return;) turns it red: replaceState ["#/nodes"]. [T]
    • Point 6. TestChannelMessagesSenderPathHashSize_TransportRoute_282 (v2+v3, db+store) passes. With mutant substr(t.raw_hex, 1, 11) at both SQL sites, the transport test fails (v2 and v3) and the FLOOD test stays green, exactly as the PR claims. With substr(…, 1, 2), both fail. [T]
  3. Point 7 — offsets are consistent. The printed byte and the hash_size label now come from the same off. [K]
    • Unit test. The new test-packets.js case is red on master (hash_size=2 printed for 0x80) and green on the PR. [T]

    • Mutants. "revert to senderPathHashSize(buf)" and "read path byte at the header offset" are both killed. "drop the 0b11 guard" is killed by an existing case. [T]

    • Browser, PR vs master frontend. Same local server and DB, with synthetic rows added to a scratch copy of the fixture. Both frontends render identical rows:

      • TRANSPORT_FLOOD 0x14…41…: 5 Path Length 0x41 hash_size=2 bytes, hash_count=1, with Next Hop/Last Hop at 1/3.
      • TRANSPORT_DIRECT 0x17…00: 5 Path Length 0x00 hash_count=0 (no encoded hash size).
      • FLOOD 0x15 81…: 1 Path Length 0x81 hash_size=3 bytes, hash_count=1.

      So there is no visible change for well-formed frames, as intended. [T]

    • E2E. test-packet-detail-sender-hash-size-obs-e2e.js passes 3/3. [T]

  4. Point 8 — removed exports. grep over the merged tree for renderObservedPathHashBadge, OBSERVED_PATH_HASH_TOOLTIP and _channelsRenderObservedPathHashBadgeForTest (*.js, *.html, *.go, *.md) finds only the two new explanatory comments; there are no code readers in public/. The .ch-path-hash-badge class is still used by the sender badge. test-channels-observed-path-hash-size-e2e.js passes 8/8. The injection-shaped rejection is still covered by the normalize case "rejects unknown, empty and injection-shaped values". [K]/[T]
  5. Points 2, 4 and 5 — comments and text.
    • Point 2. #/packets/<hash>?obs=<id> matches the ?obs= handling in packets.js. [K]
    • Point 4. The test drags th.col-time .col-resize-handle to the right (+60 px) and asserts that Time widens, which matches the corrected header. [K]
    • Point 5. In app.js, the observer is created when rows.length < COL_MEASURE_MIN_ROWS (5), with no page check, and analytics tables call makeColumnsResizable (analytics.js:521). The corrected header is right. [K]
    • All three guards in test-issue-282-comment-guards.js are red on the old wording and green on the new. [T]
  6. Scope and rules.
    • Behaviour. There is no behaviour change beyond points 1 and 7. Point 7 only differs when pkt.route_type and the header's route bits disagree, which is the point of the fix. [K]/[A]
    • XSS gate. scripts/check-xss-sinks.sh --diff origin/master passes (exit 0). [T]
    • Colours. No hardcoded colours were added; the only #… matches in added lines are #282 in comments. [K]
    • cmd/server and map[string]interface{}. cmd/server has no non-test changes, so it stays read-only, and the diff adds 0 new map[string]interface{} outside _test.go. [K]
    • Fork guards and keywords. Fork guards are unchanged: 9 in deploy.yml and 1 in release-fast-path.yml. There are no closing keywords in the title, body or commits (closingIssuesReferences = 0). [K]
    • Commits. Both commits have author and committer dborup <kontakt@meshview.dk>. [K]

Tests and mutants (my runs, merged tree 09797cdd)

Not verified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants