Skip to content

Fix comment-typing freeze: scope drag-strum keydown + fix listener leak (#2926)#2929

Open
vitalibondar wants to merge 2 commits into
basecamp:mainfrom
vitalibondar:fix-strum-keydown-typing
Open

Fix comment-typing freeze: scope drag-strum keydown + fix listener leak (#2926)#2929
vitalibondar wants to merge 2 commits into
basecamp:mainfrom
vitalibondar:fix-strum-keydown-typing

Conversation

@vitalibondar
Copy link
Copy Markdown

Fixes #2926 — typing in a card's comment box freezes Safari (macOS + iOS) for 5–10s, worsening over a session.

drag_and_strum_controller adds a global keydown listener to play an instrument while a card is dragged between columns. Two problems make typing janky:

1. Listener leak (bind reference mismatch)

connect()    { document.addEventListener("keydown", this.handleKeyDown.bind(this)) }
disconnect() { document.removeEventListener("keydown", this.handleKeyDown.bind(this)) }

.bind(this) returns a new function each time, so removeEventListener never matches the listener added in connect. Every Turbo reconnect leaks another permanent global keydown listener; they accumulate over a session and all fire on each keystroke. This matches the reported "~15 keydown per typed character, worsens during a session".

Fix: bind once, store the reference, reuse it for add/remove.

2. Handler runs while typing

The handler fired on every document keydown, including inside the comment editor. Typing capital letters / shifted keys (event.shiftKey) repeatedly hit #preloadAudioFiles, constructing new Audio() in a loop on each keystroke. The strum is only meaningful during a drag, so keydown originating from a text field / contenteditable / the lexxy-editor is now ignored via a small #isEditingText guard (isContentEditable + closest("input, textarea, select, lexxy-editor")).

Notes

🤖 Generated with Claude Code

The drag-and-strum controller adds a global keydown listener to play an
instrument while dragging cards. Two issues made typing in the comment
editor janky (reported on Safari, basecamp#2926):

1. Listener leak: connect added the handler via handleKeyDown.bind(this)
   and disconnect tried to remove it with a *different* .bind(this)
   reference, so the listener was never removed. Every Turbo reconnect
   added another permanent global keydown listener — they accumulate over
   a session and all fire on every keystroke. Bind once and reuse the
   reference so removeEventListener works.

2. Fires while typing: the handler ran on every document keydown, including
   inside the comment editor. Holding Shift (capital letters) repeatedly
   hit #preloadAudioFiles, constructing new Audio() in a loop on each
   keystroke. Ignore keydown originating from text inputs / contenteditable
   / the lexxy editor — the strum is only meaningful during a drag.

Fixes basecamp#2926

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 7, 2026 23:46
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Improves the controller’s global keyboard handling by making event listener cleanup reliable and preventing shortcuts from firing while the user is editing text.

Changes:

  • Store a bound handleKeyDown reference to ensure removeEventListener unregisters correctly.
  • Skip key handling when the event target is an editable/text input element.
  • Add a private #isEditingText helper to centralize editability detection.

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

Comment on lines +72 to +77
#isEditingText(element) {
if (!element) { return false }
if (element.isContentEditable) { return true }

return element.closest?.("input, textarea, select, lexxy-editor") != null
}
}

handleKeyDown(event) {
if (this.#isEditingText(event.target)) { return }
Address review feedback on #isEditingText:
- Only treat text-entry inputs as editing (skip checkbox/radio/range/file/
  button/etc.) so the strum isn't suppressed when a non-text control has
  focus during a drag.
- Fall back to document.activeElement when the event has no target.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vitalibondar
Copy link
Copy Markdown
Author

Thanks — both points were fair, addressed in the latest commit:

  1. Non-text inputs#isEditingText no longer treats every <input> as text entry. It now skips button/submit/reset/checkbox/radio/file/image/range/color (via input.type), so the strum is only suppressed for actual text-entry fields, textarea, select, contenteditable, and lexxy-editor.
  2. Target vs focus — added a document.activeElement fallback when the event has no target. The comment editor (lexxy-editor) is matched by tag, which also covers the Shadow DOM retargeting case (keydown/activeElement retarget to the host element), so the reported freeze stays fixed.

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.

Typing in comments freezes Safari: global keydown in drag_and_strum + heavy compositing

2 participants