Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a hotkey system: utilities for tokenizing/normalizing hotkeys and mapping KeyboardEvent → hotkey; a presentational Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Document as Document (keydown)
participant HotkeyUtil as Hotkey Utilities
participant Menu as Menu Component
participant Item as MenuItem
Note over User,Document: Global hotkey selection flow
User->>Document: press key combo
Document->>HotkeyUtil: hotkeyFromKeyboardEvent(event)
HotkeyUtil-->>Document: normalized hotkey
Document->>Menu: onDocumentKeydown(normalized hotkey)
Menu->>Menu: find first visible, enabled item with matching normalized hotkey (cache lookup)
alt match found
Menu->>Item: select item (preventDefault, stopPropagation)
Item-->>Menu: emits select
else no match
Document-->>User: no action
end
sequenceDiagram
participant Select as Select Component
participant HotkeyCache as Normalized Hotkey Cache
participant HotkeyUtil as Hotkey Utilities
participant Device as Device Detection
participant OptionList as Options
Note over Select,OptionList: Select open-state hotkey handling
Select->>Device: isAppleDevice()
Device-->>Select: platform info
OptionList->>Select: options (with hotkey)
Select->>HotkeyCache: getNormalizedHotkey(option.hotkey)
alt cache miss
HotkeyCache->>HotkeyUtil: normalizeHotkeyString(option.hotkey)
HotkeyUtil-->>HotkeyCache: normalized value
HotkeyCache-->>Select: store & return
else cache hit
HotkeyCache-->>Select: return cached
end
Select->>Select: document keydown -> hotkeyFromKeyboardEvent -> find option -> select/toggle -> emit change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 |
13 tasks
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3915/ |
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
1b3580a to
5f249c2
Compare
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
cd6eddb to
974b87d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/select/select.tsx (2)
218-264:⚠️ Potential issue | 🔴 CriticalRemove or use the new menu-surface ref plumbing.
Line 218 wires
setMenuSurfaceRefthrough, but Lines 262-264 drop the element on the floor. This already fails lint on Line 263, so CI stays red until the callback is deleted or the ref is actually stored and used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/select.tsx` around lines 218 - 264, The setMenuSurfaceRef passed down (setMenuSurfaceElement) is currently a no-op which fails lint and drops the new ref plumbing; either remove the prop wiring or implement the callback to store and use the element. Fix by adding a class property (e.g., menuSurfaceElement: HTMLLimelMenuSurfaceElement | null) and have setMenuSurfaceElement(_element) assign this.menuSurfaceElement = _element (and perform any needed follow-up like syncing open state or z-index); if you choose to remove the plumbing, delete the setMenuSurfaceRef prop usage where it is passed so the unused callback is not declared. Ensure you update any code that needs the stored ref (dropdownZIndex/anchor logic) to read from menuSurfaceElement if applicable.
224-225:⚠️ Potential issue | 🟡 MinorThe mobile guard disables hotkeys for custom multi-selects too.
Lines 224-225 only switch to the native
<select>whenthis.isMobileDevice && !this.multiple, but Lines 399-405 bail out for every mobile device. A mobile multi-select therefore still renders the custom menu while hotkeys never work. If that is not intentional, gate on the same condition asnative.Suggested guard
- this.isMobileDevice || + (this.isMobileDevice && !this.multiple) ||Also applies to: 399-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/select.tsx` around lines 224 - 225, The mobile-only guard that currently disables hotkeys on every mobile device should be changed to match the same condition used for the native prop (this.isMobileDevice && !this.multiple) so hotkeys remain active for custom multi-selects; locate the bail-out block that checks this.isMobileDevice (around the hotkey handling code in the Select component) and change its condition to this.isMobileDevice && !this.multiple (or gate it by the same computed native value) so the component only abandons custom hotkey logic when the native select is actually used.
🤖 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/hotkey/hotkey.tsx`:
- Around line 80-121: The switch handling key aliases should preserve actual
shortcut semantics: make the 'cmd'/'command' case behave the same as the 'meta'
case (use isApple to decide between the ⌘ glyph on macOS and a non-glyph label
on other platforms) instead of hardcoding ⌘, and split the
'delete'/'del'/'backspace' handling so on Apple devices 'delete' (forward
delete) renders as ⌦ and 'backspace' as ⌫ while non-Apple platforms keep "Del"
vs "Backspace" text; update the switch on the same lower variable and modify the
'cmd'/'command' and 'delete'/'del'/'backspace' cases accordingly.
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 60-61: Replace the conflicting hotkey on the menu item with text
"Copy message" by changing its hotkey property (the object with text: 'Copy
message') from "meta+c" to a non-platform-conflicting combination such as
"meta+shift+c" (or "mod+shift+c") so it no longer collides with native
Cmd/Ctrl+C; update the hotkey value on that menu item object in the MenuHotkeys
examples accordingly.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Around line 96-98: Mark the component's callback fields as readonly: change
the handler field declarations (for example the private handleSearch async arrow
function and the other callback handlers used in this file) to use the readonly
modifier so they cannot be reassigned; specifically update the field
declarations to be private readonly <handlerName> = (...) => ... (preserving
async/return types) for handleSearch and the other handler(s) referenced in this
component.
- Line 7: The example imports the internal helper isAppleDevice from
src/util/device which breaks the self-contained examples rule; remove that
import and either inline a small platform check (e.g. detect "Mac" in
navigator.platform or userAgent) directly inside the menu-searchable-hotkeys
example or move a shared helper into the examples folder and import it
relatively; update any references to isAppleDevice in this file to use the new
inline check or new local helper so the example only uses relative/example-local
code or public exports.
In `@src/components/menu/menu.tsx`:
- Line 249: The ref prop currently points to a no-op handler setSurfaceElement
which accepts an element but does nothing and causes an unused-argument lint;
remove the dead ref and the stub to fix this. Locate the setSurfaceElement
method and any usages (e.g., the ref={this.setSurfaceElement} at the Menu
component lines around where renderSurface is defined and the similar refs at
318-320) and delete the method and the ref attributes; if you plan to use the
surface element later, replace the ref with a meaningful handler or a
React.createRef() instance, otherwise simply drop both the ref attributes and
the setSurfaceElement function.
- Around line 583-596: This duplicate hotkey path bypasses the repeat-guard in
handleDocumentKeyDown; add the same event.repeat check (or route through the
shared hotkey-selection helper) before processing modifiers so repeated keydown
events don't re-enter handleSelect for lazy-loaded submenus—specifically, in the
block that computes hasModifier and calls hotkeyFromKeyboardEvent, early-return
if event.repeat is true (or call the existing helper used by
handleDocumentKeyDown) so findMenuItemByHotkey and handleSelect cannot be
invoked repeatedly while a key is held.
In `@src/components/select/examples/select-hotkeys.tsx`:
- Around line 64-68: The example option object with value 'closed' currently
uses a conflicting hotkey 'cmd+d'; update the object's hotkey property (the
entry where value === 'closed' / hotkey: 'cmd+d') to a neutral shortcut that
won't collide with browser/OS shortcuts (e.g., 'cmd+e' or 'cmd+n') so the
select-hotkeys example follows the project's hotkey guidance and functions
reliably across platforms.
---
Outside diff comments:
In `@src/components/select/select.tsx`:
- Around line 218-264: The setMenuSurfaceRef passed down (setMenuSurfaceElement)
is currently a no-op which fails lint and drops the new ref plumbing; either
remove the prop wiring or implement the callback to store and use the element.
Fix by adding a class property (e.g., menuSurfaceElement:
HTMLLimelMenuSurfaceElement | null) and have setMenuSurfaceElement(_element)
assign this.menuSurfaceElement = _element (and perform any needed follow-up like
syncing open state or z-index); if you choose to remove the plumbing, delete the
setMenuSurfaceRef prop usage where it is passed so the unused callback is not
declared. Ensure you update any code that needs the stored ref
(dropdownZIndex/anchor logic) to read from menuSurfaceElement if applicable.
- Around line 224-225: The mobile-only guard that currently disables hotkeys on
every mobile device should be changed to match the same condition used for the
native prop (this.isMobileDevice && !this.multiple) so hotkeys remain active for
custom multi-selects; locate the bail-out block that checks this.isMobileDevice
(around the hotkey handling code in the Select component) and change its
condition to this.isMobileDevice && !this.multiple (or gate it by the same
computed native value) so the component only abandons custom hotkey logic when
the native select is actually used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08786628-8049-43bf-9d03-23b75f550e70
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (20)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/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/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
2 times, most recently
from
8923b5b to
501f8f1
Compare
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
5348045 to
b32fd91
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/menu/menu.tsx`:
- Line 214: The field normalizedHotkeyCache is never reassigned, so mark it
readonly to signal immutability and satisfy static analysis; update the
declaration of normalizedHotkeyCache (the Map<string, string | null> field in
the menu component) to be readonly so it becomes: readonly normalizedHotkeyCache
= new Map<string, string | null>(); ensuring no other code attempts to reassign
the property.
- Around line 317-339: handleDocumentKeyDown is invoking hotkey handling even
when the user is typing into the menu's search input; update
handleDocumentKeyDown to early-return if the keyboard event originated from a
text-editable element by inspecting event.composedPath() or event.target and
checking for input, textarea, or elements with contentEditable=true (or a CSS
class/id of the menu search input). Keep the existing flow with
hotkeyFromKeyboardEvent, isReservedMenuHotkey, findMenuItemByHotkey, and
handleSelect unchanged—only add the input-origin guard at the top of
handleDocumentKeyDown so typing in the search field is not interpreted as a
hotkey.
In `@src/components/select/select.tsx`:
- Around line 394-437: Mark the event handler method handleDocumentKeyDown as
readonly and add a guard that ignores keyboard events originating from text
inputs or content-editable elements to future-proof against search fields;
specifically, change the declaration to a readonly property (private readonly
handleDocumentKeyDown = ...) and at the top of the function return early if the
event target (or first element in event.composedPath()) is an INPUT, TEXTAREA,
or an element with contentEditable not equal to "false" (or has role of
textbox), so hotkey handling only proceeds for non-text-entry elements.
- Line 128: The field normalizedHotkeyCache on the Select component is never
reassigned; mark it readonly to reflect immutability and satisfy static
analysis. Update the declaration of normalizedHotkeyCache (the Map<string,
string | null> instance) to be readonly so the compiler knows it won't be
reassigned, leaving mutation of the Map's contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 069840fe-8d24-402d-b1a0-f4cc3a08e8d0
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (8)
src/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.tsxsrc/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/menu.tsxsrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
0e65414 to
fe74f8e
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/menu/examples/menu-hotkeys.tsx (1)
60-61:⚠️ Potential issue | 🟡 MinorReplace
meta+cwith a non-conflicting hotkey.The documentation at lines 33-37 explicitly warns against using hotkeys that conflict with browser/OS shortcuts.
meta+cmaps toCmd+C(macOS copy) andWin+C(Windows), both platform-reserved shortcuts. Consider usingmeta+shift+cor another modifier combination instead.💡 Suggested fix
- { text: 'Copy message', hotkey: 'meta+c' }, + { text: 'Copy message', hotkey: 'meta+shift+c' },,
🤖 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 60 - 61, The hotkey "meta+c" assigned to the menu item with text "Copy message" conflicts with system/browser copy; locate the menu items array in menu-hotkeys.tsx (the entries containing { text: 'Copy link', hotkey: 'l' } and { text: 'Copy message', hotkey: 'meta+c' }) and replace the 'meta+c' string with a non-conflicting combo such as 'meta+shift+c' (or another modifier combo you prefer) so the "Copy message" hotkey no longer overrides the platform copy shortcut.src/components/menu/examples/menu-searchable-hotkeys.tsx (1)
99-101: 🧹 Nitpick | 🔵 TrivialMark handler fields as
readonly.Per static analysis, these arrow function handlers are never reassigned.
♻️ Suggested fix
- private handleSearch = async ( + private readonly handleSearch = async ( queryString: string ): Promise<Array<MenuItem | ListSeparator>> => {- private handleSelect = ( + private readonly handleSelect = ( event: LimelMenuCustomEvent<MenuItem<SuggestionValue>> ) => {,
Also applies to: 144-146
🤖 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 99 - 101, Mark arrow-function handler fields as readonly to reflect that they are never reassigned: change the class field declaration for handleSearch (private handleSearch = async (...)) to private readonly handleSearch = async (...) and do the same for the other arrow-function handler field in this file (the secondary handler defined after handleSearch). This is a simple modifier addition on the class field declarations (add readonly to those private handler fields) and requires no other logic changes.
🤖 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/menu/examples/menu-hotkeys.tsx`:
- Around line 60-61: The hotkey "meta+c" assigned to the menu item with text
"Copy message" conflicts with system/browser copy; locate the menu items array
in menu-hotkeys.tsx (the entries containing { text: 'Copy link', hotkey: 'l' }
and { text: 'Copy message', hotkey: 'meta+c' }) and replace the 'meta+c' string
with a non-conflicting combo such as 'meta+shift+c' (or another modifier combo
you prefer) so the "Copy message" hotkey no longer overrides the platform copy
shortcut.
In `@src/components/menu/examples/menu-searchable-hotkeys.tsx`:
- Around line 99-101: Mark arrow-function handler fields as readonly to reflect
that they are never reassigned: change the class field declaration for
handleSearch (private handleSearch = async (...)) to private readonly
handleSearch = async (...) and do the same for the other arrow-function handler
field in this file (the secondary handler defined after handleSearch). This is a
simple modifier addition on the class field declarations (add readonly to those
private handler fields) and requires no other logic changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8879f6b-def2-45c4-a2ab-ad79a75d524c
📒 Files selected for processing (4)
src/components/menu/examples/menu-hotkeys.tsxsrc/components/menu/examples/menu-searchable-hotkeys.tsxsrc/components/menu/menu.tsxsrc/util/hotkeys.ts
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
fe74f8e to
60f0590
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/menu/examples/menu-hotkeys.tsx (1)
61-61:⚠️ Potential issue | 🟡 MinorReplace
meta+cwith a non-conflicting hotkey.This hotkey conflicts with platform copy shortcuts (
Cmd+Con macOS,Win+Con Windows), which contradicts the warning in the documentation at lines 33-38.🛠️ Suggested fix
- { text: 'Copy message', hotkey: 'meta+c' }, + { text: 'Copy message', hotkey: 'meta+shift+c' },🤖 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` at line 61, The menu example currently assigns the hotkey string 'meta+c' to the menu item with text 'Copy message' which conflicts with platform copy shortcuts; update that hotkey value (the hotkey property on the menu item object) to a non-conflicting combination such as 'mod+shift+c' (or another app-specific chord) in src/components/menu/examples/menu-hotkeys.tsx so it no longer uses 'meta+c' and matches the docs' warning.
🤖 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-list/menu-list-renderer.tsx`:
- Around line 108-121: Remove the unnecessary (item as any) casts and use the
typed MenuItem properties directly: replace occurrences in the hasHotkey
calculation and in the primaryComponent props (commandText, hotkey, disabled) so
you read item.hotkey, item.commandText and item.disabled directly; ensure the
local variable/item is the narrowed MenuItem (as already established by the
separator check) and update hasMeta/primaryComponent to rely on those typed
properties rather than casting.
In `@src/components/select/select.tsx`:
- Around line 230-244: The component currently only attaches the document
keydown listener inside the `@Watch`('menuOpen') handler (watchOpen) so if the
component mounts with menuOpen === true the listener is never added; update the
component's connectedCallback to check this.menuOpen and, if true, add the
listener via document.addEventListener('keydown', this.handleDocumentKeyDown,
true), and ensure the listener is removed in disconnectedCallback
(document.removeEventListener(...)) to mirror the pattern used in menu.tsx and
keep behavior consistent with watchOpen.
---
Duplicate comments:
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Line 61: The menu example currently assigns the hotkey string 'meta+c' to the
menu item with text 'Copy message' which conflicts with platform copy shortcuts;
update that hotkey value (the hotkey property on the menu item object) to a
non-conflicting combination such as 'mod+shift+c' (or another app-specific
chord) in src/components/menu/examples/menu-hotkeys.tsx so it no longer uses
'meta+c' and matches the docs' warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4107755-bc6c-426b-b354-d0949a324f98
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (20)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/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/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Comment on lines
+108
to
+121
| const hasHotkey = 'hotkey' in item && !!(item as any).hotkey; | ||
| const hasMeta = | ||
| hasSubMenu || | ||
| item.badge !== undefined || | ||
| hasHotkey || | ||
| (!!('commandText' in item) && !!item.commandText); | ||
|
|
||
| const primaryComponent = hasMeta | ||
| ? { | ||
| name: 'limel-menu-item-meta', | ||
| props: { | ||
| commandText: (item as any).commandText, | ||
| hotkey: (item as any).hotkey, | ||
| disabled: !!item.disabled, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using proper types instead of (item as any) casts.
The MenuItem interface already declares hotkey?: string (see src/components/menu/menu.types.ts:58-76), so the type casts are unnecessary. The item variable at this point is already typed as MenuItem after the separator check.
♻️ Proposed refactor to remove type casts
- const hasHotkey = 'hotkey' in item && !!(item as any).hotkey;
+ const hasHotkey = !!item.hotkey;
const hasMeta =
hasSubMenu ||
item.badge !== undefined ||
hasHotkey ||
- (!!('commandText' in item) && !!item.commandText);
+ !!item.commandText;
const primaryComponent = hasMeta
? {
name: 'limel-menu-item-meta',
props: {
- commandText: (item as any).commandText,
- hotkey: (item as any).hotkey,
+ commandText: item.commandText,
+ hotkey: item.hotkey,
disabled: !!item.disabled,
- badge: (item as any).badge,
+ badge: item.badge,
showChevron: hasSubMenu,
},
}
: undefined;🤖 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 - 121,
Remove the unnecessary (item as any) casts and use the typed MenuItem properties
directly: replace occurrences in the hasHotkey calculation and in the
primaryComponent props (commandText, hotkey, disabled) so you read item.hotkey,
item.commandText and item.disabled directly; ensure the local variable/item is
the narrowed MenuItem (as already established by the separator check) and update
hasMeta/primaryComponent to rely on those typed properties rather than casting.
adrianschmidt
force-pushed
the
menu-select-hotkeys
branch
from
60f0590 to
90bcf61
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/menu/examples/menu-hotkeys.tsx (1)
60-61:⚠️ Potential issue | 🟡 MinorReplace
meta+cwith a non-conflicting hotkey.The documentation at lines 33-37 explicitly warns against using hotkeys that conflict with browser/OS shortcuts.
meta+cmaps toCmd+C(copy) on macOS, which directly contradicts this guidance.Consider using
meta+shift+cor another modifier combination instead.🤖 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 60 - 61, Replace the conflicting hotkey 'meta+c' in the menu item for "Copy message" with a non-conflicting combination (e.g., 'meta+shift+c'); locate the menu items array where entries include { text: 'Copy link', hotkey: 'l' } and { text: 'Copy message', hotkey: 'meta+c' } and update the second entry's hotkey string to 'meta+shift+c' (or another modifier+key that won't conflict with native Cmd/Ctrl+C).src/components/select/examples/select-hotkeys.tsx (1)
64-69:⚠️ Potential issue | 🟡 MinorReplace
cmd+dwith a non-conflicting hotkey.Line 31 explicitly warns against using hotkeys that conflict with browser shortcuts.
cmd+dmaps toCmd+Don macOS which bookmarks the page in Chrome, Firefox, and Safari.Consider using
cmd+shift+dor another combination instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/examples/select-hotkeys.tsx` around lines 64 - 69, Hotkey "cmd+d" in the select example conflicts with browser bookmark shortcuts; update the hotkey property in the example object (the item with value 'closed' in src/components/select/examples/select-hotkeys.tsx) to a non-conflicting combo such as 'cmd+shift+d' (or better use a platform-agnostic token like 'mod+shift+d' if your hotkey system supports it) and ensure the object's key remains hotkey so the example reflects the new binding.
🤖 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/select/examples/select-hotkeys.tsx`:
- Line 42: The class has members that are never reassigned—mark the property
options and the handleChange arrow method as readonly to satisfy static
analysis: update the declaration of options (type (Option | ListSeparator)[]) to
readonly and prepend readonly to the handleChange class field declaration so
both are immutable class members (apply the same change for the other occurrence
referenced as handleChange).
In `@src/util/hotkeys.spec.ts`:
- Around line 20-158: Add a unit test to verify canonical modifier ordering for
hotkeyFromKeyboardEvent: create a KeyboardEvent-like object with metaKey,
ctrlKey, altKey, and shiftKey all true and key/code for 'a' (code 'KeyA'), call
hotkeyFromKeyboardEvent and assert the result equals 'meta+ctrl+alt+shift+a';
place this new it(...) alongside the other hotkeyFromKeyboardEvent tests so the
canonical ordering is explicitly validated.
---
Duplicate comments:
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 60-61: Replace the conflicting hotkey 'meta+c' in the menu item
for "Copy message" with a non-conflicting combination (e.g., 'meta+shift+c');
locate the menu items array where entries include { text: 'Copy link', hotkey:
'l' } and { text: 'Copy message', hotkey: 'meta+c' } and update the second
entry's hotkey string to 'meta+shift+c' (or another modifier+key that won't
conflict with native Cmd/Ctrl+C).
In `@src/components/select/examples/select-hotkeys.tsx`:
- Around line 64-69: Hotkey "cmd+d" in the select example conflicts with browser
bookmark shortcuts; update the hotkey property in the example object (the item
with value 'closed' in src/components/select/examples/select-hotkeys.tsx) to a
non-conflicting combo such as 'cmd+shift+d' (or better use a platform-agnostic
token like 'mod+shift+d' if your hotkey system supports it) and ensure the
object's key remains hotkey so the example reflects the new binding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b644b11a-ed62-4249-ad5a-b71d824db55e
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (20)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/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/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
| @State() | ||
| private value: Option; | ||
|
|
||
| private options: (Option | ListSeparator)[] = [ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mark class members as readonly per static analysis.
The options array and handleChange arrow function are never reassigned.
🔧 Suggested fix
- private options: (Option | ListSeparator)[] = [
+ private readonly options: (Option | ListSeparator)[] = [- private handleChange = (event: LimelSelectCustomEvent<Option>) => {
+ private readonly handleChange = (event: LimelSelectCustomEvent<Option>) => {Also applies to: 86-86
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 42-42: Member 'options' is never reassigned; mark it as readonly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/select/examples/select-hotkeys.tsx` at line 42, The class has
members that are never reassigned—mark the property options and the handleChange
arrow method as readonly to satisfy static analysis: update the declaration of
options (type (Option | ListSeparator)[]) to readonly and prepend readonly to
the handleChange class field declaration so both are immutable class members
(apply the same change for the other occurrence referenced as handleChange).
Comment on lines
+20
to
+158
| describe('hotkeyFromKeyboardEvent', () => { | ||
| it('includes multiple modifiers', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: 'a', | ||
| code: 'KeyA', | ||
| altKey: true, | ||
| ctrlKey: true, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| expect(pressed).toBe('ctrl+alt+a'); | ||
| }); | ||
|
|
||
| it('handles function keys', () => { | ||
| expect( | ||
| hotkeyFromKeyboardEvent({ | ||
| key: 'F1', | ||
| code: 'F1', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any) | ||
| ).toBe('f1'); | ||
|
|
||
| expect( | ||
| hotkeyFromKeyboardEvent({ | ||
| key: 'F12', | ||
| code: 'F12', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any) | ||
| ).toBe('f12'); | ||
| }); | ||
|
|
||
| it('handles arrow keys', () => { | ||
| expect( | ||
| hotkeyFromKeyboardEvent({ | ||
| key: 'ArrowLeft', | ||
| code: 'ArrowLeft', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any) | ||
| ).toBe('arrowleft'); | ||
|
|
||
| expect( | ||
| hotkeyFromKeyboardEvent({ | ||
| key: 'ArrowRight', | ||
| code: 'ArrowRight', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any) | ||
| ).toBe('arrowright'); | ||
| }); | ||
|
|
||
| it('uses event.code for non-QWERTY letter normalization', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: '@', | ||
| code: 'KeyQ', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| // Uses code "KeyQ" to normalize to "q" (layout-independent) | ||
| expect(pressed).toBe('q'); | ||
| }); | ||
|
|
||
| it('uses event.code for digit normalization', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: ')', | ||
| code: 'Digit0', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| // Uses code "Digit0" to normalize to "0" (layout-independent) | ||
| expect(pressed).toBe('0'); | ||
| }); | ||
|
|
||
| it('does not include shift for the + key', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: '+', | ||
| code: 'Equal', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: true, | ||
| } as any); | ||
|
|
||
| expect(pressed).toBe('+'); | ||
| }); | ||
|
|
||
| it('still includes shift for letter keys', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: 'A', | ||
| code: 'KeyA', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: true, | ||
| } as any); | ||
|
|
||
| expect(pressed).toBe('shift+a'); | ||
| }); | ||
|
|
||
| it('keeps Backspace and Delete as distinct keys', () => { | ||
| const backspace = hotkeyFromKeyboardEvent({ | ||
| key: 'Backspace', | ||
| code: 'Backspace', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| const del = hotkeyFromKeyboardEvent({ | ||
| key: 'Delete', | ||
| code: 'Delete', | ||
| altKey: false, | ||
| ctrlKey: false, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| expect(backspace).toBe('backspace'); | ||
| expect(del).toBe('delete'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for canonical modifier ordering.
The hotkeyFromKeyboardEvent tests are comprehensive, but there's no explicit test verifying that modifiers are output in canonical order (meta, ctrl, alt, shift). Consider adding a test case with multiple modifiers to confirm ordering:
it('outputs modifiers in canonical order', () => {
const pressed = hotkeyFromKeyboardEvent({
key: 'a',
code: 'KeyA',
altKey: true,
ctrlKey: true,
metaKey: true,
shiftKey: true,
} as any);
expect(pressed).toBe('meta+ctrl+alt+shift+a');
});🤖 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 a unit test to verify
canonical modifier ordering for hotkeyFromKeyboardEvent: create a
KeyboardEvent-like object with metaKey, ctrlKey, altKey, and shiftKey all true
and key/code for 'a' (code 'KeyA'), call hotkeyFromKeyboardEvent and assert the
result equals 'meta+ctrl+alt+shift+a'; place this new it(...) alongside the
other hotkeyFromKeyboardEvent tests so the canonical ordering is explicitly
validated.
90bcf61 to
2d151c9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/components/select/examples/select-hotkeys.tsx (1)
64-69:⚠️ Potential issue | 🟡 MinorReplace
cmd+dwith a non-conflicting hotkey.The documentation at lines 24-32 explicitly warns against using hotkeys that conflict with common browser shortcuts, yet
cmd+ddirectly conflicts with the bookmark shortcut on macOS browsers (Cmd+D) and Windows desktop (Win+D shows desktop).Consider using
cmd+shift+doralt+dinstead.🔧 Suggested fix
{ text: 'Closed', secondaryText: 'as not planned', value: 'closed', - hotkey: 'cmd+d', + hotkey: 'alt+d', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/examples/select-hotkeys.tsx` around lines 64 - 69, The hotkey 'cmd+d' in the select option object (the entry with text 'Closed', secondaryText 'as not planned', value 'closed', hotkey: 'cmd+d') conflicts with common browser shortcuts; update that object's hotkey value to a non-conflicting combo such as 'cmd+shift+d' or 'alt+d' (i.e., modify the hotkey property on the 'Closed' option to the chosen alternative).src/components/menu/examples/menu-hotkeys.tsx (1)
60-61:⚠️ Potential issue | 🟡 MinorReplace
meta+cwith a non-conflicting hotkey.The documentation at lines 33-38 warns against hotkeys conflicting with built-in browser shortcuts, yet
meta+cmaps toCmd+C(copy) on macOS andWin+Con Windows—both claimed by the platform.🔧 Suggested fix
{ text: 'Copy link', hotkey: 'l' }, - { text: 'Copy message', hotkey: 'meta+c' }, + { text: 'Copy message', hotkey: 'meta+shift+c' },🤖 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 60 - 61, Replace the conflicting hotkey string 'meta+c' used for the "Copy message" menu item with a non-conflicting accelerator; locate the menu item object in menu-hotkeys.tsx (the entry with text: 'Copy message' and hotkey: 'meta+c') and change its hotkey to something that won’t override platform copy (for example use a letters-only or modifier combination that isn’t reserved), updating any associated hotkey handlers to match the new key string.src/components/hotkey/hotkey.tsx (1)
80-121:⚠️ Potential issue | 🟠 MajorKeep alias rendering aligned with normalized hotkey semantics.
cmd/commandare normalized to the same key asmeta, but this switch still hardcodes⌘, so the same shortcut renders differently on non-Apple devices depending on which alias the caller used. The Apple delete branch also collapsesdeleteandbackspaceinto the same glyph, which makes forward-delete hints incorrect.♻️ Suggested fix
case 'cmd': case 'command': { - return { display: '⌘', isGlyph: true }; + return isApple + ? { display: '⌘', isGlyph: true } + : { display: '⊞ Win', isGlyph: false }; } @@ case 'delete': case 'del': case 'backspace': { if (isApple) { - return { display: '⌫', isGlyph: true }; + return lower === 'backspace' + ? { display: '⌫', isGlyph: true } + : { display: '⌦', isGlyph: true }; } return lower === 'backspace' ? { display: 'Backspace', isGlyph: false } : { display: 'Del', 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 80 - 121, The switch handling normalized key aliases should render aliases consistently: change the 'cmd'/'command' case so it returns the same value as the 'meta' case (i.e., respect isApple and return the same object used for 'meta' rather than hardcoding '⌘'), and change the delete/del/backspace logic so on Apple platforms you do not collapse delete and backspace into the same glyph — return '⌫' (backspace) when lower === 'backspace' and the forward-delete glyph (e.g., '⌦') when lower === 'delete' (and keep the non-Apple text branches for 'Backspace' vs 'Del'); update the switch cases around lower, 'meta', 'cmd', 'command', and 'delete'/'del'/'backspace' accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 31-33: The kbd::first-letter rule (&::first-letter {
text-transform: uppercase; }) is redundant or ineffective for limel-hotkey
because its text is wrapped in a span; decide whether to keep it for raw
markdown kbd elements or remove/relocate it: if the intent is to style only
markdown kbd, restrict the selector to target kbd elements that contain text
(e.g., kbd:not(:has(> span)) or simply keep it but document scope); if the
intent is to style hotkey spans, remove this rule from _kbd.scss and apply
text-transform to the hotkey-specific selector in hotkey.scss (e.g.,
.limel-hotkey span or the existing span selector) so the first-letter transform
actually affects rendered text.
In `@src/components/menu/menu.tsx`:
- Around line 345-373: The isFromTextInput method currently only checks
contentEditable and native INPUT/TEXTAREA/SELECT elements; update it to also
detect elements that act as text inputs by checking each path element's role
attribute for "textbox" (and optionally "search" or other relevant ARIA roles)
and return true when found; locate the isFromTextInput( event: KeyboardEvent )
function and add a role check using element.getAttribute('role') === 'textbox'
(and consider aria-multiline handling) so hotkeys won't trigger while typing in
custom accessible text components.
In `@src/components/select/select.tsx`:
- Around line 417-431: The multiselect branch in the toggle code (inside the if
(this.multiple) block that computes currentValue/nextValue and calls
this.change.emit(nextValue)) skips the scroll-preserving logic used by
handleMenuChange(); update this branch to snapshot the menu surface scrollTop
(the same .mdc-menu-surface.scrollTop read used in handleMenuChange()), emit the
change while keeping the saved scroll position, then restore the scrollTop after
emitting so the open menu doesn't jump to the top; locate symbols matchedOption,
isMultiple, this.value, this.change.emit and reuse the exact snapshot/restore
calls used in handleMenuChange().
In `@src/util/hotkeys.spec.ts`:
- Around line 20-32: Add a unit test to verify full canonical modifier ordering
for hotkeyFromKeyboardEvent: create a case calling hotkeyFromKeyboardEvent with
all four modifier booleans true (metaKey, ctrlKey, altKey, shiftKey) and key
'a'/'KeyA' and assert the returned string equals 'meta+ctrl+alt+shift+a' so the
function's canonical ordering is explicitly validated.
In `@src/util/hotkeys.ts`:
- Around line 146-193: normalizeHotkeyString currently preserves the shift
modifier when the parsed non-modifier key is '+' which is inconsistent with
hotkeyFromKeyboardEvent (which drops shift for '+'), causing bindings like
"shift++" to never match; update normalizeHotkeyString so that after determining
key (the variable named key) you detect the literal '+' and set shift = false
(or reject the combo) before calling normalizeModifiersAndKey, mirroring
hotkeyFromKeyboardEvent’s handling; also apply the same change to the analogous
logic block used elsewhere in this file (the other normalizeHotkeyString-like
path around the second occurrence noted in the review).
---
Duplicate comments:
In `@src/components/hotkey/hotkey.tsx`:
- Around line 80-121: The switch handling normalized key aliases should render
aliases consistently: change the 'cmd'/'command' case so it returns the same
value as the 'meta' case (i.e., respect isApple and return the same object used
for 'meta' rather than hardcoding '⌘'), and change the delete/del/backspace
logic so on Apple platforms you do not collapse delete and backspace into the
same glyph — return '⌫' (backspace) when lower === 'backspace' and the
forward-delete glyph (e.g., '⌦') when lower === 'delete' (and keep the non-Apple
text branches for 'Backspace' vs 'Del'); update the switch cases around lower,
'meta', 'cmd', 'command', and 'delete'/'del'/'backspace' accordingly.
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 60-61: Replace the conflicting hotkey string 'meta+c' used for the
"Copy message" menu item with a non-conflicting accelerator; locate the menu
item object in menu-hotkeys.tsx (the entry with text: 'Copy message' and hotkey:
'meta+c') and change its hotkey to something that won’t override platform copy
(for example use a letters-only or modifier combination that isn’t reserved),
updating any associated hotkey handlers to match the new key string.
In `@src/components/select/examples/select-hotkeys.tsx`:
- Around line 64-69: The hotkey 'cmd+d' in the select option object (the entry
with text 'Closed', secondaryText 'as not planned', value 'closed', hotkey:
'cmd+d') conflicts with common browser shortcuts; update that object's hotkey
value to a non-conflicting combo such as 'cmd+shift+d' or 'alt+d' (i.e., modify
the hotkey property on the 'Closed' option to the chosen alternative).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7534572c-d3e4-4045-a561-a6a71a7ee435
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (20)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/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/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Comment on lines
+31
to
33
| &::first-letter { | ||
| text-transform: uppercase; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential style redundancy with hotkey.scss.
The &::first-letter { text-transform: uppercase; } rule here applies to kbd elements. However, hotkey.scss (which forwards this partial) also applies the same transform to span elements inside the hotkey component (lines 22-27).
Since limel-hotkey renders text inside <span> within <kbd>, the transform on kbd::first-letter may not have the intended effect (CSS ::first-letter applies to the first letter of the element's text content, but if the text is wrapped in a span, the kbd::first-letter won't match).
Consider whether this rule is intended for markdown kbd elements only, or if it should be removed since hotkey-specific styling handles it differently.
🤖 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 31 - 33, The
kbd::first-letter rule (&::first-letter { text-transform: uppercase; }) is
redundant or ineffective for limel-hotkey because its text is wrapped in a span;
decide whether to keep it for raw markdown kbd elements or remove/relocate it:
if the intent is to style only markdown kbd, restrict the selector to target kbd
elements that contain text (e.g., kbd:not(:has(> span)) or simply keep it but
document scope); if the intent is to style hotkey spans, remove this rule from
_kbd.scss and apply text-transform to the hotkey-specific selector in
hotkey.scss (e.g., .limel-hotkey span or the existing span selector) so the
first-letter transform actually affects rendered text.
Comment on lines
+345
to
+373
| private isFromTextInput(event: KeyboardEvent): boolean { | ||
| const path = | ||
| typeof event.composedPath === 'function' | ||
| ? event.composedPath() | ||
| : []; | ||
|
|
||
| for (const node of path) { | ||
| if (!('tagName' in (node as any))) { | ||
| continue; | ||
| } | ||
|
|
||
| const element = node as HTMLElement; | ||
|
|
||
| if (element.isContentEditable) { | ||
| return true; | ||
| } | ||
|
|
||
| const tagName = element.tagName; | ||
| if ( | ||
| tagName === 'INPUT' || | ||
| tagName === 'TEXTAREA' || | ||
| tagName === 'SELECT' | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding role="textbox" check for custom text inputs.
The isFromTextInput function checks for contentEditable and native input elements, but some custom components use role="textbox" for accessibility. This could cause hotkeys to fire while typing in such elements.
🔧 Suggested enhancement
if (element.isContentEditable) {
return true;
}
const tagName = element.tagName;
if (
tagName === 'INPUT' ||
tagName === 'TEXTAREA' ||
tagName === 'SELECT'
) {
return true;
}
+
+ if (element.getAttribute('role') === 'textbox') {
+ return true;
+ }🤖 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 345 - 373, The isFromTextInput
method currently only checks contentEditable and native INPUT/TEXTAREA/SELECT
elements; update it to also detect elements that act as text inputs by checking
each path element's role attribute for "textbox" (and optionally "search" or
other relevant ARIA roles) and return true when found; locate the
isFromTextInput( event: KeyboardEvent ) function and add a role check using
element.getAttribute('role') === 'textbox' (and consider aria-multiline
handling) so hotkeys won't trigger while typing in custom accessible text
components.
Comment on lines
+20
to
+32
| describe('hotkeyFromKeyboardEvent', () => { | ||
| it('includes multiple modifiers', () => { | ||
| const pressed = hotkeyFromKeyboardEvent({ | ||
| key: 'a', | ||
| code: 'KeyA', | ||
| altKey: true, | ||
| ctrlKey: true, | ||
| metaKey: false, | ||
| shiftKey: false, | ||
| } as any); | ||
|
|
||
| expect(pressed).toBe('ctrl+alt+a'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for full canonical modifier ordering.
The test at lines 21-32 verifies ctrl+alt ordering, but a past review suggested testing all four modifiers together to confirm the canonical order (meta+ctrl+alt+shift).
💡 Optional test addition
it('outputs modifiers in canonical order', () => {
const pressed = hotkeyFromKeyboardEvent({
key: 'a',
code: 'KeyA',
altKey: true,
ctrlKey: true,
metaKey: true,
shiftKey: true,
} as any);
expect(pressed).toBe('meta+ctrl+alt+shift+a');
});🤖 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 - 32, Add a unit test to verify
full canonical modifier ordering for hotkeyFromKeyboardEvent: create a case
calling hotkeyFromKeyboardEvent with all four modifier booleans true (metaKey,
ctrlKey, altKey, shiftKey) and key 'a'/'KeyA' and assert the returned string
equals 'meta+ctrl+alt+shift+a' so the function's canonical ordering is
explicitly validated.
Comment on lines
+146
to
+193
| export const normalizeHotkeyString = (hotkey: string): string | null => { | ||
| if (!hotkey) { | ||
| return null; | ||
| } | ||
|
|
||
| const tokens = tokenizeHotkeyString(hotkey); | ||
| if (tokens.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| let alt = false; | ||
| let ctrl = false; | ||
| let meta = false; | ||
| let shift = false; | ||
| let key: string | null = null; | ||
|
|
||
| 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 }); |
Contributor
There was a problem hiding this comment.
Canonicalize + the same way for strings and keyboard events.
hotkeyFromKeyboardEvent() intentionally strips shift when the key is +, but normalizeHotkeyString() preserves it. That makes bindings like shift++ normalize to shift++ even though the event side can only ever produce +, so the shortcut can never match. Drop shift for + on the string path as well, or reject that form.
♻️ Suggested fix
- return normalizeModifiersAndKey({ key, alt, ctrl, meta, shift });
+ return normalizeModifiersAndKey({
+ key,
+ alt,
+ ctrl,
+ meta,
+ shift: key === '+' ? false : shift,
+ });Also applies to: 223-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/hotkeys.ts` around lines 146 - 193, normalizeHotkeyString currently
preserves the shift modifier when the parsed non-modifier key is '+' which is
inconsistent with hotkeyFromKeyboardEvent (which drops shift for '+'), causing
bindings like "shift++" to never match; update normalizeHotkeyString so that
after determining key (the variable named key) you detect the literal '+' and
set shift = false (or reject the combo) before calling normalizeModifiersAndKey,
mirroring hotkeyFromKeyboardEvent’s handling; also apply the same change to the
analogous logic block used elsewhere in this file (the other
normalizeHotkeyString-like path around the second occurrence noted in the
review).
- 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.
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
2d151c9 to
6ff96ff
Compare
Kiarokh
force-pushed
the
menu-select-hotkeys
branch
from
f5df0e5 to
d8f1c47
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/components/menu-list/menu-list-renderer.tsx (1)
108-123: 🧹 Nitpick | 🔵 TrivialRemove redundant
anycasts in narrowedMenuItembranch.After the separator guard, these casts are unnecessary and reduce type safety/readability.
♻️ Proposed refactor
- const hasHotkey = 'hotkey' in item && !!(item as any).hotkey; + const hasHotkey = !!item.hotkey; const hasMeta = hasSubMenu || item.badge !== undefined || hasHotkey || - (!!('commandText' in item) && !!item.commandText); + !!item.commandText; const primaryComponent = hasMeta ? { name: 'limel-menu-item-meta', props: { - commandText: (item as any).commandText, - hotkey: (item as any).hotkey, + commandText: item.commandText, + hotkey: item.hotkey, disabled: !!item.disabled, - badge: (item as any).badge, + badge: item.badge, showChevron: hasSubMenu, }, } : undefined;#!/bin/bash # Verify the branch is narrowed by separator guard and locate remaining casts. rg -n -C3 "if \\('separator' in item\\)|as any\\)\\.(hotkey|commandText|badge)|'hotkey' in item|\\('commandText' in item\\)" --type=ts --type=tsxExpected result: separator guard appears before this block, and casted member access is confined to this code path.
🤖 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 - 123, The code is using redundant (item as any).<prop> casts inside the narrowed MenuItem branch (see hasHotkey, hasMeta and primaryComponent/limel-menu-item-meta props); remove these any casts and access properties directly (e.g., item.hotkey, item.commandText, item.badge) after the existing type-narrowing guards ('hotkey' in item / 'commandText' in item / separator guard) or cast item once to the correct MenuItem subtype if needed, and keep explicit boolean coercions only where required (e.g., disabled: !!item.disabled).src/components/select/examples/select-hotkeys.tsx (2)
42-42: 🧹 Nitpick | 🔵 TrivialMark class members as
readonly.Both
optionsandhandleChangeare never reassigned; marking them asreadonlysatisfies static analysis and communicates intent.🔧 Suggested fix
- private options: (Option | ListSeparator)[] = [ + private readonly options: (Option | ListSeparator)[] = [- private handleChange = (event: LimelSelectCustomEvent<Option>) => { + private readonly handleChange = (event: LimelSelectCustomEvent<Option>) => {Also applies to: 86-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/examples/select-hotkeys.tsx` at line 42, Mark the class fields that are never reassigned as readonly: add the readonly modifier to the options property and to the handleChange class field so their declarations become readonly (e.g., change "private options: (Option | ListSeparator)[] = [...]" to "private readonly options: ..." and likewise for the handleChange field), ensuring TypeScript static analysis and intent are preserved.
64-68:⚠️ Potential issue | 🟡 MinorReplace
cmd+dwith a non-conflicting hotkey.Line 31 explicitly warns to "Choose hotkeys that do not conflict with common browser shortcuts." However,
cmd+dconflicts with:
- macOS: Cmd+D bookmarks the page in all major browsers
- Windows: Win+D shows the desktop
Consider using
cmd+c(with text "Closed") or another non-conflicting combination likecmd+e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/examples/select-hotkeys.tsx` around lines 64 - 68, The selected hotkey 'cmd+d' in the example option object (the option with text "Closed", secondaryText "as not planned", value "closed" in select-hotkeys.tsx) conflicts with common browser shortcuts; change the hotkey property to a non-conflicting combination (e.g., 'cmd+c' or 'cmd+e') so it doesn't override browser actions—update the hotkey value on that option object accordingly.src/components/menu/examples/menu-hotkeys.tsx (1)
60-61:⚠️ Potential issue | 🟡 MinorReplace
meta+cwith a non-conflicting hotkey.Lines 33-37 warn about browser/OS conflicts, yet
meta+cmaps to Cmd+C (copy) on macOS and Win+C on Windows—both platform shortcuts. This undermines the example's purpose.Consider using
meta+shift+cor another combination that doesn't conflict with system clipboard operations.🤖 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 60 - 61, Update the hotkey for the menu item with text 'Copy message' to a non-conflicting combination (e.g., 'meta+shift+c') instead of 'meta+c'; locate the menu items array where the object { text: 'Copy message', hotkey: 'meta+c' } is defined and replace the hotkey string so it no longer collides with the system clipboard shortcut.src/components/hotkey/hotkey.tsx (1)
112-121: 🧹 Nitpick | 🔵 TrivialConsider distinguishing
deleteandbackspaceon Apple devices.On macOS, there are two distinct keys:
- Delete (⌫) — backspace, removes character before cursor
- Forward Delete (⌦) — removes character after cursor
Currently, all three aliases (
delete,del,backspace) render as ⌫ on Apple devices. If a user specifiesdeleteintending forward-delete behavior, the displayed glyph will be misleading.💡 Suggested fix
case 'delete': case 'del': case 'backspace': { if (isApple) { - return { display: '⌫', isGlyph: true }; + return lower === 'backspace' + ? { display: '⌫', isGlyph: true } + : { display: '⌦', isGlyph: true }; } return lower === 'backspace' ? { display: 'Backspace', isGlyph: false } : { display: 'Del', 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 112 - 121, The switch block handling 'delete'/'del'/'backspace' currently returns the same Apple glyph (⌫) for all three — update the logic in the hotkey rendering function (the switch/case in src/components/hotkey/hotkey.tsx) to distinguish forward-delete vs backspace: when isApple is true, return '⌦' (isGlyph: true) for 'delete' and 'del' (forward delete) and return '⌫' (isGlyph: true) only for 'backspace'; for non-Apple keep the existing string labels ('Backspace' for backspace, 'Del' for delete/del). Ensure you check the normalized key (lower) to decide which glyph to return.src/components/select/select.tsx (1)
417-431:⚠️ Potential issue | 🟠 MajorReuse the scroll-preserving multiselect path here.
handleMenuChange()already snapshots/restores.mdc-menu-surface.scrollTopbefore emitting in multiselect mode, but this hotkey branch still emits and returns directly. In a long open list, each toggle can jump the menu back to the top.♻️ Minimal fix
if (this.multiple) { + const selector = `#${this.portalId} limel-menu-surface`; + const menuSurface = document + .querySelector(selector) + ?.shadowRoot?.querySelector('.mdc-menu-surface'); + const scrollPosition = menuSurface?.scrollTop || 0; + const currentValue = isMultiple(this.value) ? this.value : []; const hasSelectedOption = currentValue.some( (option) => option.value === matchedOption.value ); @@ this.change.emit(nextValue); + + requestAnimationFrame(() => { + setTimeout(() => { + if (menuSurface) { + menuSurface.scrollTop = scrollPosition; + } + }); + }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/select/select.tsx` around lines 417 - 431, The hotkey branch in the multiselect path (inside the method handling key actions where it checks this.multiple and computes nextValue and calls this.change.emit(nextValue)) bypasses the scroll-preserving logic used in handleMenuChange(); fix it by snapshotting the menu surface scrollTop before emitting and restoring it after emit (reuse the same .mdc-menu-surface.scrollTop save/restore pattern or call the existing scroll-preserving helper used by handleMenuChange()), ensuring the matchedOption toggle (the computation of nextValue and the this.change.emit(nextValue) call) occurs between the snapshot and restore so the menu doesn't jump to the top when toggling via hotkey.src/util/hotkeys.ts (1)
156-193:⚠️ Potential issue | 🟡 MinorCanonicalize
+the same way on the string path.
hotkeyFromKeyboardEvent()already stripsshiftfor+, butnormalizeHotkeyString()still preserves it. A binding likeshift++normalizes toshift++even though the event side can only produce+, so the shortcut never matches.♻️ Minimal fix
- return normalizeModifiersAndKey({ key, alt, ctrl, meta, shift }); + return normalizeModifiersAndKey({ + key, + alt, + ctrl, + meta, + shift: key === '+' ? false : shift, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/hotkeys.ts` around lines 156 - 193, normalizeHotkeyString() currently preserves the shift modifier when the literal '+' is parsed from a string, causing mismatches with hotkeyFromKeyboardEvent() which canonicalizes '+' without shift; after you determine the final key in normalizeHotkeyString (the block that sets key and then returns normalizeModifiersAndKey({ key, alt, ctrl, meta, shift })), detect the canonical plus case (key === '+') and clear the shift flag before calling normalizeModifiersAndKey so string paths like "shift++" canonicalize the same way as hotkeyFromKeyboardEvent(); use the same helper functions normalizeKey, normalizeModifiersAndKey, and ensure behavior is symmetric with hotkeyFromKeyboardEvent().
🤖 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 279-287: connectedCallback currently only adds the keydown
listener when the component mounts with this.open true but doesn't call
setFocus(), leaving keyboard users outside the menu; update connectedCallback so
that when this.open is true it both adds the document keydown listener
(this.handleDocumentKeyDown) and calls this.setFocus() so focus is initialized
the same way openWatcher() does.
- Around line 403-420: findMenuItemByHotkey walks this.visibleItems even when
the menu is showing a loader, allowing hotkeys to activate items that are not
rendered; update the function (findMenuItemByHotkey) to early-return null when
the menu is in a loading state by checking the component flags (this.loading or
this.loadingSubItems) before iterating visibleItems, so no hotkey lookup occurs
while renderMenuList intentionally hides items.
---
Duplicate comments:
In `@src/components/hotkey/hotkey.tsx`:
- Around line 112-121: The switch block handling 'delete'/'del'/'backspace'
currently returns the same Apple glyph (⌫) for all three — update the logic in
the hotkey rendering function (the switch/case in
src/components/hotkey/hotkey.tsx) to distinguish forward-delete vs backspace:
when isApple is true, return '⌦' (isGlyph: true) for 'delete' and 'del' (forward
delete) and return '⌫' (isGlyph: true) only for 'backspace'; for non-Apple keep
the existing string labels ('Backspace' for backspace, 'Del' for delete/del).
Ensure you check the normalized key (lower) to decide which glyph to return.
In `@src/components/menu-list/menu-list-renderer.tsx`:
- Around line 108-123: The code is using redundant (item as any).<prop> casts
inside the narrowed MenuItem branch (see hasHotkey, hasMeta and
primaryComponent/limel-menu-item-meta props); remove these any casts and access
properties directly (e.g., item.hotkey, item.commandText, item.badge) after the
existing type-narrowing guards ('hotkey' in item / 'commandText' in item /
separator guard) or cast item once to the correct MenuItem subtype if needed,
and keep explicit boolean coercions only where required (e.g., disabled:
!!item.disabled).
In `@src/components/menu/examples/menu-hotkeys.tsx`:
- Around line 60-61: Update the hotkey for the menu item with text 'Copy
message' to a non-conflicting combination (e.g., 'meta+shift+c') instead of
'meta+c'; locate the menu items array where the object { text: 'Copy message',
hotkey: 'meta+c' } is defined and replace the hotkey string so it no longer
collides with the system clipboard shortcut.
In `@src/components/select/examples/select-hotkeys.tsx`:
- Line 42: Mark the class fields that are never reassigned as readonly: add the
readonly modifier to the options property and to the handleChange class field so
their declarations become readonly (e.g., change "private options: (Option |
ListSeparator)[] = [...]" to "private readonly options: ..." and likewise for
the handleChange field), ensuring TypeScript static analysis and intent are
preserved.
- Around line 64-68: The selected hotkey 'cmd+d' in the example option object
(the option with text "Closed", secondaryText "as not planned", value "closed"
in select-hotkeys.tsx) conflicts with common browser shortcuts; change the
hotkey property to a non-conflicting combination (e.g., 'cmd+c' or 'cmd+e') so
it doesn't override browser actions—update the hotkey value on that option
object accordingly.
In `@src/components/select/select.tsx`:
- Around line 417-431: The hotkey branch in the multiselect path (inside the
method handling key actions where it checks this.multiple and computes nextValue
and calls this.change.emit(nextValue)) bypasses the scroll-preserving logic used
in handleMenuChange(); fix it by snapshotting the menu surface scrollTop before
emitting and restoring it after emit (reuse the same .mdc-menu-surface.scrollTop
save/restore pattern or call the existing scroll-preserving helper used by
handleMenuChange()), ensuring the matchedOption toggle (the computation of
nextValue and the this.change.emit(nextValue) call) occurs between the snapshot
and restore so the menu doesn't jump to the top when toggling via hotkey.
In `@src/util/hotkeys.ts`:
- Around line 156-193: normalizeHotkeyString() currently preserves the shift
modifier when the literal '+' is parsed from a string, causing mismatches with
hotkeyFromKeyboardEvent() which canonicalizes '+' without shift; after you
determine the final key in normalizeHotkeyString (the block that sets key and
then returns normalizeModifiersAndKey({ key, alt, ctrl, meta, shift })), detect
the canonical plus case (key === '+') and clear the shift flag before calling
normalizeModifiersAndKey so string paths like "shift++" canonicalize the same
way as hotkeyFromKeyboardEvent(); use the same helper functions normalizeKey,
normalizeModifiersAndKey, and ensure behavior is symmetric with
hotkeyFromKeyboardEvent().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 815f203d-be67-49fa-8e5b-05d0b42755ba
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (20)
src/components/hotkey/examples/hotkey-basic.scsssrc/components/hotkey/examples/hotkey-basic.tsxsrc/components/hotkey/examples/hotkey-disabled.tsxsrc/components/hotkey/hotkey.scsssrc/components/hotkey/hotkey.tsxsrc/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/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsxsrc/util/device.tssrc/util/hotkeys.spec.tssrc/util/hotkeys.ts
Comment on lines
+279
to
+287
| public connectedCallback() { | ||
| if (this.open) { | ||
| document.addEventListener( | ||
| 'keydown', | ||
| this.handleDocumentKeyDown, | ||
| true | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Call setFocus() for initially open menus too.
This branch mirrors the listener setup from openWatcher(), but not its focus handling. If the component mounts with open={true}, keyboard users start outside the menu until they tab in manually.
♻️ Minimal fix
public connectedCallback() {
if (this.open) {
document.addEventListener(
'keydown',
this.handleDocumentKeyDown,
true
);
+ this.setFocus();
}
}🤖 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 279 - 287, connectedCallback
currently only adds the keydown listener when the component mounts with
this.open true but doesn't call setFocus(), leaving keyboard users outside the
menu; update connectedCallback so that when this.open is true it both adds the
document keydown listener (this.handleDocumentKeyDown) and calls this.setFocus()
so focus is initialized the same way openWatcher() does.
Comment on lines
+403
to
+420
| private findMenuItemByHotkey(pressedHotkey: string): MenuItem | null { | ||
| for (const item of this.visibleItems) { | ||
| if (!this.isMenuItem(item) || item.disabled) { | ||
| continue; | ||
| } | ||
|
|
||
| const rawHotkey = item.hotkey; | ||
| if (!rawHotkey) { | ||
| continue; | ||
| } | ||
|
|
||
| const normalized = this.getNormalizedHotkey(rawHotkey); | ||
| if (normalized && normalized === pressedHotkey) { | ||
| return item; | ||
| } | ||
| } | ||
|
|
||
| return null; |
Contributor
There was a problem hiding this comment.
Ignore hotkeys while the menu is showing its loader.
renderMenuList() intentionally renders no items while loadingSubItems or loading is true, but this lookup still walks visibleItems. During async search or lazy submenu loading, a quick hotkey can therefore select a stale item that is no longer rendered.
♻️ Minimal fix
private findMenuItemByHotkey(pressedHotkey: string): MenuItem | null {
+ if (this.loadingSubItems || this.loading) {
+ return null;
+ }
+
for (const item of this.visibleItems) {
if (!this.isMenuItem(item) || item.disabled) {
continue;
}🤖 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 403 - 420, findMenuItemByHotkey
walks this.visibleItems even when the menu is showing a loader, allowing hotkeys
to activate items that are not rendered; update the function
(findMenuItemByHotkey) to early-return null when the menu is in a loading state
by checking the component flags (this.loading or this.loadingSubItems) before
iterating visibleItems, so no hotkey lookup occurs while renderMenuList
intentionally hides items.
This was referenced Mar 25, 2026
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix https://github.com/Lundalogik/crm-client/issues/754
Summary by CodeRabbit
New Features
Style
Examples & Documentation
Tests
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: