Merged
Conversation
Contributor
🚀 Changeset Version Preview6 package(s) bumped directly, 0 bumped as dependents. 🟨 Minor bumps
|
@tanstack/angular-hotkeys
@tanstack/hotkeys
@tanstack/hotkeys-devtools
@tanstack/preact-hotkeys
@tanstack/preact-hotkeys-devtools
@tanstack/react-hotkeys
@tanstack/react-hotkeys-devtools
@tanstack/solid-hotkeys
@tanstack/solid-hotkeys-devtools
@tanstack/svelte-hotkeys
@tanstack/vue-hotkeys
@tanstack/vue-hotkeys-devtools
commit: |
📝 WalkthroughWalkthroughAdds plural hotkey-sequence registration APIs across frameworks (React/Preact/Vue/Solid/Svelte/Angular), core manager behavior changes for Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component / Hook
participant FrameworkEffect as Framework Effect / watcher
participant SequenceManager as SequenceManager (singleton)
participant Target as Target (document | element)
Component->>FrameworkEffect: provide definitions + commonOptions
FrameworkEffect->>FrameworkEffect: resolve getters/refs\nmerge options (provider < common < per-definition)
FrameworkEffect->>SequenceManager: diff registrations\nregister / update / unregister handles
SequenceManager->>Target: attach listeners to target
Target->>SequenceManager: key events
SequenceManager->>SequenceManager: detect sequence match\ninvoke callback
SequenceManager->>Component: callback runs (updates state)
FrameworkEffect->>SequenceManager: on update -> setOptions/update callback or re-register
Component->>FrameworkEffect: unmount/destroy
FrameworkEffect->>SequenceManager: unregister remaining handles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
docs/framework/preact/guides/sequences.md (1)
38-38: Optional: add acommonOptionsexample snippet.Since this line explains second-argument precedence, adding a tiny example with
useHotkeySequences(definitions, commonOptions)would make the behavior immediately actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/preact/guides/sequences.md` at line 38, Add a short example demonstrating the second-argument precedence for useHotkeySequences so readers can see how HotkeysProvider defaults, the second-argument commonOptions, and each definition’s options merge (e.g., show calling useHotkeySequences(definitions, commonOptions) with one shared option overridden by a per-definition options entry). Refer to useHotkeySequences, HotkeysProvider, commonOptions, definitions, and each definition’s options in the example so it clearly shows the precedence order in practice.examples/angular/injectHotkeySequences/src/app/app.component.html (1)
72-74: Announce triggered sequence updates to assistive tech.At Line 73, this status changes dynamically; adding
aria-live="polite"improves screen-reader feedback.♿ Suggested tweak
- <div class="info-box success"><strong>Triggered:</strong> {{ seq }}</div> + <div class="info-box success" aria-live="polite"> + <strong>Triggered:</strong> {{ seq }} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/angular/injectHotkeySequences/src/app/app.component.html` around lines 72 - 74, The dynamic "Triggered" status rendered via the lastSequence() template binding (as seq) should announce updates to assistive tech; add aria-live="polite" to the div with class "info-box success" that displays {{ seq }} so screen readers are notified of changes without interrupting. Target the template block using lastSequence() / seq and update that div to include the aria-live attribute.packages/angular-hotkeys/src/injectHotkeySequences.ts (1)
106-110: Inconsistentenabledhandling compared to other framework implementations.The Angular implementation skips registration when
enabled: falseat lines 106-110, and always passesenabled: truewhen registering (line 158). Other framework implementations (React, Vue, Solid, Preact, Svelte) pass the resolvedenabledvalue through tosetOptions()or the registration call, allowing the underlying manager to handle enable/disable state.This inconsistency means:
- Angular: Toggling
enabledcauses unregister/re-register cycles- Other frameworks: Toggling
enabledupdates the existing handle viasetOptions()Consider aligning with other frameworks by passing the
enabledvalue through to the handle:♻️ Suggested alignment with other frameworks
- const { enabled = true, ...sequenceOpts } = mergedOptions + const { enabled = true, target, ...restOptions } = mergedOptions - if (!enabled || resolvedSequence.length === 0) { + if (resolvedSequence.length === 0) { continue } + const resolvedTarget = + target ?? (typeof document !== 'undefined' ? document : null) + + if (!resolvedTarget) { + continue + } + + const sequenceOpts = { ...restOptions, enabled }And when registering:
const handle = manager.register(p.resolvedSequence, p.def.callback, { ...p.sequenceOpts, - enabled: true, target: p.resolvedTarget, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-hotkeys/src/injectHotkeySequences.ts` around lines 106 - 110, The Angular implementation currently destructures enabled from mergedOptions and skips registration when enabled is false (using const { enabled = true, ...sequenceOpts } = mergedOptions and continue), and later always registers with enabled: true; instead, preserve and pass the resolved enabled value through like other frameworks: do not early-continue on enabled === false (still register the sequence handle), and when calling the registration or setOptions helper (the code path that currently uses sequenceOpts/registration call), include enabled (the actual resolved enabled) rather than hardcoding true so the manager can toggle state via setOptions/update instead of unregistering/re-registering; adjust uses of mergedOptions, sequenceOpts, and the register/setOptions call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/angular/guides/sequences.md`:
- Around line 28-48: The example is missing the import for the Angular decorator
used: add an import for Component from '@angular/core' at the top of the snippet
so the `@Component` decorator on AppComponent resolves; keep the existing import
of injectHotkeySequences from '@tanstack/angular-hotkeys' and the AppComponent
class/constructor unchanged.
In `@examples/angular/injectHotkeySequences/src/app/app.component.ts`:
- Line 12: The history property uses the bracket-array form which violates the
lint rule; change its type to the generic form by replacing any occurrences of
string[] with Array<string> (e.g., update history = signal<string[]>([]) to
history = signal<Array<string>>([]) and adjust any other annotations or uses of
history in the AppComponent class accordingly).
In `@examples/angular/injectHotkeySequences/src/app/app.config.ts`:
- Around line 1-6: The file currently imports provideHotkeys but doesn't use it
and pulls in ApplicationConfig as a value import; change the ApplicationConfig
import to a type-only import (import type { ApplicationConfig }) and add
provideHotkeys(...) into the providers array of the exported appConfig so the
example demonstrates hotkey setup; reference the symbols ApplicationConfig,
provideHotkeys, appConfig and provideZoneChangeDetection when making these
edits.
In `@examples/angular/injectHotkeySequences/src/styles.css`:
- Around line 22-27: Remove the duplicate margin declaration in the CSS rule for
selector "header p": delete the earlier "margin: 0" and keep the later "margin:
0 auto" so only one margin declaration remains in the "header p" rule to satisfy
Stylelint and avoid redundancy.
In `@examples/preact/useHotkeySequences/src/index.css`:
- Around line 26-30: The CSS block for the selector "header p" contains a
duplicate margin declaration; remove the redundant "margin: 0;" so the rule only
keeps the intended layout declaration ("margin: 0 auto") in the header p rule to
avoid dead code and ensure the paragraph is centered horizontally.
In `@examples/react/useHotkeySequences/src/index.css`:
- Around line 26-31: The CSS selector "header p" contains a duplicate margin
declaration: remove the redundant "margin: 0" so only "margin: 0 auto" remains
(or consolidate into a single margin rule) within the header p block to satisfy
Stylelint and prevent the overwritten property.
In `@examples/solid/createHotkeySequences/src/index.css`:
- Around line 22-27: The selector "header p" contains a duplicated margin
declaration; remove the redundant "margin: 0" so only the intended "margin: 0
auto" remains (or merge them into a single "margin: 0 auto") alongside the
existing "max-width: 500px" and other properties to satisfy Stylelint and avoid
the overridden rule.
In `@examples/solid/createHotkeySequences/src/index.tsx`:
- Around line 3-8: Reorder the named imports to satisfy the ESLint sort-imports
rule: in the first import list swap createSignal and Show so Show comes before
createSignal, and in the second import list reorder the three symbols so
HotkeysProvider appears before createHotkey and createHotkeySequences
(preserving the relative order between the two create* names).
In `@examples/svelte/create-hotkey-sequences/README.md`:
- Around line 16-19: Update the recreate command in the README so the generated
project name matches the example directory: replace the erroneous
"create-hotkey" token in the shell command with "create-hotkey-sequences" (the
command string in the README's recreate example).
In `@examples/svelte/create-hotkey-sequences/src/index.css`:
- Around line 26-31: The CSS rule for the selector "header p" contains a
duplicate margin declaration; remove the redundant "margin: 0;" and keep the
intended "margin: 0 auto;" in the header p rule (locate the header p block in
index.css) so only a single margin declaration remains and the stylelint error
is resolved.
In `@examples/vue/useHotkeySequences/src/index.css`:
- Around line 26-31: The CSS rule for selector "header p" contains a duplicate
margin declaration; remove the redundant "margin: 0;" and keep a single
consolidated margin declaration (e.g., "margin: 0 auto;") in the "header p" rule
so there is no overridden/unused property.
In `@packages/solid-hotkeys/tests/createHotkeySequences.test.tsx`:
- Around line 2-10: Reorder and split the imports to satisfy ESLint rules:
alphabetize named members inside each import, move type-only imports to
top-level "import type" statements, and place the "solid-js" import before the
relative module import. Specifically, convert "type
CreateHotkeySequenceDefinition" and "type Component" to top-level import type
lines, alphabetize members in the vitest import (afterEach, beforeEach,
describe, expect, it, vi) and in the `@tanstack/hotkeys` import (SequenceManager),
ensure "createSignal" and "Component" from "solid-js" are imported before
"../src/createHotkeySequences", and keep "render" from
"@solidjs/testing-library" ordered appropriately.
---
Nitpick comments:
In `@docs/framework/preact/guides/sequences.md`:
- Line 38: Add a short example demonstrating the second-argument precedence for
useHotkeySequences so readers can see how HotkeysProvider defaults, the
second-argument commonOptions, and each definition’s options merge (e.g., show
calling useHotkeySequences(definitions, commonOptions) with one shared option
overridden by a per-definition options entry). Refer to useHotkeySequences,
HotkeysProvider, commonOptions, definitions, and each definition’s options in
the example so it clearly shows the precedence order in practice.
In `@examples/angular/injectHotkeySequences/src/app/app.component.html`:
- Around line 72-74: The dynamic "Triggered" status rendered via the
lastSequence() template binding (as seq) should announce updates to assistive
tech; add aria-live="polite" to the div with class "info-box success" that
displays {{ seq }} so screen readers are notified of changes without
interrupting. Target the template block using lastSequence() / seq and update
that div to include the aria-live attribute.
In `@packages/angular-hotkeys/src/injectHotkeySequences.ts`:
- Around line 106-110: The Angular implementation currently destructures enabled
from mergedOptions and skips registration when enabled is false (using const {
enabled = true, ...sequenceOpts } = mergedOptions and continue), and later
always registers with enabled: true; instead, preserve and pass the resolved
enabled value through like other frameworks: do not early-continue on enabled
=== false (still register the sequence handle), and when calling the
registration or setOptions helper (the code path that currently uses
sequenceOpts/registration call), include enabled (the actual resolved enabled)
rather than hardcoding true so the manager can toggle state via
setOptions/update instead of unregistering/re-registering; adjust uses of
mergedOptions, sequenceOpts, and the register/setOptions call accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 216e776a-35f5-4f4b-97a0-20356289fa95
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (95)
.changeset/plural-sequences.mddocs/config.jsondocs/framework/angular/guides/sequences.mddocs/framework/angular/reference/functions/injectHotkeySequences.mddocs/framework/angular/reference/index.mddocs/framework/angular/reference/interfaces/InjectHotkeySequenceDefinition.mddocs/framework/preact/guides/sequences.mddocs/framework/preact/reference/functions/useHotkeySequences.mddocs/framework/preact/reference/index.mddocs/framework/preact/reference/interfaces/UseHotkeySequenceDefinition.mddocs/framework/react/guides/sequences.mddocs/framework/react/reference/functions/useHotkeySequences.mddocs/framework/react/reference/index.mddocs/framework/react/reference/interfaces/UseHotkeySequenceDefinition.mddocs/framework/solid/guides/sequences.mddocs/framework/solid/reference/functions/createHotkeySequences.mddocs/framework/solid/reference/index.mddocs/framework/solid/reference/interfaces/CreateHotkeySequenceDefinition.mddocs/framework/svelte/guides/sequences.mddocs/framework/svelte/reference/functions/createHotkeySequences.mddocs/framework/svelte/reference/functions/createHotkeySequencesAttachment.mddocs/framework/svelte/reference/index.mddocs/framework/svelte/reference/interfaces/CreateHotkeySequenceDefinition.mddocs/framework/vue/guides/sequences.mddocs/framework/vue/quick-start.mddocs/framework/vue/reference/functions/useHotkeySequences.mddocs/framework/vue/reference/index.mddocs/framework/vue/reference/interfaces/UseHotkeySequenceDefinition.mdexamples/angular/injectHotkeySequences/angular.jsonexamples/angular/injectHotkeySequences/package.jsonexamples/angular/injectHotkeySequences/src/app/app.component.cssexamples/angular/injectHotkeySequences/src/app/app.component.htmlexamples/angular/injectHotkeySequences/src/app/app.component.tsexamples/angular/injectHotkeySequences/src/app/app.config.tsexamples/angular/injectHotkeySequences/src/index.htmlexamples/angular/injectHotkeySequences/src/main.tsexamples/angular/injectHotkeySequences/src/styles.cssexamples/angular/injectHotkeySequences/tsconfig.jsonexamples/preact/useHotkeySequences/eslint.config.jsexamples/preact/useHotkeySequences/index.htmlexamples/preact/useHotkeySequences/package.jsonexamples/preact/useHotkeySequences/src/index.cssexamples/preact/useHotkeySequences/src/index.tsxexamples/preact/useHotkeySequences/tsconfig.jsonexamples/preact/useHotkeySequences/vite.config.tsexamples/react/useHotkeySequences/eslint.config.jsexamples/react/useHotkeySequences/index.htmlexamples/react/useHotkeySequences/package.jsonexamples/react/useHotkeySequences/src/index.cssexamples/react/useHotkeySequences/src/index.tsxexamples/react/useHotkeySequences/tsconfig.jsonexamples/react/useHotkeySequences/vite.config.tsexamples/solid/createHotkeySequences/index.htmlexamples/solid/createHotkeySequences/package.jsonexamples/solid/createHotkeySequences/src/index.cssexamples/solid/createHotkeySequences/src/index.tsxexamples/solid/createHotkeySequences/tsconfig.jsonexamples/solid/createHotkeySequences/vite.config.tsexamples/svelte/create-hotkey-sequences/.gitignoreexamples/svelte/create-hotkey-sequences/.npmrcexamples/svelte/create-hotkey-sequences/README.mdexamples/svelte/create-hotkey-sequences/index.htmlexamples/svelte/create-hotkey-sequences/package.jsonexamples/svelte/create-hotkey-sequences/src/App.svelteexamples/svelte/create-hotkey-sequences/src/Root.svelteexamples/svelte/create-hotkey-sequences/src/index.cssexamples/svelte/create-hotkey-sequences/src/main.tsexamples/svelte/create-hotkey-sequences/static/robots.txtexamples/svelte/create-hotkey-sequences/svelte.config.jsexamples/svelte/create-hotkey-sequences/tsconfig.jsonexamples/svelte/create-hotkey-sequences/vite.config.tsexamples/vue/useHotkeySequences/eslint.config.jsexamples/vue/useHotkeySequences/index.htmlexamples/vue/useHotkeySequences/package.jsonexamples/vue/useHotkeySequences/src/App.vueexamples/vue/useHotkeySequences/src/index.cssexamples/vue/useHotkeySequences/src/index.tsexamples/vue/useHotkeySequences/src/vue.d.tsexamples/vue/useHotkeySequences/tsconfig.jsonexamples/vue/useHotkeySequences/vite.config.tspackages/angular-hotkeys/src/index.tspackages/angular-hotkeys/src/injectHotkeySequences.tspackages/preact-hotkeys/src/index.tspackages/preact-hotkeys/src/useHotkeySequences.tspackages/preact-hotkeys/tests/useHotkeySequences.test.tsxpackages/react-hotkeys/src/index.tspackages/react-hotkeys/src/useHotkeySequences.tspackages/react-hotkeys/tests/useHotkeySequences.test.tsxpackages/solid-hotkeys/src/createHotkeySequences.tspackages/solid-hotkeys/src/index.tspackages/solid-hotkeys/tests/createHotkeySequences.test.tsxpackages/svelte-hotkeys/src/createHotkeySequences.svelte.tspackages/svelte-hotkeys/src/index.tspackages/vue-hotkeys/src/index.tspackages/vue-hotkeys/src/useHotkeySequences.ts
KevinVandy
force-pushed
the
hotkeysequences
branch
from
1f650ae to
4767bbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/angular/injectHotkeySequences/src/app/app.component.html`:
- Line 112: Escape the '>' in arrow functions inside the HTML example: replace
each arrow token "=>" in the callback expressions with "=>" so the snippets
like "callback: () => scrollToTop()" become "callback: () => scrollToTop()";
apply the same change for the other occurrences referenced (lines containing the
callback arrow at the sequence entries on the same block).
- Line 112: The code example uses uppercase sequence ['G','G'] but the
docs/table describe lowercase "g g" (Vim semantics: gg -> top); update the
sequence in the example (the object containing "sequence: ['G','G'], callback:
() => scrollToTop()") to use lowercase ['g','g'] so the example matches the
table and expected behavior.
In `@examples/angular/injectHotkeySequences/src/app/app.config.ts`:
- Around line 1-3: Reorder the imports so the regular imports come before the
type-only import: place the existing `import { provideZoneChangeDetection } from
'@angular/core'` and `import { provideHotkeys } from
'@tanstack/angular-hotkeys'` first, and then move the type import `import type {
ApplicationConfig } from '@angular/core'` after them to satisfy the project's
`import/order` ESLint rule; update any references to `ApplicationConfig`,
`provideZoneChangeDetection`, or `provideHotkeys` accordingly.
In `@packages/solid-hotkeys/tests/createHotkeySequences.test.tsx`:
- Around line 5-8: The import order violates ESLint import/order: move all value
imports (createSignal from 'solid-js' and createHotkeySequences from
'../src/createHotkeySequences') before type-only imports, and group type imports
together (Component from 'solid-js' and CreateHotkeySequenceDefinition from
'../src/createHotkeySequences'); update the import statements so value imports
appear first then type imports to satisfy the rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f44e66a4-e2be-4fcf-8ae7-ad91880b1242
📒 Files selected for processing (27)
.changeset/plural-sequences.mddocs/framework/angular/guides/sequences.mddocs/framework/angular/reference/functions/injectHotkeySequences.mddocs/framework/angular/reference/interfaces/InjectHotkeySequenceDefinition.mddocs/framework/preact/reference/functions/useHotkeySequences.mddocs/framework/preact/reference/interfaces/UseHotkeySequenceDefinition.mddocs/framework/react/reference/functions/useHotkeySequences.mddocs/framework/react/reference/interfaces/UseHotkeySequenceDefinition.mddocs/framework/solid/reference/functions/createHotkeySequences.mddocs/framework/solid/reference/interfaces/CreateHotkeySequenceDefinition.mddocs/framework/svelte/reference/functions/createHotkeySequences.mddocs/framework/svelte/reference/functions/createHotkeySequencesAttachment.mddocs/framework/svelte/reference/interfaces/CreateHotkeySequenceDefinition.mddocs/framework/vue/reference/functions/useHotkeySequences.mddocs/framework/vue/reference/interfaces/UseHotkeySequenceDefinition.mdexamples/angular/injectHotkeySequences/src/app/app.component.htmlexamples/angular/injectHotkeySequences/src/app/app.component.tsexamples/angular/injectHotkeySequences/src/app/app.config.tsexamples/angular/injectHotkeySequences/src/styles.cssexamples/preact/useHotkeySequences/src/index.cssexamples/react/useHotkeySequences/src/index.cssexamples/solid/createHotkeySequences/src/index.cssexamples/solid/createHotkeySequences/src/index.tsxexamples/svelte/create-hotkey-sequences/README.mdexamples/svelte/create-hotkey-sequences/src/index.cssexamples/vue/useHotkeySequences/src/index.csspackages/solid-hotkeys/tests/createHotkeySequences.test.tsx
✅ Files skipped from review due to trivial changes (23)
- docs/framework/angular/guides/sequences.md
- docs/framework/preact/reference/interfaces/UseHotkeySequenceDefinition.md
- docs/framework/svelte/reference/functions/createHotkeySequences.md
- docs/framework/react/reference/interfaces/UseHotkeySequenceDefinition.md
- docs/framework/svelte/reference/functions/createHotkeySequencesAttachment.md
- docs/framework/angular/reference/interfaces/InjectHotkeySequenceDefinition.md
- .changeset/plural-sequences.md
- examples/svelte/create-hotkey-sequences/README.md
- docs/framework/react/reference/functions/useHotkeySequences.md
- docs/framework/angular/reference/functions/injectHotkeySequences.md
- docs/framework/vue/reference/interfaces/UseHotkeySequenceDefinition.md
- docs/framework/svelte/reference/interfaces/CreateHotkeySequenceDefinition.md
- examples/react/useHotkeySequences/src/index.css
- docs/framework/solid/reference/functions/createHotkeySequences.md
- examples/solid/createHotkeySequences/src/index.tsx
- examples/angular/injectHotkeySequences/src/styles.css
- examples/preact/useHotkeySequences/src/index.css
- docs/framework/preact/reference/functions/useHotkeySequences.md
- docs/framework/vue/reference/functions/useHotkeySequences.md
- docs/framework/solid/reference/interfaces/CreateHotkeySequenceDefinition.md
- examples/solid/createHotkeySequences/src/index.css
- examples/svelte/create-hotkey-sequences/src/index.css
- examples/vue/useHotkeySequences/src/index.css
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/angular/injectHotkeySequences/src/app/app.component.ts
|
|
||
| // In constructor or injection context: | ||
| injectHotkeySequences([ | ||
| {{ '{' }} sequence: ['G', 'G'], callback: () => scrollToTop() {{ '}' }}, |
There was a problem hiding this comment.
Escape arrow function > characters in the code example.
The > in arrow functions should be escaped as > for strict HTML compliance within the <pre> block. You've already escaped @ as @ on line 108, but the > characters on lines 112, 115, 118, and 121 also need escaping.
🔧 Proposed fix
injectHotkeySequences([
- { sequence: ['G', 'G'], callback: () => scrollToTop() },
+ { sequence: ['G', 'G'], callback: () => scrollToTop() },
{
sequence: ['ArrowUp', 'ArrowUp', 'ArrowDown', 'ArrowDown'],
- callback: () => activateCheatMode(),
+ callback: () => activateCheatMode(),
options: { timeout: 1500 },
},
- { sequence: ['C', 'I', 'W'], callback: () => changeInnerWord() },
- { sequence: ['Shift+R', 'Shift+T'], callback: () => doSomething() },
+ { sequence: ['C', 'I', 'W'], callback: () => changeInnerWord() },
+ { sequence: ['Shift+R', 'Shift+T'], callback: () => doSomething() },
])Also applies to: 115-115, 118-118, 121-121
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 112-112: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/angular/injectHotkeySequences/src/app/app.component.html` at line
112, Escape the '>' in arrow functions inside the HTML example: replace each
arrow token "=>" in the callback expressions with "=>" so the snippets like
"callback: () => scrollToTop()" become "callback: () => scrollToTop()"; apply
the same change for the other occurrences referenced (lines containing the
callback arrow at the sequence entries on the same block).
Fix inconsistency between table and code example.
The table at lines 22-24 shows g g (lowercase) for "Go to top", but the code example uses ['G', 'G'] (uppercase). In Vim semantics, gg goes to top and G goes to bottom. The code example should use ['g', 'g'] to match the documented behavior.
📝 Proposed fix
injectHotkeySequences([
- { sequence: ['G', 'G'], callback: () => scrollToTop() },
+ { sequence: ['g', 'g'], callback: () => scrollToTop() },
{🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 112-112: Special characters must be escaped : [ > ].
(spec-char-escape)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/angular/injectHotkeySequences/src/app/app.component.html` at line
112, The code example uses uppercase sequence ['G','G'] but the docs/table
describe lowercase "g g" (Vim semantics: gg -> top); update the sequence in the
example (the object containing "sequence: ['G','G'], callback: () =>
scrollToTop()") to use lowercase ['g','g'] so the example matches the table and
expected behavior.
Comment on lines
+1
to
+3
| import type { ApplicationConfig } from '@angular/core' | ||
| import { provideZoneChangeDetection } from '@angular/core' | ||
| import { provideHotkeys } from '@tanstack/angular-hotkeys' |
There was a problem hiding this comment.
Fix import order to satisfy ESLint.
The type import from @angular/core should be placed after regular imports per the project's import/order rule.
📝 Proposed fix
-import type { ApplicationConfig } from '@angular/core'
import { provideZoneChangeDetection } from '@angular/core'
import { provideHotkeys } from '@tanstack/angular-hotkeys'
+import type { ApplicationConfig } from '@angular/core'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { ApplicationConfig } from '@angular/core' | |
| import { provideZoneChangeDetection } from '@angular/core' | |
| import { provideHotkeys } from '@tanstack/angular-hotkeys' | |
| import { provideZoneChangeDetection } from '@angular/core' | |
| import { provideHotkeys } from '@tanstack/angular-hotkeys' | |
| import type { ApplicationConfig } from '@angular/core' |
🧰 Tools
🪛 ESLint
[error] 1-1: @angular/core type import should occur after import of @tanstack/angular-hotkeys
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/angular/injectHotkeySequences/src/app/app.config.ts` around lines 1
- 3, Reorder the imports so the regular imports come before the type-only
import: place the existing `import { provideZoneChangeDetection } from
'@angular/core'` and `import { provideHotkeys } from
'@tanstack/angular-hotkeys'` first, and then move the type import `import type {
ApplicationConfig } from '@angular/core'` after them to satisfy the project's
`import/order` ESLint rule; update any references to `ApplicationConfig`,
`provideZoneChangeDetection`, or `provideHotkeys` accordingly.
Comment on lines
+5
to
+8
| import { createSignal } from 'solid-js' | ||
| import type { Component } from 'solid-js' | ||
| import { createHotkeySequences } from '../src/createHotkeySequences' | ||
| import type { CreateHotkeySequenceDefinition } from '../src/createHotkeySequences' |
There was a problem hiding this comment.
Import order still violates ESLint import/order rule.
The value import from ../src/createHotkeySequences (line 7) should occur before the type import from solid-js (line 6). Group all value imports together, then all type imports.
Proposed fix
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { render } from '@solidjs/testing-library'
import { SequenceManager } from '@tanstack/hotkeys'
import { createSignal } from 'solid-js'
-import type { Component } from 'solid-js'
import { createHotkeySequences } from '../src/createHotkeySequences'
+import type { Component } from 'solid-js'
import type { CreateHotkeySequenceDefinition } from '../src/createHotkeySequences'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { createSignal } from 'solid-js' | |
| import type { Component } from 'solid-js' | |
| import { createHotkeySequences } from '../src/createHotkeySequences' | |
| import type { CreateHotkeySequenceDefinition } from '../src/createHotkeySequences' | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| import { render } from '@solidjs/testing-library' | |
| import { SequenceManager } from '@tanstack/hotkeys' | |
| import { createSignal } from 'solid-js' | |
| import { createHotkeySequences } from '../src/createHotkeySequences' | |
| import type { Component } from 'solid-js' | |
| import type { CreateHotkeySequenceDefinition } from '../src/createHotkeySequences' |
🧰 Tools
🪛 ESLint
[error] 7-7: ../src/createHotkeySequences import should occur before type import of solid-js
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-hotkeys/tests/createHotkeySequences.test.tsx` around lines 5 -
8, The import order violates ESLint import/order: move all value imports
(createSignal from 'solid-js' and createHotkeySequences from
'../src/createHotkeySequences') before type-only imports, and group type imports
together (Component from 'solid-js' and CreateHotkeySequenceDefinition from
'../src/createHotkeySequences'); update the import statements so value imports
appear first then type imports to satisfy the rule.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts (1)
72-76:⚠️ Potential issue | 🟡 MinorCallback reference remains stale if only the callback changes without sequence or options changing.
Unlike React and Preact implementations which update a
callbackRefevery render (outside effects), the Svelte implementation accessescallbackdirectly and only synchronizes it when the effect re-runs (lines 106, 119). The effect depends only onresolveMaybeGetter(sequence)andresolveMaybeGetter(options), so if only the callback reference changes—while sequence and options remain the same—the effect won't re-run and line 106 won't execute, leaving the registration with a stale callback.React and Preact prevent this by updating their callback refs on every render outside the effect, ensuring the latest callback is always available. Consider adopting a similar pattern or explicitly including callback tracking in Svelte.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts` around lines 72 - 76, The effect in createHotkeySequence registers handlers that capture the original callback and doesn't update when only callback changes; fix by introducing and updating a mutable reference (e.g., callbackRef) for the callback at the top of createHotkeySequence on every invocation and have the registered handlers call callbackRef.current (or equivalent) instead of the captured callback; keep the effect dependency on resolveMaybeGetter(sequence) and resolveMaybeGetter(options) but ensure lines that register listeners (the handlers referenced at registration sites around the existing lines ~106 and ~119) use the up-to-date callbackRef so callback changes are honored without re-running the effect.packages/angular-hotkeys/src/injectHotkeySequence.ts (1)
13-19:⚠️ Potential issue | 🟠 MajorSequence targets can't safely wait for mount yet.
sequenceOptions.target ?? documentmeans a getter like() => ({ target: this.editorRef()?.nativeElement ?? null })registers ondocumentuntil the element exists, instead of staying inactive. That is inconsistent withinjectHotkey, which preserves an explicit missing target.Suggested fix
-export interface InjectHotkeySequenceOptions extends Omit< - SequenceOptions, - 'enabled' -> { +export interface InjectHotkeySequenceOptions extends Omit< + SequenceOptions, + 'enabled' | 'target' +> { /** Whether the sequence is enabled. Defaults to true. */ enabled?: boolean + target?: HTMLElement | Document | Window | null } ... - const resolvedTarget = - sequenceOptions.target ?? - (typeof document !== 'undefined' ? document : undefined) + const resolvedTarget = + 'target' in sequenceOptions + ? (sequenceOptions.target ?? null) + : (typeof document !== 'undefined' ? document : null)Also applies to: 89-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-hotkeys/src/injectHotkeySequence.ts` around lines 13 - 19, The code currently uses "sequenceOptions.target ?? document" which treats an explicit null target (e.g. a getter that returns null until mount) as missing and falls back to document; change the fallback logic in injectHotkeySequence so it only defaults to document when the target property is truly absent/undefined (use a presence check like "'target' in sequenceOptions ? sequenceOptions.target : document" or Object.prototype.hasOwnProperty.call(sequenceOptions, 'target') ? sequenceOptions.target : document"), and apply the same change to the similar logic at the block referenced around lines 89-93 so explicit null targets remain preserved and sequences stay inactive until the real element exists.
🧹 Nitpick comments (11)
examples/preact/useHotkeySequence/src/index.tsx (1)
148-153: Consider exposing toggle state witharia-pressed.The control works, but adding toggle semantics will improve screen reader behavior.
♿ Optional accessibility tweak
<button type="button" + aria-pressed={helloSequenceEnabled} onClick={() => setHelloSequenceEnabled((v) => !v)} > {helloSequenceEnabled ? 'Disable' : 'Enable'} sequence </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/useHotkeySequence/src/index.tsx` around lines 148 - 153, The toggle button currently toggles state via setHelloSequenceEnabled and reflects it with helloSequenceEnabled but lacks ARIA toggle semantics; update the button element (the element using onClick={() => setHelloSequenceEnabled((v) => !v)}) to include aria-pressed={helloSequenceEnabled} so assistive tech understands its pressed/toggled state; keep the element as a native <button> (no role change required) and ensure the aria-pressed value is kept in sync with helloSequenceEnabled.examples/angular/injectHotkeySequence/src/app/app.component.html (1)
68-77: Consider exposing toggle/state changes to assistive tech.Nice UX addition. For accessibility, consider
aria-pressedon the toggle button andaria-live="polite"on the status text so screen readers announce state changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/angular/injectHotkeySequence/src/app/app.component.html` around lines 68 - 77, Add accessibility attributes so state changes are announced: add aria-live="polite" to the status element (sequence-toggle-status paragraph) and add aria-pressed="{{ helloSequenceEnabled() }}" to the toggle button used by toggleHelloSequence(); ensure the aria-pressed value is derived from helloSequenceEnabled() so assistive tech reflects the current enabled/disabled state when the button is clicked.examples/svelte/create-hotkey-sequence/src/App.svelte (1)
142-147: Consider marking the toggle button as pressed/unpressed for assistive tech.This is a true toggle control; adding
aria-pressedwould improve screen-reader semantics.♿ Optional accessibility tweak
<button type="button" + aria-pressed={helloSequenceEnabled} onclick={() => (helloSequenceEnabled = !helloSequenceEnabled)} > {helloSequenceEnabled ? 'Disable' : 'Enable'} sequence </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/svelte/create-hotkey-sequence/src/App.svelte` around lines 142 - 147, The toggle button controlling helloSequenceEnabled is missing aria-pressed which would expose its pressed state to assistive tech; update the button element that toggles helloSequenceEnabled to include aria-pressed bound to that boolean (e.g., aria-pressed={helloSequenceEnabled}) so screen readers receive the current state while keeping the existing click handler and visible label logic intact.examples/solid/createHotkeySequence/src/index.tsx (1)
149-154: Optional: expose toggle state witharia-pressed.Since this is a true on/off toggle, adding pressed state improves assistive-tech semantics.
Suggested tweak
<button type="button" + aria-pressed={helloSequenceEnabled()} onClick={() => setHelloSequenceEnabled(!helloSequenceEnabled())} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/solid/createHotkeySequence/src/index.tsx` around lines 149 - 154, The button is a true on/off toggle but lacks an accessibility pressed state; update the toggle button (the element using helloSequenceEnabled() and setHelloSequenceEnabled) to include an aria-pressed attribute bound to the current state (aria-pressed={helloSequenceEnabled()}), ensuring assistive tech knows when the sequence is enabled or disabled while preserving the existing onClick behavior.examples/vue/useHotkeySequence/src/App.vue (1)
153-165: Add toggle semantics for assistive technologies.Consider exposing the button state with
aria-pressed(and optionally polite live updates on status) so state changes are conveyed more reliably.♿ Suggested accessibility refinement
- <p class="sequence-toggle-status"> + <p class="sequence-toggle-status" aria-live="polite"> This sequence is <strong>{{ helloSequenceEnabled ? 'enabled' : 'disabled' }}</strong >. </p> <button type="button" + :aria-pressed="helloSequenceEnabled" `@click`="helloSequenceEnabled = !helloSequenceEnabled" > {{ helloSequenceEnabled ? 'Disable' : 'Enable' }} sequence </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/vue/useHotkeySequence/src/App.vue` around lines 153 - 165, The toggle button isn't exposing its state to assistive tech; update the button that toggles helloSequenceEnabled to include an aria-pressed attribute bound to helloSequenceEnabled (e.g., aria-pressed="helloSequenceEnabled") and mark the status paragraph with a polite live region (e.g., aria-live="polite") so screen readers are notified of changes; ensure the button text still updates based on helloSequenceEnabled and that the status paragraph (sequence-toggle-status) reflects the same state.packages/svelte-hotkeys/src/createHotkey.svelte.ts (1)
35-49: Minor: Redundant hotkey normalization.The callers of
registerHotkey(at lines 85 and 153) already callnormalizeHotkeyto computehotkeyStrfor comparison. ThenregisterHotkeycallsnormalizeHotkeyagain at line 42. Since string hotkeys pass through unchanged, this is harmless but slightly redundant.Consider accepting the already-normalized
Hotkeystring instead ofRegisterableHotkeyto avoid the extra call:♻️ Optional refactor to avoid double normalization
function registerHotkey( target: HTMLElement | Document | Window, - hotkey: RegisterableHotkey, + hotkey: Hotkey, callback: HotkeyCallback, mergedOptions: CreateHotkeyOptions, ) { return getHotkeyManager().register( - normalizeHotkey(hotkey, mergedOptions), + hotkey, callback, { ...mergedOptions, target, }, ) }Then update callers to pass
hotkeyStrinstead ofresolvedHotkey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-hotkeys/src/createHotkey.svelte.ts` around lines 35 - 49, The registerHotkey function currently accepts a RegisterableHotkey and calls normalizeHotkey again; change registerHotkey to accept an already-normalized Hotkey string (or hotkeyStr) instead of RegisterableHotkey, remove the internal normalizeHotkey call, and pass that hotkey string directly into getHotkeyManager().register; then update the callers that currently compute hotkeyStr (the places that call registerHotkey with resolvedHotkey) to pass hotkeyStr into registerHotkey so no double-normalization occurs (references: function registerHotkey, normalizeHotkey, and getHotkeyManager().register).examples/vue/useHotkeySequences/src/App.vue (1)
223-223: Set explicit button type for “Clear History”.Low-risk hardening: add
type="button"to avoid accidental submit behavior if this block is later moved inside a<form>.Suggested patch
- <button `@click`="history = []">Clear History</button> + <button type="button" `@click`="history = []">Clear History</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/vue/useHotkeySequences/src/App.vue` at line 223, The "Clear History" button currently lacks an explicit type which can cause it to act as a submit button if moved inside a form; update the button element that sets "history = []" (the Clear History button) to include type="button" so it will not trigger form submission unintentionally.packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts (1)
156-199: Consider extracting shared registration logic to reduce duplication.The effect logic in
createHotkeySequenceAttachment(lines 160-199) is nearly identical tocreateHotkeySequence(lines 80-123), differing only in the target (element vs document). This duplication could make future maintenance of the caching/update logic error-prone if changes need to be applied in both places.Consider extracting a shared internal helper:
// Example internal helper signature function createSequenceEffect( target: HTMLElement | Document | Window, sequence: MaybeGetter<HotkeySequence>, callback: HotkeyCallback, options: MaybeGetter<CreateHotkeySequenceOptions>, ): { cleanup: () => void }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts` around lines 156 - 199, The two effect blocks in createHotkeySequence and createHotkeySequenceAttachment duplicate registration/update/unregister logic (variables registration, lastSequenceKey, sequenceKey and calls to registerHotkeySequence), so extract that shared logic into a single helper (e.g., createSequenceEffect) that accepts the target (element vs document/window), the resolved sequence getter, callback, and options, and performs the resolve, comparison (by formatHotkeySequence), register, update (setOptions + update callback), and unregister behavior; then replace the duplicated $effect bodies in createHotkeySequence and createHotkeySequenceAttachment to call that helper and store/cleanup its returned handle.packages/solid-hotkeys/src/createHotkey.ts (1)
165-168: Redundant callback/options sync immediately after registration.The
manager.register()call on lines 160-163 already receivescallbackandoptionsWithoutTarget. Immediately callingregistration.callback = callbackandregistration.setOptions(optionsWithoutTarget)on lines 166-167 is redundant since the registration was just created with these exact values.♻️ Proposed fix to remove redundant sync
registration = manager.register(hotkeyString, callback, { ...optionsWithoutTarget, target: resolvedTarget, }) - if (registration.isActive) { - registration.callback = callback - registration.setOptions(optionsWithoutTarget) - } - lastHotkeyString = hotkeyString🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-hotkeys/src/createHotkey.ts` around lines 165 - 168, Remove the redundant immediate sync after creating a registration: when you call manager.register(callback, optionsWithoutTarget) you already pass the callback and options, so drop the extra assignments guarded by registration.isActive; specifically remove or skip setting registration.callback and calling registration.setOptions(optionsWithoutTarget) right after manager.register(...) (references: manager.register, registration, registration.isActive, registration.callback, registration.setOptions, optionsWithoutTarget, callback).packages/solid-hotkeys/src/createHotkeySequence.ts (1)
142-145: Redundant callback/options sync immediately after registration.Same issue as in
createHotkey.ts: themanager.register()call already receives the callback and merged options. The immediate sync on lines 143-144 is unnecessary for a freshly created registration.♻️ Proposed fix to remove redundant sync
registration = manager.register(resolvedSequence, callback, { ...mergedOptions, target: resolvedTarget, }) - if (registration.isActive) { - registration.callback = callback - registration.setOptions(optionsWithoutTarget) - } - lastSequenceKey = sequenceKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-hotkeys/src/createHotkeySequence.ts` around lines 142 - 145, The registration callback/options are being redundantly re-assigned right after creating a new registration; remove the immediate sync that sets registration.callback and calls registration.setOptions in createHotkeySequence.ts (the block guarded by registration.isActive) because manager.register(...) already receives the callback and merged options—leave manager.register(...) to initialize the registration and delete the extra lines that overwrite registration.callback and call registration.setOptions.packages/solid-hotkeys/tests/createHotkeys.test.tsx (1)
149-156: Tests rely on internal manager state structure.Accessing
manager.registrations.state.values()couples this test to implementation details ofHotkeyManager. If the internal state structure changes, these tests will break even if the public API behavior is correct.This is acceptable for verifying soft-disable semantics, but consider documenting this coupling or providing a public API for introspection if this pattern expands.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-hotkeys/tests/createHotkeys.test.tsx` around lines 149 - 156, The test currently reaches into HotkeyManager.registrations.state.values() which couples it to internal structure; change the test to use a public API (or add one) on HotkeyManager such as getRegistrations() or getRegistration(hotkey) that returns registration objects or an enabled flag, then assert via HotkeyManager.getInstance().getRegistration('Mod+Z') or iterate getRegistrations() and check options.enabled; update the test to call HotkeyManager.getInstance(), use getRegistrationCount() for the count assertion, and replace the internal-state access with the new public accessor (or document the coupling if you prefer not to add an API).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/vue/useHotkeySequences/src/App.vue`:
- Around line 63-76: The usageCode example incorrectly shows a React-style
component function (VimEditor) wrapping the Vue composable; update the usageCode
string so it demonstrates calling useHotkeySequences at top-level in a <script
setup> context instead of inside function VimEditor. Replace the existing
snippet so it imports useHotkeySequences and invokes useHotkeySequences([...])
directly in a <script setup> block (preserving the same sequence entries and
options), ensuring the example matches Vue Composition API usage.
In `@packages/angular-hotkeys/src/injectHotkeySequences.ts`:
- Around line 114-120: The code currently falls back to document for sequence
registration by using sequenceOpts.target ?? document which converts a
missing/temporarily-unset scoped target into a global registration; change the
logic to treat a null/undefined sequenceOpts.target as “skip registration” the
same way injectHotkey does: remove the document fallback and only use
sequenceOpts.target if it is non-null, continuing the loop when it is missing;
also update the type InjectHotkeySequenceOptions['target'] in
injectHotkeySequence.ts to be nullable (match InjectHotkeyOptions) so callers
can explicitly pass null/undefined to indicate “no target yet.”
In `@packages/preact-hotkeys/src/useHotkeySequences.ts`:
- Around line 97-187: The effect currently clears registrationsRef.current
during every cleanup and only depends on sequenceKey, causing unnecessary full
re-registers and stale target bindings; update the reconciliation so
registration identity includes resolved target (e.g., incorporate resolvedTarget
into registrationKey or the effect's dependency key) and do not wipe
registrationsRef.current inside this effect's cleanup—instead create a separate
unmount-only useEffect (empty deps) that iterates registrationsRef.current and
unregisters active handles and then clears the map; keep the existing
managerRef.current.register/unregister logic but compute/compare keys using both
sequence string and resolvedTarget (or a stable identifier) so target-only
changes trigger correct re-registration without losing unchanged rows.
In `@packages/vue-hotkeys/src/useHotkeySequence.ts`:
- Around line 114-124: The code currently collapses explicit null and undefined
for resolvedTarget via the ?? operator, causing an explicit null target to be
treated as document; change the fallback logic so only undefined falls back to
document (preserve explicit null). Specifically, update the finalTarget
assignment in useHotkeySequence (where resolvedTarget is used) to use a
conditional that checks resolvedTarget === undefined and only then sets document
(or null), leaving resolvedTarget === null untouched so the existing
!finalTarget cleanup branch (registration, lastSequenceKey, lastTarget) behaves
correctly.
---
Outside diff comments:
In `@packages/angular-hotkeys/src/injectHotkeySequence.ts`:
- Around line 13-19: The code currently uses "sequenceOptions.target ??
document" which treats an explicit null target (e.g. a getter that returns null
until mount) as missing and falls back to document; change the fallback logic in
injectHotkeySequence so it only defaults to document when the target property is
truly absent/undefined (use a presence check like "'target' in sequenceOptions ?
sequenceOptions.target : document" or
Object.prototype.hasOwnProperty.call(sequenceOptions, 'target') ?
sequenceOptions.target : document"), and apply the same change to the similar
logic at the block referenced around lines 89-93 so explicit null targets remain
preserved and sequences stay inactive until the real element exists.
In `@packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts`:
- Around line 72-76: The effect in createHotkeySequence registers handlers that
capture the original callback and doesn't update when only callback changes; fix
by introducing and updating a mutable reference (e.g., callbackRef) for the
callback at the top of createHotkeySequence on every invocation and have the
registered handlers call callbackRef.current (or equivalent) instead of the
captured callback; keep the effect dependency on resolveMaybeGetter(sequence)
and resolveMaybeGetter(options) but ensure lines that register listeners (the
handlers referenced at registration sites around the existing lines ~106 and
~119) use the up-to-date callbackRef so callback changes are honored without
re-running the effect.
---
Nitpick comments:
In `@examples/angular/injectHotkeySequence/src/app/app.component.html`:
- Around line 68-77: Add accessibility attributes so state changes are
announced: add aria-live="polite" to the status element (sequence-toggle-status
paragraph) and add aria-pressed="{{ helloSequenceEnabled() }}" to the toggle
button used by toggleHelloSequence(); ensure the aria-pressed value is derived
from helloSequenceEnabled() so assistive tech reflects the current
enabled/disabled state when the button is clicked.
In `@examples/preact/useHotkeySequence/src/index.tsx`:
- Around line 148-153: The toggle button currently toggles state via
setHelloSequenceEnabled and reflects it with helloSequenceEnabled but lacks ARIA
toggle semantics; update the button element (the element using onClick={() =>
setHelloSequenceEnabled((v) => !v)}) to include
aria-pressed={helloSequenceEnabled} so assistive tech understands its
pressed/toggled state; keep the element as a native <button> (no role change
required) and ensure the aria-pressed value is kept in sync with
helloSequenceEnabled.
In `@examples/solid/createHotkeySequence/src/index.tsx`:
- Around line 149-154: The button is a true on/off toggle but lacks an
accessibility pressed state; update the toggle button (the element using
helloSequenceEnabled() and setHelloSequenceEnabled) to include an aria-pressed
attribute bound to the current state (aria-pressed={helloSequenceEnabled()}),
ensuring assistive tech knows when the sequence is enabled or disabled while
preserving the existing onClick behavior.
In `@examples/svelte/create-hotkey-sequence/src/App.svelte`:
- Around line 142-147: The toggle button controlling helloSequenceEnabled is
missing aria-pressed which would expose its pressed state to assistive tech;
update the button element that toggles helloSequenceEnabled to include
aria-pressed bound to that boolean (e.g., aria-pressed={helloSequenceEnabled})
so screen readers receive the current state while keeping the existing click
handler and visible label logic intact.
In `@examples/vue/useHotkeySequence/src/App.vue`:
- Around line 153-165: The toggle button isn't exposing its state to assistive
tech; update the button that toggles helloSequenceEnabled to include an
aria-pressed attribute bound to helloSequenceEnabled (e.g.,
aria-pressed="helloSequenceEnabled") and mark the status paragraph with a polite
live region (e.g., aria-live="polite") so screen readers are notified of
changes; ensure the button text still updates based on helloSequenceEnabled and
that the status paragraph (sequence-toggle-status) reflects the same state.
In `@examples/vue/useHotkeySequences/src/App.vue`:
- Line 223: The "Clear History" button currently lacks an explicit type which
can cause it to act as a submit button if moved inside a form; update the button
element that sets "history = []" (the Clear History button) to include
type="button" so it will not trigger form submission unintentionally.
In `@packages/solid-hotkeys/src/createHotkey.ts`:
- Around line 165-168: Remove the redundant immediate sync after creating a
registration: when you call manager.register(callback, optionsWithoutTarget) you
already pass the callback and options, so drop the extra assignments guarded by
registration.isActive; specifically remove or skip setting registration.callback
and calling registration.setOptions(optionsWithoutTarget) right after
manager.register(...) (references: manager.register, registration,
registration.isActive, registration.callback, registration.setOptions,
optionsWithoutTarget, callback).
In `@packages/solid-hotkeys/src/createHotkeySequence.ts`:
- Around line 142-145: The registration callback/options are being redundantly
re-assigned right after creating a new registration; remove the immediate sync
that sets registration.callback and calls registration.setOptions in
createHotkeySequence.ts (the block guarded by registration.isActive) because
manager.register(...) already receives the callback and merged options—leave
manager.register(...) to initialize the registration and delete the extra lines
that overwrite registration.callback and call registration.setOptions.
In `@packages/solid-hotkeys/tests/createHotkeys.test.tsx`:
- Around line 149-156: The test currently reaches into
HotkeyManager.registrations.state.values() which couples it to internal
structure; change the test to use a public API (or add one) on HotkeyManager
such as getRegistrations() or getRegistration(hotkey) that returns registration
objects or an enabled flag, then assert via
HotkeyManager.getInstance().getRegistration('Mod+Z') or iterate
getRegistrations() and check options.enabled; update the test to call
HotkeyManager.getInstance(), use getRegistrationCount() for the count assertion,
and replace the internal-state access with the new public accessor (or document
the coupling if you prefer not to add an API).
In `@packages/svelte-hotkeys/src/createHotkey.svelte.ts`:
- Around line 35-49: The registerHotkey function currently accepts a
RegisterableHotkey and calls normalizeHotkey again; change registerHotkey to
accept an already-normalized Hotkey string (or hotkeyStr) instead of
RegisterableHotkey, remove the internal normalizeHotkey call, and pass that
hotkey string directly into getHotkeyManager().register; then update the callers
that currently compute hotkeyStr (the places that call registerHotkey with
resolvedHotkey) to pass hotkeyStr into registerHotkey so no double-normalization
occurs (references: function registerHotkey, normalizeHotkey, and
getHotkeyManager().register).
In `@packages/svelte-hotkeys/src/createHotkeySequence.svelte.ts`:
- Around line 156-199: The two effect blocks in createHotkeySequence and
createHotkeySequenceAttachment duplicate registration/update/unregister logic
(variables registration, lastSequenceKey, sequenceKey and calls to
registerHotkeySequence), so extract that shared logic into a single helper
(e.g., createSequenceEffect) that accepts the target (element vs
document/window), the resolved sequence getter, callback, and options, and
performs the resolve, comparison (by formatHotkeySequence), register, update
(setOptions + update callback), and unregister behavior; then replace the
duplicated $effect bodies in createHotkeySequence and
createHotkeySequenceAttachment to call that helper and store/cleanup its
returned handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efa73896-fc73-4914-8290-fe28359b1091
📒 Files selected for processing (91)
.changeset/plural-sequences.mddocs/framework/angular/guides/hotkeys.mddocs/framework/angular/guides/sequences.mddocs/framework/angular/reference/functions/injectHotkey.mddocs/framework/angular/reference/functions/injectHotkeySequence.mddocs/framework/angular/reference/functions/injectHotkeySequences.mddocs/framework/angular/reference/interfaces/InjectHotkeyOptions.mddocs/framework/angular/reference/interfaces/InjectHotkeySequenceOptions.mddocs/framework/preact/guides/hotkeys.mddocs/framework/preact/guides/sequences.mddocs/framework/preact/reference/functions/useHotkey.mddocs/framework/preact/reference/functions/useHotkeySequence.mddocs/framework/preact/reference/functions/useHotkeySequences.mddocs/framework/preact/reference/functions/useHotkeys.mddocs/framework/react/guides/hotkeys.mddocs/framework/react/guides/sequences.mddocs/framework/react/reference/functions/useHotkey.mddocs/framework/react/reference/functions/useHotkeySequence.mddocs/framework/react/reference/functions/useHotkeySequences.mddocs/framework/react/reference/functions/useHotkeys.mddocs/framework/solid/guides/hotkeys.mddocs/framework/solid/guides/sequences.mddocs/framework/svelte/guides/hotkeys.mddocs/framework/svelte/guides/sequences.mddocs/framework/svelte/reference/functions/createHotkey.mddocs/framework/svelte/reference/functions/createHotkeyAttachment.mddocs/framework/svelte/reference/functions/createHotkeySequence.mddocs/framework/svelte/reference/functions/createHotkeySequenceAttachment.mddocs/framework/svelte/reference/interfaces/CreateHotkeyOptions.mddocs/framework/svelte/reference/interfaces/CreateHotkeySequenceOptions.mddocs/framework/vue/guides/hotkeys.mddocs/framework/vue/guides/sequences.mddocs/reference/classes/HotkeyManager.mddocs/reference/functions/getHotkeyManager.mddocs/reference/interfaces/HotkeyOptions.mddocs/reference/interfaces/HotkeyRegistration.mddocs/reference/interfaces/HotkeyRegistrationHandle.mddocs/reference/interfaces/SequenceOptions.mdexamples/angular/injectHotkeySequence/src/app/app.component.htmlexamples/angular/injectHotkeySequence/src/app/app.component.tsexamples/angular/injectHotkeySequence/src/styles.cssexamples/angular/injectHotkeySequences/src/app/app.component.htmlexamples/angular/injectHotkeySequences/src/app/app.component.tsexamples/angular/injectHotkeySequences/src/styles.cssexamples/preact/useHotkeySequence/src/index.cssexamples/preact/useHotkeySequence/src/index.tsxexamples/preact/useHotkeySequences/src/index.cssexamples/preact/useHotkeySequences/src/index.tsxexamples/react/useHotkeySequence/src/index.cssexamples/react/useHotkeySequence/src/index.tsxexamples/react/useHotkeySequences/src/index.cssexamples/react/useHotkeySequences/src/index.tsxexamples/solid/createHotkeySequence/src/index.cssexamples/solid/createHotkeySequence/src/index.tsxexamples/solid/createHotkeySequences/src/index.cssexamples/solid/createHotkeySequences/src/index.tsxexamples/svelte/create-hotkey-sequence/src/App.svelteexamples/svelte/create-hotkey-sequence/src/index.cssexamples/svelte/create-hotkey-sequences/src/App.svelteexamples/svelte/create-hotkey-sequences/src/index.cssexamples/vue/useHotkeySequence/src/App.vueexamples/vue/useHotkeySequence/src/index.cssexamples/vue/useHotkeySequences/src/App.vueexamples/vue/useHotkeySequences/src/index.csspackages/angular-hotkeys/src/injectHotkey.tspackages/angular-hotkeys/src/injectHotkeySequence.tspackages/angular-hotkeys/src/injectHotkeySequences.tspackages/hotkeys/src/hotkey-manager.tspackages/hotkeys/tests/sequence-manager.test.tspackages/preact-hotkeys/src/useHotkey.tspackages/preact-hotkeys/src/useHotkeySequence.tspackages/preact-hotkeys/src/useHotkeySequences.tspackages/preact-hotkeys/src/useHotkeys.tspackages/preact-hotkeys/tests/useHotkey.test.tsxpackages/preact-hotkeys/tests/useHotkeySequences.test.tsxpackages/preact-hotkeys/tests/useHotkeys.test.tsxpackages/react-hotkeys/src/useHotkey.tspackages/react-hotkeys/src/useHotkeySequence.tspackages/react-hotkeys/src/useHotkeySequences.tspackages/react-hotkeys/src/useHotkeys.tspackages/react-hotkeys/tests/useHotkey.test.tsxpackages/react-hotkeys/tests/useHotkeySequences.test.tsxpackages/react-hotkeys/tests/useHotkeys.test.tsxpackages/solid-hotkeys/src/createHotkey.tspackages/solid-hotkeys/src/createHotkeySequence.tspackages/solid-hotkeys/tests/createHotkeySequences.test.tsxpackages/solid-hotkeys/tests/createHotkeys.test.tsxpackages/svelte-hotkeys/src/createHotkey.svelte.tspackages/svelte-hotkeys/src/createHotkeySequence.svelte.tspackages/vue-hotkeys/src/useHotkey.tspackages/vue-hotkeys/src/useHotkeySequence.ts
✅ Files skipped from review due to trivial changes (50)
- docs/framework/angular/guides/hotkeys.md
- docs/framework/vue/guides/hotkeys.md
- docs/framework/svelte/guides/hotkeys.md
- examples/angular/injectHotkeySequence/src/styles.css
- docs/framework/preact/guides/hotkeys.md
- docs/framework/angular/reference/interfaces/InjectHotkeyOptions.md
- docs/framework/angular/reference/functions/injectHotkey.md
- docs/framework/svelte/reference/interfaces/CreateHotkeySequenceOptions.md
- examples/svelte/create-hotkey-sequence/src/index.css
- docs/reference/functions/getHotkeyManager.md
- docs/framework/preact/reference/functions/useHotkey.md
- docs/framework/preact/reference/functions/useHotkeySequence.md
- examples/react/useHotkeySequence/src/index.css
- docs/framework/react/reference/functions/useHotkey.md
- examples/solid/createHotkeySequence/src/index.css
- examples/vue/useHotkeySequence/src/index.css
- docs/framework/svelte/reference/functions/createHotkeySequence.md
- docs/framework/solid/guides/hotkeys.md
- docs/framework/svelte/reference/functions/createHotkey.md
- docs/framework/svelte/reference/functions/createHotkeyAttachment.md
- docs/framework/svelte/reference/interfaces/CreateHotkeyOptions.md
- docs/framework/angular/reference/functions/injectHotkeySequence.md
- docs/reference/interfaces/HotkeyRegistration.md
- examples/preact/useHotkeySequence/src/index.css
- docs/framework/svelte/reference/functions/createHotkeySequenceAttachment.md
- docs/framework/react/reference/functions/useHotkeySequence.md
- packages/hotkeys/tests/sequence-manager.test.ts
- docs/reference/interfaces/HotkeyRegistrationHandle.md
- .changeset/plural-sequences.md
- docs/framework/solid/guides/sequences.md
- docs/framework/react/reference/functions/useHotkeys.md
- docs/reference/classes/HotkeyManager.md
- docs/framework/svelte/guides/sequences.md
- docs/framework/angular/reference/functions/injectHotkeySequences.md
- docs/framework/preact/reference/functions/useHotkeys.md
- packages/hotkeys/src/hotkey-manager.ts
- docs/framework/vue/guides/sequences.md
- examples/solid/createHotkeySequences/src/index.tsx
- docs/framework/preact/reference/functions/useHotkeySequences.md
- examples/svelte/create-hotkey-sequences/src/index.css
- examples/react/useHotkeySequences/src/index.css
- examples/vue/useHotkeySequences/src/index.css
- docs/framework/react/reference/functions/useHotkeySequences.md
- examples/angular/injectHotkeySequences/src/styles.css
- examples/solid/createHotkeySequences/src/index.css
- docs/framework/react/guides/hotkeys.md
- packages/preact-hotkeys/tests/useHotkeySequences.test.tsx
- docs/framework/angular/reference/interfaces/InjectHotkeySequenceOptions.md
- examples/preact/useHotkeySequences/src/index.tsx
- examples/preact/useHotkeySequences/src/index.css
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/framework/angular/guides/sequences.md
- docs/framework/react/guides/sequences.md
- examples/svelte/create-hotkey-sequences/src/App.svelte
- packages/solid-hotkeys/tests/createHotkeySequences.test.tsx
- packages/react-hotkeys/tests/useHotkeySequences.test.tsx
- docs/framework/preact/guides/sequences.md
Comment on lines
+114
to
+120
| const resolvedTarget = | ||
| sequenceOpts.target ?? | ||
| (typeof document !== 'undefined' ? document : null) | ||
|
|
||
| if (!resolvedTarget) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Don't fall back to document when a scoped target isn't ready.
sequenceOpts.target ?? document turns an explicit or temporary “no target yet” state into a global registration, so element-scoped sequences can fire on document while their host is still mounting. injectHotkey already treats an explicit missing target as “skip registration”; this bulk API should mirror that contract.
Suggested fix
- const resolvedTarget =
- sequenceOpts.target ??
- (typeof document !== 'undefined' ? document : null)
+ const resolvedTarget =
+ 'target' in sequenceOpts
+ ? (sequenceOpts.target ?? null)
+ : (typeof document !== 'undefined' ? document : null)Also make InjectHotkeySequenceOptions['target'] nullable in packages/angular-hotkeys/src/injectHotkeySequence.ts, like InjectHotkeyOptions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const resolvedTarget = | |
| sequenceOpts.target ?? | |
| (typeof document !== 'undefined' ? document : null) | |
| if (!resolvedTarget) { | |
| continue | |
| } | |
| const resolvedTarget = | |
| 'target' in sequenceOpts | |
| ? (sequenceOpts.target ?? null) | |
| : (typeof document !== 'undefined' ? document : null) | |
| if (!resolvedTarget) { | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-hotkeys/src/injectHotkeySequences.ts` around lines 114 -
120, The code currently falls back to document for sequence registration by
using sequenceOpts.target ?? document which converts a missing/temporarily-unset
scoped target into a global registration; change the logic to treat a
null/undefined sequenceOpts.target as “skip registration” the same way
injectHotkey does: remove the document fallback and only use sequenceOpts.target
if it is non-null, continuing the loop when it is missing; also update the type
InjectHotkeySequenceOptions['target'] in injectHotkeySequence.ts to be nullable
(match InjectHotkeyOptions) so callers can explicitly pass null/undefined to
indicate “no target yet.”
Comment on lines
+97
to
+187
| const sequenceKey = sequenceStrings.join('\0') | ||
|
|
||
| useEffect(() => { | ||
| const prevRegistrations = registrationsRef.current | ||
| const nextRegistrations = new Map<string, RegistrationRecord>() | ||
|
|
||
| const rows: Array<{ | ||
| registrationKey: string | ||
| def: (typeof definitionsRef.current)[number] | ||
| seq: HotkeySequence | ||
| seqStr: string | ||
| mergedOptions: UseHotkeySequenceOptions | ||
| resolvedTarget: Document | HTMLElement | Window | ||
| }> = [] | ||
|
|
||
| for (let i = 0; i < definitionsRef.current.length; i++) { | ||
| const def = definitionsRef.current[i]! | ||
| const seqStr = sequenceStringsRef.current[i]! | ||
| const seq = def.sequence | ||
| if (seq.length === 0) { | ||
| continue | ||
| } | ||
|
|
||
| const mergedOptions = { | ||
| ...defaultOptionsRef.current, | ||
| ...commonOptionsRef.current, | ||
| ...def.options, | ||
| } as UseHotkeySequenceOptions | ||
|
|
||
| const resolvedTarget = isRef(mergedOptions.target) | ||
| ? mergedOptions.target.current | ||
| : (mergedOptions.target ?? | ||
| (typeof document !== 'undefined' ? document : null)) | ||
|
|
||
| if (!resolvedTarget) { | ||
| continue | ||
| } | ||
|
|
||
| const registrationKey = `${i}:${seqStr}` | ||
| rows.push({ | ||
| registrationKey, | ||
| def, | ||
| seq, | ||
| seqStr, | ||
| mergedOptions, | ||
| resolvedTarget, | ||
| }) | ||
| } | ||
|
|
||
| const nextKeys = new Set(rows.map((r) => r.registrationKey)) | ||
|
|
||
| for (const [key, record] of prevRegistrations) { | ||
| if (!nextKeys.has(key) && record.handle.isActive) { | ||
| record.handle.unregister() | ||
| } | ||
| } | ||
|
|
||
| for (const row of rows) { | ||
| const { registrationKey, def, seq, mergedOptions, resolvedTarget } = row | ||
|
|
||
| const existing = prevRegistrations.get(registrationKey) | ||
| if (existing?.handle.isActive && existing.target === resolvedTarget) { | ||
| nextRegistrations.set(registrationKey, existing) | ||
| continue | ||
| } | ||
|
|
||
| if (existing?.handle.isActive) { | ||
| existing.handle.unregister() | ||
| } | ||
|
|
||
| const handle = managerRef.current.register(seq, def.callback, { | ||
| ...mergedOptions, | ||
| target: resolvedTarget, | ||
| }) | ||
| nextRegistrations.set(registrationKey, { | ||
| handle, | ||
| target: resolvedTarget, | ||
| }) | ||
| } | ||
|
|
||
| registrationsRef.current = nextRegistrations | ||
|
|
||
| return () => { | ||
| for (const { handle } of registrationsRef.current.values()) { | ||
| if (handle.isActive) { | ||
| handle.unregister() | ||
| } | ||
| } | ||
| registrationsRef.current = new Map() | ||
| } | ||
| }, [sequenceKey]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Preact, does useEffect run the previous cleanup before re-running the effect when dependencies change, and do ref.current mutations trigger the effect when the dependency array itself stays the same?
💡 Result:
Yes, in Preact, useEffect runs the previous cleanup function before re-running the effect when dependencies change, matching React's behavior. Mutations to ref.current do not trigger the effect if the dependency array remains the same, as refs are designed for mutable values that do not cause re-renders or dependency changes.
Citations:
- 1: https://preactjs.com/guide/v10/hooks/
- 2: https://preactjs.com/guide/v10/hooks
- 3: https://reacttraining.com/blog/useEffect-cleanup
- 4: https://medium.com/@vinaykumarbr07/understanding-useeffect-execution-cleanup-in-react-with-examples-ad74f1c7a42b
- 5: useEffect(effect, [ref.current]) is prematurely re-running facebook/react#14387
- 6: https://stackoverflow.com/questions/60476155/is-it-safe-to-use-ref-current-as-useeffects-dependency-when-ref-points-to-a-dom
Reconciliation has the same lifecycle gap as the React version.
Line 179 clears registrationsRef.current during effect cleanup, so any sequenceKey change forces a full re-register on the next run instead of reusing unchanged rows. The effect is also keyed only by sequenceKey, while line 202 strips target from setOptions, so a target-only change leaves the sequence bound to the old element. Move the full teardown to an unmount-only effect and include resolved target identity in the reconciliation key.
Also applies to: 195-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/preact-hotkeys/src/useHotkeySequences.ts` around lines 97 - 187, The
effect currently clears registrationsRef.current during every cleanup and only
depends on sequenceKey, causing unnecessary full re-registers and stale target
bindings; update the reconciliation so registration identity includes resolved
target (e.g., incorporate resolvedTarget into registrationKey or the effect's
dependency key) and do not wipe registrationsRef.current inside this effect's
cleanup—instead create a separate unmount-only useEffect (empty deps) that
iterates registrationsRef.current and unregisters active handles and then clears
the map; keep the existing managerRef.current.register/unregister logic but
compute/compare keys using both sequence string and resolvedTarget (or a stable
identifier) so target-only changes trigger correct re-registration without
losing unchanged rows.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react-hotkeys/src/useHotkeySequences.ts (1)
133-133: Index-based registration key causes full re-registration on array reordering.Using
${i}:${seqStr}as the registration key means reordering definitions (e.g., swapping positions) will unregister and re-register all affected sequences, even if their content hasn't changed. Per the context snippets,register()always generates a new ID, so this creates ID instability.Consider using a stable key based solely on the sequence string, or allowing definitions to provide an optional
keyproperty:+ // In UseHotkeySequenceDefinition interface: + /** Optional stable key for reconciliation (defaults to sequence string) */ + key?: string- const registrationKey = `${i}:${seqStr}` + const registrationKey = def.key ?? seqStrNote: Using only
seqStrwithout the index would require handling duplicate sequences differently (e.g., appending a counter for duplicates).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-hotkeys/src/useHotkeySequences.ts` at line 133, The registration key construction using `${i}:${seqStr}` in registrationKey causes re-registration when definitions are reordered; change registrationKey to be stable by deriving it from the sequence content (e.g., use seqStr) or an explicit per-definition key property (support an optional definitions[].key), and handle duplicate sequences by appending a deterministic counter or occurrence index to seqStr when necessary; update any logic that reads registrationKey (including the register() call and deregistration logic) to use this new stable key so IDs remain stable across reorders.packages/react-hotkeys/tests/useHotkeySequences.test.tsx (2)
110-113: Sequence matching uses array indexing instead of formatHotkeySequence.The test finds the disabled registration by checking
r.sequence[0] === 'D' && r.sequence[1] === 'D'. This works but is fragile if the sequence format changes. Consider using a more robust approach:const disabledView = [...manager.registrations.state.values()].find( (r) => r.sequence.join(',') === 'D,D', )However, this is a minor concern for test code and the current approach is readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-hotkeys/tests/useHotkeySequences.test.tsx` around lines 110 - 113, The test locates the disabled registration by indexing into r.sequence (r.sequence[0] === 'D' && r.sequence[1] === 'D'), which is fragile; update the predicate to compare the whole sequence string instead (e.g., use formatHotkeySequence(r.sequence) === 'D,D' or r.sequence.join(',') === 'D,D') when searching via manager.registrations.state.values() so the test relies on a stable sequence format rather than fixed indices.
116-149: Missing test for removing sequences from the array.The test suite covers adding sequences dynamically but doesn't explicitly test removing sequences (like the
useHotkeystests do with "should handle dynamic array changes (remove hotkey)"). Consider adding a test to verify that removing a definition from the array properly unregisters it:it('should handle dynamic array changes (remove sequence)', () => { const gg = vi.fn() const dd = vi.fn() const { rerender } = renderHook( ({ defs }: { defs: Array<UseHotkeySequenceDefinition> }) => useHotkeySequences(defs), { initialProps: { defs: [ { sequence: ['G', 'G'], callback: gg }, { sequence: ['D', 'D'], callback: dd }, ], }, }, ) expect(SequenceManager.getInstance().getRegistrationCount()).toBe(2) rerender({ defs: [{ sequence: ['G', 'G'], callback: gg }], }) expect(SequenceManager.getInstance().getRegistrationCount()).toBe(1) dispatchKey('d') dispatchKey('d') expect(dd).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-hotkeys/tests/useHotkeySequences.test.tsx` around lines 116 - 149, Add a test that mirrors the existing "add sequence" test but verifies removal: render the hook via useHotkeySequences with two definitions (sequence G,G and D,D), assert SequenceManager.getInstance().getRegistrationCount() is 2, call rerender with only the G,G definition, assert registration count is 1, then dispatchKey('d') twice and assert the dd callback was not called; reference the existing test's use of renderHook, rerender, dispatchKey, and SequenceManager.getInstance().getRegistrationCount() to locate where to add this new "should handle dynamic array changes (remove sequence)" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-hotkeys/src/useHotkeySequences.ts`:
- Line 133: The registration key construction using `${i}:${seqStr}` in
registrationKey causes re-registration when definitions are reordered; change
registrationKey to be stable by deriving it from the sequence content (e.g., use
seqStr) or an explicit per-definition key property (support an optional
definitions[].key), and handle duplicate sequences by appending a deterministic
counter or occurrence index to seqStr when necessary; update any logic that
reads registrationKey (including the register() call and deregistration logic)
to use this new stable key so IDs remain stable across reorders.
In `@packages/react-hotkeys/tests/useHotkeySequences.test.tsx`:
- Around line 110-113: The test locates the disabled registration by indexing
into r.sequence (r.sequence[0] === 'D' && r.sequence[1] === 'D'), which is
fragile; update the predicate to compare the whole sequence string instead
(e.g., use formatHotkeySequence(r.sequence) === 'D,D' or r.sequence.join(',')
=== 'D,D') when searching via manager.registrations.state.values() so the test
relies on a stable sequence format rather than fixed indices.
- Around line 116-149: Add a test that mirrors the existing "add sequence" test
but verifies removal: render the hook via useHotkeySequences with two
definitions (sequence G,G and D,D), assert
SequenceManager.getInstance().getRegistrationCount() is 2, call rerender with
only the G,G definition, assert registration count is 1, then dispatchKey('d')
twice and assert the dd callback was not called; reference the existing test's
use of renderHook, rerender, dispatchKey, and
SequenceManager.getInstance().getRegistrationCount() to locate where to add this
new "should handle dynamic array changes (remove sequence)" case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fa2fc56-22a5-4cab-8139-570bb41c59de
📒 Files selected for processing (15)
docs/framework/angular/reference/functions/injectHotkeySequence.mddocs/framework/angular/reference/interfaces/InjectHotkeySequenceOptions.mdexamples/vue/useHotkeySequences/src/App.vuepackages/angular-hotkeys/src/injectHotkeySequence.tspackages/angular-hotkeys/src/injectHotkeySequences.tspackages/preact-hotkeys/src/useHotkeySequences.tspackages/preact-hotkeys/src/useHotkeys.tspackages/preact-hotkeys/tests/useHotkeySequences.test.tsxpackages/preact-hotkeys/tests/useHotkeys.test.tsxpackages/react-hotkeys/src/useHotkeySequences.tspackages/react-hotkeys/src/useHotkeys.tspackages/react-hotkeys/tests/useHotkeySequences.test.tsxpackages/react-hotkeys/tests/useHotkeys.test.tsxpackages/vue-hotkeys/src/useHotkey.tspackages/vue-hotkeys/src/useHotkeySequence.ts
✅ Files skipped from review due to trivial changes (2)
- docs/framework/angular/reference/functions/injectHotkeySequence.md
- packages/preact-hotkeys/tests/useHotkeySequences.test.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/vue-hotkeys/src/useHotkeySequence.ts
- packages/react-hotkeys/src/useHotkeys.ts
- examples/vue/useHotkeySequences/src/App.vue
- packages/preact-hotkeys/src/useHotkeys.ts
- packages/vue-hotkeys/src/useHotkey.ts
- packages/preact-hotkeys/src/useHotkeySequences.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Behavior Change
Examples
Documentation
Tests
Chores