Swarm Fix: [bug][alpha] Which Key (WhichKey): keydown with repeat === true extends the pending chord and can cancel the sequence#37721
Conversation
…eat === true` extends the pending chord and can cancel the sequence Signed-off-by: hinzwilliam52-ship-it <hinzwilliam52@gmail.com>
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@FIX_PROPOSAL.md`:
- Line 15: Add a test suite exercising the key-chord handling to verify repeated
keydown events are ignored and the sequence remains open for the next deliberate
press: write tests targeting the KeyChordHandler (or SequenceManager) and its
handleKeyDown behavior to simulate repeated keydown with event.repeat=true and
assert the sequence does not append repeats, simulate legitimate multi-key
chords to assert chords still form, and include edge-case tests (rapid repeated
presses, modifier keys, and keyup without prior keydown) to ensure no
regressions; use the public API/tests helpers that call
handleKeyDown/handleKeyUp (or processKeyEvent) and assert internal
sequence/state and emitted chord events.
- Around line 5-13: Move the raw window.addEventListener usage in
WhichKeyContext.tsx into a useEffect hook that registers a named handleKeydown
function and removes it in the cleanup return; inside handleKeydown check
event.repeat early to ignore auto-repeats, then compute the keystroke sequence
(rename the variable newPrefix to newSequence or keystrokeSequence to reflect
its purpose) and perform the existing sequence logic; ensure useEffect
dependency array includes any state/props used (e.g., currentSequence) and
replace the `// ...` placeholders with the full handler logic so the handler
works correctly in context.
| ```typescript | ||
| // In WhichKeyContext.tsx | ||
| window.addEventListener('keydown', (event) => { | ||
| // ... | ||
| if (event.repeat) return; // Add this line to ignore repeat events | ||
| const newPrefix = [...currentSequence.keystrokes, keystroke]; | ||
| // ... | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MDN KeyboardEvent repeat property documentation and browser support
💡 Result:
The MDN documentation for the KeyboardEvent.repeat property states that it is a read-only boolean property of the KeyboardEvent interface. It returns true if the given key is being held down such that it is automatically repeating. Syntax: var repeat = event.repeat; Value: A boolean value. Specification: Defined in UI Events # dom-keyboardevent-repeat. Browser support (as of 2026, based on CanIUse data): - Chrome: Full support since version 32 (partial before), full in recent versions (137+). - Firefox: Full support since 28. - Safari: Full support since 10.1 (desktop), 10.3 (iOS). - Edge: Full support since 79 (and IE 9-11 supported). - Opera: Full support since recent versions. - Mobile browsers generally follow desktop counterparts, with full support in recent versions. The feature has broad support across modern browsers since around 2015-2017, with no support in very old versions (e.g., Chrome <32, Firefox <28). MDN notes it as baseline compatible since 2025 for latest browsers.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat
- 2: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
- 3: https://caniuse.com/mdn-api_keyboardevent_repeat
The proposed code snippet has several concerns.
- Event listener management: Using
window.addEventListenerdirectly in a React component typically requires proper setup inuseEffectwith cleanup to prevent memory leaks:
Recommended pattern
useEffect(() => {
const handleKeydown = (event: KeyboardEvent) => {
if (event.repeat) return;
// ... rest of handler
};
window.addEventListener('keydown', handleKeydown);
return () => window.removeEventListener('keydown', handleKeydown);
}, [/* dependencies */]);-
Misleading variable name:
newPrefixon line 10 represents a keystroke sequence, not a prefix, which is confusing terminology. -
Incomplete snippet: The
// ...placeholders make it difficult to verify the fix works correctly in context.
Note: event.repeat is a valid and well-supported property for detecting auto-repeated key presses across all modern browsers (Chrome 32+, Firefox 28+, Safari 10.1+, Edge 79+).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FIX_PROPOSAL.md` around lines 5 - 13, Move the raw window.addEventListener
usage in WhichKeyContext.tsx into a useEffect hook that registers a named
handleKeydown function and removes it in the cleanup return; inside
handleKeydown check event.repeat early to ignore auto-repeats, then compute the
keystroke sequence (rename the variable newPrefix to newSequence or
keystrokeSequence to reflect its purpose) and perform the existing sequence
logic; ensure useEffect dependency array includes any state/props used (e.g.,
currentSequence) and replace the `// ...` placeholders with the full handler
logic so the handler works correctly in context.
| }); | ||
| ``` | ||
|
|
||
| This will prevent the repeat events from appending to the sequence and canceling the chord. Instead, the sequence will remain open for the next deliberate key press. No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add tests to verify the expected behavior.
The proposal describes the expected behavior but doesn't include any tests to verify that:
- Repeated keydown events are properly ignored
- The sequence remains open for the next deliberate key press
- The fix doesn't prevent legitimate multi-key chords from working
- Edge cases are handled correctly
Would you like me to generate a test suite that covers these scenarios, or open an issue to track adding comprehensive tests for this fix?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FIX_PROPOSAL.md` at line 15, Add a test suite exercising the key-chord
handling to verify repeated keydown events are ignored and the sequence remains
open for the next deliberate press: write tests targeting the KeyChordHandler
(or SequenceManager) and its handleKeyDown behavior to simulate repeated keydown
with event.repeat=true and assert the sequence does not append repeats, simulate
legitimate multi-key chords to assert chords still form, and include edge-case
tests (rapid repeated presses, modifier keys, and keyup without prior keydown)
to ensure no regressions; use the public API/tests helpers that call
handleKeyDown/handleKeyUp (or processKeyEvent) and assert internal
sequence/state and emitted chord events.
1 participant
Description
Brief description of the changes in this PR.
Related Issue
Fixes #(issue number)
Type of Change
Checklist
Testing
Describe the tests you ran to verify your changes.
Screenshots (if applicable)
Add screenshots to help explain your changes.
Summary by CodeRabbit