Skip to content

Perf/hotkey recorder optimizations #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ryzmae wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Perf/hotkey recorder optimizations #48

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b666cf
refactor: Remove state reset in HotkeyRecorder destroy method
ryzmae Mar 1, 2026
81415ae
refactor: Use IDLE_STATE for initial store state in HotkeyRecorder
ryzmae Mar 1, 2026
c7a5b83
chore: add changeset
ryzmae Mar 1, 2026
4d1f315
refactor: update store state to set isRecording to true in HotkeyReco…
ryzmae Mar 1, 2026
48da959
refactor: update store state to set recordedHotkey in HotkeyRecorder
ryzmae Mar 1, 2026
bfd855f
ci: apply automated fixes
autofix-ci[bot] Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-snails-retire.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@tanstack/hotkeys': patch
---

Remove redundant `setState` in `destroy()` and share a single `IDLE_STATE` constant across `stop()` and `cancel()`
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
34 changes: 10 additions & 24 deletions packages/hotkeys/src/recorder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ export interface HotkeyRecorderState {
recordedHotkey: Hotkey | null
}

/**
* Initial idle state for the recorder, used when not recording.
*/
const IDLE_STATE: HotkeyRecorderState = { isRecording: false, recordedHotkey: null }

/**
* Options for configuring a HotkeyRecorder instance.
*/
Expand Down Expand Up @@ -74,10 +79,7 @@ export class HotkeyRecorder {
* The TanStack Store instance containing the recorder state.
* Use this to subscribe to state changes or access current state.
*/
readonly store: Store<HotkeyRecorderState> = new Store<HotkeyRecorderState>({
isRecording: false,
recordedHotkey: null,
})
readonly store: Store<HotkeyRecorderState> = new Store<HotkeyRecorderState>(IDLE_STATE)

#keydownHandler: ((event: KeyboardEvent) => void) | null = null
#options: HotkeyRecorderOptions
Expand Down Expand Up @@ -113,10 +115,7 @@ export class HotkeyRecorder {
}

// Update store state
this.store.setState(() => ({
isRecording: true,
recordedHotkey: null,
}))
this.store.setState(() => (IDLE_STATE))
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// Create keydown handler
const handler = (event: KeyboardEvent) => {
Expand Down Expand Up @@ -172,10 +171,7 @@ export class HotkeyRecorder {
}

// 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)
Comment on lines 181 to 188
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Expand All @@ -199,10 +195,7 @@ export class HotkeyRecorder {
}

// Update store state
this.store.setState(() => ({
isRecording: false,
recordedHotkey: null,
}))
this.store.setState(() => (IDLE_STATE))
}

/**
Expand All @@ -219,10 +212,7 @@ export class HotkeyRecorder {
}

// Update store state
this.store.setState(() => ({
isRecording: false,
recordedHotkey: null,
}))
this.store.setState(() => (IDLE_STATE))

// Call cancel callback
this.#options.onCancel?.()
Expand Down Expand Up @@ -258,9 +248,5 @@ export class HotkeyRecorder {
*/
destroy(): void {
this.stop()
this.store.setState(() => ({
isRecording: false,
recordedHotkey: null,
}))
}
}
Loading