feat: add ember-hotkeys#117
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new Ember adapter package ChangesEmber Hotkeys Adapter Package
Sequence Diagram(s)sequenceDiagram
participant Template as "Ember Template\n({{on-hotkey}})"
participant Helper as "OnHotkeyHelper\n/ OnHotkeySequenceHelper"
participant Manager as "HotkeyManager\n/ SequenceManager"
participant Document as "document\n(events)"
Template->>Helper: instantiate helper with args
Helper->>Manager: register(hotkey/sequence, callback, options)
Note right of Manager: store registration handle
Document->>Manager: keydown / keyup events
Manager->>Manager: match registrations
Manager-->>Helper: invoke callback(event, context)
Helper-->>Template: execute provided callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 12 minutes and 12 seconds.Comment |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
| "$schema": "https://unpkg.com/knip@5/schema.json", | ||
| "ignoreDependencies": ["@faker-js/faker"], | ||
| "ignoreWorkspaces": ["examples/**"], | ||
| "ignoreWorkspaces": ["examples/**", "packages/ember-hotkeys"], |
There was a problem hiding this comment.
note: Ember uses some virtual modules which all trigger errors. This seems like the most sane approach.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
knip.json (1)
4-4: ⚡ Quick winAvoid fully excluding the new published workspace from Knip checks.
At Line 4, adding
"packages/ember-hotkeys"toignoreWorkspacesdisables unused dependency/export detection for the entire new adapter. Prefer keeping it analyzed and adding targeted ignores under a workspace entry if needed.Suggested config adjustment
- "ignoreWorkspaces": ["examples/**", "packages/ember-hotkeys"], + "ignoreWorkspaces": ["examples/**"], "workspaces": { "packages/react-hotkeys": { "ignore": [] + }, + "packages/ember-hotkeys": { + "ignore": [] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@knip.json` at line 4, Remove "packages/ember-hotkeys" from the top-level "ignoreWorkspaces" array so the new workspace remains subject to Knip checks; instead add a workspace-specific ignore entry for the ember-hotkeys package (using the workspace-specific config key in knip.json) and list only the specific files/dependencies/exports that should be ignored—this preserves full analysis for the adapter while allowing targeted exceptions and references the "ignoreWorkspaces" key and the "packages/ember-hotkeys" workspace name to locate where to change the config.docs/framework/ember/guides/formatting-display.md (1)
22-27: ⚡ Quick win
formatWithLabelsexample omits output annotations.Unlike the
formatForDisplaysection above which shows// Mac: "⌘S" | Windows: "Ctrl+S"output comments, theformatWithLabelsexamples provide no hint of what the output looks like, reducing the guide's usefulness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/ember/guides/formatting-display.md` around lines 22 - 27, The examples for formatWithLabels lack output annotations; update the snippet showing calls to formatWithLabels('Mod+S') and formatWithLabels('Mod+Shift+Z') to include inline comments with the expected returned strings (e.g., Mac and Windows labels like 'Mac: "⌘S" | Windows: "Ctrl+S"' and 'Mac: "⌘⇧Z" | Windows: "Ctrl+Shift+Z"') so readers can see exact output; locate the examples using the formatWithLabels symbol and add the corresponding comment annotations after each call.packages/ember-hotkeys/src/template-registry.ts (1)
4-7: ⚡ Quick winGlint registry merge step is undocumented.
The exported
Registryinterface follows the correct "consumer-side merge" pattern, but none of the new documentation pages (quick-start.md, guides) explain the required global augmentation that activates Glint type-checking. Users who skip this step will get no template type errors.Consider adding a "Glint Setup" section to
quick-start.md:// types/ember-hotkeys.d.ts import type EmberHotkeysRegistry from '@tanstack/ember-hotkeys/template-registry'; declare module '@glint/environment-ember-loose/registry' { export default interface Registry extends EmberHotkeysRegistry {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ember-hotkeys/src/template-registry.ts` around lines 4 - 7, Add a "Glint Setup" section to the quick-start and relevant guides that documents the required global augmentation so Glint picks up the template registry: instruct users to create a declaration file (e.g., in types/) that imports the exported Registry interface from this package (Registry which contains 'on-hotkey' and 'on-hotkey-sequence' referencing OnHotkeyHelper and OnHotkeySequenceHelper) and augments the '@glint/environment-ember-loose/registry' module by extending its exported default Registry with the imported EmberHotkeys Registry; include a short note where to place the file and that restarting type-checking is needed.packages/ember-hotkeys/src/helpers/on-hotkey-sequence.ts (1)
65-67: 💤 Low valueConsider clearing
this.handlewhen skipping registration for an empty sequence.After unregistering in this guard,
this.handleretains a stale reference to the now-unregistered handle. WhilewillDestroycallingunregister()a second time is safe (idempotent Map removal), explicitly nulling out the field makes the invariant clearer:this.handleis truthy if and only if a live registration exists.♻️ Proposed fix
if (!sequence || sequence.length === 0) { + this.handle = undefined; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ember-hotkeys/src/helpers/on-hotkey-sequence.ts` around lines 65 - 67, When the guard in on-hotkey-sequence (the if (!sequence || sequence.length === 0) path) skips registration, clear any stale registration state by calling the existing unregister() (if needed) and then nulling this.handle so the field only points to a live registration; ensure this change keeps willDestroy()’s call to unregister() safe/idempotent and use the class methods/fields this.handle and unregister() to locate and update the code.packages/ember-hotkeys/src/helpers/on-hotkey.ts (1)
66-80: ⚡ Quick win
computealways re-registers instead of usingsetOptions/callbacksetter for in-place updates.
HotkeyOptions.enabledis explicitly documented: "Toggling this should update the existing handle viasetOptionsrather than unregistering." The same applies to callback-only updates (the handle exposes acallbacksetter to avoid stale-closure issues without re-registering). Currently, everycomputeinvocation—including ones where onlyenabledorcallbackchanged—does a fullunregister+register, creating a new registration ID each time and losing devtools continuity.A minimal fix: track the previous hotkey and target, and branch on whether the binding itself changed.
♻️ Proposed refactor
export default class OnHotkeyHelper extends Helper<OnHotkeySignature> { private handle?: HotkeyRegistrationHandle; + private prevHotkey?: RegisterableHotkey; + private prevTarget?: HTMLElement | Document | Window | null; compute( [hotkey, callback]: [RegisterableHotkey, HotkeyCallback], options: OnHotkeyOptions, ): void { - this.handle?.unregister(); - const { enabled = true, target, ...rest } = options; - const manager = getHotkeyManager(); - - this.handle = manager.register(hotkey, callback, { - ...rest, - enabled, - target: target ?? undefined, - }); + + const isSameBinding = + this.handle && + hotkey === this.prevHotkey && + target === this.prevTarget; + + if (isSameBinding) { + this.handle!.setOptions({ ...rest, enabled, target: target ?? undefined }); + this.handle!.callback = callback; + return; + } + + this.handle?.unregister(); + const manager = getHotkeyManager(); + this.handle = manager.register(hotkey, callback, { + ...rest, + enabled, + target: target ?? undefined, + }); + this.prevHotkey = hotkey; + this.prevTarget = target; }Note: for object-type
RegisterableHotkeyvalues,===falls back to reference equality—a new object instance will still trigger re-registration, which is correct. Only string hotkeys (the common case) benefit from the in-place update path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ember-hotkeys/src/helpers/on-hotkey.ts` around lines 66 - 80, compute currently always calls this.handle?.unregister() and re-registers via manager.register, which loses registration identity; instead, cache the previous hotkey and target on the instance (e.g., this._prevHotkey, this._prevTarget) and if the incoming hotkey and normalized target are === to the cached ones then update in-place by setting this.handle.callback = callback (to avoid stale closures) and calling this.handle.setOptions({ enabled, ...rest }) rather than unregistering; only when the hotkey or target changed do the unregister + manager.register flow and update the cached this._prevHotkey/this._prevTarget accordingly. Ensure you normalize target to undefined when comparing (target ?? undefined) to match the register call.
🤖 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/ember/guides/hotkey-recording.md`:
- Around line 17-54: The example leaks a global keydown listener because
recorder.start() can be active when the component is destroyed; add a
willDestroy lifecycle hook on the ShortcutRecorder component that ensures the
HotkeyRecorder is cleaned up (call recorder.cancel() or the recorder's provided
cleanup/stop method) so the listener is removed; update willDestroy to call
this.recorder.cancel() (or this.recorder.stop/removeListener if available) and
set isRecording = false to mirror cancelRecording behavior.
In `@docs/framework/ember/guides/hotkeys.md`:
- Around line 62-74: The docs example incorrectly uses target=this which passes
the component instance rather than an HTMLElement; update the guide to show
capturing the DOM element into a field (e.g., panelEl: HTMLElement | null) via a
did-insert or action like setRef and then pass that field to the onHotkey helper
(target=this.panelEl) so the helper receives an HTMLElement | Document | Window
| null; reference the onHotkey helper, the target argument, and the suggested
names panelEl and setRef in the explanation and example.
- Around line 24-35: The example uses the type HotkeyCallbackContext but does
not import it, so the snippet will not compile; update the example import line
that currently brings in onHotkey (used in the template) to also import
HotkeyCallbackContext from '@tanstack/ember-hotkeys' so the save function
signature (save(event: KeyboardEvent, context: HotkeyCallbackContext)) resolves
correctly and the example compiles.
In `@docs/framework/ember/guides/key-state-tracking.md`:
- Around line 20-101: Both examples import getKeyStateTracker() but never use
it; update the FileActions and KeyDebugger examples to subscribe to the
singleton KeyStateTracker instead of attaching raw document listeners.
Specifically, in the FileActions constructor obtain const tracker =
getKeyStateTracker() and subscribe to tracker.store (or tracker.subscribe) to
update the `@tracked` isShiftHeld property when tracker reports Shift
pressed/released (and unregister the subscription in registerDestructor), and in
KeyDebugger replace the manual document listeners with a subscription to
tracker.store that maps the tracker's held keys into this.heldKeys (also cleaned
up in registerDestructor); keep the same template outputs but demonstrate using
tracker.store so the examples actually use getKeyStateTracker()/tracker.
- Around line 105-112: Update the docs to use the real KeyStateTracker API:
change the property name heldKeyCodes to heldCodes and update the usage examples
to access state via the store (e.g., use getKeyStateTracker() to get tracker and
read tracker.store.state.heldKeys and tracker.store.state.heldCodes) so
references match the KeyStateTracker implementation and exported
getKeyStateTracker function.
In `@docs/framework/ember/quick-start.md`:
- Around line 91-107: The example is missing imports for the onHotkey helper and
the render test helper, which causes Glint/runtime errors; add an import for
onHotkey from '@tanstack/ember-hotkeys/helpers/on-hotkey' and import render from
'@ember/test-helpers', keeping the existing imports for
triggerKeyPress/triggerKeyRelease so the test uses render(...) and the
{{onHotkey "Mod+S" onSave}} helper in scope.
In `@docs/overview.md`:
- Around line 49-54: The "For a complete walkthrough" sentence is missing the
Svelte quick-start link; update the walkthrough sentence that currently lists
React/Angular/Vue/Lit/Ember quick-start links to also include a [Svelte Quick
Start](framework/svelte/quick-start) entry so it matches the frameworks listed
earlier (the modified "React and Preact hooks, ... Svelte actions, and Ember
helpers" line). Ensure the new link follows the same comma/oxford comma
formatting as the others and appears in the same bracket/link form as the
existing quick-start links.
In `@packages/ember-hotkeys/.gitignore`:
- Around line 1-20: Add a POSIX-compliant trailing newline to the .gitignore
file by ensuring the last line ("*.local") ends with a newline character; update
the file so it terminates with a newline (i.e., add an empty line after
"*.local").
In `@packages/ember-hotkeys/README.md`:
- Line 49: The markdown heading level jumps from the top-level H1 ("#") to an H3
("### <a href=\"https://tanstack.com/hotkeys\">Read the docs →</a>") which
triggers MD001; change that H3 to an H2 (replace the triple-# with double-# for
the "Read the docs →" heading) so headings increment by one and the file's
heading hierarchy is valid.
- Around line 14-16: The README's bundle size badge and link use the wrong
package name; update the anchor href and the img src (the <a href="..."> and
<img src="..."> entries) to reference "@tanstack/ember-hotkeys" instead of
"@tanstack/react-hotkeys" so the badge and link show the Ember adapter's bundle
stats.
In `@packages/ember-hotkeys/src/test-support.ts`:
- Around line 30-39: The synthetic KeyboardEvent currently sets code: parsed.key
which yields semantically incorrect event.code values; update both
triggerKeyPress and triggerKeyRelease where the KeyboardEvent is constructed to
remove the code property (i.e., do not set code at all) so the event uses the
default empty string instead of an incorrect value, leaving parsed.key only on
the key property and keeping all other modifier/bubble/cancelable fields
unchanged.
In `@packages/ember-hotkeys/tests/integration/test-support-test.gts`:
- Around line 19-31: The test fails on non-mac CI because the hotkey is
registered with platform="mac" but triggerKeyPress('Mod+S') calls parseHotkey
without a platform and auto-detects the runtime platform, causing modifier
mismatch; fix by removing the explicit platform="mac" from the template so the
onHotkey registration and triggerKeyPress both use the same auto-detected
platform (alternatively pass the same platform into triggerKeyPress), ensuring
parseHotkey/modifier flags match when matchesKeyboardEvent compares them.
---
Nitpick comments:
In `@docs/framework/ember/guides/formatting-display.md`:
- Around line 22-27: The examples for formatWithLabels lack output annotations;
update the snippet showing calls to formatWithLabels('Mod+S') and
formatWithLabels('Mod+Shift+Z') to include inline comments with the expected
returned strings (e.g., Mac and Windows labels like 'Mac: "⌘S" | Windows:
"Ctrl+S"' and 'Mac: "⌘⇧Z" | Windows: "Ctrl+Shift+Z"') so readers can see exact
output; locate the examples using the formatWithLabels symbol and add the
corresponding comment annotations after each call.
In `@knip.json`:
- Line 4: Remove "packages/ember-hotkeys" from the top-level "ignoreWorkspaces"
array so the new workspace remains subject to Knip checks; instead add a
workspace-specific ignore entry for the ember-hotkeys package (using the
workspace-specific config key in knip.json) and list only the specific
files/dependencies/exports that should be ignored—this preserves full analysis
for the adapter while allowing targeted exceptions and references the
"ignoreWorkspaces" key and the "packages/ember-hotkeys" workspace name to locate
where to change the config.
In `@packages/ember-hotkeys/src/helpers/on-hotkey-sequence.ts`:
- Around line 65-67: When the guard in on-hotkey-sequence (the if (!sequence ||
sequence.length === 0) path) skips registration, clear any stale registration
state by calling the existing unregister() (if needed) and then nulling
this.handle so the field only points to a live registration; ensure this change
keeps willDestroy()’s call to unregister() safe/idempotent and use the class
methods/fields this.handle and unregister() to locate and update the code.
In `@packages/ember-hotkeys/src/helpers/on-hotkey.ts`:
- Around line 66-80: compute currently always calls this.handle?.unregister()
and re-registers via manager.register, which loses registration identity;
instead, cache the previous hotkey and target on the instance (e.g.,
this._prevHotkey, this._prevTarget) and if the incoming hotkey and normalized
target are === to the cached ones then update in-place by setting
this.handle.callback = callback (to avoid stale closures) and calling
this.handle.setOptions({ enabled, ...rest }) rather than unregistering; only
when the hotkey or target changed do the unregister + manager.register flow and
update the cached this._prevHotkey/this._prevTarget accordingly. Ensure you
normalize target to undefined when comparing (target ?? undefined) to match the
register call.
In `@packages/ember-hotkeys/src/template-registry.ts`:
- Around line 4-7: Add a "Glint Setup" section to the quick-start and relevant
guides that documents the required global augmentation so Glint picks up the
template registry: instruct users to create a declaration file (e.g., in types/)
that imports the exported Registry interface from this package (Registry which
contains 'on-hotkey' and 'on-hotkey-sequence' referencing OnHotkeyHelper and
OnHotkeySequenceHelper) and augments the
'@glint/environment-ember-loose/registry' module by extending its exported
default Registry with the imported EmberHotkeys Registry; include a short note
where to place the file and that restarting type-checking is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a533bb3-8549-4de5-9e91-f50e0d06589b
⛔ Files ignored due to path filters (2)
docs/framework/ember/reference/index.mdis excluded by!docs/**/reference/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (41)
.changeset/add-ember-hotkeys.mdREADME.mddocs/config.jsondocs/framework/ember/guides/formatting-display.mddocs/framework/ember/guides/hotkey-recording.mddocs/framework/ember/guides/hotkeys.mddocs/framework/ember/guides/key-state-tracking.mddocs/framework/ember/guides/sequence-recording.mddocs/framework/ember/guides/sequences.mddocs/framework/ember/quick-start.mddocs/installation.mddocs/overview.mdknip.jsonpackage.jsonpackages/ember-hotkeys/.env.developmentpackages/ember-hotkeys/.gitignorepackages/ember-hotkeys/.template-lintrc.mjspackages/ember-hotkeys/README.mdpackages/ember-hotkeys/addon-main.cjspackages/ember-hotkeys/babel.config.cjspackages/ember-hotkeys/babel.publish.config.cjspackages/ember-hotkeys/config/ember-cli-update.jsonpackages/ember-hotkeys/eslint.config.mjspackages/ember-hotkeys/package.jsonpackages/ember-hotkeys/pnpm-workspace.yamlpackages/ember-hotkeys/rollup.config.mjspackages/ember-hotkeys/src/helpers/on-hotkey-sequence.tspackages/ember-hotkeys/src/helpers/on-hotkey.tspackages/ember-hotkeys/src/index.tspackages/ember-hotkeys/src/template-registry.tspackages/ember-hotkeys/src/test-support.tspackages/ember-hotkeys/testem.cjspackages/ember-hotkeys/tests/index.htmlpackages/ember-hotkeys/tests/integration/helpers/on-hotkey-sequence-test.gtspackages/ember-hotkeys/tests/integration/helpers/on-hotkey-test.gtspackages/ember-hotkeys/tests/integration/test-support-test.gtspackages/ember-hotkeys/tests/test-helper.tspackages/ember-hotkeys/tsconfig.jsonpackages/ember-hotkeys/tsconfig.publish.jsonpackages/ember-hotkeys/unpublished-development-types/index.d.tspackages/ember-hotkeys/vite.config.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ember-hotkeys/README.md`:
- Around line 8-9: Update the npm badge anchor and image to point to the Ember
adapter package instead of the core package: replace the href value
"https://www.npmjs.com/package/@tanstack/hotkeys" and the shield image src
"https://img.shields.io/npm/dm/@tanstack/hotkeys.svg" with the equivalent URLs
for "@tanstack/ember-hotkeys" (and update the img alt text if present) so the
README's anchor and <img> show stats for `@tanstack/ember-hotkeys`.
In `@packages/ember-hotkeys/src/test-support.ts`:
- Line 24: Update the JSDoc examples for triggerKeyPress and triggerKeyRelease
to show callers awaiting Ember's settled() after dispatching; specifically note
that these functions dispatch events synchronously but hotkey handlers often
trigger async Ember work (route transitions, data loads, tracked updates), so
example usage should be "triggerKeyPress('Mod+S'); await settled();" (and
similarly for triggerKeyRelease) to ensure assertions run after side-effects
complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4767f98-67ce-4621-8a77-69e2c07c8e38
📒 Files selected for processing (6)
docs/framework/ember/guides/hotkey-recording.mddocs/framework/ember/guides/hotkeys.mdpackages/ember-hotkeys/.gitignorepackages/ember-hotkeys/README.mdpackages/ember-hotkeys/src/test-support.tspackages/ember-hotkeys/tests/integration/test-support-test.gts
✅ Files skipped from review due to trivial changes (3)
- packages/ember-hotkeys/.gitignore
- docs/framework/ember/guides/hotkey-recording.md
- packages/ember-hotkeys/tests/integration/test-support-test.gts
Co-authored-by: Copilot <copilot@github.com>
🎯 Changes
Adds
ember-hotkeyspackage. This features two helpers, and test suite helpers for easier testing.#112
disclaimer: development supported with AI, but reviewed and tested by a human!
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit