From 02da93e2668cdc05d1d3d81e0bfd3d5f9edcb494 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:26:17 +0200 Subject: [PATCH 1/4] docs: plan command palette sort modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The palette has one ordering story and it is built for the keyboard: type three characters and rankEntries does the work. Not typing gets you catalog registration order, which is deliberate about authoring adjacency ("like things stay adjacent") rather than findability. A mouse user who does not already know a command's name gets ~99 flat rows and no affordance. Records the design: one control, four modes, empty-query only. Also records what was deliberately left out and why — no filter chips (grouping subsumes them), no sort for the other four palette lists, no chord. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-07-28-command-palette-sort-modes.md | 304 ++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md 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..f3cdf283 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md @@ -0,0 +1,304 @@ +# 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 `surface`, 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. `surface` is already mandatory +on every `CommandDef` (`docs/command-style.md` rule 10) and is already carried +through to `ResolvedCommand` *specifically* so consumers can group by it — +`types.ts` says so in as many words. The data model has been waiting for this UI. + +### 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 ║ +║ ── APP ──────────────────────── ║ +║ New Tab ⌘T ║ +║ Open Settings ⌘, ║ +║ ── SESSION ──────────────────── ║ +║ Reload Agent ║ +║ Rewind to Prompt… ║ +``` + +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' + +sortCommands(commands, mode, historyScore): ResolvedCommand[] +groupCommands(commands, starred): CommandGroup[] // grouped mode only +``` + +`groupCommands` returns `{ label, commands }[]`, so the component renders +headers without knowing the surface taxonomy. Group order is fixed and +declared in the module: + +``` +★ STARRED · APP · SESSION · GRID · DISPATCH · EDITOR · DEBUG +``` + +`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. +- Escape closes the menu and **must not** close the Dialog — `stopPropagation` + on the menu's key handler, since the palette's own `onEscapeKeyDown` ladder + owns Escape at the Dialog level. +- Click-outside closes, via a `pointerdown` listener on `document` while open. + +--- + +## 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/CommandPalette.tsx` | wire control, render group headers, `data-palette-row`, scroll fix | +| `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. + +**`rankCommands.test.ts`** +- **The invariant:** a non-empty query ignores sort mode entirely — a tier-5 + prefix match stays first under every one of the four modes. +- Starred commands stay partitioned above unstarred under every mode. +- Sorting applies within both partitions, not across them. + +No renderer test for the popover. The behavior worth pinning (ordering) is pure +and covered above; a happy-dom test asserting that a menu opens on click would +pin the implementation, not the contract. + +--- + +## 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. From 2bc3fe1865b1b9cfc98814265e65ab754718c35e Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:26:27 +0200 Subject: [PATCH 2/4] feat(command-palette): browse-order sort modes Four ways to order the command list while the search box is empty: catalog (today's behavior, still the default), A-Z, grouped by surface, and recently used. Persisted as Settings.commandSortMode. The load-bearing constraint is that none of this may touch a SEARCH result. Applying a sort on top of relevance would let A-Z push a tier-5 prefix match below a tier-1 subsequence match, which is the exact inversion class rankEntries was extracted to eliminate. rankCommands enforces it with an early return above the sort, and sortCommands has no notion of a query at all -- it cannot violate a rule it lacks the inputs to violate. browseOrder returns the ordering AND its section headers from one call. An earlier draft had the component call a sort function and a separate grouping function; that is two derivations of one order, and the failure is silent -- a header rendered above the wrong row while the flat array the selection model indexes believes something else. Flattening the groups to produce the list makes them structurally incapable of drifting. Starring composes rather than competes: stars stay hard-partitioned to the top and the chosen sort applies within each half. Letting a sort dissolve the user's explicit pins would mean choosing A-Z silently unpinned everything. In grouped mode that invisible split becomes a leading "Starred" section. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/app-state/settings/persistence.ts | 18 ++ src/renderer/src/app-state/settings/types.ts | 22 ++ .../command-palette/lib/rankCommands.test.ts | 90 ++++++ .../command-palette/lib/rankCommands.ts | 47 ++- .../command-palette/lib/sortCommands.test.ts | 224 ++++++++++++++ .../command-palette/lib/sortCommands.ts | 277 ++++++++++++++++++ 6 files changed, 667 insertions(+), 11 deletions(-) create mode 100644 src/renderer/src/features/command-palette/lib/rankCommands.test.ts create mode 100644 src/renderer/src/features/command-palette/lib/sortCommands.test.ts create mode 100644 src/renderer/src/features/command-palette/lib/sortCommands.ts diff --git a/src/renderer/src/app-state/settings/persistence.ts b/src/renderer/src/app-state/settings/persistence.ts index aeb54480..e197cee3 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,21 @@ 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 — rather than reaching `browseOrder`, where it + * would fall through every branch and silently produce the catalog order + * anyway, just with a control claiming a mode that is not in effect. + */ +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..05893e28 --- /dev/null +++ b/src/renderer/src/features/command-palette/lib/sortCommands.test.ts @@ -0,0 +1,224 @@ +import { describe, expect, it } from 'vitest' + +import { + browseOrder, + groupCommands, + isCommandSortMode, + STARRED_GROUP_LABEL, +} from '@renderer/features/command-palette/lib/sortCommands' +import type { CommandSurface, 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. +function cmd(id: string, title: string, surface: CommandSurface = 'app'): ResolvedCommand { + return { + id, + title, + description: `${title} description`, + surface, + 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('app-b', 'Open Settings', 'app'), + cmd('sess-a', 'Copy Last Response', 'session'), + cmd('app-a', 'New Tab', 'app'), + cmd('dbg', 'Save Debug Logs', 'debug'), + ] + + 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', + 'Open Settings', + 'Copy Last Response', + 'Reload Agent', + 'Save Debug Logs', + ]) + expect([...result.headers.entries()]).toEqual([ + [0, 'App'], + [2, 'Session'], + [4, 'Debug'], + ]) + }) + + it('drops empty groups instead of rendering a heading over nothing', () => { + const result = browseOrder([cmd('only', 'Only One', 'editor')], 'grouped', NO_HISTORY, NOTHING_STARRED) + + expect([...result.headers.values()]).toEqual(['Editor']) + }) + + it('leads with a starred section and pulls those rows out of their surfaces', () => { + 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 now has one member, so it is still present but no longer holds + // the starred row. + expect(titles(result.commands)).toEqual([ + 'Reload Agent', + 'New Tab', + 'Open Settings', + 'Copy Last Response', + '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, and that command's group + // label matches the header. + const result = browseOrder(commands, 'grouped', NO_HISTORY, { 'app-a': true }) + + for (const [index] of result.headers) { + expect(result.commands[index]).toBeDefined() + } + expect(result.headers.size).toBe(4) // Starred + App + Session + Debug + }) +}) + +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('never emits both grid and dispatch sections populated in one build', () => { + // Not a rule this module enforces — `surfaceAvailable` in the registry + // guarantees it upstream — but the group order lists both, so this pins + // that each is handled independently and neither swallows the other. + const groups = groupCommands([cmd('g', 'Split Pane Right', 'grid')], NOTHING_STARRED) + expect(groups.map(group => group.label)).toEqual(['Grid']) + }) +}) 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..d789da8f --- /dev/null +++ b/src/renderer/src/features/command-palette/lib/sortCommands.ts @@ -0,0 +1,277 @@ +import type { CommandSurface, 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 surface groups are presented in `grouped` mode. + * + * WHY this fixed order and not the `CommandSurface` union's declaration order: + * this is a browse sequence, so it runs most-general to most-specialized — + * `app` (always meaningful) first, `debug` (developer tooling) last. The union + * in `types.ts` is a taxonomy, not a ranking, and coupling the two would mean a + * future reordering of the type for readability silently reshuffled the UI. + * + * `grid` and `dispatch` are mutually exclusive at runtime — `surfaceAvailable` + * in `registry.ts` hides one or the other depending on Dispatch Mode — so at + * most one of those two sections can ever render. Both are listed because this + * module has no way to know which mode is active, and must not care. + */ +const SURFACE_ORDER: readonly CommandSurface[] = [ + 'app', + 'session', + 'grid', + 'dispatch', + 'editor', + 'debug', +] + +const SURFACE_LABELS: Record = { + app: 'App', + session: 'Session', + grid: 'Grid', + dispatch: 'Dispatch', + editor: 'Editor', + debug: 'Debug', +} + +/** 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 in practice at least two of the six + * surfaces are empty in any given mode (`grid` and `dispatch` can never both be + * populated). + */ +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 })) + + for (const surface of SURFACE_ORDER) { + const inSurface = rest.filter(entry => entry.command.surface === surface) + if (inSurface.length === 0) continue + // Alphabetical WITHIN a group. Once the section 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. + inSurface.sort(byTitleThenIndex) + groups.push({ + label: SURFACE_LABELS[surface], + commands: inSurface.map(entry => entry.command), + }) + } + + return groups +} From 27c25134479c5e180a4dc33aed8a1aaaf53882df Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:26:38 +0200 Subject: [PATCH 3/4] feat(command-palette): sort control, section headers, and a scroll fix The picker sits in the header slot Manage already uses, in commands mode only. While a query is present it shows "Relevance" and disables itself -- the sort genuinely is inert then, and saying so beats letting the user wonder why their setting stopped working. Focus discipline is the whole difficulty in the control: the search input must keep DOM focus throughout, because onKeyDown (arrows, Enter, the mode ladder) is bound to it and typing right after picking a sort has to work. preventDefault on mousedown -- rather than a focus() call afterwards -- means focus never moves in the first place, so there is no restore to sequence against React's commit. Escape closes the menu and stops propagating, or the Dialog's back-out ladder would skip a rung and drop the user's sub-mode. Section headers render inside the existing flat list via an index->label map rather than restructuring it into sections. selectedIndex, arrow navigation, hover, Enter and the clamp effect all keep indexing one flat array, so a keyboard handler whose comments already record three fixed bugs is untouched. Fixes a latent bug on the way through: the scroll-into-view effect resolved rows as listRef.children[selectedIndex], which assumed every child of the list is a selectable row. That was ALREADY false -- the ai-workspace modes render an error banner as a sibling, so every scroll target was off by one while an error showed. Rows now carry data-palette-row and are resolved by attribute, which sibling chrome cannot shift. Co-Authored-By: Claude Opus 5 (1M context) --- .../command-palette/ui/CommandPalette.tsx | 145 ++++++++++---- .../command-palette/ui/CommandSortControl.tsx | 177 ++++++++++++++++++ 2 files changed, 290 insertions(+), 32 deletions(-) create mode 100644 src/renderer/src/features/command-palette/ui/CommandSortControl.tsx diff --git a/src/renderer/src/features/command-palette/ui/CommandPalette.tsx b/src/renderer/src/features/command-palette/ui/CommandPalette.tsx index 6225ec57..0202ce75 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,23 @@ 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 }), + [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 +945,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 + const filteredLength = mode === 'resume' ? filteredSessions.length @@ -981,7 +1010,18 @@ 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' }) } @@ -1613,6 +1653,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} + /> + )}
@@ -1711,34 +1763,58 @@ function OpenCommandPalette({ No matching commands
) : ( - paletteCommands.map((command, i) => ( -
setSelectedIndex(i)} - onClick={() => executeCommand(command)} - > -
- {command.title} - {command.state && } -
- {command.shortcut && ( - - {command.shortcut} - + paletteCommands.map((command, i) => { + const groupHeader = commandGroupHeaders.get(i - directAgentRowOffset) + return ( + + {groupHeader && ( +
+ {groupHeader} +
)} -
- )) +
setSelectedIndex(i)} + onClick={() => executeCommand(command)} + > +
+ {command.title} + {command.state && } +
+ {command.shortcut && ( + + {command.shortcut} + + )} +
+ + ) + }) ))} {mode === 'resume' && @@ -1764,6 +1840,7 @@ function OpenCommandPalette({ : 'text-ink-dim hover:bg-row-hover-bg' } `} + data-palette-row={i} onMouseEnter={() => setSelectedIndex(i)} onClick={() => executeResume(session)} > @@ -1804,6 +1881,7 @@ function OpenCommandPalette({ + + {open && !searching && ( +
+ {COMMAND_SORT_MODES.map(candidate => ( + + ))} +
+ )} + + ) +} From 477571e6e5a72587cc0d482545234f546cbab747 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:59:30 +0200 Subject: [PATCH 4/4] fix(command-palette): resolve two-reviewer review of the sort modes A Claude and a Codex reviewer went at this independently; both returned FIX-FIRST and both landed on the same seam. Every finding below was verified against the code before acting on it. HIGH -- changing the sort mode left selectedIndex on a different command. Reordering at UNCHANGED length is invisible to both guards: the clamp effect keys on filteredLength, the scroll effect on selectedIndex, and neither moves. So the highlight stayed on row N while row N became something else, and Enter ran a command the user never looked at -- from a catalog containing destructive entries. This is the exact hazard already documented on setQuery, which every enter*Mode callback respects; the sort control was the one reordering path that did not. MEDIUM -- three keys went to the wrong widget. The control's React onKeyDown was unreachable dead code: keepFocusInSearchInput guarantees focus never enters its subtree, and the search input is a SIBLING, not a descendant. So Escape reached Radix and closed the whole palette, while arrows and Enter drove the command list hidden behind the open menu. Replaced with one capture-phase document listener, which beats both competitors (React 18 delegates to the root container; Radix's dismiss layer bubbles) and makes the menu genuinely keyboard-operable, as its ARIA roles always claimed. DESIGN -- grouping keyed on `surface`, which is the conflation CommandCategory was introduced to undo: surface is a machine applicability dimension driving mode gating, and reusing it for presentation means a command cannot be reclassified without changing when it applies. It was also worse at the job -- one section held ~40% of the list. Now groups by category (8 balanced buckets), with a trailing "Other" so uncategorized and extension-contributed commands are never silently dropped from the mode built for discovery. Also: scroll effect now depends on the rendered order, not just the index; section headings are no longer aria-hidden (grouped mode's entire value is the structure it was hiding from assistive tech); the starred divider from #619 is suppressed in grouped mode, where a labelled heading already says it; and the coerceCommandSortMode comment no longer misstates the un-coerced fallback -- it would render as "recent", not catalog. Adds CommandSortControl.renderer.test.tsx, which the plan had declined. That call was right about opening and wrong about keys, and this is the file that would have caught it. Merges origin/main (5 commits, including #619's star column) and resolves the row-render conflict, keeping both the star affordances and grouped headers. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-07-28-command-palette-sort-modes.md | 101 +++++++++++--- .../ui/CommandSortControl.renderer.test.tsx | 126 ++++++++++++++++++ 2 files changed, 207 insertions(+), 20 deletions(-) create mode 100644 src/renderer/src/features/command-palette/ui/CommandSortControl.renderer.test.tsx 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 index f3cdf283..4e4d09e3 100644 --- a/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md +++ b/docs/superpowers/plans/2026-07-28-command-palette-sort-modes.md @@ -49,13 +49,30 @@ to be insufficient, filtering is a separate, later change. |---|---|---| | `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 `surface`, section headers, alphabetical within group | **browsing / discovery** — the actual reported pain | +| `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. `surface` is already mandatory -on every `CommandDef` (`docs/command-style.md` rule 10) and is already carried -through to `ResolvedCommand` *specifically* so consumers can group by it — -`types.ts` says so in as many words. The data model has been waiting for this UI. +`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 @@ -132,15 +149,20 @@ With a query present: ``` ║ ── ★ STARRED ────────────────── ║ -║ Reader Mode ⟨ON⟩ ⌘⇧R ║ -║ ── APP ──────────────────────── ║ +║ ★ Reader Mode ⟨ON⟩ ⌘⇧R ║ +║ ── CREATE ───────────────────── ║ +║ New Agent… ⌘N ║ ║ New Tab ⌘T ║ -║ Open Settings ⌘, ║ ║ ── 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. @@ -156,16 +178,17 @@ Pure, no React, no storage, no `Date.now()` — same contract as its neighbours ```ts export type CommandSortMode = 'catalog' | 'alpha' | 'grouped' | 'recent' -sortCommands(commands, mode, historyScore): ResolvedCommand[] +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 surface taxonomy. Group order is fixed and +headers without knowing the category taxonomy. Group order is fixed and declared in the module: ``` -★ STARRED · APP · SESSION · GRID · DISPATCH · EDITOR · DEBUG +★ Starred · Create · Navigate · Session · Layout & Dispatch · +Editor & Files · Workspace Tools · Preferences · Developer · Other ``` `grid` and `dispatch` are mutually exclusive at runtime (`surfaceAvailable` in @@ -242,10 +265,32 @@ 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. -- Escape closes the menu and **must not** close the Dialog — `stopPropagation` - on the menu's key handler, since the palette's own `onEscapeKeyDown` ladder - owns Escape at the Dialog level. - 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. --- @@ -258,7 +303,10 @@ Focus discipline, which is the whole difficulty: | `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` | @@ -275,17 +323,30 @@ tests are pure: - `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` 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 — a tier-5 - prefix match stays first under every one of the four modes. +- **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. -No renderer test for the popover. The behavior worth pinning (ordering) is pure -and covered above; a happy-dom test asserting that a menu opens on click would -pin the implementation, not the contract. +**`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. --- diff --git a/src/renderer/src/features/command-palette/ui/CommandSortControl.renderer.test.tsx b/src/renderer/src/features/command-palette/ui/CommandSortControl.renderer.test.tsx new file mode 100644 index 00000000..27b200b1 --- /dev/null +++ b/src/renderer/src/features/command-palette/ui/CommandSortControl.renderer.test.tsx @@ -0,0 +1,126 @@ +import { fireEvent, render, screen } from '@testing-library/react' +import { useRef } from 'react' +import { describe, expect, it, vi } from 'vitest' + +import { CommandSortControl } from '@renderer/features/command-palette/ui/CommandSortControl' +import type { CommandSortMode } from '@renderer/features/command-palette/lib/sortCommands' + +// WHY this file exists, against the plan's original judgement. +// +// The plan declined a renderer test here, reasoning that "a happy-dom test +// asserting that a menu opens on click would pin the implementation, not the +// contract." That was right about *opening* and wrong about *keys*, and review +// found the gap: the control's key handling was a React `onKeyDown` on its own +// root, which can never fire, because focus is deliberately kept in the palette's +// search input — a SIBLING of the control, not a descendant. +// +// The consequences were real contract violations, not implementation details: +// Escape closed the entire palette, and ↑/↓/Enter drove the command list hidden +// behind the open menu. So these tests reproduce the ACTUAL DOM relationship +// (input beside control, focus in the input) and assert on behavior a user can +// observe. They would all have failed against the first implementation. + +/** The real header shape: the search input and the control are siblings, and an + * outer container stands in for the Dialog that would receive a leaked key. */ +function Harness({ + mode = 'catalog', + onChange = () => {}, + searching = false, + onOuterKeyDown, +}: { + mode?: CommandSortMode + onChange?: (next: CommandSortMode) => void + searching?: boolean + onOuterKeyDown?: (event: React.KeyboardEvent) => void +}) { + const inputRef = useRef(null) + return ( +
+ + +
+ ) +} + +const openMenu = () => fireEvent.click(screen.getByRole('button', { name: /sort commands/i })) + +describe('CommandSortControl keyboard contract', () => { + it('closes on Escape typed in the search input, without letting it reach the dialog', () => { + // The original bug: this key never reached the control, fell through to + // Radix's document dismiss handler, and closed the whole palette while the + // menu stayed mounted. + const onOuterKeyDown = vi.fn() + render() + openMenu() + expect(screen.getByRole('menu')).toBeTruthy() + + fireEvent.keyDown(screen.getByTestId('search'), { key: 'Escape' }) + + expect(screen.queryByRole('menu')).toBeNull() + expect(onOuterKeyDown).not.toHaveBeenCalled() + }) + + it('swallows ArrowDown/ArrowUp so they cannot drive the list behind the menu', () => { + const onOuterKeyDown = vi.fn() + render() + openMenu() + + fireEvent.keyDown(screen.getByTestId('search'), { key: 'ArrowDown' }) + fireEvent.keyDown(screen.getByTestId('search'), { key: 'ArrowUp' }) + + expect(onOuterKeyDown).not.toHaveBeenCalled() + // Still open — arrows navigate the menu rather than dismissing it. + expect(screen.getByRole('menu')).toBeTruthy() + }) + + it('commits the arrowed-to mode on Enter instead of running a palette command', () => { + const onChange = vi.fn() + const onOuterKeyDown = vi.fn() + render() + openMenu() + + // Opens on the active mode ('catalog', index 0); one step down is 'alpha'. + fireEvent.keyDown(screen.getByTestId('search'), { key: 'ArrowDown' }) + fireEvent.keyDown(screen.getByTestId('search'), { key: 'Enter' }) + + expect(onChange).toHaveBeenCalledWith('alpha') + expect(onOuterKeyDown).not.toHaveBeenCalled() + expect(screen.queryByRole('menu')).toBeNull() + }) + + it('leaves keys alone once closed, so the palette keeps working normally', () => { + // The mirror of the tests above, and the one that would catch an + // over-aggressive fix: the capture listener must be torn down on close, or + // the palette's own arrows and Enter would be dead for the rest of the + // session. + const onOuterKeyDown = vi.fn() + render() + openMenu() + fireEvent.keyDown(screen.getByTestId('search'), { key: 'Escape' }) + + fireEvent.keyDown(screen.getByTestId('search'), { key: 'ArrowDown' }) + fireEvent.keyDown(screen.getByTestId('search'), { key: 'Enter' }) + + expect(onOuterKeyDown).toHaveBeenCalledTimes(2) + }) + + it('opens the keyboard cursor on the ACTIVE mode, not the top of the list', () => { + const onChange = vi.fn() + render() + openMenu() + + // 'grouped' is index 2; Enter with no arrowing re-selects it rather than + // silently jumping the user to 'catalog'. + fireEvent.keyDown(screen.getByTestId('search'), { key: 'Enter' }) + expect(onChange).toHaveBeenCalledWith('grouped') + }) + + it('reports relevance and refuses to open while a query is present', () => { + render() + const button = screen.getByRole('button', { name: /relevance/i }) + + expect(button.hasAttribute('disabled')).toBe(true) + fireEvent.click(button) + expect(screen.queryByRole('menu')).toBeNull() + }) +})