feat(ui): add ⌘+K command palette for quick nav and actions#2159
feat(ui): add ⌘+K command palette for quick nav and actions#2159
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
bed7157 to
9fb8bf3
Compare
|
I love it, super clean and useful! 🥳 |
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
3b1e01b to
e280565
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
e280565 to
10fbdc8
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
10fbdc8 to
15a4d07
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
15a4d07 to
f2b53bf
Compare
serhalp
changed the title
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
2 times, most recently
from
6777ee1 to
823c9cf
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
823c9cf to
2667fda
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
4968468 to
36ee45d
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
36ee45d to
bfdaa6c
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
bfdaa6c to
35cc79f
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
35cc79f to
9b82c5f
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
9b82c5f to
24821c2
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
24821c2 to
3afa608
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
3afa608 to
46b4c51
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
46b4c51 to
ac7836c
Compare
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
ac7836c to
9983364
Compare
📝 WalkthroughWalkthroughThis pull request introduces a command palette feature accessible via Ctrl+K (Windows/Linux) or ⌘K (macOS) throughout the application. It includes a new Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
app/utils/package-download.ts (1)
32-41: Make link/object-URL cleanup unconditional withfinally.If
appendChild/clickthrows, cleanup andrevokeObjectURLare skipped. Wrapping the interaction intry/finallyavoids leaks and keeps behaviour deterministic.♻️ Suggested refactor
export async function downloadPackageTarball( packageName: string, version: DownloadablePackageVersion, ) { const tarballUrl = version.dist.tarball if (!tarballUrl) return const downloadUrl = await getDownloadUrl(tarballUrl) const link = document.createElement('a') - link.href = downloadUrl ?? tarballUrl - link.download = `${packageName.replace(/\//g, '__')}-${version.version}.tgz` - document.body.appendChild(link) - link.click() - document.body.removeChild(link) - - if (downloadUrl) { - URL.revokeObjectURL(downloadUrl) - } + try { + link.href = downloadUrl ?? tarballUrl + link.download = `${packageName.replace(/\//g, '__')}-${version.version}.tgz` + document.body.appendChild(link) + link.click() + } finally { + if (link.isConnected) { + document.body.removeChild(link) + } + if (downloadUrl) { + URL.revokeObjectURL(downloadUrl) + } + } }As per coding guidelines: "Use error handling patterns consistently".
test/nuxt/app/utils/package-download.spec.ts (1)
78-116: Add one test for the explicit non-OK HTTP response branch.
getDownloadUrlhas a dedicated!response.okbranch (Line 11 in the utility), but this spec currently covers only thrown-fetch failure for fallback.🧪 Optional test addition
+ it('falls back to the remote tarball URL when fetch returns a non-OK response', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ + ok: false, + status: 503, + blob: async () => new Blob(['ignored']), + })), + ) + Object.defineProperty(URL, 'createObjectURL', { + configurable: true, + writable: true, + value: vi.fn(), + }) + Object.defineProperty(URL, 'revokeObjectURL', { + configurable: true, + writable: true, + value: vi.fn(), + }) + + await downloadPackageTarball('vue', { + version: '3.5.0', + dist: { + tarball: 'https://registry.npmjs.org/vue/-/vue-3.5.0.tgz', + }, + }) + + const anchor = appendSpy.mock.calls[0]?.[0] + expect(anchor).toBeInstanceOf(HTMLAnchorElement) + expect((anchor as HTMLAnchorElement).href).toBe( + 'https://registry.npmjs.org/vue/-/vue-3.5.0.tgz', + ) + expect(URL.createObjectURL).not.toHaveBeenCalled() + expect(URL.revokeObjectURL).not.toHaveBeenCalled() + expect(consoleError).toHaveBeenCalled() + })As per coding guidelines: "Write unit tests for core functionality using
vitest".app/components/Settings/AccentColorPicker.vue (1)
17-19: Update the stale inline comment to reflect current default handling.The comment still references a clear-button/value-empty default, but the code now resolves via
defaultId.Suggested wording tweak
- // Remove checked from the server-default (clear button, value="") + // Remove checked from the server-default option to avoid dual checked radiosapp/components/Settings/BgThemePicker.vue (1)
17-19: Refresh the inline comment to match the new default-option logic.The current wording still implies an empty-value clear default, which is no longer what this block does.
Suggested wording tweak
- // Remove checked from the server-default (clear button, value="") + // Remove checked from the server-default option to avoid dual checked radiosapp/components/Package/ExternalLinks.vue (1)
109-117: Avoid chained non-null assertions inhrefassignment.This branch is already guarded, so you can keep full type-safety without
!assertions.As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".♻️ Suggested typed simplification
if (displayVersion.value?.bugs?.url) { + const bugsUrl = displayVersion.value.bugs.url commands.push({ id: 'package-link-issues', group: 'links', label: $t('package.links.issues'), keywords: [...packageKeywords, $t('package.links.issues')], iconClass: 'i-lucide:circle-alert', - href: displayVersion.value!.bugs!.url!, + href: bugsUrl, }) }app/components/CommandPalette.client.vue (1)
319-323: Use the global button focus rule here.
button:focus-visibleis already styled globally, so the inlinefocus-visible:outline-accent/70classes duplicate that rule on the back button and on command rows that render as<button>. Please rely on the global rule for the button variant, or split the shared row class so only link variants carry extra focus treatment. Based on learnings, "focus-visible styling for button and select elements is implemented globally in app/assets/main.css ... Do not apply per-element inline utility classes."Also applies to: 380-457
app/composables/useCommandPaletteGlobalCommands.ts (1)
212-604: Split the global command builder into smaller factories.This computed now mixes root navigation, settings, connections, help links, and account-specific branches in one block, which makes it hard to review and easy to regress when new commands are added. Extracting per-section builders would make the command set much easier to audit and extend. As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)."
test/nuxt/composables/use-command-palette-commands.spec.ts (1)
44-115: Move wrapper disposal intoafterEach.Every test unmounts manually, but if an assertion fails earlier the scope-disposal cleanup never runs and the command-palette registries can leak into later cases. Tracking the mounted wrapper centrally, like the component spec does, would make teardown deterministic.
♻️ Suggested cleanup pattern
+let currentWrapper: Awaited<ReturnType<typeof mountSuspended>> | null = null + async function captureCommandPalette(options?: { route?: string query?: string colorMode?: 'system' | 'light' | 'dark' @@ - const wrapper = await mountSuspended(WrapperComponent, { + currentWrapper = await mountSuspended(WrapperComponent, { route: options?.route ?? '/', }) return { - wrapper, + wrapper: currentWrapper, groupedCommands, flatCommands, routePath, submitSearchQuery, } } afterEach(() => { + currentWrapper?.unmount() + currentWrapper = null const commandPalette = useCommandPalette() commandPalette.close() commandPalette.clearPackageContext()Also applies to: 117-129
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad281cb6-8c50-44bc-a99e-7668d57cdbd2
📒 Files selected for processing (43)
CONTRIBUTING.mdapp/app.vueapp/components/AppFooter.vueapp/components/AppHeader.vueapp/components/CommandPalette.client.vueapp/components/Diff/ViewerPanel.vueapp/components/Header/MobileMenu.client.vueapp/components/Package/DownloadButton.vueapp/components/Package/ExternalLinks.vueapp/components/Package/Header.vueapp/components/Package/Playgrounds.vueapp/components/Package/SkillsCard.vueapp/components/Settings/AccentColorPicker.vueapp/components/Settings/BgThemePicker.vueapp/components/Terminal/Install.vueapp/composables/useCommandPalette.tsapp/composables/useCommandPaletteCommands.tsapp/composables/useCommandPaletteGlobalCommands.tsapp/composables/useCommandPalettePackageCommands.tsapp/composables/useCommandPalettePackageVersions.tsapp/composables/useCommandPaletteVersionCommands.tsapp/composables/usePlatformModifierKey.tsapp/composables/useSettings.tsapp/pages/compare.vueapp/pages/diff/[[org]]/[packageName]/v/[versionRange].vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vueapp/pages/package-docs/[...path].vueapp/pages/package/[[org]]/[name].vueapp/pages/profile/[identity]/index.vueapp/types/command-palette.tsapp/utils/package-download.tsdocs/content/2.guide/1.features.mddocs/content/2.guide/2.keyboard-shortcuts.mddocs/content/index.mdi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsontest/nuxt/a11y.spec.tstest/nuxt/app/utils/package-download.spec.tstest/nuxt/components/CommandPalette.spec.tstest/nuxt/composables/use-command-palette-commands.spec.tstest/nuxt/composables/use-command-palette-package-versions.spec.tstest/nuxt/composables/use-command-palette.spec.ts
| const isHome = computed(() => route.name === 'index') | ||
|
|
||
| const discord = useDiscordLink() | ||
| const { commandPaletteShortcutLabel } = usePlatformModifierKey() |
There was a problem hiding this comment.
Use the platform-aware modifier label in the description text.
The shortcut row uses commandPaletteShortcutLabel, but the description still interpolates shortcuts.ctrl_key. On Apple devices this can produce mixed messaging.
Suggested fix
-const { commandPaletteShortcutLabel } = usePlatformModifierKey()
+const { commandPaletteShortcutLabel, primaryModifierKeyLabel } = usePlatformModifierKey()
...
-{{ $t('shortcuts.command_palette_description', { ctrlKey: $t('shortcuts.ctrl_key') }) }}
+{{ $t('shortcuts.command_palette_description', { ctrlKey: primaryModifierKeyLabel }) }}Also applies to: 56-60
| const keyboardShortcuts = useKeyboardShortcuts() | ||
| const discord = useDiscordLink() | ||
| const { open: openCommandPalette } = useCommandPalette() | ||
| const { commandPaletteShortcutLabel } = usePlatformModifierKey() |
There was a problem hiding this comment.
Use the platform-aware modifier label in the tooltip title.
commandPaletteShortcutLabel is platform-specific, but the title currently interpolates shortcuts.ctrl_key, which can read inconsistently on macOS.
Suggested fix
-const { commandPaletteShortcutLabel } = usePlatformModifierKey()
+const { commandPaletteShortcutLabel, primaryModifierKeyLabel } = usePlatformModifierKey()
...
-:title="$t('shortcuts.command_palette_description', { ctrlKey: $t('shortcuts.ctrl_key') })"
+:title="$t('shortcuts.command_palette_description', { ctrlKey: primaryModifierKeyLabel })"Also applies to: 268-268
| const isToggleShortcut = | ||
| event.key.toLowerCase() === 'k' && | ||
| (event.metaKey || event.ctrlKey) && | ||
| !event.altKey && | ||
| !event.shiftKey | ||
|
|
||
| if (isToggleShortcut) { | ||
| if (!keyboardShortcuts.value) return | ||
|
|
||
| event.preventDefault() | ||
| toggle() | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the actual file
find . -name "CommandPalette.client.vue" -type fRepository: npmx-dev/npmx.dev
Length of output: 103
🏁 Script executed:
# Check the specific lines mentioned in the review (167-178)
cat -n app/components/CommandPalette.client.vue | sed -n '160,180p'Repository: npmx-dev/npmx.dev
Length of output: 605
🌐 Web query:
KeyboardEvent repeat property auto-repeat behaviour
💡 Result:
The KeyboardEvent.repeat read-only property is a boolean that returns true if the key is being held down such that it is automatically repeating (auto-repeating), and false otherwise. It is set on keydown events (and potentially beforeinput/input during repeat) when the user agent detects sustained key depression after an initial delay, at an environment-dependent repeat rate. Key behavior: - Initial keydown: repeat = false - During auto-repeat (after hold delay): repeat = true on repeating keydown events (and related input events if applicable) - keyup: repeat = false (single event) This follows the UI Events specification: when a key is depressed for a sustained period, keydown (repeat=true), beforeinput (if character key), input may repeat at system-configured rate. Primarily used in keydown handlers to distinguish initial press from repeats, e.g.: if (!event.repeat) { /* initial press / } else { / repeating */ } Broad browser support (Chrome 32+, Firefox 28+, Safari 10.1+, etc.), though keypress (deprecated) may not reliably set repeat in all browsers. Modifier keys like Shift may or may not auto-repeat depending on OS/browser (e.g., Windows yes, macOS/Linux often no).
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/repeat
- 2: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
- 3: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
- 4: https://w3.org/TR/uievents
- 5: https://w3c.github.io/uievents/split/keyboard-events.html
- 6: https://www.w3.org/TR/uievents/Overview.html
Prevent repeated toggles from auto-repeat key events.
The toggle path fires on every keydown event, including when a key repeats during sustained depression. Holding Ctrl/⌘+K will fire the initial keydown (opening the palette), then auto-repeat keydown events will immediately close it again. Add a check for event.repeat to ignore auto-repeated key events.
💡 Minimal fix
if (isToggleShortcut) {
if (!keyboardShortcuts.value) return
+ if (event.repeat) return
event.preventDefault()
toggle()
return
}📝 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.
| const isToggleShortcut = | |
| event.key.toLowerCase() === 'k' && | |
| (event.metaKey || event.ctrlKey) && | |
| !event.altKey && | |
| !event.shiftKey | |
| if (isToggleShortcut) { | |
| if (!keyboardShortcuts.value) return | |
| event.preventDefault() | |
| toggle() | |
| return | |
| const isToggleShortcut = | |
| event.key.toLowerCase() === 'k' && | |
| (event.metaKey || event.ctrlKey) && | |
| !event.altKey && | |
| !event.shiftKey | |
| if (isToggleShortcut) { | |
| if (!keyboardShortcuts.value) return | |
| if (event.repeat) return | |
| event.preventDefault() | |
| toggle() | |
| return | |
| } |
| const queryOverrideRegistry = new Map<string, (query: string) => CommandPaletteCommand[] | null>() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected result: only "client-only" lines.
# Any "server-capable" match confirms that SSR can hit the process-wide registry.
rg -n '\buseCommandPaletteQueryOverride\s*\(' app | while IFS=: read -r file line _; do
if [[ "$file" == *.client.* ]]; then
scope="client-only"
else
scope="server-capable"
fi
printf '%s\t%s:%s\n' "$scope" "$file" "$line"
doneRepository: npmx-dev/npmx.dev
Length of output: 186
🏁 Script executed:
# Check useCommandPaletteVersionCommands.ts around line 50
cat -n app/composables/useCommandPaletteVersionCommands.ts | sed -n '40,60p'Repository: npmx-dev/npmx.dev
Length of output: 906
🏁 Script executed:
# Check the broader context of useCommandPaletteQueryOverride definition and guards
cat -n app/composables/useCommandPalette.ts | sed -n '295,315p'Repository: npmx-dev/npmx.dev
Length of output: 600
🏁 Script executed:
# Check if useCommandPaletteVersionCommands.ts has any import.meta.client guards
rg -n 'import\.meta\.client' app/composables/useCommandPaletteVersionCommands.tsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Verify the Command Palette component/usage to see if it's client-only
fd -e vue -e ts app | xargs rg -l 'CommandPalette|useCommandPalette' | head -10Repository: npmx-dev/npmx.dev
Length of output: 101
🏁 Script executed:
# Get full filename info and check if it's .client or not
fd 'useCommandPaletteVersionCommands'Repository: npmx-dev/npmx.dev
Length of output: 112
🏁 Script executed:
# Search for all imports of useCommandPaletteVersionCommands
rg -n 'useCommandPaletteVersionCommands' appRepository: npmx-dev/npmx.dev
Length of output: 719
🏁 Script executed:
# Check the actual defineComposable pattern and whether useCommandPalette has guards
cat -n app/composables/useCommandPalette.ts | head -80Repository: npmx-dev/npmx.dev
Length of output: 2902
🏁 Script executed:
# Find the setQueryOverride function implementation
cat -n app/composables/useCommandPalette.ts | sed -n '150,250p'Repository: npmx-dev/npmx.dev
Length of output: 3231
🏁 Script executed:
# Check what happens when resolveQueryOverride is called - is it server-side only?
rg -n 'resolveQueryOverride\|setQueryOverride\|clearQueryOverride' app/composables/useCommandPalette.tsRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check if the pages that call useCommandPaletteVersionCommands are marked as client-only
fd -e vue 'package.*\.vue$|diff.*\.vue$' | xargs grep -l 'defineRouteRules\|ssr:' || echo "No SSR rules found"Repository: npmx-dev/npmx.dev
Length of output: 79
Guard process-wide registry mutations from server-side execution.
queryOverrideRegistry is a module-level Map mutated by setQueryOverride() without an import.meta.client guard. Since useCommandPaletteVersionCommands (which calls useCommandPaletteQueryOverride) is imported from server-capable pages and Nuxt renders those pages server-side, concurrent requests can overwrite each other's {scopeId}:{group} entries. The related queryOverrides state is correctly per-request, but the registry persists across requests.
Guard the mutation with import.meta.client (as done in useCommandPalettePackageContext at line 231), or move the registry to per-request state.
Occurs at: lines 164–176, 300–311
| queryOverrides.value.forEach(({ scopeId, group }) => { | ||
| const resolve = resolveQueryOverride(scopeId, group) | ||
| if (!resolve) return | ||
| const overrideCommands = resolve(trimmedQuery.value) | ||
| if (overrideCommands == null) return | ||
| overrideCommands.forEach(command => { | ||
| if (command.action != null) { | ||
| command.action = closeThen(command.action) | ||
| } | ||
| }) | ||
| nextCommands = [ | ||
| ...nextCommands.filter(command => command.group !== group), | ||
| ...overrideCommands, | ||
| ] | ||
| }) |
There was a problem hiding this comment.
Keep query overrides scoped to the current palette view.
This loop applies overrides even when the current view does not expose that group. In practice, opening a submenu such as the language chooser and typing a valid semver range can still inject the versions group into that submenu. Gate the override by the groups present in commands.value for the active view.
Suggested fix
const matchedCommands = computed(() => {
+ const availableGroups = new Set(commands.value.map(command => command.group))
let nextCommands = results.value.map(result => result.item)
queryOverrides.value.forEach(({ scopeId, group }) => {
+ if (!availableGroups.has(group)) return
+
const resolve = resolveQueryOverride(scopeId, group)
if (!resolve) return
const overrideCommands = resolve(trimmedQuery.value)📝 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.
| queryOverrides.value.forEach(({ scopeId, group }) => { | |
| const resolve = resolveQueryOverride(scopeId, group) | |
| if (!resolve) return | |
| const overrideCommands = resolve(trimmedQuery.value) | |
| if (overrideCommands == null) return | |
| overrideCommands.forEach(command => { | |
| if (command.action != null) { | |
| command.action = closeThen(command.action) | |
| } | |
| }) | |
| nextCommands = [ | |
| ...nextCommands.filter(command => command.group !== group), | |
| ...overrideCommands, | |
| ] | |
| }) | |
| const matchedCommands = computed(() => { | |
| const availableGroups = new Set(commands.value.map(command => command.group)) | |
| let nextCommands = results.value.map(result => result.item) | |
| queryOverrides.value.forEach(({ scopeId, group }) => { | |
| if (!availableGroups.has(group)) return | |
| const resolve = resolveQueryOverride(scopeId, group) | |
| if (!resolve) return | |
| const overrideCommands = resolve(trimmedQuery.value) | |
| if (overrideCommands == null) return | |
| overrideCommands.forEach(command => { | |
| if (command.action != null) { | |
| command.action = closeThen(command.action) | |
| } | |
| }) | |
| nextCommands = [ | |
| ...nextCommands.filter(command => command.group !== group), | |
| ...overrideCommands, | |
| ] | |
| }) | |
| }) |
| if (!isViewingFile.value || isBinaryFile.value || !fileContent.value) return [] | ||
|
|
||
| const commands: CommandPaletteContextCommandInput[] = [ | ||
| { | ||
| id: 'code-copy-link', | ||
| group: 'actions', | ||
| label: $t('code.copy_link'), | ||
| keywords: [packageName.value, filePath.value ?? ''], | ||
| iconClass: 'i-lucide:link', | ||
| action: () => { | ||
| copyPermalinkUrl() | ||
| }, | ||
| }, | ||
| { | ||
| id: 'code-copy-file', | ||
| group: 'actions', | ||
| label: $t('command_palette.code.copy_file'), | ||
| keywords: [packageName.value, filePath.value ?? '', $t('common.copy')], | ||
| iconClass: 'i-lucide:file', | ||
| action: () => { | ||
| copyFileContent() | ||
| }, | ||
| }, | ||
| ] | ||
|
|
||
| if (filePath.value) { | ||
| commands.push({ | ||
| id: 'code-open-raw', | ||
| group: 'links', | ||
| label: $t('code.raw'), | ||
| keywords: [packageName.value, filePath.value], | ||
| iconClass: 'i-lucide:file-output', | ||
| href: `https://cdn.jsdelivr.net/npm/${packageName.value}@${version.value}/${filePath.value}`, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do not hide the raw-file action behind fileContent.
The early return at Line 339 drops all file-context commands when the file is binary or not yet loaded, so the palette loses “Open raw” in the same states where the page still offers View raw. Register the raw link from filePath first, then gate only the copy/markdown commands on fileContent.
Suggested fix
useCommandPaletteContextCommands(
computed((): CommandPaletteContextCommandInput[] => {
- if (!isViewingFile.value || isBinaryFile.value || !fileContent.value) return []
-
- const commands: CommandPaletteContextCommandInput[] = [
+ if (!isViewingFile.value) return []
+
+ const commands: CommandPaletteContextCommandInput[] = []
+
+ if (filePath.value) {
+ commands.push({
+ id: 'code-open-raw',
+ group: 'links',
+ label: $t('code.raw'),
+ keywords: [packageName.value, filePath.value],
+ iconClass: 'i-lucide:file-output',
+ href: `https://cdn.jsdelivr.net/npm/${packageName.value}@${version.value}/${filePath.value}`,
+ })
+ }
+
+ if (isBinaryFile.value || !fileContent.value) return commands
+
+ commands.push(
{
id: 'code-copy-link',
group: 'actions',
label: $t('code.copy_link'),
keywords: [packageName.value, filePath.value ?? ''],
@@
action: () => {
copyFileContent()
},
},
- ]
-
- if (filePath.value) {
- commands.push({
- id: 'code-open-raw',
- group: 'links',
- label: $t('code.raw'),
- keywords: [packageName.value, filePath.value],
- iconClass: 'i-lucide:file-output',
- href: `https://cdn.jsdelivr.net/npm/${packageName.value}@${version.value}/${filePath.value}`,
- })
- }
+ )
if (fileContent.value.markdownHtml) {| if (profile.value.website) { | ||
| commands.push({ | ||
| id: 'profile-website', | ||
| group: 'links', | ||
| label: $t('profile.website'), | ||
| keywords: [profile.value.website, profile.value.handle ?? identity.value], | ||
| iconClass: 'i-lucide:link', | ||
| href: profile.value.website, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find command-palette execution paths for href commands.
rg -n -C3 "useCommandPaletteContextCommands|href\\s*:|execute|runCommand|window\\.open|navigateTo|location\\.href" app
# 2) Find existing URL sanitisation helpers and scheme checks.
rg -n -C3 "sanitize|safe.*url|isSafeUrl|normalize.*url|protocol\\s*===\\s*'http:'|protocol\\s*===\\s*\"http:\"|javascript:" appRepository: npmx-dev/npmx.dev
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any existing URL validation utilities
rg -n "validateUrl|isValidUrl|isValidHref|normalizeUrl" app
# Check if there's any profile validation on website field
rg -n "profile.*website|website.*validation|website.*scheme" app
# Look for any documentation or comments about navigateTo safety
rg -n -B2 -A2 "navigateTo.*external.*true" appRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find profile type definitions
fd -e ts -e tsx -e vue | xargs grep -l "profile.*website\|interface.*Profile\|type Profile" app | head -10
# Look at the profile page to understand data source
grep -n "profile.*value" app/pages/profile/[identity]/index.vue | head -20
# Check types/command-palette.ts for CommandPaletteHrefCommand constraints
cat -n app/types/command-palette.ts | grep -A 5 "CommandPaletteHrefCommand"Repository: npmx-dev/npmx.dev
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the profile type definition and website field
cat -n server/utils/atproto/utils/profile.ts
# Check command-palette types
cat -n app/types/command-palette.ts | head -80
# Search for profile data fetch
rg -n "useAsyncData.*profile|useFetch.*profile" app/pages/profile/Repository: npmx-dev/npmx.dev
Length of output: 7258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find NPMXProfile type definition
find . -name "*.ts" -o -name "*.tsx" | xargs grep -l "NPMXProfile" | head -10
# Check for shared/types directory
fd -t f "social" | grep types
# Look at the lexicon definition file for profile schema
fd -t f "profile" | grep -E "lexicon|schema" | head -10
# Check CommandPalette client to see actual href handling
sed -n '110,125p' app/components/CommandPalette.client.vue
</parameter>Repository: npmx-dev/npmx.dev
Length of output: 698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# NPMXProfile type definition
cat -n shared/types/social.ts
# Profile lexicon schema
cat -n lexicons/dev/npmx/actor/profile.jsonRepository: npmx-dev/npmx.dev
Length of output: 2213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show full context around the vulnerable code
sed -n '115,125p' app/pages/profile/[identity]/index.vue
# Check if profile.value is guaranteed to exist at this point
sed -n '98,125p' app/pages/profile/[identity]/index.vueRepository: npmx-dev/npmx.dev
Length of output: 1251
Validate/sanitise profile.value.website before exposing it as a command href to enforce strict type-safety.
The website field comes from user-profile data and is passed directly to the command palette href without scheme validation. Although the backend lexicon schema specifies a URI format, a known bug (evident from the profile.ts code at line 90) allows non-URI strings to be stored despite validation. The frontend type system offers no constraint (website?: string), and no explicit URL scheme whitelist exists. This violates the coding guideline requiring strictly type-safe code.
The condition if (profile.value.website) guards against undefined values, but does not validate the URL scheme. Implement explicit http/https scheme validation before use:
Suggested hardening
if (profile.value.website) {
+ let safeWebsiteUrl: string | null = null
+ try {
+ const parsed = new URL(profile.value.website)
+ if (parsed.protocol === 'http:' || parsed.protocol === 'https:') {
+ safeWebsiteUrl = parsed.href
+ }
+ } catch {
+ // Ignore invalid URL
+ }
+
+ if (!safeWebsiteUrl) {
+ return commands
+ }
+
commands.push({
id: 'profile-website',
group: 'links',
label: $t('profile.website'),
keywords: [profile.value.website, profile.value.handle ?? identity.value],
iconClass: 'i-lucide:link',
- href: profile.value.website,
+ href: safeWebsiteUrl,
})
}| - Switch settings such as language, accent color, and background theme | ||
| - Open external resources like docs, chat, source, and package links | ||
|
|
||
| You can also open it from the `jump to...` button in the app header or from the mobile menu if you prefer a visible entry point. |
There was a problem hiding this comment.
Header trigger wording looks outdated.
This line mentions a jump to... button, but the UI label introduced in this PR is quick actions. Updating the wording will avoid user confusion.
| ::: | ||
|
|
||
| :::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Press / to search, . for code viewer, arrow keys to navigate results."} | ||
| :::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Open the command palette with ⌘K on macOS or Ctrl+K on Windows and Linux, press / to search, and use arrow keys to move fast."} |
There was a problem hiding this comment.
Clarify shortcut sequencing in the feature blurb.
The sentence currently reads as if / is part of the command-palette interaction flow; consider separating palette and global search shortcuts to avoid ambiguity.
Proposed wording
-:::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Open the command palette with ⌘K on macOS or Ctrl+K on Windows and Linux, press / to search, and use arrow keys to move fast."}
+:::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Open the command palette with ⌘K on macOS or Ctrl+K on Windows and Linux, or press / to focus search, then use arrow keys to move fast."}📝 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.
| :::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Open the command palette with ⌘K on macOS or Ctrl+K on Windows and Linux, press / to search, and use arrow keys to move fast."} | |
| :::u-page-feature{icon="i-lucide:keyboard" to="/guide/keyboard-shortcuts" title="Navigate with keyboard" description="Open the command palette with ⌘K on macOS or Ctrl+K on Windows and Linux, or press / to focus search, then use arrow keys to move fast."} |
| it('should have no accessibility violations when open', async () => { | ||
| const component = await mountSuspended(CommandPaletteHarness) | ||
| await nextTick() | ||
| await nextTick() | ||
|
|
||
| const results = await runAxe(component) | ||
| expect(results.violations).toEqual([]) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the command palette renders outside wrapper root.
# Expected:
# - If Teleport/to="body"/dialog mounting is present in CommandPalette.client.vue,
# this test should audit document.body (or a targeted body-level node), not wrapper clones.
fd "CommandPalette.client.vue"
rg -n "Teleport|to=\"body\"|showModal|<dialog" app/components/CommandPalette.client.vue
rg -n "function runAxe|cloneNode\\(true\\)" test/nuxt/a11y.spec.tsRepository: npmx-dev/npmx.dev
Length of output: 373
🏁 Script executed:
# Get more context on the dialog element positioning within CommandPalette
rg -B 10 -A 10 "modalRef.*showModal" app/components/CommandPalette.client.vue
# Get the full runAxe function to understand how it audits
sed -n '36,60p' test/nuxt/a11y.spec.ts
# Check the template structure to see if dialog is a direct child
rg -B 5 "ref.*modal|<dialog" app/components/CommandPalette.client.vue | head -40Repository: npmx-dev/npmx.dev
Length of output: 1748
🏁 Script executed:
# Find CommandPaletteHarness definition
fd "CommandPalette" --extension "ts" --extension "tsx" --extension "vue" | xargs rg -l "CommandPaletteHarness"
# Check the test file to see if there's a harness definition or import
rg -B 5 "CommandPaletteHarness" test/nuxt/a11y.spec.ts | head -30
# Find Modal component
fd "Modal" --extension "vue" | head -5Repository: npmx-dev/npmx.dev
Length of output: 736
🏁 Script executed:
# Get the CommandPaletteHarness definition from a11y.spec.ts (the actual file we're reviewing)
rg -B 10 -A 30 "CommandPaletteHarness" test/nuxt/a11y.spec.ts
# Check Modal.client.vue to see if it wraps a native dialog element
cat -n app/components/Modal.client.vue | head -80Repository: npmx-dev/npmx.dev
Length of output: 4865
This accessibility test does not audit the opened palette dialog.
Modal.client.vue renders its <dialog> element via <Teleport to="body">, placing it outside the component tree. The runAxe() function clones wrapper.element (the test harness root) and audits only that isolated clone. Since the dialog is teleported to body and not a child of the wrapper, the cloned element will not include the opened palette, so this test can pass without auditing the actual modal UI.
The palette opens correctly (onMounted calls commandPalette?.open()), but the accessibility audit never includes the real dialog DOM.
Suggested fix
- const results = await runAxe(component)
+ const results = await axe.run(document.body, axeRunOptions)
expect(results.violations).toEqual([])2 participants
🔗 Linked issue
closes #81
supersedes stale #470
🧭 Context
npmx already exposes a ton of useful capabilities (with way more to come), but they require quite a bit of precise clicking around. We always imagined npmx as a power tool for power users. The command palette is a familiar solution to provide discoverable, fast, efficient, repeatable access to an app's capabilities.
📚 Description
This PR adds a command palette with access to every page, every action, and every capability of npmx.
It can be opened from anywhere in the app by pressing ⌘+K on macOS / Ctrl+K on Windows/Linux, or by clicking the new "quick actions" nav item in the header.
The palette includes a set of "global" commands and a composable allowing a page or component to register specific commands that should be made available when that page/component is visible.
The palette supports multi-step flows, such as "change language" → languages are listed.
I should've maybe kept this PR small and added more commands later, but... oops, I believe I covered every single page and capability:
All commands
Global commands (always available)
Package context
All pages with a package context also include:
Package page
Package code page
Package diff page
Compare page
Profile page
There are two behaviours worth calling out separately:
Search for "<query>". Selecting this submits a search for the user's query.The palette has full keyboard navigation support and screen reader support.
Screenshots
New header nav item (desktop)
Global commands (desktop)
Global commands — logged in via atproto (desktop)
Global commands (mobile, light)
Languages (desktop)
Accent colors (desktop)
Background shades (desktop, light)
New header nav item (desktop, non-homepage)
Package page commands (desktop)
Package page - input is valid semver (desktop)
Package code page (desktop)
Package diff page (desktop)
Compare page (desktop)
Profile page (desktop)
"Search for" fallback command (desktop)
Future work
rand hitting Enter to open the package's repo right away). We should probably do this eventually.