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.
2 Skipped Deployments
|
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
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a first-class command palette: a client component 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 |
![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 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
There was a problem hiding this comment.
I may be tripping, but I don't think this is a real issue
There was a problem hiding this comment.
@serhalp You're right, and I apologize for the noise. After closer analysis: queryOverrideRegistry is only ever read inside useCommandPaletteCommands, which is consumed exclusively by CommandPalette.client.vue — a client-only component that never executes server-side. Even though the registry can be written during SSR page setup, no SSR code path ever reads it, so the cross-request contamination scenario I described simply cannot occur. The concern was invalid.
✏️ Learnings added
Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 2159
File: app/composables/useCommandPalette.ts:14-15
Timestamp: 2026-03-24T12:14:10.506Z
Learning: In npmx-dev/npmx.dev (`app/composables/useCommandPalette.ts`), the module-level Maps (`queryOverrideRegistry`, `actionRegistry`, `actionIdsByScopeId`, `packageContextOpenRegistry`) are written to from server-capable pages during SSR setup, but they are **only read** by `useCommandPaletteCommands.ts`, which is consumed exclusively by `CommandPalette.client.vue`. Because the registry is never read server-side, there is no SSR cross-request contamination risk. Do not flag these registries for missing `import.meta.client` guards on write paths.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 2159
File: app/components/AppFooter.vue:8-8
Timestamp: 2026-03-24T02:08:02.618Z
Learning: In npmx.dev (app/components/AppFooter.vue), `shortcuts.command_palette_description` is a cross-platform description that hardcodes the macOS ⌘K shortcut inline and uses a `{ctrlKey}` interpolation parameter only for the Windows/Linux "Ctrl" label. `$t('shortcuts.ctrl_key')` always resolves to "Ctrl". Do not replace this with `primaryModifierKeyLabel` from `usePlatformModifierKey()`, as that would incorrectly render ⌘ for the Windows/Linux portion of the description on Apple devices.
Learnt from: danielroe
Repo: npmx-dev/npmx.dev PR: 1204
File: app/composables/npm/useUserPackages.ts:40-41
Timestamp: 2026-02-08T13:24:06.104Z
Learning: In Nuxt 3, when using useAsyncData or useLazyAsyncData, allow and rely on reactive keys by typing them as MaybeRefOrGetter<string>. Keys can be a string, a Ref<string>, a ComputedRef<string>, or a getter () => string. Reactive keys should automatically trigger refetches when their value changes. In reviews, verify that data scripts pass a MaybeRefOrGetter<string> (not a plain string-only key if reactivity is intended), and that the logic handles dynamic key changes without stale data.
Learnt from: MatteoGabriele
Repo: npmx-dev/npmx.dev PR: 1922
File: app/composables/usePackageListPreferences.ts:52-53
Timestamp: 2026-03-05T10:14:50.799Z
Learning: In Nuxt projects (here npmx-dev/npmx.dev), exports from shared/types/* (e.g., PAGE_SIZE_OPTIONS, DEFAULT_COLUMNS, DEFAULT_PREFERENCES, PageSize) are auto-imported by Nuxt for composables and components. Do not add explicit import statements for these constants/types when using files under shared/types/, and rely on the auto-imported bindings in files under app/composables (and similarly in components). This pattern applies to all TS files within app/composables that reference these shared/types exports.
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Outdated
Show resolved
Hide resolved
serhalp
force-pushed
the
serhalp/cmdk-palette
branch
from
4d4ddfb to
c9b5586
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/composables/useCommandPaletteCommands.ts (1)
23-223: SplituseCommandPaletteCommands()into smaller helpers.This composable now owns localisation, command normalisation, root search composition, Fuse wiring, query overrides, grouping, and view metadata in one place. Extracting a few of those steps into dedicated helpers/composables would make the reactive flow much easier to follow and keep the function closer to the repo guideline.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 133f648f-7f8f-42cc-a51b-5f94084a592e
📒 Files selected for processing (9)
app/composables/useCommandPaletteCommands.tsapp/composables/useCommandPalettePackageCommands.tsapp/composables/useCommandPalettePackageVersions.tsapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vueapp/pages/profile/[identity]/index.vueshared/utils/url.tstest/nuxt/a11y.spec.tstest/nuxt/composables/use-command-palette-commands.spec.tstest/nuxt/composables/use-command-palette-package-versions.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/nuxt/composables/use-command-palette-package-versions.spec.ts
- app/pages/profile/[identity]/index.vue
- app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
- app/composables/useCommandPalettePackageVersions.ts
| const { results } = useFuse(query, commands, { | ||
| fuseOptions: { | ||
| keys: ['label', 'keywords'], | ||
| threshold: 0.3, | ||
| ignoreLocation: true, | ||
| ignoreDiacritics: true, | ||
| }, | ||
| matchAllWhenSearchEmpty: true, | ||
| }) |
There was a problem hiding this comment.
Use trimmedQuery for Fuse matching.
Line 156 still feeds Fuse the raw query, while the rest of this composable already branches on trimmedQuery. That makes whitespace-only input inconsistent: commands falls back to the default root list, but Fuse can still filter that list out by searching for spaces.
Suggested fix
- const { results } = useFuse(query, commands, {
+ const { results } = useFuse(trimmedQuery, commands, {📝 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 { results } = useFuse(query, commands, { | |
| fuseOptions: { | |
| keys: ['label', 'keywords'], | |
| threshold: 0.3, | |
| ignoreLocation: true, | |
| ignoreDiacritics: true, | |
| }, | |
| matchAllWhenSearchEmpty: true, | |
| }) | |
| const { results } = useFuse(trimmedQuery, commands, { | |
| fuseOptions: { | |
| keys: ['label', 'keywords'], | |
| threshold: 0.3, | |
| ignoreLocation: true, | |
| ignoreDiacritics: true, | |
| }, | |
| matchAllWhenSearchEmpty: true, | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/composables/useCommandPaletteCommands.ts (1)
150-154: Consider avoiding spread in map for efficiency.The static analysis tool flags spreading to modify object properties in
mapcalls as inefficient. For a command palette with a small number of commands this is unlikely to be a bottleneck, but if performance becomes a concern, you could pre-allocate the keywords array or useObject.assign.Alternative approach
return base.map(command => { - return { - ...command, - keywords: [...command.keywords, groupLabel(command.group)], - } + const keywords = command.keywords.slice() + keywords.push(groupLabel(command.group)) + return Object.assign({}, command, { keywords }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b42d3e1-fc6d-4a49-bfad-32c393ac7152
📒 Files selected for processing (10)
app/composables/useCommandPaletteCommands.tsapp/composables/useCommandPalettePackageCommands.tsapp/composables/useCommandPalettePackageVersions.tsapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vueapp/pages/profile/[identity]/index.vuedocs/content/index.mdshared/utils/url.tstest/nuxt/a11y.spec.tstest/nuxt/composables/use-command-palette-commands.spec.tstest/nuxt/composables/use-command-palette-package-versions.spec.ts
✅ Files skipped from review due to trivial changes (2)
- docs/content/index.md
- test/nuxt/composables/use-command-palette-commands.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/nuxt/composables/use-command-palette-package-versions.spec.ts
- app/composables/useCommandPalettePackageVersions.ts
| const dialog = document.getElementById('command-palette-modal') | ||
| const announcer = document.getElementById('command-palette-modal-announcement') | ||
|
|
||
| expect(dialog).not.toBeNull() | ||
|
|
||
| const results = await runAxeElements([announcer, dialog]) | ||
| expect(results.violations).toEqual([]) |
There was a problem hiding this comment.
Assert announcer existence before running axe.
At Line 526, announcer may be null and is silently skipped by runAxeElements, so a missing live region would not fail this test.
Suggested fix
const dialog = document.getElementById('command-palette-modal')
const announcer = document.getElementById('command-palette-modal-announcement')
expect(dialog).not.toBeNull()
+ expect(announcer).not.toBeNull()
const results = await runAxeElements([announcer, dialog])
expect(results.violations).toEqual([])📝 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 dialog = document.getElementById('command-palette-modal') | |
| const announcer = document.getElementById('command-palette-modal-announcement') | |
| expect(dialog).not.toBeNull() | |
| const results = await runAxeElements([announcer, dialog]) | |
| expect(results.violations).toEqual([]) | |
| const dialog = document.getElementById('command-palette-modal') | |
| const announcer = document.getElementById('command-palette-modal-announcement') | |
| expect(dialog).not.toBeNull() | |
| expect(announcer).not.toBeNull() | |
| const results = await runAxeElements([announcer, dialog]) | |
| expect(results.violations).toEqual([]) |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/composables/useCommandPalettePackageCommands.ts (2)
58-110: Consider extracting repeated conditions to local variables.The same condition is evaluated twice for each command—once for
activeand again insideactiveLabel. This could be simplified by computing each condition once:const isPackagePage = ['package', 'package-version'].includes(`${route.name ?? ''}`) const isDocsPage = route.name === 'docs' const isCodePage = route.name === 'code' const isComparePage = route.name === 'compare' && isCurrentPackageCompare(resolvedContext.packageName)Then use these variables for both
activeandactiveLabelproperties. This reduces redundant evaluation and improves readability.
13-157: Consider extracting command builders to reduce function length.The main function exceeds 140 lines. While the logic is cohesive, you could improve maintainability by extracting helper functions for building each command group (e.g.,
buildBaseCommands,buildDownloadCommand,buildDiffCommand). This would align better with the guideline to keep functions "generally under 50 lines".That said, since this is a single computed block with clear structure, this is a nice-to-have rather than essential. As per coding guidelines: "Keep functions focused and manageable (generally under 50 lines)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 586e63cc-12af-44e9-9e55-e8503c257ccc
📒 Files selected for processing (1)
app/composables/useCommandPalettePackageCommands.ts
knowler
left a comment
knowler
left a comment
There was a problem hiding this comment.
This is a partial review, but this is looking good.
| <section | ||
| v-for="group in groupedCommands" | ||
| :key="group.id" | ||
| :aria-labelledby="`${dialogId}-group-${group.id}`" |
There was a problem hiding this comment.
These already have headings, so I don’t see a need to make them implicit role=region as well.
| :aria-labelledby="`${dialogId}-group-${group.id}`" |
| :aria-label="$t('command_palette.results_label')" | ||
| role="region" |
| :class=" | ||
| activeIndex === (commandIndexMap.get(command.id) ?? -1) | ||
| ? 'border-border/80 bg-bg' | ||
| : '' | ||
| " |
There was a problem hiding this comment.
Could these be applied with just CSS? I can see hover: ones, but could we add focus-visible: ones too?
| :aria-current="command.active ? 'true' : undefined" | ||
| @click="handleCommandClick(command)" | ||
| @focus="activeIndex = commandIndexMap.get(command.id) ?? -1" | ||
| @mouseenter="activeIndex = commandIndexMap.get(command.id) ?? -1" |
There was a problem hiding this comment.
Why should the active index change on mouseenter?
4 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.