Open
Conversation
Contributor
📝 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: Presses key combo
Document->>HotkeyUtil: hotkeyFromKeyboardEvent(event)
HotkeyUtil-->>Document: normalized hotkey string
Document->>Menu: findMenuItemByHotkey(normalized)
Menu->>Menu: getNormalizedHotkey(item.hotkey) (cache)
alt match found
Menu->>Item: trigger selection (handleSelect)
Item-->>Menu: emit 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()
HotkeyUtil-->>HotkeyCache: normalized value
HotkeyCache-->>Select: store & return
else cache hit
HotkeyCache-->>Select: return cached
end
Select->>Select: on document keydown -> findOptionByHotkey -> select
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
- 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
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.
Comment on lines
230
to
+244
| @Watch('menuOpen') | ||
| protected watchOpen(newValue: boolean, oldValue: boolean) { | ||
| if (newValue) { | ||
| document.addEventListener( | ||
| 'keydown', | ||
| this.handleDocumentKeyDown, | ||
| true | ||
| ); | ||
| } else { | ||
| document.removeEventListener( | ||
| 'keydown', | ||
| this.handleDocumentKeyDown, | ||
| true | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding listener in connectedCallback when menuOpen is initially true.
Unlike menu.tsx (see src/components/menu/menu.tsx:279-312) which adds the document listener in both connectedCallback AND @Watch('open'), this component only uses @Watch('menuOpen'). If the component mounts with menuOpen=true (edge case), the listener won't be attached until the value changes.
♻️ Proposed fix to match menu.tsx pattern
public connectedCallback() {
this.initialize();
+ if (this.menuOpen) {
+ document.addEventListener(
+ 'keydown',
+ this.handleDocumentKeyDown,
+ true
+ );
+ }
}🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix https://github.com/Lundalogik/crm-client/issues/754
Summary by CodeRabbit
New Features
Style
Tests
Documentation
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: