feat: add lit adapater packages#59
Merged
KevinVandy merged 8 commits intoTanStack:mainfrom Mar 29, 2026
Merged
Conversation
fredericbahr
changed the title
![chatgpt-codex-connector[bot]](https://avatars.gh.lumenfield.work/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c2183d968
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fredericbahr
force-pushed
the
feat-lit-adapter
branch
from
0c2183d to
af261b1
Compare
KevinVandy
changed the title
@tanstack/angular-hotkeys
@tanstack/hotkeys
@tanstack/hotkeys-devtools
@tanstack/lit-hotkeys
@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: |
|
|
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:
📝 WalkthroughWalkthroughAdds a new Lit adapter package ( Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Lit Element
participant Controller as HotkeyController
participant Manager as HotkeyManager
participant Target as Document/Element
Host->>Controller: addController(controller)
Host->>Controller: hostConnected()
Controller->>Controller: resolve target (options.target or document)
Controller->>Controller: normalize hotkey & bind callback to host
Controller->>Manager: register(hotkey, callback, options)
Manager->>Target: addEventListener(keydown/keyup)
Manager-->>Controller: return registration handle
Note right of Target: User presses keys
Target->>Manager: keydown event
Manager->>Manager: match registered hotkeys
Manager->>Host: invoke callback(event, context)
Host->>Controller: hostDisconnected()
Controller->>Manager: unregister(handle)
Manager->>Target: removeEventListener
sequenceDiagram
participant Host as Lit Element
participant Decorator as `@hotkeySequence`
participant Controller as HotkeySequenceController
participant Manager as SequenceManager
participant Target as Document/Element
Decorator->>Host: wrap connectedCallback
Host->>Decorator: connectedCallback()
Decorator->>Controller: new HotkeySequenceController(host, sequence, cb, options)
Host->>Controller: addController(controller)
Host->>Controller: hostConnected()
Controller->>Controller: resolve target & bind callback
Controller->>Manager: registerSequence(sequence, callback, options)
Manager->>Target: listen for sequence events
Note right of Target: user types sequence (e.g., "G" then "G")
Target->>Manager: keydown events
Manager->>Manager: track progress, apply timeout rules
alt sequence complete
Manager->>Host: execute callback
end
Host->>Controller: hostDisconnected()
Controller->>Manager: unregisterSequence(handle)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
examples/lit/hotkey-sequence/src/index.css (2)
137-149: Add explicit keyboard focus styling for buttons.You already style hover; adding
:focus-visiblekeeps keyboard navigation clear and consistent.Suggested patch
button:hover { background: `#0052a3`; } + +button:focus-visible { + outline: 2px solid `#0066cc`; + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-sequence/src/index.css` around lines 137 - 149, Add explicit keyboard focus styling for the existing button rules: implement a focus-visible style for the button selector (e.g., target button:focus-visible) that provides a clear outline or box-shadow and visible contrast (and optionally include a fallback button:focus for browsers without focus-visible) so keyboard users see focused state consistent with the hover state; update the rules near the existing button and button:hover selectors to include these focus styles while keeping current colors and rounded corners.
93-97: Increase hint text contrast for readability.This hint style is quite low-contrast at small size; darkening it a bit improves legibility without changing layout.
Suggested patch
.hint { font-size: 12px; - color: `#888`; + color: `#666`; font-style: italic; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-sequence/src/index.css` around lines 93 - 97, The .hint rule in examples/lit/hotkey-sequence/src/index.css uses a low-contrast color (`#888`) that reduces readability at small sizes; update the .hint CSS rule to use a darker color (for example `#666` or `#444`) to improve contrast while keeping font-size and font-style the same so layout is unaffected.examples/lit/hotkey-sequence/package.json (1)
14-14: Update dependency to use workspace protocol to ensure example consumes local package.The example currently uses
"@tanstack/lit-hotkeys": "^0.4.0", which may resolve to a published version from npm rather than the local workspace package. To dogfood current branch changes, useworkspace:*orworkspace:^instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-sequence/package.json` at line 14, Update the dependency entry for "@tanstack/lit-hotkeys" in the example's package.json to use the workspace protocol so it consumes the local package instead of the published npm version; replace the version string "^0.4.0" with a workspace spec such as "workspace:*" (or "workspace:^" if you want caret behavior) to ensure the example dogfoods the local package.examples/lit/hotkey/src/app.ts (1)
410-420: Arrow function property vs method for hotkey handlers.Using arrow function class properties (like
_handleSaving) with decorators can have subtle differences from regular methods. The decorator accessesdescriptor?.valuewhich may beundefinedfor arrow function properties since they're initialized in the constructor, not defined on the prototype.The fallback
host[propertyKey]on line 52 of the decorator handles this, but it's worth noting the inconsistency across handlers (some use arrow functions, some use methods).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey/src/app.ts` around lines 410 - 420, The _handleSaving handler is defined as an arrow function property which may not appear on the prototype for the hotkey decorator to inspect (descriptor?.value can be undefined); convert _handleSaving into a normal class method (e.g., private _handleSaving(event: KeyboardEvent, ctx: HotkeyCallbackContext) { ... }) so the decorator can access it via the descriptor, and update any usages that relied on lexical binding (if necessary bind in constructor or use the decorator fallback). Ensure consistency with other handlers by making them methods too so the decorator's descriptor-based logic works uniformly.packages/lit-hotkeys/src/decorators/hotkey.ts (1)
29-32: Default options are replaced, not merged.When a user passes partial options like
{ requireReset: true }, the entireHOTKEY_DEFAULT_OPTIONSobject is replaced rather than merged. This means other sensible defaults (e.g.,preventDefault: true) won't apply unless explicitly provided.Consider spreading defaults:
♻️ Proposed fix to merge defaults
export function hotkey( hotkey: RegisterableHotkey, - options: HotkeyOptions = HOTKEY_DEFAULT_OPTIONS, + options: Partial<HotkeyOptions> = {}, ) { + const mergedOptions: HotkeyOptions = { ...HOTKEY_DEFAULT_OPTIONS, ...options } return function <T extends HotkeyCallback>( ... const controller: HotkeyController = new HotkeyController( this, hotkey, callback, - options, + mergedOptions, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/decorators/hotkey.ts` around lines 29 - 32, The hotkey function currently replaces HOTKEY_DEFAULT_OPTIONS when callers pass partial HotkeyOptions; update the function to merge defaults with provided options so missing keys keep defaults (e.g., combine HOTKEY_DEFAULT_OPTIONS and the incoming options before using them). Locate the hotkey function and ensure you merge HOTKEY_DEFAULT_OPTIONS with the parameter named options (or set a mergedOptions local variable) so flags like preventDefault remain true unless explicitly overridden; keep the parameter type HotkeyOptions and use the merged result throughout the function.packages/lit-hotkeys/src/controllers/hotkey-sequence.ts (1)
48-78: Consider the same default options merging pattern.Similar to the
@hotkeydecorator, the constructor usesoptions = HOTKEY_SEQUENCE_DEFAULT_OPTIONSwhich replaces rather than merges. When users provide partial options like{ timeout: 500 }, other defaults won't apply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/controllers/hotkey-sequence.ts` around lines 48 - 78, The current logic in hostConnected relies on this.options but the constructor used options = HOTKEY_SEQUENCE_DEFAULT_OPTIONS which overwrites defaults; instead merge user options with HOTKEY_SEQUENCE_DEFAULT_OPTIONS so missing defaults remain (use something like mergedOptions = { ...HOTKEY_SEQUENCE_DEFAULT_OPTIONS, ...this.options }), then use mergedOptions when extracting enabled/target and when passing options to manager.register; update references in hostConnected (replace uses of this.options with mergedOptions) and ensure target resolution and optionsWithoutTarget are derived from the merged object.packages/lit-hotkeys/src/decorators/hotkey-sequence.ts (1)
34-37: Inconsistency:descriptoris required here but optional in@hotkey.In
hotkey.tsline 36,descriptorisTypedPropertyDescriptor<T> | undefined, allowing the decorator to work on both methods and class field properties. Here it's required, which may cause issues when decorating arrow function class fields.♻️ Proposed fix for consistency
return function <T extends HotkeyCallback>( proto: LitElement, methodName: string, - descriptor: TypedPropertyDescriptor<T>, + descriptor?: TypedPropertyDescriptor<T>, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/decorators/hotkey-sequence.ts` around lines 34 - 37, The decorator's inner function currently requires descriptor but should accept an optional descriptor to match `@hotkey` and support arrow-function class fields; change the signature in the returned function to accept descriptor?: TypedPropertyDescriptor<T> | undefined (or descriptor: TypedPropertyDescriptor<T> | undefined) and then update any code that reads/writes descriptor.value or descriptor.initializer to safely handle descriptor being undefined (create/assign a proper descriptor when decorating a class field or use proto[methodName] fallback). Ensure the returned function (the decorator in hotkey-sequence.ts) mirrors the optional descriptor handling used in hotkey.ts so both method and field/arrow-function decorations work consistently.
🤖 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/lit/hotkey/package.json`:
- Around line 13-15: Replace the external semver for the internal package
`@tanstack/lit-hotkeys` in the example package.json (currently "^0.4.0") with the
workspace protocol "workspace:*" so pnpm resolves the local package; make the
same change for the other lit example's package.json (the hotkey-sequence
example) to ensure both use "workspace:*" for `@tanstack/lit-hotkeys`.
In `@examples/lit/hotkey/src/app.ts`:
- Around line 523-528: The hotkey decorator value and the displayed lastHotkey
are inconsistent in the _f4 handler: `@hotkey`('Meta+F4') registers Meta+F4 but
the handler sets this.lastHotkey = 'Alt+F4' (method _f4), causing UI confusion;
fix by making the two match—either change the decorator on _f4 to
`@hotkey`('Alt+F4') to match the UI and alert text, or update this.lastHotkey and
the alert text to 'Meta+F4' so they consistently reflect the registered hotkey.
In `@packages/lit-hotkeys/CHANGELOG.md`:
- Around line 1-111: The CHANGELOG.md currently has the wrong package header
"@tanstack/react-hotkeys" and mismatched entries for this package; update the
file (CHANGELOG.md) to target "lit-hotkeys" by replacing the title string and
regenerating or replacing the changelog content with the correct lit-hotkeys
release history, ensuring the top-level header reads "lit-hotkeys" (or the
package's exact name) and entries correspond to lit-hotkeys releases rather than
`@tanstack/react-hotkeys`.
In `@packages/lit-hotkeys/src/controllers/hotkey-sequence.ts`:
- Line 72: Remove the debug console.log call that prints 'optionsWithoutTarget'
in the HotkeySequence controller (the line referencing this.sequence and
optionsWithoutTarget); locate the statement in the hotkey-sequence.ts
implementation (inside the HotkeySequence class / relevant method where
this.sequence and optionsWithoutTarget are used) and delete that console.log so
no debug output remains in production.
In `@packages/lit-hotkeys/src/controllers/hotkey.ts`:
- Around line 47-74: HOTKEY_DEFAULT_OPTIONS must be made server-safe by removing
any module-scope reference to document (e.g., change the default target from
document to undefined or null in packages/lit-hotkeys/src/constants.ts) and keep
hostConnected() as the place that chooses document at runtime; then ensure
hostConnected() uses the existing runtime check ('target' in this.options ?
(this.options.target ?? null) : typeof document !== "undefined" ? document :
null) to lazily resolve to document only when typeof document !== "undefined" so
that HOTKEY_DEFAULT_OPTIONS, HotkeyOptions, and the hostConnected() logic no
longer import a browser global at module load (adjust HotkeyOptions type if
needed to allow undefined/null for options.target).
In `@packages/lit-hotkeys/src/index.ts`:
- Line 4: The comment "// React-specific exports" is stale and misleading for
this Lit-focused package; update that comment in
packages/lit-hotkeys/src/index.ts by replacing the exact string "//
React-specific exports" with a Lit-appropriate comment such as "// Lit-specific
exports" or "// Exports for Lit / LitElement consumers" so the comment
accurately reflects the package intent and exports.
In `@packages/lit-hotkeys/tsconfig.json`:
- Around line 2-4: The package tsconfig is missing experimental decorators which
breaks compilation of the legacy decorator syntax used by `@hotkey` and
`@hotkeySequence` (they use TypedPropertyDescriptor parameters); open the
packages/lit-hotkeys tsconfig (the object containing
"extends"/"include"/"exclude") and add "experimentalDecorators": true under
compilerOptions so TypeScript accepts legacy decorators; ensure the
compilerOptions section exists (create it if needed) and keep other options
intact.
In `@packages/lit-hotkeys/vitest.config.ts`:
- Around line 2-6: The vitest config incorrectly imports and applies the React
Vite plugin; remove the import statement "import react from
'@vitejs/plugin-react'" and remove the react() entry from the plugins array in
the default export (the plugins field in the defineConfig block) so the Lit
package's tests do not load React-specific tooling; if a Lit-specific Vite
plugin is required, add that instead, otherwise leave plugins empty or only
include plugins actually declared in this package.json.
---
Nitpick comments:
In `@examples/lit/hotkey-sequence/package.json`:
- Line 14: Update the dependency entry for "@tanstack/lit-hotkeys" in the
example's package.json to use the workspace protocol so it consumes the local
package instead of the published npm version; replace the version string
"^0.4.0" with a workspace spec such as "workspace:*" (or "workspace:^" if you
want caret behavior) to ensure the example dogfoods the local package.
In `@examples/lit/hotkey-sequence/src/index.css`:
- Around line 137-149: Add explicit keyboard focus styling for the existing
button rules: implement a focus-visible style for the button selector (e.g.,
target button:focus-visible) that provides a clear outline or box-shadow and
visible contrast (and optionally include a fallback button:focus for browsers
without focus-visible) so keyboard users see focused state consistent with the
hover state; update the rules near the existing button and button:hover
selectors to include these focus styles while keeping current colors and rounded
corners.
- Around line 93-97: The .hint rule in
examples/lit/hotkey-sequence/src/index.css uses a low-contrast color (`#888`) that
reduces readability at small sizes; update the .hint CSS rule to use a darker
color (for example `#666` or `#444`) to improve contrast while keeping font-size and
font-style the same so layout is unaffected.
In `@examples/lit/hotkey/src/app.ts`:
- Around line 410-420: The _handleSaving handler is defined as an arrow function
property which may not appear on the prototype for the hotkey decorator to
inspect (descriptor?.value can be undefined); convert _handleSaving into a
normal class method (e.g., private _handleSaving(event: KeyboardEvent, ctx:
HotkeyCallbackContext) { ... }) so the decorator can access it via the
descriptor, and update any usages that relied on lexical binding (if necessary
bind in constructor or use the decorator fallback). Ensure consistency with
other handlers by making them methods too so the decorator's descriptor-based
logic works uniformly.
In `@packages/lit-hotkeys/src/controllers/hotkey-sequence.ts`:
- Around line 48-78: The current logic in hostConnected relies on this.options
but the constructor used options = HOTKEY_SEQUENCE_DEFAULT_OPTIONS which
overwrites defaults; instead merge user options with
HOTKEY_SEQUENCE_DEFAULT_OPTIONS so missing defaults remain (use something like
mergedOptions = { ...HOTKEY_SEQUENCE_DEFAULT_OPTIONS, ...this.options }), then
use mergedOptions when extracting enabled/target and when passing options to
manager.register; update references in hostConnected (replace uses of
this.options with mergedOptions) and ensure target resolution and
optionsWithoutTarget are derived from the merged object.
In `@packages/lit-hotkeys/src/decorators/hotkey-sequence.ts`:
- Around line 34-37: The decorator's inner function currently requires
descriptor but should accept an optional descriptor to match `@hotkey` and support
arrow-function class fields; change the signature in the returned function to
accept descriptor?: TypedPropertyDescriptor<T> | undefined (or descriptor:
TypedPropertyDescriptor<T> | undefined) and then update any code that
reads/writes descriptor.value or descriptor.initializer to safely handle
descriptor being undefined (create/assign a proper descriptor when decorating a
class field or use proto[methodName] fallback). Ensure the returned function
(the decorator in hotkey-sequence.ts) mirrors the optional descriptor handling
used in hotkey.ts so both method and field/arrow-function decorations work
consistently.
In `@packages/lit-hotkeys/src/decorators/hotkey.ts`:
- Around line 29-32: The hotkey function currently replaces
HOTKEY_DEFAULT_OPTIONS when callers pass partial HotkeyOptions; update the
function to merge defaults with provided options so missing keys keep defaults
(e.g., combine HOTKEY_DEFAULT_OPTIONS and the incoming options before using
them). Locate the hotkey function and ensure you merge HOTKEY_DEFAULT_OPTIONS
with the parameter named options (or set a mergedOptions local variable) so
flags like preventDefault remain true unless explicitly overridden; keep the
parameter type HotkeyOptions and use the merged result throughout the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1535cb6b-4154-4a0a-8ada-e423d6cf8f8a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
examples/lit/hotkey-sequence/.gitignoreexamples/lit/hotkey-sequence/index.htmlexamples/lit/hotkey-sequence/package.jsonexamples/lit/hotkey-sequence/src/app.tsexamples/lit/hotkey-sequence/src/index.cssexamples/lit/hotkey-sequence/tsconfig.jsonexamples/lit/hotkey/index.htmlexamples/lit/hotkey/package.jsonexamples/lit/hotkey/src/app.tsexamples/lit/hotkey/src/index.cssexamples/lit/hotkey/tsconfig.jsonpackage.jsonpackages/lit-hotkeys/CHANGELOG.mdpackages/lit-hotkeys/README.mdpackages/lit-hotkeys/eslint.config.jspackages/lit-hotkeys/package.jsonpackages/lit-hotkeys/src/constants.tspackages/lit-hotkeys/src/controllers/held-keys.tspackages/lit-hotkeys/src/controllers/hotkey-sequence.tspackages/lit-hotkeys/src/controllers/hotkey.tspackages/lit-hotkeys/src/controllers/index.tspackages/lit-hotkeys/src/controllers/key-hold.tspackages/lit-hotkeys/src/decorators/hotkey-sequence.tspackages/lit-hotkeys/src/decorators/hotkey.tspackages/lit-hotkeys/src/decorators/index.tspackages/lit-hotkeys/src/index.tspackages/lit-hotkeys/tests/useHotkey.test.tsxpackages/lit-hotkeys/tsconfig.docs.jsonpackages/lit-hotkeys/tsconfig.jsonpackages/lit-hotkeys/tsdown.config.tspackages/lit-hotkeys/vitest.config.ts
fredericbahr
force-pushed
the
feat-lit-adapter
branch
3 times, most recently
from
d6fc21d to
ee07fc6
Compare
Member
|
Is this still in draft? |
Contributor
Author
Yes still in draft. I need to implement the recorder and then write the documentation for the lit adapter. Update from 27.03:
|
fredericbahr
force-pushed
the
feat-lit-adapter
branch
3 times, most recently
from
957eb41 to
c2f4b71
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
examples/lit/held-keys/src/app.ts (1)
41-51: Consider usingwillUpdateor a controller subscription for history tracking.Using
updated()to modify reactive state (this.history) can cause an additional render cycle. While the deduplication check (last !== combo) prevents infinite loops, this pattern triggers unnecessary re-renders when other state changes while keys are held.A cleaner approach would be to track history via a subscription in the controller or use
willUpdatewithchangedPropertiesto only act whenheldKeyschanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/held-keys/src/app.ts` around lines 41 - 51, The component currently mutates reactive state inside updated() by setting this.history when heldKeys changes, causing extra render cycles; instead, move the history tracking into willUpdate (check changedProperties.has('heldKeys')) or into the heldKeys controller subscription so you only update history when heldKeys actually changes—refactor the logic in updated() (the combo computation and dedupe using last !== combo) into willUpdate or a controller subscriber and assign this.history there to avoid triggering an extra render.examples/lit/hotkey-sequence/src/app.ts (1)
200-203: Consider using@hotkeyfor single-key shortcuts.
@hotkeySequence(['Shift+G'])registers a single-element sequence. While this works, using@hotkey('Shift+G')would be more semantically appropriate for a single chord and avoids the sequence timeout overhead.Suggested change
- `@hotkeySequence`(['Shift+G']) - private _shiftG() { + `@hotkey`('Shift+G') + private _shiftG(_event: KeyboardEvent, _ctx: HotkeyCallbackContext) { this.addToHistory('G → Go to bottom') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-sequence/src/app.ts` around lines 200 - 203, The handler _shiftG is currently decorated with `@hotkeySequence`(['Shift+G']) even though it's a single-chord shortcut; change the decorator to `@hotkey`('Shift+G') on the _shiftG method (and update any imports if necessary) so the single-key chord uses the hotkey decorator instead of the sequence implementation to avoid sequence timeout overhead and better express intent.docs/framework/lit/guides/formatting-display.md (1)
97-129: Controller instantiation infirstUpdatedmay miss initial connection.Creating
HotkeyControllerinfirstUpdatedmeans the controller'shostConnectedwon't be called automatically (sinceconnectedCallbackhas already fired). Consider instantiating the controller as a class field or in the constructor for proper lifecycle integration.Suggested alternative pattern
`@customElement`('menu-item') class MenuItem extends LitElement { `@property`({ type: String }) label = '' `@property`({ type: String }) hotkey = '' private hotkeyController?: HotkeyController connectedCallback() { super.connectedCallback() if (this.hotkey) { this.hotkeyController = new HotkeyController( this, this.hotkey, () => this.dispatchEvent(new CustomEvent('action', { bubbles: true, composed: true })), ) } } render() { return html` <div class="menu-item"> <span>${this.label}</span> <span class="menu-shortcut">${formatForDisplay(this.hotkey)}</span> </div> ` } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/lit/guides/formatting-display.md` around lines 97 - 129, The HotkeyController is being created in firstUpdated so its hostConnected lifecycle hook may be missed; move the controller creation out of firstUpdated and instantiate it either as a class field, in the constructor, or in connectedCallback so Lit will call the controller lifecycle methods automatically. Specifically, stop calling new HotkeyController(...) inside firstUpdated and instead create and add the controller (via addController or keeping a private hotkeyController field) in the constructor or connectedCallback, referencing the HotkeyController, addController, firstUpdated, connectedCallback, constructor, and the hotkey property to locate the code to change.examples/lit/hotkey-recorder/src/index.css (1)
87-100: Add a reduced-motion fallback for the recording pulse.Both recording states use infinite animation with no
prefers-reduced-motionescape hatch. For a keyboard-centric demo, that’s an avoidable accessibility rough edge.♿ Suggested follow-up
`@keyframes` recordingPulse { 0%, 100% { box-shadow: 0 0 0 3px rgba(255, 193, 7, 0.2); } 50% { box-shadow: 0 0 0 6px rgba(255, 193, 7, 0.1); } } + +@media (prefers-reduced-motion: reduce) { + .recording-notice, + .shortcut-item.recording { + animation: none; + } +}Also applies to: 121-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-recorder/src/index.css` around lines 87 - 100, The recording pulse currently runs infinitely via the .recording-notice rule and `@keyframes` pulse with no accessibility fallback; update the CSS to respect the user's prefers-reduced-motion preference by adding a media query (prefers-reduced-motion: reduce) that disables or replaces the animation for .recording-notice (and any other recording state at lines ~121-135) — for example, remove animation or set it to none and ensure any visual state remains visible without motion while keeping the pulse keyframes intact for non-reduced-motion users.
🤖 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/config.json`:
- Around line 240-264: Add the missing "Hotkey Sequence Recording" guide entry
to the "lit" section's children array: create a new child object with "label":
"Hotkey Sequence Recording" and "to":
"framework/lit/guides/hotkey-sequence-recording" and insert it alongside the
other guide entries (e.g., near "Hotkey Recording" / "Key State Tracking") so
the "label": "lit" -> "children" list matches the other frameworks; ensure the
slug matches the other frameworks' naming convention.
In `@docs/framework/lit/guides/hotkeys.md`:
- Around line 184-214: The decorator uses this.panelRef.value at decoration time
which is undefined, so replace the decorator-based registration with deferred
registration: create a panelRef via createRef, remove `@hotkey`(...) on
closePanel, and instead instantiate a HotkeyController (or call hotkey
registration) inside firstUpdated (or connectedCallback) where panelRef.value is
available; reference the panelRef, HotkeyController (or your library's
registration API), and the closePanel handler to wire the 'Escape' key with {
target: this.panelRef.value } so the hotkey is registered against the actual DOM
node.
- Around line 48-74: The constructor of MyEditor redundantly calls
this.addController(this.saveHotkey) even though HotkeyController's constructor
already registers itself with the host; remove the explicit addController call
from the MyEditor constructor so the controller (saveHotkey) is only added once
and rely on HotkeyController to attach itself to the host.
In `@examples/lit/held-keys/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" so only "margin:
0 auto" remains in the "header p" rule to keep the intended centering and
eliminate the overridden property.
In `@examples/lit/hotkey-recorder/src/app.ts`:
- Around line 127-140: The loop is treating an intentionally cleared shortcut
(empty string) as falsy because it uses the || operator when selecting a hotkey;
update the selection to explicitly check for undefined/null so that
this._shortcuts[actionId] === '' is honored. In the block iterating
DEFAULT_SHORTCUT_ACTIONS, replace the fallback logic for hotkey (currently using
(this._shortcuts[actionId] || action.defaultHotkey)) with an explicit
null/undefined check (e.g., use a conditional that picks
this._shortcuts[actionId] if it is not null/undefined, otherwise
action.defaultHotkey) so cleared values from onClear are registered as
empty/cleared; keep the rest of the registration flow (manager.register,
callback, this._registrations.push) unchanged.
In `@examples/lit/hotkey-recorder/src/index.css`:
- Around line 14-18: The CSS rule for selector "header p" contains a redundant
margin declaration ("margin: 0;" followed by "margin: 0 auto;"); remove the
first "margin: 0;" so the block only includes the intended "margin: 0 auto;"
(keep color and max-width unchanged).
- Around line 121-135: The keyframe name recordingPulse violates the stylesheet
naming rule; rename the keyframe and its usage to kebab-case (e.g.,
recording-pulse) so the `@keyframes` block and the animation reference on
.shortcut-item.recording stay in sync—update the animation property on the
.shortcut-item.recording rule to use the new kebab-case name and rename the
`@keyframes` declaration accordingly (ensure both the identifier in the animation:
... and the `@keyframes` recordingPulse are changed together).
In `@examples/lit/hotkey/src/app.ts`:
- Around line 90-119: The demo code snippets show React patterns (setCount,
useState, and an enabled prop) which don't match this Lit example; replace those
React-specific usages with the Lit class-based equivalents used in this file:
use the component's reactive property (enabled) and the class method handler
used with the `@hotkey` decorator (the existing decorated handler method name in
this file) instead of setCount/useState, and render/flip the component's enabled
property directly in the code blocks so the displayed snippets mirror the actual
Lit implementation (reference: the `@hotkey` decorator, the component's enabled
reactive property, and the decorated handler method).
- Around line 45-54: HotkeyController is only rendered as text and never wired
to the refs, so sidebarRef, modalRef, and editorRef never get scoped shortcuts
and the counters/modal behavior never update; create a HotkeyController instance
in the component lifecycle (e.g., firstUpdated or connectedCallback), register
scoped shortcuts against sidebarRef.current, modalRef.current, and
editorRef.current using the HotkeyController API (attach handlers that increment
sidebarShortcutCount/modalShortcutCount/editorShortcutCount, set modalOpen =
false for Escape, and update/save editorContent for Mod+S), ensure each handler
calls event.preventDefault()/stopImmediatePropagation() to avoid falling back to
global handlers, and unregister/cleanup the controller in disconnectedCallback;
reference HotkeyController, sidebarRef, modalRef, editorRef,
sidebarShortcutCount, modalShortcutCount, editorShortcutCount, modalOpen, and
editorContent when implementing.
- Around line 123-199: Summary: The demo uses system/broswer-intercepted
shortcuts (e.g., 'Mod+1', 'Alt+ArrowLeft', 'Control+PageUp', 'Alt+F4', 'F12',
'Control+Tab') which will appear to "fail" in the browser. Fix: In the Number
Key Combinations, Navigation Key Combinations, and Function Key Combinations
sections (where formatForDisplay is used and the sample `@hotkey` strings like
'Mod+1', 'Alt+ArrowLeft', 'Control+PageUp', 'Alt+F4', 'F12' appear), either swap
those strings to browser-safe alternatives (choose combos not reserved by
browsers/OS) and update the matching example `@hotkey`(...) snippets, or add a
visible note/label near each section saying these are illustrative and may be
intercepted by the browser/OS; ensure consistency between display
(formatForDisplay calls) and the actual `@hotkey` example strings such as those in
the pre.code-blocks.
---
Nitpick comments:
In `@docs/framework/lit/guides/formatting-display.md`:
- Around line 97-129: The HotkeyController is being created in firstUpdated so
its hostConnected lifecycle hook may be missed; move the controller creation out
of firstUpdated and instantiate it either as a class field, in the constructor,
or in connectedCallback so Lit will call the controller lifecycle methods
automatically. Specifically, stop calling new HotkeyController(...) inside
firstUpdated and instead create and add the controller (via addController or
keeping a private hotkeyController field) in the constructor or
connectedCallback, referencing the HotkeyController, addController,
firstUpdated, connectedCallback, constructor, and the hotkey property to locate
the code to change.
In `@examples/lit/held-keys/src/app.ts`:
- Around line 41-51: The component currently mutates reactive state inside
updated() by setting this.history when heldKeys changes, causing extra render
cycles; instead, move the history tracking into willUpdate (check
changedProperties.has('heldKeys')) or into the heldKeys controller subscription
so you only update history when heldKeys actually changes—refactor the logic in
updated() (the combo computation and dedupe using last !== combo) into
willUpdate or a controller subscriber and assign this.history there to avoid
triggering an extra render.
In `@examples/lit/hotkey-recorder/src/index.css`:
- Around line 87-100: The recording pulse currently runs infinitely via the
.recording-notice rule and `@keyframes` pulse with no accessibility fallback;
update the CSS to respect the user's prefers-reduced-motion preference by adding
a media query (prefers-reduced-motion: reduce) that disables or replaces the
animation for .recording-notice (and any other recording state at lines
~121-135) — for example, remove animation or set it to none and ensure any
visual state remains visible without motion while keeping the pulse keyframes
intact for non-reduced-motion users.
In `@examples/lit/hotkey-sequence/src/app.ts`:
- Around line 200-203: The handler _shiftG is currently decorated with
`@hotkeySequence`(['Shift+G']) even though it's a single-chord shortcut; change
the decorator to `@hotkey`('Shift+G') on the _shiftG method (and update any
imports if necessary) so the single-key chord uses the hotkey decorator instead
of the sequence implementation to avoid sequence timeout overhead and better
express intent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e54926a-0847-4c3a-a136-f2616d962154
📒 Files selected for processing (40)
.npmrcdocs/config.jsondocs/devtools.mddocs/framework/lit/guides/formatting-display.mddocs/framework/lit/guides/hotkey-recording.mddocs/framework/lit/guides/hotkeys.mddocs/framework/lit/guides/key-state-tracking.mddocs/framework/lit/guides/sequences.mddocs/framework/lit/quick-start.mddocs/framework/lit/reference/classes/HeldKeyCodesController.mddocs/framework/lit/reference/classes/HeldKeysController.mddocs/framework/lit/reference/classes/HotkeyController.mddocs/framework/lit/reference/classes/HotkeyRecorderController.mddocs/framework/lit/reference/classes/HotkeySequenceController.mddocs/framework/lit/reference/classes/KeyHoldController.mddocs/framework/lit/reference/functions/hotkey.mddocs/framework/lit/reference/functions/hotkeySequence.mddocs/framework/lit/reference/index.mddocs/installation.mddocs/overview.mdexamples/lit/held-keys/index.htmlexamples/lit/held-keys/package.jsonexamples/lit/held-keys/src/app.tsexamples/lit/held-keys/src/index.cssexamples/lit/held-keys/tsconfig.jsonexamples/lit/hotkey-recorder/index.htmlexamples/lit/hotkey-recorder/package.jsonexamples/lit/hotkey-recorder/src/app.tsexamples/lit/hotkey-recorder/src/index.cssexamples/lit/hotkey-recorder/tsconfig.jsonexamples/lit/hotkey-sequence/.gitignoreexamples/lit/hotkey-sequence/index.htmlexamples/lit/hotkey-sequence/package.jsonexamples/lit/hotkey-sequence/src/app.tsexamples/lit/hotkey-sequence/src/index.cssexamples/lit/hotkey-sequence/tsconfig.jsonexamples/lit/hotkey/index.htmlexamples/lit/hotkey/package.jsonexamples/lit/hotkey/src/app.tsexamples/lit/hotkey/src/index.css
✅ Files skipped from review due to trivial changes (30)
- docs/overview.md
- examples/lit/hotkey-sequence/.gitignore
- examples/lit/hotkey/index.html
- examples/lit/held-keys/index.html
- docs/devtools.md
- docs/framework/lit/reference/classes/HotkeyController.md
- .npmrc
- examples/lit/hotkey-recorder/index.html
- examples/lit/hotkey/package.json
- docs/framework/lit/reference/functions/hotkeySequence.md
- examples/lit/hotkey-recorder/package.json
- docs/framework/lit/reference/classes/KeyHoldController.md
- docs/framework/lit/reference/classes/HeldKeysController.md
- examples/lit/hotkey-sequence/tsconfig.json
- examples/lit/held-keys/package.json
- docs/framework/lit/reference/classes/HotkeySequenceController.md
- examples/lit/hotkey-sequence/package.json
- docs/framework/lit/reference/classes/HotkeyRecorderController.md
- docs/framework/lit/reference/functions/hotkey.md
- docs/framework/lit/quick-start.md
- examples/lit/held-keys/tsconfig.json
- docs/framework/lit/guides/sequences.md
- docs/framework/lit/guides/hotkey-recording.md
- examples/lit/hotkey-sequence/index.html
- examples/lit/hotkey/src/index.css
- examples/lit/hotkey-recorder/tsconfig.json
- docs/framework/lit/reference/index.md
- examples/lit/hotkey-sequence/src/index.css
- docs/framework/lit/guides/key-state-tracking.md
- docs/framework/lit/reference/classes/HeldKeyCodesController.md
fredericbahr
force-pushed
the
feat-lit-adapter
branch
from
c2f4b71 to
2596073
Compare
Using the event.composedPaths we can travers the bubbled path to recognize if the event originated from an input which now support shadow roots.
fredericbahr
force-pushed
the
feat-lit-adapter
branch
2 times, most recently
from
a02d279 to
2290cb5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (3)
examples/lit/hotkey/src/app.ts (2)
217-221:⚠️ Potential issue | 🟠 MajorSeveral demo snippets still show impossible decorator syntax.
@hotkey(...)decorates a class member; it can't take an inline callback body like the blocks at Lines 218-221, 320-323, 345-348, 376-379, and 405-409. Readers copying these examples into a Lit component will get invalid TypeScript.🛠️ Example fix pattern
-@hotkey('Mod+S', (_event, { hotkey, parsedHotkey }) => { - console.log('Hotkey:', hotkey) - console.log('Parsed:', parsedHotkey) -}) +@hotkey('Mod+S') +private _handleSaving( + _event: KeyboardEvent, + { hotkey, parsedHotkey }: HotkeyCallbackContext, +) { + console.log('Hotkey:', hotkey) + console.log('Parsed:', parsedHotkey) +}Apply the same method-style pattern to the other demo snippets in this file.
Also applies to: 320-323, 345-348, 376-379, 405-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey/src/app.ts` around lines 217 - 221, The examples show invalid decorator syntax by using `@hotkey`(...) with an inline callback body; instead, change each snippet to the method-style pattern where the `@hotkey`('Mod+S') decorator is placed above a class method (e.g., onSave) and move the callback implementation into that method signature (use parameters like _event, { hotkey, parsedHotkey }) so the decorator decorates the method rather than taking a callback; update all occurrences that currently use inline bodies (the snippets using `@hotkey` in the demo) to follow this method-decorated pattern.
270-349:⚠️ Potential issue | 🟠 MajorThese demo shortcuts will look broken in a browser-hosted example.
Mod+1,Alt+ArrowLeft,Control+PageUp,Meta/Alt+F4,F12, andControl+Tabare commonly intercepted before page code runs. The handlers at Lines 608-717 and 781-785 will therefore fail intermittently even when the adapter is working.Also applies to: 383-409, 608-717, 781-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey/src/app.ts` around lines 270 - 349, The demo uses key combos that browsers often intercept (e.g., Mod+1, Alt+ArrowLeft, Control+PageUp, Alt/Meta+F4, F12, Control+Tab) so the `@hotkey` handlers shown in the template and the actual decorator examples (the literal `@hotkey`(...) usages in the code blocks) will fail intermittently; update the demo to either (A) replace those intercepted combos with safe, non-OS-reserved combinations everywhere they appear (change the strings inside the `@hotkey`(...) decorators shown in the code blocks and the displayed formatForDisplay(...) values) or (B) add a visible warning banner in the UI near the sections (use the same template where activeTab, navigationCount, and functionKeyCount are rendered and the formatForDisplay calls are used) that explains these shortcuts may be intercepted by the browser; locate occurrences by searching for formatForDisplay calls, the activeTab/navigationCount/functionKeyCount members, and the literal `@hotkey`('...') decorator examples and update them accordingly.examples/lit/hotkey-recorder/src/app.ts (1)
72-80:⚠️ Potential issue | 🟠 MajorCleared shortcuts immediately fall back to the default binding.
onClearstores'', but the||fallback at Lines 130-131 and 260-262 treats that as falsy, so the shortcut is re-registered and re-shown instead of staying cleared.🛠️ Proposed fix
for (const [actionId, action] of Object.entries(DEFAULT_SHORTCUT_ACTIONS)) { - const hotkey = (this._shortcuts[actionId] || - action.defaultHotkey) as Hotkey + const storedHotkey = this._shortcuts[actionId] + const hotkey = + storedHotkey === undefined ? action.defaultHotkey : storedHotkey + + if (!hotkey) continue @@ <kbd> - ${formatForDisplay( - (this._shortcuts[actionId] || - action.defaultHotkey) as Hotkey, - )} + ${ + this._shortcuts[actionId] + ? formatForDisplay(this._shortcuts[actionId] as Hotkey) + : '—' + } </kbd>Also applies to: 129-142, 253-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-recorder/src/app.ts` around lines 72 - 80, The clearing logic currently writes an empty string into _shortcuts in onClear (using this._recordingActionId) which is then treated as falsy by the registration/display code that uses the || fallback; change onClear to store a distinct sentinel (e.g., null or a special token like '__CLEARED__') instead of '' and then update the hotkey lookup/registration and UI display logic (the code that currently does binding = shortcuts[action] || defaultBinding and the hotkey registration in _reregisterHotkeys) to use an explicit check (nullish check or equality against the sentinel) so cleared bindings are honored (not replaced by the default) and avoid re-registering cleared shortcuts.
🧹 Nitpick comments (5)
docs/framework/lit/guides/formatting-display.md (1)
201-201: Minor wording cleanup: “component parts” is redundant.Prefer “components” or “parts”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/lit/guides/formatting-display.md` at line 201, The phrase "component parts" is redundant; locate the sentence "Parse a hotkey string into its component parts:" in the document and replace "component parts" with a single word—either "components" or "parts"—so the line reads e.g. "Parse a hotkey string into its components:" to improve wording.packages/lit-hotkeys/README.md (1)
49-49: Normalize heading level progressionLine 49 uses
###directly under a top-level#section. This triggers MD001 and makes document structure less clear for navigation tooling.Suggested patch
-### <a href="https://tanstack.com/hotkeys">Read the docs →</a> +## <a href="https://tanstack.com/hotkeys">Read the docs →</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/README.md` at line 49, The heading "Read the docs →" is currently using a level-3 heading (###) directly under a top-level section, breaking heading progression; update that line to use a level-2 heading (##) so it follows the top-level '#' section properly and satisfies MD001. Locate the heading text "Read the docs →" in README.md and change its leading hashes from "###" to "##" while keeping the link and text intact.packages/lit-hotkeys/src/controllers/hotkey.ts (1)
44-49: Constructor does not auto-register with host.Unlike
KeyHoldController,HotkeyRecorderController, andHotkeySequenceRecorderController, this controller does not callthis._host.addController(this)in the constructor. This creates an inconsistent API where some controllers auto-register and others require manual registration.♻️ Suggested fix for consistency
constructor( private _host: ReactiveControllerHost, private _hotkey: RegisterableHotkey, private _callback: HotkeyCallback, private _options: HotkeyOptions = HOTKEY_DEFAULT_OPTIONS, - ) {} + ) { + this._host.addController(this) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/controllers/hotkey.ts` around lines 44 - 49, The constructor for this controller currently accepts _host, _hotkey, _callback, and _options but does not auto-register with the host; to fix, add a call to this._host.addController(this) inside the constructor so the controller automatically registers just like KeyHoldController, HotkeyRecorderController, and HotkeySequenceRecorderController — update the constructor in hotkey.ts to invoke _host.addController(this) (ensuring you reference the existing private _host field and do not change the constructor signature).packages/hotkeys/src/sequence-manager.ts (1)
473-478: Truncated comment needs completion.The comment on line 473 appears to be cut off mid-sentence: "Check if the event is bubbling to an input element that is not the registration target to". Please complete the sentence to clarify the intent.
📝 Suggested fix
- // Check if the event is bubbling to an input element that is not the registration target to + // Check if the event is bubbling through an input element that is not the registration target for (const element of event.composedPath()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hotkeys/src/sequence-manager.ts` around lines 473 - 478, Complete the truncated comment above the composedPath loop to explain intent: replace "Check if the event is bubbling to an input element that is not the registration target to" with a full sentence clarifying that the loop skips handling when the event originated from or is bubbling through a different input element (i.e., if any element in event.composedPath() is an input and is not equal to registration.target, continue the outer registrationIds loop), referencing isInputElement, event.composedPath(), and registration.target so readers understand why the handler is ignored for non-target input elements.packages/lit-hotkeys/src/controllers/hotkey-sequence.ts (1)
38-43: Consider auto-registering the controller for consistency.Unlike sibling controllers (
KeyHoldController,HotkeyRecorderController,HotkeyController), this controller does not callthis._host.addController(this)in the constructor. This forces callers to manually register it, creating an inconsistent API surface.Compare with
key-hold.tsline 62 which doesthis._host.addController(this).♻️ Suggested fix for consistency
constructor( private _host: ReactiveControllerHost, private _sequence: HotkeySequence, private _callback: HotkeyCallback, private _options: SequenceOptions = HOTKEY_SEQUENCE_DEFAULT_OPTIONS, - ) {} + ) { + this._host.addController(this) + }Then update the docstring example accordingly:
* `@example` * ```ts * class MyElement extends LitElement { * private seq = new HotkeySequenceController(this, ['G', 'G'], () => this.goToTop()) - * - * constructor() { - * super() - * this.addController(this.seq) - * } * } * ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/controllers/hotkey-sequence.ts` around lines 38 - 43, The constructor for the HotkeySequenceController does not auto-register with the host, unlike sibling controllers; modify the constructor in hotkey-sequence.ts to call this._host.addController(this) so the controller self-registers (i.e., add the registration call inside the constructor that currently accepts _host, _sequence, _callback, _options), and update the example docstring to remove the manual this.addController(this.seq) call so usage matches KeyHoldController/HotkeyController patterns.
🤖 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/lit/guides/formatting-display.md`:
- Around line 131-136: The usage snippet is labeled as HTML but contains Lit
template binding syntax (e.g. menu-item elements using `@action`=${save},
`@action`=${undo}, `@action`=${find}), which can mislead users; update the example
to show it as a Lit/TypeScript template literal instead of plain HTML by
wrapping it in a ts/`html` code fence or otherwise indicating it's a Lit
template (preserve the menu-item elements and the save/undo/find handler names
and the `@action` bindings so readers know these are Lit bindings).
- Around line 109-118: The HotkeyController is only registered in firstUpdated
so it captures the initial this.hotkey and won't update when the hotkey property
changes; change the registration to handle property updates by storing the
controller instance (e.g., this._hotkeyController) and creating or replacing it
inside updated(changedProperties: PropertyValues) when
changedProperties.has('hotkey') — remove or replace the old controller and add a
new HotkeyController(this, this.hotkey, ...) so bindings reflect the current
hotkey; keep firstUpdated minimal (or remove the addController there) and ensure
the unique symbols referenced are HotkeyController, firstUpdated, updated,
hotkey, and this._hotkeyController (or similar instance field) so future hotkey
changes recreate the controller.
In `@docs/framework/lit/guides/hotkeys.md`:
- Around line 184-200: The example won't compile because `@customElement` was
wrapped in backticks and the file imports the `hotkey` decorator while
instantiating `HotkeyController`; fix by removing the backticks so the decorator
reads `@customElement`('my-panel') above class MyPanel, change the import to bring
in HotkeyController (e.g. import { HotkeyController } from
'@tanstack/lit-hotkeys') or import both symbols as needed, and ensure the
instance creation in firstUpdated uses the correct class name HotkeyController
with the proper constructor args (this, 'Escape', handler, { target:
this.panelRef.value }) and that panelRef, escapeHotkey, and firstUpdated
signatures/types remain intact.
In `@examples/lit/hotkey-recorder/src/app.ts`:
- Around line 58-71: _handleEdit clears all bindings before starting recording
but the recorder.onCancel handler only clears _recordingActionId, leaving
hotkeys unregistered; update the recorder onCancel callback (and the duplicate
at lines around the other occurrence) to clear this._recordingActionId and then
call this._reregisterHotkeys() so bindings are restored, referencing the
HotkeyRecorderController instance (recorder), the _recordingActionId field, and
the _reregisterHotkeys method.
In `@examples/lit/hotkey/src/app.ts`:
- Line 422: The Escape key handler currently promises to "reset all counters"
but does not reset sidebarShortcutCount, modalShortcutCount, or
editorShortcutCount; update the Escape/Reset handler (the function that responds
to the Escape key and the similar block around the code handling lines ~799-810)
to explicitly set sidebarShortcutCount, modalShortcutCount, and
editorShortcutCount to zero (using the same state setters or mutation pattern
used for the other counters) so all counters are reset consistently when Escape
is pressed.
- Around line 247-266: The current implementation only guards execution inside
the handler (_handleConditional) but does not unregister the hotkey; expose the
HotkeyController registration handle created by the `@hotkey` decorator (or obtain
the controller instance) and call its setOptions({ enabled: this.enabled })
whenever the component property `enabled` changes (e.g., in updated or a
watcher) so the hotkey registration is actually enabled/disabled instead of
relying on the manual early-return; ensure the initial registration uses the
same enabled state and keep _handleConditional as the callback referenced by the
decorator.
In `@examples/lit/key-hold/src/index.css`:
- Around line 26-30: The CSS rule for the selector header p contains a duplicate
margin declaration; remove the redundant "margin: 0" so the block only uses
"margin: 0 auto" (i.e., update the header p rule to eliminate the earlier
margin: 0 line and keep margin: 0 auto to satisfy Stylelint and avoid the
override).
In `@packages/lit-hotkeys/README.md`:
- Line 69: The CONTRIBUTING.md link in packages/lit-hotkeys/README.md is
incorrect (it uses ./CONTRIBUTING.md); update the link in README.md to point to
the correct location by replacing the relative path with either the repo-root
relative path ../../CONTRIBUTING.md or the absolute GitHub URL (e.g.,
https://github.com/TanStack/hotkeys/blob/main/CONTRIBUTING.md) so the link
resolves from the package README; modify the anchor text where the link is
defined in README.md accordingly.
- Around line 8-16: The three anchor tags linking to npm, GitHub, and
Bundlephobia use an escaped target value and the nested img tags have duplicate
alt attributes; update each anchor (the <a
href="https://www.npmjs.com/package/@tanstack/hotkeys">, <a
href="https://github.com/TanStack/hotkeys">, and <a
href="https://bundlephobia.com/result?p=@tanstack/lit-hotkeys@latest">) to use a
real target="_parent" (or remove the target if not needed) and remove the
duplicate alt attribute on each corresponding img so each img has a single,
meaningful alt value.
In `@packages/lit-hotkeys/src/controllers/held-key-codes.ts`:
- Around line 45-53: In hostConnected(), initialize this._value immediately from
the current tracker state before creating the subscription: call
getKeyStateTracker(), read tracker.store.state.heldCodes and assign it to
this._value, then call this._host.requestUpdate(), and after that set up
tracker.store.subscribe(...) as current code does; this ensures hostConnected,
_value, getKeyStateTracker, tracker.store.state, tracker.store.subscribe and
_host.requestUpdate are used so the first render reflects already-held keys.
- Around line 11-23: The JSDoc example uses the wrong property and assumes value
is an array; update the example to reference the controller instance name
HeldKeyCodesController exposes (heldKeyCodes) instead of heldKeys, and show the
heldKeyCodes.value record as a joined string by taking its values (e.g., use
Object.values on heldKeyCodes.value and join with " + ") or keys as appropriate,
falling back to 'None' when empty; locate the example block around the class
KeyDisplay and replace the incorrect this.heldKeys.value.join(...) usage with a
join over Object.values(this.heldKeyCodes.value) and correct the property name
to heldKeyCodes.
In `@packages/lit-hotkeys/src/controllers/hotkey-recorder.ts`:
- Around line 64-98: The recorder is created only in the constructor and
destroyed in hostDisconnected, so reconnecting the host subscribes to a
destroyed HotkeyRecorder; modify hostConnected/hostDisconnected to recreate the
recorder on reconnect: in hostConnected(), if this._recorder is
missing/destroyed (e.g., null/undefined) instantiate a new
HotkeyRecorder(this._options) before subscribing to this._recorder.store and set
this._unsubscribe to the new subscription; in hostDisconnected(), keep the
unsubscribe behavior but either do not destroy the recorder (just stop it) or,
if you continue to destroy it, set this._recorder = undefined/null after calling
this._recorder.destroy() so hostConnected() can detect and recreate it on the
next connect (ensure startRecording/stopRecording now operate against the live
_recorder instance).
In `@packages/lit-hotkeys/src/controllers/hotkey-sequence-recorder.ts`:
- Around line 76-114: The controller creates the HotkeySequenceRecorder once in
the constructor (this._recorder) but calls this._recorder.destroy() in
hostDisconnected(), which leaves a destroyed recorder when the host reconnects;
replace the destroy() call with this._recorder.stop() (or, if you prefer
recreation, remove destroy() and re-instantiate this._recorder inside
hostConnected()) and ensure subscription handling stays the same so the recorder
is not left unusable after disconnect/reconnect; update hostDisconnected() to
call stop() and clear subscriptions, or add recreation logic in hostConnected()
that reinitializes this._recorder and re-subscribes.
In `@packages/lit-hotkeys/src/controllers/hotkey.ts`:
- Line 48: HOTKEY_DEFAULT_OPTIONS currently holds a direct document reference
causing SSR import crashes; replace the module-scope constant with a
factory/getter that lazily resolves the target at runtime (e.g.,
createDefaultHotkeyOptions() or getHotkeyDefaultOptions()) so the object is
constructed only after checking typeof document !== "undefined"; update the
Hotkey class initializer (the _options: HotkeyOptions default) to call that
factory and ensure HotkeyOptions (and any uses of HOTKEY_DEFAULT_OPTIONS) accept
undefined/null for target so the module import itself no longer accesses
document.
In `@packages/lit-hotkeys/src/decorators/hotkey-sequence.ts`:
- Around line 51-59: The fallback branch for building the callback in the
`@hotkeySequence` decorator can lose the correct this context because it uses
host[methodName] directly; change the fallback so the method is bound to host
(i.e., call .bind(host)) when descriptor.value is falsy, ensuring the created
const callback (HotkeyCallback) is always a function bound to the correct this;
update the code around descriptor, callback, host and methodName so both
branches produce a bound function and keep the existing type checks/throw for
non-functions.
---
Duplicate comments:
In `@examples/lit/hotkey-recorder/src/app.ts`:
- Around line 72-80: The clearing logic currently writes an empty string into
_shortcuts in onClear (using this._recordingActionId) which is then treated as
falsy by the registration/display code that uses the || fallback; change onClear
to store a distinct sentinel (e.g., null or a special token like '__CLEARED__')
instead of '' and then update the hotkey lookup/registration and UI display
logic (the code that currently does binding = shortcuts[action] ||
defaultBinding and the hotkey registration in _reregisterHotkeys) to use an
explicit check (nullish check or equality against the sentinel) so cleared
bindings are honored (not replaced by the default) and avoid re-registering
cleared shortcuts.
In `@examples/lit/hotkey/src/app.ts`:
- Around line 217-221: The examples show invalid decorator syntax by using
`@hotkey`(...) with an inline callback body; instead, change each snippet to the
method-style pattern where the `@hotkey`('Mod+S') decorator is placed above a
class method (e.g., onSave) and move the callback implementation into that
method signature (use parameters like _event, { hotkey, parsedHotkey }) so the
decorator decorates the method rather than taking a callback; update all
occurrences that currently use inline bodies (the snippets using `@hotkey` in the
demo) to follow this method-decorated pattern.
- Around line 270-349: The demo uses key combos that browsers often intercept
(e.g., Mod+1, Alt+ArrowLeft, Control+PageUp, Alt/Meta+F4, F12, Control+Tab) so
the `@hotkey` handlers shown in the template and the actual decorator examples
(the literal `@hotkey`(...) usages in the code blocks) will fail intermittently;
update the demo to either (A) replace those intercepted combos with safe,
non-OS-reserved combinations everywhere they appear (change the strings inside
the `@hotkey`(...) decorators shown in the code blocks and the displayed
formatForDisplay(...) values) or (B) add a visible warning banner in the UI near
the sections (use the same template where activeTab, navigationCount, and
functionKeyCount are rendered and the formatForDisplay calls are used) that
explains these shortcuts may be intercepted by the browser; locate occurrences
by searching for formatForDisplay calls, the
activeTab/navigationCount/functionKeyCount members, and the literal
`@hotkey`('...') decorator examples and update them accordingly.
---
Nitpick comments:
In `@docs/framework/lit/guides/formatting-display.md`:
- Line 201: The phrase "component parts" is redundant; locate the sentence
"Parse a hotkey string into its component parts:" in the document and replace
"component parts" with a single word—either "components" or "parts"—so the line
reads e.g. "Parse a hotkey string into its components:" to improve wording.
In `@packages/hotkeys/src/sequence-manager.ts`:
- Around line 473-478: Complete the truncated comment above the composedPath
loop to explain intent: replace "Check if the event is bubbling to an input
element that is not the registration target to" with a full sentence clarifying
that the loop skips handling when the event originated from or is bubbling
through a different input element (i.e., if any element in event.composedPath()
is an input and is not equal to registration.target, continue the outer
registrationIds loop), referencing isInputElement, event.composedPath(), and
registration.target so readers understand why the handler is ignored for
non-target input elements.
In `@packages/lit-hotkeys/README.md`:
- Line 49: The heading "Read the docs →" is currently using a level-3 heading
(###) directly under a top-level section, breaking heading progression; update
that line to use a level-2 heading (##) so it follows the top-level '#' section
properly and satisfies MD001. Locate the heading text "Read the docs →" in
README.md and change its leading hashes from "###" to "##" while keeping the
link and text intact.
In `@packages/lit-hotkeys/src/controllers/hotkey-sequence.ts`:
- Around line 38-43: The constructor for the HotkeySequenceController does not
auto-register with the host, unlike sibling controllers; modify the constructor
in hotkey-sequence.ts to call this._host.addController(this) so the controller
self-registers (i.e., add the registration call inside the constructor that
currently accepts _host, _sequence, _callback, _options), and update the example
docstring to remove the manual this.addController(this.seq) call so usage
matches KeyHoldController/HotkeyController patterns.
In `@packages/lit-hotkeys/src/controllers/hotkey.ts`:
- Around line 44-49: The constructor for this controller currently accepts
_host, _hotkey, _callback, and _options but does not auto-register with the
host; to fix, add a call to this._host.addController(this) inside the
constructor so the controller automatically registers just like
KeyHoldController, HotkeyRecorderController, and
HotkeySequenceRecorderController — update the constructor in hotkey.ts to invoke
_host.addController(this) (ensuring you reference the existing private _host
field and do not change the constructor signature).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40918760-fb69-4d01-9f0c-ba53f58228c8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (79)
.changeset/add-lit-adapter.md.npmrcdocs/config.jsondocs/devtools.mddocs/framework/lit/guides/formatting-display.mddocs/framework/lit/guides/hotkey-recording.mddocs/framework/lit/guides/hotkeys.mddocs/framework/lit/guides/key-state-tracking.mddocs/framework/lit/guides/sequence-recording.mddocs/framework/lit/guides/sequences.mddocs/framework/lit/quick-start.mddocs/framework/lit/reference/classes/HeldKeyCodesController.mddocs/framework/lit/reference/classes/HeldKeysController.mddocs/framework/lit/reference/classes/HotkeyController.mddocs/framework/lit/reference/classes/HotkeyRecorderController.mddocs/framework/lit/reference/classes/HotkeySequenceController.mddocs/framework/lit/reference/classes/KeyHoldController.mddocs/framework/lit/reference/functions/hotkey.mddocs/framework/lit/reference/functions/hotkeySequence.mddocs/framework/lit/reference/index.mddocs/installation.mddocs/overview.mdexamples/lit/held-keys/index.htmlexamples/lit/held-keys/package.jsonexamples/lit/held-keys/src/app.tsexamples/lit/held-keys/src/index.cssexamples/lit/held-keys/tsconfig.jsonexamples/lit/hotkey-recorder/index.htmlexamples/lit/hotkey-recorder/package.jsonexamples/lit/hotkey-recorder/src/app.tsexamples/lit/hotkey-recorder/src/index.cssexamples/lit/hotkey-recorder/tsconfig.jsonexamples/lit/hotkey-sequence-recorder/index.htmlexamples/lit/hotkey-sequence-recorder/package.jsonexamples/lit/hotkey-sequence-recorder/src/app.tsexamples/lit/hotkey-sequence-recorder/src/index.cssexamples/lit/hotkey-sequence-recorder/tsconfig.jsonexamples/lit/hotkey-sequence/.gitignoreexamples/lit/hotkey-sequence/index.htmlexamples/lit/hotkey-sequence/package.jsonexamples/lit/hotkey-sequence/src/app.tsexamples/lit/hotkey-sequence/src/index.cssexamples/lit/hotkey-sequence/tsconfig.jsonexamples/lit/hotkey/index.htmlexamples/lit/hotkey/package.jsonexamples/lit/hotkey/src/app.tsexamples/lit/hotkey/src/index.cssexamples/lit/hotkey/tsconfig.jsonexamples/lit/key-hold/index.htmlexamples/lit/key-hold/package.jsonexamples/lit/key-hold/src/app.tsexamples/lit/key-hold/src/index.cssexamples/lit/key-hold/tsconfig.jsonpackage.jsonpackages/hotkeys/src/sequence-manager.tspackages/lit-hotkeys/CHANGELOG.mdpackages/lit-hotkeys/README.mdpackages/lit-hotkeys/eslint.config.jspackages/lit-hotkeys/package.jsonpackages/lit-hotkeys/src/constants.tspackages/lit-hotkeys/src/controllers/held-key-codes.tspackages/lit-hotkeys/src/controllers/held-keys.tspackages/lit-hotkeys/src/controllers/hotkey-recorder.tspackages/lit-hotkeys/src/controllers/hotkey-sequence-recorder.tspackages/lit-hotkeys/src/controllers/hotkey-sequence.tspackages/lit-hotkeys/src/controllers/hotkey.tspackages/lit-hotkeys/src/controllers/index.tspackages/lit-hotkeys/src/controllers/key-hold.tspackages/lit-hotkeys/src/decorators/hotkey-sequence.tspackages/lit-hotkeys/src/decorators/hotkey.tspackages/lit-hotkeys/src/decorators/index.tspackages/lit-hotkeys/src/index.tspackages/lit-hotkeys/tests/hotkey-sequence.spec.tspackages/lit-hotkeys/tests/hotkey.spec.tspackages/lit-hotkeys/tsconfig.docs.jsonpackages/lit-hotkeys/tsconfig.jsonpackages/lit-hotkeys/tsdown.config.tspackages/lit-hotkeys/vitest.config.tsscripts/generate-docs.ts
✅ Files skipped from review due to trivial changes (50)
- docs/overview.md
- packages/lit-hotkeys/tsconfig.json
- examples/lit/held-keys/index.html
- examples/lit/key-hold/package.json
- examples/lit/hotkey-sequence/.gitignore
- examples/lit/hotkey-recorder/index.html
- packages/lit-hotkeys/eslint.config.js
- .changeset/add-lit-adapter.md
- examples/lit/key-hold/index.html
- examples/lit/hotkey-sequence-recorder/index.html
- examples/lit/hotkey-sequence-recorder/package.json
- packages/lit-hotkeys/src/index.ts
- docs/framework/lit/reference/classes/HotkeyController.md
- examples/lit/hotkey/package.json
- docs/framework/lit/reference/index.md
- docs/config.json
- packages/lit-hotkeys/vitest.config.ts
- examples/lit/hotkey/tsconfig.json
- docs/framework/lit/reference/classes/KeyHoldController.md
- docs/framework/lit/reference/classes/HeldKeysController.md
- docs/framework/lit/reference/classes/HotkeySequenceController.md
- examples/lit/held-keys/src/index.css
- packages/lit-hotkeys/src/constants.ts
- examples/lit/hotkey-sequence-recorder/tsconfig.json
- packages/lit-hotkeys/package.json
- examples/lit/hotkey-sequence/tsconfig.json
- examples/lit/hotkey-recorder/tsconfig.json
- docs/framework/lit/reference/classes/HotkeyRecorderController.md
- docs/framework/lit/reference/functions/hotkeySequence.md
- docs/framework/lit/reference/classes/HeldKeyCodesController.md
- docs/framework/lit/quick-start.md
- docs/framework/lit/guides/sequence-recording.md
- examples/lit/hotkey-recorder/package.json
- examples/lit/key-hold/tsconfig.json
- packages/lit-hotkeys/src/decorators/hotkey.ts
- examples/lit/held-keys/tsconfig.json
- docs/installation.md
- packages/lit-hotkeys/tests/hotkey.spec.ts
- docs/framework/lit/reference/functions/hotkey.md
- docs/framework/lit/guides/key-state-tracking.md
- examples/lit/hotkey-sequence/src/index.css
- docs/framework/lit/guides/hotkey-recording.md
- examples/lit/hotkey-sequence-recorder/src/index.css
- docs/framework/lit/guides/sequences.md
- examples/lit/hotkey-sequence/package.json
- examples/lit/hotkey/src/index.css
- packages/lit-hotkeys/CHANGELOG.md
- examples/lit/hotkey-sequence/index.html
- examples/lit/hotkey-recorder/src/index.css
- examples/lit/held-keys/package.json
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/devtools.md
- .npmrc
- packages/lit-hotkeys/tsconfig.docs.json
- packages/lit-hotkeys/src/decorators/index.ts
- examples/lit/hotkey/index.html
- packages/lit-hotkeys/tsdown.config.ts
- packages/lit-hotkeys/src/controllers/held-keys.ts
- examples/lit/held-keys/src/app.ts
- packages/lit-hotkeys/src/controllers/index.ts
- package.json
fredericbahr
force-pushed
the
feat-lit-adapter
branch
from
2290cb5 to
ed83f61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docs/framework/lit/guides/hotkeys.md (1)
189-189:⚠️ Potential issue | 🟡 MinorRemove backticks around
@customElementdecorator.The decorator is wrapped in backticks (
`@customElement`), which is invalid TypeScript syntax and will cause a compilation error if users copy this example.📝 Proposed fix
-`@customElement`('my-panel') +@customElement('my-panel')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/lit/guides/hotkeys.md` at line 189, The example mistakenly wraps the decorator in backticks causing invalid TypeScript; remove the backticks so the decorator appears as `@customElement`('my-panel') in the example. Locate the snippet containing `@customElement` and the 'my-panel' identifier in the docs and replace the backticked version with the plain decorator syntax to ensure the code compiles.docs/framework/lit/guides/formatting-display.md (2)
131-136:⚠️ Potential issue | 🟡 MinorCode block language should be
ts, nothtml.The snippet uses Lit template binding syntax (
@action=${save}), which is not valid HTML. Label it as TypeScript to avoid misleading copy/paste usage.📝 Proposed fix
Usage: -```html -<menu-item label="Save" hotkey="Mod+S" `@action`=${save}></menu-item> -<menu-item label="Undo" hotkey="Mod+Z" `@action`=${undo}></menu-item> -<menu-item label="Find" hotkey="Mod+F" `@action`=${find}></menu-item> +```ts +html` + <menu-item label="Save" hotkey="Mod+S" `@action`=${save}></menu-item> + <menu-item label="Undo" hotkey="Mod+Z" `@action`=${undo}></menu-item> + <menu-item label="Find" hotkey="Mod+F" `@action`=${find}></menu-item> +`</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/framework/lit/guides/formatting-display.mdaround lines 131 - 136,
Change the fenced code block language from html to ts and show this as a Lit
template literal so readers know the snippet uses Lit bindings: replace the
html block containing the <menu-item ... `@action`=${save}> / `@action`=${undo} / `@action`=${find} lines with ats block that includes html...wrapping the
three elements (preserving the labels and hotkey attributes and the
@action=${save|undo|find} bindings).</details> --- `109-118`: _⚠️ Potential issue_ | _🟡 Minor_ **`HotkeyController` won't react to `hotkey` property changes.** Registering in `firstUpdated` captures the initial `hotkey` value only. If the `hotkey` property changes later, the binding becomes stale. <details> <summary>📝 Suggested pattern for reactive hotkey changes</summary> ```diff class MenuItem extends LitElement { `@property`({ type: String }) label = '' `@property`({ type: String }) hotkey = '' + private hotkeyController?: HotkeyController - firstUpdated(changedProperties: PropertyValues) { - super.firstUpdated(changedProperties) - this.addController( - new HotkeyController(this, this.hotkey, () => - this.dispatchEvent( - new CustomEvent('action', { bubbles: true, composed: true }), - ), - ), - ) + updated(changedProperties: PropertyValues) { + if (changedProperties.has('hotkey') && this.hotkey) { + if (this.hotkeyController) this.removeController(this.hotkeyController) + this.hotkeyController = new HotkeyController(this, this.hotkey, () => + this.dispatchEvent(new CustomEvent('action', { bubbles: true, composed: true })), + ) + this.addController(this.hotkeyController) + } } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/framework/lit/guides/formatting-display.md` around lines 109 - 118, The HotkeyController is added in firstUpdated so it only captures the initial hotkey value; change the component to register or update the controller reactively so it sees subsequent hotkey changes—either add the controller in the constructor (using this.addController(new HotkeyController(this, this.hotkey, ...))) and have the controller read the host.hotkey on demand, or keep adding/updating the controller inside the component's updated(changedProperties) hook to detect changes to the hotkey property and recreate or call a setter on HotkeyController; update the logic referencing firstUpdated, HotkeyController, hotkey, addController, and updated/constructor accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>examples/lit/hotkey-sequence-recorder/src/app.ts (1)</summary><blockquote> `139-139`: **Consider removing or gating the debug log for production.** This `console.log` will output on every hotkey trigger. While acceptable for a demo app, consider removing it or wrapping in a development check if this example is meant to be copied. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@examples/lit/hotkey-sequence-recorder/src/app.ts` at line 139, Remove or gate the noisy console.log that prints `${action.name} triggered:, sequence` on every hotkey trigger; locate the log in app.ts where console.log is called (referencing action.name and sequence) and either delete it for production builds or wrap it behind a development check (e.g., an isDev/PROCESS.ENV.NODE_ENV check or a debug logger) so it only runs in non-production environments. ``` </details> </blockquote></details> <details> <summary>packages/lit-hotkeys/README.md (1)</summary><blockquote> `49-49`: **Heading level jumps from implicit h1 to h3.** The markdown heading `### [Become a Sponsor!]` (h3) appears without an h2 preceding it. This may cause accessibility or rendering issues in some markdown processors. <details> <summary>📝 Proposed fix</summary> ```diff -### [Become a Sponsor!](https://github.com/sponsors/tannerlinsley/) +#### [Become a Sponsor!](https://github.com/sponsors/tannerlinsley/) ``` Or restructure with proper heading hierarchy. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/README.md` at line 49, The README has an h3 heading ("### [Become a Sponsor!]") that jumps from the implicit h1, harming semantic hierarchy; update that heading to an h2 (change "### [Become a Sponsor!]" to "## [Become a Sponsor!]") or insert an appropriate h2 section above it so the heading levels are sequential—look for the "### [Become a Sponsor!]" and the surrounding headings in the README.md to apply the fix. ``` </details> </blockquote></details> <details> <summary>docs/framework/lit/guides/hotkeys.md (1)</summary><blockquote> `153-153`: **Minor style: repetitive use of "again".** Consider rewording for clarity. <details> <summary>📝 Proposed fix</summary> ```diff -When `true`, the hotkey fires only once per key press. The key must be released and pressed again to fire again. Defaults to `false`. +When `true`, the hotkey fires only once per key press. The key must be released before it can fire on the next press. Defaults to `false`. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/framework/lit/guides/hotkeys.md` at line 153, The description for the hotkey option that currently reads "When `true`, the hotkey fires only once per key press. The key must be released and pressed again to fire again." is repetitive; update the `once`/hotkey option description to a single, concise sentence (e.g., state that when true the hotkey fires a single time per press and requires releasing and pressing again to re-trigger) to remove the duplicated "again" and improve clarity. ``` </details> </blockquote></details> <details> <summary>packages/lit-hotkeys/src/controllers/key-hold.ts (1)</summary><blockquote> `67-72`: **Prefer canonical key normalization over `toLowerCase()` matching.** On Line 67 and Line 71, case-folding works for basic keys but can diverge from tracker normalization rules (aliases/canonical forms). Align comparison with the same normalization strategy used by `KeyStateTracker`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/lit-hotkeys/src/controllers/key-hold.ts` around lines 67 - 72, The code uses simple toLowerCase() for normalization (const normalizedKey = this._key.toLowerCase()) and when comparing held keys, which can diverge from KeyStateTracker's canonicalization; replace both uses with the same canonical normalize routine from KeyStateTracker (e.g., call KeyStateTracker.normalizeKey(this._key) or tracker.normalizeKey(...) depending on where the helper lives) and compare against heldKey normalized via the same function inside the subscribe predicate so all comparisons use the identical canonical form. ``` </details> </blockquote></details> <details> <summary>examples/lit/held-keys/src/app.ts (1)</summary><blockquote> `71-87`: **Consider a polite live-region summary for the changing key state.** These sections update visually as keys change, but screen readers typically will not announce that by default. A small `aria-live="polite"` summary for the current combo/history would make the demo more usable in assistive-tech workflows. Also applies to: 119-128 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@examples/lit/held-keys/src/app.ts` around lines 71 - 87, Add a polite aria-live summary that announces the current key combo/history whenever the keys change: create a small live region (aria-live="polite") that derives its text from the keys array and formatForDisplay output (e.g., "Keys held: A + Shift" or a brief history) and render/update it whenever the keys variable changes; place the live region near the existing visual elements (the div with class "key-display" and/or the div with class "stats") so screen readers will announce changes—also add the same live-region update where the alternate UI at lines ~119-128 updates. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@examples/lit/held-keys/src/app.ts:
- Around line 39-52: The updated() method currently uses the last history entry
to dedupe combos which misses repeat presses and repopulates after clear; add a
private sentinel (e.g., lastObservedCombo: string | null) to track the last
observed combo from heldKeys.value, update that sentinel when keys are held,
push to history only if combo !== lastObservedCombo, and reset lastObservedCombo
to null when no keys are held so subsequent presses are recorded; modify the
updated() logic (referencing updated(), heldKeys, history) to use
lastObservedCombo instead of comparing against this.history[this.history.length- 1].
In
@packages/lit-hotkeys/src/controllers/key-hold.ts:
- Around line 65-81: The hostConnected() method in KeyHoldController doesn't
initialize _value from current state, so if the key is already held when the
host connects _value stays stale; before creating the subscription, read current
held keys from getKeyStateTracker() (use tracker.store.state.heldKeys), compute
isHeld by comparing heldKey.toLowerCase() to this._key.toLowerCase(), assign
this._value = isHeld and call this._host.requestUpdate() if it differs, then
proceed to subscribe and set _unsubscribe as currently done; keep references to
hostConnected, getKeyStateTracker, tracker.store.state.heldKeys, _key, _value,
and _host.requestUpdate to locate the change.
Duplicate comments:
In@docs/framework/lit/guides/formatting-display.md:
- Around line 131-136: Change the fenced code block language from html to ts and
show this as a Lit template literal so readers know the snippet uses Lit
bindings: replace thehtml block containing the <menu-item ... `@action`=${save}> / `@action`=${undo} / `@action`=${find} lines with ats block
that includes html...wrapping the three elements (preserving
the labels and hotkey attributes and the@action=${save|undo|find} bindings).- Around line 109-118: The HotkeyController is added in firstUpdated so it only
captures the initial hotkey value; change the component to register or update
the controller reactively so it sees subsequent hotkey changes—either add the
controller in the constructor (using this.addController(new
HotkeyController(this, this.hotkey, ...))) and have the controller read the
host.hotkey on demand, or keep adding/updating the controller inside the
component's updated(changedProperties) hook to detect changes to the hotkey
property and recreate or call a setter on HotkeyController; update the logic
referencing firstUpdated, HotkeyController, hotkey, addController, and
updated/constructor accordingly.In
@docs/framework/lit/guides/hotkeys.md:
- Line 189: The example mistakenly wraps the decorator in backticks causing
invalid TypeScript; remove the backticks so the decorator appears as
@customElement('my-panel') in the example. Locate the snippet containing
@customElementand the 'my-panel' identifier in the docs and replace the
backticked version with the plain decorator syntax to ensure the code compiles.
Nitpick comments:
In@docs/framework/lit/guides/hotkeys.md:
- Line 153: The description for the hotkey option that currently reads "When
true, the hotkey fires only once per key press. The key must be released and
pressed again to fire again." is repetitive; update theonce/hotkey option
description to a single, concise sentence (e.g., state that when true the hotkey
fires a single time per press and requires releasing and pressing again to
re-trigger) to remove the duplicated "again" and improve clarity.In
@examples/lit/held-keys/src/app.ts:
- Around line 71-87: Add a polite aria-live summary that announces the current
key combo/history whenever the keys change: create a small live region
(aria-live="polite") that derives its text from the keys array and
formatForDisplay output (e.g., "Keys held: A + Shift" or a brief history) and
render/update it whenever the keys variable changes; place the live region near
the existing visual elements (the div with class "key-display" and/or the div
with class "stats") so screen readers will announce changes—also add the same
live-region update where the alternate UI at lines ~119-128 updates.In
@examples/lit/hotkey-sequence-recorder/src/app.ts:
- Line 139: Remove or gate the noisy console.log that prints
${action.name} triggered:, sequenceon every hotkey trigger; locate the log in app.ts where
console.log is called (referencing action.name and sequence) and either delete
it for production builds or wrap it behind a development check (e.g., an
isDev/PROCESS.ENV.NODE_ENV check or a debug logger) so it only runs in
non-production environments.In
@packages/lit-hotkeys/README.md:
- Line 49: The README has an h3 heading ("### [Become a Sponsor!]") that jumps
from the implicit h1, harming semantic hierarchy; update that heading to an h2
(change "### [Become a Sponsor!]" to "## [Become a Sponsor!]") or insert an
appropriate h2 section above it so the heading levels are sequential—look for
the "### [Become a Sponsor!]" and the surrounding headings in the README.md to
apply the fix.In
@packages/lit-hotkeys/src/controllers/key-hold.ts:
- Around line 67-72: The code uses simple toLowerCase() for normalization (const
normalizedKey = this._key.toLowerCase()) and when comparing held keys, which can
diverge from KeyStateTracker's canonicalization; replace both uses with the same
canonical normalize routine from KeyStateTracker (e.g., call
KeyStateTracker.normalizeKey(this._key) or tracker.normalizeKey(...) depending
on where the helper lives) and compare against heldKey normalized via the same
function inside the subscribe predicate so all comparisons use the identical
canonical form.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `12770e03-2cff-4ee8-af7e-3a6c9f6e0fde` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2290cb5ac25fcf03a2ac7a11000273d2cdb249ab and ed83f61b2d5680cfaab77e29b5aa9312631b106b. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (78)</summary> * `.changeset/add-lit-adapter.md` * `.npmrc` * `docs/config.json` * `docs/devtools.md` * `docs/framework/lit/guides/formatting-display.md` * `docs/framework/lit/guides/hotkey-recording.md` * `docs/framework/lit/guides/hotkeys.md` * `docs/framework/lit/guides/key-state-tracking.md` * `docs/framework/lit/guides/sequence-recording.md` * `docs/framework/lit/guides/sequences.md` * `docs/framework/lit/quick-start.md` * `docs/framework/lit/reference/classes/HeldKeyCodesController.md` * `docs/framework/lit/reference/classes/HeldKeysController.md` * `docs/framework/lit/reference/classes/HotkeyController.md` * `docs/framework/lit/reference/classes/HotkeyRecorderController.md` * `docs/framework/lit/reference/classes/HotkeySequenceController.md` * `docs/framework/lit/reference/classes/KeyHoldController.md` * `docs/framework/lit/reference/functions/hotkey.md` * `docs/framework/lit/reference/functions/hotkeySequence.md` * `docs/framework/lit/reference/index.md` * `docs/installation.md` * `docs/overview.md` * `examples/lit/held-keys/index.html` * `examples/lit/held-keys/package.json` * `examples/lit/held-keys/src/app.ts` * `examples/lit/held-keys/src/index.css` * `examples/lit/held-keys/tsconfig.json` * `examples/lit/hotkey-recorder/index.html` * `examples/lit/hotkey-recorder/package.json` * `examples/lit/hotkey-recorder/src/app.ts` * `examples/lit/hotkey-recorder/src/index.css` * `examples/lit/hotkey-recorder/tsconfig.json` * `examples/lit/hotkey-sequence-recorder/index.html` * `examples/lit/hotkey-sequence-recorder/package.json` * `examples/lit/hotkey-sequence-recorder/src/app.ts` * `examples/lit/hotkey-sequence-recorder/src/index.css` * `examples/lit/hotkey-sequence-recorder/tsconfig.json` * `examples/lit/hotkey-sequence/.gitignore` * `examples/lit/hotkey-sequence/index.html` * `examples/lit/hotkey-sequence/package.json` * `examples/lit/hotkey-sequence/src/app.ts` * `examples/lit/hotkey-sequence/src/index.css` * `examples/lit/hotkey-sequence/tsconfig.json` * `examples/lit/hotkey/index.html` * `examples/lit/hotkey/package.json` * `examples/lit/hotkey/src/app.ts` * `examples/lit/hotkey/src/index.css` * `examples/lit/hotkey/tsconfig.json` * `examples/lit/key-hold/index.html` * `examples/lit/key-hold/package.json` * `examples/lit/key-hold/src/app.ts` * `examples/lit/key-hold/src/index.css` * `examples/lit/key-hold/tsconfig.json` * `package.json` * `packages/lit-hotkeys/CHANGELOG.md` * `packages/lit-hotkeys/README.md` * `packages/lit-hotkeys/eslint.config.js` * `packages/lit-hotkeys/package.json` * `packages/lit-hotkeys/src/constants.ts` * `packages/lit-hotkeys/src/controllers/held-key-codes.ts` * `packages/lit-hotkeys/src/controllers/held-keys.ts` * `packages/lit-hotkeys/src/controllers/hotkey-recorder.ts` * `packages/lit-hotkeys/src/controllers/hotkey-sequence-recorder.ts` * `packages/lit-hotkeys/src/controllers/hotkey-sequence.ts` * `packages/lit-hotkeys/src/controllers/hotkey.ts` * `packages/lit-hotkeys/src/controllers/index.ts` * `packages/lit-hotkeys/src/controllers/key-hold.ts` * `packages/lit-hotkeys/src/decorators/hotkey-sequence.ts` * `packages/lit-hotkeys/src/decorators/hotkey.ts` * `packages/lit-hotkeys/src/decorators/index.ts` * `packages/lit-hotkeys/src/index.ts` * `packages/lit-hotkeys/tests/hotkey-sequence.spec.ts` * `packages/lit-hotkeys/tests/hotkey.spec.ts` * `packages/lit-hotkeys/tsconfig.docs.json` * `packages/lit-hotkeys/tsconfig.json` * `packages/lit-hotkeys/tsdown.config.ts` * `packages/lit-hotkeys/vitest.config.ts` * `scripts/generate-docs.ts` </details> <details> <summary>✅ Files skipped from review due to trivial changes (57)</summary> * .npmrc * docs/devtools.md * examples/lit/hotkey-sequence-recorder/index.html * examples/lit/hotkey-recorder/index.html * .changeset/add-lit-adapter.md * packages/lit-hotkeys/CHANGELOG.md * examples/lit/held-keys/package.json * packages/lit-hotkeys/tsconfig.json * examples/lit/hotkey-sequence/index.html * examples/lit/hotkey-sequence/.gitignore * docs/config.json * examples/lit/held-keys/index.html * docs/framework/lit/reference/classes/HotkeyController.md * packages/lit-hotkeys/tsconfig.docs.json * examples/lit/hotkey-recorder/package.json * docs/framework/lit/reference/classes/HeldKeysController.md * examples/lit/key-hold/package.json * packages/lit-hotkeys/eslint.config.js * packages/lit-hotkeys/src/index.ts * examples/lit/hotkey-sequence-recorder/package.json * packages/lit-hotkeys/vitest.config.ts * docs/framework/lit/reference/classes/HotkeySequenceController.md * docs/framework/lit/reference/functions/hotkeySequence.md * examples/lit/hotkey-sequence-recorder/tsconfig.json * docs/framework/lit/reference/classes/HeldKeyCodesController.md * packages/lit-hotkeys/package.json * examples/lit/held-keys/tsconfig.json * docs/framework/lit/reference/classes/KeyHoldController.md * examples/lit/key-hold/tsconfig.json * examples/lit/hotkey-recorder/tsconfig.json * docs/framework/lit/reference/index.md * docs/framework/lit/reference/functions/hotkey.md * docs/framework/lit/quick-start.md * docs/framework/lit/guides/sequence-recording.md * packages/lit-hotkeys/src/constants.ts * examples/lit/hotkey-sequence/tsconfig.json * packages/lit-hotkeys/src/decorators/hotkey-sequence.ts * examples/lit/held-keys/src/index.css * examples/lit/hotkey/package.json * packages/lit-hotkeys/tsdown.config.ts * docs/framework/lit/guides/sequences.md * examples/lit/hotkey-sequence/package.json * examples/lit/key-hold/src/index.css * docs/framework/lit/reference/classes/HotkeyRecorderController.md * examples/lit/hotkey-sequence/src/index.css * examples/lit/key-hold/src/app.ts * docs/framework/lit/guides/hotkey-recording.md * docs/framework/lit/guides/key-state-tracking.md * packages/lit-hotkeys/src/controllers/index.ts * docs/overview.md * examples/lit/hotkey/src/index.css * examples/lit/hotkey-sequence-recorder/src/index.css * examples/lit/hotkey/tsconfig.json * package.json * examples/lit/hotkey-recorder/src/index.css * examples/lit/hotkey-recorder/src/app.ts * packages/lit-hotkeys/tests/hotkey.spec.ts </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (11)</summary> * examples/lit/key-hold/index.html * examples/lit/hotkey/index.html * scripts/generate-docs.ts * packages/lit-hotkeys/src/controllers/held-keys.ts * packages/lit-hotkeys/src/controllers/held-key-codes.ts * packages/lit-hotkeys/src/decorators/index.ts * packages/lit-hotkeys/src/controllers/hotkey-recorder.ts * packages/lit-hotkeys/tests/hotkey-sequence.spec.ts * examples/lit/hotkey/src/app.ts * packages/lit-hotkeys/src/controllers/hotkey-sequence-recorder.ts * docs/installation.md </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
Author
|
@KevinVandy i hope an initial implementation for the lit adapter is ready. I am happy for any feedback. A dev-tools have not been implemented yet. Might be able to look into that later. |
KevinVandy
changed the title
Merged
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
2 participants
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
Add Lit Adapter for Hotkeys
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores
Tests