Repository navigation
fix(ui): stop compare/channels/live leaking handlers on #app (#428) - #437
Merged
Merged
Conversation
Remainder of #405: the 15 page-module handlers #419 found bound to the shared #app/document/window and never removed in destroy() — compare.js (1), channels.js (6), live.js (8). Each is now bound to the page's own container (click delegation in compare.js/channels.js) or stored as a module ref and removed in destroy() (the global document/window binds: modal Escape handlers, sidebar/feed/timeline drag tracking, the show-encrypted-channels and tile-provider-changed events, the VCR more-menu outside-click, and live.js's DragManager resize clamp). live.js's orientationchange listener was a ref-mismatch bug on top of the leak: destroy() removed `_onResize` under the 'orientationchange' name, which never matched the actual anonymous listener bound there, so it was never removed at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
Rapport — CS-Minimax PR#437 #428 — head 28c88a2CI is fully resolved: every job on the PR head has finished. The only test failure is Evidence tags: [T] run locally, [A] analysis/code reading, [K] CI. CI — run 38415434009 (pull_request, attempt 1)
The failure is not from this PR.
What the change isOne commit,
[A] Re-audited both files for anything missed: TestsOn the merged tree (
MutantsTwo of my own, applied to the merged tree and run against the new E2Es on the same server:
Always-rules
Left for the owner
Not verified
|
Collaborator
Review — CS-Macmini PR#437 — head 28c88a2Dom: APPROVE med nits Independent, read-only review of PR #437 against issue #428 and the #419 PR body's "Same-class leaks found but NOT fixed" table (the authoritative list of the 15 sites this PR is scoped to close). Evidence tags: [T] run locally, [A] analysis/code reading, [K] CI. Findings
No findings above informational/low — nothing here warrants REQUEST CHANGES. Issue #428 checklist — point by pointSource of truth: #419's sweep table listed exactly 15 leaking sites —
No scope creep: the diff touches only the 15 sites plus the 3 new E2E test files, two mechanical test-harness stub fixes ( TestsAll run on the merge of head
Mutants (2 of my own, independent of the author's)
Both mutants restored afterward; re-ran the clean suites to confirm restoration (all green again). Always-rules
Not verified
|
Owner
Author
Rapport — CS-Minimax PR#437 master-sync — head 2da5f06Review feedback addressed (commit Merged Review findings — "Review — CS-Macmini PR#437" (APPROVE with nits)
No blocking findings to re-fix; all three were nits/informational in the source review and none required a code change in Tests (post-merge, on
|
| Job | Result |
|---|---|
| ✅ Go Build & Test | success |
| 🧱 Validate Docker Image | success |
| 🎭 Playwright E2E Tests | failure — test-touch-gestures-coverage-e2e.js only, reproduced identically on both the initial run and one re-run |
| 🏗️ Build & Publish Docker Image | failure, gate only (Require tests and image validation, no further steps run) |
| 📦 Release / 🚀 Deploy Staging / 📝 Badges | skipped (PR) |
This failure is not caused by this PR's diff, and is not the #1692 timing bug the task expected to be the sole remaining redness. It is a sibling of the same root cause as #439/#1692, in a different, not-yet-patched file:
- [A]
#428's diff touches onlycompare.js,channels.js,live.js, two test-harness stub fixes, and 3 new lines indeploy.yml.test-touch-gestures-coverage-e2e.jsandpublic/packets.jsare untouched by this PR. - [K]
test-touch-gestures-coverage-e2e.jsfails identically both times:FAIL: no packets row available — fixture/setup problem, thenpage.waitForSelector: Timeout 10000ms exceededon#pktBody tr[data-hash], atopenRowOverlay/main— the same stack both runs. - [T] Reproduced locally in isolation: against the long-running local server from this session's own earlier test pass (fixture freshened ~1h earlier), the test fails with the exact same message. Against a freshly rebuilt fixture + freshly started server, it passes immediately (12/12). This isolates the cause to fixture-timestamp age vs. the packets page's default 15-minute view window — the identical mechanism
e5663475's commit message describes for#1692("the test timed out whenever the E2E suite reached it more than 15 minutes after the fixture was freshened"). - [A]
e5663475's fix only seeded a widerlocalStoragetime window insidetest-issue-1692-packets-init-parallel-e2e.jsitself. It did not changepackets.js's actual default window, and no other E2E file received the same seeding.test-touch-gestures-coverage-e2e.jshas no such seed and simply waits on the default-window row. - [A] This PR's 3 new
#428E2E files run immediately beforetest-touch-gestures-coverage-e2e.jsindeploy.yml's fail-fast order (lines 451-453 vs. 466) and add roughly 20-30s of cumulative runtime that wasn't there pre-merge, which is enough to tip an already-thin margin against the 15-minute fixture-freshness window later in the same sequential run. - Re-ran the CI run once (
gh run rerun --failed) to rule out ordinary non-determinism; it failed identically both times at the same step, which argues this is a deterministic timing threshold rather than one-off flakiness, so a further re-run would not be expected to help. This test is not on the known-flaky list (#271,#301). Build & Publish Docker Imagefails only on its gate (E2E_RESULT: failure), same conditional behavior as the prior report.
Always-rules
- [T] Commit author on the merge:
dborup <kontakt@meshview.dk>. - [T] No closing keywords used anywhere in this comment or in the merge commit message.
- [T]
git ls-remoteon the branch before writing this report:2da5f06b, matching what was pushed — no concurrent push. - Ordinary merge commit pushed, no rebase/amend/force-push. PR left as draft.
Rester (left for the owner)
- CI stays red on
Playwright E2E Tests/Build & Publish Docker Image's gate, caused bytest-touch-gestures-coverage-e2e.js's own fixture-freshness fragility (the same class as#439, a different file), not by anything in this PR's diff. Recommend tracking it as its own issue and applying the same remedy#439used (seed a wider time window via an init script beforepackets.jsevaluates) totest-touch-gestures-coverage-e2e.js, or moving it earlier indeploy.yml's fail-fast order. Out of scope here per "no other changes." - Finding 1 (rapid-race case doesn't independently kill the
_moreMenuDocClickHandlermutant) — left open with reasoning above; the other two live.js mutant-killing cases already cover it solidly. - Findings 2 and 3 from the source review are informational/confirmed-correct and need no action.
Not verified: a real multi-hour browsing session (leak-freedom still checked by net add/remove counting, not heap measurement); the orientationchange handler on real mobile hardware; no staging/production access, no API key used, no writes to the repository beyond this comment and the merge commit already pushed.
# Conflicts: # .github/workflows/deploy.yml
Owner
Author
Rapport — CS-Minimax PR#437 runde-3 — head c9e453eReview feedback addressed (commit Evidence tags: [T] run locally, [A] analysis/code reading, [K] CI. 1. CI failure on
|
| Job | Result |
|---|---|
| ✅ Go Build & Test | success (26m29s) |
| 🧱 Validate Docker Image | success (51s) |
| 🎭 Playwright E2E Tests (1/3) | success (10m31s) |
| 🎭 Playwright E2E Tests (2/3) | success (11m2s) — contains test-touch-gestures-coverage-e2e.js |
| 🎭 Playwright E2E Tests (3/3) | success (11m0s) — contains the 3 new #428 suites (shard 3) |
| 📊 Verify E2E Shards & Coverage | success (16s) |
| 🏗️ Build & Publish Docker Image | success (15s) |
| 📦 Release / 🚀 Deploy Staging / 📝 Badges | skipped (PR, as before) |
CI is fully green. Neither known-flaky test (#271, #301) was encountered; no re-run was necessary.
Always-rules
- [T] Commit author on the merge:
dborup <kontakt@meshview.dk>. - [T] No closing keywords used anywhere in this comment or in the merge commit message.
- [T]
git ls-remoteon the branch before writing this report:c9e453e8, matching what was pushed — no concurrent push. - Ordinary merge commit pushed, no rebase/amend/force-push. PR left as draft.
Rester (left for the owner)
- Shard 3's estimated total runtime is now ~520s vs shards 1/2's ~506s (~3% heavier) after adding the 3 new
#428suites there. Purely informational — not a CI gate, and well within the sharding's own tolerance — but worth a look if#436's balance is ever revisited by a different mechanism than manual placement. - All three nits/informational findings from "Review — CS-Macmini PR#437" remain open exactly as scoped in round 2 (see above) — no new information this round changes that assessment.
Not verified: a real multi-hour browsing session (leak-freedom still checked by net add/remove counting, not heap measurement); the orientationchange handler on real mobile hardware; no staging/production access, no API key used, no writes to the repository beyond this comment and the merge commit already pushed.
dborup
marked this pull request as ready for review
October 13, 2026 22:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #428
Remainder of #405: the #419 sweep found 15 page-module handlers bound to
the shared
#app/document/windowand never removed indestroy()—compare.js(1),channels.js(6),live.js(8), listed in #419'sdescription under "Same-class leaks found but NOT fixed". This PR fixes
all 15, following the same pattern #419 used for
observers.js.Fix
compare.js— the rowkeydownhandler (fix(store): charge the path_json fallback and pin the hash-migration accounting (#202) #209) is bound to.compare-page(its own container) instead of#app.channels.js— theclickdelegation (ch-back/ch-close-node)is bound to
.ch-layoutinstead of#app. The other 5 (Add/Share modalEscape, themc-channels-show-encrypted-changedwindow event, and thesidebar-resize
mousemove/mouseup) are genuinely global — they'restored as module refs and removed in
destroy().live.js— all 8 are global (window'smc-tile-provider-changedand the DragManager resize clamp;
document's feed-resize drag, the VCR"more" overflow-menu outside-click, and the timeline-scrub drag), stored
and removed the same way.
orientationchangewas a ref-mismatch bug on top of the leak:destroy()calledwindow.removeEventListener('orientationchange', _onResize), but the listener actually bound there was a different,anonymous arrow function — so it was never removed at all, even though
the code looked like it handled it.
Page behaviour (keyboard shortcuts, clicks, VCR, channel selection,
modals, sidebar/feed/timeline drag) is unchanged — each E2E below drives
the real DOM interaction, not just the listener count.
Live init/teardown races
Live's
init()isasyncand can still be resuming past anawaitafter the router has already switched pages. Traced every one of the 8
bind sites against the two
awaits ininit()'s main flow(
fetch('/api/config/map'),loadNodes()) and thegeneration !== liveGenerationguards immediately after each: every handler is bound ina block that runs synchronously once a guard has passed, so there's no
window where a stale
init()can bind a listener after its owndestroy()already ran. A dedicated E2E case (enter/leave Live 3xquickly, not waiting for it to settle) plus the existing
test-issue-125-live-toggles-early-e2e.js(which already specificallycovers "leaving Live while init awaits
loadNodes()") both stay green.Tests
Each page gets an E2E regression test (real browser + Go server), with a
mutant per page (reverting that file to its pre-#428
origin/masterversion). Listener counting uses the same "patch
addEventListenerandcount" technique #419's E2E used for
#app, extended todocument/windowwith a net (add minus remove) count filtered by thehandler's own function name — not by raw event type. That isolates
exactly the handler under test: other pages bind/unbind their own
keydown/etc. listeners too (confirmed while building this:nodes.js'sown Escape binds already drift independently across repeated visits),
so a type-only count would be contaminated by unrelated pages.
test-issue-428-compare-listener-leak-e2e.js#appgains 0keydownlisteners visiting Compare; row Enter still navigates; after Compare→Nodes a sentinel keydown on#appis inert; 3 visits don't stack.origin/mastercompare.js → 3/4 red (#appgains 1, sentinel navigates, 3 visits stack to 3).test-issue-428-channels-listener-leak-e2e.js#appgains 0click; all 5 global handlers net to exactly 1 while mounted and back to 0 after leaving / after 3 visits; after leaving, ach-backclick on#appis inert; behaviour: Add/Share modal Escape still closes each, sidebar drag still resizes + persists, the show-encrypted event still triggers a reload,ch-backdelegation still navigates.origin/masterchannels.js → 4/10 red (#appclick leak, both net-while-mounted and net-stacking cases, the inert-click case).test-issue-428-live-listener-leak-e2e.js_onOrientationChangespecifically checked; rapid 3x enter/leave doesn't leak or throw; a normal visit right after rapid cycling still mounts cleanly (WS beacon present).origin/masterlive.js → red (handlers never bound under their #428 names, so "net while mounted" and the post-race "still binds" checks fail — e.g._onOrientationChange net while mounted: 0).Two pre-existing test harnesses needed updating for the container
rebinds (not behaviour bugs — their fake DOM didn't have the new
querySelectorcall):test-frontend-helpers.js: the fake#app.querySelector('.ch-layout')stub lacked
addEventListener; merged it into the branch that alreadyreturns a full element stub.
test-xss-escape-sinks.js: same gap for#app.querySelector('.compare-page')in its
loadComparePage()harness.Verification
Against a local Go server with
test-fixtures/e2e-fixture.dbprepared asin CI (freshen → migrate → seed Kpa-clawbot#2073/#199/#245/Kpa-clawbot#1486/Kpa-clawbot#1791), port 13950,
-public public:shown red against its file's
origin/mastermutant.sh test-all.sh→ 242/242 (registry meta-test green: unit tests intest-all.sh, the 3 new E2E indeploy.yml).node test-frontend-helpers.js→ 713/0.cmd/servergo test ./...→ ok (no server change; read-only invariantintact).
green):
test-channels-ws-batch-e2e,test-channels-ws-race-1498-e2e,test-channels-client-state-152-e2e/-decrypt-e2e,test-channels-selection-flow-e2e,test-channels-add-modal-e2e,test-channels-share-color-e2e,test-channel-modal-e2e,test-channel-color-picker-e2e,test-channels-list-render-e2e,test-channel-issue-1111-e2e,test-channels-154-155-e2e,test-channel-proposals-e2e,test-channel-qr-e2e,test-channels-observed-path-hash-size-e2e,test-issue-1367-channels-chat-app-e2e,test-issue-1224-channels-mobile-ux-e2e,test-live-mql-leak-1180-e2e,test-live-fullscreen-1572-e2e,test-issue-1205-live-controls-anchor-e2e,test-issue-1206-resize-observer-leak-e2e,test-live-layout-1178-1179-e2e,test-issue-1510-live-nav-pin-e2e,test-issue-1234-live-chrome-pass2-e2e,test-issue-1244-live-vcr-row-hints-e2e,test-issue-125-live-toggles-early-e2e(the existing Liveinit/teardown-race test),
test-audio-live-1297-e2e,test-issue-1136-live-region-e2e,test-issue-220-live-filter-readiness-e2e,test-pr-1490-live-map-gpu-animations-e2e,test-live-pulse-ring-weight-e2e,test-live-multibyte-only-e2e,test-issue-1640-compare-discovery-e2e,test-issue-1639-observers-sort-e2e,test-issue-199-inactive-observer-e2e,test-xss-333-pages-e2e,test-node-liveness-e2e.scripts/check-xss-sinks.sh --diff origin/master→ exit 0.deploy.yml, 1 inrelease-fast-path.yml.map[string]interface{}outside tests; no hardcoded colours;cmd/serverread-only (diff touches onlypublic/*.js, 3 new testfiles, 2 test-harness fixes, and the 3 new
deploy.ymlE2E lines).Left / not done
#405is the parent issue; this PR closes out the remainder fix(ui): stop observers.js leaking delegated handlers on #app (#405) #419 leftopen under "Same-class leaks found but NOT fixed" — Page modules leak delegated handlers on #app/document (compare, channels, live) — remainder of #405 #428 itself should
be closed by the owner after review, not by this PR.
🤖 Generated with Claude Code