feat: adds tabs to global searchbar #13629
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. 📝 WalkthroughWalkthroughReworks CommandPalette.svelte to add a 4-mode tabbed search (All, Commands, Documentation, Go to), platform-aware shortcut chips, keyboard shortcuts to open/preselect modes (F1, >, Ctrl/Cmd+K, Ctrl/Cmd+F), Tab/Shift+Tab cycling through tabs, and updated input/empty-state UI including a NotFoundIcon. Adds NotFoundLightIcon and NotFoundDarkIcon components and a theme-aware NotFoundIcon wrapper. Adds platform detection on mount and minor focus/scroll/try-catch behavior changes. Tests updated to cover opening shortcuts and tab cycling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
✨ 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 (
|
|
Hey @gastoner, I have some feedback to share as well. 
|
|
Also, please consider a good use of keyboard shortcuts, particularly |
702675d to
b3a48f3
Compare
|
@MariaLeonova we probably dont, I was just feeling that it would be good to differentiate |
My thought was that for commands it's something that they type that shows up in the search bar (i.e. > symbol). And for other tabs it's not, there it's just searching. Helper text would be more beneficial in this case, especially in the case of searching across Podman Desktop. We can't really use the word resources there, so the bar could let them know what the searchable assets are. |
b3a48f3 to
6dce772
Compare
There was a problem hiding this comment.
Looks good.
- When the panel is started with the
F1OR>keyboard shortcuts, it should go straight to the Commands view. - When the panel is started with the
⌘K/CTRL + Kkeyboard shortcuts, it should go straight to the Documentations view. - When the panel is started with the
⌘F/CTRL + Fkeyboard shortcuts, it should go straight to the Go to view.
6dce772 to
2f07923
Compare
|
@MariaLeonova @vancura also I assume I should add |
Yes, and it should be in the mocks, as well. I think it's here: #13629 (review) Also at some point we discussed not duplicating the shortcuts in the bottom bar, so there (if needed) we only have the mention of Algolia. So iy would look like this: The latest tab designs can be seen here. |
2a40e1b to
88f3295
Compare
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
88f3295 to
c55e103
Compare
| return; | ||
| } else if (e.ctrlKey || e.metaKey) { | ||
| if (e.shiftKey && e.key.toLowerCase() === 'p') { | ||
| searchOptionsSelectedIndex = 0; |
There was a problem hiding this comment.
I don't see prevent default call for these new shortcuts ?
| let modifierS: string = $derived(isMac ? '⇧' : 'Shift+'); | ||
| let searchOptions: SearchOption[] = $derived([ | ||
| { text: 'All', shortCut: [`${modifierC}${modifierS}P`] }, | ||
| { text: 'Commands', shortCut: ['F1', '>'] }, |
There was a problem hiding this comment.
F1 is used at several places no in the code ( and others)
Probably need to be a constant to avoid any further change that will only change one path
|
|
||
| {#if filteredCommandInfoItems.length === 0} | ||
| <div class='flex grow items-center flex-col gap-2 py-4'> | ||
| <Icon icon={isDarkTheme ? NotFoundDarkIcon : NotFoundLightIcon} /> |
There was a problem hiding this comment.
should it be a single icon object with a theme property ?
b/c I don't see in which case we would use just one icon file
There was a problem hiding this comment.
I was asking the same thing, but those are unique (the eyes are in both cases black as they should be)
There was a problem hiding this comment.
yes but a single NotFoundIcon could use the isDark property automatically no ?
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (2)
68-72: Wire tabs to actual filtering (meet pre-filtering objective).The computed list ignores the selected tab. Implement mode-aware filtering; guard for unset context.
-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.by(() => { + const q = (inputValue ?? '').toLowerCase(); + const ctx = globalContext; + const base = commandInfoItems + .filter(p => !ctx || isPropertyValidInContext(p.enablement, ctx)); + + switch (searchOptionsSelectedIndex) { + case 0: // All + return base.filter(i => (q ? i.title?.toLowerCase().includes(q) : true)); + case 1: // Commands + // For now commands == all items; keep same filter until other sources are added. + return base.filter(i => (q ? i.title?.toLowerCase().includes(q) : true)); + case 2: // Documentation + // TODO: hook docs source (Algolia/etc.) + return []; + case 3: // Go to + // TODO: hook navigation targets + return []; + default: + return base; + } +});
159-162: Tab switching is disabled when there are no results. Move Tab handling before the “no items” early-return.This breaks keyboard tab navigation in empty states.
// no items, abort - if (filteredCommandInfoItems.length === 0) { - return; - } + if (e.key === TAB_KEY) { + switchSearchOption(e.shiftKey ? -1 : 1); + e.preventDefault(); + return; + } + if (filteredCommandInfoItems.length === 0) { + return; + } @@ - } else if (e.key === TAB_KEY) { - switchSearchOption(e.shiftKey ? -1 : 1); - e.preventDefault(); - } + }Also applies to: 199-202
♻️ Duplicate comments (6)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (6)
303-305: Match design: increase chip horizontal padding.Design feedback asked for px-2 to align with footer chips.
- <div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-0.5 shadow-sm shadow-b-1'> + <div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-2 py-0.5 shadow-sm shadow-b-1'>
331-338: Placeholder action: add TODO or wire “Variables” to actual behavior.Right now it only logs; clarify intent.
- <Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button> + <Button icon={faChevronRight} type='link' on:click={(): void => { + // TODO: Implement Variables search/filter functionality + // e.g., switch to "Go to" or open Variables quick pick + }}>Variables</Button>
44-51: Fix TDZ risk: declare state before derived (searchIcon depends on searchOptionsSelectedIndex).Move
searchOptionsSelectedIndexabovesearchIconto avoid ordering issues. This was flagged earlier and still applies.- let searchIcon = $derived.by(() => { - if (searchOptionsSelectedIndex === 1) { - // Commands - return faChevronRight; - } else { - return faMagnifyingGlass; - } - }); +let searchOptionsSelectedIndex: number = $state(0); +let searchIcon = $derived.by(() => + searchOptionsSelectedIndex === 1 ? faChevronRight : faMagnifyingGlass, +); - let isMac: boolean = $state(false); - let modifierC: string = $derived(isMac ? '⌘' : 'Ctrl+'); - let modifierS: string = $derived(isMac ? '⇧' : 'Shift+'); - let searchOptions: SearchOption[] = $derived([ +let isMac: boolean = $state(false); +let modifierC: string = $derived(isMac ? '⌘' : 'Ctrl+'); +let modifierS: string = $derived(isMac ? '⇧' : 'Shift+'); +let searchOptions: SearchOption[] = $derived([ @@ -]); -let searchOptionsSelectedIndex: number = $state(0); +]);Also applies to: 62-62
117-123: Reset both selection pointers when opening.Avoid stale row selection on open by clearing
selectedFilteredIndextoo.function displaySearchBar(): void { // clear the input value inputValue = ''; selectedIndex = 0; + selectedFilteredIndex = 0; // toggle the display display = true; }
125-133: Don’t clear input when typing “>” inside the already open input; only switch tab.Currently, pressing “>” always calls
displaySearchBar()and wipes the field. Preserve input when palette is open and focused.- if (e.key === `${F1}` || e.key === '>') { + // If '>' is typed while input is focused and palette is open, just switch to Commands. + if (e.key === '>' && display && e.target === inputElement) { + searchOptionsSelectedIndex = 1; + e.preventDefault(); + return; + } + if (e.key === `${F1}` || e.key === '>') { selectedFilteredIndex = 0; searchOptionsSelectedIndex = 1; - displaySearchBar(); + if (!display) { + displaySearchBar(); + } e.preventDefault(); return; }
138-151: Only call displaySearchBar() when closed; switching tabs while open should not reset input.Also ensure we preventDefault in a single place.
- } else if (e.ctrlKey || e.metaKey) { - if (e.shiftKey && e.key.toLowerCase() === 'p') { - searchOptionsSelectedIndex = 0; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'k') { - searchOptionsSelectedIndex = 2; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'f') { - searchOptionsSelectedIndex = 3; - displaySearchBar(); - e.preventDefault(); - } + } else if (e.ctrlKey || e.metaKey) { + const key = e.key.toLowerCase(); + const map: Record<string, number> = { p: 0, k: 2, f: 3 }; + if ((key === 'p' && e.shiftKey) || key === 'k' || key === 'f') { + searchOptionsSelectedIndex = map[key]; + if (!display) { + selectedFilteredIndex = 0; + displaySearchBar(); + } + e.preventDefault(); + return; + } }
🧹 Nitpick comments (4)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (4)
199-202: Optional a11y: support ArrowLeft/ArrowRight for tab switching (WAI-ARIA pattern).Keep Tab/Shift+Tab if product wants it, but add arrows for expected tablist navigation.
- } else if (e.key === TAB_KEY) { - switchSearchOption(e.shiftKey ? -1 : 1); - e.preventDefault(); - } + } else if (e.key === 'ArrowLeft' || e.key === 'ArrowRight') { + switchSearchOption(e.key === 'ArrowRight' ? 1 : -1); + e.preventDefault(); + }
291-291: Expose proper tablist/tabpanel semantics; set aria-selected and roving tabindex.Improves screen reader support and focus management.
- <div class="flex flex-row m-2"> + <div class="flex flex-row m-2" role="tablist" aria-label="Search scopes"> {#each searchOptions as searchOption, index (index)} <Button type="tab" class="focus:outline-hidden" on:click={(): void => { searchOptionsSelectedIndex = index; }} - selected={searchOptionsSelectedIndex === index}> + selected={searchOptionsSelectedIndex === index} + role="tab" + aria-selected={searchOptionsSelectedIndex === index} + tabindex={searchOptionsSelectedIndex === index ? '0' : '-1'}>Also applies to: 293-311
316-318: Use Svelte’son:clickinstead ofonclick.Prefer the directive for consistency and typings.
- <button - onclick={(): Promise<void> => clickOnItem(i)} + <button + on:click={(): Promise<void> => clickOnItem(i)}
232-235: Typo: rename hideCommandPallete → hideCommandPalette (and update call sites).Improves readability and searchability.
-function hideCommandPallete(): void { +function hideCommandPalette(): void { display = false; onclose?.(); }- hideCommandPallete(); + hideCommandPalette();- hideCommandPallete(); + hideCommandPalette();- hideCommandPallete(); + hideCommandPalette();- hideCommandPallete(); + hideCommandPalette();Also applies to: 134-136, 194-195, 228-229, 239-240
📜 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(10 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 / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: build website
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: macOS
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
205-209: Nice wrap-around logic.Clean modulo math and direction offset.
| contextsUnsubscribe = context.subscribe(value => { | ||
| globalContext = value; | ||
| }); | ||
| themeUnsubscribe = isDark.subscribe(value => { |
There was a problem hiding this comment.
suggestion: it seems that we are in a Runes component, so it should be possible to use $isDark directly without subscribe in the onMount
There was a problem hiding this comment.
<script lang="ts">
import { faChevronRight, faMagnifyingGlass } from '@fortawesome/free-solid-svg-icons';
import { Button, Input } from '@podman-desktop/ui-svelte';
import { Icon } from '@podman-desktop/ui-svelte/icons';
import { onDestroy, onMount, tick } from 'svelte';
import type { Unsubscriber } from 'svelte/store';
import { isDark } from '/@/stores/appearance';
import { commandsInfos } from '/@/stores/commands';
import { context } from '/@/stores/context';
import type { CommandInfo } from '/@api/command-info';
import type { ContextUI } from '../context/context';
import ArrowDownIcon from '../images/ArrowDownIcon.svelte';
import ArrowUpIcon from '../images/ArrowUpIcon.svelte';
import EnterIcon from '../images/EnterIcon.svelte';
import NotFoundDarkIcon from '../images/NotFoundDarkIcon.svelte';
import NotFoundLightIcon from '../images/NotFoundLightIcon.svelte';
import { isPropertyValidInContext } from '../preferences/Util';
const ENTER_KEY = 'Enter';
const ESCAPE_KEY = 'Escape';
const ARROW_DOWN_KEY = 'ArrowDown';
const ARROW_UP_KEY = 'ArrowUp';
const TAB_KEY = 'Tab';
interface Props {
display?: boolean;
onclose?: () => void;
}
interface SearchOption {
text: string;
shortCut?: string[];
}
let { display = false, onclose }: Props = $props();
let outerDiv: HTMLDivElement | undefined = $state(undefined);
let inputElement: HTMLInputElement | undefined = $state(undefined);
let inputValue: string | undefined = $state('');
let scrollElements: HTMLLIElement[] = $state([]);
let searchIcon = $derived.by(() => {
if (searchOptionsSelectedIndex === 1) {
// Commands
return faChevronRight;
} else {
return faMagnifyingGlass;
}
});
let isMac: boolean = $state(false);
let modifierC: string = $derived(isMac ? '⌘' : 'Ctrl+');
let modifierS: string = $derived(isMac ? '⇧' : 'Shift+');
let searchOptions: SearchOption[] = $derived([
{ text: 'All', shortCut: [`${modifierC}${modifierS}P`] },
{ text: 'Commands', shortCut: ['F1', '>'] },
{ text: 'Documentation', shortCut: [`${modifierC}K`] },
{ text: 'Go to', shortCut: [`${modifierC}F`] },
]);
let searchOptionsSelectedIndex: number = $state(0);
let commandInfoItems: CommandInfo[] = $state([]);
let globalContext: ContextUI;
let filteredCommandInfoItems: CommandInfo[] = $derived(
commandInfoItems
.filter(property => isPropertyValidInContext(property.enablement, globalContext))
.filter(item => (inputValue ? item.title?.toLowerCase().includes(inputValue.toLowerCase()) : true)),
);
let contextsUnsubscribe: Unsubscriber;
onMount(async () => {
const platform = await window.getOsPlatform();
isMac = platform === 'darwin';
});
onMount(() => {
contextsUnsubscribe = context.subscribe(value => {
globalContext = value;
});
// subscribe to the commands
return commandsInfos.subscribe(infos => {
commandInfoItems = infos;
});
});
onDestroy(() => {
contextsUnsubscribe?.();
});
// Focus the input when the command palette becomes visible
$effect(() => {
if (display && inputElement) {
tick()
.then(() => {
inputElement?.focus();
})
.catch((error: unknown) => {
console.error('Unable to focus input box', error);
});
}
});
let selectedFilteredIndex = $state(0);
let selectedIndex = 0;
function displaySearchBar(): void {
// clear the input value
inputValue = '';
selectedIndex = 0;
// toggle the display
display = true;
}
async function handleKeydown(e: KeyboardEvent): Promise<void> {
// toggle display using F1 or ESC keys
if (e.key === 'F1' || e.key === '>') {
selectedFilteredIndex = 0;
searchOptionsSelectedIndex = 1;
displaySearchBar();
e.preventDefault();
return;
} else if (e.key === ESCAPE_KEY) {
// here we toggle the display
hideCommandPallete();
e.preventDefault();
return;
} else if (e.ctrlKey || e.metaKey) {
if (e.shiftKey && e.key.toLowerCase() === 'p') {
searchOptionsSelectedIndex = 0;
displaySearchBar();
} else if (e.key.toLowerCase() === 'k') {
searchOptionsSelectedIndex = 2;
displaySearchBar();
} else if (e.key.toLowerCase() === 'f') {
searchOptionsSelectedIndex = 3;
displaySearchBar();
}
}
// for other keys, only check if it's being displayed
if (!display) {
return;
}
// no items, abort
if (filteredCommandInfoItems.length === 0) {
return;
}
if (e.key === ARROW_DOWN_KEY) {
// if down key is pressed, move the index
selectedFilteredIndex++;
if (selectedFilteredIndex >= filteredCommandInfoItems.length) {
selectedFilteredIndex = 0;
}
selectedIndex = commandInfoItems.indexOf(filteredCommandInfoItems[selectedFilteredIndex]);
scrollElements[selectedFilteredIndex].scrollIntoView({
behavior: 'smooth',
block: 'nearest',
inline: 'start',
});
e.preventDefault();
} else if (e.key === ARROW_UP_KEY) {
// if up key is pressed, move the index
selectedFilteredIndex--;
if (selectedFilteredIndex < 0) {
selectedFilteredIndex = filteredCommandInfoItems.length - 1;
}
selectedIndex = commandInfoItems.indexOf(filteredCommandInfoItems[selectedFilteredIndex]);
scrollElements[selectedFilteredIndex].scrollIntoView({
behavior: 'smooth',
block: 'nearest',
inline: 'start',
});
e.preventDefault();
} else if (e.key === ENTER_KEY) {
// hide the command palette
hideCommandPallete();
selectedIndex = commandInfoItems.indexOf(filteredCommandInfoItems[selectedFilteredIndex]);
await executeCommand(selectedIndex);
e.preventDefault();
} else if (e.key === TAB_KEY) {
switchSearchOption(e.shiftKey ? -1 : 1);
e.preventDefault();
}
}
function switchSearchOption(direction: 1 | -1): void {
const searchOptionsLength = searchOptions.length;
const offset = direction === 1 ? 0 : searchOptionsLength;
searchOptionsSelectedIndex = (searchOptionsSelectedIndex + direction + offset) % searchOptionsLength;
}
async function executeCommand(index: number): Promise<void> {
// get command id
const commandId = commandInfoItems[index].id;
// execute the command
try {
await window.executeCommand(commandId);
} catch (error) {
console.error('error executing command', error);
}
}
function handleMousedown(e: MouseEvent): void {
if (!display) {
return;
}
if (outerDiv && !e.defaultPrevented && e.target instanceof Node && !outerDiv.contains(e.target)) {
hideCommandPallete();
}
}
function hideCommandPallete(): void {
display = false;
onclose?.();
}
async function clickOnItem(index: number): Promise<void> {
// hide the command palette
hideCommandPallete();
// select the index from the cursor
selectedIndex = commandInfoItems.indexOf(filteredCommandInfoItems[index]);
await executeCommand(selectedIndex);
}
async function onInputChange(): Promise<void> {
// in case of quick pick, filter the items
selectedFilteredIndex = 0;
if (filteredCommandInfoItems.length > 0) {
selectedIndex = commandInfoItems.indexOf(filteredCommandInfoItems[selectedFilteredIndex]);
}
}
async function onAction(): Promise<void> {
// When input is cleared, refocus it
tick()
.then(() => {
inputElement?.focus();
})
.catch((error: unknown) => {
console.error('Unable to focus input box', error);
});
}
</script>
<svelte:window on:keydown={handleKeydown} on:mousedown={handleMousedown} />
{#if display}
<div class="fixed top-0 left-0 right-0 bottom-0 bg-[var(--pd-modal-fade)] opacity-60 h-full z-50" style='-webkit-app-region: none;'></div>
<div class="absolute m-auto left-0 right-0 z-50">
<div class="flex justify-center items-center mt-1">
<div
bind:this={outerDiv}
class="bg-[var(--pd-content-card-bg)] w-[700px] max-h-fit shadow-lg p-2 rounded-sm shadow-[var(--pd-input-field-stroke)] text-base">
<div class="w-full flex flex-row gap-2 items-center">
<Input
aria-label='Command palette command input'
bind:value={inputValue}
bind:element={inputElement}
clearable={true}
on:input={onInputChange}
on:action={onAction}
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" >
{#snippet left()}
<Icon icon={searchIcon} class="pr-1"/>
{/snippet}
</Input>
</div>
<div class="flex flex-row m-2">
{#each searchOptions as searchOption, index (index)}
<Button
type="tab"
class="focus:outline-hidden"
on:click={(): void => {
searchOptionsSelectedIndex = index;
}}
selected={searchOptionsSelectedIndex === index}>
<div class="flex items-center gap-2">
{#if searchOption.shortCut}
{#each searchOption.shortCut as shortCut, i (i)}
<div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-0.5 shadow-sm shadow-b-1'>
{shortCut}
</div>
{/each}
{/if}
{searchOption.text}
</div>
</Button>
{/each}
</div>
<ul class="max-h-[50vh] overflow-y-auto flex flex-col mt-1">
{#each filteredCommandInfoItems as item, i (item.id)}
<li class="flex w-full flex-row" bind:this={scrollElements[i]} aria-label={item.id}>
<button
onclick={(): Promise<void> => clickOnItem(i)}
class="text-[var(--pd-dropdown-item-text)] text-left relative w-full rounded-sm {i === selectedFilteredIndex
? 'bg-[var(--pd-modal-dropdown-highlight)] selected'
: 'hover:bg-[var(--pd-dropdown-bg)]'} px-1">
<div class="flex flex-col w-full">
<div class="flex flex-row w-full max-w-[700px] truncate">
<div class="text-base py-[2pt]">{item.title}</div>
</div>
</div>
</button>
</li>
{/each}
</ul>
{#if filteredCommandInfoItems.length === 0}
<div class='flex grow items-center flex-col gap-2 py-4'>
<Icon icon={$isDark ? NotFoundDarkIcon : NotFoundLightIcon} />
<div class='text-lg font-bold'>No results matching '{inputValue}' found</div>
<div class='text-md'>Not what you expected? Double-check your spelling or try searching for:</div>
<Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button>
</div>
{/if}
<div class="border-[var(--pd-global-nav-bg-border)] border-t-[1px] flex flex-row items-center px-3 pt-2 gap-4 text-sm">
<span class="flex items-center gap-2 text-[var(--pd-button-tab-text)] border-[var(--pd-button-tab-border-selected)]">
<Icon icon={EnterIcon} class="bg-[var(--pd-action-button-bg)] rounded-sm p-1.5" size='2.2em'/>
To select
</span>
<div class="flex items-center gap-2 text-[var(--pd-button-tab-text)] border-[var(--pd-button-tab-border-selected)]">
<Icon icon={ArrowUpIcon} class="bg-[var(--pd-action-button-bg)] rounded-sm p-1.5" size='2.2em'/>
<Icon icon={ArrowDownIcon} class="bg-[var(--pd-action-button-bg)] rounded-sm p-1.5" size='2.2em'/>
To navigate
</div>
<div class="flex items-center gap-2 text-[var(--pd-button-tab-text)] border-[var(--pd-button-tab-border-selected)]">
<div class='bg-[var(--pd-action-button-bg)] rounded-sm text-base px-1 py-0.5'>esc</div>
To close
</div>
</div>
</div>
</div>
</div>
{/if}
There was a problem hiding this comment.
I think that if I use just $isDark, it will get only the current value. So If you try to change the preference, the Image will be same
There was a problem hiding this comment.
Edit: when we switch light / dark with the menu open, the image stays the same yes with this code
There was a problem hiding this comment.
But we have the same issue with your current implemntation, so I am still pushing to use $isDark directly
| let contextsUnsubscribe: Unsubscriber; | ||
| let themeUnsubscribe: Unsubscriber; | ||
| onMount(async () => { |
There was a problem hiding this comment.
Duplicated onMount method with line 83
There was a problem hiding this comment.
There was a problem with the return method. I think I could connect those 2 when I create a new unsubscriber for commandsInfos. But I didn't want to do this refactoring
There was a problem hiding this comment.
Ok I tried and indeed there was a problem with the return. But I am still confused how Svelte handles multiple onMount
| } | ||
| } | ||
| function switchSearchOption(direction: 1 | -1): void { |
|
@eqqe #13661 here is an another PR that adds content into docs tab if you want to take a look |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
65-69: Wire filteredCommandInfoItems to respect the selected tab
Replace the current$deriveddefinition (lines 65–69) with a$derived.bycallback that switches onsearchOptionsSelectedIndex, returning your base filters for “All” (0) and “Commands” (1) and stubbing out the other tabs until their data is hooked up. For example:-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.by(() => { + const q = (inputValue ?? '').toLowerCase(); + const base = commandInfoItems + .filter(p => isPropertyValidInContext(p.enablement, globalContext)) + .filter(i => (q ? i.title?.toLowerCase().includes(q) : true)); + switch (searchOptionsSelectedIndex) { + case 0: // All + case 1: // Commands + return base; + case 2: // Documentation + case 3: // Go to + return []; // TODO: hook in documentation and go-to sources + default: + return base; + } +});This ensures each tab actually filters the results as intended.
♻️ Duplicate comments (5)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (5)
109-115: Reset both selection indices when openingAvoid stale highlighted row by resetting
selectedFilteredIndextoo.function displaySearchBar(): void { // clear the input value inputValue = ''; selectedIndex = 0; + selectedFilteredIndex = 0; // toggle the display display = true; }
131-143: Shortcut branches should not clear input when already open; ensure preventDefault in all branchesAlign behavior with UX expectations and prior feedback.
- if (e.shiftKey && e.key.toLowerCase() === 'p') { - searchOptionsSelectedIndex = 0; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'k') { - searchOptionsSelectedIndex = 2; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'f') { - searchOptionsSelectedIndex = 3; - displaySearchBar(); - e.preventDefault(); - } + const key = e.key.toLowerCase(); + const map: Record<string, number> = { p: 0, k: 2, f: 3 }; + if ((key === 'p' && e.shiftKey) || key === 'k' || key === 'f') { + searchOptionsSelectedIndex = map[key]; + if (!display) { + displaySearchBar(); + } + e.preventDefault(); + return; + }
323-330: Clarify placeholder action for “Variables”The button only logs to console; add a TODO or wire up navigation to avoid confusing users.
-<Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button> +<Button icon={faChevronRight} type='link' onclick={(): void => { + // TODO: Implement Variables search/filter action +}}>Variables</Button>
42-49: Fix TDZ risk: declare state before derived
searchIconderives fromsearchOptionsSelectedIndexbefore it’s declared. Move the state above the derived.-let searchIcon = $derived.by(() => { - if (searchOptionsSelectedIndex === 1) { - // Commands - return faChevronRight; - } else { - return faMagnifyingGlass; - } -}); - -let isMac: boolean = $state(false); +let searchOptionsSelectedIndex: number = $state(0); +let searchIcon = $derived.by(() => (searchOptionsSelectedIndex === 1 ? faChevronRight : faMagnifyingGlass)); +let isMac: boolean = $state(false); @@ -let searchOptionsSelectedIndex: number = $state(0);Also applies to: 60-60
119-125: Don’t clear input when palette is already open (F1 / “>”)Calling
displaySearchBar()always clears the field. Only call it when closed; when open, just switch to Commands.- if (e.key === `${F1}` || e.key === '>') { - selectedFilteredIndex = 0; - searchOptionsSelectedIndex = 1; - displaySearchBar(); + if (e.key === F1 || e.key === '>') { + selectedFilteredIndex = 0; + searchOptionsSelectedIndex = 1; + if (!display) { + displaySearchBar(); + } e.preventDefault(); return; }Optional: if
displayis true and focus is in the input, swallow the “>” and switch tabs without affectinginputValue.
🧹 Nitpick comments (6)
packages/renderer/src/lib/images/NotFoundIcon.svelte (1)
2-4: Simplify: use store directly; drop manual subscribe/unsubscribe and forward props to inner iconsLeverage Svelte’s store auto-subscription and forward size/class so parent Icon wrappers can style consistently.
<script lang="ts"> -import { onDestroy, onMount } from 'svelte'; -import type { Unsubscriber } from 'svelte/store'; - import { isDark } from '/@/stores/appearance'; import NotFoundDarkIcon from '../images/NotFoundDarkIcon.svelte'; import NotFoundLightIcon from '../images/NotFoundLightIcon.svelte'; -let isDarkTheme = $state(false); -let unsubscribe: Unsubscriber; - -onMount(() => { - unsubscribe = isDark.subscribe(value => { - isDarkTheme = value; - }); -}); - -onDestroy(() => { - unsubscribe?.(); -}); +interface Props { + class?: string; + size?: string | number; +} +let { class: klass = '', size }: Props = $props(); </script> -{#if isDarkTheme} - <NotFoundDarkIcon /> +{#if $isDark} + <NotFoundDarkIcon class={klass} size={size} /> {:else} - <NotFoundLightIcon /> + <NotFoundLightIcon class={klass} size={size} /> {/if}Also applies to: 10-21, 24-28
packages/renderer/src/lib/dialogs/CommandPalette.svelte (5)
191-194: Keyboard a11y: support ArrowLeft/ArrowRight for tab switching (keep Tab if product wants it)Per WAI-ARIA Tabs pattern, Left/Right should switch tabs. You can keep Tab/Shift+Tab if mandated, but add arrows for a11y parity.
-} else if (e.key === TAB_KEY) { - switchSearchOption(e.shiftKey ? -1 : 1); +} else if (e.key === TAB_KEY) { + switchSearchOption(e.shiftKey ? -1 : 1); + e.preventDefault(); +} else if (e.key === 'ArrowLeft' || e.key === 'ArrowRight') { + switchSearchOption(e.key === 'ArrowRight' ? 1 : -1); e.preventDefault(); }Also add
role="tablist"on the tab container (see below).
283-304: Add ARIA roles and reset selection on tab click
- Expose
role="tablist"for the container.- When switching tabs via click, also reset
selectedFilteredIndexto avoid stale selection.-<div class="flex flex-row m-2"> +<div class="flex flex-row m-2" role="tablist" aria-label="Search scopes"> @@ - on:click={(): void => { - searchOptionsSelectedIndex = index; - }} + on:click={(): void => { + searchOptionsSelectedIndex = index; + selectedFilteredIndex = 0; + }}Confirm
Button type="tab"setsrole="tab"andaria-selected; if not, set them explicitly.
295-297: Match design: increase horizontal padding on chipsPer design feedback, use more generous padding for the shortcut chips.
-<div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-0.5 shadow-sm shadow-b-1'> +<div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-2 shadow-sm shadow-b-1'>
24-24: Minor: avoid unnecessary template string for key constantUse
e.key === F1instead ofe.key === `${F1}`.-const F1 = 'F1'; +const F1 = 'F1'; // unify with other key consts @@ - if (e.key === `${F1}` || e.key === '>') { + if (e.key === F1 || e.key === '>') {Also applies to: 119-119
224-227: Naming nit: “Palette” spellingRename
hideCommandPalletetohideCommandPaletteand update call sites for consistency.-function hideCommandPallete(): void { +function hideCommandPalette(): void { display = false; onclose?.(); } @@ - hideCommandPallete(); + hideCommandPalette(); @@ - hideCommandPallete(); + hideCommandPalette();Also applies to: 118-129, 185-191
📜 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 (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte(9 hunks)packages/renderer/src/lib/images/NotFoundIcon.svelte(1 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). (8)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2025
- GitHub Check: linter, formatters
- GitHub Check: Windows
- GitHub Check: typecheck
- GitHub Check: macOS
- GitHub Check: build website
| }); | ||
| </script> | ||
|
|
||
| {#if isDarkTheme} |
There was a problem hiding this comment.
use $isDark and remove the subscribe/unsubscribe stuff to use the store value in reactive mode directly
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
185751d to
70ee35a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (1)
65-69: Wire tabs into filtering logic (pre-filter by selected scope).Meets the PR objective and avoids showing non-command items in Commands. Docs/Go to can be stubbed empty until sources are plugged.
-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.by(() => { + const q = (inputValue ?? '').toLowerCase(); + const base = commandInfoItems.filter(p => isPropertyValidInContext(p.enablement, globalContext)); + const textMatch = (t?: string) => (q ? t?.toLowerCase().includes(q) : true); + switch (searchOptionsSelectedIndex) { + case 0: // All (currently same dataset as commands) + case 1: // Commands + return base.filter(i => textMatch(i.title)); + case 2: // Documentation (hook external source later) + case 3: // Go to (hook targets later) + return []; + default: + return base.filter(i => textMatch(i.title)); + } +});
♻️ Duplicate comments (6)
packages/renderer/src/lib/images/NotFoundIcon.svelte (1)
8-12: LGTM: uses$isDarkstore reactively (matches prior feedback).packages/renderer/src/lib/dialogs/CommandPalette.svelte (5)
191-194: Keyboard a11y: use ArrowLeft/Right to switch tabs; add tab roles.Tab should move focus; arrows should change tabs. Also expose WAI-ARIA roles.
- } else if (e.key === TAB_KEY) { - switchSearchOption(e.shiftKey ? -1 : 1); + } else if (e.key === 'ArrowLeft' || e.key === 'ArrowRight') { + switchSearchOption(e.key === 'ArrowRight' ? 1 : -1); e.preventDefault(); }- <div class="flex flex-row m-2"> + <div class="flex flex-row m-2" role="tablist" aria-label="Search scopes"> {#each searchOptions as searchOption, index (index)} <Button type="tab" class="focus:outline-hidden" + role="tab" + aria-selected={searchOptionsSelectedIndex === index} on:click={(): void => { searchOptionsSelectedIndex = index; }} selected={searchOptionsSelectedIndex === index}>Also applies to: 283-304
323-330: ‘Variables’ link is a stub — add TODO or implement.- <Button icon={faChevronRight} type='link' onclick={(): void => {console.log('Variables clicked');}}>Variables</Button> + <Button + icon={faChevronRight} + type='link' + onclick={(): void => { + // TODO: Implement Variables search/filter action (e.g., switch tab/scope and prefill query) + console.log('Variables clicked'); + }}> + Variables + </Button>
42-49: Avoid potential TDZ: declaresearchOptionsSelectedIndexbeforesearchIconderived.Svelte runes can trip over late-bound state. Move the state above the derived.
-let searchIcon = $derived.by(() => { - if (searchOptionsSelectedIndex === 1) { - // Commands - return faChevronRight; - } else { - return faMagnifyingGlass; - } -}); - -let isMac: boolean = $state(false); +let searchOptionsSelectedIndex: number = $state(0); +let searchIcon = $derived.by(() => (searchOptionsSelectedIndex === 1 ? faChevronRight : faMagnifyingGlass)); +let isMac: boolean = $state(false); @@ -let searchOptionsSelectedIndex: number = $state(0);Also applies to: 60-60
109-115: Reset filtered selection when opening to prevent stale row highlight.function displaySearchBar(): void { // clear the input value inputValue = ''; selectedIndex = 0; + selectedFilteredIndex = 0; // toggle the display display = true; }
119-123: Don’t clear input when palette is already open; gatedisplaySearchBar()and handle>gracefully.Calling
displaySearchBar()always clearsinputValue. Switch tabs without clearing when open.- if (e.key === `${F1}` || e.key === '>') { - selectedFilteredIndex = 0; - searchOptionsSelectedIndex = 1; - displaySearchBar(); + if (e.key === `${F1}` || e.key === '>') { + selectedFilteredIndex = 0; + searchOptionsSelectedIndex = 1; + if (!display) { + displaySearchBar(); + } e.preventDefault(); return; @@ - } else if (e.ctrlKey || e.metaKey) { - if (e.shiftKey && e.key.toLowerCase() === 'p') { - searchOptionsSelectedIndex = 0; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'k') { - searchOptionsSelectedIndex = 2; - displaySearchBar(); - e.preventDefault(); - } else if (e.key.toLowerCase() === 'f') { - searchOptionsSelectedIndex = 3; - displaySearchBar(); - e.preventDefault(); - } + } else if (e.ctrlKey || e.metaKey) { + const key = e.key.toLowerCase(); + const map: Record<string, number> = { p: 0, k: 2, f: 3 }; + if ((key === 'p' && e.shiftKey) || key === 'k' || key === 'f') { + searchOptionsSelectedIndex = map[key]; + if (!display) { + displaySearchBar(); + } + e.preventDefault(); + return; + } }Also applies to: 131-143
🧹 Nitpick comments (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte (2)
224-227: Fix typo: renamehideCommandPallete→hideCommandPalette.Minor readability polish; update all call sites.
-function hideCommandPallete(): void { +function hideCommandPalette(): void { display = false; onclose?.(); }- } else if (e.key === ESCAPE_KEY) { + } else if (e.key === ESCAPE_KEY) { // here we toggle the display - hideCommandPallete(); + hideCommandPalette();- // hide the command palette - hideCommandPallete(); + // hide the command palette + hideCommandPalette();- // hide the command palette - hideCommandPallete(); + // hide the command palette + hideCommandPalette();Also applies to: 125-129, 186-190, 231-236
295-297: Match design: increase shortcut chip horizontal padding.Aligns with comps (“px-2”).
- <div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-0.5 shadow-sm shadow-b-1'> + <div class='bg-[var(--pd-search-bar-nav-button)] rounded-sm px-2 shadow-sm shadow-b-1'>
📜 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 (2)
packages/renderer/src/lib/dialogs/CommandPalette.svelte(9 hunks)packages/renderer/src/lib/images/NotFoundIcon.svelte(1 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: smoke e2e tests (development)
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (production)
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2025
- GitHub Check: Linux
- GitHub Check: build website
- GitHub Check: macOS
- GitHub Check: Windows
| <script lang="ts"> | ||
| import { isDark } from '/@/stores/appearance'; | ||
| import NotFoundDarkIcon from '../images/NotFoundDarkIcon.svelte'; | ||
| import NotFoundLightIcon from '../images/NotFoundLightIcon.svelte'; | ||
| </script> | ||
|
|
||
| {#if $isDark} | ||
| <NotFoundDarkIcon /> | ||
| {:else} | ||
| <NotFoundLightIcon /> | ||
| {/if} No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Forward size/class/style props to inner icons (and use local ./ imports).
Without prop forwarding, parent <Icon> sizing/styling won’t apply; the wrapper locks the inner icon at its defaults. Also, prefer ./ over ../images for sibling imports.
<script lang="ts">
-import { isDark } from '/@/stores/appearance';
-
-import NotFoundDarkIcon from '../images/NotFoundDarkIcon.svelte';
-import NotFoundLightIcon from '../images/NotFoundLightIcon.svelte';
+import { isDark } from '/@/stores/appearance';
+import NotFoundDarkIcon from './NotFoundDarkIcon.svelte';
+import NotFoundLightIcon from './NotFoundLightIcon.svelte';
+
+// forward common props used by icon components
+export let size: string = '88';
+export let class: string = '';
+export let style: string = '';
</script>
{#if $isDark}
- <NotFoundDarkIcon />
+ <NotFoundDarkIcon {size} {class} {style} />
{:else}
- <NotFoundLightIcon />
+ <NotFoundLightIcon {size} {class} {style} />
{/if}📝 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.
| <script lang="ts"> | |
| import { isDark } from '/@/stores/appearance'; | |
| import NotFoundDarkIcon from '../images/NotFoundDarkIcon.svelte'; | |
| import NotFoundLightIcon from '../images/NotFoundLightIcon.svelte'; | |
| </script> | |
| {#if $isDark} | |
| <NotFoundDarkIcon /> | |
| {:else} | |
| <NotFoundLightIcon /> | |
| {/if} | |
| <script lang="ts"> | |
| import { isDark } from '/@/stores/appearance'; | |
| import NotFoundDarkIcon from './NotFoundDarkIcon.svelte'; | |
| import NotFoundLightIcon from './NotFoundLightIcon.svelte'; | |
| // forward common props used by icon components | |
| export let size: string = '88'; | |
| export let class: string = ''; | |
| export let style: string = ''; | |
| </script> | |
| {#if $isDark} | |
| <NotFoundDarkIcon {size} {class} {style} /> | |
| {:else} | |
| <NotFoundLightIcon {size} {class} {style} /> | |
| {/if} |
What does this PR do?
Adds tabs to searchbar so we can easily pre-filter options
Screenshot / video of UI
Screen.Recording.2025-08-29.at.18.30.25.mov
What issues does this PR fix or reference?
Closes #13627
How to test this PR?
Enable searchbar and try out different tabs (so far there is no content in them other than commands)