diff --git a/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md b/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md new file mode 100644 index 00000000..4e4d09e3 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md @@ -0,0 +1,365 @@ +# Command Palette — Sort Modes + +> **Status:** in progress. Branch `feat/command-palette-sort-modes`. +> +> **Problem owner:** mouse-first browsing. The palette's empty-query list is ~99 +> rows in catalog registration order, with no visual structure. A keyboard user +> types three characters and the ranker does the work; a mouse user scrolls a +> flat undifferentiated list and scans. + +--- + +## 1. Why this exists + +The palette has exactly one ordering story today, and it is optimized for the +keyboard: + +- **Typing** → `rankEntries` tier ladder (prefix > substring > acronym > + keyword > body > subsequence). Excellent. +- **Not typing** → catalog registration order, verbatim, with starred commands + hard-partitioned to the top. + +Catalog order is *deliberate* — `catalog.ts` calls it a user-visible invariant +and `catalog.test.ts` pins it — but it is deliberate about **authoring +adjacency** ("like things stay adjacent"), not about **findability**. A user who +does not know the command's name gets no affordance at all: no alphabet to +binary-search, no categories to narrow by eye. + +This is the gap. It is a *browse* problem, not a *search* problem, and it only +bites when the query is empty. + +### Why not a filter control + +The obvious second half — filter chips per surface — is deliberately **not** +built. Grouping already answers the question filtering would answer ("show me +only session commands") without adding state, without a second control to keep +consistent with the first, and without a mode where the user can filter +themselves into an empty list and not know why. If browsing by group turns out +to be insufficient, filtering is a separate, later change. + +--- + +## 2. The design + +**One control. Four modes. Empty query only.** + +### 2.1 Modes + +| Mode | Order | Serves | +|---|---|---| +| `catalog` | Catalog registration order (today's behavior) | the existing muscle memory; the default | +| `alpha` | Title, `localeCompare` | "I know the name, I just can't spot it" | +| `grouped` | By `category`, section headers, alphabetical within group | **browsing / discovery** — the actual reported pain | +| `recent` | History score DESC, then catalog order | muscle memory, made explicit | + +`grouped` is the one that motivated the feature. + +**Grouping keys on `CommandCategory`, not `CommandSurface`.** The first +implementation used `surface` and that was wrong on both counts: + +- *Correctness.* `CommandCategory`'s doc comment records that `surface` is a + MACHINE applicability dimension driving mode gating, and that reading it as a + category is a conflation the codebase already paid for once in the Settings + list — "once one field means both, you cannot reclassify a command's + presentation without changing when it applies." +- *Fitness.* By surface the buckets are app 41 / dispatch 34 / session 32 / + grid 11 / debug 11 / editor 9, and since grid and dispatch are mutually + exclusive one section holds ~40% of the visible list — barely a narrowing. By + category they are session 24 / layout-dispatch 16 / navigate 12 / + developer 12 / workspace-tools 11 / editor-files 10 / create 10 / + preferences 3. + +`category` is optional on `CommandDef` until the governance migration completes, +and extension-contributed commands cannot declare one, so uncategorized rows get +a trailing **Other** section rather than being silently dropped from the one +mode built for discovery. + +### 2.2 The cardinal rule is preserved + +**Sorting applies to the empty-query browse state only.** The moment the user +types, `rankEntries` relevance ordering wins outright and the sort mode is inert. + +This is not a limitation to be lifted later — it is the same invariant +`rankEntries` and `rankCommands` already defend in three separate comment blocks: +*a text match always beats every other signal*. A sort mode that reordered search +results would let "A–Z" push a tier-5 prefix match below a tier-1 subsequence +match, which is precisely the inversion class this subsystem was rewritten to +eliminate. + +The UI makes this legible rather than mysterious: while a query is present the +control **shows `Relevance` and disables itself**. The user is told what the +ordering is instead of wondering why their chosen sort stopped applying. + +### 2.3 Composition with starring + +Starring already perturbs the resting order — `rankCommands` hard-partitions +starred commands to the top on an empty query, and its comment block explicitly +defends that against the "resting order must not shuffle" rule (a star is a +deliberate act by the person now looking at the list). + +Sort modes compose **inside** that partition rather than replacing it: + +``` + ┌─ starred ────────────┐ + │ sorted by mode │ ← stars stay pinned; sorting orders within + ├─ everything else ────┤ + │ sorted by mode │ + └──────────────────────┘ +``` + +In `grouped` mode the partition simply becomes visible as a leading **`★ +STARRED`** section, which is more honest than the current invisible split. + +Rationale: starring answers *"which commands are mine"*; sorting answers *"how +do I want to scan the list"*. They are orthogonal questions and neither should +silently cancel the other. Making sort override starring would mean the user's +explicit pins vanish the moment they pick A–Z — an obviously wrong outcome. + +### 2.4 Layout + +Header, `commands` mode, empty query: + +``` +╔═══════════════════════════════════════════════════════════════╗ +║ Type a command… [ ⇅ Grouped ▾ ] ║ +╠═══════════════════════════════╤═══════════════════════════════╣ +``` + +Open menu: + +``` + ┌──────────────────────────┐ + │ ✓ Catalog order │ + │ A – Z │ + │ Grouped │ + │ Recently used │ + └──────────────────────────┘ +``` + +With a query present: + +``` +║ reader [ ⇅ Relevance ] ║ + ▲ disabled, title= + "Sorting applies when + the search box is empty" +``` + +`grouped` list body: + +``` +║ ── ★ STARRED ────────────────── ║ +║ ★ Reader Mode ⟨ON⟩ ⌘⇧R ║ +║ ── CREATE ───────────────────── ║ +║ New Agent… ⌘N ║ +║ New Tab ⌘T ║ +║ ── SESSION ──────────────────── ║ +║ Reload Agent ║ +║ Rewind to Prompt… ║ +``` + +The `★` column and the starred-block divider come from the starring feature +(#619) and are preserved: in `grouped` mode the divider is suppressed, because a +labelled `★ Starred` heading already says what the unlabelled rule was there to +imply. + +The control sits in the same header slot `Manage` already occupies in +`prompt-template` mode, so no new layout geometry is introduced. + +--- + +## 3. Architecture + +### 3.1 New module: `lib/sortCommands.ts` + +Pure, no React, no storage, no `Date.now()` — same contract as its neighbours +`rankEntries.ts` and `rankCommands.ts`. + +```ts +export type CommandSortMode = 'catalog' | 'alpha' | 'grouped' | 'recent' + +browseOrder(commands, mode, historyScore, starred): BrowseOrder +groupCommands(commands, starred): CommandGroup[] // grouped mode only +``` + +`groupCommands` returns `{ label, commands }[]`, so the component renders +headers without knowing the category taxonomy. Group order is fixed and +declared in the module: + +``` +★ Starred · Create · Navigate · Session · Layout & Dispatch · +Editor & Files · Workspace Tools · Preferences · Developer · Other +``` + +`grid` and `dispatch` are mutually exclusive at runtime (`surfaceAvailable` in +`registry.ts` hides one or the other), so at most one of those two ever renders. +Empty groups are dropped. + +### 3.2 `rankCommands` gains a mode parameter + +```ts +rankCommands(commands, query, historyScore, starred, sortMode) +``` + +- `query.length > 0` → unchanged. Returns `rankEntries` output verbatim. +- `query.length === 0` → partition by star (existing behavior), then + `sortCommands` each half. + +The existing star partition stays exactly where it is and keeps its comment +block. This change adds a step *inside* each half; it does not restructure the +partition. + +### 3.3 Selection model — headers must not break it + +`selectedIndex` indexes `paletteCommands`, a flat array. Grouped mode inserts +header elements into the DOM but **must not** make them selectable: arrow keys, +Enter, hover and the clamp effect all stay index-over-commands. + +So: `paletteCommands` stays flat and authoritative. Grouping is a **render-time +concern only** — a `Map` of "header to draw before row *i*", +consumed inside the existing `.map()`. Nothing in the keyboard handler changes. + +### 3.4 The `scrollIntoView` fix (opportunistic, in blast radius) + +```ts +const el = listRef.current.children[selectedIndex] // ← positional +``` + +This assumes list children map 1:1 onto `selectedIndex`. That is **already +false**: `ai-workspace-open`/`clear` render an error banner as a sibling child of +the same container, so while an error is showing every scroll target is off by +one. Grouped mode's headers would make it wrong in a fourth mode. + +Fix once, properly: every selectable row in every mode gets +`data-palette-row={i}`, and the effect resolves by attribute: + +```ts +listRef.current.querySelector(`[data-palette-row="${selectedIndex}"]`) +``` + +Positional indexing into rendered children was a latent bug waiting for exactly +this kind of change; the attribute makes the row's identity explicit and immune +to sibling chrome. + +### 3.5 Persistence + +`Settings.commandSortMode: CommandSortMode`, default `'catalog'`. + +Coerced in `persistence.ts` against the valid set, falling back to `'catalog'` on +anything unrecognized — same shape as the existing `coerceCommandStarred` / +`coerceCommandVisibilityOverrides` guards, and for the same reason: a +hand-edited or version-skewed blob must degrade, never throw into render. + +Default `'catalog'` keeps the change **purely additive**. Fresh installs and +existing users see the exact palette they see today until they choose otherwise. + +### 3.6 New component: `ui/CommandSortControl.tsx` + +Its own file rather than another closure inside a 2100-line component — the +palette is already too big, and a self-contained popover with focus and +click-outside handling is exactly the kind of unit that should be readable on its +own. + +Focus discipline, which is the whole difficulty: + +- `onMouseDown` → `preventDefault()` on the button and every menu item, so the + search input **never loses focus**. Typing immediately after picking a sort + must work. +- Click-outside closes, via a `pointerdown` listener on `document` while open. + `pointerdown` rather than `click`, so a press landing on a palette row closes + the menu before that row's command can run. +- **All menu keys are handled on `document` in the CAPTURE phase.** + +That last point was the design's one real mistake, caught in review. The first +implementation put a React `onKeyDown` on the control's root — which is +unreachable dead code, because `keepFocusInSearchInput` guarantees focus never +enters the subtree, and the search input is a *sibling* of the control, not a +descendant. Three keys went to the wrong widget: + +| Key | Where it actually went | +|---|---| +| Escape | Radix's dismiss handler — **closed the whole palette** | +| ↑ / ↓ | moved the selection in the list *behind* the open menu | +| Enter | ran `paletteCommands[selectedIndex]` from that hidden list | + +Capture-phase on `document` beats both competitors: React 18 delegates synthetic +events to the root container (a descendant of `document`, so later), and Radix's +dismiss layer listens on `document` in the bubble phase (later still). One +listener therefore fixes all three, and makes the menu genuinely keyboard-operable +— which its ARIA roles were already promising. + +The keyboard cursor is component state, not DOM focus, so the highlight is +painted from `highlighted` rather than `:focus`; hover writes to the same state, +exactly as the palette's own rows do. + +--- + +## 4. Files + +| File | Change | +|---|---| +| `lib/sortCommands.ts` | **new** — modes, sorting, grouping, labels | +| `lib/sortCommands.test.ts` | **new** — pure unit tests | +| `lib/rankCommands.ts` | mode param; sort inside each star partition | +| `lib/rankCommands.test.ts` | **new** — star × sort composition, query-wins invariant | +| `ui/CommandSortControl.tsx` | **new** — button + popover | +| `ui/CommandSortControl.renderer.test.tsx` | **new** — the keyboard contract | +| `ui/CommandPalette.tsx` | wire control, render group headers, `data-palette-row`, scroll fix | +| `registry.ts` | carry `category` through to `ResolvedCommand` | +| `types.ts` | `ResolvedCommand.category`; note that `surface` is not a grouping axis | +| `app-state/settings/types.ts` | `commandSortMode` field + default | +| `app-state/settings/persistence.ts` | `coerceCommandSortMode` | + +--- + +## 5. Testing + +Colocated, per `testing/README.md`. The valuable surface here is pure, so the +tests are pure: + +**`sortCommands.test.ts`** +- `catalog` returns input order by reference-equal content (identity behavior). +- `alpha` sorts by title; ties fall back to catalog index deterministically. +- `recent` puts history-scored commands first, unscored keep catalog order. +- `grouped` emits groups in the declared fixed order, drops empty groups, + sorts alphabetically within a group. +- `grouped` emits `★ Starred` first and only when something is starred. +- `grouped` keys on **category, not surface** — every fixture shares one + surface, so a regression to the old axis collapses them into one section. +- Uncategorized commands land in a trailing `Other` section rather than + vanishing. + +**`rankCommands.test.ts`** +- **The invariant:** a non-empty query ignores sort mode entirely — results are + asserted *byte-identical* across all four modes, not merely "the winner stayed + on top". +- Starred commands stay partitioned above unstarred under every mode. +- Sorting applies within both partitions, not across them. + +**`CommandSortControl.renderer.test.tsx`** + +This file exists because the plan originally declined it, and review found +exactly what that let through. The reasoning — "a test asserting a menu opens on +click pins the implementation, not the contract" — was right about *opening* and +wrong about *keys*: Escape closing the whole palette, and ↑/↓/Enter driving the +list behind an open menu, are contract violations a user can observe. + +The tests reproduce the real DOM relationship (search input as a **sibling** of +the control, focus in the input) and assert behavior, not structure. All of them +fail against the pre-review implementation. + +--- + +## 6. What this deliberately does not do + +- **No filter control.** See §1. Grouping subsumes the need; two controls is a + consistency burden for a benefit nobody has asked for yet. +- **No sort in the other four palette lists** (sessions, buried, templates, AI + workspaces). They are short, already have meaningful intrinsic orders + (recency, `[...custom, ...builtin]`), and none of them is the reported pain. +- **No keyboard shortcut to cycle sort.** This is a mouse-first affordance by + construction; a keyboard user types and gets relevance, which is better than + any sort. Adding a chord would spend a scarce binding on the users who need it + least. +- **No per-mode memory** (e.g. "grouped in commands mode, alpha elsewhere"). + One setting, one behavior. diff --git a/src/renderer/src/app-state/settings/persistence.ts b/src/renderer/src/app-state/settings/persistence.ts index aeb54480..8e26d0a0 100644 --- a/src/renderer/src/app-state/settings/persistence.ts +++ b/src/renderer/src/app-state/settings/persistence.ts @@ -17,6 +17,8 @@ import { } from '@renderer/app-state/settings/savedThemes' import type { SavedTheme } from '@renderer/app-state/settings/savedThemes' import { parseCustomAppearanceJson } from '@renderer/app-state/settings/customAppearance' +import { isCommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' +import type { CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' import type { AccentId, FontFamilyId, @@ -138,6 +140,7 @@ export function coerceSettings(value: unknown): Settings { // shape. A garbage value collapses to `{}` (nothing overridden), // matching the "absent ≡ declared default" semantic. commandStarred: coerceCommandStarred(parsed.commandStarred), + commandSortMode: coerceCommandSortMode(parsed.commandSortMode), mouseModeEnabled: parsed.mouseModeEnabled === true, commandVisibilityOverrides: coerceCommandVisibilityOverrides( parsed.commandVisibilityOverrides, @@ -289,6 +292,22 @@ function coerceCommandStarred(value: unknown): Record { return result } +/** + * Fall back to the shipped default on anything unrecognized. + * + * The union is closed, but the blob it is read from is not: a settings file + * written by a future build (a fifth mode), hand-edited in devtools, or + * truncated mid-write can all put a string here that no longer means anything. + * An unknown value must degrade to 'catalog' — the behavior the palette had + * before this setting existed. Letting one through would NOT be harmless: it + * falls past the 'catalog' and 'alpha' branches in `orderFlat` and lands on the + * history sort, so an unrecognized mode would silently render as "recently + * used" while the control displayed whatever string it read. + */ +function coerceCommandSortMode(value: unknown): CommandSortMode { + return isCommandSortMode(value) ? value : DEFAULT_SETTINGS.commandSortMode +} + function coerceCommandVisibilityOverrides(value: unknown): Record { if (!value || typeof value !== 'object' || Array.isArray(value)) return {} const result: Record = {} diff --git a/src/renderer/src/app-state/settings/types.ts b/src/renderer/src/app-state/settings/types.ts index 24b03dc4..bd843517 100644 --- a/src/renderer/src/app-state/settings/types.ts +++ b/src/renderer/src/app-state/settings/types.ts @@ -5,6 +5,7 @@ import type { ColorFlagId } from '@renderer/app-state/settings/dispatchColorFlag import type { DictationProvider } from '@shared/types/dictation' import type { MouseButtonBinding, MouseChordBinding } from '@renderer/lib/mouseBinding' import type { ConfigurableBuiltInMcpDomain } from '@mcp/shared/types' +import type { CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' // Built-in theme ids only. 'custom' used to live here as a sentinel that // rendered as a picker cell but acted as a button (it opened the JSON editor @@ -411,6 +412,24 @@ export type Settings = { * two lifecycles beats one map that forces the cheap one to pay the * expensive one's cost. */ commandStarred: Record + /** How the command palette orders its list while the search box is EMPTY. + * + * Scope is the whole point: this never touches search results. Typing is + * answered by relevance alone (`rankEntries`), and a sort applied on top of + * that would let 'A – Z' push a tier-5 prefix match below a tier-1 + * subsequence match — the inversion class the ranking rewrite exists to + * prevent. `rankCommands` enforces the split with an early return; the + * header control shows "Relevance" and disables itself while a query is + * present so the user is told, not left guessing. + * + * WHY a persisted setting rather than per-open state: it is a scanning + * preference, not a transient one. Someone who browses by group wants to + * browse by group tomorrow too, and re-picking it on every palette open + * would make the feature not worth using. + * + * Defaults to 'catalog' — today's exact behavior — which keeps the whole + * feature additive: nothing about the palette changes until the user asks. */ + commandSortMode: CommandSortMode /** Show pointer-only affordances that a keyboard user does not need — today * the composer's Send and Stop buttons. * @@ -530,6 +549,9 @@ export const DEFAULT_SETTINGS: Settings = { // Nothing starred until the user stars something. The palette's resting // order is otherwise exactly the catalog order it has always been. commandStarred: {}, + // Catalog order is what the palette has always shown, so it stays the + // default and the sort control is a pure opt-in. + commandSortMode: 'catalog', // Off by default. The composer buttons cost ~28px of pane height per pane // and a keyboard user gets nothing from them, so this is opt-in. mouseModeEnabled: false, diff --git a/src/renderer/src/features/command-palette/lib/rankCommands.test.ts b/src/renderer/src/features/command-palette/lib/rankCommands.test.ts new file mode 100644 index 00000000..8503f17f --- /dev/null +++ b/src/renderer/src/features/command-palette/lib/rankCommands.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest' + +import { rankCommands } from '@renderer/features/command-palette/lib/rankCommands' +import { COMMAND_SORT_MODES } from '@renderer/features/command-palette/lib/sortCommands' +import type { ResolvedCommand } from '@renderer/features/command-palette/types' + +function cmd(id: string, title: string, keywords: string[] = []): ResolvedCommand { + return { + id, + title, + description: `${title} description`, + surface: 'app', + keywords, + keepPaletteOpen: false, + state: null, + run: () => {}, + } +} + +const titles = (commands: readonly ResolvedCommand[]): string[] => + commands.map(command => command.title) + +const NO_HISTORY = new Map() +const NOTHING_STARRED: Record = {} + +describe('rankCommands — search always beats the sort mode', () => { + // THE invariant this feature had to not break. A sort mode governs the browse + // state only; the moment a query exists, relevance owns the ordering. If this + // ever fails, 'A – Z' is capable of pushing a tier-5 prefix match below a + // tier-1 subsequence match — the inversion class `rankEntries` was extracted + // to eliminate. + const commands = [ + // Deliberately arranged so that alphabetical, catalog and recent orders all + // disagree with relevance: the exact-prefix match sorts LAST alphabetically + // and sits LAST in catalog order. + cmd('apple', 'Apple Something'), + cmd('banana', 'Banana Thing'), + cmd('zebra', 'Zebra Mode'), + ] + const history = new Map([ + ['apple', 0.95], + ['banana', 0.9], + ]) + const starred = { apple: true, banana: true } + + it('puts the prefix match first under every sort mode', () => { + for (const mode of COMMAND_SORT_MODES) { + const result = rankCommands(commands, 'zebra', history, starred, mode) + expect(result.commands[0]?.title, `mode: ${mode}`).toBe('Zebra Mode') + } + }) + + it('is byte-identical across sort modes for any given query', () => { + // Stronger than the check above: the sort mode must have NO observable + // effect at all while searching, not merely leave the winner in place. + const baseline = rankCommands(commands, 'an', history, starred, 'catalog') + for (const mode of COMMAND_SORT_MODES) { + const result = rankCommands(commands, 'an', history, starred, mode) + expect(titles(result.commands), `mode: ${mode}`).toEqual(titles(baseline.commands)) + } + }) + + it('never emits section headers for a search result', () => { + // Headers describe a browse structure; a relevance-ordered list has none. + const result = rankCommands(commands, 'a', NO_HISTORY, NOTHING_STARRED, 'grouped') + expect(result.commands.length).toBeGreaterThan(0) + expect(result.headers.size).toBe(0) + }) +}) + +describe('rankCommands — browse state applies the sort mode', () => { + const commands = [cmd('c', 'Charlie'), cmd('a', 'Alpha'), cmd('b', 'Bravo')] + + it('defaults to catalog order when no mode is supplied', () => { + // The parameter is optional so existing call sites (and the native-menu + // resolution path) keep today's behavior without being touched. + const result = rankCommands(commands, '', NO_HISTORY, NOTHING_STARRED) + expect(titles(result.commands)).toEqual(['Charlie', 'Alpha', 'Bravo']) + }) + + it('sorts alphabetically on an empty query when asked', () => { + const result = rankCommands(commands, '', NO_HISTORY, NOTHING_STARRED, 'alpha') + expect(titles(result.commands)).toEqual(['Alpha', 'Bravo', 'Charlie']) + }) + + it('still hoists starred commands ahead of the sort', () => { + const result = rankCommands(commands, '', NO_HISTORY, { c: true }, 'alpha') + expect(titles(result.commands)).toEqual(['Charlie', 'Alpha', 'Bravo']) + }) +}) diff --git a/src/renderer/src/features/command-palette/lib/rankCommands.ts b/src/renderer/src/features/command-palette/lib/rankCommands.ts index 0f1c422c..714b3b25 100644 --- a/src/renderer/src/features/command-palette/lib/rankCommands.ts +++ b/src/renderer/src/features/command-palette/lib/rankCommands.ts @@ -1,5 +1,10 @@ import type { ResolvedCommand } from '@renderer/features/command-palette/types' import { primary, rankEntries, secondary } from '@renderer/features/command-palette/lib/rankEntries' +// EMPTY_HEADERS is shared rather than re-declared: search results are never +// sectioned either, and headers describe a browse structure a relevance-ordered +// list does not have. +import { browseOrder, EMPTY_HEADERS } from '@renderer/features/command-palette/lib/sortCommands' +import type { BrowseOrder, CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' // rankCommands — the ordering function for the command-palette's command // list specifically. The generic relevance machinery now lives in @@ -28,8 +33,15 @@ import { primary, rankEntries, secondary } from '@renderer/features/command-pale // // Empty-query behavior (registry order, no history reordering) also lives in // `rankEntries` now, for the same reason it did here: the palette's resting -// state must not shuffle. Starred commands are the ONE documented exception — -// see the WHY on the partition below before assuming that is a bug. +// state must not shuffle by itself. Two documented exceptions, both of which +// are the user asking for it rather than the app deciding on its own: +// +// 1. STARRED commands hoist to the top — see the WHY on the partition below. +// 2. The SORT MODE (`sortCommands.ts`) reorders the browse list on request. +// +// Neither can touch a SEARCH result. Both are applied only on the empty-query +// path, below the `query.length > 0` early return, which is what keeps "a text +// match always beats every other signal" true. // Re-exported because the palette's other call sites import `fuzzyMatch` // from here. Its real definition — and the warning about never running it @@ -53,7 +65,8 @@ export function rankCommands( query: string, historyScore: Map, starred: Record, -): ResolvedCommand[] { + sortMode: CommandSortMode = 'catalog', +): BrowseOrder { const ranked = rankEntries( commands, query, @@ -61,13 +74,24 @@ export function rankCommands( command => (starred[command.id] ? STAR_WEIGHT : 0) + (historyScore.get(command.id) ?? 0), ) - if (query.length > 0) return ranked + // THE INVARIANT, and the reason `sortMode` is not consulted anywhere above: + // a non-empty query is answered by relevance alone. The sort mode governs the + // BROWSE state only. + // + // This is not a limitation waiting to be lifted. Applying a sort to search + // results would let 'A – Z' place a tier-1 subsequence match above the tier-5 + // prefix match the user typed in full — the exact inversion class that + // `rankEntries` was extracted to eliminate (see the plan doc referenced in + // its header). The control in the header shows "Relevance" and disables + // itself while a query is present, so this is legible to the user rather than + // looking like the setting stopped working. + if (query.length > 0) return { commands: ranked, headers: EMPTY_HEADERS } // Empty query needs its own handling, because `rankEntries` short-circuits // and returns the input verbatim WITHOUT sorting — so the extraTiebreak // above is dead here. // - // WHY the partition lives in this adapter and not in `rankEntries`: five + // WHY the star partition lives in this adapter and not in `rankEntries`: five // lists share that short-circuit (commands, prompt templates, sessions, // buried tabs, AI workspaces). Editing it would silently reshuffle four // resting orders nobody asked to change. @@ -80,10 +104,11 @@ export function rankCommands( // the list, so the top row moving is precisely what they asked for. DO NOT // "fix" this back to match the sibling rule without reading this paragraph. // - // Two filter passes rather than a sort: this is a STABLE partition, so - // catalog registration order is preserved within each half — and that order - // is a declared user-visible invariant (catalog.ts). - const starredRows = ranked.filter(command => starred[command.id]) - if (starredRows.length === 0) return ranked - return [...starredRows, ...ranked.filter(command => !starred[command.id])] + // The partition itself now lives in `browseOrder`, which owns every + // browse-state ordering decision (star split, the four sort modes, and the + // section headers that must stay in lockstep with the flat order). It still + // preserves catalog registration order within each half when the mode is + // 'catalog' — that order is a declared user-visible invariant (catalog.ts) + // and remains the default. + return browseOrder(ranked, sortMode, historyScore, starred) } diff --git a/src/renderer/src/features/command-palette/lib/sortCommands.test.ts b/src/renderer/src/features/command-palette/lib/sortCommands.test.ts new file mode 100644 index 00000000..41ff94ee --- /dev/null +++ b/src/renderer/src/features/command-palette/lib/sortCommands.test.ts @@ -0,0 +1,277 @@ +import { describe, expect, it } from 'vitest' + +import { + browseOrder, + groupCommands, + isCommandSortMode, + STARRED_GROUP_LABEL, +} from '@renderer/features/command-palette/lib/sortCommands' +import type { CommandCategory, ResolvedCommand } from '@renderer/features/command-palette/types' + +// Minimal command factory. Only the four fields the sorter reads are real — +// everything else is filler, deliberately, so a change to `ResolvedCommand`'s +// unrelated fields cannot break these tests. +// +// `category` is what grouping keys on; `surface` is fixed at 'app' throughout +// precisely to prove grouping does NOT consult it. +function cmd(id: string, title: string, category?: CommandCategory): ResolvedCommand { + return { + id, + title, + description: `${title} description`, + surface: 'app', + category, + keywords: [], + keepPaletteOpen: false, + state: null, + run: () => {}, + } +} + +const titles = (commands: readonly ResolvedCommand[]): string[] => + commands.map(command => command.title) + +const NO_HISTORY = new Map() +const NOTHING_STARRED: Record = {} + +describe('isCommandSortMode', () => { + it('accepts the four shipped modes and rejects anything else', () => { + expect(isCommandSortMode('catalog')).toBe(true) + expect(isCommandSortMode('alpha')).toBe(true) + expect(isCommandSortMode('grouped')).toBe(true) + expect(isCommandSortMode('recent')).toBe(true) + + // The cases persistence actually has to survive: a mode from a future + // build, a hand-edited value, a truncated blob. + expect(isCommandSortMode('by-vibes')).toBe(false) + expect(isCommandSortMode(undefined)).toBe(false) + expect(isCommandSortMode(null)).toBe(false) + expect(isCommandSortMode(3)).toBe(false) + }) +}) + +describe('browseOrder — catalog', () => { + it('returns catalog order untouched', () => { + const commands = [cmd('c', 'Zebra'), cmd('a', 'Apple'), cmd('b', 'Mango')] + const result = browseOrder(commands, 'catalog', NO_HISTORY, NOTHING_STARRED) + + expect(titles(result.commands)).toEqual(['Zebra', 'Apple', 'Mango']) + expect(result.headers.size).toBe(0) + }) + + it('does not hand back the caller array itself', () => { + // The palette memoizes on this result; returning the input by reference + // would let a downstream mutation reach the registry's array. + const commands = [cmd('a', 'Apple')] + expect(browseOrder(commands, 'catalog', NO_HISTORY, NOTHING_STARRED).commands).not.toBe(commands) + }) +}) + +describe('browseOrder — alpha', () => { + it('sorts by title', () => { + const commands = [cmd('c', 'Zebra'), cmd('a', 'Apple'), cmd('b', 'Mango')] + const result = browseOrder(commands, 'alpha', NO_HISTORY, NOTHING_STARRED) + + expect(titles(result.commands)).toEqual(['Apple', 'Mango', 'Zebra']) + }) + + it('orders embedded numbers numerically, not lexically', () => { + // Lexical sorting puts "Focus Pane 10" before "Focus Pane 2", which reads + // as broken in a list people scan by eye. + const commands = [cmd('b', 'Focus Pane 10'), cmd('a', 'Focus Pane 2')] + const result = browseOrder(commands, 'alpha', NO_HISTORY, NOTHING_STARRED) + + expect(titles(result.commands)).toEqual(['Focus Pane 2', 'Focus Pane 10']) + }) + + it('breaks title ties by catalog index rather than leaving it to Array.sort', () => { + // Duplicate titles are reachable: per-provider generated commands, and + // state-dependent `title(ctx)` values that resolve to the same string. + const commands = [cmd('first', 'Same'), cmd('second', 'Same'), cmd('third', 'Same')] + const result = browseOrder(commands, 'alpha', NO_HISTORY, NOTHING_STARRED) + + expect(result.commands.map(command => command.id)).toEqual(['first', 'second', 'third']) + }) +}) + +describe('browseOrder — recent', () => { + it('floats scored commands and leaves the unscored tail in catalog order', () => { + const commands = [ + cmd('never-1', 'Never One'), + cmd('often', 'Often'), + cmd('never-2', 'Never Two'), + cmd('sometimes', 'Sometimes'), + ] + const history = new Map([ + ['often', 0.9], + ['sometimes', 0.4], + ]) + + const result = browseOrder(commands, 'recent', history, NOTHING_STARRED) + + // Scored first, highest first; the two unscored keep their catalog + // positions relative to each other. + expect(titles(result.commands)).toEqual(['Often', 'Sometimes', 'Never One', 'Never Two']) + }) +}) + +describe('browseOrder — starring composes with sorting', () => { + const commands = [ + cmd('zebra', 'Zebra'), + cmd('apple', 'Apple'), + cmd('mango', 'Mango'), + cmd('kiwi', 'Kiwi'), + ] + const starred: Record = { mango: true, zebra: true } + + it('keeps starred commands above unstarred ones under every flat mode', () => { + for (const mode of ['catalog', 'alpha', 'recent'] as const) { + const result = browseOrder(commands, mode, NO_HISTORY, starred) + const starredCount = result.commands.filter(command => starred[command.id]).length + + expect(starredCount).toBe(2) + // Both stars occupy the first two slots, whatever the mode did inside + // each half. Choosing A–Z must never silently unpin anything. + expect(result.commands.slice(0, 2).every(command => starred[command.id])).toBe(true) + } + }) + + it('applies the sort within each partition, not across them', () => { + const result = browseOrder(commands, 'alpha', NO_HISTORY, starred) + + // Starred half sorted A–Z, then unstarred half sorted A–Z. If the sort ran + // across the whole list, Apple would be first overall. + expect(titles(result.commands)).toEqual(['Mango', 'Zebra', 'Apple', 'Kiwi']) + }) +}) + +describe('browseOrder — grouped', () => { + const commands = [ + cmd('sess-b', 'Reload Agent', 'session'), + cmd('pref-a', 'Open Settings', 'preferences'), + cmd('sess-a', 'Copy Last Response', 'session'), + cmd('create-a', 'New Tab', 'create'), + cmd('dev-a', 'Save Debug Logs', 'developer'), + ] + + it('emits sections in the declared order with rows alphabetical inside each', () => { + const result = browseOrder(commands, 'grouped', NO_HISTORY, NOTHING_STARRED) + + expect(titles(result.commands)).toEqual([ + 'New Tab', + 'Copy Last Response', + 'Reload Agent', + 'Open Settings', + 'Save Debug Logs', + ]) + expect([...result.headers.entries()]).toEqual([ + [0, 'Create'], + [1, 'Session'], + [3, 'Preferences'], + [4, 'Developer'], + ]) + }) + + it('groups by category and NOT by surface', () => { + // Every command here shares surface 'app' and differs only by category. If + // grouping ever regresses to keying on `surface` — the conflation + // CommandCategory exists to prevent — this collapses to one section. + const result = browseOrder(commands, 'grouped', NO_HISTORY, NOTHING_STARRED) + expect(result.headers.size).toBe(4) + }) + + it('drops empty groups instead of rendering a heading over nothing', () => { + const result = browseOrder( + [cmd('only', 'Only One', 'editor-files')], + 'grouped', + NO_HISTORY, + NOTHING_STARRED, + ) + + expect([...result.headers.values()]).toEqual(['Editor & Files']) + }) + + it('keeps uncategorized commands visible in a trailing Other section', () => { + // Category is optional on CommandDef until the governance migration lands, + // and extension-contributed commands cannot declare one at all. Dropping + // them would silently hide working commands from the one mode built for + // discovery. + const result = browseOrder( + [cmd('mystery', 'Mystery Command'), cmd('known', 'Known Command', 'create')], + 'grouped', + NO_HISTORY, + NOTHING_STARRED, + ) + + expect(titles(result.commands)).toEqual(['Known Command', 'Mystery Command']) + expect([...result.headers.entries()]).toEqual([ + [0, 'Create'], + [1, 'Other'], + ]) + }) + + it('leads with a starred section and pulls those rows out of their categories', () => { + const result = browseOrder(commands, 'grouped', NO_HISTORY, { 'sess-b': true }) + + expect(result.headers.get(0)).toBe(STARRED_GROUP_LABEL) + expect(result.commands[0]?.title).toBe('Reload Agent') + // Session still exists with its remaining member, but no longer holds the + // starred row. + expect(titles(result.commands)).toEqual([ + 'Reload Agent', + 'New Tab', + 'Copy Last Response', + 'Open Settings', + 'Save Debug Logs', + ]) + }) + + it('anchors every header on the first row of its section', () => { + // The invariant that makes headers safe to render inside the flat list: a + // header index always points at a real command. + const result = browseOrder(commands, 'grouped', NO_HISTORY, { 'create-a': true }) + + for (const [index] of result.headers) { + expect(result.commands[index]).toBeDefined() + } + expect(result.headers.size).toBe(4) // Starred + Session + Preferences + Developer + }) +}) + +describe('groupCommands', () => { + it('preserves the incoming order of starred rows rather than re-sorting them', () => { + // A starred row's position is the one thing the user has expressed a direct + // opinion about. + const commands = [cmd('z', 'Zebra'), cmd('a', 'Apple')] + const groups = groupCommands(commands, { z: true, a: true }) + + expect(groups).toHaveLength(1) + expect(groups[0]?.label).toBe(STARRED_GROUP_LABEL) + expect(titles(groups[0]?.commands ?? [])).toEqual(['Zebra', 'Apple']) + }) + + it('places every category in the declared browse order', () => { + // Pins the full ordering, so a reshuffle of CATEGORY_ORDER is a deliberate + // act with a failing test attached rather than a silent UI change. + const oneEach = [ + cmd('h', 'H', 'developer'), + cmd('g', 'G', 'preferences'), + cmd('f', 'F', 'workspace-tools'), + cmd('e', 'E', 'editor-files'), + cmd('d', 'D', 'layout-dispatch'), + cmd('c', 'C', 'session'), + cmd('b', 'B', 'navigate'), + cmd('a', 'A', 'create'), + ] + expect(groupCommands(oneEach, NOTHING_STARRED).map(group => group.label)).toEqual([ + 'Create', + 'Navigate', + 'Session', + 'Layout & Dispatch', + 'Editor & Files', + 'Workspace Tools', + 'Preferences', + 'Developer', + ]) + }) +}) diff --git a/src/renderer/src/features/command-palette/lib/sortCommands.ts b/src/renderer/src/features/command-palette/lib/sortCommands.ts new file mode 100644 index 00000000..ab18a37d --- /dev/null +++ b/src/renderer/src/features/command-palette/lib/sortCommands.ts @@ -0,0 +1,308 @@ +import type { CommandCategory, ResolvedCommand } from '@renderer/features/command-palette/types' + +// sortCommands — how the command list is ordered when the user is BROWSING +// rather than searching. +// +// WHY this module exists at all. The palette had exactly one ordering story and +// it was built for the keyboard: type three characters and `rankEntries` does +// the work. Not typing got you catalog registration order, verbatim. That order +// is deliberate — `catalog.ts` declares it a user-visible invariant and +// `catalog.test.ts` pins it — but it is deliberate about AUTHORING ADJACENCY +// ("like things stay adjacent"), which is a different goal from FINDABILITY. A +// mouse user who does not already know the command's name gets no affordance +// from it: no alphabet to scan, no categories to narrow by eye, just ~99 rows. +// +// WHY it is a separate module from `rankCommands`. Two genuinely different +// questions, and keeping them apart is what stops the second from eroding the +// first: +// +// rankCommands / rankEntries — "how well does this row match what you typed" +// sortCommands — "how do you want to scan a list you have not +// typed into" +// +// They never compete, because sorting is only ever consulted when the query is +// empty (see the guard in `rankCommands`). A sort mode that reordered SEARCH +// results would let 'alpha' push a tier-5 prefix match below a tier-1 +// subsequence match — exactly the inversion class the ranking rewrite existed +// to eliminate. That is why this module has no notion of a query at all: it +// cannot violate the rule it does not have the inputs to violate. +// +// Pure on purpose — no React, no storage, no Date.now() — matching its +// neighbours `rankEntries.ts` and `rankCommands.ts`, so the whole ordering story +// stays testable without a DOM. + +export type CommandSortMode = 'catalog' | 'alpha' | 'grouped' | 'recent' + +/** Every valid mode, for persistence coercion and for rendering the picker. + * Declared as a const tuple so the menu, the settings coercion and the type + * all derive from ONE list — a fifth mode added to the union but forgotten + * here would otherwise be silently unreachable in the UI. */ +export const COMMAND_SORT_MODES = ['catalog', 'alpha', 'grouped', 'recent'] as const + +export function isCommandSortMode(value: unknown): value is CommandSortMode { + return typeof value === 'string' && (COMMAND_SORT_MODES as readonly string[]).includes(value) +} + +/** Short label for the control's resting state and its menu rows. Kept beside + * the modes rather than in the component so a new mode cannot ship nameless. */ +export const COMMAND_SORT_MODE_LABELS: Record = { + catalog: 'Catalog order', + alpha: 'A – Z', + grouped: 'Grouped', + recent: 'Recently used', +} + +/** + * Order in which category sections are presented in `grouped` mode. + * + * WHY `CommandCategory` and NOT `CommandSurface` — this was the first + * implementation and it was wrong. `surface` is a MACHINE applicability + * dimension: it answers "does this concept exist in the current layout" and + * drives mode gating. `CommandCategory`'s own doc comment records that reusing + * it as a presentation axis is a conflation the codebase already paid for once + * (the Settings list did it), because once one field means both, a command + * cannot be reclassified for display without changing WHEN IT APPLIES. + * + * It is also simply worse at the job this mode exists for. By surface the + * buckets are app 41 / dispatch 34 / session 32 / grid 11 / debug 11 / + * editor 9 — and since grid and dispatch are mutually exclusive, one section + * holds ~40% of the visible list, which is barely a narrowing at all. By + * category they are session 24 / layout-dispatch 16 / navigate 12 / + * developer 12 / workspace-tools 11 / editor-files 10 / create 10 / + * preferences 3: eight sections a person can actually scan. + * + * WHY this fixed order rather than the union's declaration order: this is a + * browse sequence, running from what people reach for most to what they reach + * for least, ending with developer tooling. The union in `types.ts` is a + * taxonomy, not a ranking, and coupling them would mean a future reordering of + * the type for readability silently reshuffled the UI. + */ +const CATEGORY_ORDER: readonly CommandCategory[] = [ + 'create', + 'navigate', + 'session', + 'layout-dispatch', + 'editor-files', + 'workspace-tools', + 'preferences', + 'developer', +] + +const CATEGORY_LABELS: Record = { + create: 'Create', + navigate: 'Navigate', + session: 'Session', + 'layout-dispatch': 'Layout & Dispatch', + 'editor-files': 'Editor & Files', + 'workspace-tools': 'Workspace Tools', + preferences: 'Preferences', + developer: 'Developer', +} + +/** + * Section for commands that declare no category. + * + * `CommandDef.category` is optional until the governance migration makes it + * required, and extension-contributed commands have no way to declare one at + * all. Dropping those rows would silently hide working commands from a browse + * mode — the worst possible failure for a feature whose entire purpose is + * discovery — so they get a labelled home at the end instead. + */ +const UNCATEGORIZED_LABEL = 'Other' + +/** The starred section's label in `grouped` mode. Starring already hoists rows + * to the top invisibly (see `rankCommands`); grouped mode is the one view that + * can afford to say so out loud. */ +export const STARRED_GROUP_LABEL = '★ Starred' + +export type CommandGroup = { + label: string + commands: ResolvedCommand[] +} + +/** + * The browse-state ordering, plus the section headers that go with it. + * + * ONE function returning BOTH because they must never disagree. An earlier + * draft had the component call a sort function and a separate grouping + * function; that is two derivations of the same order, and the failure mode is + * silent and ugly — a header rendered above the wrong row, with the flat array + * the selection model indexes still believing something else. Deriving the flat + * list BY FLATTENING the groups makes the two structurally incapable of + * drifting. + * + * `headers` is keyed by position in the returned `commands` array — the same + * array the caller renders and `selectedIndex` indexes — and is empty for every + * mode except `grouped`. + */ +export type BrowseOrder = { + commands: ResolvedCommand[] + /** Readonly so the shared `EMPTY_HEADERS` singleton below can be handed out + * to every non-grouped caller without any of them being able to mutate it + * for all the others. */ + headers: ReadonlyMap +} + +export function browseOrder( + commands: readonly ResolvedCommand[], + mode: CommandSortMode, + historyScore: Map, + starred: Record, +): BrowseOrder { + if (mode === 'grouped') { + const groups = groupCommands(commands, starred) + const flat: ResolvedCommand[] = [] + const headers = new Map() + for (const group of groups) { + // The header's index is wherever this group's first row lands in the + // flattened list, which is simply the length so far. Computing it from a + // running offset rather than an `indexOf` lookup keeps this exact even if + // two groups were ever to contain the same command object. + headers.set(flat.length, group.label) + flat.push(...group.commands) + } + return { commands: flat, headers } + } + + // Non-grouped modes: stars stay hard-partitioned to the top (the behavior + // `rankCommands` has always had — see its comment block, which defends + // starring as the ONE thing allowed to perturb the resting order), and the + // chosen sort applies WITHIN each half. + // + // WHY sorting does not simply override the star partition: starring answers + // "which commands are mine" and sorting answers "how do I want to scan the + // list". They are orthogonal, and letting a sort mode dissolve the user's + // explicit pins would mean choosing A–Z silently unpinned everything. + const starredRows = commands.filter(command => starred[command.id]) + if (starredRows.length === 0) { + return { commands: orderFlat(commands, mode, historyScore), headers: EMPTY_HEADERS } + } + const rest = commands.filter(command => !starred[command.id]) + return { + commands: [ + ...orderFlat(starredRows, mode, historyScore), + ...orderFlat(rest, mode, historyScore), + ], + headers: EMPTY_HEADERS, + } +} + +/** + * The "this list has no sections" value. + * + * One shared instance rather than a fresh `new Map()` per call, for two + * reasons: the result feeds a `useMemo` the palette re-reads on every render, + * so a stable reference keeps downstream comparisons honest; and typing it + * `ReadonlyMap` means every non-grouped caller can safely be handed the SAME + * object without one of them being able to mutate it for all the others. + * + * Exported because `rankCommands` needs the same value for its search-result + * path — search results are never sectioned either, and two separately-declared + * empty maps for one concept is exactly the kind of near-duplicate that drifts. + */ +export const EMPTY_HEADERS: ReadonlyMap = new Map() + +/** + * Compare by title, with the caller's array index as the deterministic + * tiebreak. + * + * The index fallback matters more than it looks: two commands can share a title + * (a per-provider generated pair, or a state-dependent `title(ctx)` that + * resolved to the same string), and without it `Array.sort` falls through to + * implementation-defined behavior for equal elements — meaning the list could + * reorder itself between renders with no input change. `rankEntries` guards the + * same hazard the same way, deliberately. + * + * `localeCompare` rather than `<`: titles are user-facing prose containing + * digits ("Focus Pane 2", "Focus Pane 10"), and `numeric` keeps 2 before 10. + */ +function byTitleThenIndex( + a: { command: ResolvedCommand; index: number }, + b: { command: ResolvedCommand; index: number }, +): number { + const byTitle = a.command.title.localeCompare(b.command.title, undefined, { + numeric: true, + sensitivity: 'base', + }) + return byTitle !== 0 ? byTitle : a.index - b.index +} + +/** Flat ordering for the non-grouped modes. Split out so the star partition + * above can apply the same rule to each half without duplicating it. */ +function orderFlat( + commands: readonly ResolvedCommand[], + mode: CommandSortMode, + historyScore: Map, +): ResolvedCommand[] { + if (mode === 'catalog') return [...commands] + + const decorated = commands.map((command, index) => ({ command, index })) + + if (mode === 'alpha') { + decorated.sort(byTitleThenIndex) + return decorated.map(entry => entry.command) + } + + // `recent`: history score DESC, then catalog order for everything the user + // has never run. The unscored tail keeping catalog order (rather than falling + // to alphabetical) is deliberate — the point of this mode is "float what I + // use", not "reorganize everything", so the part of the list the mode has no + // opinion about should look exactly like it always did. + decorated.sort((a, b) => { + const scoreA = historyScore.get(a.command.id) ?? 0 + const scoreB = historyScore.get(b.command.id) ?? 0 + if (scoreA !== scoreB) return scoreB - scoreA + return a.index - b.index + }) + return decorated.map(entry => entry.command) +} + +/** + * Split a flat command list into rendered sections. + * + * Exported for its own tests; the palette consumes it through `browseOrder`. + * + * Empty groups are dropped rather than rendered as bare headers — a section + * heading over nothing reads as a bug, and several categories are empty in any + * given context once mode gating and picker visibility have filtered the list. + */ +export function groupCommands( + commands: readonly ResolvedCommand[], + starred: Record, +): CommandGroup[] { + const groups: CommandGroup[] = [] + + // Starred first, and NOT re-sorted alphabetically: a starred row's position + // is the one thing the user has expressed a direct opinion about, so the + // section preserves the order it arrived in. + const starredCommands = commands.filter(command => starred[command.id]) + if (starredCommands.length > 0) { + groups.push({ label: STARRED_GROUP_LABEL, commands: starredCommands }) + } + + const rest = commands + .filter(command => !starred[command.id]) + .map((command, index) => ({ command, index })) + + const sectionFor = (entries: typeof rest, label: string): void => { + if (entries.length === 0) return + // Alphabetical WITHIN a section. Once the headings carry the structure, + // catalog adjacency inside a section stops earning its keep — the user is + // scanning a short labelled list at that point, and A–Z is the fastest + // thing to scan. + const sorted = [...entries].sort(byTitleThenIndex) + groups.push({ label, commands: sorted.map(entry => entry.command) }) + } + + for (const category of CATEGORY_ORDER) { + sectionFor(rest.filter(entry => entry.command.category === category), CATEGORY_LABELS[category]) + } + + // Last, so a categorized command never sorts below an uncategorized one. + // Empty in practice today, and it stays empty as long as every command + // declares a category — but see UNCATEGORIZED_LABEL for why this cannot be a + // silent drop. + sectionFor(rest.filter(entry => entry.command.category === undefined), UNCATEGORIZED_LABEL) + + return groups +} diff --git a/src/renderer/src/features/command-palette/registry.ts b/src/renderer/src/features/command-palette/registry.ts index bcc5708b..a1d88aa7 100644 --- a/src/renderer/src/features/command-palette/registry.ts +++ b/src/renderer/src/features/command-palette/registry.ts @@ -172,6 +172,14 @@ export function buildCommandRegistry(ctx: CommandContext): ResolvedCommand[] { title: typeof command.title === 'function' ? command.title(ctx) : command.title, description, surface: command.surface, + // Carried through so the palette's `grouped` sort mode can section the + // list by the taxonomy that was DESIGNED for user-facing grouping. + // Grouping by `surface` instead is the exact conflation `CommandCategory` + // was introduced to undo (see its doc comment): `surface` answers "does + // this concept exist in the current layout", which is a machine + // applicability question, and reusing it as a presentation axis means a + // command cannot be reclassified without changing when it applies. + category: command.category, // The FIRST effective binding, in display form. A command may have // several (Close Pane has Cmd+W and Alt+W); the row shows one, and the // first is the primary by declaration order. Undefined when the command diff --git a/src/renderer/src/features/command-palette/types.ts b/src/renderer/src/features/command-palette/types.ts index 9731b05f..3210d23b 100644 --- a/src/renderer/src/features/command-palette/types.ts +++ b/src/renderer/src/features/command-palette/types.ts @@ -484,8 +484,16 @@ export type ResolvedCommand = { title: string description: string /** Carried through from CommandDef so palette/menu consumers can - * group or label by surface without re-importing the raw defs. */ + * label by surface without re-importing the raw defs. + * + * NOT the field to group a user-facing list by — use `category` below. + * See `CommandCategory`'s doc for why the two must not be conflated. */ surface: CommandSurface + /** User-facing grouping, carried through for the palette's `grouped` sort + * mode. Still optional here because it is optional on `CommandDef` until the + * governance migration makes it required; consumers must handle its absence + * rather than assume total coverage. */ + category?: CommandCategory /** * The chord this command will actually run, in display form, or undefined * when it has none. diff --git a/src/renderer/src/features/command-palette/ui/CommandPalette.tsx b/src/renderer/src/features/command-palette/ui/CommandPalette.tsx index 34aa2498..1ca871e4 100644 --- a/src/renderer/src/features/command-palette/ui/CommandPalette.tsx +++ b/src/renderer/src/features/command-palette/ui/CommandPalette.tsx @@ -1,7 +1,7 @@ import { DEFAULT_PROVIDER, isAgentProviderKind } from '@shared/types/providerKind' import type { AgentProviderKind } from '@shared/types/providerKind' import { getProviderFeatures } from '@providers/shared/featureCapabilities' -import { memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react' +import { Fragment, memo, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react' import ReactMarkdown from 'react-markdown' import { @@ -25,6 +25,8 @@ import { } from '@renderer/features/command-palette/lib/recentCommandHistory' import { useGlobalToast } from '@renderer/ui/GlobalToast' import { rankCommands } from '@renderer/features/command-palette/lib/rankCommands' +import { CommandSortControl } from '@renderer/features/command-palette/ui/CommandSortControl' +import type { CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' import { body, primary, @@ -906,9 +908,37 @@ function OpenCommandPalette({ }, [commandStarred, setSettings], ) - const filteredCommands = useMemo( - () => rankCommands(commands, queryText, historyScoreMap, commandStarred), - [commands, queryText, historyScoreMap, commandStarred], + // The sort mode is a persisted browse preference, read the same way starring + // is — straight from settings, deliberately NOT through `commandContext.flags`. + // Same reasoning as the comment above: flags feed the context memo, and + // putting a value there that changes on a menu click would rebuild all 99 + // commands (every function title, every getState, resolveEffectiveKeybindings) + // to reorder a list that is already built. + const commandSortMode = settings.commandSortMode + const setCommandSortMode = useCallback( + (next: CommandSortMode) => { + setSettings({ commandSortMode: next }) + // Load-bearing, and for exactly the reason spelled out on the `setQuery` + // handler below: a reordering that leaves the LENGTH unchanged is + // invisible to both guards. The clamp effect keys on `filteredLength`, + // which does not move; the scroll effect keys on `selectedIndex`, which + // does not move either. So without this the highlight stays on row N + // while row N becomes a completely different command, and Enter runs + // something the user never looked at — and the catalog contains + // destructive commands. + // + // Every `enter*Mode` callback resets the index for the same reason. The + // sort control was the one reordering path that did not. + setSelectedIndex(0) + }, + [setSettings], + ) + // `headers` is the section map for `grouped` mode and empty for every other + // mode. It comes back from the SAME call that produced the ordering, so a + // header can never be drawn above the wrong row — see `browseOrder`. + const { commands: filteredCommands, headers: commandGroupHeaders } = useMemo( + () => rankCommands(commands, queryText, historyScoreMap, commandStarred, commandSortMode), + [commands, queryText, historyScoreMap, commandStarred, commandSortMode], ) const directAgentTarget = useMemo( () => resolveAgentPaneLabel(workspace.state, queryText, workspace.tileTabs), @@ -929,6 +959,19 @@ function OpenCommandPalette({ [directAgentCommand, filteredCommands], ) + // `commandGroupHeaders` is keyed by index into `filteredCommands`, but the + // rendered list is `paletteCommands` — one longer whenever a direct agent + // coordinate row is prepended. Shifting the lookup by that offset keeps the + // two aligned. + // + // In practice the two are mutually exclusive: headers exist only for an EMPTY + // query, and `resolveAgentPaneLabel` needs a query matching /^[A-Z]+[1-9]\d*$/ + // to produce a row at all. The offset is here anyway because relying on that + // coincidence would put a silent off-by-one behind any future change to + // either rule, and the failure mode — every section heading sitting one row + // too high — is exactly the kind of thing that ships unnoticed. + const directAgentRowOffset = directAgentCommand ? 1 : 0 + /** * Index of the LAST starred row, so it can carry a rule separating the * pinned block from everything else. -1 when no separator should render. @@ -941,9 +984,16 @@ function OpenCommandPalette({ * * Also -1 when every row is starred or none is, since a separator at the * very top or very bottom of the list divides nothing. + * + * ALSO -1 in `grouped` sort mode, added when sort modes landed: grouped mode + * already renders a labelled "★ Starred" section, so the rule would draw a + * second, unlabelled divider immediately under a heading that says the same + * thing. Headers own the structure in that mode; this separator is the + * fallback for the modes that have none. */ const starredBoundaryIndex = useMemo(() => { if (queryText.length > 0) return -1 + if (commandSortMode === 'grouped') return -1 let starredCount = 0 for (const command of paletteCommands) { if (!commandStarred[command.id]) break @@ -951,7 +1001,7 @@ function OpenCommandPalette({ } if (starredCount === 0 || starredCount === paletteCommands.length) return -1 return starredCount - 1 - }, [commandStarred, paletteCommands, queryText]) + }, [commandSortMode, commandStarred, paletteCommands, queryText]) const filteredLength = mode === 'resume' @@ -1005,11 +1055,27 @@ function OpenCommandPalette({ useEffect(() => { if (!listRef.current) return - const el = listRef.current.children[selectedIndex] + // Resolve the row by its declared index, NOT by position among the + // container's children. + // + // `children[selectedIndex]` assumed every child of the list is a selectable + // row, and that was already false before grouping existed: the + // `ai-workspace-open`/`clear` modes render an error banner as a sibling of + // the rows, so while an error was showing every scroll target was off by + // one. Grouped mode's section headings would have made it wrong in a fourth + // mode. An explicit `data-palette-row` makes a row's index part of its + // identity, so sibling chrome — banners, headings, anything added later — + // can never shift it again. + const el = listRef.current.querySelector(`[data-palette-row="${selectedIndex}"]`) if (el instanceof HTMLElement) { el.scrollIntoView({ block: 'nearest' }) } - }, [selectedIndex]) + // `paletteCommands` is a dependency, not just `selectedIndex`: switching sort + // mode moves the selected row to a different scroll offset (grouped mode + // inserts headers, which shifts everything below them) while the index may + // be unchanged. Keyed on the index alone, the effect would not re-run and + // the highlighted row could sit off-screen with nothing visibly selected. + }, [selectedIndex, paletteCommands]) const executeCommand = useCallback( (command: ResolvedCommand) => { @@ -1637,6 +1703,18 @@ function OpenCommandPalette({ Manage )} + {/* Commands mode only. The other ten modes render short, intrinsically + ordered lists (session recency, buried-at time, [...custom, + ...builtin]) where a sort control would be chrome without a + purpose — the command list is the only one long enough to be hard + to scan. */} + {mode === 'commands' && ( + 0} + /> + )}
@@ -1735,56 +1813,85 @@ function OpenCommandPalette({ No matching commands
) : ( - paletteCommands.map((command, i) => ( -
setSelectedIndex(i)} - onClick={() => executeCommand(command)} - > -
- {/* Marks starred rows in the list itself. Without it the - pinned block at the top looked like an arbitrary - reordering — the star lived only in the detail pane, - so identifying which commands were pinned meant - selecting them one at a time. Fixed-width so titles - stay left-aligned whether or not a row is starred. */} - { + const groupHeader = commandGroupHeaders.get(i - directAgentRowOffset) + return ( + + {groupHeader && ( +
+ {groupHeader} +
+ )} +
setSelectedIndex(i)} + onClick={() => executeCommand(command)} > - ★ - - {/* The glyph above is aria-hidden because announcing - "star" on all 102 rows is noise. But starred state was - then conveyed only visually, so a screen-reader user - got a list silently reordered for a reason they could - not perceive. This says it once, only where it is - true. */} - {commandStarred[command.id] ? Starred. : null} - {command.title} - {command.state && } -
- {command.shortcut && ( - - {command.shortcut} - - )} -
- )) +
+ {/* Marks starred rows in the list itself. Without it the + pinned block at the top looked like an arbitrary + reordering — the star lived only in the detail pane, + so identifying which commands were pinned meant + selecting them one at a time. Fixed-width so titles + stay left-aligned whether or not a row is starred. */} + + ★ + + {/* The glyph above is aria-hidden because announcing + "star" on all 102 rows is noise. But starred state was + then conveyed only visually, so a screen-reader user + got a list silently reordered for a reason they could + not perceive. This says it once, only where it is + true. */} + {commandStarred[command.id] ? Starred. : null} + {command.title} + {command.state && } +
+ {command.shortcut && ( + + {command.shortcut} + + )} +
+ + ) + }) ))} {mode === 'resume' && @@ -1810,6 +1917,7 @@ function OpenCommandPalette({ : 'text-ink-dim hover:bg-row-hover-bg' } `} + data-palette-row={i} onMouseEnter={() => setSelectedIndex(i)} onClick={() => executeResume(session)} > @@ -1850,6 +1958,7 @@ function OpenCommandPalette({ + + {open && !searching && ( +
+ {COMMAND_SORT_MODES.map((candidate, index) => ( + + ))} +
+ )} + + ) +}