Conversation
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3818/ |
📝 WalkthroughWalkthroughAdds a hotkey subsystem: normalization utilities, a visual/interactive Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant Hotkey as limel-hotkey
participant Registry as HotkeyRegistry
participant Consumer as EventConsumer
User->>Document: keydown
Document->>Hotkey: global keydown listener (capture)
Hotkey->>Hotkey: normalize hotkey (cache)
Hotkey->>Registry: lookup enabled handlers for canonical hotkey
alt single handler
Registry-->>Hotkey: handler found
Hotkey->>Document: preventDefault / stopPropagation (if configured)
Hotkey->>Consumer: emit hotkeyTrigger(detail)
else duplicates
Registry-->>Hotkey: multiple handlers
Hotkey->>Hotkey: console.warn, pick first enabled
Hotkey->>Consumer: emit hotkeyTrigger(detail for first)
else none
Hotkey-->>Document: allow default
end
sequenceDiagram
participant User
participant UI as Menu UI
participant Menu as limel-menu
participant Surface as MenuSurface
participant MenuHotkey as MenuHotkeyHandler
participant Items as MenuItems
User->>UI: open menu
UI->>Menu: opens
Menu->>Surface: attach surface element (setSurfaceElement)
User->>Surface: press hotkey (e.g., Alt+C)
Surface->>MenuHotkey: receives hotkeyTrigger / keydown
MenuHotkey->>MenuHotkey: normalize hotkey, ignore reserved keys
MenuHotkey->>Items: find visible & enabled item by hotkey
alt match found
Items-->>MenuHotkey: matching item
MenuHotkey->>Menu: trigger select(item)
Menu->>UI: close menu, emit select event
else no match or reserved
MenuHotkey-->>Surface: ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Kiarokh
force-pushed
the
menu-hotkeys
branch
2 times, most recently
from
e2c148e to
530a80c
Compare
Kiarokh
added
feature
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/menu/examples/menu-hotkeys.tsx (1)
73-88: 🛠️ Refactor suggestion | 🟠 MajorWrap multiple JSX elements in
<Host>instead of an array literal.Per the coding guidelines for StencilJS
.tsxfiles, when returning multiple top-level JSX elements from therendermethod, they should be wrapped in the special<Host>element rather than an array literal.♻️ Proposed fix
+import { Host } from '@stencil/core';public render() { - return [ + return ( + <Host> + <limel-menu items={this.items} onSelect={this.handleSelect}> + <limel-icon-button + label="Menu" + icon="menu_2" + elevated={true} + slot="trigger" + /> + </limel-menu> + <limel-example-value + label="Last selected item" + value={this.lastSelectedItem} + /> + </Host> + ); - <limel-menu items={this.items} onSelect={this.handleSelect}> - <limel-icon-button - label="Menu" - icon="menu_2" - elevated={true} - slot="trigger" - /> - </limel-menu>, - <limel-example-value - label="Last selected item" - value={this.lastSelectedItem} - />, - ]; }As per coding guidelines: "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.tsx`:
- Around line 367-385: In findMenuItemByHotkey, remove the redundant type cast
by relying on the isMenuItem type guard: replace the line that assigns rawHotkey
= (item as MenuItem).hotkey with a direct access const rawHotkey = item.hotkey;
(keep the existing checks using this.isMenuItem, this.getNormalizedHotkey,
item.disabled and this.visibleItems unchanged).
In `@src/util/hotkeys.ts`:
- Around line 47-55: The return logic in the normalization branch is redundant:
after checking if /^f\d{1,2}$/.test(normalized) and returning normalized, the
subsequent if (normalized.length === 1) branch also returns normalized and the
final return returns normalized again; simplify by removing the length check and
directly returning normalized if the regex test fails. Update the hotkey
normalization function to keep the /^f\d{1,2}$/ early return and replace the
remaining branches with a single return normalized to preserve existing behavior
(refer to the normalized variable and the regex check).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.e2e.ts`:
- Around line 265-306: Tests use fixed waits (page.waitForTimeout(200/50)),
causing flakiness; replace those with deterministic checks: after
limelMenu.setProperty('items') and trigger.click(), wait for the menu to be open
or the focused item to appear (for example, await page.waitForFunction to assert
trigger.getAttribute('aria-expanded') === 'true' or wait for a focused menu item
via page.find('li[aria-selected="true"], li:focus') ) before pressing keys with
page.keyboard.press('Enter'/'ArrowDown'), and remove the waitForTimeout calls so
selection assertions (spy on 'select') run only after the menu/focus state is
confirmed.
In `@src/util/hotkeys.ts`:
- Around line 203-231: The isKeyboardEventFromTextInput function currently
treats missing composedPath() as an empty path and can miss text inputs; update
isKeyboardEventFromTextInput to fall back to a single-node path using
event.target (and/or event.srcElement for legacy IE) when composedPath is not
available, then run the same checks (isContentEditable, tagName
INPUT/TEXTAREA/SELECT, role="textbox") against that fallback node so hotkeys
won't fire while typing in unsupported browsers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 66-73: normalizeHotkeyString currently splits on "+" and drops
empty tokens so a lone this.value of "+" becomes null and can never match
hotkeyFromKeyboardEvent; fix by special-casing the exact string "+" inside
normalizeHotkeyString (or the caller before normalizing) to return the canonical
token that normalizeKey expects (e.g., map "+" to the supported token name) so
pressed comparisons work, or alternatively update the docs/examples to use the
supported token instead of "+"; ensure the mapping uses the same normalized form
returned by normalizeKey so the pressed === expected check succeeds.
dd96fcb to
1a3d2c4
Compare
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 55-61: The mapped <kbd> elements rendered in parts.map(...) lack a
key which breaks the useJsxKeyInIterable lint rule; update the map callback in
the render (the parts.map producing <kbd> and calling this.formatDisplayToken)
to provide a unique key for each <kbd> — e.g., use the part value or an index
fallback (key={part} or key={`${part}-${i}`}) so each <kbd> inside Host has a
stable key.
In `@src/util/device.ts`:
- Around line 49-60: The return expression in isAppleDevice() is redundant
because isIPadOS13Plus is defined as (isMacLike && getMaxTouchPoints() > 1), so
including it separately is unnecessary; update the function to simplify the
final return to "isIPadIPhoneIPod || isMacLike" (or remove isIPadOS13Plus usage
while keeping the variable for documentation if preferred) and ensure variables
getUserAgent(), getPlatform(), isIPadIPhoneIPod, isMacLike, and isIPadOS13Plus
are left intact for clarity.
In `@src/util/hotkeys.spec.ts`:
- Around line 14-40: Add unit tests for hotkeyFromKeyboardEvent to cover
modifier combinations, function keys, arrow keys, and code-based normalization:
add a test like "includes multiple modifiers" that asserts
hotkeyFromKeyboardEvent returns "ctrl+alt+a" for an event with key 'a' and
ctrlKey/altKey true; add "handles function keys" asserting 'f1' and 'f12' for
events with key 'F1'/'F12'; add "handles arrow keys" asserting
'arrowleft'/'arrowright' for ArrowLeft/ArrowRight keys; and add "uses event.code
for non-QWERTY" that simulates a non-letter key where key differs from code
(e.g., key='@' code='Quote' or code-based letter) to ensure the function
normalizes using event.code when appropriate by comparing expected normalized
output from hotkeyFromKeyboardEvent.
a69816e to
bf75cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 55-61: The kbd elements rendered in the Host mapping currently set
aria-hidden="true" which triggers the a11y lint rule; remove the aria-hidden
attribute from the <kbd> elements in the parts.map render soScreen readers can
access the keys while keeping the Host's aria-label (refer to Host, parts.map
and formatDisplayToken in hotkey.tsx to locate the code). Ensure no other
aria-hidden is added and that formatDisplayToken continues to produce the
visible label for each kbd.
In `@src/components/menu/menu.e2e.ts`:
- Around line 362-381: The test dispatches the KeyboardEvent to
limel-menu-surface immediately after clicking the trigger, which can race if the
surface hasn't opened; after clicking the trigger (the variable trigger and the
call to trigger.click()/page.waitForChanges()), wait deterministically for the
surface to exist and be open/visible (e.g., use page.waitForSelector or a short
polling loop to confirm document.querySelector('limel-menu-surface') is present
and has the open/visible state) before executing the evaluated block that
dispatches the KeyboardEvent; update the code that finds limel-menu-surface and
dispatches the event (the evaluate callback) to run only once that deterministic
wait completes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 78-83: The UI shows 'meta' as 'Ctrl' for non-Apple (case 'meta' /
'cmd' / 'command' returning isApple ? '⌘' : 'Ctrl') but matching still expects
Meta, so fix by normalizing the incoming hotkey tokens when not on Apple: in the
hotkey normalization function (the code that processes 'lower' tokens before
matching, e.g., where you switch on lower) map 'meta'/'cmd'/'command' → 'ctrl'
for non-Apple platforms so both display and matching align (alternatively, if
you prefer to keep matching as Meta, change the display branch to render
'Meta'/'Win' instead—choose one consistent approach and apply it in the switch
handling and the hotkey normalization logic).
| ); | ||
| } | ||
|
|
||
| private formatDisplayToken(token: string, isApple: boolean): string { |
There was a problem hiding this comment.
Does this need to live inside the component? We can move it to a separate function for easier testing
There was a problem hiding this comment.
it could, but I also wanted to use it to add a class to render the glyphs a bit larger than the letters. added here: 195e489
Is it a big deal?
There was a problem hiding this comment.
No it's not 😄 Just separation of concerns
| if ((event as any)[Hotkey.handledFlag]) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why do we need this check? Do we expect the event to propagate though multiple hotkeys? It does not look like they will ever be nested inside each other
There was a problem hiding this comment.
I added it because of some discussions I had with a 🤖 friend, when trying to mitigate buggy behaviours, due to having multiple instances of the same component potentially rendered all at once.
Asked it to explain, and it said:
That check is there to prevent duplicate handling of the same physical keypress when there are multiple limel-hotkey instances on the page, not because they’re nested.
What happens otherwise:
Every
limel-hotkeyregisters@Listen('keydown', { target: 'document', capture: true }).
A single keydown will invokehandleKeyDown()once per instance.
If you render several hotkeys (like in the example, or a menu with many items), each instance will evaluate the event; without a guard, more than one instance can emit if they happen to match (or if you accidentally have duplicates).
Why a flag on the event:
stopPropagation()isn’t ideal here because we’re listening on document in capture and we don’t want to interfere with other global handlers in the app.
The per-event flag makes it “handled once” across alllimel-hotkeylisteners, regardless of render order.
So yes: we do expect the same event to “reach” multiple hotkey components because they all listen globally, even though they aren’t nested.If you’re confident you’ll never have duplicate hotkeys active at the same time, you could remove it—but in menus/examples it’s very easy to have multiple limel-hotkey elements present, so it’s a safety net.
There was a problem hiding this comment.
But with this check, it will be unclear which of those hotkeys will be triggered. There is no way for the consumer to know if their instance will be allowed or blocked by this check. Is that the behavior we want? The way it will work now will most likely be the component that renders first is the one that will receive the event
There was a problem hiding this comment.
Hm… u r right.
what's a better approach? Just remove it?
There was a problem hiding this comment.
Yeah I think we should remove it. What does the 🤖 say?
There was a problem hiding this comment.
Now I've kept the mechanics to trigger a console warning when a duplicate hotkey is identified. ec3a5c0
This way, the limel-hotkey component will only work for the first instance, and reduces a little bit of the confusion risk for the end-user. And the consumer can hopefully see a warning in the console.
Most of this responsibility is on the consumer anyway. But it's good to have some error prevention in the component itself as well.
Kiarokh
force-pushed
the
menu-hotkeys
branch
from
a4ac606 to
909b857
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (14)
src/util/device.ts (2)
1-21: Explicit return-type annotations on private helpers (pre-existing)The three unexported helpers carry explicit
: string/: numberannotations that TypeScript infers correctly on its own. This was already raised in a previous review round and remains unaddressed.Based on learnings: "prefer omitting explicit return type annotations for functions when TypeScript inference is sufficient; only add an explicit return type when inference would be insufficient or incorrect."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/device.ts` around lines 1 - 21, Remove the redundant explicit return-type annotations from the three unexported helper functions so TypeScript can infer them automatically: delete ": string" from getUserAgent and getPlatform and ": number" from getMaxTouchPoints; keep the existing runtime guards and returns intact and do not change the function bodies or exports (these are private helpers so inferred types are sufficient).
49-60: RedundantisIPadOS13Plusin the return expression (pre-existing)
isIPadOS13Plusis defined asisMacLike && getMaxTouchPoints() > 1, making it a strict subset ofisMacLike. Including it inisIPadIPhoneIPod || isIPadOS13Plus || isMacLikeis therefore redundant. This was already flagged in a previous review round.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/device.ts` around lines 49 - 60, The isIPadOS13Plus local is redundant because it is defined as isMacLike && getMaxTouchPoints() > 1 and then OR'd with isMacLike in isAppleDevice(); update isAppleDevice() to return isIPadIPhoneIPod || isMacLike (and remove the unused isIPadOS13Plus variable and its getMaxTouchPoints check), keeping helper calls getUserAgent and getPlatform as-is so only the redundant branch is eliminated.src/components/menu-list/menu-list-renderer.tsx (1)
108-108: Redundant'hotkey' in itemguard — still flagged from the previous review.After the separator early-return TypeScript narrows
itemtoMenuItem, making theincheck unnecessary. The previous suggestion to simplify to!!(item as MenuItem).hotkeyremains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu-list/menu-list-renderer.tsx` at line 108, The guard "'hotkey' in item" is redundant after the early-return narrow to MenuItem; replace the check for hasHotkey with a simple truthy cast like !!(item as MenuItem).hotkey to rely on TypeScript narrowing and remove the unnecessary 'in' operator; update the declaration that defines hasHotkey (the const hasHotkey variable in menu-list-renderer.tsx) and ensure any references assume item is a MenuItem.src/components/hotkey/hotkey.types.ts (1)
1-5:LimelHotkeyTriggerDetailis still missing JSDoc.The type is part of the public API (imported from
@limetech/lime-elements) but has no description or property-level documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.types.ts` around lines 1 - 5, Add JSDoc to the public type LimelHotkeyTriggerDetail describing its purpose and each property: a short description for the type itself (e.g., "Details provided when a Limel hotkey is triggered"), then property tags for hotkey (string identifier for the hotkey), value (string payload or label associated with the trigger), and keyboardEvent (the original KeyboardEvent instance). Place the JSDoc immediately above the exported type definition so the documentation appears in the public API and tooling.src/components/markdown/partial-styles/_kbd.scss (1)
26-29:0.375pxblur radius still inconsistent with the surroundingremvalues.All other offsets in the
box-shadowdeclaration userem. The past review comment flagging this remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The box-shadow blur radius uses a pixel value (0.375px) while all other offsets use rems; update the blur radius in the box-shadow declaration in _kbd.scss to an equivalent rem value (e.g., 0.0234375rem if using 16px root) so units are consistent with the other offsets, keeping the rest of the box-shadow entries and var(--button-shadow-normal) unchanged.src/util/hotkeys.spec.ts (1)
1-130: Existing test cases are correct; missing coverage from prior review still outstanding.
isKeyboardEventFromTextInput,normalizeHotkeyStringedge cases (empty/null input, modifier-only strings such as"ctrl+shift"), and directtokenizeHotkeyStringtests were requested in the previous review cycle and remain absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.spec.ts` around lines 1 - 130, Add unit tests to cover the missing hotkeys edge cases: create new specs in src/util/hotkeys.spec.ts that (1) call normalizeHotkeyString with empty string and null and assert it returns the normalized empty result (e.g., ''), and assert modifier-only inputs like "ctrl+shift" produce the normalized modifier-only token sequence (e.g., "ctrl+shift"); (2) directly test tokenizeHotkeyString with cases including single "+" and "++" and mixed modifier+key strings to verify tokenization; and (3) add tests for isKeyboardEventFromTextInput covering typical text-input events versus non-text events (e.g., key 'a' on input/textarea targets vs function keys or modifier-only events) to assert the boolean result; if any test fails, update the corresponding functions (normalizeHotkeyString, tokenizeHotkeyString, isKeyboardEventFromTextInput) to conform to these expected behaviors.src/components/menu/menu.tsx (1)
308-337: Inline type duplicatesLimelHotkeyTriggerDetail; import the shared type instead.The inline type on lines 309–313 is identical to the
LimelHotkeyTriggerDetailtype inhotkey.types.ts. Using the shared type avoids drift if the type changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.tsx` around lines 308 - 337, The inline event detail type used in the handleHotkeyTrigger method duplicates the shared LimelHotkeyTriggerDetail; replace the inline type with an import of LimelHotkeyTriggerDetail from hotkey.types and annotate the event parameter as CustomEvent<LimelHotkeyTriggerDetail> in the handleHotkeyTrigger signature, adding the import at the top of the file and leaving the method body unchanged (method name: handleHotkeyTrigger).src/components/list-item/menu-item-meta/menu-item-meta.tsx (1)
59-64: Hotkeys in closed menus may still steal keyboard events from the rest of the application.
limel-hotkeyregisters a document-levelkeydownlistener and can callpreventDefault()+ set thehandledFlagwhen a matching key is pressed. Iflimel-portalkeeps the DOM alive (hidden but attached) whenvisible=false, theselimel-hotkeyinstances remain active and will intercept keypresses even when the menu is closed.The menu's
handleHotkeyTriggerhas a!this.openguard, but that only prevents selection — it doesn't prevent thelimel-hotkeycomponent itself from consuming the keyboard event before it reaches other handlers.Consider threading the menu's
openstate intodisabledhere:-<limel-hotkey value={this.hotkey} disabled={this.disabled} /> +<limel-hotkey value={this.hotkey} disabled={this.disabled || !this.menuOpen} />(This would require a new
menuOpenprop propagated from the menu.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx` around lines 59 - 64, renderCommandText currently renders <limel-hotkey value={this.hotkey} disabled={this.disabled} /> which leaves hotkeys active even when the menu is closed (limel-hotkey listens at document level); update the component to accept a new boolean prop (e.g., menuOpen) and pass combinedDisabled = this.disabled || !this.menuOpen into renderCommandText so the <limel-hotkey> receives disabled when the menu is closed, add the new menuOpen prop to the menu item component and ensure the parent menu propagates its open state down to this component.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:meta+enterandcmd+enternormalize to the same hotkey, causing a duplicate-hotkey console warning.Since
cmdis an alias formeta, both entries register under the same normalized key. This will trigger the duplicate-detectionconsole.warnevery time the hotkey is pressed in this basic example, which is confusing for readers treating this as a reference. Consider removing one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example registers the same hotkey twice because "cmd" aliases "meta", causing duplicate-normalization warnings; remove one of the duplicate <limel-hotkey> entries (either the limel-hotkey with value="meta+enter" or the one with value="cmd+enter") in the hotkey-basic example so only a single <limel-hotkey> element registers the Enter shortcut; update the JSX in the hotkey-basic example where the <limel-hotkey> elements are declared to eliminate the duplicate.src/components/hotkey/hotkey.scss (1)
16-20: Possible unit mismatch inbox-shadow:0.375pxmay be a typo for0.375rem.The blur radius uses
pxwhile the surrounding values userem, which seems inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.scss` around lines 16 - 20, The box-shadow declaration contains a likely unit mismatch: change the blur radius in the rule that sets "box-shadow" (currently "0 0.625rem 0.375px -0.5rem ...") to use rem instead of px so it matches the other values (e.g., "0.375rem"); update the box-shadow value used in the hotkey component's stylesheet accordingly.src/components/hotkey/hotkey.tsx (1)
133-183: Missingwinandwindowscases informatDisplayToken.
normalizeKeyinhotkeys.tsmapswin/windowstometa, butformatDisplayTokendoesn't handle these tokens. If a consumer passesvalue="win+c", the "win" token renders as raw text instead of⊞ Winor⌘.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 183, formatDisplayToken is missing handling for the "win" and "windows" tokens so keys like "win+c" render raw; update the switch in formatDisplayToken to treat 'win' and 'windows' the same as 'meta' (or call normalizeKey before switching) so they return the platform-specific glyph/display (e.g., ⊞ Win or ⌘) — reference the formatDisplayToken function and the existing 'meta' case to add 'win' and 'windows' alternatives.src/util/hotkeys.ts (3)
100-103: Redundant function-key guard — the branch is a no-op.Both the
ifbody and the fallthrough at line 104 returnnormalized; the regex check is dead code.♻️ Proposed fix
- if (/^f\d{1,2}$/.test(normalized)) { - return normalized; - } - return normalized;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 100 - 103, The regex branch checking /^f\d{1,2}$/ is redundant because it returns the same value as the fallthrough; remove the entire if block (the /^f\d{1,2}$/.test(normalized) check and its return) from the hotkey normalization function so there is a single return normalized; path, or if special handling for function keys was intended, implement the intended behavior inside the function (e.g., transform or prefix function keys) instead of a no-op; refer to the normalized variable and the regex /^f\d{1,2}$/ in hotkeys.ts to locate the dead branch.
29-30:backspace: 'delete'alias collapses two physically distinct keys — document the intent.On Windows/Linux,
Backspace(event.key = "Backspace") andDelete(event.key = "Delete") are distinct keys. With this alias both map to the canonical string"delete", making them indistinguishable to consumers. The JSDoc at line 15 shows the output but not the rationale. If this cross-platform unification is intentional, add a brief comment so future maintainers don't inadvertently "fix" it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 29 - 30, Summary: The mapping that sets backspace: 'delete' collapses two distinct physical keys into one canonical "delete" value without explanation; either document this cross-platform unification or separate them. Fix: in src/util/hotkeys.ts, update the hotkey canonicalization mapping (the entry backspace: 'delete' and the related del: 'delete') — if the unification is intentional, add a one-line comment above the mapping (and update the JSDoc near the existing doc comment around the mapping) explaining that Backspace and Delete are normalized to "delete" for cross-platform consistency; if it was unintentional, change the mapping so Backspace maps to a distinct canonical value (e.g., 'backspace') and adjust any consumer code that relies on the current canonical string. Ensure you reference the mapping keys backspace and del when making the change.
20-20: 🧹 Nitpick | 🔵 Trivial
KEY_ALIASESshould beReadonlyto prevent accidental property mutation.
constprevents reference reassignment but leaves the entries mutable —KEY_ALIASES['x'] = 'y'compiles and silently corrupts normalization for all callers.constfreezes the reference;readonlyfreezes the contents.♻️ Proposed fix
-const KEY_ALIASES: Record<string, string> = { +const KEY_ALIASES: Readonly<Record<string, string>> = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` at line 20, Change the KEY_ALIASES declaration to an immutable type so its entries can’t be mutated: update the declaration of KEY_ALIASES to use Readonly<Record<string, string>> (e.g. const KEY_ALIASES: Readonly<Record<string, string>> = { ... }) and, if you want runtime enforcement, call Object.freeze(KEY_ALIASES) after initialization; this prevents accidental writes like KEY_ALIASES['x'] = 'y' while keeping the rest of normalization logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.e2e.tsx`:
- Around line 177-219: The test "does not allow reserved keys (Enter) to be used
as item hotkeys" currently only asserts that no select events match the "Second"
item, but this can be confusing because Enter may still activate the focused
item; update the test (or its description) to clarify intent by adding a short
comment or adjusting the it(...) title to state it only verifies that Enter does
not trigger hotkey-based selection for the "Second" item (e.g., mention that
native activation of the focused item may still fire a select for "First"), and
keep all existing logic (handler, hotkeyItems, surface dispatchEvent) unchanged
so readers know this test is scoped to hotkey matching rather than general Enter
behavior.
In `@src/components/menu/menu.types.ts`:
- Around line 64-65: The JSDoc above the hotkey property currently reads
"Hotkey(s)..." but the TypeScript type is a single string; update the comment
for the property (the doc for the hotkey/Hotkey field in menu.types.ts) to
singular wording — e.g., "Hotkey that can be used to select this item while the
menu is open." — so the documentation matches the type signature.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example registers the same hotkey twice because "cmd"
aliases "meta", causing duplicate-normalization warnings; remove one of the
duplicate <limel-hotkey> entries (either the limel-hotkey with
value="meta+enter" or the one with value="cmd+enter") in the hotkey-basic
example so only a single <limel-hotkey> element registers the Enter shortcut;
update the JSX in the hotkey-basic example where the <limel-hotkey> elements are
declared to eliminate the duplicate.
In `@src/components/hotkey/hotkey.scss`:
- Around line 16-20: The box-shadow declaration contains a likely unit mismatch:
change the blur radius in the rule that sets "box-shadow" (currently "0 0.625rem
0.375px -0.5rem ...") to use rem instead of px so it matches the other values
(e.g., "0.375rem"); update the box-shadow value used in the hotkey component's
stylesheet accordingly.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 133-183: formatDisplayToken is missing handling for the "win" and
"windows" tokens so keys like "win+c" render raw; update the switch in
formatDisplayToken to treat 'win' and 'windows' the same as 'meta' (or call
normalizeKey before switching) so they return the platform-specific
glyph/display (e.g., ⊞ Win or ⌘) — reference the formatDisplayToken function and
the existing 'meta' case to add 'win' and 'windows' alternatives.
In `@src/components/hotkey/hotkey.types.ts`:
- Around line 1-5: Add JSDoc to the public type LimelHotkeyTriggerDetail
describing its purpose and each property: a short description for the type
itself (e.g., "Details provided when a Limel hotkey is triggered"), then
property tags for hotkey (string identifier for the hotkey), value (string
payload or label associated with the trigger), and keyboardEvent (the original
KeyboardEvent instance). Place the JSDoc immediately above the exported type
definition so the documentation appears in the public API and tooling.
In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx`:
- Around line 59-64: renderCommandText currently renders <limel-hotkey
value={this.hotkey} disabled={this.disabled} /> which leaves hotkeys active even
when the menu is closed (limel-hotkey listens at document level); update the
component to accept a new boolean prop (e.g., menuOpen) and pass
combinedDisabled = this.disabled || !this.menuOpen into renderCommandText so the
<limel-hotkey> receives disabled when the menu is closed, add the new menuOpen
prop to the menu item component and ensure the parent menu propagates its open
state down to this component.
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The box-shadow blur radius uses a pixel value (0.375px)
while all other offsets use rems; update the blur radius in the box-shadow
declaration in _kbd.scss to an equivalent rem value (e.g., 0.0234375rem if using
16px root) so units are consistent with the other offsets, keeping the rest of
the box-shadow entries and var(--button-shadow-normal) unchanged.
In `@src/components/menu-list/menu-list-renderer.tsx`:
- Line 108: The guard "'hotkey' in item" is redundant after the early-return
narrow to MenuItem; replace the check for hasHotkey with a simple truthy cast
like !!(item as MenuItem).hotkey to rely on TypeScript narrowing and remove the
unnecessary 'in' operator; update the declaration that defines hasHotkey (the
const hasHotkey variable in menu-list-renderer.tsx) and ensure any references
assume item is a MenuItem.
In `@src/components/menu/menu.tsx`:
- Around line 308-337: The inline event detail type used in the
handleHotkeyTrigger method duplicates the shared LimelHotkeyTriggerDetail;
replace the inline type with an import of LimelHotkeyTriggerDetail from
hotkey.types and annotate the event parameter as
CustomEvent<LimelHotkeyTriggerDetail> in the handleHotkeyTrigger signature,
adding the import at the top of the file and leaving the method body unchanged
(method name: handleHotkeyTrigger).
In `@src/util/device.ts`:
- Around line 1-21: Remove the redundant explicit return-type annotations from
the three unexported helper functions so TypeScript can infer them
automatically: delete ": string" from getUserAgent and getPlatform and ":
number" from getMaxTouchPoints; keep the existing runtime guards and returns
intact and do not change the function bodies or exports (these are private
helpers so inferred types are sufficient).
- Around line 49-60: The isIPadOS13Plus local is redundant because it is defined
as isMacLike && getMaxTouchPoints() > 1 and then OR'd with isMacLike in
isAppleDevice(); update isAppleDevice() to return isIPadIPhoneIPod || isMacLike
(and remove the unused isIPadOS13Plus variable and its getMaxTouchPoints check),
keeping helper calls getUserAgent and getPlatform as-is so only the redundant
branch is eliminated.
In `@src/util/hotkeys.spec.ts`:
- Around line 1-130: Add unit tests to cover the missing hotkeys edge cases:
create new specs in src/util/hotkeys.spec.ts that (1) call normalizeHotkeyString
with empty string and null and assert it returns the normalized empty result
(e.g., ''), and assert modifier-only inputs like "ctrl+shift" produce the
normalized modifier-only token sequence (e.g., "ctrl+shift"); (2) directly test
tokenizeHotkeyString with cases including single "+" and "++" and mixed
modifier+key strings to verify tokenization; and (3) add tests for
isKeyboardEventFromTextInput covering typical text-input events versus non-text
events (e.g., key 'a' on input/textarea targets vs function keys or
modifier-only events) to assert the boolean result; if any test fails, update
the corresponding functions (normalizeHotkeyString, tokenizeHotkeyString,
isKeyboardEventFromTextInput) to conform to these expected behaviors.
In `@src/util/hotkeys.ts`:
- Around line 100-103: The regex branch checking /^f\d{1,2}$/ is redundant
because it returns the same value as the fallthrough; remove the entire if block
(the /^f\d{1,2}$/.test(normalized) check and its return) from the hotkey
normalization function so there is a single return normalized; path, or if
special handling for function keys was intended, implement the intended behavior
inside the function (e.g., transform or prefix function keys) instead of a
no-op; refer to the normalized variable and the regex /^f\d{1,2}$/ in hotkeys.ts
to locate the dead branch.
- Around line 29-30: Summary: The mapping that sets backspace: 'delete'
collapses two distinct physical keys into one canonical "delete" value without
explanation; either document this cross-platform unification or separate them.
Fix: in src/util/hotkeys.ts, update the hotkey canonicalization mapping (the
entry backspace: 'delete' and the related del: 'delete') — if the unification is
intentional, add a one-line comment above the mapping (and update the JSDoc near
the existing doc comment around the mapping) explaining that Backspace and
Delete are normalized to "delete" for cross-platform consistency; if it was
unintentional, change the mapping so Backspace maps to a distinct canonical
value (e.g., 'backspace') and adjust any consumer code that relies on the
current canonical string. Ensure you reference the mapping keys backspace and
del when making the change.
- Line 20: Change the KEY_ALIASES declaration to an immutable type so its
entries can’t be mutated: update the declaration of KEY_ALIASES to use
Readonly<Record<string, string>> (e.g. const KEY_ALIASES:
Readonly<Record<string, string>> = { ... }) and, if you want runtime
enforcement, call Object.freeze(KEY_ALIASES) after initialization; this prevents
accidental writes like KEY_ALIASES['x'] = 'y' while keeping the rest of
normalization logic intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (18)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-hotkeys
branch
from
909b857 to
7cab23f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (19)
src/util/device.ts (1)
1-21: 🧹 Nitpick | 🔵 TrivialExplicit return-type annotations on unexported helpers are still present.
getUserAgent(),getPlatform(), andgetMaxTouchPoints()are unexported; TypeScript infers their return types exactly. The annotations add no contract value.♻️ Proposed simplification
-function getUserAgent(): string { +function getUserAgent() { -function getPlatform(): string { +function getPlatform() { -function getMaxTouchPoints(): number { +function getMaxTouchPoints() {Based on learnings: "prefer omitting explicit return type annotations for functions when TypeScript inference is sufficient."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/device.ts` around lines 1 - 21, Remove the redundant explicit return-type annotations from the unexported helper functions getUserAgent, getPlatform, and getMaxTouchPoints: let TypeScript infer the return types by deleting the ": string" and ": number" annotations on those function declarations while keeping their bodies and runtime behavior unchanged (no changes to exported APIs).src/components/menu-list/menu-list-renderer.tsx (1)
108-108: 🧹 Nitpick | 🔵 TrivialRedundant
'hotkey' in itemguard — simplify to a truthiness check.After the separator early-return at line 89, TypeScript narrows
itemtoMenuItem, which means accessing an optional property that is absent returnsundefined. Theincheck is superfluous.♻️ Proposed simplification
- const hasHotkey = 'hotkey' in item && !!(item as any).hotkey; + const hasHotkey = !!(item as MenuItem).hotkey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu-list/menu-list-renderer.tsx` at line 108, The `'hotkey' in item` check is redundant because `item` is already narrowed to MenuItem after the earlier separator early-return; replace the current declaration of hasHotkey (const hasHotkey = 'hotkey' in item && !!(item as any).hotkey;) with a simple truthiness check like const hasHotkey = !!(item as MenuItem).hotkey or const hasHotkey = Boolean(item.hotkey) to remove the unnecessary `in` guard and keep proper typing.src/components/markdown/partial-styles/_kbd.scss (1)
26-29:⚠️ Potential issue | 🟡 Minor
0.375pxblur radius — same sub-pixel issue as inhotkey.scss.Since
hotkey.scss@forwards this file, this is the source of the mismatch. All other shadow values userem; the blur should be0.375rem.🐛 Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The box-shadow in _kbd.scss uses a sub-pixel blur "0.375px" which causes a unit mismatch (hotkey.scss forwards this file); change that blur value to "0.375rem" so it matches the other shadow values and the rest of the codebase. Locate the box-shadow declaration in src/components/markdown/partial-styles/_kbd.scss and replace the "0.375px" blur term with "0.375rem" (ensure consistency with hotkey.scss which forwards this file).src/components/hotkey/hotkey.scss (1)
16-20:⚠️ Potential issue | 🟡 Minor
0.375pxblur radius — still effectively zero in most browsers.The second shadow layer mixes
remvalues everywhere except the blur radius (0.375px). A sub-pixel blur rounds to 0–1 px; the intended value is likely0.375rem(~6 px).🐛 Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.scss` around lines 16 - 20, The box-shadow declaration's second shadow layer uses a sub-pixel blur "0.375px", which is effectively zero in many browsers; update the blur radius in the box-shadow property (the line starting with "0 0.625rem 0.375px -0.5rem rgb(...)") to use "0.375rem" instead of "0.375px" so the blur scales with rem units and produces the intended visible blur.src/components/menu/menu.types.ts (1)
64-66:⚠️ Potential issue | 🟡 Minor
"Hotkey(s)"— plural is misleading for astringtype.The parenthetical
(s)suggestsstring | string[], but the property only accepts a single string.📝 Proposed fix
- * Hotkey(s) that can be used to select this item while the menu is open. + * A hotkey that can be used to select this item while the menu is open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.types.ts` around lines 64 - 66, The JSDoc "Hotkey(s) that can be used..." is misleading because the property it documents is typed as a single string; update the JSDoc above the hotkey property in src/components/menu/menu.types.ts to use singular wording (e.g., "Hotkey that can be used to select this item while the menu is open.") or alternatively change the property's type to string | string[] if multiple hotkeys were intended; adjust either the comment or the property type for consistency with the property name (hotkey/hotkeys) used in the file.src/components/hotkey/hotkey.tsx (2)
61-62:@Eventoptions are redundant — StencilJS defaults tobubbles: true, composed: true.
bubblesdefaults totrueandcomposeddefaults totruein StencilJS, so the explicit object{ bubbles: true, composed: true }is a no-op. This was already noted by a human reviewer. If you want to keep the options explicit for documentation clarity that's fine, but otherwise@Event()alone is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 61 - 62, The `@Event` decorator on the hotkeyTrigger EventEmitter is using redundant options ({ bubbles: true, composed: true }) because StencilJS already defaults those to true; remove the explicit options and change the decorator to `@Event`() for the hotkeyTrigger field (symbol: hotkeyTrigger) to eliminate the no-op, or keep the explicit object only if you intentionally want to preserve inline documentation clarity.
133-182:formatDisplayTokenis missing'win'and'windows'aliases.The documentation (e.g.,
menu-hotkeys.tsxline 26) explicitly listswinandwindowsas aliases formeta.normalizeKeyinhotkeys.tsalready maps them tometafor matching, butformatDisplayToken'sswitchhas nocase 'win': case 'windows':, so they fall through toreturn { display: trimmed, isGlyph: false }and render as the raw strings"win"/"windows"instead of the proper platform glyph.🔧 Proposed fix
case 'meta': { return isApple ? { display: '⌘', isGlyph: true } : { display: '⊞ Win', isGlyph: false }; } + case 'win': + case 'windows': { + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; + } + case 'cmd':🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 182, formatDisplayToken fails to special-case the 'win' and 'windows' aliases so those tokens render as raw text instead of mapping to the platform meta glyph; update the switch in formatDisplayToken to include case 'win': and case 'windows': (grouped with the existing 'meta' branch) so they return the same { display: '⌘' / '⊞ Win', isGlyph: ... } behavior as 'meta' (respecting isApple), keeping normalizeKey behavior unchanged.src/util/hotkeys.spec.ts (1)
20-158: Good additions; unresolved coverage gaps remain.The new
hotkeyFromKeyboardEventtests covering multiple modifiers, function keys, arrow keys, non-QWERTY code normalization, and Backspace/Delete are all correct and appreciated. Tests forisKeyboardEventFromTextInput, edge cases fornormalizeHotkeyString(empty/null input, modifier-only strings like"ctrl+shift"that should returnnull), and directtokenizeHotkeyStringtests for complex inputs (e.g."ctrl++a") are still absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.spec.ts` around lines 20 - 158, Add missing unit tests to close coverage gaps: create tests for isKeyboardEventFromTextInput verifying text input elements and non-text elements; add edge-case tests for normalizeHotkeyString to assert it returns null for empty/null inputs and for modifier-only strings like "ctrl+shift"; and add direct tokenizeHotkeyString tests for complex inputs such as "ctrl++a" (and similar cases) to ensure tokens are parsed correctly; place these in src/util/hotkeys.spec.ts alongside existing hotkeyFromKeyboardEvent tests and reference the functions isKeyboardEventFromTextInput, normalizeHotkeyString, and tokenizeHotkeyString when asserting expected outputs.src/components/menu/examples/menu-hotkeys.tsx (1)
63-71:'Organize'hotkey'o'navigates into the sub-menu rather than selecting — clarify in a comment.When
'o'is pressed, the menu'shandleSelectbranches onArray.isArray(items) && items.length > 0, navigating into the sub-menu instead of emitting aselectevent. This is intentional behaviour worth documenting inline so example readers aren't confused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-hotkeys.tsx` around lines 63 - 71, The 'Organize' item defines hotkey 'o' but pressing it opens the sub-menu because handleSelect checks Array.isArray(items) && items.length > 0 and navigates rather than emitting select; add a concise inline comment either on the 'Organize' menu entry or directly above the handleSelect implementation noting that hotkeys on items with a non-empty items array will open the submenu (not emit select), so readers understand this intentional behavior and won't be surprised by the navigation.src/components/list-item/menu-item-meta/menu-item-meta.tsx (2)
59-64:limel-hotkeyinstances in closed menus remain registered and active.
disabled={this.disabled}reflects only the menu item's disabled state, not the menu'sopenstate. When the menu is closed but its portal DOM stays attached, any enabledlimel-hotkeyrendered here stays in the global registry, intercepts matching keypresses, callsevent.preventDefault(), and setshandledFlag— stealing events from other global handlers even though the menu is not open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx` around lines 59 - 64, renderCommandText currently always renders a <limel-hotkey> when this.hotkey is set, which registers global key handlers even when the menu is closed; update renderCommandText to only render the <limel-hotkey> when the menu is actually open (e.g. check a boolean like this.open or this.menuOpen propagated into this component) in addition to this.hotkey and this.disabled so the hotkey is not registered while the menu is closed, or alternatively move hotkey rendering to the parent that controls open state; ensure you reference renderCommandText, limel-hotkey, this.hotkey and this.disabled when making the change.
22-23:reflect: trueadded tocommandTextis an unconfirmed behavioral change.Previously,
commandTextwas not reflected to the DOM attribute. Addingreflect: truenow serializes the value as an HTML attribute on every render. Please confirm this is intentional (e.g., a CSS[command-text]selector is needed) and not an accidental side-effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx` around lines 22 - 23, The change adds reflect: true to the `@Prop` decorator on commandText (public commandText?: string) which causes the property to be serialized to an HTML attribute on every render—confirm intent or revert: if unintentional, remove reflect: true from the `@Prop` on commandText to restore previous behavior; if intentional (e.g., relying on a CSS attribute selector like [command-text] or external consumers reading the attribute), leave reflect: true and add a short inline comment above the property explaining why reflection is required and add a test or note in the component docs verifying the attribute is relied upon.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:meta+enterandcmd+enternormalize to the same key — will always trigger aconsole.warn.
cmdis an alias formeta, so both entries register asmeta+enter. Every keypress will fire the duplicate-detection warning inhotkey.tsx, making the basic example noisy for anyone opening the console.Either remove one of the two lines, or move the alias-pairing into the
hotkey-duplicatesexample where the warning is the intended teaching point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example registers both "meta+enter" and its alias "cmd+enter" in src/components/hotkey/examples/hotkey-basic.tsx which normalize to the same hotkey and triggers the duplicate-detection in hotkey.tsx on every keypress; fix this by removing one of the duplicate limel-hotkey entries (either the "meta+enter" or "cmd+enter" line) or move the alias pair into the hotkey-duplicates example so the warning remains only where it’s intended, ensuring the basic example registers a single unique hotkey.src/components/menu/menu.e2e.tsx (1)
127-175: Good coverage; note unresolved stale portal concern.The new hotkey selection test looks correct. However,
document.querySelector('limel-menu-surface')at Line 153 (and similarly at Lines 198, 239) can return a surface from a prior test's un-torn-down portal, silently misdirecting the dispatched event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.e2e.tsx` around lines 127 - 175, The test can pick up a stale limel-menu-surface from a previous test; before rendering/opening the menu (or before querying for the surface) remove any existing portal surfaces by calling document.querySelectorAll('limel-menu-surface') and removing them, then proceed with render/open and query document.querySelector('limel-menu-surface') for the current test; update the test around the render/root/waitForChanges/trigger flow that interacts with <limel-menu> and limel-menu-surface so it cleans prior surfaces first to ensure the dispatched KeyboardEvent targets the surface created by this test only.src/components/menu/menu.tsx (3)
309-334: Clean hotkey dispatch with proper guard clauses.Previous review concerns (no-op
preventDefaulton the CustomEvent, inline type duplication) are both addressed. The early-return chain is readable and correctly funnels only valid, non-reserved matches intohandleSelect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.tsx` around lines 309 - 334, The handleHotkeyTrigger implementation looks correct and the previous problems are resolved; keep the early-return guard chain as-is in handleHotkeyTrigger, ensure you continue to use isReservedMenuHotkey(pressedHotkey) to skip reserved keys, findMenuItemByHotkey(pressedHotkey) to locate actionable items, and then call handleSelect(matchedItem) after stopping propagation and preventing default on event.detail.keyboardEvent; no further changes required.
36-37: Good: shared type imported, relative paths used.The import of
LimelHotkeyTriggerDetailaddresses the previously flagged inline type duplication. Both imports correctly use relative paths per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.tsx` around lines 36 - 37, The imports for shared symbols are correct: keep the relative imports of normalizeHotkeyString and LimelHotkeyTriggerDetail as they replace the previous inline duplicated type; no code changes needed — ensure the import lines referencing normalizeHotkeyString and LimelHotkeyTriggerDetail remain unchanged.
363-381: LGTM — clean iteration with caching.Previous redundant cast concern is resolved. The search correctly restricts to visible, enabled menu items and leverages the normalized hotkey cache.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.tsx` around lines 363 - 381, The implementation of findMenuItemByHotkey correctly iterates this.visibleItems, filters non-menu or disabled items, uses getNormalizedHotkey(rawHotkey) and returns the matching MenuItem; no code changes are required—mark this as approved and remove the duplicated review comment to avoid noise in the PR.src/util/hotkeys.ts (3)
89-100: LGTM — concise normalization with alias lookup.Previous redundant function-key guard has been removed. The function is straightforward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 89 - 100, The normalizeKey function is concise and correct; no code change required—keep the implementation of normalizeKey (which trims, lowercases, returns null on empty, and consults KEY_ALIASES) as-is and ensure KEY_ALIASES remains defined and populated elsewhere in the module.
20-35: Good:backspacealias removed, keys are now distinguishable.The previously flagged
backspace: 'delete'alias has been removed.BackspaceandDeleteare now distinct canonical keys, and the module JSDoc (line 15) confirms this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 20 - 35, KEY_ALIASES should no longer map "backspace" to "delete"—confirm that the KEY_ALIASES constant no longer contains a backspace alias and that the module JSDoc (the comment around hotkeys.ts) documents that Backspace and Delete are distinct; search for any references to KEY_ALIASES or code that expects a "backspace"→"delete" mapping and update those usages/tests to handle "backspace" and "delete" separately.
102-137: LGTM — canonical modifier ordering is clear and consistent.The
control→ctrlalias is now centralized inKEY_ALIASES, so the modifier check on line 115 correctly uses only canonical names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 102 - 137, No functional change is needed: the normalizeModifiersAndKey function correctly normalizes keys, ignores pure modifier presses, and uses canonical modifier names (meta, ctrl, alt, shift) as intended; simply close out the PR as approved and remove any duplicate review comment threads if present to avoid confusion (refer to normalizeModifiersAndKey and KEY_ALIASES for context).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.tsx`:
- Around line 336-361: The isReservedMenuHotkey function misparses hotkeys
containing the literal '+' key because it uses hotkey.split('+') and filters
empty parts; update isReservedMenuHotkey to derive the final key using the
existing tokenizer instead of naive split: import or reference
tokenizeHotkeyString (already present via normalizeHotkeyString import) and call
tokenizeHotkeyString(hotkey) to get tokens, then use the last token as key for
the reserved checks (preserving '+' when it's the actual key); keep the rest of
the reserved-key logic unchanged.
In `@src/util/device.ts`:
- Line 49: Remove the redundant explicit return type on isAppleDevice(): boolean
so it matches the other exported helpers (isIOSDevice, isAndroidDevice,
isMobileDevice) that rely on TypeScript return-type inference; locate the
isAppleDevice function and delete the ": boolean" annotation from its signature
so the function signature becomes just export function isAppleDevice() and keeps
behavior unchanged.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example registers both "meta+enter" and its alias
"cmd+enter" in src/components/hotkey/examples/hotkey-basic.tsx which normalize
to the same hotkey and triggers the duplicate-detection in hotkey.tsx on every
keypress; fix this by removing one of the duplicate limel-hotkey entries (either
the "meta+enter" or "cmd+enter" line) or move the alias pair into the
hotkey-duplicates example so the warning remains only where it’s intended,
ensuring the basic example registers a single unique hotkey.
In `@src/components/hotkey/hotkey.scss`:
- Around line 16-20: The box-shadow declaration's second shadow layer uses a
sub-pixel blur "0.375px", which is effectively zero in many browsers; update the
blur radius in the box-shadow property (the line starting with "0 0.625rem
0.375px -0.5rem rgb(...)") to use "0.375rem" instead of "0.375px" so the blur
scales with rem units and produces the intended visible blur.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 61-62: The `@Event` decorator on the hotkeyTrigger EventEmitter is
using redundant options ({ bubbles: true, composed: true }) because StencilJS
already defaults those to true; remove the explicit options and change the
decorator to `@Event`() for the hotkeyTrigger field (symbol: hotkeyTrigger) to
eliminate the no-op, or keep the explicit object only if you intentionally want
to preserve inline documentation clarity.
- Around line 133-182: formatDisplayToken fails to special-case the 'win' and
'windows' aliases so those tokens render as raw text instead of mapping to the
platform meta glyph; update the switch in formatDisplayToken to include case
'win': and case 'windows': (grouped with the existing 'meta' branch) so they
return the same { display: '⌘' / '⊞ Win', isGlyph: ... } behavior as 'meta'
(respecting isApple), keeping normalizeKey behavior unchanged.
In `@src/components/list-item/menu-item-meta/menu-item-meta.tsx`:
- Around line 59-64: renderCommandText currently always renders a <limel-hotkey>
when this.hotkey is set, which registers global key handlers even when the menu
is closed; update renderCommandText to only render the <limel-hotkey> when the
menu is actually open (e.g. check a boolean like this.open or this.menuOpen
propagated into this component) in addition to this.hotkey and this.disabled so
the hotkey is not registered while the menu is closed, or alternatively move
hotkey rendering to the parent that controls open state; ensure you reference
renderCommandText, limel-hotkey, this.hotkey and this.disabled when making the
change.
- Around line 22-23: The change adds reflect: true to the `@Prop` decorator on
commandText (public commandText?: string) which causes the property to be
serialized to an HTML attribute on every render—confirm intent or revert: if
unintentional, remove reflect: true from the `@Prop` on commandText to restore
previous behavior; if intentional (e.g., relying on a CSS attribute selector
like [command-text] or external consumers reading the attribute), leave reflect:
true and add a short inline comment above the property explaining why reflection
is required and add a test or note in the component docs verifying the attribute
is relied upon.
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The box-shadow in _kbd.scss uses a sub-pixel blur "0.375px"
which causes a unit mismatch (hotkey.scss forwards this file); change that blur
value to "0.375rem" so it matches the other shadow values and the rest of the
codebase. Locate the box-shadow declaration in
src/components/markdown/partial-styles/_kbd.scss and replace the "0.375px" blur
term with "0.375rem" (ensure consistency with hotkey.scss which forwards this
file).
In `@src/components/menu-list/menu-list-renderer.tsx`:
- Line 108: The `'hotkey' in item` check is redundant because `item` is already
narrowed to MenuItem after the earlier separator early-return; replace the
current declaration of hasHotkey (const hasHotkey = 'hotkey' in item && !!(item
as any).hotkey;) with a simple truthiness check like const hasHotkey = !!(item
as MenuItem).hotkey or const hasHotkey = Boolean(item.hotkey) to remove the
unnecessary `in` guard and keep proper typing.
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 63-71: The 'Organize' item defines hotkey 'o' but pressing it
opens the sub-menu because handleSelect checks Array.isArray(items) &&
items.length > 0 and navigates rather than emitting select; add a concise inline
comment either on the 'Organize' menu entry or directly above the handleSelect
implementation noting that hotkeys on items with a non-empty items array will
open the submenu (not emit select), so readers understand this intentional
behavior and won't be surprised by the navigation.
In `@src/components/menu/menu.e2e.tsx`:
- Around line 127-175: The test can pick up a stale limel-menu-surface from a
previous test; before rendering/opening the menu (or before querying for the
surface) remove any existing portal surfaces by calling
document.querySelectorAll('limel-menu-surface') and removing them, then proceed
with render/open and query document.querySelector('limel-menu-surface') for the
current test; update the test around the render/root/waitForChanges/trigger flow
that interacts with <limel-menu> and limel-menu-surface so it cleans prior
surfaces first to ensure the dispatched KeyboardEvent targets the surface
created by this test only.
In `@src/components/menu/menu.tsx`:
- Around line 309-334: The handleHotkeyTrigger implementation looks correct and
the previous problems are resolved; keep the early-return guard chain as-is in
handleHotkeyTrigger, ensure you continue to use
isReservedMenuHotkey(pressedHotkey) to skip reserved keys,
findMenuItemByHotkey(pressedHotkey) to locate actionable items, and then call
handleSelect(matchedItem) after stopping propagation and preventing default on
event.detail.keyboardEvent; no further changes required.
- Around line 36-37: The imports for shared symbols are correct: keep the
relative imports of normalizeHotkeyString and LimelHotkeyTriggerDetail as they
replace the previous inline duplicated type; no code changes needed — ensure the
import lines referencing normalizeHotkeyString and LimelHotkeyTriggerDetail
remain unchanged.
- Around line 363-381: The implementation of findMenuItemByHotkey correctly
iterates this.visibleItems, filters non-menu or disabled items, uses
getNormalizedHotkey(rawHotkey) and returns the matching MenuItem; no code
changes are required—mark this as approved and remove the duplicated review
comment to avoid noise in the PR.
In `@src/components/menu/menu.types.ts`:
- Around line 64-66: The JSDoc "Hotkey(s) that can be used..." is misleading
because the property it documents is typed as a single string; update the JSDoc
above the hotkey property in src/components/menu/menu.types.ts to use singular
wording (e.g., "Hotkey that can be used to select this item while the menu is
open.") or alternatively change the property's type to string | string[] if
multiple hotkeys were intended; adjust either the comment or the property type
for consistency with the property name (hotkey/hotkeys) used in the file.
In `@src/util/device.ts`:
- Around line 1-21: Remove the redundant explicit return-type annotations from
the unexported helper functions getUserAgent, getPlatform, and
getMaxTouchPoints: let TypeScript infer the return types by deleting the ":
string" and ": number" annotations on those function declarations while keeping
their bodies and runtime behavior unchanged (no changes to exported APIs).
In `@src/util/hotkeys.spec.ts`:
- Around line 20-158: Add missing unit tests to close coverage gaps: create
tests for isKeyboardEventFromTextInput verifying text input elements and
non-text elements; add edge-case tests for normalizeHotkeyString to assert it
returns null for empty/null inputs and for modifier-only strings like
"ctrl+shift"; and add direct tokenizeHotkeyString tests for complex inputs such
as "ctrl++a" (and similar cases) to ensure tokens are parsed correctly; place
these in src/util/hotkeys.spec.ts alongside existing hotkeyFromKeyboardEvent
tests and reference the functions isKeyboardEventFromTextInput,
normalizeHotkeyString, and tokenizeHotkeyString when asserting expected outputs.
In `@src/util/hotkeys.ts`:
- Around line 89-100: The normalizeKey function is concise and correct; no code
change required—keep the implementation of normalizeKey (which trims,
lowercases, returns null on empty, and consults KEY_ALIASES) as-is and ensure
KEY_ALIASES remains defined and populated elsewhere in the module.
- Around line 20-35: KEY_ALIASES should no longer map "backspace" to
"delete"—confirm that the KEY_ALIASES constant no longer contains a backspace
alias and that the module JSDoc (the comment around hotkeys.ts) documents that
Backspace and Delete are distinct; search for any references to KEY_ALIASES or
code that expects a "backspace"→"delete" mapping and update those usages/tests
to handle "backspace" and "delete" separately.
- Around line 102-137: No functional change is needed: the
normalizeModifiersAndKey function correctly normalizes keys, ignores pure
modifier presses, and uses canonical modifier names (meta, ctrl, alt, shift) as
intended; simply close out the PR as approved and remove any duplicate review
comment threads if present to avoid confusion (refer to normalizeModifiersAndKey
and KEY_ALIASES for context).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (18)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-hotkeys
branch
from
7cab23f to
1f0986d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/components/menu-list/menu-list-renderer.tsx (1)
108-108: 🛠️ Refactor suggestion | 🟠 MajorRedundant
'hotkey' in itemguard — still unaddressed.After the
'separator' in itemearly-return at line 89, TypeScript narrowsitemtoMenuItem. Accessing an undefined optional property already returnsundefined, making the'hotkey' in itemprefix unnecessary.♻️ Proposed simplification
- const hasHotkey = 'hotkey' in item && !!(item as any).hotkey; + const hasHotkey = !!(item as MenuItem).hotkey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu-list/menu-list-renderer.tsx` at line 108, The check "'hotkey' in item" is redundant after the earlier "'separator' in item" early-return which narrows item to MenuItem; replace the hasHotkey expression so it simply coerces the optional property (e.g., use Boolean(item.hotkey) or !!item.hotkey) and remove the (item as any) cast so hasHotkey is defined as a direct, typed check against item.hotkey in the menu-list-renderer.tsx code (referencing the const hasHotkey and the earlier "'separator' in item" branch).src/components/hotkey/hotkey.scss (1)
16-20:⚠️ Potential issue | 🟡 Minor
0.375pxblur radius — still unaddressed.Same unit mismatch as in
_kbd.scssline 28: the blur value usespxwhile all surrounding values userem. Sincehotkey.scssdefines its own overridekbdblock (separate from the@forwarded styles), this must be fixed here independently.🔧 Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.scss` around lines 16 - 20, The box-shadow in the kbd override inside hotkey.scss uses a mismatched unit "0.375px" among rem values; update the blur radius to the rem equivalent (e.g., replace 0.375px with the proper rem value consistent with surrounding tokens) in the box-shadow declaration so all components use rem units, ensuring the kbd override mirrors the unit usage from _kbd.scss; edit the box-shadow line in the kbd/block within hotkey.scss to use the calculated rem instead of 0.375px.src/components/menu/menu.e2e.tsx (1)
153-154:⚠️ Potential issue | 🟡 MinorStale
document.querySelector('limel-menu-surface')— portal cleanup risk still unresolved.Since each
render()generates a newportalIdviacreateRandomString(), multiplelimel-menu-surfaceelements can coexist indocument.bodyacross tests if portal teardown hasn't completed.document.querySelectorreturns the first match, which may belong to a prior test — silently misdirecting event dispatches and making assertions unreliable.The same risk applies to Lines 198-199 and Lines 239-240.
Consider scoping the surface lookup to the specific portal container, e.g.:
const portalContainer = document.getElementById(portalId); const surface = portalContainer?.querySelector('limel-menu-surface');or adding explicit cleanup between tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.e2e.tsx` around lines 153 - 154, The test is querying document.querySelector('limel-menu-surface') which can return a surface from a previous render because render() generates a new portalId via createRandomString(); change the lookup to scope to the test's portal container using the portalId (e.g. get the portal container by document.getElementById(portalId) and then portalContainer.querySelector('limel-menu-surface')) or ensure explicit portal cleanup between tests; update all occurrences (the checks around surface lookup and event dispatchs) that currently use document.querySelector('limel-menu-surface') to use the scoped portalContainer lookup referencing portalId and render().src/components/markdown/partial-styles/_kbd.scss (1)
26-29:⚠️ Potential issue | 🟡 Minor
0.375pxblur radius — still unaddressed.The second shadow layer uses
pxfor blur while all other values userem. A sub-pixel blur is effectively a no-op in most browsers.🔧 Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The second box-shadow layer uses a sub-pixel blur value "0.375px" which is inconsistent with the rem-based values and effectively ignored by browsers; update the blur to a rem value (use the rem equivalent 0.0234375rem or pick a sensible rem like 0.025rem/0.03rem) in the box-shadow definition in _kbd.scss (the second shadow layer inside the box-shadow declaration) so all offsets/blur values consistently use rem units.src/components/menu/menu.types.ts (1)
64-65:⚠️ Potential issue | 🟡 Minor
Hotkey(s)wording still misleads consumers — property accepts a singlestring.The parenthetical
(s)impliesstring | string[].📝 Proposed fix
- * Hotkey(s) that can be used to select this item while the menu is open. + * A hotkey that can be used to select this item while the menu is open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.types.ts` around lines 64 - 65, The JSDoc for the menu item hotkey currently reads "Hotkey(s)" which incorrectly implies an array; update the comment above the hotkey property in src/components/menu/menu.types.ts (the JSDoc immediately preceding the hotkey/hotkeys property) to read "Hotkey that can be used to select this item while the menu is open." and ensure the property type remains a single string (not string[]), so consumers aren't misled.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:⚠️ Potential issue | 🟡 MinorRemove duplicated normalized hotkey from the basic example.
Line 51 and Line 52 normalize to the same hotkey (meta+enter), which triggers duplicate warnings and "first handler wins" behavior in a file meant to demonstrate basic usage.Suggested fix
<limel-hotkey value="meta+enter" /> - <limel-hotkey value="cmd+enter" /> <limel-hotkey value="ctrl+shift+c" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example registers two limel-hotkey elements whose values normalize to the same shortcut (meta+enter), causing duplicate warnings and first-handler-wins behavior; fix by removing or changing one of the <limel-hotkey> entries in hotkey-basic.tsx (the elements with value="meta+enter" and value="cmd+enter") so the example only registers unique normalized hotkeys (e.g., remove the cmd+enter element or replace it with a different shortcut like ctrl+enter).src/components/hotkey/hotkey.tsx (1)
133-143:⚠️ Potential issue | 🟡 MinorAdd
win/windowsalias handling in token display.
Line 133-143 mapsmeta/cmd/command, butwinandwindowscurrently fall through to raw text despite being aliases ofmetain hotkey normalization/docs.Suggested fix
- case 'meta': { + case 'meta': + case 'win': + case 'windows': { return isApple ? { display: '⌘', isGlyph: true } : { display: '⊞ Win', isGlyph: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 143, The switch over token lowercase values (switch (lower)) currently handles 'meta', 'cmd', and 'command' but misses 'win' and 'windows', causing those aliases to render as raw text; add case 'win' and case 'windows' grouped with 'meta' (or map them to the same return logic) so they return { display: '⌘', isGlyph: true } on Apple and { display: '⊞ Win', isGlyph: false } otherwise—update the switch in the function that renders key tokens (the switch (lower) block) to include these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.tsx`:
- Around line 339-363: The isReservedMenuHotkey function currently treats 'tab'
and arrow keys as reserved regardless of modifiers, preventing valid combos like
'alt+arrowdown'; update isReservedMenuHotkey (and its use of
tokenizeHotkeyString/tokens and hasModifiers) so that arrows and 'tab' are only
reserved when there are no modifiers (i.e., when tokens.length === 1), and keep
'enter', 'space', and 'escape' reserved only when unmodified as already
implemented; ensure the final return logic checks modifiers before classifying
arrows/tab as reserved.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example registers two limel-hotkey elements whose
values normalize to the same shortcut (meta+enter), causing duplicate warnings
and first-handler-wins behavior; fix by removing or changing one of the
<limel-hotkey> entries in hotkey-basic.tsx (the elements with value="meta+enter"
and value="cmd+enter") so the example only registers unique normalized hotkeys
(e.g., remove the cmd+enter element or replace it with a different shortcut like
ctrl+enter).
In `@src/components/hotkey/hotkey.scss`:
- Around line 16-20: The box-shadow in the kbd override inside hotkey.scss uses
a mismatched unit "0.375px" among rem values; update the blur radius to the rem
equivalent (e.g., replace 0.375px with the proper rem value consistent with
surrounding tokens) in the box-shadow declaration so all components use rem
units, ensuring the kbd override mirrors the unit usage from _kbd.scss; edit the
box-shadow line in the kbd/block within hotkey.scss to use the calculated rem
instead of 0.375px.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 133-143: The switch over token lowercase values (switch (lower))
currently handles 'meta', 'cmd', and 'command' but misses 'win' and 'windows',
causing those aliases to render as raw text; add case 'win' and case 'windows'
grouped with 'meta' (or map them to the same return logic) so they return {
display: '⌘', isGlyph: true } on Apple and { display: '⊞ Win', isGlyph: false }
otherwise—update the switch in the function that renders key tokens (the switch
(lower) block) to include these cases.
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The second box-shadow layer uses a sub-pixel blur value
"0.375px" which is inconsistent with the rem-based values and effectively
ignored by browsers; update the blur to a rem value (use the rem equivalent
0.0234375rem or pick a sensible rem like 0.025rem/0.03rem) in the box-shadow
definition in _kbd.scss (the second shadow layer inside the box-shadow
declaration) so all offsets/blur values consistently use rem units.
In `@src/components/menu-list/menu-list-renderer.tsx`:
- Line 108: The check "'hotkey' in item" is redundant after the earlier
"'separator' in item" early-return which narrows item to MenuItem; replace the
hasHotkey expression so it simply coerces the optional property (e.g., use
Boolean(item.hotkey) or !!item.hotkey) and remove the (item as any) cast so
hasHotkey is defined as a direct, typed check against item.hotkey in the
menu-list-renderer.tsx code (referencing the const hasHotkey and the earlier
"'separator' in item" branch).
In `@src/components/menu/menu.e2e.tsx`:
- Around line 153-154: The test is querying
document.querySelector('limel-menu-surface') which can return a surface from a
previous render because render() generates a new portalId via
createRandomString(); change the lookup to scope to the test's portal container
using the portalId (e.g. get the portal container by
document.getElementById(portalId) and then
portalContainer.querySelector('limel-menu-surface')) or ensure explicit portal
cleanup between tests; update all occurrences (the checks around surface lookup
and event dispatchs) that currently use
document.querySelector('limel-menu-surface') to use the scoped portalContainer
lookup referencing portalId and render().
In `@src/components/menu/menu.types.ts`:
- Around line 64-65: The JSDoc for the menu item hotkey currently reads
"Hotkey(s)" which incorrectly implies an array; update the comment above the
hotkey property in src/components/menu/menu.types.ts (the JSDoc immediately
preceding the hotkey/hotkeys property) to read "Hotkey that can be used to
select this item while the menu is open." and ensure the property type remains a
single string (not string[]), so consumers aren't misled.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (18)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-hotkeys
branch
from
1f0986d to
baed4c0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/components/markdown/partial-styles/_kbd.scss (1)
26-29:⚠️ Potential issue | 🟡 MinorUnit mismatch:
0.375pxshould likely be0.375rem.Line 28 uses
0.375pxfor the blur radius while all other shadow values useremunits. This appears to be a typo that was flagged previously but not yet addressed.🔧 Proposed fix
box-shadow: var(--button-shadow-normal), - 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02), 0 0.025rem 0.5rem 0 rgb(var(--contrast-100)) inset;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The box-shadow declaration in _kbd.scss uses a mismatched unit "0.375px" (in the box-shadow list) which should be "0.375rem"; update the blur radius in the box-shadow tuple inside the box-shadow property (the line with "0 0.625rem 0.375px -0.5rem") to "0 0.625rem 0.375rem -0.5rem" so all shadow measurements consistently use rem units.src/components/menu/menu.types.ts (1)
64-77:⚠️ Potential issue | 🟡 MinorAlign
hotkeyJSDoc with its single-string type.Line 65 says “Hotkey(s)”, but Line 77 defines
hotkey?: string(single value). Please switch to singular wording to avoid implying array support.Proposed fix
- * Hotkey(s) that can be used to select this item while the menu is open. + * A hotkey that can be used to select this item while the menu is open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.types.ts` around lines 64 - 77, Update the JSDoc for the hotkey property to match its single-string type: change the header and any plural wording (e.g., "Hotkey(s)" and references implying arrays) to singular wording like "Hotkey" and clarify it accepts a single string (examples remain fine); locate the documentation above the hotkey?: string declaration in menu.types.ts and adjust the text to avoid implying array support.src/components/menu-list/menu-list-renderer.tsx (1)
108-113: 🧹 Nitpick | 🔵 TrivialRemove redundant hotkey guard and
anycast.After the separator early-return,
itemis already aMenuItem, so Line 108 can be simplified and kept type-safe.Proposed simplification
- const hasHotkey = 'hotkey' in item && !!(item as any).hotkey; + const hasHotkey = !!item.hotkey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu-list/menu-list-renderer.tsx` around lines 108 - 113, Simplify and make the hotkey check type-safe: remove the redundant "'hotkey' in item" guard and the "(item as any)" cast used when computing hasHotkey, and instead directly test the MenuItem's hotkey property for truthiness; then update hasMeta to use that new hasHotkey variable (leave the other checks—hasSubMenu, item.badge, commandText—unchanged). This keeps types intact after the separator early-return where item is already a MenuItem and removes the unnecessary any cast from hasHotkey and hasMeta.src/components/menu/menu.e2e.tsx (1)
153-154:⚠️ Potential issue | 🟡 MinorScope menu-surface lookup to the current portal instance.
Using
document.querySelector('limel-menu-surface')(Line 153, Line 198, Line 239, Line 274) can pick a stale surface from another test instance.Proposed fix pattern
- const surface = document.querySelector('limel-menu-surface'); + const trigger = root.querySelector('button[slot="trigger"]') as HTMLElement; + const portalId = trigger.getAttribute('aria-controls'); + const surface = document + .getElementById(portalId!) + ?.querySelector('limel-menu-surface');Also applies to: 198-199, 239-240, 274-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.e2e.tsx` around lines 153 - 154, Tests currently use document.querySelector('limel-menu-surface') which can return a stale surface from another test; update each occurrence to query the surface from the current portal instance instead (e.g., replace document.querySelector('limel-menu-surface') with portal.querySelector('limel-menu-surface') or the local portal variable used in the test setup) so the lookup is scoped to the test's portal element (adjust any variable name to match the existing portal variable in the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The box-shadow declaration in _kbd.scss uses a mismatched
unit "0.375px" (in the box-shadow list) which should be "0.375rem"; update the
blur radius in the box-shadow tuple inside the box-shadow property (the line
with "0 0.625rem 0.375px -0.5rem") to "0 0.625rem 0.375rem -0.5rem" so all
shadow measurements consistently use rem units.
In `@src/components/menu-list/menu-list-renderer.tsx`:
- Around line 108-113: Simplify and make the hotkey check type-safe: remove the
redundant "'hotkey' in item" guard and the "(item as any)" cast used when
computing hasHotkey, and instead directly test the MenuItem's hotkey property
for truthiness; then update hasMeta to use that new hasHotkey variable (leave
the other checks—hasSubMenu, item.badge, commandText—unchanged). This keeps
types intact after the separator early-return where item is already a MenuItem
and removes the unnecessary any cast from hasHotkey and hasMeta.
In `@src/components/menu/menu.e2e.tsx`:
- Around line 153-154: Tests currently use
document.querySelector('limel-menu-surface') which can return a stale surface
from another test; update each occurrence to query the surface from the current
portal instance instead (e.g., replace
document.querySelector('limel-menu-surface') with
portal.querySelector('limel-menu-surface') or the local portal variable used in
the test setup) so the lookup is scoped to the test's portal element (adjust any
variable name to match the existing portal variable in the test).
In `@src/components/menu/menu.types.ts`:
- Around line 64-77: Update the JSDoc for the hotkey property to match its
single-string type: change the header and any plural wording (e.g., "Hotkey(s)"
and references implying arrays) to singular wording like "Hotkey" and clarify it
accepts a single string (examples remain fine); locate the documentation above
the hotkey?: string declaration in menu.types.ts and adjust the text to avoid
implying array support.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (7)
src/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Line 7: The example imports isAppleDevice from an internal util which couples
the example to implementation details; make the example self-contained by
removing the import and inlining the device detection logic directly inside the
example (e.g., replace the import of isAppleDevice and any calls to
isAppleDevice with a small local helper or constant within the
menu-searchable-hotkeys.tsx example), or alternatively expose isAppleDevice via
the public API and update the import to use that public export; locate usages of
isAppleDevice in the example and update them to use the new local helper or the
public export accordingly.
- Around line 69-91: The render method currently returns an array of JSX
elements; update the component's render (in the class containing render, which
uses limel-menu and limel-example-value and references handleSearch,
handleSelect, and lastSelectedItem) to wrap these multiple elements inside a
single <Host> element instead of returning an array literal so the JSX follows
StencilJS guidelines; locate the render method and replace the array return with
a single Host wrapper containing the limel-menu (with
searcher={this.handleSearch} and onSelect={this.handleSelect}) and the
limel-example-value (using this.lastSelectedItem).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsx
Kiarokh
force-pushed
the
menu-hotkeys
branch
from
f0fdc5a to
daf8388
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/components/menu/examples/menu-searchable-hotkeys.tsx (1)
32-36:⚠️ Potential issue | 🟡 MinorFix typo in documentation.
Line 33 has a grammatical error: "they focus" should be "the focus".
📝 Proposed fix
-* Consider that when the menu is opened, they focus is on the search input, +* Consider that when the menu is opened, the focus is on the search input,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` around lines 32 - 36, In the comment block inside src/components/menu/examples/menu-searchable-hotkeys.tsx (above the example in the MenuSearchableHotkeys component), fix the typo "they focus" to "the focus" so the sentence reads correctly; locate the multi-line comment describing focus behavior when the menu opens and replace the incorrect word to correct the documentation grammar.src/components/markdown/partial-styles/_kbd.scss (1)
28-28:⚠️ Potential issue | 🟡 MinorLikely unit typo in box-shadow blur radius.
Line 28 uses
0.375px, which is inconsistent with the surrounding rem-based shadow values and likely unintended.Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` at line 28, In _kbd.scss update the box-shadow value that currently uses a pixel unit (0.375px) to use the consistent rem unit (0.375rem) so it aligns with the surrounding rem-based shadow values; locate the box-shadow declaration in the kbd styling (the line containing "0 0.625rem 0.375px -0.5rem") and change the blur radius unit from px to rem.src/components/menu/menu.e2e.tsx (1)
153-155:⚠️ Potential issue | 🟡 MinorUse portal-scoped surface queries to avoid stale element selection.
At Line 154 and the other listed occurrences,
document.querySelector('limel-menu-surface')can resolve a stale surface from another portal instance, making these tests flaky. Scope the lookup to the current menu’s portal container (via itsportalId) before dispatching events.Suggested direction
-const surface = document.querySelector('limel-menu-surface'); +const portalId = (root as any).portalId; +const portal = document.getElementById(portalId); +const surface = portal?.querySelector('limel-menu-surface'); expect(surface).toBeTruthy();Also applies to: 199-200, 240-241, 275-276, 313-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.e2e.tsx` around lines 153 - 155, The tests currently query document.querySelector('limel-menu-surface') which can return a stale surface from another portal; instead, use the menu's portal container (via the menu instance's portalId) to scope the lookup before interacting—e.g. get the portal element by its id (constructed from portalId) and run querySelector('limel-menu-surface') on that container so each test targets its own surface; update all occurrences that use document.querySelector('limel-menu-surface') (the lookups around the surface variable and subsequent event dispatches) to perform the scoped query against the portal container retrieved from the menu's portalId.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:⚠️ Potential issue | 🟡 MinorAvoid normalized duplicate hotkeys in the basic example.
At Line 51–52,
meta+enterandcmd+enternormalize to the same hotkey, so this basic example will emit duplicate-hotkey warnings and blur the intended “basic usage” path.Proposed fix
<limel-hotkey value="meta+enter" /> - <limel-hotkey value="cmd+enter" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example includes duplicate normalized hotkeys: the two limel-hotkey elements with value="meta+enter" and value="cmd+enter" resolve to the same binding and cause duplicate-hotkey warnings; fix by removing or changing one of them (e.g., keep value="meta+enter" and delete the limel-hotkey with value="cmd+enter", or replace it with a different combo like value="ctrl+enter") so the basic example demonstrates a single unique hotkey.src/components/hotkey/hotkey.tsx (1)
133-143:⚠️ Potential issue | 🟡 MinorAlign alias rendering with
metasemantics across platforms.On Line 140–Line 143,
cmd/commandalways render⌘, whilemetais platform-aware, andwin/windowsfall through to raw text. This creates misleading hints on non-Apple platforms for aliases that normalize tometa.🔧 Proposed fix
switch (lower) { case 'meta': { return isApple ? { display: '⌘', isGlyph: true } : { display: '⊞ Win', isGlyph: false }; } + case 'win': + case 'windows': case 'cmd': case 'command': { - return { display: '⌘', isGlyph: true }; + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 143, The switch handling key aliases treats 'cmd'/'command' differently from 'meta' and causes incorrect glyphs on non-Apple platforms; update the switch in the function that maps keys (the block switching on variable lower) so that 'cmd' and 'command' alias to the same platform-aware logic as 'meta' (i.e., return { display: '⌘', isGlyph: true } when isApple is true, otherwise return the Windows text like { display: '⊞ Win', isGlyph: false }), and ensure 'win'/'windows' continue to produce the Windows text variant rather than always falling through to raw text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/device.ts`:
- Around line 26-29: isIOSDevice incorrectly returns false for modern iPadOS
Safari because the UA can be "Macintosh"; update isIOSDevice (and callers like
isMobileDevice) to detect iPadOS by checking for either the existing
/iPad|iPhone|iPod/i pattern OR a Macintosh UA combined with touch capability
(e.g., navigator.maxTouchPoints > 1), while preserving the existing !(globalThis
as any).MSStream exclusion and using getUserAgent() to obtain the UA.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example includes duplicate normalized hotkeys: the two
limel-hotkey elements with value="meta+enter" and value="cmd+enter" resolve to
the same binding and cause duplicate-hotkey warnings; fix by removing or
changing one of them (e.g., keep value="meta+enter" and delete the limel-hotkey
with value="cmd+enter", or replace it with a different combo like
value="ctrl+enter") so the basic example demonstrates a single unique hotkey.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 133-143: The switch handling key aliases treats 'cmd'/'command'
differently from 'meta' and causes incorrect glyphs on non-Apple platforms;
update the switch in the function that maps keys (the block switching on
variable lower) so that 'cmd' and 'command' alias to the same platform-aware
logic as 'meta' (i.e., return { display: '⌘', isGlyph: true } when isApple is
true, otherwise return the Windows text like { display: '⊞ Win', isGlyph: false
}), and ensure 'win'/'windows' continue to produce the Windows text variant
rather than always falling through to raw text.
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Line 28: In _kbd.scss update the box-shadow value that currently uses a pixel
unit (0.375px) to use the consistent rem unit (0.375rem) so it aligns with the
surrounding rem-based shadow values; locate the box-shadow declaration in the
kbd styling (the line containing "0 0.625rem 0.375px -0.5rem") and change the
blur radius unit from px to rem.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Around line 32-36: In the comment block inside
src/components/menu/examples/menu-searchable-hotkeys.tsx (above the example in
the MenuSearchableHotkeys component), fix the typo "they focus" to "the focus"
so the sentence reads correctly; locate the multi-line comment describing focus
behavior when the menu opens and replace the incorrect word to correct the
documentation grammar.
In `@src/components/menu/menu.e2e.tsx`:
- Around line 153-155: The tests currently query
document.querySelector('limel-menu-surface') which can return a stale surface
from another portal; instead, use the menu's portal container (via the menu
instance's portalId) to scope the lookup before interacting—e.g. get the portal
element by its id (constructed from portalId) and run
querySelector('limel-menu-surface') on that container so each test targets its
own surface; update all occurrences that use
document.querySelector('limel-menu-surface') (the lookups around the surface
variable and subsequent event dispatches) to perform the scoped query against
the portal container retrieved from the menu's portalId.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (19)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
| export function isIOSDevice() { | ||
| return /iPad|iPhone|iPod/i.test(userAgent) && !(window as any).MSStream; | ||
| const userAgent = getUserAgent(); | ||
| return /iPad|iPhone|iPod/i.test(userAgent) && !(globalThis as any).MSStream; | ||
| } |
There was a problem hiding this comment.
iPadOS 13+ is misclassified as non-iOS, breaking mobile detection
Line 28 only matches iPad|iPhone|iPod. On newer iPadOS Safari (Mac-like UA), isIOSDevice() returns false, which makes isMobileDevice() on Line 43 return false too.
💡 Suggested fix
export function isIOSDevice() {
const userAgent = getUserAgent();
- return /iPad|iPhone|iPod/i.test(userAgent) && !(globalThis as any).MSStream;
+ const platform = getPlatform();
+ const isIPadIPhoneIPod = /iPad|iPhone|iPod/i.test(userAgent);
+ const isIPadOS13Plus = /Mac/i.test(platform) && getMaxTouchPoints() > 1;
+
+ return (
+ (isIPadIPhoneIPod || isIPadOS13Plus) &&
+ !(globalThis as any).MSStream
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/device.ts` around lines 26 - 29, isIOSDevice incorrectly returns
false for modern iPadOS Safari because the UA can be "Macintosh"; update
isIOSDevice (and callers like isMobileDevice) to detect iPadOS by checking for
either the existing /iPad|iPhone|iPod/i pattern OR a Macintosh UA combined with
touch capability (e.g., navigator.maxTouchPoints > 1), while preserving the
existing !(globalThis as any).MSStream exclusion and using getUserAgent() to
obtain the UA.
There was a problem hiding this comment.
@adrianschmidt-bot there are some comments in this PR about improvements that can be done in src/util/device.ts Even though there are some changes in the current PR in the same file, this file is originally too old and its internal logics can be certainly better. Once the current PR is merged, make a new PR and improve this helper file. I am not sure where this file is used in this library. We need to make sure we don't break anything. Therefore, I want this to be done and tested separately (but including the content from this current PR of course).
There was a problem hiding this comment.
Got it! I'll create a separate PR to improve src/util/device.ts once this PR is merged. Will make sure to audit all usages across the library and test thoroughly. Added it to my task list. 👍
There was a problem hiding this comment.
@Kiarokh Brilliant use of the bot! I noticed that it had checked if this PR was still open as part of its recurring todo-list check, and was curious to see what that was about. And finding this thread was the origin made me smile!
I've had similar cases, where I wanted something fixed in a follow-up, but never thought about actually asking the bot to keep track of when the PR would be closed, so I just had it create an issue that I had to come back to myself later on. But this is so much more convenient!
(Also, this was good timing, because I set up the automatic checking of the todo-list just a few days ago, so before then, this wouldn't have worked 😄)
There was a problem hiding this comment.
great @adrianschmidt I had no idea whether this would work or not. But I thought it was worth testing. 😉
adrianschmidt
force-pushed
the
menu-hotkeys
branch
from
daf8388 to
263e1c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
src/util/device.ts (1)
26-29:⚠️ Potential issue | 🟠 MajoriPadOS 13+ is still missed in iOS detection (breaks mobile classification).
Line 28 only checks
iPad|iPhone|iPod, so iPadOS Safari with Mac-like UA still returns false inisIOSDevice(), which also makesisMobileDevice()return false for those devices.💡 Suggested fix
export function isIOSDevice() { const userAgent = getUserAgent(); - return /iPad|iPhone|iPod/i.test(userAgent) && !(globalThis as any).MSStream; + const platform = getPlatform(); + const isIPadIPhoneIPod = /iPad|iPhone|iPod/i.test(userAgent); + const isIPadOS13Plus = /Mac/i.test(platform) && getMaxTouchPoints() > 1; + + return ( + (isIPadIPhoneIPod || isIPadOS13Plus) && + !(globalThis as any).MSStream + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/device.ts` around lines 26 - 29, isIOSDevice() currently only tests /iPad|iPhone|iPod/i so modern iPadOS 13+ (which reports a Mac-like UA) is missed; update isIOSDevice() to also detect iPadOS by checking getUserAgent() plus platform/touch capabilities (e.g., detect navigator.platform === 'MacIntel' and navigator.maxTouchPoints > 1 or similar via globalThis) so that isIOSDevice() (and consequently isMobileDevice()) returns true for iPadOS Safari; modify the function that calls getUserAgent() (isIOSDevice) to include this additional platform+touch check while preserving the existing regex path.src/components/markdown/partial-styles/_kbd.scss (1)
26-29:⚠️ Potential issue | 🟡 MinorUnit mismatch:
0.375pxlikely intended to be0.375rem.The box-shadow blur radius at line 28 uses
pxwhile all other shadow values userem. A sub-pixel blur (0.375px) effectively rounds to 0-1px in most browsers, which is probably not the intended effect.🔧 Proposed fix
box-shadow: var(--button-shadow-normal), - 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02), 0 0.025rem 0.5rem 0 rgb(var(--contrast-100)) inset;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The box-shadow declaration uses a mismatched unit "0.375px" for the blur radius; update the box-shadow value in the _kbd.scss rule (the box-shadow line containing "0 0.625rem 0.375px -0.5rem") to use rem instead of px (i.e., change 0.375px → 0.375rem) so all shadow offsets/blur/spread use consistent rem units.src/components/hotkey/hotkey.tsx (2)
133-182:⚠️ Potential issue | 🟡 MinorAdd missing
winandwindowscases toformatDisplayToken.The documentation states
winandwindowsare aliases formeta, andnormalizeKeyinhotkeys.tsmaps them accordingly. However,formatDisplayTokenlacks these cases, causing them to render as raw text instead of the proper glyph.🔧 Proposed fix
case 'meta': { return isApple ? { display: '⌘', isGlyph: true } : { display: '⊞ Win', isGlyph: false }; } + case 'win': + case 'windows': { + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; + } + case 'cmd': case 'command': {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 182, formatDisplayToken is missing handling for the 'win' and 'windows' aliases so those keys fall through to raw text; add case 'win' and case 'windows' alongside the existing 'meta' handling in function formatDisplayToken so they return the same object (use the isApple conditional to return { display: '⌘', isGlyph: true } for Apple and { display: '⊞ Win', isGlyph: false } otherwise), keeping the same structure used for the 'meta' branch.
140-143:⚠️ Potential issue | 🟡 MinorMissing platform awareness for
cmd/commanddisplay tokens.These cases unconditionally return
⌘, but on Windows/Linux acmd+shotkey would display⌘ + Swhile actually triggering on the Windows/Meta key. This creates a visual mismatch. Consider aligning with themetacase's platform-aware rendering.🔧 Proposed fix
case 'cmd': case 'command': { - return { display: '⌘', isGlyph: true }; + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 140 - 143, The 'cmd' / 'command' switch branch is returning an unconditional '⌘' glyph which causes incorrect visuals on non-mac platforms; update the case for 'cmd' and 'command' (in the hotkey token/rendering switch inside the Hotkey component / hotkey rendering function) to be platform-aware and mirror the logic used by the 'meta' case (use the same isMac/platform detection and return either { display: '⌘', isGlyph: true } for mac or the platform-appropriate string (e.g., 'Win'/'Ctrl' or the same display as the 'meta' branch) for non-mac). Ensure the returned shape still matches { display, isGlyph } so downstream rendering is unchanged.src/components/hotkey/hotkey.scss (1)
16-19:⚠️ Potential issue | 🟡 MinorSame
0.375pxunit inconsistency as in_kbd.scss.This local
box-shadowoverride has the same0.375pxblur value. Since this file@forwards_kbd.scss, consider fixing both locations for consistency, or rely solely on the fix in the source partial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.scss` around lines 16 - 19, Replace the inconsistent "0.375px" blur value in the box-shadow declaration with the correct unit to match surrounding values (e.g., "0.375rem") and make the same change in the corresponding declaration in _kbd.scss (or remove the local override in hotkey.scss and rely on the forwarded _kbd.scss partial) so both box-shadow entries are consistent; locate the box-shadow line in hotkey.scss and the matching box-shadow in _kbd.scss to apply the fix.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:⚠️ Potential issue | 🟡 MinorAvoid duplicate normalized hotkeys in the basic example.
At Line 51–52,
meta+enterandcmd+enternormalize to the same hotkey, so this example can trigger duplicate-hotkey warnings.🛠️ Proposed fix
<limel-hotkey value="meta+enter" /> - <limel-hotkey value="cmd+enter" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example contains duplicate normalized hotkeys: the <limel-hotkey value="meta+enter" /> and <limel-hotkey value="cmd+enter" /> entries normalize to the same hotkey and cause duplicate-hotkey warnings; fix it by removing or replacing one of these tags in the hotkey-basic.tsx example (e.g., remove the <limel-hotkey value="cmd+enter" /> line or replace it with a different combination such as <limel-hotkey value="ctrl+enter" />) so each <limel-hotkey> in the example has a unique normalized value.src/components/menu/examples/menu-searchable-hotkeys.tsx (2)
7-7: 🧹 Nitpick | 🔵 TrivialMake the example self-contained instead of importing internal device utilities.
At Line 7, importing
isAppleDevicefrom../../../util/devicecouples this example to internals.♻️ Proposed direction
-import { isAppleDevice } from '../../../util/device'; +const isAppleDevice = () => + /Mac|iPhone|iPad|iPod/i.test(navigator.userAgent);Based on learnings: "For lime-elements, example files should be self-contained and avoid importing internal implementation details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` at line 7, The example imports isAppleDevice from internal util/device; remove that external import and make the example self-contained by implementing a small local isAppleDevice helper inside menu-searchable-hotkeys.tsx (or the same example module) and use it where referenced (replace usages of isAppleDevice with the local function); the helper can check navigator.platform or navigator.userAgent for Apple identifiers (e.g., /Mac|iPhone|iPad|iPod|Macintosh/) and return a boolean so the example no longer depends on internal utilities.
33-34:⚠️ Potential issue | 🟡 MinorFix small documentation typo in the example description.
At Line 33, “they focus” should be “the focus”.
📝 Proposed fix
- * Consider that when the menu is opened, they focus is on the search input, + * Consider that when the menu is opened, the focus is on the search input,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` around lines 33 - 34, Fix the typo in the example description comment inside the MenuSearchableHotkeys example: change the phrase "they focus is on the search input" to "the focus is on the search input" in the comment above the component (look for the menu-searchable-hotkeys.tsx example / MenuSearchableHotkeys) so the sentence reads correctly.src/components/menu/menu.e2e.tsx (1)
153-155:⚠️ Potential issue | 🟠 MajorScope menu-surface lookup to the current test instance to avoid portal flakiness.
Using
document.querySelector('limel-menu-surface')can target a stale portal surface from another test run. Query via the current menu’s portal container instead.🔧 Proposed pattern
- const surface = document.querySelector('limel-menu-surface'); + const portalId = (root as any).portalId; + const portal = document.getElementById(portalId); + const surface = portal?.querySelector('limel-menu-surface'); expect(surface).toBeTruthy();Also applies to: 199-201, 240-242, 275-277, 313-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.e2e.tsx` around lines 153 - 155, The test is querying the global document for 'limel-menu-surface', which can pick up a stale portal from another test; instead scope the lookup to the current menu instance's portal container (use the menu's portal/container element reference used in the test to query for 'limel-menu-surface' rather than document.querySelector), updating each occurrence that sets const surface = document.querySelector('limel-menu-surface') (and the similar checks at the other locations) to query from the current menu's portal container so the surface is tied to this test instance.src/components/menu/menu.types.ts (1)
64-66:⚠️ Potential issue | 🟡 MinorUse singular wording for
hotkeydocumentation.At Line 65, “Hotkey(s)” implies multiple values, but
hotkeyis typed as a singlestring.📝 Proposed fix
- * Hotkey(s) that can be used to select this item while the menu is open. + * A hotkey that can be used to select this item while the menu is open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.types.ts` around lines 64 - 66, The doc comment for the hotkey property uses plural wording "Hotkey(s)" but the symbol hotkey is typed as a single string; update the comment to use singular wording (e.g., "Hotkey") and clarify that this string represents one keyboard shortcut used to select the item while the menu is open, distinguishing it from commandText which is purely visual.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.tsx`:
- Around line 314-339: Extract the duplicated hotkey selection flow used in
handleHotkeyTrigger and handleInputKeyDown into a single helper (e.g.,
selectMenuItemByHotkey) that accepts the hotkey string and optional
KeyboardEvent; this helper should perform the shared checks (ensure this.open,
ensure hotkey is present, call isReservedMenuHotkey, call findMenuItemByHotkey)
and, when a match is found, call handleSelect(matchedItem) and return a flag
indicating whether it handled the event so callers can
stopPropagation/preventDefault; replace the duplicated logic in
handleHotkeyTrigger and handleInputKeyDown to delegate to this helper and only
run event.stopPropagation()/preventDefault() when the helper reports the hotkey
was handled.
- Around line 314-337: In handleHotkeyTrigger, the code uses
event.detail.keyboardEvent without guaranteeing it exists; add a guard for
event.detail?.keyboardEvent (or store it as a local const keyboardEvent =
event.detail?.keyboardEvent) and only call stopPropagation() and
preventDefault() if that keyboardEvent is present, leaving existing early
returns intact and keeping the remaining logic (isReservedMenuHotkey,
findMenuItemByHotkey) unchanged.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example contains duplicate normalized hotkeys: the
<limel-hotkey value="meta+enter" /> and <limel-hotkey value="cmd+enter" />
entries normalize to the same hotkey and cause duplicate-hotkey warnings; fix it
by removing or replacing one of these tags in the hotkey-basic.tsx example
(e.g., remove the <limel-hotkey value="cmd+enter" /> line or replace it with a
different combination such as <limel-hotkey value="ctrl+enter" />) so each
<limel-hotkey> in the example has a unique normalized value.
In `@src/components/hotkey/hotkey.scss`:
- Around line 16-19: Replace the inconsistent "0.375px" blur value in the
box-shadow declaration with the correct unit to match surrounding values (e.g.,
"0.375rem") and make the same change in the corresponding declaration in
_kbd.scss (or remove the local override in hotkey.scss and rely on the forwarded
_kbd.scss partial) so both box-shadow entries are consistent; locate the
box-shadow line in hotkey.scss and the matching box-shadow in _kbd.scss to apply
the fix.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 133-182: formatDisplayToken is missing handling for the 'win' and
'windows' aliases so those keys fall through to raw text; add case 'win' and
case 'windows' alongside the existing 'meta' handling in function
formatDisplayToken so they return the same object (use the isApple conditional
to return { display: '⌘', isGlyph: true } for Apple and { display: '⊞ Win',
isGlyph: false } otherwise), keeping the same structure used for the 'meta'
branch.
- Around line 140-143: The 'cmd' / 'command' switch branch is returning an
unconditional '⌘' glyph which causes incorrect visuals on non-mac platforms;
update the case for 'cmd' and 'command' (in the hotkey token/rendering switch
inside the Hotkey component / hotkey rendering function) to be platform-aware
and mirror the logic used by the 'meta' case (use the same isMac/platform
detection and return either { display: '⌘', isGlyph: true } for mac or the
platform-appropriate string (e.g., 'Win'/'Ctrl' or the same display as the
'meta' branch) for non-mac). Ensure the returned shape still matches { display,
isGlyph } so downstream rendering is unchanged.
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The box-shadow declaration uses a mismatched unit "0.375px"
for the blur radius; update the box-shadow value in the _kbd.scss rule (the
box-shadow line containing "0 0.625rem 0.375px -0.5rem") to use rem instead of
px (i.e., change 0.375px → 0.375rem) so all shadow offsets/blur/spread use
consistent rem units.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Line 7: The example imports isAppleDevice from internal util/device; remove
that external import and make the example self-contained by implementing a small
local isAppleDevice helper inside menu-searchable-hotkeys.tsx (or the same
example module) and use it where referenced (replace usages of isAppleDevice
with the local function); the helper can check navigator.platform or
navigator.userAgent for Apple identifiers (e.g.,
/Mac|iPhone|iPad|iPod|Macintosh/) and return a boolean so the example no longer
depends on internal utilities.
- Around line 33-34: Fix the typo in the example description comment inside the
MenuSearchableHotkeys example: change the phrase "they focus is on the search
input" to "the focus is on the search input" in the comment above the component
(look for the menu-searchable-hotkeys.tsx example / MenuSearchableHotkeys) so
the sentence reads correctly.
In `@src/components/menu/menu.e2e.tsx`:
- Around line 153-155: The test is querying the global document for
'limel-menu-surface', which can pick up a stale portal from another test;
instead scope the lookup to the current menu instance's portal container (use
the menu's portal/container element reference used in the test to query for
'limel-menu-surface' rather than document.querySelector), updating each
occurrence that sets const surface =
document.querySelector('limel-menu-surface') (and the similar checks at the
other locations) to query from the current menu's portal container so the
surface is tied to this test instance.
In `@src/components/menu/menu.types.ts`:
- Around line 64-66: The doc comment for the hotkey property uses plural wording
"Hotkey(s)" but the symbol hotkey is typed as a single string; update the
comment to use singular wording (e.g., "Hotkey") and clarify that this string
represents one keyboard shortcut used to select the item while the menu is open,
distinguishing it from commandText which is purely visual.
In `@src/util/device.ts`:
- Around line 26-29: isIOSDevice() currently only tests /iPad|iPhone|iPod/i so
modern iPadOS 13+ (which reports a Mac-like UA) is missed; update isIOSDevice()
to also detect iPadOS by checking getUserAgent() plus platform/touch
capabilities (e.g., detect navigator.platform === 'MacIntel' and
navigator.maxTouchPoints > 1 or similar via globalThis) so that isIOSDevice()
(and consequently isMobileDevice()) returns true for iPadOS Safari; modify the
function that calls getUserAgent() (isIOSDevice) to include this additional
platform+touch check while preserving the existing regex path.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (19)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
| private readonly handleHotkeyTrigger = ( | ||
| event: CustomEvent<LimelHotkeyTriggerDetail> | ||
| ) => { | ||
| if (!this.open) { | ||
| return; | ||
| } | ||
|
|
||
| const pressedHotkey = event.detail?.hotkey; | ||
| if (!pressedHotkey) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.isReservedMenuHotkey(pressedHotkey)) { | ||
| return; | ||
| } | ||
|
|
||
| const matchedItem = this.findMenuItemByHotkey(pressedHotkey); | ||
| if (!matchedItem) { | ||
| return; | ||
| } | ||
|
|
||
| event.stopPropagation(); | ||
| event.detail.keyboardEvent.stopPropagation(); | ||
| event.detail.keyboardEvent.preventDefault(); |
There was a problem hiding this comment.
Guard keyboardEvent before dereference in hotkey trigger handler.
event.detail?.hotkey is guarded, but event.detail.keyboardEvent is still used unguarded. A malformed/custom hotkeyTrigger event can throw at runtime.
🛠️ Proposed fix
private readonly handleHotkeyTrigger = (
event: CustomEvent<LimelHotkeyTriggerDetail>
) => {
if (!this.open) {
return;
}
const pressedHotkey = event.detail?.hotkey;
- if (!pressedHotkey) {
+ const keyboardEvent = event.detail?.keyboardEvent;
+ if (!pressedHotkey || !keyboardEvent) {
return;
}
if (this.isReservedMenuHotkey(pressedHotkey)) {
return;
}
const matchedItem = this.findMenuItemByHotkey(pressedHotkey);
if (!matchedItem) {
return;
}
event.stopPropagation();
- event.detail.keyboardEvent.stopPropagation();
- event.detail.keyboardEvent.preventDefault();
+ keyboardEvent.stopPropagation();
+ keyboardEvent.preventDefault();
this.handleSelect(matchedItem);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private readonly handleHotkeyTrigger = ( | |
| event: CustomEvent<LimelHotkeyTriggerDetail> | |
| ) => { | |
| if (!this.open) { | |
| return; | |
| } | |
| const pressedHotkey = event.detail?.hotkey; | |
| if (!pressedHotkey) { | |
| return; | |
| } | |
| if (this.isReservedMenuHotkey(pressedHotkey)) { | |
| return; | |
| } | |
| const matchedItem = this.findMenuItemByHotkey(pressedHotkey); | |
| if (!matchedItem) { | |
| return; | |
| } | |
| event.stopPropagation(); | |
| event.detail.keyboardEvent.stopPropagation(); | |
| event.detail.keyboardEvent.preventDefault(); | |
| private readonly handleHotkeyTrigger = ( | |
| event: CustomEvent<LimelHotkeyTriggerDetail> | |
| ) => { | |
| if (!this.open) { | |
| return; | |
| } | |
| const pressedHotkey = event.detail?.hotkey; | |
| const keyboardEvent = event.detail?.keyboardEvent; | |
| if (!pressedHotkey || !keyboardEvent) { | |
| return; | |
| } | |
| if (this.isReservedMenuHotkey(pressedHotkey)) { | |
| return; | |
| } | |
| const matchedItem = this.findMenuItemByHotkey(pressedHotkey); | |
| if (!matchedItem) { | |
| return; | |
| } | |
| event.stopPropagation(); | |
| keyboardEvent.stopPropagation(); | |
| keyboardEvent.preventDefault(); | |
| this.handleSelect(matchedItem); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/menu.tsx` around lines 314 - 337, In handleHotkeyTrigger,
the code uses event.detail.keyboardEvent without guaranteeing it exists; add a
guard for event.detail?.keyboardEvent (or store it as a local const
keyboardEvent = event.detail?.keyboardEvent) and only call stopPropagation() and
preventDefault() if that keyboardEvent is present, leaving existing early
returns intact and keeping the remaining logic (isReservedMenuHotkey,
findMenuItemByHotkey) unchanged.
| /** | ||
| * Use to display optional keyboard shortcut or command hint, e.g. `⌘ + K` | ||
| */ | ||
| @Prop() | ||
| @Prop({ reflect: true }) | ||
| public commandText?: string; | ||
|
|
||
| /** | ||
| * Hotkey to display. When provided, `commandText` is ignored. | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public hotkey?: string; |
There was a problem hiding this comment.
If I recall correctly, you said that the hotkey only works once you've opened a menu, it's basically a way to navigate the menu.
commandText on the other hand, was, when we added it, meant for showing a keyboard command that would activate the underlying function (like "copy"), regardless of whether the menu was open or not. It's a way to communicate a separate way to trigger the same command, not a way to navigate the menu, right?
What's the plan for commandText? Are we going to use it ourselves still? Do we intend to keep it around, or should it be deprecated? The use cases aren't exactly the same, but given that commandText will always lose out to hotkey, it feels like you've kind of staked out hotkey as the new direction? 🙂
There was a problem hiding this comment.
yeah, this was a tricky decision.
The builtin hotkeys are visualized as physical keybaord keys, while the command text, is just a grey string.
I thought if a consumer uses the inbuilt hotkeys, they would have to follow the intentional design, which (in case limel-menu or limel-select) means: the hotkeys only work if they menu is open. This approach is healthy I'd say, as it also reduces the chance of having several identical hotkeys defined in the same view/ pane/ page, doing different things.
For these cases, I think displaying the grey commandText is overkill. So I chose to hide it, and replace it with the visuals for the hotkey instead. Can't say that's a 100% good choice. But we can display both simultaneously if consumers complain later.
Why not deprecate commmandText?
A consumer may want to define global hotkey which should work even when the menu is closed; or they may want to mae the hotkeys run when the menu is open, but maintain the entire logic of how their hotkeys work on their side. In this case, I think it is good that they still have the commmandText to use, and give their users some tips about their existing hotkeys, and make them more discoverable.
Discoverability
Is having a hidden but globally accessible and active hotkey a good design? I doubt it!
If the UI doesn't visibly and clearly communicate that they are hotkeys implemented and globally available in this view/ pane /page, associated with some action, then I think that's a questionable design.
In the web client, I want to ensure that:
- what we call "unpromoted actions" will run, only when the menu is open.
- what we call "promoted actions" will run, only when the floating action bar is visible.
- other actions tied to buttons like Post, Send, Save, Cancel are run, only when their respective buttons are displayed and are enabled.
The only global hotkeys would be those that are used for global navigation, e.g. going Home, navigating to Search results, opening the Add new dialog, etc…
Not sure if I answered all your questions. Please ask or call on Slack if easier 😊
|
a potential upcoming PR showing how |
- Made it safe to import outside the browser (removed top-level `window.navigator.userAgent` access) - Added export function `isAppleDevice(): boolean`
for rendering hotkeys, and emitting events when hotkeys are pressed.
adrianschmidt
force-pushed
the
menu-hotkeys
branch
from
263e1c3 to
b5cb6c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/components/hotkey/hotkey.scss (1)
16-20:⚠️ Potential issue | 🟡 MinorPossible unit mismatch:
0.375pxlikely a typo for0.375rem.The box-shadow mixes
remfor y-offset (0.625rem) and spread (-0.5rem) withpxfor blur (0.375px). A sub-pixel blur radius is effectively 0-1px after browser rounding, which is probably not intended.🔧 Proposed fix
- 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.scss` around lines 16 - 20, The box-shadow in hotkey.scss mixes px and rem units (specifically "0.375px") which is likely a typo; update the blur radius in the box-shadow declaration inside the hotkey.scss file (the rule containing "box-shadow: var(--button-shadow-pressed), 0 0.625rem 0.375px -0.5rem ...") to use rem (e.g., "0.375rem") so units are consistent and the blur is not sub-pixel.src/components/hotkey/examples/hotkey-basic.tsx (1)
51-52:⚠️ Potential issue | 🟡 MinorDuplicate hotkeys will trigger a console warning.
cmdis an alias formeta, so bothmeta+enterandcmd+enternormalize to the same hotkey. This will cause the duplicate-detection logic to emitconsole.warnon every keypress, which may confuse readers using this as a reference example.Consider removing one of them, or moving this pairing to the
hotkey-duplicatesexample where duplicate behavior is explicitly demonstrated.🔧 Proposed fix: Keep only one
<limel-hotkey value="meta+enter" /> - <limel-hotkey value="cmd+enter" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/examples/hotkey-basic.tsx` around lines 51 - 52, The example registers two identical hotkeys ("meta+enter" and "cmd+enter") which normalize to the same binding and trigger console.warn; remove one of the limel-hotkey elements (either the <limel-hotkey value="meta+enter" /> or the <limel-hotkey value="cmd+enter" />) from hotkey-basic.tsx, or move the pair into the hotkey-duplicates example so the duplicate behavior is shown intentionally.src/components/menu/examples/menu-searchable-hotkeys.tsx (2)
7-7: 🧹 Nitpick | 🔵 TrivialInternal utility import in example file.
Importing
isAppleDevicefrom an internal path ('../../../util/device') couples this example to implementation details. Example files should be self-contained or import from the public API.♻️ Inline the logic to make the example self-contained
-import { isAppleDevice } from '../../../util/device'; + +const isAppleDevice = (): boolean => { + return /Mac|iPhone|iPad|iPod/i.test(navigator.userAgent); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` at line 7, The example imports an internal helper isAppleDevice from '../../../util/device', which couples the example to internal implementation; inline the minimal device-detection logic directly in the example (replace references to isAppleDevice) so the file is self-contained: add a small function (e.g., const isAppleDevice = () => /Mac|iPhone|iPad|iPod/.test(navigator.platform || '') || navigator.userAgent.includes('Mac') ) inside the menu-searchable-hotkeys component (MenuSearchableHotkeys) and remove the internal import; ensure all uses of isAppleDevice in the file now call the local inline function.
32-36:⚠️ Potential issue | 🟡 MinorFix typo in documentation.
Line 33 contains a grammatical error: "they focus" should be "the focus".
📝 Proposed fix
- * Consider that when the menu is opened, they focus is on the search input, + * Consider that when the menu is opened, the focus is on the search input,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/examples/menu-searchable-hotkeys.tsx` around lines 32 - 36, Fix the typo in the example comment that reads "they focus" — change it to "the focus" in the comment starting "Consider that when the menu is opened, they focus is on the search input..." so the sentence reads "Consider that when the menu is opened, the focus is on the search input..." which will correct the grammar in the menu-searchable-hotkeys example comment.src/components/markdown/partial-styles/_kbd.scss (1)
26-29:⚠️ Potential issue | 🟡 MinorUnit mismatch in box-shadow:
0.375pxshould likely be0.375rem.Line 28 uses
0.375pxfor the blur radius while all other shadow values userem. This appears to be a typo that creates visual inconsistency.🔧 Proposed fix
box-shadow: var(--button-shadow-normal), - 0 0.625rem 0.375px -0.5rem rgb(var(--color-black), 0.02), + 0 0.625rem 0.375rem -0.5rem rgb(var(--color-black), 0.02), 0 0.025rem 0.5rem 0 rgb(var(--contrast-100)) inset;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markdown/partial-styles/_kbd.scss` around lines 26 - 29, The box-shadow declaration in _kbd.scss uses a mismatched unit — change the blur radius "0.375px" to "0.375rem" in the box-shadow list (the rule containing var(--button-shadow-normal), 0 0.625rem 0.375px -0.5rem rgb(...), 0 0.025rem 0.5rem 0 rgb(...)) so all shadow offsets/blur/spread use rem units for visual consistency.src/components/menu/menu.tsx (1)
321-337:⚠️ Potential issue | 🟡 MinorGuard
keyboardEventbefore dereferencing it.
pressedHotkeyis null-checked, but Lines 336-337 still assumeevent.detail.keyboardEventexists. Since this handler is wired throughaddEventListener, a malformed/customhotkeyTriggerevent can still reach it and throw at runtime.Suggested fix
const pressedHotkey = event.detail?.hotkey; - if (!pressedHotkey) { + const keyboardEvent = event.detail?.keyboardEvent; + if (!pressedHotkey || !keyboardEvent) { return; } @@ event.stopPropagation(); - event.detail.keyboardEvent.stopPropagation(); - event.detail.keyboardEvent.preventDefault(); + keyboardEvent.stopPropagation(); + keyboardEvent.preventDefault(); this.handleSelect(matchedItem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/menu/menu.tsx` around lines 321 - 337, The handler currently dereferences event.detail.keyboardEvent without guarding it; update the hotkey handler (around pressedHotkey, isReservedMenuHotkey, findMenuItemByHotkey) to check that event.detail and event.detail.keyboardEvent are defined before calling event.detail.keyboardEvent.stopPropagation() and .preventDefault(); if keyboardEvent is missing, still call event.stopPropagation() and bail or handle appropriately so malformed/custom hotkeyTrigger events cannot throw.src/components/hotkey/hotkey.tsx (1)
133-180:⚠️ Potential issue | 🟡 MinorRender
win/windowsthe same way they normalize.
normalizeKey()accepts both aliases asmeta, butformatDisplayToken()falls through to raw text for them. That makesvalue="win+k"renderwin + keven though it triggers the Meta/Windows-key combo.Suggested fix
case 'meta': { return isApple ? { display: '⌘', isGlyph: true } : { display: '⊞ Win', isGlyph: false }; } + case 'win': + case 'windows': { + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; + } + case 'cmd': case 'command': { return { display: '⌘', isGlyph: true }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/hotkey/hotkey.tsx` around lines 133 - 180, formatDisplayToken currently doesn't handle the 'win'/'windows' aliases that normalizeKey maps to 'meta', causing "win+k" to render raw text; update the switch in formatDisplayToken to treat 'win' and 'windows' like 'meta' (use the same returned object based on isApple) so the Windows key is displayed as the Meta/Win glyph/text consistently with normalizeKey (refer to the switch handling of 'meta', the local variables lower and isApple, and normalizeKey for expected normalization).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.e2e.tsx`:
- Around line 154-155: Tests for limel-menu are leaving portal elements
(limel-menu-surface) in the document which accumulate across runs; add an
afterEach cleanup that removes any portal elements created by render() (e.g.,
querySelectorAll('limel-menu-surface') and remove each) or explicitly remove the
surface element after each test so stale portals don't pollute subsequent tests;
update the test file to include an afterEach hook that locates
limel-menu-surface nodes and removes them to ensure clean DOM between runs.
In `@src/util/hotkeys.ts`:
- Around line 163-194: The loop currently lets the "last non-modifier wins" (so
"a+b" becomes "b"); instead, detect multiple non-modifier tokens and return null
for malformed inputs: while iterating tokens (using normalizeKey) track whether
a non-modifier key has already been seen (the variable key) and if you encounter
a second non-modifier, immediately return null. Keep the existing modifier
handling (alt/ctrl/meta/shift) and ultimately call normalizeModifiersAndKey({
key, alt, ctrl, meta, shift }) only when exactly one non-modifier key exists.
---
Duplicate comments:
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 51-52: The example registers two identical hotkeys ("meta+enter"
and "cmd+enter") which normalize to the same binding and trigger console.warn;
remove one of the limel-hotkey elements (either the <limel-hotkey
value="meta+enter" /> or the <limel-hotkey value="cmd+enter" />) from
hotkey-basic.tsx, or move the pair into the hotkey-duplicates example so the
duplicate behavior is shown intentionally.
In `@src/components/hotkey/hotkey.scss`:
- Around line 16-20: The box-shadow in hotkey.scss mixes px and rem units
(specifically "0.375px") which is likely a typo; update the blur radius in the
box-shadow declaration inside the hotkey.scss file (the rule containing
"box-shadow: var(--button-shadow-pressed), 0 0.625rem 0.375px -0.5rem ...") to
use rem (e.g., "0.375rem") so units are consistent and the blur is not
sub-pixel.
In `@src/components/hotkey/hotkey.tsx`:
- Around line 133-180: formatDisplayToken currently doesn't handle the
'win'/'windows' aliases that normalizeKey maps to 'meta', causing "win+k" to
render raw text; update the switch in formatDisplayToken to treat 'win' and
'windows' like 'meta' (use the same returned object based on isApple) so the
Windows key is displayed as the Meta/Win glyph/text consistently with
normalizeKey (refer to the switch handling of 'meta', the local variables lower
and isApple, and normalizeKey for expected normalization).
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 26-29: The box-shadow declaration in _kbd.scss uses a mismatched
unit — change the blur radius "0.375px" to "0.375rem" in the box-shadow list
(the rule containing var(--button-shadow-normal), 0 0.625rem 0.375px -0.5rem
rgb(...), 0 0.025rem 0.5rem 0 rgb(...)) so all shadow offsets/blur/spread use
rem units for visual consistency.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Line 7: The example imports an internal helper isAppleDevice from
'../../../util/device', which couples the example to internal implementation;
inline the minimal device-detection logic directly in the example (replace
references to isAppleDevice) so the file is self-contained: add a small function
(e.g., const isAppleDevice = () =>
/Mac|iPhone|iPad|iPod/.test(navigator.platform || '') ||
navigator.userAgent.includes('Mac') ) inside the menu-searchable-hotkeys
component (MenuSearchableHotkeys) and remove the internal import; ensure all
uses of isAppleDevice in the file now call the local inline function.
- Around line 32-36: Fix the typo in the example comment that reads "they focus"
— change it to "the focus" in the comment starting "Consider that when the menu
is opened, they focus is on the search input..." so the sentence reads "Consider
that when the menu is opened, the focus is on the search input..." which will
correct the grammar in the menu-searchable-hotkeys example comment.
In `@src/components/menu/menu.tsx`:
- Around line 321-337: The handler currently dereferences
event.detail.keyboardEvent without guarding it; update the hotkey handler
(around pressedHotkey, isReservedMenuHotkey, findMenuItemByHotkey) to check that
event.detail and event.detail.keyboardEvent are defined before calling
event.detail.keyboardEvent.stopPropagation() and .preventDefault(); if
keyboardEvent is missing, still call event.stopPropagation() and bail or handle
appropriately so malformed/custom hotkeyTrigger events cannot throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffd5d0c7-c2c5-4e32-9636-d347e2237c8e
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (19)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/examples/hotkey-duplicates.tsxsrc/components/hotkey/examples/hotkey-prevent-default.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/components/hotkey/hotkey.types.tssrc/components/list-item/menu-item-meta/menu-item-meta.tsxsrc/components/markdown/partial-styles/_kbd.scsssrc/components/menu-list/menu-list-renderer.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.e2e.tsxsrc/components/menu/menu.tsxsrc/components/menu/menu.types.tssrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
| const surface = document.querySelector('limel-menu-surface'); | ||
| expect(surface).toBeTruthy(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test setup/teardown and portal handling
echo "=== Checking for afterEach/beforeEach/cleanup ==="
rg -n "afterEach|beforeEach|cleanup" src/components/menu/menu.e2e.tsx
echo ""
echo "=== Examining test structure around lines 118-155 ==="
sed -n '100,170p' src/components/menu/menu.e2e.tsxRepository: Lundalogik/lime-elements
Length of output: 2816
🏁 Script executed:
#!/bin/bash
# Get more of the test file to see all hotkeys tests
sed -n '131,250p' src/components/menu/menu.e2e.tsxRepository: Lundalogik/lime-elements
Length of output: 4371
🏁 Script executed:
#!/bin/bash
# Find the end of hotkeys describe block to count tests
echo "=== Finding end of hotkeys block ==="
rg -n "describe\|^\s*}\);" src/components/menu/menu.e2e.tsx | tail -20
echo ""
echo "=== Count it blocks in hotkeys ==="
sed -n '131,$p' src/components/menu/menu.e2e.tsx | rg -c "^\s+it\("Repository: Lundalogik/lime-elements
Length of output: 142
Add cleanup hook to prevent portal accumulation across tests.
The hotkeys tests will accumulate stale portal elements in the DOM across 4 test runs without explicit cleanup (lines 118-121 already document this limitation). While document.querySelector('limel-menu-surface') will find the most recent surface, stale portals persist, polluting subsequent test runs.
Add an afterEach hook to clean up portals created by the render() calls, or explicitly remove the surface element after each test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/menu/menu.e2e.tsx` around lines 154 - 155, Tests for
limel-menu are leaving portal elements (limel-menu-surface) in the document
which accumulate across runs; add an afterEach cleanup that removes any portal
elements created by render() (e.g., querySelectorAll('limel-menu-surface') and
remove each) or explicitly remove the surface element after each test so stale
portals don't pollute subsequent tests; update the test file to include an
afterEach hook that locates limel-menu-surface nodes and removes them to ensure
clean DOM between runs.
| for (const token of tokens) { | ||
| const normalized = normalizeKey(token); | ||
| if (!normalized) { | ||
| continue; | ||
| } | ||
|
|
||
| if (normalized === 'alt') { | ||
| alt = true; | ||
| continue; | ||
| } | ||
| if (normalized === 'ctrl') { | ||
| ctrl = true; | ||
| continue; | ||
| } | ||
| if (normalized === 'meta') { | ||
| meta = true; | ||
| continue; | ||
| } | ||
| if (normalized === 'shift') { | ||
| shift = true; | ||
| continue; | ||
| } | ||
|
|
||
| // Last non-modifier wins | ||
| key = normalized; | ||
| } | ||
|
|
||
| if (!key) { | ||
| return null; | ||
| } | ||
|
|
||
| return normalizeModifiersAndKey({ key, alt, ctrl, meta, shift }); |
There was a problem hiding this comment.
Reject multiple non-modifier tokens instead of silently taking the last one.
The JSDoc says invalid strings return null, but a+b currently normalizes to b and ctrl+a+b to ctrl+b. That makes malformed configs bind the wrong shortcut instead of being ignored.
Suggested fix
for (const token of tokens) {
const normalized = normalizeKey(token);
if (!normalized) {
continue;
}
@@
- // Last non-modifier wins
+ if (key) {
+ return null;
+ }
+
key = normalized;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/hotkeys.ts` around lines 163 - 194, The loop currently lets the
"last non-modifier wins" (so "a+b" becomes "b"); instead, detect multiple
non-modifier tokens and return null for malformed inputs: while iterating tokens
(using normalizeKey) track whether a non-modifier key has already been seen (the
variable key) and if you encounter a second non-modifier, immediately return
null. Keep the existing modifier handling (alt/ctrl/meta/shift) and ultimately
call normalizeModifiersAndKey({ key, alt, ctrl, meta, shift }) only when exactly
one non-modifier key exists.
Architectural Analysis of Hotkey Implementation(by Claude Code with Opus 4.6, effort: high) I've reviewed the PR, the issue, the review discussion, and the existing webclient keyboard shortcut system. Here's an analysis of the concerns Adrian raised, and a suggestion for how to move forward. Two Distinct Concepts Being ConflatedThere are two fundamentally different things at play here:
The PR's What the Current Implementation DoesEach
So the architecture is: N global keydown listeners (one per menu item with a hotkey), all evaluating every keypress, with the menu filtering out events when closed. Problems1. Architectural mismatch with the existing system. 2. The component does too much.
For the menu-scoped use case, the visual rendering is great, but the global listening is architecturally wrong — even though the menu happens to suppress it when closed. The menu should own its keyboard handling internally. 3. Performance at scale. 4. The "handled flag" fragility. Suggested SeparationA. Visual component (display-only)Keep the
B. Menu-internal keyboard navigationBuilt into the menu component itself:
// Single listener, internal to menu
@Listen('keydown')
private handleKeydown(event: KeyboardEvent) {
if (!this.open) return;
const pressed = hotkeyFromKeyboardEvent(event);
const matchedItem = this.findMenuItemByHotkey(pressed);
if (matchedItem) {
this.handleSelect(matchedItem);
event.preventDefault();
}
}C. Global shortcuts (already exists, separate concern)The Communication to the UserWith this separation, the convention becomes self-reinforcing:
Single keys in menus can't work globally anyway (they'd fire while typing), and modifier keys shown outside menus obviously work globally. No special notation needed. Recommendation
What Can Be Kept From This PRA large portion of the work here is reusable:
The main change would be removing the event-listening responsibility from |
|
@adrianschmidt I agree with everything, and have no issues with moving out the listener to the menu. I actually started like this, and think that approach is better. Having a hotkey visualizer component is way better than leaving the event emitting too, to the visualizer. This will make it really hard for the hotkey to be used inside a tooltip surface for instance. However, one thing that I think is unnecessarily in the way is how hotkeys are handled in the web-client today. I really dislike that the are globally available all the time. I think, a hotkey, must be scoped/limited to its container. In my opinions, a hotkey should:
So having a globally active hotkey is a bad idea. Without this approach that I explained above, we can never use the same hotkey combination for two different things, in two different places within the same app-screen. What to do about webclient's existing I hope I could clearly explain how I think and reason about this. Shall I now move out the event emitting logic from limel-hotkey and refactor in this PR? |
|
closing this PR in favor of #3915 where the requested changes above are applied, but we also have a new implementation of hotkeys in I think we should do some refactoring regarding how hotkeys are designed to work in the web client. We should adress that issue there instead. @adrianschmidt-bot I asked you to make a PR once this PR is merged, as explained here: #3818 (comment) But now since I'm closing this PR without merging, I want you to keep an eye on the other pr instead: #3915 Hope it works |
|
Got it! 👍 I've updated my notes — I'll watch PR #3915 instead and create the |
5 participants
fix https://github.com/Lundalogik/crm-client/issues/754
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: