fix(react-hotkeys): set hotkey options after ui commit #113#141
fix(react-hotkeys): set hotkey options after ui commit #113#141IanWorley wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesHotkey options timing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-hotkeys/src/useHotkey.ts`:
- Around line 181-193: Move the callback assignment from the render path into
the existing post-commit useEffect alongside setOptions, so aborted renders
cannot mutate the active registration. In that effect, update the callback and
call registrationRef.current.setOptions only when optionsWithoutTarget differ
from the previously committed options, preserving subscriber stability when
options are unchanged.
In `@packages/react-hotkeys/tests/useHotkey.test.tsx`:
- Around line 260-285: Clean up the listener created by
manager.registrations.subscribe in the “should notify store subscribers after
commit, not during render” test. Capture the returned unsubscribe function and
invoke it after the assertion so the timeline closure cannot receive store
updates from subsequent tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4401acc1-20af-4245-a03c-e121826016e2
📒 Files selected for processing (3)
.changeset/tangy-clubs-divide.mdpackages/react-hotkeys/src/useHotkey.tspackages/react-hotkeys/tests/useHotkey.test.tsx
| // Sync callback on EVERY render (outside useEffect) | ||
| // This avoids stale closures - the callback always has access to latest state | ||
| if (registrationRef.current?.isActive) { | ||
| registrationRef.current.callback = callback | ||
| registrationRef.current.setOptions(optionsWithoutTarget) | ||
| } | ||
|
|
||
| // Sync options after the UI commits so store subscribers aren't | ||
| // notified during the render phase | ||
| useEffect(() => { | ||
| if (registrationRef.current?.isActive) { | ||
| registrationRef.current.setOptions(optionsWithoutTarget) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if HotkeyRegistrationHandle.setOptions handles deep/shallow equality internally.
ast-grep run --pattern $'class HotkeyRegistrationHandle {
$$$
setOptions($_) {
$$$
}
$$$
}' --lang typescript packages/hotkeys/src/hotkey-manager.tsRepository: TanStack/hotkeys
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and locate the registration/update logic.
git ls-files 'packages/react-hotkeys/src/*' 'packages/hotkeys/src/*' | sed 's#^`#FILE` #'
printf '\n--- setOptions references ---\n'
rg -n "setOptions|callback = callback|isActive|useEffect|useLayoutEffect" packages/react-hotkeys/src packages/hotkeys/src
printf '\n--- outline: useHotkey.ts ---\n'
ast-grep outline packages/react-hotkeys/src/useHotkey.ts --view expanded
printf '\n--- outline: hotkey-manager.ts ---\n'
ast-grep outline packages/hotkeys/src/hotkey-manager.ts --view expandedRepository: TanStack/hotkeys
Length of output: 10133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- packages/hotkeys/src/hotkey-manager.ts (setOptions region) ---\n'
sed -n '360,420p' packages/hotkeys/src/hotkey-manager.ts
printf '\n--- packages/react-hotkeys/src/useHotkey.ts (relevant region) ---\n'
sed -n '150,210p' packages/react-hotkeys/src/useHotkey.ts
printf '\n--- packages/react-hotkeys/src/useHotkeySequence.ts (similar pattern) ---\n'
sed -n '160,190p' packages/react-hotkeys/src/useHotkeySequence.ts
printf '\n--- packages/react-hotkeys/src/useHotkeys.ts (similar pattern) ---\n'
sed -n '170,210p' packages/react-hotkeys/src/useHotkeys.tsRepository: TanStack/hotkeys
Length of output: 5254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Store implementation ---\n'
rg -n "class Store|setState\\(|state =" packages/hotkeys/src packages/react-hotkeys/src
printf '\n--- likely store file(s) ---\n'
fd -a "store" packages/hotkeys/src packages/react-hotkeys/src || true
fd -a "state" packages/hotkeys/src packages/react-hotkeys/src || true
printf '\n--- hotkey-manager around registrations store setup ---\n'
sed -n '200,260p' packages/hotkeys/src/hotkey-manager.tsRepository: TanStack/hotkeys
Length of output: 4277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- Store class definition ---\n'
sed -n '1,220p' packages/hotkeys/src/key-state-tracker.ts
printf '\n--- hotkey-manager registration handle section ---\n'
sed -n '330,395p' packages/hotkeys/src/hotkey-manager.tsRepository: TanStack/hotkeys
Length of output: 8174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/react-hotkeys/src/useHotkey.tsRepository: TanStack/hotkeys
Length of output: 6244
Move the callback sync into the commit phase. registrationRef.current.callback = callback mutates the hotkey manager during render, which can leak an aborted concurrent render into the active registration. The useEffect here also runs after every render, and setOptions always publishes a new registration map, so this still churns subscribers even when the options are unchanged. Fold both updates into the post-commit sync and skip setOptions when nothing actually changed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-hotkeys/src/useHotkey.ts` around lines 181 - 193, Move the
callback assignment from the render path into the existing post-commit useEffect
alongside setOptions, so aborted renders cannot mutate the active registration.
In that effect, update the callback and call registrationRef.current.setOptions
only when optionsWithoutTarget differ from the previously committed options,
preserving subscriber stability when options are unchanged.
db8c905 to
371a1ae
Compare
1 participant
🎯 Changes
This PR prevent UseHotkey from emitting store state changes in mid render. Previously on every rerender the Hotkey Register options were set with SetOptions during the render body. The problem is that store subscribers like useHotkeyRegistrations would receive store update mid render. This would causes yet another rerender while react was rending already. By applying useEffect to setOptions we now wait till the component using UseHotkey has committed its ui changes before triggering a store update.
Issue:
#113
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit