fix: Prevent command center shortcuts while typing in selects #2583
Conversation
Console (appwrite/console)Project ID: Tip Git integration provides automatic deployments with optional PR comments |
WalkthroughThe PR removes a local input-detection helper in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-06T06:47:24.645ZApplied to files:
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/commandCenter/commands.ts (2)
106-125: Consider simplifying by relying onclosestalone.The function performs redundant checks: lines 110-114 test the element directly, but
closest(lines 116-122) already checks the element itself before traversing ancestors. The entire function could be streamlined to just theclosestcall with appropriate selectors.Additionally, line 107's cast
(element as Element) ?? nullis misleading—the cast executes regardless of whetherelementis null, making the?? nullineffective.Consider this simplified approach:
function isTargetInputLike(element: EventTarget | null) { - const el = (element as Element) ?? null; - if (!(el instanceof HTMLElement)) return false; - - if (INPUT_TAGS.includes(el.tagName)) return true; - if (el.isContentEditable) return true; - - const role = el.getAttribute('role'); - if (role && INPUT_ROLES.includes(role.toLowerCase())) return true; - - if ( - el.closest( - 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]' - ) - ) { - return true; - } - - return false; + if (!(element instanceof HTMLElement)) return false; + + return !!element.closest( + 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]' + ); }
116-122: CSS selector case-sensitivity is theoretically valid but not an issue in practice.Verification found zero uppercase or mixed-case role attributes (e.g.,
role="Combobox",role="TextBox") anywhere in the codebase. All role attributes are lowercase. The code at line 114 already handles case-insensitive role matching via.toLowerCase()for direct attribute checks, and allclosest()selectors safely use lowercase values matching the actual attributes present.The suggestion to add the case-insensitive flag
ito the CSS selectors is a valid defensive measure for future-proofing, but it addresses a theoretical concern rather than an actual bug. The current implementation works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/commandCenter/commandCenter.svelte(2 hunks)src/lib/commandCenter/commands.ts(2 hunks)src/lib/elements/forms/inputSelect.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2567
File: src/lib/elements/forms/inputSelect.svelte:26-36
Timestamp: 2025-11-06T06:47:24.645Z
Learning: In the appwrite.io/pink-svelte library, the Input.Select component internally uses an `input` element (not a `select` element) that triggers the `on:input` event, so event.target should be cast as HTMLInputElement in event handlers.
📚 Learning: 2025-11-06T06:47:24.645Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2567
File: src/lib/elements/forms/inputSelect.svelte:26-36
Timestamp: 2025-11-06T06:47:24.645Z
Learning: In the appwrite.io/pink-svelte library, the Input.Select component internally uses an `input` element (not a `select` element) that triggers the `on:input` event, so event.target should be cast as HTMLInputElement in event handlers.
Applied to files:
src/lib/commandCenter/commands.tssrc/lib/elements/forms/inputSelect.sveltesrc/lib/commandCenter/commandCenter.svelte
📚 Learning: 2025-09-25T04:23:14.025Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/input.svelte:20-24
Timestamp: 2025-09-25T04:23:14.025Z
Learning: The Selector.Switch component from appwrite.io/pink-svelte doesn't accept `aria-describedby` or rest props (`...restProps`), limiting accessibility customization options.
Applied to files:
src/lib/elements/forms/inputSelect.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (6)
src/lib/elements/forms/inputSelect.svelte (1)
59-59: LGTM! The attribute integrates correctly with the detection logic.The
data-command-center-ignoreattribute will be detected by theclosestselector inisTargetInputLike(line 118 of commands.ts), ensuring keyboard shortcuts are suppressed when typing in this select component.src/lib/commandCenter/commands.ts (3)
103-104: LGTM! Constants are well-defined.The INPUT_TAGS and INPUT_ROLES constants provide a clear, maintainable list of elements and ARIA roles that should be treated as input-like.
127-129: LGTM! Clean public API for input detection.The exported function provides a clear interface for checking whether keyboard events originate from input-like elements.
231-239: LGTM! Correctly integrated into the command handler.The
isKeyboardEventFromInput(event)check is properly placed in the conditional to skip command processing when typing in input fields.src/lib/commandCenter/commandCenter.svelte (2)
33-38: LGTM! Import correctly updated.The
isKeyboardEventFromInputfunction is properly imported from the centralized commands module.
103-110: LGTM! Correctly prevents shortcuts during input.The guard clause at line 105 properly uses
isKeyboardEventFromInput(e)to skip keyboard event processing when typing in input-like elements, preventing command center shortcuts from interfering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/commandCenter/commands.ts (1)
214-219: Integration looks correct.The
isKeyboardEventFromInput(event)check is properly integrated into the condition chain and will effectively prevent command shortcuts when typing in input-like elements.Optional performance consideration: The
isKeyboardEventFromInputfunction callsclosest()on every keydown event, which traverses the DOM tree. While this is a reasonable trade-off for correctness, if performance becomes a concern under heavy keyboard input, consider memoizing results based onevent.targetor exploring event delegation patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/commandCenter/commands.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2567
File: src/lib/elements/forms/inputSelect.svelte:26-36
Timestamp: 2025-11-06T06:47:24.645Z
Learning: In the appwrite.io/pink-svelte library, the Input.Select component internally uses an `input` element (not a `select` element) that triggers the `on:input` event, so event.target should be cast as HTMLInputElement in event handlers.
🔇 Additional comments (1)
src/lib/commandCenter/commands.ts (1)
110-112: LGTM! Clean abstraction.The function provides a clean public API for keyboard event input detection with clear naming and proper delegation.
src/lib/commandCenter/commands.ts
Outdated
| function isTargetInputLike(element: EventTarget | null) { | ||
| if (!(element instanceof HTMLElement)) return false; | ||
| return !!element.closest( | ||
| 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix contenteditable selector to exclude false values.
The selector [contenteditable] matches any element with the contenteditable attribute, including contenteditable="false", which explicitly marks an element as non-editable. This could incorrectly suppress shortcuts for non-editable elements.
Apply this diff to match only editable elements:
- return !!element.closest(
- 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]'
- );
+ return !!element.closest(
+ 'input,textarea,select,[contenteditable="true"],[contenteditable=""],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]'
+ );Or alternatively, check the attribute value programmatically:
function isTargetInputLike(element: EventTarget | null) {
if (!(element instanceof HTMLElement)) return false;
- return !!element.closest(
- 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]'
- );
+ const match = element.closest(
+ 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]'
+ );
+ if (!match) return false;
+ if (match.hasAttribute('contenteditable') && match.getAttribute('contenteditable') === 'false') {
+ return false;
+ }
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isTargetInputLike(element: EventTarget | null) { | |
| if (!(element instanceof HTMLElement)) return false; | |
| return !!element.closest( | |
| 'input,textarea,select,[contenteditable],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]' | |
| ); | |
| } | |
| function isTargetInputLike(element: EventTarget | null) { | |
| if (!(element instanceof HTMLElement)) return false; | |
| return !!element.closest( | |
| 'input,textarea,select,[contenteditable="true"],[contenteditable=""],[role="combobox"],[role="textbox"],[role="searchbox"],[data-command-center-ignore]' | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/lib/commandCenter/commands.ts around lines 103 to 108, the CSS selector
'[contenteditable]' incorrectly matches elements with contenteditable="false";
update the check so only truly editable elements are treated as inputs. Replace
the selector to explicitly match editable values (e.g. use
'[contenteditable=""],[contenteditable="true"]' in the selector list) or modify
the function to programmatically inspect element.getAttribute('contenteditable')
and return false when the attribute value === 'false' and true for '', 'true' or
when attribute is absent but element is actually editable; ensure the final
logic still returns true for inputs, textareas, selects,
contenteditable=true/empty, role=textbox/combobox/searchbox and elements with
data-command-center-ignore.
src/lib/commandCenter/commands.ts
Outdated
| } | ||
|
|
||
| export function isKeyboardEventFromInput(event: KeyboardEvent) { | ||
| return isTargetInputLike(event.target); |
There was a problem hiding this comment.
why not just export isTargetInputLike?
There was a problem hiding this comment.
would this not affect every page with input selects and not just wizards?
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit