Conversation
Because it already does this in the stop function itself
There was a problem hiding this comment.
Pull request overview
This PR aims to optimize the HotkeyRecorder state handling by de-duplicating “idle” state initialization/resets and removing a redundant setState call in destroy(), with a patch changeset for release notes.
Changes:
- Introduces a shared
IDLE_STATEconstant and uses it for the store’s initial state and several state resets. - Removes the extra
store.setStatecall fromdestroy()(now relies onstop()). - Adds a patch changeset describing the optimization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/hotkeys/src/recorder.ts | Refactors recorder state initialization/resets to use IDLE_STATE and removes redundant reset in destroy() |
| .changeset/wild-snails-retire.md | Adds release note entry for the optimization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '@tanstack/hotkeys': patch | ||
| --- | ||
|
|
||
| Remove redundant `setState` in `destroy()` and share a single `IDLE_STATE` constant across `stop()` and `cancel()` |
There was a problem hiding this comment.
The changeset message says the shared IDLE_STATE is used across stop() and cancel(), but the code also uses it in start() and on successful record, and destroy() no longer directly sets state either. Please update the changeset text to accurately describe the shipped behavior so release notes are correct.
packages/hotkeys/src/recorder.ts
Outdated
| // Update store state | ||
| this.store.setState(() => ({ | ||
| isRecording: true, | ||
| recordedHotkey: null, | ||
| })) | ||
| this.store.setState(() => (IDLE_STATE)) |
There was a problem hiding this comment.
This start() state transition is behavior that’s easy to regress (as in this change) and currently appears untested in this package. Consider adding a focused unit test for HotkeyRecorder that asserts isRecording becomes true on start(), resets on stop()/cancel()/destroy(), and that recordedHotkey is set when a valid hotkey is recorded.
packages/hotkeys/src/recorder.ts
Outdated
| // Update store state | ||
| this.store.setState(() => ({ | ||
| isRecording: true, | ||
| recordedHotkey: null, | ||
| })) | ||
| this.store.setState(() => (IDLE_STATE)) |
There was a problem hiding this comment.
In start(), the recorder state is being set to IDLE_STATE (where isRecording is false). This makes recorder.store.state.isRecording remain false even after start() and contradicts the class docs/example (and the previous behavior), likely breaking UI that relies on isRecording to show recording mode. Update start() to set isRecording: true (while clearing recordedHotkey to null).
| // Update store state immediately | ||
| this.store.setState(() => ({ | ||
| isRecording: false, | ||
| recordedHotkey: finalHotkey, | ||
| })) | ||
| this.store.setState(() => (IDLE_STATE)) | ||
|
|
||
| // Call callback AFTER listener is removed and state is set | ||
| this.#options.onRecord(finalHotkey) |
There was a problem hiding this comment.
When a valid hotkey is recorded, the code now sets state to IDLE_STATE, which clears recordedHotkey to null. This means recordedHotkey is never set to the captured finalHotkey anywhere in this file, so consumers subscribing to the store will not be able to read the recorded value from state (regression from previous behavior). Set state to isRecording: false and recordedHotkey: finalHotkey at this point.
packages/hotkeys/src/recorder.ts
Outdated
| // Update store state | ||
| this.store.setState(() => ({ | ||
| isRecording: true, | ||
| recordedHotkey: null, | ||
| })) | ||
| this.store.setState(() => (IDLE_STATE)) |
There was a problem hiding this comment.
The new IDLE_STATE constant is used in start() and when a hotkey is successfully recorded, not just in stop(), cancel(), and destroy() as described in the PR description. Either narrow the usage to match the description (idle reset only) or update the PR description to reflect the actual behavioral changes, especially since using IDLE_STATE in start() changes isRecording semantics.
ryzmae
left a comment
ryzmae
left a comment
There was a problem hiding this comment.
ive reverted the start functions back (my fault)
🎯 Changes
Remove redundant setState in destroy() and share a single IDLE_STATE constant across stop(), cancel(), and destroy().
✅ Checklist
pnpm run test:pr.🚀 Release Impact
NOTE:
I am 16 and I randomly found this while browsing through the codebase so please be helpful if I did something wrong.