feat(desktop): add global command palette#4488
Conversation
Adds ⌘⇧K command palette mounted in the dashboard layout, with a module-based architecture so feature areas contribute commands without touching a central switch. Modules ship: workspace (New/Search files/ Link task/Remove from sidebar/Delete/Open in), actions (Toggle theme/ sidebars, Mute notifications, keyboard shortcuts, Check for updates), navigation (Settings sub-palette, Recently Viewed, Docs). Workspace-local mutations route through small intent stores (delete, remove-from-sidebar, right-sidebar toggle, set-preferred-open-in-app) so the palette can drive state owned by per-workspace hooks.
📝 WalkthroughWalkthroughThis PR adds a comprehensive global command palette to the desktop renderer, exposing commands for theme switching, sidebar toggling, notifications, settings navigation, workspace operations, and file opening with external apps. The system includes modular command providers, a hierarchical frame stack for submenus, context-driven filtering, and intent-based state management for sidebar and dialog operations. ChangesDesktop Command Palette Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Greptile SummaryThis PR adds a global command palette (⌘⇧K) to the desktop app using a module-based architecture where feature areas contribute commands through a
Confidence Score: 3/5The palette infrastructure is solid, but the Link task command silently does nothing despite reporting success — that feature should not ship in its current state. The Link task sub-palette fires a success toast and closes the palette without actually linking anything to the workspace —
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx | Renders a searchable task list but linkTaskToWorkspace is an unimplemented stub — success toast fires yet nothing is persisted. |
| apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx | Main dialog; correctly manages frame stack and query state, but children/renderFrame precedence means any co-defined run on framed commands is permanently dead code. |
| apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx | Provides actions commands; run on actions.toggleTheme is unreachable dead code because renderFrame causes handleSelect to push a frame instead. |
| apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx | Navigation provider; run on nav.settings is unreachable because children causes the palette to push a sub-frame instead of executing run. |
| apps/desktop/src/renderer/lib/router-instance.ts | Module-level router singleton; typed as AnyRouter, so all callers lose TanStack Router's compile-time route-path safety. |
| apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx | Aggregates workspace, sidebar-project, and notification-mute state into a memoized CommandContext; correctly guards against null workspace IDs in live queries. |
| apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts | Open-in provider; correctly gates on workspace presence and host identity; updates preferred app via intent store before opening. |
| apps/desktop/src/renderer/commandPalette/ui/RecentlyViewed/RecentlyViewedFrame.tsx | Mirrors HistoryDropdown rendering; filtering logic correctly gates entry types on isV2CloudEnabled; no issues found. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
HK["⌘⇧K hotkey"] --> FSS["useFrameStackStore setOpen(true)"]
FSS --> CP["CommandPalette Dialog"]
CP --> CLV["CommandListView root level"]
CP --> SPV["SubPaletteView when frame pushed"]
CLV --> UAC["useActiveCommands useSyncExternalStore"]
UAC --> REG["registry Map of providers"]
REG --> WP["workspaceProvider"]
REG --> AP["actionsProvider"]
REG --> OIP["openInProvider"]
REG --> NP["navigationProvider"]
CP -- "children/renderFrame?" --> PUSH["pushFrame SubPaletteView"]
CP -- "leaf command" --> EXEC["executeCommand track + run"]
EXEC --> CLOSE["handleOpenChange false"]
PUSH --> TF["ThemeFrame"]
PUSH --> LTF["LinkTaskFrame stub"]
PUSH --> RVF["RecentlyViewedFrame"]
PUSH --> STAB["Settings tabs list"]
EXEC --> IS["intent stores"]
IS --> MOUNTS["Mount components"]
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx:88-94
**`linkTaskToWorkspace` is a no-op stub**
The function discards both arguments and returns immediately — only `void taskId; void workspaceId;`. When a user selects a task, the toast `"Linked {slug} to workspace"` fires, but nothing is persisted. The link operation is completely missing, making the entire "Link task" command non-functional despite reporting success.
### Issue 2 of 4
apps/desktop/src/renderer/lib/router-instance.ts:1-14
**`AnyRouter` loses all route-type safety at call sites**
`getRouterInstance()` is typed as `AnyRouter`, so every `navigate({ to: tab.path })` call (including the 20+ settings tabs in `settings/commands.ts`) accepts an arbitrary string without compile-time validation. Typos in path strings will silently navigate to a 404 rather than producing a type error. Typing the ref as the concrete router type (returned by `createRouter`) and exporting that type would restore the full route-type checking that TanStack Router provides.
### Issue 3 of 4
apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx:46-51
**`run` on `actions.toggleTheme` is unreachable dead code**
`handleSelect` in `CommandPalette` checks `command.children || command.renderFrame` first and returns early via `pushFrame`, so any command that defines `renderFrame` will never have its `run` executed. The `cycleTheme()` call here is permanently skipped. If cycling the theme without opening the picker is still desired, it needs a separate command; otherwise `run` should be removed.
### Issue 4 of 4
apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx:13-22
**`run` on `nav.settings` is unreachable dead code**
`CommandPalette.handleSelect` short-circuits to `pushFrame` whenever `command.children` is truthy — the `run` callback that navigates to `/settings/account` is never invoked. If the intent is to navigate directly when the user selects the top-level "Settings" item, `children` would need to be absent. Otherwise `run` can be dropped.
Reviews (1): Last reviewed commit: "feat(desktop): add global command palett..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| async function linkTaskToWorkspace( | ||
| taskId: string, | ||
| workspaceId: string, | ||
| ): Promise<void> { | ||
| void taskId; |
There was a problem hiding this comment.
linkTaskToWorkspace is a no-op stub
The function discards both arguments and returns immediately — only void taskId; void workspaceId;. When a user selects a task, the toast "Linked {slug} to workspace" fires, but nothing is persisted. The link operation is completely missing, making the entire "Link task" command non-functional despite reporting success.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx
Line: 88-94
Comment:
**`linkTaskToWorkspace` is a no-op stub**
The function discards both arguments and returns immediately — only `void taskId; void workspaceId;`. When a user selects a task, the toast `"Linked {slug} to workspace"` fires, but nothing is persisted. The link operation is completely missing, making the entire "Link task" command non-functional despite reporting success.
How can I resolve this? If you propose a fix, please make it concise.| import type { AnyRouter } from "@tanstack/react-router"; | ||
|
|
||
| let routerRef: AnyRouter | null = null; | ||
|
|
||
| export function setRouterInstance(router: AnyRouter): void { | ||
| routerRef = router; | ||
| } | ||
|
|
||
| export function getRouterInstance(): AnyRouter { | ||
| if (!routerRef) { | ||
| throw new Error("Router instance not set yet"); | ||
| } | ||
| return routerRef; | ||
| } |
There was a problem hiding this comment.
AnyRouter loses all route-type safety at call sites
getRouterInstance() is typed as AnyRouter, so every navigate({ to: tab.path }) call (including the 20+ settings tabs in settings/commands.ts) accepts an arbitrary string without compile-time validation. Typos in path strings will silently navigate to a 404 rather than producing a type error. Typing the ref as the concrete router type (returned by createRouter) and exporting that type would restore the full route-type checking that TanStack Router provides.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/router-instance.ts
Line: 1-14
Comment:
**`AnyRouter` loses all route-type safety at call sites**
`getRouterInstance()` is typed as `AnyRouter`, so every `navigate({ to: tab.path })` call (including the 20+ settings tabs in `settings/commands.ts`) accepts an arbitrary string without compile-time validation. Typos in path strings will silently navigate to a 404 rather than producing a type error. Typing the ref as the concrete router type (returned by `createRouter`) and exporting that type would restore the full route-type checking that TanStack Router provides.
How can I resolve this? If you propose a fix, please make it concise.| { | ||
| id: "actions.toggleTheme", | ||
| title: "Toggle theme", | ||
| section: "actions", | ||
| icon: PaletteIcon, | ||
| keywords: ["dark", "light", "appearance", "color"], |
There was a problem hiding this comment.
run on actions.toggleTheme is unreachable dead code
handleSelect in CommandPalette checks command.children || command.renderFrame first and returns early via pushFrame, so any command that defines renderFrame will never have its run executed. The cycleTheme() call here is permanently skipped. If cycling the theme without opening the picker is still desired, it needs a separate command; otherwise run should be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx
Line: 46-51
Comment:
**`run` on `actions.toggleTheme` is unreachable dead code**
`handleSelect` in `CommandPalette` checks `command.children || command.renderFrame` first and returns early via `pushFrame`, so any command that defines `renderFrame` will never have its `run` executed. The `cycleTheme()` call here is permanently skipped. If cycling the theme without opening the picker is still desired, it needs a separate command; otherwise `run` should be removed.
How can I resolve this? If you propose a fix, please make it concise.| title: "Settings", | ||
| section: "navigation", | ||
| icon: SettingsIcon, | ||
| hotkeyId: "OPEN_SETTINGS", | ||
| children: settingsTabCommands, | ||
| run: () => { | ||
| void getRouterInstance().navigate({ to: "/settings/account" }); | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
run on nav.settings is unreachable dead code
CommandPalette.handleSelect short-circuits to pushFrame whenever command.children is truthy — the run callback that navigates to /settings/account is never invoked. If the intent is to navigate directly when the user selects the top-level "Settings" item, children would need to be absent. Otherwise run can be dropped.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx
Line: 13-22
Comment:
**`run` on `nav.settings` is unreachable dead code**
`CommandPalette.handleSelect` short-circuits to `pushFrame` whenever `command.children` is truthy — the `run` callback that navigates to `/settings/account` is never invoked. If the intent is to navigate directly when the user selects the top-level "Settings" item, `children` would need to be absent. Otherwise `run` can be dropped.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
apps/desktop/src/renderer/commandPalette/CommandPaletteHost.tsx (1)
29-33: ⚡ Quick winConsider inlining CommandPaletteTrigger into the host component.
The
CommandPaletteTriggercomponent is a small internal helper that could be inlined intoCommandPaletteHostto follow the single-component-per-file convention and improve code locality.♻️ Proposed inline refactor
export function CommandPaletteHost({ children }: { children?: ReactNode }) { + const setOpen = useFrameStackStore((s) => s.setOpen); + useEffect(() => { const unregister = registerAllModules(); return unregister; }, []); + + useHotkey("OPEN_COMMAND_PALETTE", () => setOpen(true)); return ( <CommandContextProvider> - <CommandPaletteTrigger /> <CommandPalette /> <DeleteWorkspaceMount /> <RemoveFromSidebarMount /> <SetPreferredOpenInAppMount /> {children} </CommandContextProvider> ); } - -function CommandPaletteTrigger() { - const setOpen = useFrameStackStore((s) => s.setOpen); - useHotkey("OPEN_COMMAND_PALETTE", () => setOpen(true)); - return null; -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/CommandPaletteHost.tsx` around lines 29 - 33, The small helper component CommandPaletteTrigger should be inlined into CommandPaletteHost: remove the separate CommandPaletteTrigger function and move its logic into CommandPaletteHost by calling useFrameStackStore((s) => s.setOpen) and useHotkey("OPEN_COMMAND_PALETTE", () => setOpen(true)) directly inside CommandPaletteHost (ensuring hooks remain at the top level of the component), then delete the unused CommandPaletteTrigger export/definition to keep a single-component-per-file and improve locality.apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx (2)
104-104: 💤 Low valueRemove redundant responsive breakpoint.
The
sm:!max-w-[720px]class duplicates the base!max-w-[720px]with the same value, making the breakpoint override unnecessary.♻️ Simplify by removing redundant class
- className="!max-w-[720px] sm:!max-w-[720px] translate-y-0 max-h-[80vh] overflow-hidden p-0" + className="!max-w-[720px] translate-y-0 max-h-[80vh] overflow-hidden p-0"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx` at line 104, The className string on the CommandPalette component contains a redundant responsive override; remove the duplicated "sm:!max-w-[720px]" from the className (leave "!max-w-[720px]" intact) so the base max-width value isn't needlessly overridden; update the className in CommandPalette.tsx where the className is defined to drop the "sm:!max-w-[720px]" token.
105-105: 💤 Low valueConsider documenting the magic number.
The hardcoded
278pxin the vertical positioning calculation appears to be approximately half the expected dialog height for centering. A brief comment would help future maintainers understand this offset.📝 Add clarifying comment
+ {/* Position dialog near top-center: 278px ≈ half the typical dialog height */} style={{ top: "max(16px, calc(50% - 278px))" }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx` at line 105, The inline style on the CommandPalette component uses a magic number "278px" in style={{ top: "max(16px, calc(50% - 278px))" }}; add a short clarifying comment next to this expression (or extract to a named constant like DIALOG_HALF_HEIGHT) explaining that 278px represents half the dialog's intended height used to vertically center the palette and why the lower bound 16px is required, so future maintainers understand the offset and can update it if the dialog size changes.apps/desktop/src/renderer/commandPalette/ui/SubPaletteView/SubPaletteView.tsx (1)
25-25: 💤 Low valueConsider memoizing the filtered children.
The
visiblearray is recomputed on every render. Since the filtering depends onchildren(memoized) andcontext(from provider), wrapping this in a separateuseMemowould avoid redundant filter operations.♻️ Memoize the filtering step
- const visible = children.filter((c) => (c.when ? c.when(context) : true)); + const visible = useMemo( + () => children.filter((c) => (c.when ? c.when(context) : true)), + [children, context] + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/ui/SubPaletteView/SubPaletteView.tsx` at line 25, The visible computation is re-run on every render; wrap the filter in a useMemo to memoize it: replace the direct assignment to visible with a useMemo that returns children.filter((c) => (c.when ? c.when(context) : true)) and set the dependency array to [children, context] (ensure useMemo is imported), referencing the visible variable and the children and context used in SubPaletteView to avoid unnecessary recalculations.apps/desktop/src/renderer/commandPalette/ui/QuickOpen/quickOpenStore.ts (1)
18-18: ⚡ Quick winInconsistent state cleanup in
closeaction.The
closeaction setsopen: falsebut does not resettargettonull. This differs from the pattern used in other intent stores in this PR:
delete-workspace-intent.tsline 18:close: () => set({ target: null })remove-workspace-from-sidebar-intent.ts:clear()setstarget: nullset-preferred-open-in-app-intent.ts:clear()setstarget: nullLeaving a stale
targetafter close may cause issues if consumers checktargetwithout also checkingopen.🔄 Proposed fix to align with other intent stores
- close: () => set({ open: false }), + close: () => set({ open: false, target: null }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/ui/QuickOpen/quickOpenStore.ts` at line 18, The close action currently only sets open: false leaving a stale target; update the close handler in quickOpenStore (the close: () => set({...}) action) to also reset target to null (e.g., set({ open: false, target: null })) so its cleanup matches other intent stores (delete-workspace-intent, remove-workspace-from-sidebar-intent, set-preferred-open-in-app-intent) and prevents consumers from relying on a stale target after close.apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx (1)
90-96: 🏗️ Heavy liftIncomplete implementation:
linkTaskToWorkspaceis a no-op stub.The function currently discards both parameters without performing any linking operation. However, line 64 shows a success toast to the user ("Linked ${slug} to workspace"), creating misleading feedback when no actual linking occurs.
Do you want me to help implement the actual linking logic, or is this intentionally stubbed for future work? If it's future work, consider either:
- Removing the success toast until the implementation is complete
- Adding a TODO comment and tracking issue reference
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx` around lines 90 - 96, The function linkTaskToWorkspace is currently a no-op stub that discards taskId and workspaceId while a success toast ("Linked ${slug} to workspace") is shown elsewhere; implement real linking by replacing the void stubs in linkTaskToWorkspace with an async call to the appropriate service/API/store (e.g., call your backend endpoint or task-store method) to associate the taskId with workspaceId, await the response, and throw or return errors on failure; ensure the success toast is only shown after a successful response and that errors are caught and surfaced (e.g., show an error toast or rethrow), or if you intend to leave it unimplemented, remove the premature success toast and add a TODO with an issue reference in linkTaskToWorkspace instead.apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts (1)
19-22: ⚡ Quick winPrefer updater form of
setfor consistency and idiomatic Zustand.The current implementation uses
get()followed byset(). Using the updater form ofsetis more idiomatic in Zustand and matches the pattern used inright-sidebar-toggle-intent.ts.♻️ Refactor to use updater form
request: (target) => { - const prevTick = get().target?.tick ?? 0; - set({ target: { ...target, tick: prevTick + 1 } }); + set((state) => ({ + target: { ...target, tick: (state.target?.tick ?? 0) + 1 } + })); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts` around lines 19 - 22, The request handler currently uses get() then set(), which is non-idiomatic for Zustand; change it to the updater form of set by calling set(state => ({ target: { ...target, tick: (state.target?.tick ?? 0) + 1 } })); this ensures you read the previous tick from the state snapshot atomically and matches the pattern used in right-sidebar-toggle-intent (use set with a function, reference the existing state.target?.tick, and increment it by 1 while spreading the incoming target).apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts (1)
19-22: ⚡ Quick winPrefer updater form of
setfor consistency and idiomatic Zustand.The current implementation uses
get()followed byset(). Using the updater form ofsetis more idiomatic in Zustand and matches the pattern used inright-sidebar-toggle-intent.ts.♻️ Refactor to use updater form
request: (target) => { - const prevTick = get().target?.tick ?? 0; - set({ target: { ...target, tick: prevTick + 1 } }); + set((state) => ({ + target: { ...target, tick: (state.target?.tick ?? 0) + 1 } + })); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts` around lines 19 - 22, The request handler in remove-workspace-from-sidebar-intent currently calls get() then set(), so change it to the updater form of set used elsewhere (e.g., right-sidebar-toggle-intent.ts): call set with a function that receives the current state and returns the new state, compute prevTick from state.target?.tick ?? 0 and set target to { ...target, tick: prevTick + 1 } inside that returned object; keep the same property names (request, target) so only the set/get pattern is replaced with set(state => ({ ...state, target: { ...target, tick: prevTick + 1 } })).apps/desktop/src/renderer/commandPalette/core/types.ts (1)
35-37: ⚡ Quick winConsider documenting the precedence between
run,children, andrenderFrame.The
Commandinterface allows all three properties (run,children,renderFrame) to be defined simultaneously. While this provides flexibility, it's unclear which takes precedence during execution. Consider adding a JSDoc comment clarifying the intended behavior when multiple properties are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/core/types.ts` around lines 35 - 37, The Command interface in types.ts currently allows run, children, and renderFrame to coexist without documented precedence; add a clear JSDoc comment on the Command interface (or directly above the run/children/renderFrame properties) that states the execution/rendering order and which property wins when multiple are present (e.g., "If run is present it will be executed on activation; otherwise children will be used to expand subcommands; renderFrame is only used for custom rendering and does not affect activation"). Reference the Command interface and the run, children, and renderFrame property names in the comment so future readers know the intended behavior and precedence.apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx (2)
39-49: ⚡ Quick winConsider skipping the query when projectId is null.
Similar to the workspace query, this executes with an empty string filter when
projectIdis null. Skipping the query would be more efficient.⚡ Proposed optimization
const { data: preferredAppRows = [] } = useLiveQuery( (q) => + projectId === null + ? [] + : q .from({ sp: collections.v2SidebarProjects }) - .where(({ sp }) => eq(sp.projectId, projectId ?? "")) + .where(({ sp }) => eq(sp.projectId, projectId)) .select(({ sp }) => ({ defaultOpenInApp: sp.defaultOpenInApp })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx` around lines 39 - 49, The query for preferredAppRows runs even when projectId is null, causing an unnecessary filter with an empty string; update the useLiveQuery call in ContextProvider (the block using useLiveQuery, collections.v2SidebarProjects, projectId and preferredAppRows) to short-circuit and return an empty array when projectId is null/undefined—i.e., only call useLiveQuery (or pass the query function) when projectId is truthy, otherwise set preferredAppRows to [] so preferredOpenInApp logic remains unchanged.
22-35: ⚡ Quick winConsider skipping the query when workspaceId is null.
The query executes unconditionally with
eq(workspaces.id, v2WorkspaceId ?? ""), which means it runs a query filtering on an empty string whenv2WorkspaceIdis null. While this returns empty results, it's more efficient to skip the query entirely.⚡ Proposed optimization
const { data: v2WorkspaceRows = [] } = useLiveQuery( (q) => + v2WorkspaceId === null + ? [] + : q .from({ workspaces: collections.v2Workspaces }) - .where(({ workspaces }) => eq(workspaces.id, v2WorkspaceId ?? "")) + .where(({ workspaces }) => eq(workspaces.id, v2WorkspaceId)) .select(({ workspaces }) => ({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx` around lines 22 - 35, Summary: Skip running the Dexie useLiveQuery when v2WorkspaceId is null to avoid executing a query filtered by an empty string. Fix: inside the useLiveQuery callback (the function passed to useLiveQuery that currently builds q.from({ workspaces: collections.v2Workspaces })...), add an early guard like if (!v2WorkspaceId) return [] to return an empty array immediately; keep the dependency array [collections, v2WorkspaceId] and leave the rest of the query (where + select) unchanged so the query only executes when v2WorkspaceId is truthy.apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts (1)
107-107: ⚡ Quick winConsider using
asynckeyword for consistency and error handling.The
runhandler at line 107 returns a promise fromopenInwithout theasynckeyword, while the submenu handlers (line 86-94) useasync/await. This inconsistency could lead to unhandled promise rejections if the execute handler doesn't properly catch errors from non-async functions that return promises.Proposed fix for consistency
-run: (ctx) => openIn(ctx, preferredOption.id), +run: async (ctx) => { + await openIn(ctx, preferredOption.id); +},Alternatively, if synchronous wrapping is preferred:
-run: (ctx) => openIn(ctx, preferredOption.id), +run: (ctx) => void openIn(ctx, preferredOption.id),The first option is recommended for consistent error surfacing via the execute handler's catch block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts` at line 107, The run handler currently returns the promise from openIn(ctx, preferredOption.id) without using async/await; update the run handler (the function assigned to run where preferredOption.id is used) to be async and await the call to openIn so errors are propagated consistently like the submenu handlers (i.e., change the run function to an async function that awaits openIn(ctx, preferredOption.id)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx`:
- Line 56: The route.params is currently hardcoded to {} in ContextProvider.tsx;
update the CommandContext route construction to derive params from the router
match returned by useMatchRoute (the v2Match object) instead of the empty
object. Locate where route: { pathname: location.pathname, params: {} } is
created and replace params with the actual match params (e.g., v2Match?.params
or an empty fallback) so dynamic route segments are passed into CommandContext
and commands can access them.
In `@apps/desktop/src/renderer/commandPalette/core/execute.ts`:
- Around line 9-10: Analytics "command_run" is being sent before verifying the
command is executable; move or conditionally invoke track so it only runs when
command.run exists—i.e., check command.run (the run handler on the command
object) first and then call track("command_run", { commandId: command.id,
section: command.section }) immediately before executing command.run (or inside
the branch that calls command.run) to avoid recording events for non-executable
commands such as those with only children or renderFrame.
In `@apps/desktop/src/renderer/commandPalette/core/types.ts`:
- Line 10: The route params type is too strict: change the declaration of params
in the command palette route types (the params field in types.ts) from
Record<string, string> to allow undefined values produced by TanStack Router —
e.g., use Record<string, string | undefined> (or an equivalent optional/partial
mapping) so optional or missing route params from matchRoute type-check
correctly when building context.
In `@apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx`:
- Around line 37-39: The query key passed to
electronQueryClient.invalidateQueries is incorrectly nested as a double array
which prevents matching the tRPC cache key; update the call in commands.tsx that
calls electronQueryClient.invalidateQueries (the same call that targets
"settings","getNotificationSoundsMuted") to use a flat array key matching
utils.settings.getNotificationSoundsMuted.invalidate() — i.e., replace the
nested [["settings","getNotificationSoundsMuted"]] with a single-level
["settings","getNotificationSoundsMuted"] so the query is properly invalidated.
In `@apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx`:
- Line 36: Replace the direct window.open("https://docs.superset.sh", "_blank",
"noreferrer") call with the tRPC mutation openUrl: import/use the existing trpc
client and call the openUrl mutation with the URL (e.g., trpc.openUrl.mutate({
url: "https://docs.superset.sh" })) instead of window.open, remove the erroneous
third parameter, and await/handle the promise or errors appropriately where this
is invoked (replace the window.open call in commands.tsx).
In
`@apps/desktop/src/renderer/commandPalette/ui/CommandListView/CommandListView.tsx`:
- Line 17: The CommandEmpty usage renders a non-selectable message; update the
CommandEmpty component's className to include the required select-text and
cursor-text classes so the text becomes user-selectable and shows the text
cursor; locate the CommandEmpty definition in the ui component (symbol:
CommandEmpty in packages/ui/src/components/ui/command.tsx) and append
"select-text cursor-text" to its existing className ("py-6 text-center
text-sm"), and also ensure any instance in CommandListView (symbol:
CommandListView in
apps/desktop/src/renderer/commandPalette/ui/CommandListView/CommandListView.tsx)
reflects this change if it overrides or supplies classes.
In
`@apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx`:
- Line 115: In the className string in CommandPalette (the JSX element in
CommandPalette.tsx) remove the invalid Tailwind prefix
"**:data-[slot=command-input-wrapper]:h-12" and replace it with the correct
arbitrary variant "[&_[data-slot=command-input-wrapper]]:h-12" so the intended
height style on the command-input-wrapper descendant is applied; update the
className entry near the existing bracketed selectors (the same string that
contains [&_[cmdk-group-heading]] and [&_[cmdk-item]]) to use the corrected
selector.
---
Nitpick comments:
In `@apps/desktop/src/renderer/commandPalette/CommandPaletteHost.tsx`:
- Around line 29-33: The small helper component CommandPaletteTrigger should be
inlined into CommandPaletteHost: remove the separate CommandPaletteTrigger
function and move its logic into CommandPaletteHost by calling
useFrameStackStore((s) => s.setOpen) and useHotkey("OPEN_COMMAND_PALETTE", () =>
setOpen(true)) directly inside CommandPaletteHost (ensuring hooks remain at the
top level of the component), then delete the unused CommandPaletteTrigger
export/definition to keep a single-component-per-file and improve locality.
In `@apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx`:
- Around line 39-49: The query for preferredAppRows runs even when projectId is
null, causing an unnecessary filter with an empty string; update the
useLiveQuery call in ContextProvider (the block using useLiveQuery,
collections.v2SidebarProjects, projectId and preferredAppRows) to short-circuit
and return an empty array when projectId is null/undefined—i.e., only call
useLiveQuery (or pass the query function) when projectId is truthy, otherwise
set preferredAppRows to [] so preferredOpenInApp logic remains unchanged.
- Around line 22-35: Summary: Skip running the Dexie useLiveQuery when
v2WorkspaceId is null to avoid executing a query filtered by an empty string.
Fix: inside the useLiveQuery callback (the function passed to useLiveQuery that
currently builds q.from({ workspaces: collections.v2Workspaces })...), add an
early guard like if (!v2WorkspaceId) return [] to return an empty array
immediately; keep the dependency array [collections, v2WorkspaceId] and leave
the rest of the query (where + select) unchanged so the query only executes when
v2WorkspaceId is truthy.
In `@apps/desktop/src/renderer/commandPalette/core/types.ts`:
- Around line 35-37: The Command interface in types.ts currently allows run,
children, and renderFrame to coexist without documented precedence; add a clear
JSDoc comment on the Command interface (or directly above the
run/children/renderFrame properties) that states the execution/rendering order
and which property wins when multiple are present (e.g., "If run is present it
will be executed on activation; otherwise children will be used to expand
subcommands; renderFrame is only used for custom rendering and does not affect
activation"). Reference the Command interface and the run, children, and
renderFrame property names in the comment so future readers know the intended
behavior and precedence.
In `@apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts`:
- Line 107: The run handler currently returns the promise from openIn(ctx,
preferredOption.id) without using async/await; update the run handler (the
function assigned to run where preferredOption.id is used) to be async and await
the call to openIn so errors are propagated consistently like the submenu
handlers (i.e., change the run function to an async function that awaits
openIn(ctx, preferredOption.id)).
In
`@apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx`:
- Line 104: The className string on the CommandPalette component contains a
redundant responsive override; remove the duplicated "sm:!max-w-[720px]" from
the className (leave "!max-w-[720px]" intact) so the base max-width value isn't
needlessly overridden; update the className in CommandPalette.tsx where the
className is defined to drop the "sm:!max-w-[720px]" token.
- Line 105: The inline style on the CommandPalette component uses a magic number
"278px" in style={{ top: "max(16px, calc(50% - 278px))" }}; add a short
clarifying comment next to this expression (or extract to a named constant like
DIALOG_HALF_HEIGHT) explaining that 278px represents half the dialog's intended
height used to vertically center the palette and why the lower bound 16px is
required, so future maintainers understand the offset and can update it if the
dialog size changes.
In `@apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx`:
- Around line 90-96: The function linkTaskToWorkspace is currently a no-op stub
that discards taskId and workspaceId while a success toast ("Linked ${slug} to
workspace") is shown elsewhere; implement real linking by replacing the void
stubs in linkTaskToWorkspace with an async call to the appropriate
service/API/store (e.g., call your backend endpoint or task-store method) to
associate the taskId with workspaceId, await the response, and throw or return
errors on failure; ensure the success toast is only shown after a successful
response and that errors are caught and surfaced (e.g., show an error toast or
rethrow), or if you intend to leave it unimplemented, remove the premature
success toast and add a TODO with an issue reference in linkTaskToWorkspace
instead.
In `@apps/desktop/src/renderer/commandPalette/ui/QuickOpen/quickOpenStore.ts`:
- Line 18: The close action currently only sets open: false leaving a stale
target; update the close handler in quickOpenStore (the close: () => set({...})
action) to also reset target to null (e.g., set({ open: false, target: null }))
so its cleanup matches other intent stores (delete-workspace-intent,
remove-workspace-from-sidebar-intent, set-preferred-open-in-app-intent) and
prevents consumers from relying on a stale target after close.
In
`@apps/desktop/src/renderer/commandPalette/ui/SubPaletteView/SubPaletteView.tsx`:
- Line 25: The visible computation is re-run on every render; wrap the filter in
a useMemo to memoize it: replace the direct assignment to visible with a useMemo
that returns children.filter((c) => (c.when ? c.when(context) : true)) and set
the dependency array to [children, context] (ensure useMemo is imported),
referencing the visible variable and the children and context used in
SubPaletteView to avoid unnecessary recalculations.
In `@apps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.ts`:
- Around line 19-22: The request handler in remove-workspace-from-sidebar-intent
currently calls get() then set(), so change it to the updater form of set used
elsewhere (e.g., right-sidebar-toggle-intent.ts): call set with a function that
receives the current state and returns the new state, compute prevTick from
state.target?.tick ?? 0 and set target to { ...target, tick: prevTick + 1 }
inside that returned object; keep the same property names (request, target) so
only the set/get pattern is replaced with set(state => ({ ...state, target: {
...target, tick: prevTick + 1 } })).
In `@apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts`:
- Around line 19-22: The request handler currently uses get() then set(), which
is non-idiomatic for Zustand; change it to the updater form of set by calling
set(state => ({ target: { ...target, tick: (state.target?.tick ?? 0) + 1 } }));
this ensures you read the previous tick from the state snapshot atomically and
matches the pattern used in right-sidebar-toggle-intent (use set with a
function, reference the existing state.target?.tick, and increment it by 1 while
spreading the incoming target).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3de20f6c-cb95-4796-b15d-5bbc1966c6e3
📒 Files selected for processing (41)
apps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/commandPalette/CommandPaletteHost.tsxapps/desktop/src/renderer/commandPalette/core/ContextProvider.tsxapps/desktop/src/renderer/commandPalette/core/execute.tsapps/desktop/src/renderer/commandPalette/core/frames.tsapps/desktop/src/renderer/commandPalette/core/registry.tsapps/desktop/src/renderer/commandPalette/core/sections.tsapps/desktop/src/renderer/commandPalette/core/types.tsapps/desktop/src/renderer/commandPalette/core/useActiveCommands.tsapps/desktop/src/renderer/commandPalette/index.tsapps/desktop/src/renderer/commandPalette/modules/actions/commands.tsxapps/desktop/src/renderer/commandPalette/modules/index.tsapps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsxapps/desktop/src/renderer/commandPalette/modules/openIn/commands.tsapps/desktop/src/renderer/commandPalette/modules/settings/commands.tsapps/desktop/src/renderer/commandPalette/modules/workspace/commands.tsxapps/desktop/src/renderer/commandPalette/ui/CommandItemRow/CommandItemRow.tsxapps/desktop/src/renderer/commandPalette/ui/CommandListView/CommandListView.tsxapps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsxapps/desktop/src/renderer/commandPalette/ui/DeleteWorkspaceMount/DeleteWorkspaceMount.tsxapps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsxapps/desktop/src/renderer/commandPalette/ui/QuickOpen/quickOpenStore.tsapps/desktop/src/renderer/commandPalette/ui/RecentlyViewed/RecentlyViewedFrame.tsxapps/desktop/src/renderer/commandPalette/ui/RemoveFromSidebarMount/RemoveFromSidebarMount.tsxapps/desktop/src/renderer/commandPalette/ui/SetPreferredOpenInAppMount/SetPreferredOpenInAppMount.tsxapps/desktop/src/renderer/commandPalette/ui/SubPaletteView/SubPaletteView.tsxapps/desktop/src/renderer/commandPalette/ui/ThemeFrame/ThemeFrame.tsxapps/desktop/src/renderer/components/ThemeSwatch/ThemeSwatch.tsxapps/desktop/src/renderer/components/ThemeSwatch/index.tsapps/desktop/src/renderer/hotkeys/registry.tsapps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/lib/router-instance.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsxapps/desktop/src/renderer/stores/delete-workspace-intent.tsapps/desktop/src/renderer/stores/remove-workspace-from-sidebar-intent.tsapps/desktop/src/renderer/stores/right-sidebar-toggle-intent.tsapps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.tspackages/ui/src/components/ui/command.tsx
|
|
||
| const context = useMemo<CommandContext>( | ||
| () => ({ | ||
| route: { pathname: location.pathname, params: {} }, |
There was a problem hiding this comment.
Route params should be derived from the router match.
The params field is hardcoded to an empty object {}, but the route match from useMatchRoute can provide actual route parameters. This causes the CommandContext.route.params to always be empty, preventing commands from accessing dynamic route segments.
🔧 Proposed fix to derive params from v2Match
const context = useMemo<CommandContext>(
() => ({
- route: { pathname: location.pathname, params: {} },
+ route: {
+ pathname: location.pathname,
+ params: v2Match !== false ? v2Match : {}
+ },
workspace: v2Workspace📝 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.
| route: { pathname: location.pathname, params: {} }, | |
| route: { | |
| pathname: location.pathname, | |
| params: v2Match !== false ? v2Match : {} | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx` at line
56, The route.params is currently hardcoded to {} in ContextProvider.tsx; update
the CommandContext route construction to derive params from the router match
returned by useMatchRoute (the v2Match object) instead of the empty object.
Locate where route: { pathname: location.pathname, params: {} } is created and
replace params with the actual match params (e.g., v2Match?.params or an empty
fallback) so dynamic route segments are passed into CommandContext and commands
can access them.
| track("command_run", { commandId: command.id, section: command.section }); | ||
| if (!command.run) return; |
There was a problem hiding this comment.
Analytics event may track non-executable commands.
The analytics event is recorded on line 9 before checking whether command.run exists on line 10. This means commands that have no run handler (e.g., commands with only children or renderFrame) will still emit a "command_run" event, which may inflate analytics counts or misrepresent user behavior.
📊 Proposed fix to track only executable commands
export async function executeCommand(
command: Command,
context: CommandContext,
): Promise<void> {
- track("command_run", { commandId: command.id, section: command.section });
if (!command.run) return;
+ track("command_run", { commandId: command.id, section: command.section });
try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/commandPalette/core/execute.ts` around lines 9 -
10, Analytics "command_run" is being sent before verifying the command is
executable; move or conditionally invoke track so it only runs when command.run
exists—i.e., check command.run (the run handler on the command object) first and
then call track("command_run", { commandId: command.id, section: command.section
}) immediately before executing command.run (or inside the branch that calls
command.run) to avoid recording events for non-executable commands such as those
with only children or renderFrame.
| export interface CommandContext { | ||
| route: { | ||
| pathname: string; | ||
| params: Record<string, string>; |
There was a problem hiding this comment.
Route params type may be too strict.
params: Record<string, string> requires all param values to be strings, but Tanstack Router can produce undefined values for optional or missing params. This may cause type mismatches when building the context from matchRoute.
🔧 Proposed fix
- params: Record<string, string>;
+ params: Record<string, string | undefined>;📝 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.
| params: Record<string, string>; | |
| params: Record<string, string | undefined>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/commandPalette/core/types.ts` at line 10, The route
params type is too strict: change the declaration of params in the command
palette route types (the params field in types.ts) from Record<string, string>
to allow undefined values produced by TanStack Router — e.g., use Record<string,
string | undefined> (or an equivalent optional/partial mapping) so optional or
missing route params from matchRoute type-check correctly when building context.
| await electronQueryClient.invalidateQueries({ | ||
| queryKey: [["settings", "getNotificationSoundsMuted"]], | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for similar invalidateQueries patterns in the codebase
rg -n -C3 'invalidateQueries.*queryKey.*settings' --type=tsx --type=tsRepository: superset-sh/superset
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for invalidateQueries calls in the codebase
rg -n -C3 'invalidateQueries' apps/desktop/src --max-count=20Repository: superset-sh/superset
Length of output: 13780
🏁 Script executed:
#!/bin/bash
# Find tRPC router definitions for settings
fd -e ts -e tsx | xargs rg -l 'settings.*getNotificationSoundsMuted' | head -10Repository: superset-sh/superset
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Look at the file in question to see the full context
cat -n apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx | head -50Repository: superset-sh/superset
Length of output: 1963
🏁 Script executed:
#!/bin/bash
# Find tRPC router definition for settings
rg -n 'settings' apps/desktop/src/lib/trpc -A 5 -B 5 | head -100Repository: superset-sh/superset
Length of output: 9560
🏁 Script executed:
#!/bin/bash
# Search for useQuery calls with getNotificationSoundsMuted
rg -n 'getNotificationSoundsMuted' apps/desktop/srcRepository: superset-sh/superset
Length of output: 1786
🏁 Script executed:
#!/bin/bash
# Look for tRPC settings router definition
fd -e ts -e tsx apps/desktop/src | xargs rg -l 'createTRPCRouter.*settings|settings.*createTRPCRouter' 2>/dev/nullRepository: superset-sh/superset
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Check the settings router definition
sed -n '840,870p' apps/desktop/src/lib/trpc/routers/settings/index.tsRepository: superset-sh/superset
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Look at how other procedures in the same router are used with invalidateQueries
rg -B 2 -A 2 'utils\.settings\.' apps/desktop/src/renderer --max-count=10Repository: superset-sh/superset
Length of output: 39128
🏁 Script executed:
#!/bin/bash
# Check how RingtonesSettings uses the tRPC utils for getNotificationSoundsMuted
sed -n '140,155p' apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsxRepository: superset-sh/superset
Length of output: 624
Fix the tRPC query key nesting level.
The query key at line 38 has an extra level of array nesting. Change:
queryKey: [["settings", "getNotificationSoundsMuted"]],
to:
queryKey: ["settings", "getNotificationSoundsMuted"],
The tRPC utilities (e.g., utils.settings.getNotificationSoundsMuted.invalidate()) use a flat array key. When calling queryClient.invalidateQueries() directly, the key format must match. The double-nested array will prevent the query from being invalidated, leaving stale data in the cache.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx` around
lines 37 - 39, The query key passed to electronQueryClient.invalidateQueries is
incorrectly nested as a double array which prevents matching the tRPC cache key;
update the call in commands.tsx that calls electronQueryClient.invalidateQueries
(the same call that targets "settings","getNotificationSoundsMuted") to use a
flat array key matching utils.settings.getNotificationSoundsMuted.invalidate() —
i.e., replace the nested [["settings","getNotificationSoundsMuted"]] with a
single-level ["settings","getNotificationSoundsMuted"] so the query is properly
invalidated.
| section: "navigation", | ||
| icon: BookOpenIcon, | ||
| run: () => { | ||
| window.open("https://docs.superset.sh", "_blank", "noreferrer"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that Electron shell API is available and preferred for external URLs
rg -n 'shell\.(openExternal|openPath)' --type=ts -C2Repository: superset-sh/superset
Length of output: 7312
🏁 Script executed:
#!/bin/bash
# First, verify the actual code at the specified location
echo "=== Checking the actual file content ==="
cat -n apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx | head -50Repository: superset-sh/superset
Length of output: 1599
🏁 Script executed:
#!/bin/bash
# Check the tRPC external router to see what's available
echo "=== Checking tRPC external router ==="
cat -n apps/desktop/src/lib/trpc/routers/external/index.ts | head -150Repository: superset-sh/superset
Length of output: 5082
Use tRPC's openUrl mutation instead of window.open() for opening external URLs.
This code violates the Electron IPC guideline: renderer process communication must use tRPC. Additionally, the third parameter "noreferrer" has no effect in window.open() — the third parameter expects window features (like "width=500,height=400"), not a rel attribute, so this will be silently ignored.
A tRPC route for this already exists:
run: () => {
- window.open("https://docs.superset.sh", "_blank", "noreferrer");
+ void electronTrpcClient.external.openUrl.mutate("https://docs.superset.sh");
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx` at
line 36, Replace the direct window.open("https://docs.superset.sh", "_blank",
"noreferrer") call with the tRPC mutation openUrl: import/use the existing trpc
client and call the openUrl mutation with the URL (e.g., trpc.openUrl.mutate({
url: "https://docs.superset.sh" })) instead of window.open, remove the erroneous
third parameter, and await/handle the promise or errors appropriately where this
is invoked (replace the window.open call in commands.tsx).
|
|
||
| return ( | ||
| <CommandList> | ||
| <CommandEmpty>No commands found.</CommandEmpty> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if CommandEmpty component includes select-text cursor-text classes
# Search for CommandEmpty definition in ui package
ast-grep --pattern $'const CommandEmpty = $$$'
# Also search with forwardRef pattern
ast-grep --pattern $'export const CommandEmpty = forwardRef($$$)'
# Check className assignments in command.tsx
rg -n "CommandEmpty" --type=tsx --type=ts -A 5 -B 2 packages/ui/src/components/ui/command.tsxRepository: superset-sh/superset
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Find the command component files
fd -e ts -e tsx -e js -e jsx "command" packages/ui/src/components
# Search for CommandEmpty definition
rg -n "CommandEmpty" packages/ui/src/components --type ts --type js -A 10 -B 2
# Also check if there's a command.tsx or command.ts file
find packages/ui/src/components -name "*command*"Repository: superset-sh/superset
Length of output: 6542
Add select-text cursor-text classes to CommandEmpty component.
The CommandEmpty component in packages/ui/src/components/ui/command.tsx (line 110) currently has className="py-6 text-center text-sm" but is missing the required select-text cursor-text classes. Since this component is used in the desktop renderer, the message must be user-selectable per coding guidelines. Update the className to include these required classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/commandPalette/ui/CommandListView/CommandListView.tsx`
at line 17, The CommandEmpty usage renders a non-selectable message; update the
CommandEmpty component's className to include the required select-text and
cursor-text classes so the text becomes user-selectable and shows the text
cursor; locate the CommandEmpty definition in the ui component (symbol:
CommandEmpty in packages/ui/src/components/ui/command.tsx) and append
"select-text cursor-text" to its existing className ("py-6 text-center
text-sm"), and also ensure any instance in CommandListView (symbol:
CommandListView in
apps/desktop/src/renderer/commandPalette/ui/CommandListView/CommandListView.tsx)
reflects this change if it overrides or supplies classes.
| </DialogHeader> | ||
| <Command | ||
| shouldFilter={!currentFrame || !currentFrame.command.renderFrame} | ||
| className="[&_[cmdk-group-heading]]:text-muted-foreground **:data-[slot=command-input-wrapper]:h-12 [&_[cmdk-group-heading]]:px-2 [&_[cmdk-group-heading]]:font-medium [&_[cmdk-group]]:px-2 [&_[cmdk-group]:not([hidden])_~[cmdk-group]]:pt-0 [&_[cmdk-input-wrapper]_svg]:h-5 [&_[cmdk-input-wrapper]_svg]:w-5 [&_[cmdk-input]]:h-12 [&_[cmdk-item]]:px-2 [&_[cmdk-item]]:py-3 [&_[cmdk-item]_svg]:h-5 [&_[cmdk-item]_svg]:w-5 [&_[cmdk-list]]:max-h-[min(500px,calc(80vh-3rem))]" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if **: is a valid Tailwind prefix or if it should be [&_
# Search for other uses of **: prefix in the codebase
rg -n '\*\*:' --type=tsx --type=ts
# Search for similar command-input-wrapper styling patterns
rg -n 'command-input-wrapper' --type=tsx --type=ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Search for **: prefix pattern without file type specification
rg '\*\*:' -A 2 -B 2
# Also search for command-input-wrapper to find usage patterns
rg 'command-input-wrapper' -A 2 -B 2Repository: superset-sh/superset
Length of output: 50379
🏁 Script executed:
#!/bin/bash
# Read the CommandPalette.tsx file to see the exact code
cat -n apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx | sed -n '110,120p'Repository: superset-sh/superset
Length of output: 902
🌐 Web query:
Tailwind CSS **: prefix syntax validity and meaning
💡 Result:
Tailwind CSS “: prefix” syntax depends on what you mean by “prefix”: 1) “Tailwind class name prefixes” (like tw:, kz:) — validity and meaning In Tailwind CSS v4, configuring a prefix is done via import syntax, and the prefix behaves like a variant that is applied at the beginning of the class name. Tailwind’s docs show: @import "tailwindcss" prefix(tw);, which makes generated utility classes require the prefix when used in HTML (for example, the docs show classes like tw:flex and tw:bg-red-500). [1] In other words, a “tw:”-style prefix is valid Tailwind v4 syntax, and it means “only match/use Tailwind utilities when the class is prefixed with tw:”. The Tailwind docs also note that these prefixes work “a little differently than in v3” and “look like variants and are always at the beginning of the class name.” [1] Additionally, the prefix is applied to generated CSS variables as well (Tailwind generates --tw-… variables). [1] Tailwind also restricts what characters are allowed in the prefix; the Tailwind team’s implementation notes mention “Only alpha (a-z) characters are allowed in a prefix”. [2] 2) The unrelated “variant prefixes” (sm:, md:, hover:, dark:) — validity and meaning Tailwind also uses “state/responsive prefixes” like hover: and sm:. Those are valid and mean the utility applies only when that variant condition matches (hover, breakpoint, dark mode, etc.). [3] 3) “Colon” usage inside arbitrary values/properties (e.g. [color:red], [property:value]) If by “: prefix syntax” you meant colon syntax inside arbitrary values/properties, that’s a different feature: Tailwind arbitrary properties use [property:value] syntax. [4] This is not a “class-name prefix” (like tw:), but the colon is part of the arbitrary CSS declaration inside square brackets. What NOT to do - Don’t confuse a “class-name prefix” like tw: (configured prefix/variant-like) with “arbitrary property syntax” [property:value]. They live in different parts of the syntax. - In Tailwind, the configured prefix should be set via @import "tailwindcss" prefix(...); (v4) rather than older v3-style config in a JS file. [5][1] Example: using a Tailwind v4 configured prefix If you import with prefix(tw): [1] @import "tailwindcss" prefix(tw); Then Tailwind utilities are written with the prefix at the beginning of the class: tw:flex, tw:bg-red-500, tw:hover:bg-red-600. [1] If you tell me exactly what string you’re trying to use (e.g. “tw:bg-white” vs “tw:[color:red]” vs something like “:bg-red-500”), I can confirm which Tailwind parsing rule applies and whether it should generate CSS.
Citations:
- 1: https://v3.tailwindcss.com/docs/v4-beta
- 2: Add support for prefixes tailwindlabs/tailwindcss#14501
- 3: https://tailwindcss.com/docs/styling-with-utility-classes
- 4: Add support for arbitrary properties tailwindlabs/tailwindcss#6161
- 5: https://stackoverflow.com/questions/79568691/tailwindcss-v4-does-not-generate-certain-styles-when-using-prefix
Remove **: prefix from the className; use [&_[data-slot=command-input-wrapper]]:h-12 instead.
The **:data-[slot=command-input-wrapper]:h-12 class uses **: which is not valid Tailwind syntax. Tailwind prefixes support only alpha characters (a–z), so the ** prefix violates this restriction and will be ignored, leaving the height rule unapplied. The intended selector should use the arbitrary variant form [&_[data-slot=command-input-wrapper]]:h-12 to target the descendant element.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/commandPalette/ui/CommandPalette/CommandPalette.tsx`
at line 115, In the className string in CommandPalette (the JSX element in
CommandPalette.tsx) remove the invalid Tailwind prefix
"**:data-[slot=command-input-wrapper]:h-12" and replace it with the correct
arbitrary variant "[&_[data-slot=command-input-wrapper]]:h-12" so the intended
height style on the command-input-wrapper descendant is applied; update the
className entry near the existing bracketed selectors (the same string that
contains [&_[cmdk-group-heading]] and [&_[cmdk-item]]) to use the corrected
selector.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 41 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx">
<violation number="1" location="apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx:90">
P1: Selecting a task never links it to the workspace because `linkTaskToWorkspace` is unimplemented (no-op), despite showing a success toast.</violation>
</file>
<file name="apps/desktop/src/renderer/commandPalette/modules/settings/commands.ts">
<violation number="1" location="apps/desktop/src/renderer/commandPalette/modules/settings/commands.ts:151">
P2: Handle navigation promise rejections instead of discarding them with `void`; failed navigations can become unhandled promise rejections.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts:16">
P1: `tick` is reset after `clear()`, which can cause subsequent intent requests to be ignored by the mount's `lastTickRef` guard.</violation>
</file>
<file name="apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts">
<violation number="1" location="apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts:78">
P2: The preferred "Open in …" command is dropped when `preferredOpenInApp` is a valid app that is not in `APP_OPTIONS`, which can disable the `OPEN_IN_APP` command/hotkey for those workspaces.</violation>
</file>
<file name="apps/desktop/src/renderer/hotkeys/registry.ts">
<violation number="1" location="apps/desktop/src/renderer/hotkeys/registry.ts:706">
P2: Windows/Linux `Open Command Palette` reuses `Ctrl+Shift+K`, which already maps to `Clear Terminal`, creating a conflicting hotkey when the terminal is focused.</violation>
</file>
<file name="apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx">
<violation number="1" location="apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx:52">
P3: `run` is unreachable when `renderFrame` is present because selection pushes a frame and exits early. Remove this callback or split the behavior into separate commands.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
| } | ||
|
|
||
| async function linkTaskToWorkspace( |
There was a problem hiding this comment.
P1: Selecting a task never links it to the workspace because linkTaskToWorkspace is unimplemented (no-op), despite showing a success toast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/commandPalette/ui/LinkTask/LinkTaskFrame.tsx, line 90:
<comment>Selecting a task never links it to the workspace because `linkTaskToWorkspace` is unimplemented (no-op), despite showing a success toast.</comment>
<file context>
@@ -0,0 +1,96 @@
+ );
+}
+
+async function linkTaskToWorkspace(
+ taskId: string,
+ workspaceId: string,
</file context>
| clear: () => void; | ||
| } | ||
|
|
||
| export const useSetPreferredOpenInAppIntent = |
There was a problem hiding this comment.
P1: tick is reset after clear(), which can cause subsequent intent requests to be ignored by the mount's lastTickRef guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/stores/set-preferred-open-in-app-intent.ts, line 16:
<comment>`tick` is reset after `clear()`, which can cause subsequent intent requests to be ignored by the mount's `lastTickRef` guard.</comment>
<file context>
@@ -0,0 +1,24 @@
+ clear: () => void;
+}
+
+export const useSetPreferredOpenInAppIntent =
+ create<SetPreferredOpenInAppIntentState>((set, get) => ({
+ target: null,
</file context>
| } | ||
|
|
||
| const preferredApp = context.workspace.preferredOpenInApp ?? "finder"; | ||
| const preferredOption = findOption(preferredApp); |
There was a problem hiding this comment.
P2: The preferred "Open in …" command is dropped when preferredOpenInApp is a valid app that is not in APP_OPTIONS, which can disable the OPEN_IN_APP command/hotkey for those workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/commandPalette/modules/openIn/commands.ts, line 78:
<comment>The preferred "Open in …" command is dropped when `preferredOpenInApp` is a valid app that is not in `APP_OPTIONS`, which can disable the `OPEN_IN_APP` command/hotkey for those workspaces.</comment>
<file context>
@@ -0,0 +1,122 @@
+ }
+
+ const preferredApp = context.workspace.preferredOpenInApp ?? "finder";
+ const preferredOption = findOption(preferredApp);
+
+ const submenuChildren: Command[] = APP_OPTIONS.map((option) => ({
</file context>
| OPEN_COMMAND_PALETTE: { | ||
| key: { | ||
| mac: L("meta+shift+k"), | ||
| windows: L("ctrl+shift+k"), |
There was a problem hiding this comment.
P2: Windows/Linux Open Command Palette reuses Ctrl+Shift+K, which already maps to Clear Terminal, creating a conflicting hotkey when the terminal is focused.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/hotkeys/registry.ts, line 706:
<comment>Windows/Linux `Open Command Palette` reuses `Ctrl+Shift+K`, which already maps to `Clear Terminal`, creating a conflicting hotkey when the terminal is focused.</comment>
<file context>
@@ -700,6 +700,16 @@ export const HOTKEYS_REGISTRY = {
+ OPEN_COMMAND_PALETTE: {
+ key: {
+ mac: L("meta+shift+k"),
+ windows: L("ctrl+shift+k"),
+ linux: L("ctrl+shift+k"),
+ },
</file context>
| section: "actions", | ||
| icon: PaletteIcon, | ||
| keywords: ["dark", "light", "appearance", "color"], | ||
| run: () => cycleTheme(), |
There was a problem hiding this comment.
P3: run is unreachable when renderFrame is present because selection pushes a frame and exits early. Remove this callback or split the behavior into separate commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx, line 52:
<comment>`run` is unreachable when `renderFrame` is present because selection pushes a frame and exits early. Remove this callback or split the behavior into separate commands.</comment>
<file context>
@@ -0,0 +1,119 @@
+ section: "actions",
+ icon: PaletteIcon,
+ keywords: ["dark", "light", "appearance", "color"],
+ run: () => cycleTheme(),
+ renderFrame: () => <ThemeFrame />,
+ },
</file context>
…context Command run handlers fire from inside React, so we can expose navigate through CommandContext (sourced from useNavigate in ContextProvider) instead of stashing the router on a module-level singleton. Also drops the agentConfig field from the Recently Viewed automations live query to track the agent-config -> agent column rename.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/commandPalette/modules/settings/commands.ts (1)
34-140: ⚡ Quick winConsider adding keywords to improve command palette search discoverability.
Many tabs currently lack keywords, which reduces search effectiveness in the command palette. Since search is a primary interaction mode, adding aliases and alternate terms would improve the user experience.
💡 Suggested keyword additions
{ id: "account", title: "Account", path: "/settings/account", icon: UserIcon, + keywords: ["profile", "user", "email"], },{ id: "billing", title: "Billing", path: "/settings/billing", icon: CreditCardIcon, + keywords: ["payment", "subscription", "invoice"], },{ id: "security", title: "Security", path: "/settings/security", icon: KeyRoundIcon, + keywords: ["auth", "authentication", "password", "2fa"], },{ id: "organization", title: "Organization", path: "/settings/organization", icon: BuildingIcon, + keywords: ["company", "workspace"], },{ id: "teams", title: "Teams", path: "/settings/teams", icon: UsersIcon }, + { id: "teams", title: "Teams", path: "/settings/teams", icon: UsersIcon, keywords: ["members", "collaborate", "users"] },Similar additions could be made for other tabs based on common user search terms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/commandPalette/modules/settings/commands.ts` around lines 34 - 140, The TABS array has many settings entries missing a keywords field which hurts command palette search; for each SettingsTab object lacking keywords (e.g., ids "account", "behavior", "terminal", "git", "experimental", "integrations", "organization", "teams", "links", "permissions", "hosts", "projects", "ringtones", "billing", "security", "agents", "presets") add a keywords: string[] with common aliases and search terms (e.g., "account" -> ["profile","user"], "behavior" -> ["prefs","preferences"], "terminal" -> ["cli","shell"], "git" -> ["version control","repo"], etc.) so each tab has relevant synonyms to improve discoverability in the command palette; update the TABS entries to include these keywords arrays where missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/renderer/commandPalette/modules/settings/commands.ts`:
- Around line 34-140: The TABS array has many settings entries missing a
keywords field which hurts command palette search; for each SettingsTab object
lacking keywords (e.g., ids "account", "behavior", "terminal", "git",
"experimental", "integrations", "organization", "teams", "links", "permissions",
"hosts", "projects", "ringtones", "billing", "security", "agents", "presets")
add a keywords: string[] with common aliases and search terms (e.g., "account"
-> ["profile","user"], "behavior" -> ["prefs","preferences"], "terminal" ->
["cli","shell"], "git" -> ["version control","repo"], etc.) so each tab has
relevant synonyms to improve discoverability in the command palette; update the
TABS entries to include these keywords arrays where missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 931ce4b7-8437-459d-9c0c-1905822e3acd
📒 Files selected for processing (6)
apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsxapps/desktop/src/renderer/commandPalette/core/types.tsapps/desktop/src/renderer/commandPalette/modules/actions/commands.tsxapps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsxapps/desktop/src/renderer/commandPalette/modules/settings/commands.tsapps/desktop/src/renderer/commandPalette/ui/RecentlyViewed/RecentlyViewedFrame.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/commandPalette/modules/navigation/commands.tsx
- apps/desktop/src/renderer/commandPalette/core/ContextProvider.tsx
- apps/desktop/src/renderer/commandPalette/core/types.ts
- apps/desktop/src/renderer/commandPalette/ui/RecentlyViewed/RecentlyViewedFrame.tsx
- apps/desktop/src/renderer/commandPalette/modules/actions/commands.tsx
1 participant
Summary
CommandProviderinterface, no central switchHistoryDropdownrendering), Open documentationArchitecture
apps/desktop/src/renderer/commandPalette/types,registry,ContextProvider,useActiveCommands,sections,execute,frames. Section order is resolved per route; commands carry an optionalwhenpredicate and eitherrun,children, orrenderFramefor sub-palettesworkspace,actions,openIn,navigation,settings. Each exports aCommandProviderCommandPaletteHostsubscribe to those intents and apply the mutation using the relevant hook — same pattern asuseQuickOpenStoreautoUpdate.checkInteractivetrpc proc so the palette + Help menu share one entry pointTest plan
_dashboard/*v2SidebarProjects.defaultOpenInAppso the top-level "Open in X" reflects the choice next timeThemeSwatchas Settings → Appearancesettings.setNotificationSoundsMuted(same setting as Ringtones page); title flips between Mute/UnmutesetRightSidebarOpenvia the right-sidebar intent storeHistoryDropdownSummary by cubic
Adds a global command palette (⌘⇧K / Ctrl+Shift+K) in the dashboard to speed up navigation and common actions using a module-based provider system with sub-palettes and contextual commands.
New Features
OPEN_COMMAND_PALETTE; mounted in the dashboard only.ThemeSwatch), Toggle left/right sidebars, Mute/Unmute notifications, Show keyboard shortcuts, Check for updates viaautoUpdate.checkInteractive.Refactors
CommandContext.navigate(fromuseNavigate) instead of a router singleton.CommandProvider; small intent stores drive workspace-local mutations (delete, remove from sidebar, right sidebar toggle, set preferred “open in” app).Written for commit 41aca77. Summary will update on new commits.
Summary by CodeRabbit