refactor: migrated command palette component to svelte5 #13628
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe CommandPalette.svelte component is migrated to Svelte 5 patterns: the exported prop export let display is replaced by a Props interface and let { display = false }: Props = $props(); local variables are converted to $state(...) (inputElement, outerDiv, inputValue, scrollElements, commandInfoItems, selectedFilteredIndex), and a reactive filter is replaced by a $derived store for filteredCommandInfoItems. Lifecycle hooks (onMount/onDestroy), keyboard show/hide handling (F1/ESCAPE), adjustments to selection/scroll behavior, and template event binding changes (oninput/onclick) plus aria-label additions are included. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the CommandPalette component from Svelte 4 to Svelte 5 syntax, updating reactive declarations and event handlers to use the new Svelte 5 patterns.
- Updates component props to use the new
$props()syntax with TypeScript interfaces - Converts reactive variables to use
$state()and$derived()runes - Updates event handlers from
on:eventtooneventsyntax
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let selectedFilteredIndex = 0; | ||
| let selectedFilteredIndex = $state(0); | ||
| let selectedIndex = 0; |
There was a problem hiding this comment.
The selectedIndex variable should also be migrated to use $state() for consistency with other reactive variables in the Svelte 5 migration. It appears to be used reactively based on the context.
| let selectedIndex = 0; | |
| let selectedIndex = $state(0); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
193-197: Fix event bindings: useon:clickinstead ofonclickSame issue as above —
onclickwill not set up a Svelte event listener and the click handler will not run.Apply this diff:
- <button - onclick={(): Promise<void> => clickOnItem(i)} + <button + on:click={() => clickOnItem(i)}
🧹 Nitpick comments (4)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (4)
29-29: Align type with runtime:globalContextstarts undefinedAt initialization,
globalContextis undefined until the store subscription runs. Widen the type to reflect reality and avoid misleading the compiler and future readers.Apply this diff:
-let globalContext: ContextUI; +let globalContext: ContextUI | undefined;
23-25: Prefer plain refs (no$state) for element bindings
bind:thisdoesn't need$stateand using it here adds unnecessary invalidation. Use plain nullable refs; keep$statefor actual reactive state likeinputValue.Apply this diff:
-let inputElement: HTMLInputElement | HTMLTextAreaElement | undefined = $state(undefined); -let outerDiv: HTMLDivElement | undefined = $state(undefined); -let inputValue: string | undefined = $state(''); +let inputElement: HTMLInputElement | HTMLTextAreaElement | null = null; +let outerDiv: HTMLDivElement | null = null; +let inputValue: string | undefined = $state('');
160-160: Remove unnecessaryasync/Promise<void>
onInputChangedoesn't await anything. Dropasyncand the promise return type.Apply this diff:
-async function onInputChange(): Promise<void> { +function onInputChange(): void {
56-80: Comment nit: it's not a true toggle on F1Minor mismatch — the comment says “toggle display using F1 or ESC” but F1 unconditionally sets
display = true. If intended, adjust the comment to avoid confusion.Apply this diff:
- // toggle display using F1 or ESC keys + // open with F1, close with ESC
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte(3 hunks)
⏰ 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). (11)
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: build website
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (development)
- GitHub Check: macOS
- GitHub Check: unit tests / windows-2025
- GitHub Check: Linux
- GitHub Check: smoke e2e tests (production)
- GitHub Check: Windows
🔇 Additional comments (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (2)
53-53: Good use of$statefor selection indexThis state participates in keyboard navigation and is appropriately reactive.
18-23: Missingbind:displayon CommandPalette usageIt looks like
CommandPaletteis only rendered inpackages/renderer/src/App.svelte(line 133) without abind:display, so itsdisplayprop will always stay at the default (false). Please verify whether:
- A parent should be binding
display(e.g.bind:display={isPaletteVisible})- Or the component is expected to manage its own visibility internally
If two-way binding is intended, update the call site accordingly:
packages/renderer/src/App.svelte - <CommandPalette /> + <CommandPalette bind:display={isPaletteVisible} />
| let filteredCommandInfoItems: CommandInfo[] = $derived( | ||
| commandInfoItems | ||
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | ||
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against uninitialized globalContext in derived filter
globalContext is assigned asynchronously via the context store. If commandsInfos emits first, isPropertyValidInContext(..., globalContext) may receive undefined, risking a runtime error depending on its implementation. Add a guard so filtering degrades gracefully until context arrives.
Apply this diff:
let filteredCommandInfoItems: CommandInfo[] = $derived(
commandInfoItems
- .filter(property => isPropertyValidInContext(property.enablement, globalContext))
+ .filter(property =>
+ globalContext
+ ? isPropertyValidInContext(property.enablement, globalContext)
+ : true,
+ )
.filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : 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.
| let filteredCommandInfoItems: CommandInfo[] = $derived( | |
| commandInfoItems | |
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | |
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)), | |
| ); | |
| let filteredCommandInfoItems: CommandInfo[] = $derived( | |
| commandInfoItems | |
| .filter(property => | |
| globalContext | |
| ? isPropertyValidInContext(property.enablement, globalContext) | |
| : true, | |
| ) | |
| .filter(item => | |
| inputValue | |
| ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) | |
| : true, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 31–35,
the derived filter calls isPropertyValidInContext(..., globalContext) but
globalContext is populated asynchronously and may be undefined when commands
emit; change the filter to guard against an uninitialized context by
short-circuiting (e.g., if globalContext is falsy return true so items are not
filtered out until context is available) then call isPropertyValidInContext only
when globalContext is defined; update the derived expression accordingly so
filtering degrades gracefully until context arrives.
| oninput={onInputChange} | ||
| class="px-1 w-full text-[var(--pd-input-field-focused-text)] bg-[var(--pd-input-field-focused-bg)] border border-[var(--pd-input-field-stroke)] focus:outline-hidden" /> |
There was a problem hiding this comment.
Fix event bindings: use Svelte's on: directive, not DOM on* attributes
oninput={...} won't attach a listener in Svelte templates; it will set an element property/attribute instead. This breaks the input filtering. Use on:input.
Apply this diff:
- oninput={onInputChange}
+ on:input={onInputChange}📝 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.
| oninput={onInputChange} | |
| class="px-1 w-full text-[var(--pd-input-field-focused-text)] bg-[var(--pd-input-field-focused-bg)] border border-[var(--pd-input-field-stroke)] focus:outline-hidden" /> | |
| on:input={onInputChange} | |
| class="px-1 w-full text-[var(--pd-input-field-focused-text)] bg-[var(--pd-input-field-focused-bg)] border border-[var(--pd-input-field-stroke)] focus:outline-hidden" /> |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 186-187,
the input uses a DOM attribute binding "oninput={onInputChange}" which does not
register an event listener in Svelte; replace it with Svelte's event directive
"on:input={onInputChange}" (and similarly convert any other DOM on* attributes
in this file to on:* directives) so the input filtering handler is actually
attached.
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
c09e628 to
88d864c
Compare
|
@jeffmaury @axel7083 I've switched 2 other props to state which caused tests to fail |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (3)
53-55:selectedIndexdoesn’t need to be reactive; optional consistency tweak.It isn’t used in the template or any
$derived, so leaving it as a plain number is fine. If you prefer consistency with other runes-backed state, convert it to$state.-let selectedFilteredIndex = $state(0); -let selectedIndex = 0; +let selectedFilteredIndex = $state(0); +let selectedIndex = $state(0);
186-186: Use Svelte’son:directive for events;oninputwon’t attach a listener.This breaks input filtering.
Apply this diff:
- oninput={onInputChange} + on:input={onInputChange}
193-195: Useon:clicknotonclickin Svelte templates.
onclickwon’t register an event handler.Apply this diff:
- <button - onclick={(): Promise<void> => clickOnItem(i)} - aria-label={item.title} + <button + on:click={(): Promise<void> => clickOnItem(i)} + aria-label={item.title}
🧹 Nitpick comments (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (2)
101-106: GuardscrollIntoViewto avoid rare undefined access.If the list re-renders between key events,
scrollElements[selectedFilteredIndex]can transiently be undefined.Apply this diff:
- scrollElements[selectedFilteredIndex].scrollIntoView({ + scrollElements[selectedFilteredIndex]?.scrollIntoView({ behavior: 'smooth', block: 'nearest', inline: 'start', });Repeat the same change for the second call below.
Also applies to: 114-119
25-25: NarrowinputValuetostring(binding guarantees a string).Keeps types aligned with
bind:valueand simplifies downstream checks.-let inputValue: string | undefined = $state(''); +let inputValue: string = $state('');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte(3 hunks)
⏰ 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). (11)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: linter, formatters
- GitHub Check: macOS
- GitHub Check: typecheck
- GitHub Check: build website
- GitHub Check: Linux
| let { display = false }: Props = $props(); | ||
| let inputElement: HTMLInputElement | HTMLTextAreaElement | undefined = $state(undefined); |
There was a problem hiding this comment.
Make display bindable in Svelte 5 (props are read-only by default).
You mutate display in multiple places (e.g., Line 64, Line 77, Line 122, Line 147, Line 153). In Svelte 5, props are immutable unless declared with $bindable. Without this, assignments will fail/throw.
Apply this diff:
-let { display = false }: Props = $props();
+let { display = $bindable(false) }: Props = $props();📝 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.
| let { display = false }: Props = $props(); | |
| let inputElement: HTMLInputElement | HTMLTextAreaElement | undefined = $state(undefined); | |
| let { display = $bindable(false) }: Props = $props(); | |
| let inputElement: HTMLInputElement | HTMLTextAreaElement | undefined = $state(undefined); |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 22–23,
the component destructures the prop "display" from Props but then mutates it in
multiple places (lines ~64, 77, 122, 147, 153); in Svelte 5 props are read-only
by default so these assignments will throw. Make "display" bindable by declaring
it with $bindable (e.g., $bindable let { display = false }: Props) or equivalent
Svelte 5 syntax, keep the rest of the code that assigns to display unchanged,
and update any type annotations if necessary so the component can legally mutate
display via the bindable prop. Ensure you only change the prop declaration line
and do not remove existing mutations elsewhere.
| let globalContext: ContextUI; | ||
| $: filteredCommandInfoItems = commandInfoItems | ||
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | ||
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)); | ||
| let filteredCommandInfoItems: CommandInfo[] = $derived( | ||
| commandInfoItems | ||
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | ||
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)), | ||
| ); |
There was a problem hiding this comment.
Fix two issues in filtering: non-reactive globalContext + unsafe optional chaining on title.
globalContextis a plain variable; assigning to it in the store subscription won’t invalidate this$derived. Make it$state(...)so changes trigger recomputation.item.title?.toLowerCase().includes(...)will throw whentitleis undefined because.includesis still called onundefined. Coalesce to an empty string or add another optional chain.
Apply this diff:
-let globalContext: ContextUI;
+let globalContext: ContextUI | undefined = $state(undefined);
let filteredCommandInfoItems: CommandInfo[] = $derived(
commandInfoItems
- .filter(property => isPropertyValidInContext(property.enablement, globalContext))
- .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)),
+ .filter(property =>
+ globalContext ? isPropertyValidInContext(property.enablement, globalContext) : true,
+ )
+ .filter(item =>
+ inputValue
+ ? (item.title?.toLowerCase() ?? '').includes(inputValue.toLowerCase())
+ : 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.
| let globalContext: ContextUI; | |
| $: filteredCommandInfoItems = commandInfoItems | |
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | |
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)); | |
| let filteredCommandInfoItems: CommandInfo[] = $derived( | |
| commandInfoItems | |
| .filter(property => isPropertyValidInContext(property.enablement, globalContext)) | |
| .filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)), | |
| ); | |
| let globalContext: ContextUI | undefined = $state(undefined); | |
| let filteredCommandInfoItems: CommandInfo[] = $derived( | |
| commandInfoItems | |
| .filter(property => | |
| globalContext ? isPropertyValidInContext(property.enablement, globalContext) : true, | |
| ) | |
| .filter(item => | |
| inputValue | |
| ? (item.title?.toLowerCase() ?? '').includes(inputValue.toLowerCase()) | |
| : true, | |
| ), | |
| ); |
🤖 Prompt for AI Agents
packages/renderer/src/lib/dialogs/CommandPalette.svelte around lines 29-35:
globalContext is a plain variable so updates won’t retrigger the $derived store,
and the title check uses unsafe optional chaining leading to calling .includes
on undefined; make globalContext a reactive state/store (e.g., change its
declaration to a Svelte reactive state/store so assignments from subscriptions
invalidate the derived store) and update the filter to coalesce title to an
empty string before calling toLowerCase/includes (e.g., use (item.title ?? '')
or similar) so .includes is never invoked on undefined.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
What does this PR do?
Migrates command palette to svelte5
Screenshot / video of UI
What issues does this PR fix or reference?
Related to #13627
How to test this PR?
Unit tests