Fix BC-10091759496: stop leaking event listeners across Stimulus connect/disconnect

[jeremy]

Summary

Fixes BC-10091759496 — typing in a card editor progressively freezes Safari (worst in "Add to Dock" PWA mode), getting worse the longer the session runs.

Problem

drag_and_strum_controller attaches a document-level keydown listener on connect(). Under Turbo Drive, every board visit connects the controller again — and disconnect() never actually removes the previous listener. They accumulate for the life of the tab. Each leaked listener fires on any Shift-modified keystroke (capitals, punctuation while typing) and rebuilds 6 Audio objects, so per-keystroke cost grows with every board you've visited. Long-lived Safari/PWA sessions (which never full-reload) accumulate unbounded listeners → the reported progressive freeze.

Solution

Store the bound handler once in connect() and remove that same reference in disconnect().

// connect()
this.boundHandleKeyDown = this.handleKeyDown.bind(this)
document.addEventListener("keydown", this.boundHandleKeyDown)
// disconnect()
document.removeEventListener("keydown", this.boundHandleKeyDown)

Root Cause

disconnect() called document.removeEventListener("keydown", this.handleKeyDown.bind(this)). Function.prototype.bind() returns a new function object every call, so the reference passed to removeEventListener never equalled the one addEventListener registered — the removal was a silent no-op.

Origin

Introduced in 88ef14c "Preload various instruments" (Andy Smith, 2025-09-26) — the connect/disconnect pair used a fresh .bind(this) on both sides from the start. Not a regression; born broken.

Verification (real Safari engine — no regression test)

Reproduced in WebKit (Playwright's WebKit 26.5 = Safari's engine) by driving the real Stimulus connect/disconnect lifecycle across 10 simulated board visits and counting live document keydown listeners by function identity:

board visit #:       1  2  3  4  5  6  7  8  9  10
BEFORE (current):    1  2  3  4  5  6  7  8  9  10   <- leaked listeners
AFTER  (this fix):   0  0  0  0  0  0  0  0  0  0

Current code leaks exactly one listener per visit (matching "gets worse the longer the session runs"); with this fix the count stays flat. No unit test added — this is a one-time lifecycle correctness fix with no realistic regression path, and a listener-count test would be permanent scar tissue.

Related / not covered here

• Cross-links fizzy#2926, which independently profiled this controller's new Audio() storm during typing but did not diagnose the listener leak. Typing in comments freezes Safari: global keydown in drag_and_strum + heavy compositing #2926 also flags a separate GPU-compositing cost during editor mutation that this fix does not address — worth a follow-up.
• A handleKeyDown guard (early-return when focus is in a typing context, or only preload during an active drag) would further cut per-fire cost, but is a behavior change and is left out of this focused fix.

Bug Card

https://app.basecamp.com/2914079/buckets/27/card_tables/cards/10091759496

[Copilot]

Pull request overview

Fixes a Stimulus lifecycle bug in drag_and_strum_controller where a document-level keydown listener was accumulating across Turbo-driven board navigations, causing progressively worse typing performance (notably in Safari/PWA sessions).

Changes:

• Store a single bound handleKeyDown function reference on connect().
• Remove the exact same function reference on disconnect() to prevent leaked listeners.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jeremy]

🤖 Broadened to fix the whole defect class in one PR (per our fix-comprehensively practice — the diagnosis found three instances of the same idiom).

Second commit applies the identical fix to two more controllers that use the same broken removeEventListener(fn.bind(this)) idiom:

• tooltip_controller.js (mouseenter/mouseout)
• dialog_manager_controller.js (dialog:show)

These bind to this.element rather than document, so they don't leak unbounded like the drag_and_strum one (the listener dies when the element is GC'd) — but they stack duplicate listeners across a disconnect/reconnect of the same element (cached Turbo previews, morphing), firing the handler N times per event. The document-scoped drag_and_strum leak (first commit) is the one that causes the reported Safari/PWA freeze; these two are same-pattern hygiene.

Captured as a reusable pattern (event-listener-removal-identity-mismatch) so /coworker-hunt can sweep other apps.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.