[LKPR-124] feat: Command Palette (⌘K) - #310
Merged
Merged
Conversation
added 2 commits
July 9, 2026 16:49
…ds.ts, hotkeys.ts; wire AppShell + TopBar
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a command palette for dashboard v2, with shared command data and constants, a global keyboard shortcut, a searchable overlay, and shell wiring to open and close it from the top bar or hotkey. ChangesCommand Palette
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/dashboard_v2/src/lib/components/overlays/CommandPalette.svelte`:
- Around line 133-141: The CommandPalette dialog currently exposes modal
semantics but does not keep keyboard focus inside the overlay. Update the
CommandPalette.svelte dialog handling around the open state and the palette-card
container so Tab/Shift+Tab are trapped within the command palette while it is
open, using the existing open/onClose flow and the dialog element marked with
role="dialog" and aria-modal="true". Ensure focus moves into the palette on open
and cannot escape to background content until onClose runs.
- Around line 46-105: Selection logic in CommandPalette.svelte is still using
filtered for Arrow/Enter and aria-activedescendant, which can drift from the
grouped UI order defined by renderItems. Add a command-only list derived from
renderItems (excluding headers) and use that list for activeIndex navigation,
Enter selection, and the active descendant reference. Keep the grouping/order
behavior from GROUP_ORDER, but make all keyboard focus and invocation paths
operate on the rendered command sequence so recent items or reordered groups
stay aligned.
In `@src/dashboard_v2/src/lib/hotkeys.ts`:
- Line 19: The `hotkeys.ts` platform check using `navigator.platform` may be
flagged by lint/CI even though it is acceptable for deciding between ⌘ and ⌃;
confirm whether there is a repo rule against deprecated Web APIs, and if so,
update the `isMac` detection logic in `hotkeys.ts` to the approved alternative
while keeping the same modifier-key behavior used by the hotkey handling code.
- Line 22: The hotkey check in the hotkeys handler is too strict, so the
shortcut fails when Caps Lock makes the key uppercase. Update the key comparison
in the hotkey listener logic (the branch that checks modifierHeld and e.key) to
normalize the pressed key before matching, so both 'k' and 'K' trigger the same
shortcut without changing the existing modifier requirement.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a46d407-35c8-4cf8-9dd0-73255d4cff48
📒 Files selected for processing (7)
src/dashboard_v2/src/lib/commands.test.tssrc/dashboard_v2/src/lib/commands.tssrc/dashboard_v2/src/lib/components/overlays/CommandPalette.sveltesrc/dashboard_v2/src/lib/components/shell/AppShell.sveltesrc/dashboard_v2/src/lib/components/shell/TopBar.sveltesrc/dashboard_v2/src/lib/hotkeys.test.tssrc/dashboard_v2/src/lib/hotkeys.ts
Contributor
Coverage report
The report is also available in the GitHub Actions job summary. |
…lette logic - Extract all SVG paths to constants/icons.ts (single source of truth) - Extract hotkey config to constants/keybindings.ts (key, modifier, display symbols) - routes.ts imports from icons.ts — no more inline path strings - commands.ts imports from icons.ts — removes local ICON_* duplicates - hotkeys.ts driven by COMMAND_PALETTE_HOTKEY config — no magic strings - CommandPalette.svelte: pure UI component; accepts commands as prop, removes goto/buildCommands coupling (no SvelteKit knowledge) - AppShell.svelte: owns command construction + goto wiring (composition root) - TopBar.svelte: derives modifier hint from keybinding config (⌘ vs Ctrl) stays in sync automatically if shortcut ever changes 48/48 tests pass, 0 build errors
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a global Command Palette (⌘K / Ctrl+K) for the dashboard_v2 SvelteKit app (LKPR-124). It adds a typed command registry, a global hotkey listener, and a centered modal overlay, then wires them into the existing app shell so the previously dead TopBar search button now opens the palette. It fits into the ongoing dashboard-v2 shell epic, reusing existing primitives (OverlayScrim, Icon, NAV_ROUTES, design tokens).
Changes:
- New
commands.tsregistry (buildCommands,filterCommands,GROUP_ORDER/GROUP_LABELS) with jump commands auto-generated fromNAV_ROUTESand two static actions. - New
CommandPalette.svelte(640px centered modal, live filter, ↑↓/Enter/Esc keyboard nav, grouped rendering) plushotkeys.tsglobal ⌘K/Ctrl+K listener; wired intoAppShell.svelteandTopBar.svelte. - Unit tests for
commands.ts(17) andhotkeys.ts(6); the component itself is not tested.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/dashboard_v2/src/lib/commands.ts |
New typed command registry; builds jump commands from NAV_ROUTES and static actions, plus filterCommands substring filter. |
src/dashboard_v2/src/lib/commands.test.ts |
Unit tests for command shape, unique IDs, jump/action wiring, and filtering. |
src/dashboard_v2/src/lib/hotkeys.ts |
Global keydown listener for ⌘K (Mac) / Ctrl+K; returns cleanup fn. |
src/dashboard_v2/src/lib/hotkeys.test.ts |
Tests modifier handling, negative cases, and cleanup. |
src/dashboard_v2/src/lib/components/overlays/CommandPalette.svelte |
New palette modal: input, grouped results, keyboard nav, empty state, footer hints. Selection indexing relies on filtered/flatIndex orders staying in sync. |
src/dashboard_v2/src/lib/components/shell/AppShell.svelte |
Mounts CommandPalette, manages open state, registers hotkey in onMount. |
src/dashboard_v2/src/lib/components/shell/TopBar.svelte |
Adds onOpenPalette prop and wires the search trigger button to it. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/dashboard_v2/src/lib/hotkeys.ts (2)
19-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCaps Lock still breaks the shortcut.
e.key === COMMAND_PALETTE_HOTKEY.keyis a strict lowercase comparison; with Caps Lock on,e.keyis'K', so the shortcut silently fails to fire. This was flagged on a previous commit and remains unaddressed in this version.🔧 Proposed fix
- if (e[modifier] && e.key === COMMAND_PALETTE_HOTKEY.key) { + if (e[modifier] && e.key.toLowerCase() === COMMAND_PALETTE_HOTKEY.key) {🤖 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 `@src/dashboard_v2/src/lib/hotkeys.ts` around lines 19 - 28, The command palette shortcut in handleKeydown is still failing when Caps Lock changes the key casing. Update the key check in hotkeys.ts so it normalizes e.key before comparing against COMMAND_PALETTE_HOTKEY.key, while keeping the existing modifier logic with COMMAND_PALETTE_HOTKEY.macModifier and COMMAND_PALETTE_HOTKEY.otherModifier. Make sure onOpen still only fires when the intended modifier plus the hotkey key are pressed, regardless of uppercase/lowercase input.
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
navigator.platformis deprecated.Previously flagged as acceptable-but-worth-confirming for this specific ⌘/Ctrl detection use case; still unresolved. Feature-detection alternatives exist but this remains one of the few legitimate uses of
navigator.platform.🤖 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 `@src/dashboard_v2/src/lib/hotkeys.ts` at line 20, Replace the deprecated navigator.platform check in hotkeys.ts with a non-deprecated OS detection approach inside the hotkey handling logic (for example, prefer navigator.userAgentData.platform when available and fall back safely). Update the isMac computation in the hotkeys utility so ⌘/Ctrl detection still works without relying on navigator.platform.
🤖 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 `@src/dashboard_v2/src/lib/components/shell/TopBar.svelte`:
- Around line 15-20: The Mac platform check is duplicated between TopBar.svelte
and hotkeys.ts, so extract the detection logic into a shared isMacPlatform()
helper and use it from both places. Update TopBar’s isMac computation to call
the shared helper, and make attachCommandPaletteHotkey reuse the same helper so
any future platform-detection change stays consistent.
---
Duplicate comments:
In `@src/dashboard_v2/src/lib/hotkeys.ts`:
- Around line 19-28: The command palette shortcut in handleKeydown is still
failing when Caps Lock changes the key casing. Update the key check in
hotkeys.ts so it normalizes e.key before comparing against
COMMAND_PALETTE_HOTKEY.key, while keeping the existing modifier logic with
COMMAND_PALETTE_HOTKEY.macModifier and COMMAND_PALETTE_HOTKEY.otherModifier.
Make sure onOpen still only fires when the intended modifier plus the hotkey key
are pressed, regardless of uppercase/lowercase input.
- Line 20: Replace the deprecated navigator.platform check in hotkeys.ts with a
non-deprecated OS detection approach inside the hotkey handling logic (for
example, prefer navigator.userAgentData.platform when available and fall back
safely). Update the isMac computation in the hotkeys utility so ⌘/Ctrl detection
still works without relying on navigator.platform.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e4b626f-f942-4984-abc1-a7e172c2a77b
📒 Files selected for processing (8)
src/dashboard_v2/src/lib/commands.tssrc/dashboard_v2/src/lib/components/overlays/CommandPalette.sveltesrc/dashboard_v2/src/lib/components/shell/AppShell.sveltesrc/dashboard_v2/src/lib/components/shell/TopBar.sveltesrc/dashboard_v2/src/lib/constants/icons.tssrc/dashboard_v2/src/lib/constants/keybindings.tssrc/dashboard_v2/src/lib/constants/routes.tssrc/dashboard_v2/src/lib/hotkeys.ts
… color tokens
constants/icons.ts
- Added ICON_CHECK, ICON_X_CLOSE, ICON_INFO_DOT (Toast)
- Added ICON_INFO_CIRCLE, ICON_TRASH (ConfirmDialog)
- Added ICON_SEARCH (TopBar — replaces inline SVG structure)
constants/keybindings.ts
- Added macAriaKeyshortcuts / otherAriaKeyshortcuts fields
- TopBar derives aria-keyshortcuts value from config (was hardcoded 'Meta+k')
constants/strings.ts (NEW)
- PALETTE_STRINGS: dialogAriaLabel, inputPlaceholder, closeButtonAriaLabel,
resultsListAriaLabel, emptyStatePreamble, footerNavigate/Select/Close
- TOP_BAR_STRINGS: breadcrumbNavAriaLabel, breadcrumbRoot, searchTriggerPlaceholder
- NAV_RAIL_STRINGS: navAriaLabel, logoAlt, healthDotTitle/AriaLabel, badgePendingSuffix
- TOAST_STRINGS: dismissAriaLabel
- CONFIRM_STRINGS: cancelLabel
app.css — added missing tokens:
- --color-danger-hover (#b91c1c)
- --color-scrim, --color-shadow-sm/md/lg/xl (overlay shadows)
- --color-toast-bg/text/dismiss/dismiss-hover
- --color-text-on-filled (#ffffff for filled button text)
Components:
- OverlayScrim: rgba(0,0,0,0.4) → var(--color-scrim)
- Toast: rgba/hex colours → tokens; icons → ICON_* constants; text → TOAST_STRINGS
- ConfirmDialog: rgba shadows → tokens; hex white → tokens; icons → ICON_* constants;
hardcoded 'b91c1c' fallback → var(--color-danger-hover); text → CONFIRM_STRINGS
- CommandPalette: shadows → tokens; all aria/placeholder text → PALETTE_STRINGS
- TopBar: inline SVG → <Icon path={ICON_SEARCH}>; all text → TOP_BAR_STRINGS;
aria-keyshortcuts → derived from config
- NavRail: all aria text → NAV_RAIL_STRINGS
48/48 tests pass, 0 build errors
added 2 commits
July 9, 2026 18:13
… Caps Lock, navigator.platform deprecation, inline SVG
…tkeys.test for userAgent mock; fix scrollIntoView in jsdom
Contributor
Author
Review comments resolved (dee1c91 + 35b4e1e)All valid bot comments addressed. Here's what was done and why: ✅ Fixed
❌ Not fixed — intentional
Test results |
Jessinra
approved these changes
Jul 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/dashboard_v2/src/lib/components/shell/TopBar.svelte`:
- Around line 17-35: The platform sniffing in TopBar.svelte duplicates the
mac-detection logic from hotkeys.ts. Replace the inline navigator/userAgentData
fallback chain in this block with the shared isMacPlatform() helper so the
modifier display and ariaKeyshortcuts derive from one source of truth. Keep the
SSR-safe behavior in TopBar by relying on the helper rather than reimplementing
the detection here.
In `@src/dashboard_v2/src/lib/constants/strings.ts`:
- Around line 18-44: The placeholder text is duplicated between
PALETTE_STRINGS.inputPlaceholder and TOP_BAR_STRINGS.searchTriggerPlaceholder,
so update the strings in src/dashboard_v2/src/lib/constants/strings.ts to derive
one from the other instead of hardcoding both. Keep the shared value in a single
source of truth within the PALETTE_STRINGS/TOP_BAR_STRINGS constants so any
future copy change stays synchronized.
In `@src/dashboard_v2/src/lib/hotkeys.test.ts`:
- Around line 1-20: The hotkey tests only cover the navigator.userAgent fallback
and never exercise the primary navigator.userAgentData.platform path. Add a test
in hotkeys.test.ts that configures navigator.userAgentData with platform set to
macOS and verifies attachCommandPaletteHotkey() triggers onOpen for the correct
modifier, while keeping the existing mockMac()/mockWindows() fallback tests
intact.
In `@src/dashboard_v2/src/lib/hotkeys.ts`:
- Around line 20-31: The platform-detection logic in hotkeys handling is
duplicated with TopBar.svelte and should be extracted into a shared helper. Move
the `userAgentData`/`userAgent` Mac check into a reusable function in a shared
module such as `platform.ts`, then import and use it from the hotkeys code
instead of inlining the detection. Keep the existing `COMMAND_PALETTE_HOTKEY`
modifier selection and `isMac` decision intact, but source the platform result
from the shared helper so both call sites stay in sync.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33a0869c-d741-4193-b1c9-b53314380278
📒 Files selected for processing (15)
src/dashboard_v2/src/app.csssrc/dashboard_v2/src/lib/components/overlays/CommandPalette.sveltesrc/dashboard_v2/src/lib/components/overlays/CommandPalette.test.tssrc/dashboard_v2/src/lib/components/overlays/ConfirmDialog.sveltesrc/dashboard_v2/src/lib/components/overlays/Toast.sveltesrc/dashboard_v2/src/lib/components/shell/AppShell.sveltesrc/dashboard_v2/src/lib/components/shell/NavRail.sveltesrc/dashboard_v2/src/lib/components/shell/TopBar.sveltesrc/dashboard_v2/src/lib/components/ui/OverlayScrim.sveltesrc/dashboard_v2/src/lib/constants/icons.tssrc/dashboard_v2/src/lib/constants/keybindings.tssrc/dashboard_v2/src/lib/constants/strings.tssrc/dashboard_v2/src/lib/hotkeys.test.tssrc/dashboard_v2/src/lib/hotkeys.tssrc/dashboard_v2/src/test-setup.ts
💤 Files with no reviewable changes (1)
- src/dashboard_v2/src/lib/components/shell/AppShell.svelte
Comment on lines
+17
to
+35
| // Derive the correct modifier symbol from config — stays in sync with hotkeys.ts | ||
| // navigator.platform is deprecated; prefer userAgentData.platform (Chrome 90+) | ||
| // with a userAgent fallback for Firefox/Safari. | ||
| // Cast needed because userAgentData is not yet in the lib.dom typings. | ||
| const ua = typeof navigator !== 'undefined' | ||
| ? (navigator as Navigator & { userAgentData?: { platform: string } }) | ||
| : null; | ||
| const isMac = ua | ||
| ? (ua.userAgentData | ||
| ? ua.userAgentData.platform === 'macOS' | ||
| : /Mac|iPhone|iPad|iPod/.test(navigator.userAgent)) | ||
| : false; | ||
| const modifierDisplay = isMac | ||
| ? COMMAND_PALETTE_HOTKEY.macModifierDisplay | ||
| : COMMAND_PALETTE_HOTKEY.otherModifierDisplay; | ||
| const keyDisplay = COMMAND_PALETTE_HOTKEY.keyDisplay; | ||
| const ariaKeyshortcuts = isMac | ||
| ? COMMAND_PALETTE_HOTKEY.macAriaKeyshortcuts | ||
| : COMMAND_PALETTE_HOTKEY.otherAriaKeyshortcuts; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same platform-detection logic duplicated from hotkeys.ts.
This is the same userAgentData/userAgent fallback chain as in hotkeys.ts (with an added SSR guard). This is the same duplication concern raised previously for the mac-sniffing check — once a shared isMacPlatform() helper exists (see comment on hotkeys.ts), this block reduces to a single call.
♻️ Suggested change (after extracting the shared helper)
- const ua = typeof navigator !== 'undefined'
- ? (navigator as Navigator & { userAgentData?: { platform: string } })
- : null;
- const isMac = ua
- ? (ua.userAgentData
- ? ua.userAgentData.platform === 'macOS'
- : /Mac|iPhone|iPad|iPod/.test(navigator.userAgent))
- : false;
+ const isMac = isMacPlatform();🤖 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 `@src/dashboard_v2/src/lib/components/shell/TopBar.svelte` around lines 17 -
35, The platform sniffing in TopBar.svelte duplicates the mac-detection logic
from hotkeys.ts. Replace the inline navigator/userAgentData fallback chain in
this block with the shared isMacPlatform() helper so the modifier display and
ariaKeyshortcuts derive from one source of truth. Keep the SSR-safe behavior in
TopBar by relying on the helper rather than reimplementing the detection here.
Comment on lines
+18
to
+44
| export const PALETTE_STRINGS = { | ||
| /** role="dialog" aria-label */ | ||
| dialogAriaLabel: 'Command palette', | ||
| /** Text input placeholder */ | ||
| inputPlaceholder: 'Search or jump to…', | ||
| /** Close button aria-label */ | ||
| closeButtonAriaLabel: 'Close palette', | ||
| /** Results listbox aria-label */ | ||
| resultsListAriaLabel: 'Commands', | ||
| /** Static prefix for the empty-state message — e.g. 'No results for "foo"' */ | ||
| emptyStatePreamble: 'No results for', | ||
| /** Footer keyboard hint labels */ | ||
| footerNavigate: 'navigate', | ||
| footerSelect: 'select', | ||
| footerClose: 'close', | ||
| } as const; | ||
|
|
||
| // ── Top Bar ─────────────────────────────────────────────────────────────────── | ||
|
|
||
| export const TOP_BAR_STRINGS = { | ||
| /** aria-label for the breadcrumb <nav> */ | ||
| breadcrumbNavAriaLabel: 'Breadcrumb', | ||
| /** Static "root" segment shown in the breadcrumb trail */ | ||
| breadcrumbRoot: APP_NAME, | ||
| /** Visible placeholder inside the search trigger button */ | ||
| searchTriggerPlaceholder: 'Search or jump to…', | ||
| } as const; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate literal between PALETTE_STRINGS.inputPlaceholder and TOP_BAR_STRINGS.searchTriggerPlaceholder.
Both are 'Search or jump to…'. Consider deriving one from the other to keep them in sync if either changes.
♻️ Suggested consolidation
export const TOP_BAR_STRINGS = {
breadcrumbNavAriaLabel: 'Breadcrumb',
breadcrumbRoot: APP_NAME,
- searchTriggerPlaceholder: 'Search or jump to…',
+ searchTriggerPlaceholder: PALETTE_STRINGS.inputPlaceholder,
} as const;📝 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.
Suggested change
| export const PALETTE_STRINGS = { | |
| /** role="dialog" aria-label */ | |
| dialogAriaLabel: 'Command palette', | |
| /** Text input placeholder */ | |
| inputPlaceholder: 'Search or jump to…', | |
| /** Close button aria-label */ | |
| closeButtonAriaLabel: 'Close palette', | |
| /** Results listbox aria-label */ | |
| resultsListAriaLabel: 'Commands', | |
| /** Static prefix for the empty-state message — e.g. 'No results for "foo"' */ | |
| emptyStatePreamble: 'No results for', | |
| /** Footer keyboard hint labels */ | |
| footerNavigate: 'navigate', | |
| footerSelect: 'select', | |
| footerClose: 'close', | |
| } as const; | |
| // ── Top Bar ─────────────────────────────────────────────────────────────────── | |
| export const TOP_BAR_STRINGS = { | |
| /** aria-label for the breadcrumb <nav> */ | |
| breadcrumbNavAriaLabel: 'Breadcrumb', | |
| /** Static "root" segment shown in the breadcrumb trail */ | |
| breadcrumbRoot: APP_NAME, | |
| /** Visible placeholder inside the search trigger button */ | |
| searchTriggerPlaceholder: 'Search or jump to…', | |
| } as const; | |
| export const PALETTE_STRINGS = { | |
| /** role="dialog" aria-label */ | |
| dialogAriaLabel: 'Command palette', | |
| /** Text input placeholder */ | |
| inputPlaceholder: 'Search or jump to…', | |
| /** Close button aria-label */ | |
| closeButtonAriaLabel: 'Close palette', | |
| /** Results listbox aria-label */ | |
| resultsListAriaLabel: 'Commands', | |
| /** Static prefix for the empty-state message — e.g. 'No results for "foo"' */ | |
| emptyStatePreamble: 'No results for', | |
| /** Footer keyboard hint labels */ | |
| footerNavigate: 'navigate', | |
| footerSelect: 'select', | |
| footerClose: 'close', | |
| } as const; | |
| // ── Top Bar ─────────────────────────────────────────────────────────────────── | |
| export const TOP_BAR_STRINGS = { | |
| /** aria-label for the breadcrumb <nav> */ | |
| breadcrumbNavAriaLabel: 'Breadcrumb', | |
| /** Static "root" segment shown in the breadcrumb trail */ | |
| breadcrumbRoot: APP_NAME, | |
| /** Visible placeholder inside the search trigger button */ | |
| searchTriggerPlaceholder: PALETTE_STRINGS.inputPlaceholder, | |
| } as const; |
🤖 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 `@src/dashboard_v2/src/lib/constants/strings.ts` around lines 18 - 44, The
placeholder text is duplicated between PALETTE_STRINGS.inputPlaceholder and
TOP_BAR_STRINGS.searchTriggerPlaceholder, so update the strings in
src/dashboard_v2/src/lib/constants/strings.ts to derive one from the other
instead of hardcoding both. Keep the shared value in a single source of truth
within the PALETTE_STRINGS/TOP_BAR_STRINGS constants so any future copy change
stays synchronized.
Comment on lines
+1
to
+20
| import { describe, it, expect, vi, afterEach } from 'vitest'; | ||
| import { attachCommandPaletteHotkey } from '$lib/hotkeys.js'; | ||
|
|
||
| // Helper to mock platform detection. | ||
| // hotkeys.ts reads navigator.userAgentData.platform (Chrome 90+) with a | ||
| // navigator.userAgent fallback, so we mock via userAgent which jsdom exposes. | ||
| function mockMac() { | ||
| vi.spyOn(navigator, 'userAgent', 'get').mockReturnValue( | ||
| 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36' | ||
| ); | ||
| // Ensure userAgentData is absent so userAgent fallback is used | ||
| Object.defineProperty(navigator, 'userAgentData', { value: undefined, configurable: true }); | ||
| } | ||
|
|
||
| function mockWindows() { | ||
| vi.spyOn(navigator, 'userAgent', 'get').mockReturnValue( | ||
| 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36' | ||
| ); | ||
| Object.defineProperty(navigator, 'userAgentData', { value: undefined, configurable: true }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Primary userAgentData detection path is never actually exercised.
Both mockMac() and mockWindows() force navigator.userAgentData to undefined, so every test only covers the userAgent regex fallback branch. The ua.userAgentData.platform === 'macOS' branch — the one used in modern Chrome/Edge (90+) — has zero test coverage. Add at least one test that sets userAgentData to confirm that path also resolves the modifier correctly.
🧪 Suggested addition
function mockMacUserAgentData() {
Object.defineProperty(navigator, 'userAgentData', {
value: { platform: 'macOS' },
configurable: true
});
}it('calls onOpen on ⌘K via userAgentData.platform', () => {
mockMacUserAgentData();
const onOpen = vi.fn();
cleanup = attachCommandPaletteHotkey(onOpen);
fire('k', { metaKey: true });
expect(onOpen).toHaveBeenCalledOnce();
});
🤖 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 `@src/dashboard_v2/src/lib/hotkeys.test.ts` around lines 1 - 20, The hotkey
tests only cover the navigator.userAgent fallback and never exercise the primary
navigator.userAgentData.platform path. Add a test in hotkeys.test.ts that
configures navigator.userAgentData with platform set to macOS and verifies
attachCommandPaletteHotkey() triggers onOpen for the correct modifier, while
keeping the existing mockMac()/mockWindows() fallback tests intact.
Comment on lines
+20
to
+31
| // navigator.platform is deprecated; prefer userAgentData.platform (Chrome 90+) | ||
| // with a userAgent fallback for Firefox/Safari. | ||
| // Cast needed because userAgentData is not yet in the lib.dom typings. | ||
| const ua = navigator as Navigator & { userAgentData?: { platform: string } }; | ||
| const isMac = ua.userAgentData | ||
| ? ua.userAgentData.platform === 'macOS' | ||
| : /Mac|iPhone|iPad|iPod/.test(navigator.userAgent); | ||
| const modifier = isMac | ||
| ? COMMAND_PALETTE_HOTKEY.macModifier | ||
| : COMMAND_PALETTE_HOTKEY.otherModifier; | ||
|
|
||
| if (e[modifier] && e.key.toLowerCase() === COMMAND_PALETTE_HOTKEY.key) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Platform-detection logic duplicated with TopBar.svelte.
The userAgentData → userAgent fallback chain here is nearly byte-for-byte duplicated in TopBar.svelte (lines 21-28). A prior review already flagged this duplication for the earlier navigator.platform implementation — the new, more complex fallback logic re-introduces the same DRY problem and doubles the maintenance/drift risk (e.g., if the macOS string check or fallback regex needs a fix later).
Extract into a shared helper (e.g. $lib/platform.ts) and import it from both files.
♻️ Suggested extraction
// src/dashboard_v2/src/lib/platform.ts
export function isMacPlatform(): boolean {
if (typeof navigator === 'undefined') return false;
const ua = navigator as Navigator & { userAgentData?: { platform: string } };
return ua.userAgentData
? ua.userAgentData.platform === 'macOS'
: /Mac|iPhone|iPad|iPod/.test(navigator.userAgent);
}-import { COMMAND_PALETTE_HOTKEY } from '$lib/constants/keybindings.js';
+import { COMMAND_PALETTE_HOTKEY } from '$lib/constants/keybindings.js';
+import { isMacPlatform } from '$lib/platform.js';
...
- const ua = navigator as Navigator & { userAgentData?: { platform: string } };
- const isMac = ua.userAgentData
- ? ua.userAgentData.platform === 'macOS'
- : /Mac|iPhone|iPad|iPod/.test(navigator.userAgent);
+ const isMac = isMacPlatform();🤖 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 `@src/dashboard_v2/src/lib/hotkeys.ts` around lines 20 - 31, The
platform-detection logic in hotkeys handling is duplicated with TopBar.svelte
and should be extracted into a shared helper. Move the
`userAgentData`/`userAgent` Mac check into a reusable function in a shared
module such as `platform.ts`, then import and use it from the hotkeys code
instead of inlining the detection. Keep the existing `COMMAND_PALETTE_HOTKEY`
modifier selection and `isMac` decision intact, but source the platform result
from the shared helper so both call sites stay in sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the Command Palette behind a ⌘K / Ctrl+K global hotkey.
New files
src/lib/commands.ts— Command registry withbuildCommands,filterCommands,GROUP_ORDER; jump commands auto-generated fromNAV_ROUTESsrc/lib/hotkeys.ts— Global keydown listener for ⌘K (Mac) / Ctrl+K; returns cleanup fnsrc/lib/components/overlays/CommandPalette.svelte— 640px centered modal, blurred/dimmed scrim, ↑↓Enter keyboard nav, live filter, 3 groups (Recent / Jump to / Actions), footer hint barModified files
AppShell.svelte— mountsCommandPalette+ registers hotkey viaattachCommandPaletteHotkeyTopBar.svelte— search-trigger button now callsonOpenPaletteprop (previously was a dead button)Tests
hotkeys.ts(⌘K, Ctrl+K, bare K, ⌘J, cleanup, Mac-vs-Win modifier)commands.ts(buildCommands shape, unique IDs, jump wiring, action callbacks, filterCommands empty/whitespace/case/no-match)ACs met
CommandPalette.sveltecreatedcommands.tsregistry createdhotkeys.ts⌘K/Ctrl+K listener createdAppShell.sveltemounts palette + registers hotkeyTopBar.sveltewires search trigger to open palette