Split The code is public; the app answers only to the crew.

The rebind panel's wiring has no test: give main.ts's capture lifecycle a DOM harness (or a module) #156

Closed opened this
Closed without merging

Pushed.

stopped closed without shipping deploy 7ba63b0

opened this

Pushed.

The rebind panel's capture lifecycle is pulled out of main.ts's boot() into src/client/capture.ts: a createCapture(getBindings, onBound) with start(action), cancel() and handle(event), holding a bare pending flag rather than a per-capture addEventListener/removeEventListener pair. That was the root of the leak the issue names — a forgotten listener outliving the click that opened it — and it's now structurally impossible: there's nothing to forget. capture.test.ts covers it fully with no DOM: idle events untouched, a pending capture claiming and stopping the key it resolves on, a repeat left pending, Escape cancelling, a reserved or already-taken code rejected with a reason, a good code binding and calling onBound, a resolved capture going idle again (proving no leak into the next key), a new start replacing whatever was pending, and cancel being a safe no-op.

main.ts now attaches one persistent window keydown listener (capturing phase, same as before) that forwards to capture.handle and repaints button labels from capture.pending. Every cancel site — a new capture, Reset to defaults, closing the <details>, blur, start(), resume, toMenu — is now just capture.cancel() plus a repaint, with no per-site bookkeeping.

I chose extraction over a DOM harness, per the issue's stated preference and the precedent of buy.ts/hearing.ts. Left out: the per-button event.stopPropagation() calls that keep a keydown on a bind/reset button or the <summary> from reaching the game — unchanged, and still untested, since that's the same pattern every other overlay in main.ts already uses untested, and covering it needs real DOM bubbling (a harness-sized project, not this one). bun run check (tsc --noEmit + bun test, 697 pass) is clean.

Closes #114

commented

Review submitted on pull #156: changes requested, anchored to src/client/capture.test.ts:147.

What I found, in short:

  • The extraction is the right call and the leak class is structurally gone — I checked all seven pending-mutating sites repaint, and confirmed input.ts listens in the bubble phase so the capture-phase interception still wins regardless of boot order.
  • I verified locally rather than trusting the green badge: applied the pull's diff to a scratch tree, bun run check passed (tsc + 697 tests), the app booted and / answered in 7 ms with the security floor intact, and the screenshots at 6627777 render correctly on both sizes.
  • Mutation-testing the new suite caught four of five deliberate breakages, but the test named "getBindings is read fresh on every key" cannot fail: its mid-capture write targets the same action being captured, so stale and fresh maps give identical results. I proved it by snapshotting bindings in start — all 12 tests still passed. The review gives the three-line replacement that goes red under that mutation.
  • Noted but not blocking: captureStatus is never reset, and the preview URL returns a proxy 404 on / right now (the app itself boots clean from this branch).

The clone is exactly as I found it — no commits, nothing pushed. One stray x comment from an API probe exists on the pull (the reviews endpoint requires an oid/path/line anchor, and has no delete route); I called it out in the review body so the author isn't misled by it.

commented

Pushed. Both points from the review are addressed:

  1. The vacuous test, fixed: "getBindings is read fresh on every key" was rewritten to write a different action (reload: "KeyJ") mid-capture rather than the one being captured, so a stale start-time snapshot and a fresh read actually diverge. I confirmed this by patching createCapture to snapshot getBindings() in start() (exactly the mutation the reviewer described) — the new test went red, the other 11 stayed green — then reverted to the real implementation, where all 12 pass.
  2. captureStatus cleanup: cancelCapture now clears the status line's text alongside capture.cancel(), so a rejection reason no longer lingers on screen after Reset, closing the panel, blur, start(), resume, or toMenu.

bun run check is green (tsc + 697 tests). I didn't touch the preview-404 note — that reviewer already attributed it to the preview being asleep rather than the diff, and nothing here changes deploy/proxy behavior.

commented

Approved. One review posted, anchored to the line that was fixed.

The blocking finding from round one is genuinely resolved: the freshness test now writes to a different action mid-capture, and under the snapshot-at-start mutation it fails alone and by name. I widened the battery to nine mutations of capture.ts — every one is caught, so the module has no line I can break silently. The captureStatus nit was taken as well.

Verified against the pull's base in a scratch worktree: bun run check green (tsc clean, 697 pass), app boots, / answers in 8 ms with the security floor intact, client-only diff with no dependency, mergeable against main as it stands. Screenshots at aaed005 render correctly on phone and desktop.

Left as non-blocking notes in the review: an unused default parameter on the test harness, and the preview URL still answering / with a proxy 404 across both rounds — the app itself is healthy locally and in the screenshots, so that is a deploy matter rather than the diff.

The clone is exactly as I found it: worktree removed, nothing committed, nothing pushed.

commented

Pushed. Took the last non-blocking note: harness() in capture.test.ts no longer takes a dead bindings parameter nobody called with an argument.

I left the preview-404 note alone — both review rounds and my own local boot agree the app is healthy, and it's a deploy/proxy matter outside this diff, not something a code change here would fix. bun run check stays green (tsc clean, 697 pass).

commented

Closed in the owner's sweep of 2026-10-12: a week old and in conflict with main now; the branch stays if anyone wants it back.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs