From 4e47ea1f55f41233f1798c1bab3cea0d386ab452 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:22:09 +0200 Subject: [PATCH 1/2] feat(settings): one command list, with a Palette column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settings had two lists of the same commands: a searchable, category-grouped keybinding editor, and a flat unsearchable column of visibility buttons. Merge them into the keybinding editor and delete the other. The keybinding list wins because it is built from the FULL command catalog, not the picker-filtered subset — so the merged list is itself the "show all commands" surface. A user who hid something finds it by searching Settings and re-ticking the box. That is why this adds no reveal-all toggle and no bulk enable/disable: the merge removes the need for both. SHOW_HIDDEN_COMMANDS stays a programmatic escape hatch. The column is labelled Palette, never Enabled. Unticking hides the command from the picker LIST and nothing else; it stays executable by the chord shown on the same row, by the native menu, and by programmatic dispatch. Treating picker visibility as an on/off switch is how a cosmetic "tidy my palette" preference once silently killed File -> New Tab. Three row states, not two, because the populations differ and the group gate adds a case: - editable: the ordinary checkbox - excluded: em-dash for commands the palette structurally never lists, where the old list rendered a live switch that persisted an override and could never change anything - group-suppressed: disabled + explanation for members of a disabled command group. This FIXES a live defect — the old list drew all six navigation commands as ON while the palette omitted them, because the group gate outranks per-command overrides, so ticking one wrote an override that changed nothing. Settings stated the opposite of what the user could see. The write rule (prune the override when it equals the declared default) moves next to the read rule in pickerVisibility.ts, so both halves of the same policy have one home. settingsRegistry.ts previously held the write rule inline AND a drifted second copy of the read rule; both are gone, along with listPickerCommandMeta and PickerCommandMeta, now dead. Search covers the new concern: the haystack includes the declared tier and "hidden", so typing either finds what you came for. Resets stay separate. One control doing both would mean restoring shortcuts silently un-hides every command the user deliberately tidied away. Verified: tsc clean on both projects, 1711 tests / 251 files green, check:keybindings OK. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-07-28-unified-command-settings.md | 131 +++++++++++ .../command-palette/pickerVisibility.ts | 33 +++ .../src/features/command-palette/registry.ts | 51 +---- .../features/settings/lib/settingsRegistry.ts | 120 ---------- .../settings/ui/CommandKeybindingsRow.tsx | 214 ++++++++++++++++-- .../src/features/settings/ui/SettingsList.tsx | 34 --- .../voice-dictation/DictationHistoryRow.tsx | 4 +- 7 files changed, 361 insertions(+), 226 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-28-unified-command-settings.md diff --git a/docs/superpowers/plans/2026-07-28-unified-command-settings.md b/docs/superpowers/plans/2026-07-28-unified-command-settings.md new file mode 100644 index 00000000..4fac7e67 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-unified-command-settings.md @@ -0,0 +1,131 @@ +# One Command List in Settings + +**Goal:** Settings has two lists of the same commands. Merge them into one — the +keybinding editor — with a palette-visibility checkbox on the right of each row. + +**Why the keybinding editor wins:** it already has search across title, id, +description, keywords and chords; it already groups by category with an +exhaustive `CATEGORY_RANK`; and it is built from the **full** command catalog +rather than the picker-filtered subset. The visibility list has none of that — +it is a flat, unsearchable column of buttons. + +**The load-bearing consequence, and the reason this merge is worth doing at +all:** because the keybinding list shows *every* command including the hidden +ones, the merged list is itself the "reveal all commands" surface. A user who +hid something finds it by searching Settings and re-ticking the box. That is why +this plan adds **no** reveal-all toggle and **no** bulk enable/disable — the +merge removes the need for both. `showHiddenCommands` stays exactly as it is: a +programmatic escape hatch (`SHOW_HIDDEN_COMMANDS = false` in +`CommandPalette.tsx`), not a user control. + +--- + +## 1. The naming decision, settled up front + +The checkbox column is labelled **Palette**, never "Enabled". + +This is not cosmetic. `pickerVisibility.ts` carries an explicit READ-THIS block: +picker visibility is a *presentation* question and wiring it into any execution +path is a regression. The audit that produced that comment found a real +instance — the native File menu resolved ids against the picker-FILTERED +registry, so `commandVisibilityOverrides['new-tab'] = false`, a purely cosmetic +"tidy my palette" preference, silently killed **File → New Tab**. The user lost +a capability with no error and no way to connect cause to effect. + +A column headed "Enabled" invites exactly that misreading from the next person +who touches this code. A command unticked here stays fully executable by +keybinding, by native menu, and by programmatic dispatch. The row proves it: +the same line still shows and edits that command's chord. + +--- + +## 2. Three row states, not two + +The merged list draws from `builtInCommandCatalog`; the old visibility list drew +from `listPickerCommandMeta()`. Those populations differ, and the group gate +adds a third case. Every row must render one of: + +| State | When | Renders | +| --- | --- | --- | +| **Editable** | ordinary command | live checkbox | +| **Not applicable** | id in `PALETTE_SELF_EXCLUDED_COMMAND_IDS` | `—` + title explaining the palette never lists it | +| **Group-suppressed** | `commandGroup: 'navigation'` while `navigationCommandsEnabled` is false | disabled checkbox, unticked, title naming the parent switch | + +The third state fixes a live defect rather than preserving behaviour. Today +Settings renders all six navigation commands as ON while the palette omits them, +and toggling one writes an override that changes nothing — because +`isVisibleInPicker` checks the group gate *before* per-command overrides +(deliberately: a child switch that appears able to contradict its disabled +parent is the "disabled parent, enabled child" trap). Settings currently states +the opposite of what the user can see. + +--- + +## 3. Where the logic comes from + +Nothing new is written. Both halves already exist and are already tested: + +- **Read** — `isVisibleInPicker(command, policy)` with + `showHiddenCommands: false`. Settings shows the persisted preference, never + the transient reveal-all state. +- **Write** — the prune-on-default rule currently inline in + `settingsRegistry.ts`: setting a command back to its declared tier *deletes* + the override rather than storing a redundant one, so the map only ever holds + deliberate deviations and a future change to a command's declared default is + not silently overridden by a stale entry. + +Both move into `CommandKeybindingsRow.tsx` unchanged in behaviour. + +--- + +## 4. Tasks + +- [ ] **Task 1 — Add the visibility column.** In `CommandKeybindingsRow.tsx`: + read `commandVisibilityOverrides` + `navigationCommandsEnabled` from the + store, compute per-row state per §2, render the checkbox at the right edge + of each command row, and write through the prune-on-default rule. + Extract that rule as an exported helper so it has exactly one home. +- [ ] **Task 2 — Include visibility in the reset.** The row already has a reset + for keybindings. Give the reset control both actions, clearly separated — + one must not silently perform the other. +- [ ] **Task 3 — Delete the old row.** Remove the `command-picker-visibility` + registry entry, the `command-visibility` member of the `SettingDefinition` + union, its `SettingsList.tsx` block, and `resolveCommandVisible` + + `listPickerCommandMeta` if nothing else consumes them. Check before + deleting: `listPickerCommandMeta` may have other callers. +- [ ] **Task 4 — Keep search honest.** The search haystack must cover the new + concern, otherwise a user typing "hidden" finds nothing. Include the + command's declared tier in the searchable text. +- [ ] **Task 5 — Verify.** `tsc` on both projects (raw — electron-vite and + vitest do not type-check), `npm run check:keybindings`, full suite. + +--- + +## 5. Constraints + +- **Comment policy** (`CLAUDE.md`): thick WHY comments. The §1 naming decision + and the §2 group-suppressed state both need the reasoning in the code, not + only here — a future reader who "simplifies" the three states back to two + reintroduces the lying-Settings bug. +- **Copy style** (`docs/command-style.md`): stable noun-phrase titles, no + Toggle/Enable/Show verbs. +- **Do not touch** `pickerVisibility.ts`'s resolution order, the + `PALETTE_SELF_EXCLUDED_COMMAND_IDS` set, or `SHOW_HIDDEN_COMMANDS`. +- Tests go beside their source (`testing/README.md`); filename picks the Vitest + project. + +--- + +## 6. Self-review + +**Least certain:** whether the Settings category description still reads +correctly once two rows become one — worth a look at +`settingsCategories.ts`'s `commands` entry during Task 3. + +**Deliberately out of scope:** a reveal-all control (the merged list is one), +bulk enable/disable (an empty palette with no obvious way back is a worse state +than the problem it solves), and per-category bulk actions. + +**Known limitation kept:** function-typed titles still fall back to the command +id as their label, because resolving them needs a live `CommandContext` that +Settings deliberately does not have. diff --git a/src/renderer/src/features/command-palette/pickerVisibility.ts b/src/renderer/src/features/command-palette/pickerVisibility.ts index 1f97c0c0..f87f2d50 100644 --- a/src/renderer/src/features/command-palette/pickerVisibility.ts +++ b/src/renderer/src/features/command-palette/pickerVisibility.ts @@ -82,3 +82,36 @@ export function declaredTier( ): CommandPickerVisibility { return command.pickerVisibility ?? 'default' } + +/** + * The WRITE half of the same rule `isVisibleInPicker` reads. + * + * Returns the next override map for "the user set this command's palette + * visibility to `visible`". + * + * WHY it prunes instead of always storing the boolean: setting a command back + * to its declared tier DELETES the entry rather than recording a redundant + * `true`/`false`. The map then only ever holds deliberate deviations, which + * matters the day a command's shipped default changes — a stale entry that + * merely restated the old default would silently keep overriding the new one, + * and the user would have no idea they were pinning it. + * + * Lives here, next to the read rule, because the two have to agree about what + * "declared default" means. It used to be inline in `settingsRegistry.ts`, + * which is also where the *second*, drifted copy of the read rule lived — that + * copy did not know about command groups and made Settings state the opposite + * of what the palette showed. One home for each half, both in this file. + */ +export function setPickerVisibilityOverride( + overrides: Record | undefined, + command: Pick, + visible: boolean, +): Record { + const next = { ...(overrides ?? {}) } + if (visible === (declaredTier(command) === 'default')) { + delete next[command.id] + } else { + next[command.id] = visible + } + return next +} diff --git a/src/renderer/src/features/command-palette/registry.ts b/src/renderer/src/features/command-palette/registry.ts index bcc5708b..3d38bf50 100644 --- a/src/renderer/src/features/command-palette/registry.ts +++ b/src/renderer/src/features/command-palette/registry.ts @@ -1,6 +1,6 @@ import { builtInCommandCatalog } from '@renderer/features/command-palette/catalog' import { PALETTE_SELF_EXCLUDED_COMMAND_IDS } from '@renderer/features/command-palette/commands/paletteCommands' -import { declaredTier, isVisibleInPicker } from '@renderer/features/command-palette/pickerVisibility' +import { isVisibleInPicker } from '@renderer/features/command-palette/pickerVisibility' import { displayKeybinding } from '@renderer/features/command-keybindings/normalize' import { resolveEffectiveKeybindings } from '@renderer/features/command-keybindings/resolve' import { commandAllowedByRenderedViewPolicy } from '@renderer/workspace/agentDisplayMode' @@ -8,8 +8,6 @@ import { commandTargetSessionId } from '@renderer/workspace/hook/selectors/comma import type { CommandContext, CommandDef, - CommandGroup, - CommandPickerVisibility, CommandSurface, ResolvedCommand, } from '@renderer/features/command-palette/types' @@ -185,53 +183,6 @@ export function buildCommandRegistry(ctx: CommandContext): ResolvedCommand[] { }) } -/** Static metadata for one command, surfaced to the settings UI so a - * user can flip its picker visibility without the settings layer - * needing a live CommandContext. */ -export type PickerCommandMeta = { - id: string - title: string - pickerVisibility: CommandPickerVisibility - /** Carried so Settings can apply the SAME group gate the picker applies. - * Omitting it was what let the Settings list claim the six Navigation - * Commands were visible while the palette hid them. */ - commandGroup?: CommandGroup -} - -/** - * Flat, context-free list of every command's identity + declared - * picker visibility, for the "Commands" settings category. - * - * WHY context-free: the settings screen has no CommandContext (no - * focused session, no live ui callbacks) and shouldn't synthesize a - * fake one just to read titles. So this deliberately skips per-command - * `when`/`surface` gating — the settings list is the FULL catalog of - * commands a user might want to show/hide, not the subset currently - * applicable. A command being mode-gated out right now doesn't change - * whether the user wants it in the picker when it IS applicable. - * - * Function-typed titles (`title: (ctx) => string`) can't be resolved - * without a context, so we fall back to the stable `id` as the label. - * Those are the toggle-style commands whose text flips with state; the - * id is a stable, recognizable stand-in for a settings row and avoids - * inventing a dummy context purely for a display string. - */ -export function listPickerCommandMeta(): PickerCommandMeta[] { - return commandDefs - // Commands the palette structurally never renders must not appear here - // either. `open-command-palette` was getting a Settings switch that could - // never change anything — buildCommandRegistry filters it out BEFORE any - // visibility logic runs — while still persisting an override when toggled. - // A control that visibly does nothing is worse than an absent one. - .filter(command => !PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(command.id)) - .map(command => ({ - id: command.id, - title: typeof command.title === 'function' ? command.id : command.title, - pickerVisibility: declaredTier(command), - ...(command.commandGroup ? { commandGroup: command.commandGroup } : {}), - })) -} - /** First effective binding as a display chord, or undefined when unbound. */ function displayBinding(bindings: readonly string[] | undefined): string | undefined { const first = bindings?.[0] diff --git a/src/renderer/src/features/settings/lib/settingsRegistry.ts b/src/renderer/src/features/settings/lib/settingsRegistry.ts index ef2470e0..e8014804 100644 --- a/src/renderer/src/features/settings/lib/settingsRegistry.ts +++ b/src/renderer/src/features/settings/lib/settingsRegistry.ts @@ -14,9 +14,6 @@ import type { import type { Workspace } from '@renderer/workspace/workspaceStore' import { SETTING_CATEGORIES } from '@renderer/features/settings/lib/settingsCategories' import type { SettingCategoryId } from '@renderer/features/settings/lib/settingsCategories' -import { listPickerCommandMeta } from '@renderer/features/command-palette/registry' -import { isVisibleInPicker } from '@renderer/features/command-palette/pickerVisibility' -import type { PickerCommandMeta } from '@renderer/features/command-palette/registry' import type { ConfigurableBuiltInMcpDomain } from '@mcp/shared/types' import type { MouseButtonBinding } from '@renderer/lib/mouseBinding' import { coerceMouseChordBinding } from '@renderer/lib/mouseBinding' @@ -252,38 +249,6 @@ export type SettingDefinition = type: 'command-keybindings' } } - | { - id: string - category: SettingCategoryId - title: string - description: string - keywords: string[] - metadata?: SettingMetadata - control: { - type: 'command-visibility' - /** Full command catalog to render rows for. Carried as a value - * (not re-derived in the view) so the registry stays the single - * source of "what commands exist". */ - commands: PickerCommandMeta[] - /** Whether a given command currently shows in the picker, after - * applying the user's override on top of the declared default. - * The view only needs the resolved boolean, not the resolution - * rules. */ - isVisible: (settings: Settings, command: PickerCommandMeta) => boolean - /** Flip one command's visibility. Writes a sparse override entry; - * setting it back to the declared default prunes the entry so the - * map never accumulates no-op rows. */ - onToggleCommand: ( - ctx: SettingActionContext, - command: PickerCommandMeta, - visible: boolean, - ) => void - /** Drop all overrides, returning every command to its declared - * default. */ - onResetVisibility: (ctx: SettingActionContext) => void - } - } - const ACCENT_OPTIONS: ChoiceOption[] = ACCENTS.map(accent => ({ value: accent.id, label: accent.name, @@ -323,45 +288,6 @@ const DICTATION_PROVIDER_OPTIONS: ChoiceOption[] }, ] -// Resolve a command's effective picker visibility from settings alone. -// Mirrors `commandVisible` in the command registry, minus the live -// `showHiddenCommands` escape hatch (the settings UI always edits the -// underlying preference, never the transient reveal-all state). -// -// This now delegates to the SHARED resolver rather than re-implementing the -// rule. It used to be a private second copy, justified by "the settings layer -// shouldn't depend on the registry's CommandContext-typed internals" — a real -// concern that `pickerVisibility.ts` removed by taking a context-free policy -// struct instead of a CommandContext. -// -// Keeping the copy after that was an active defect, not just duplication: the -// copy knew about overrides and the declared tier only, so it never learned -// about the Navigation Commands group. On a fresh install Settings rendered -// all six navigation switches ON (they declare no tier, so the copy said -// "visible") while the palette omitted them — Settings stating the opposite of -// what the user could see. Toggling one wrote an override and still changed -// nothing, because the group gate deliberately outranks per-command overrides. -// That is exactly the "switches that appear able to override their parent" -// shape the precedence rule exists to prevent. -// -// `showHiddenCommands: false` is passed deliberately: Settings shows the -// PERSISTED preference, not the transient reveal-all state, so a user reading -// this list sees what their profile actually does. -function resolveCommandVisible(settings: Settings, command: PickerCommandMeta): boolean { - return isVisibleInPicker( - { - id: command.id, - pickerVisibility: command.pickerVisibility, - commandGroup: command.commandGroup, - }, - { - overrides: settings.commandVisibilityOverrides, - showHiddenCommands: false, - navigationCommandsEnabled: settings.navigationCommandsEnabled, - }, - ) -} - function updateDefaultBuiltInMcpDomain( ctx: SettingActionContext, domain: ConfigurableBuiltInMcpDomain, @@ -380,11 +306,6 @@ function updateDefaultBuiltInMcpDomain( } export function getSettingsRegistry(): SettingDefinition[] { - // Resolved once per registry build. The command catalog is static for - // the lifetime of the app (it's the flat `commandDefs` array), so - // there's no reason to recompute it per render. - const pickerCommands = listPickerCommandMeta() - return [ { id: 'theme-mode', @@ -688,47 +609,6 @@ export function getSettingsRegistry(): SettingDefinition[] { onToggle: (ctx, value) => ctx.onChange({ navigationCommandsEnabled: value }), }, }, - { - id: 'command-picker-visibility', - category: 'commands', - title: 'Command Picker Visibility', - description: - 'Choose which commands appear in the command picker. Hiding a command only removes it from the picker list — its keyboard shortcut still works.', - keywords: [ - 'command', - 'picker', - 'palette', - 'visibility', - 'hide', - 'show', - 'advanced', - 'debug', - ], - metadata: { scope: 'app', apply: 'immediate', storage: 'settings' }, - control: { - type: 'command-visibility', - commands: pickerCommands, - isVisible: resolveCommandVisible, - onToggleCommand: (ctx, command, visible) => { - const next = { ...ctx.settings.commandVisibilityOverrides } - // Prune the entry when the new state equals the command's - // declared default, so the override map only ever holds - // deliberate deviations. Without this, toggling a command off - // then on again would leave a redundant `true` (or `false`) - // that survives a default change in a future release. - const declaredVisible = command.pickerVisibility === 'default' - if (visible === declaredVisible) { - delete next[command.id] - } else { - next[command.id] = visible - } - ctx.onChange({ commandVisibilityOverrides: next }) - }, - onResetVisibility: ctx => { - ctx.onChange({ commandVisibilityOverrides: {} }) - }, - }, - }, { id: 'aggressive-debug-persistence', category: 'experimental', diff --git a/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx b/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx index 56506fb3..5948ed57 100644 --- a/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx +++ b/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx @@ -15,8 +15,13 @@ import { resolveEffectiveKeybindings, setCommandKeybindings, } from '@renderer/features/command-keybindings/resolve' +import { + declaredTier, + isVisibleInPicker, + setPickerVisibilityOverride, +} from '@renderer/features/command-palette/pickerVisibility' import type { Keybinding } from '@renderer/features/command-keybindings/normalize' -import type { CommandCategory } from '@renderer/features/command-palette/types' +import type { CommandCategory, CommandDef } from '@renderer/features/command-palette/types' // --------------------------------------------------------------------------- // Commands & Shortcuts: the built-in keybinding editor (governance plan §4). @@ -70,6 +75,63 @@ const CATEGORY_RANK: Record = { const CATEGORY_ORDER = (Object.keys(CATEGORY_RANK) as CommandCategory[]) .sort((a, b) => CATEGORY_RANK[a] - CATEGORY_RANK[b]) +/** + * What the Palette column can show for one command. THREE states, not two. + * + * A plain boolean was not enough, and collapsing this back to one would + * reintroduce a defect rather than simplify: + * + * - `excluded` — the palette structurally never lists this command + * (`PALETTE_SELF_EXCLUDED_COMMAND_IDS`, i.e. "open the palette" inside the + * palette). The old Settings list rendered a live switch here that persisted + * an override and could never change anything, because the registry filters + * the command out BEFORE any visibility logic runs. A control that visibly + * does nothing is worse than an absent one. + * - `group-suppressed` — a member of a disabled command GROUP. The group gate + * outranks per-command overrides by design (`isVisibleInPicker` step 2 + * before step 3), so an editable checkbox here would be a switch that + * appears able to contradict its own disabled parent. The old list did + * exactly this: it drew all six navigation commands as ON while the palette + * omitted them, and ticking one wrote an override that changed nothing. + * - `editable` — the ordinary case. + */ +type PaletteState = + | { kind: 'editable'; visible: boolean } + | { kind: 'excluded' } + | { kind: 'group-suppressed'; groupLabel: string } + +/** Human name for a command group, for the "off via X" explanation. Keyed by + * the group id so adding a group without a label is visible here rather than + * rendering a bare identifier at the user. */ +const COMMAND_GROUP_LABELS: Record = { + navigation: 'Navigation Commands', +} + +function paletteState( + command: CommandDef, + overrides: Record | undefined, + navigationCommandsEnabled: boolean, +): PaletteState { + if (PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(command.id)) return { kind: 'excluded' } + if (command.commandGroup === 'navigation' && !navigationCommandsEnabled) { + return { + kind: 'group-suppressed', + groupLabel: COMMAND_GROUP_LABELS[command.commandGroup] ?? command.commandGroup, + } + } + return { + kind: 'editable', + // `showHiddenCommands: false` deliberately: Settings shows the PERSISTED + // preference, not the transient reveal-all state, so what the user reads + // here is what their profile actually does. + visible: isVisibleInPicker(command, { + overrides, + showHiddenCommands: false, + navigationCommandsEnabled, + }), + } +} + type PendingConflict = { commandId: string binding: Keybinding @@ -98,6 +160,8 @@ export function CommandKeybindingsRow() { const [conflict, setConflict] = useState(null) const overrides = settings.commandKeybindingOverrides + const visibilityOverrides = settings.commandVisibilityOverrides + const navigationCommandsEnabled = settings.navigationCommandsEnabled const defaults = useMemo(() => buildDefaultKeybindings(), []) const effective = useMemo(() => { @@ -122,15 +186,21 @@ export function CommandKeybindingsRow() { // A command the palette never renders still gets a binding row — it is // reachable by chord, menu and programmatic call, so it is bindable. .filter(command => command.category) - .map(command => ({ - id: command.id, - title: typeof command.title === 'function' ? command.id : command.title, - category: command.category as CommandCategory, - description: command.description, - keywords: command.keywords ?? [], - bindings: effective.get(command.id) ?? [], - customized: overrides[command.id] !== undefined, - })) + .map(command => { + const palette = paletteState(command, visibilityOverrides, navigationCommandsEnabled) + return { + id: command.id, + command, + title: typeof command.title === 'function' ? command.id : command.title, + category: command.category as CommandCategory, + description: command.description, + keywords: command.keywords ?? [], + bindings: effective.get(command.id) ?? [], + customized: overrides[command.id] !== undefined, + palette, + tier: declaredTier(command), + } + }) .filter(row => { if (!needle) return true const haystack = [ @@ -139,10 +209,28 @@ export function CommandKeybindingsRow() { row.description, ...row.keywords, ...row.bindings.map(displayKeybinding), + // Searchable by the visibility concern too, now that it lives here. + // Without this a user typing "hidden" — the whole reason they opened + // this list — matches nothing. + row.tier, + row.palette.kind === 'editable' && !row.palette.visible ? 'hidden' : '', ].join(' ').toLowerCase() return haystack.includes(needle) }) - }, [query, effective, overrides]) + }, [query, effective, overrides, visibilityOverrides, navigationCommandsEnabled]) + + const setPaletteVisible = useCallback( + (command: CommandDef, visible: boolean) => { + setSettings({ + commandVisibilityOverrides: setPickerVisibilityOverride( + visibilityOverrides, + command, + visible, + ), + }) + }, + [visibilityOverrides, setSettings], + ) const grouped = useMemo(() => { const byCategory = new Map() @@ -311,6 +399,20 @@ export function CommandKeybindingsRow() { ) : null} + {/* Column header. Without it the checkbox column is unexplained, and the + one thing a user must not assume about it is that it disables the + command. The sub-line says so in the only place they will read it. */} +
+ Command + Shortcut + + Palette + +
+
{grouped.map(group => (
@@ -324,9 +426,6 @@ export function CommandKeybindingsRow() { >
{row.title} - {PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(row.id) ? ( - (not shown in palette) - ) : null}
@@ -370,6 +469,19 @@ export function CommandKeybindingsRow() { Reset ) : null} + + {/* Palette column, pinned to the right edge of every row. + Labelled "palette", NEVER "enabled": unticking hides the + command from the picker LIST and nothing else — it stays + fully executable by the chord shown on this very row, by + the native menu, and by programmatic dispatch. Treating + this as an on/off switch is how a cosmetic "tidy my + palette" preference once silently killed File → New Tab. + See the READ THIS block in pickerVisibility.ts. */} + setPaletteVisible(row.command, visible)} + />
))} @@ -377,16 +489,78 @@ export function CommandKeybindingsRow() { ))}
- + {/* Two resets, kept SEPARATE on purpose. One control doing both would + mean a user restoring their shortcuts silently un-hides every command + they deliberately tidied away (or the reverse) — a destructive side + effect on a concern they did not mention. They live side by side + because the list now edits both concerns; they do not merge. */} +
+ + +
) } +/** + * The Palette cell. A real `` rather than the styled + * ` - ) - })} - - - - ) : null} - {/* CLI auto-updater — the row owns its own subscription because the value lives in setup.json (main-owned), not in the renderer Settings store. See diff --git a/src/renderer/src/features/voice-dictation/DictationHistoryRow.tsx b/src/renderer/src/features/voice-dictation/DictationHistoryRow.tsx index 37f3ea3f..2c20f530 100644 --- a/src/renderer/src/features/voice-dictation/DictationHistoryRow.tsx +++ b/src/renderer/src/features/voice-dictation/DictationHistoryRow.tsx @@ -134,8 +134,8 @@ export function DictationHistoryRow() {
Recent — last {stats.retainedEntries}
- {/* Bounded, matching the `command-visibility` row's precedent in - SettingsList. Unbounded, 200 retained transcripts inject ~5,600px + {/* Bounded, matching the keybinding list's precedent in + CommandKeybindingsRow. Unbounded, 200 retained transcripts inject ~5,600px into the middle of the Settings page and push every row below this one — including Dictation Shortcut, which is what a user most likely opened Settings to find — off screen. */} From 8d64196e261fae5e802dcaa7a78a6ff83fc65820 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 28 Jul 2026 16:47:13 +0200 Subject: [PATCH 2/2] fix(settings): resolve Codex + Claude review of the merged command list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers cleared the core: picker visibility stays presentation-only, no command lost or gained a row, and the prune rule is correct for all eight tier/value combinations. Seven fixes below. CORRECTION FIRST. The previous commit message and the plan claimed this PR fixed a live defect — that Settings showed all six navigation commands as ON while the palette hid them. That was WRONG and Codex caught it. At base 1e4f2d8f, resolveCommandVisible already delegated to isVisibleInPicker INCLUDING commandGroup, so the old list computed the checked state correctly. That bug existed once and was fixed in an earlier PR; the past-tense comment on PickerCommandMeta.commandGroup describing it is what misled the claim. What the third row state actually adds is smaller: those rows used to render an enabled, unticked checkbox whose click wrote an override the group gate silently outranked. Now the row is disabled and names the parent switch. Better affordance, not a correctness fix. The plan and the code comment now say so. ACCESSIBILITY. The checkbox had no accessible name on any of 98 rows — the wrapping label holds no text, so screen readers announced "checkbox, unchecked" 98 times with no indication of which command. And the suppressed state explained itself only through `title`, which is mouse-hover-only on a control that is out of the tab order anyway. Both now carry aria-label. SEARCH. Deleting the old row deleted its Settings-search vocabulary with it, so typing "hide", "hidden", "visibility" or "visible" into Settings returned nothing at all even though the control was right there. The surviving row now carries both vocabularies, its description covers both concerns, and the Commands category description no longer claims the category is only about the picker. DUPLICATED RULE. paletteState re-implemented the group gate as a literal `commandGroup === 'navigation'` test — a second copy of the rule, in the PR that consolidated the other half, and one that does not generalize: teaching isVisibleInPicker about a second gated group would leave Settings rendering an enabled checkbox that snaps back when ticked. Extracted suppressingCommandGroup() as the single implementation, and made COMMAND_GROUP_LABELS Record so a new group is a compile error rather than a bare id shown to the user. Also: the in-list "hidden" search token now covers group-suppressed rows (which ship suppressed by default, so they were the likeliest reason to search for it and the only ones excluded); the column header moved inside the scroll container and sticks, since outside it drifted by the scrollbar width; and the orphaned JSX comment left behind by the deleted branch is gone. TESTS. setPickerVisibilityOverride shipped with none — the prune rule is the half most likely to be "simplified" into `next[id] = visible`, and the suite would have stayed green. Ten new cases cover both directions for every tier, a round-trip property against isVisibleInPicker, non-mutation, the undefined map, and suppressingCommandGroup's agreement with the gate. Verified: tsc clean on both projects, 1721 tests / 251 files green, check:keybindings OK. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-07-28-unified-command-settings.md | 51 +++++++--- .../command-palette/pickerVisibility.test.ts | 92 +++++++++++++++++ .../command-palette/pickerVisibility.ts | 36 ++++++- .../settings/lib/settingsCategories.ts | 2 +- .../features/settings/lib/settingsRegistry.ts | 20 +++- .../settings/ui/CommandKeybindingsRow.tsx | 99 +++++++++++++------ .../src/features/settings/ui/SettingsList.tsx | 7 -- 7 files changed, 252 insertions(+), 55 deletions(-) diff --git a/docs/superpowers/plans/2026-07-28-unified-command-settings.md b/docs/superpowers/plans/2026-07-28-unified-command-settings.md index 4fac7e67..5e14a229 100644 --- a/docs/superpowers/plans/2026-07-28-unified-command-settings.md +++ b/docs/superpowers/plans/2026-07-28-unified-command-settings.md @@ -1,5 +1,13 @@ # One Command List in Settings +> **Status: IMPLEMENTED**, then revised after a two-reviewer pass (one Codex, +> one Claude). Both cleared the core — picker visibility stays presentation-only, +> no command lost or gained a row, the prune rule is correct for every tier. +> Between them they found one factual error in this plan (see the correction in +> §2), a missing accessible name on all 98 checkboxes, a lost Settings-search +> vocabulary, and a second copy of the group-gate rule that did not generalize. +> All fixed. + **Goal:** Settings has two lists of the same commands. Merge them into one — the keybinding editor — with a palette-visibility checkbox on the right of each row. @@ -51,13 +59,22 @@ adds a third case. Every row must render one of: | **Not applicable** | id in `PALETTE_SELF_EXCLUDED_COMMAND_IDS` | `—` + title explaining the palette never lists it | | **Group-suppressed** | `commandGroup: 'navigation'` while `navigationCommandsEnabled` is false | disabled checkbox, unticked, title naming the parent switch | -The third state fixes a live defect rather than preserving behaviour. Today -Settings renders all six navigation commands as ON while the palette omits them, -and toggling one writes an override that changes nothing — because -`isVisibleInPicker` checks the group gate *before* per-command overrides +**Correction (post-review):** an earlier draft of this plan claimed the third +state fixed a live defect — that Settings rendered all six navigation commands +as ON while the palette omitted them. **That was wrong**, and the review caught +it. At base `1e4f2d8f`, `resolveCommandVisible` already delegated to +`isVisibleInPicker` *including* `commandGroup`, so the old list computed the +checked state correctly. That bug existed once and was fixed in an earlier PR; +the past-tense comment on `PickerCommandMeta.commandGroup` describing it is what +misled the draft. + +What the third state actually adds is smaller and worth stating honestly: those +rows previously rendered as an ordinary **enabled, unticked** checkbox with no +explanation. Clicking one wrote an override that changed nothing visible, +because `isVisibleInPicker` checks the group gate *before* per-command overrides (deliberately: a child switch that appears able to contradict its disabled -parent is the "disabled parent, enabled child" trap). Settings currently states -the opposite of what the user can see. +parent is the "disabled parent, enabled child" trap). Now the row is disabled +and names the parent switch. Better affordance, not a correctness fix. --- @@ -80,23 +97,23 @@ Both move into `CommandKeybindingsRow.tsx` unchanged in behaviour. ## 4. Tasks -- [ ] **Task 1 — Add the visibility column.** In `CommandKeybindingsRow.tsx`: +- [x] **Task 1 — Add the visibility column.** In `CommandKeybindingsRow.tsx`: read `commandVisibilityOverrides` + `navigationCommandsEnabled` from the store, compute per-row state per §2, render the checkbox at the right edge of each command row, and write through the prune-on-default rule. Extract that rule as an exported helper so it has exactly one home. -- [ ] **Task 2 — Include visibility in the reset.** The row already has a reset +- [x] **Task 2 — Include visibility in the reset.** The row already has a reset for keybindings. Give the reset control both actions, clearly separated — one must not silently perform the other. -- [ ] **Task 3 — Delete the old row.** Remove the `command-picker-visibility` +- [x] **Task 3 — Delete the old row.** Remove the `command-picker-visibility` registry entry, the `command-visibility` member of the `SettingDefinition` union, its `SettingsList.tsx` block, and `resolveCommandVisible` + `listPickerCommandMeta` if nothing else consumes them. Check before deleting: `listPickerCommandMeta` may have other callers. -- [ ] **Task 4 — Keep search honest.** The search haystack must cover the new +- [x] **Task 4 — Keep search honest.** The search haystack must cover the new concern, otherwise a user typing "hidden" finds nothing. Include the command's declared tier in the searchable text. -- [ ] **Task 5 — Verify.** `tsc` on both projects (raw — electron-vite and +- [x] **Task 5 — Verify.** `tsc` on both projects (raw — electron-vite and vitest do not type-check), `npm run check:keybindings`, full suite. --- @@ -106,7 +123,8 @@ Both move into `CommandKeybindingsRow.tsx` unchanged in behaviour. - **Comment policy** (`CLAUDE.md`): thick WHY comments. The §1 naming decision and the §2 group-suppressed state both need the reasoning in the code, not only here — a future reader who "simplifies" the three states back to two - reintroduces the lying-Settings bug. + reintroduces an enabled checkbox whose value the group gate silently + outranks. - **Copy style** (`docs/command-style.md`): stable noun-phrase titles, no Toggle/Enable/Show verbs. - **Do not touch** `pickerVisibility.ts`'s resolution order, the @@ -118,9 +136,12 @@ Both move into `CommandKeybindingsRow.tsx` unchanged in behaviour. ## 6. Self-review -**Least certain:** whether the Settings category description still reads -correctly once two rows become one — worth a look at -`settingsCategories.ts`'s `commands` entry during Task 3. +**Was least certain, and the review confirmed it mattered:** whether the +Settings copy still read correctly once two rows became one. It did not — the +surviving row kept only the keybinding vocabulary, so a user searching Settings +for "hide" or "visibility" got zero results even though the control was right +there. Both the row's keywords/description and the `commands` category +description now cover both concerns. **Deliberately out of scope:** a reveal-all control (the merged list is one), bulk enable/disable (an empty palette with no obvious way back is a worse state diff --git a/src/renderer/src/features/command-palette/pickerVisibility.test.ts b/src/renderer/src/features/command-palette/pickerVisibility.test.ts index 5f24719f..de784cc6 100644 --- a/src/renderer/src/features/command-palette/pickerVisibility.test.ts +++ b/src/renderer/src/features/command-palette/pickerVisibility.test.ts @@ -3,6 +3,8 @@ import { describe, expect, it } from 'vitest' import { declaredTier, isVisibleInPicker, + setPickerVisibilityOverride, + suppressingCommandGroup, } from '@renderer/features/command-palette/pickerVisibility' import type { CommandPickerVisibility } from '@renderer/features/command-palette/types' @@ -100,4 +102,94 @@ describe('isVisibleInPicker', () => { expect(isVisibleInPicker(cmd('a'), policy({ overrides }))).toBe(true) }) }) + + describe('setPickerVisibilityOverride (the WRITE half)', () => { + // The prune rule is the half most likely to be "simplified" into + // `next[id] = visible` by someone who does not read the docstring. Without + // these four cases the whole suite stays green through that change, and the + // bug only shows up the day a command's shipped default changes and a stale + // entry that merely restated the old default keeps overriding the new one. + + it('deletes the entry when a default-tier command is set visible', () => { + const next = setPickerVisibilityOverride({ a: false }, cmd('a'), true) + expect(next).not.toHaveProperty('a') + }) + + it('stores false when a default-tier command is hidden', () => { + expect(setPickerVisibilityOverride({}, cmd('a'), false)).toEqual({ a: false }) + }) + + it('deletes the entry when a hidden-tier command is set back to hidden', () => { + for (const tier of ['advanced', 'experimental', 'debug'] as const) { + const next = setPickerVisibilityOverride({ a: true }, cmd('a', tier), false) + expect(next, tier).not.toHaveProperty('a') + } + }) + + it('stores true when a hidden-tier command is revealed', () => { + expect(setPickerVisibilityOverride({}, cmd('a', 'debug'), true)).toEqual({ a: true }) + }) + + it('never mutates the map it was given', () => { + const before = { a: false, b: true } + setPickerVisibilityOverride(before, cmd('a'), true) + expect(before).toEqual({ a: false, b: true }) + }) + + it('tolerates an undefined map, like the read half does', () => { + expect(setPickerVisibilityOverride(undefined, cmd('a'), false)).toEqual({ a: false }) + }) + + it('round-trips with isVisibleInPicker for every tier', () => { + // The two halves must agree about what "declared default" means; this is + // the property that keeps them from drifting apart. + for (const tier of [undefined, 'advanced', 'experimental', 'debug'] as const) { + for (const visible of [true, false]) { + const overrides = setPickerVisibilityOverride({}, cmd('a', tier), visible) + expect(isVisibleInPicker(cmd('a', tier), policy({ overrides })), `${tier}/${visible}`) + .toBe(visible) + } + } + }) + }) + + describe('suppressingCommandGroup', () => { + // Settings needs to tell "user unticked it" apart from "its parent group is + // off" so it can disable that row and NAME the parent. Exported for exactly + // that; pinned here so it cannot drift from the gate isVisibleInPicker uses. + it('names the group when the group is off', () => { + expect( + suppressingCommandGroup( + { commandGroup: 'navigation' }, + { navigationCommandsEnabled: false }, + ), + ).toBe('navigation') + }) + + it('returns null when the group is on, or when the command has no group', () => { + expect( + suppressingCommandGroup( + { commandGroup: 'navigation' }, + { navigationCommandsEnabled: true }, + ), + ).toBeNull() + expect(suppressingCommandGroup({}, { navigationCommandsEnabled: false })).toBeNull() + }) + + it('agrees with isVisibleInPicker: a suppressed command is never visible', () => { + const command = { id: 'a', commandGroup: 'navigation' as const } + expect(suppressingCommandGroup(command, { navigationCommandsEnabled: false })).not.toBeNull() + expect( + isVisibleInPicker(command, policy({ navigationCommandsEnabled: false })), + ).toBe(false) + // ...even with an explicit override trying to force it on, because the + // group gate deliberately outranks per-command overrides. + expect( + isVisibleInPicker( + command, + policy({ navigationCommandsEnabled: false, overrides: { a: true } }), + ), + ).toBe(false) + }) + }) }) diff --git a/src/renderer/src/features/command-palette/pickerVisibility.ts b/src/renderer/src/features/command-palette/pickerVisibility.ts index f87f2d50..67b7c5b5 100644 --- a/src/renderer/src/features/command-palette/pickerVisibility.ts +++ b/src/renderer/src/features/command-palette/pickerVisibility.ts @@ -1,4 +1,8 @@ -import type { CommandDef, CommandPickerVisibility } from '@renderer/features/command-palette/types' +import type { + CommandDef, + CommandGroup, + CommandPickerVisibility, +} from '@renderer/features/command-palette/types' /** * Everything the visibility decision needs, and nothing else. @@ -61,7 +65,7 @@ export function isVisibleInPicker( ): boolean { if (policy.showHiddenCommands) return true - if (command.commandGroup === 'navigation' && !policy.navigationCommandsEnabled) return false + if (suppressingCommandGroup(command, policy) !== null) return false // Optional-chain defensively: this runs inside the palette's first-render // useMemo, so if `overrides` is ever undefined (a persisted-settings shape @@ -75,6 +79,34 @@ export function isVisibleInPicker( return declaredTier(command) === 'default' } +/** + * Which command GROUP, if any, is currently suppressing this command — step 2 + * of the resolution order, extracted so it has exactly one implementation. + * + * WHY it is exported rather than inlined into `isVisibleInPicker`: Settings + * needs to distinguish "hidden because the user unticked it" from "hidden + * because its parent group is off", so it can disable that row and NAME the + * parent instead of offering a checkbox whose value the group gate will + * immediately outrank. Before this existed the Settings row re-implemented the + * `commandGroup === 'navigation' && !navigationCommandsEnabled` test locally — + * a second copy of the rule, in the same PR that consolidated the other half. + * The copy did not generalize: adding a second gated group would teach + * `isVisibleInPicker` about it and leave Settings rendering an enabled, + * unticked checkbox that snaps back the moment it is ticked. + * + * Returns the group id so the caller can label it, or `null` when nothing is + * group-suppressing this command. + */ +export function suppressingCommandGroup( + command: Pick, + policy: Pick, +): CommandGroup | null { + if (command.commandGroup === 'navigation' && !policy.navigationCommandsEnabled) { + return 'navigation' + } + return null +} + /** The command's declared tier, with the documented `absent ≡ 'default'` rule * applied in ONE place so callers never re-implement the fallback and drift. */ export function declaredTier( diff --git a/src/renderer/src/features/settings/lib/settingsCategories.ts b/src/renderer/src/features/settings/lib/settingsCategories.ts index 33f0457f..559f4cb0 100644 --- a/src/renderer/src/features/settings/lib/settingsCategories.ts +++ b/src/renderer/src/features/settings/lib/settingsCategories.ts @@ -32,7 +32,7 @@ export const SETTING_CATEGORIES: SettingCategory[] = [ { id: 'commands', label: 'Commands', - description: 'Which commands appear in the command picker.', + description: 'Keyboard shortcuts, and which commands appear in the command picker.', }, { id: 'dictation', diff --git a/src/renderer/src/features/settings/lib/settingsRegistry.ts b/src/renderer/src/features/settings/lib/settingsRegistry.ts index e8014804..64b0d042 100644 --- a/src/renderer/src/features/settings/lib/settingsRegistry.ts +++ b/src/renderer/src/features/settings/lib/settingsRegistry.ts @@ -565,9 +565,14 @@ export function getSettingsRegistry(): SettingDefinition[] { { id: 'command-keybindings', category: 'commands', - title: 'Keyboard Shortcuts', + title: 'Commands and Shortcuts', description: - 'Assign, add, or remove keyboard shortcuts for built-in commands. A command may have several bindings or none. Conflicts are blocked and name the command or app interaction that already owns the chord.', + 'Assign keyboard shortcuts, and choose which commands appear in the command picker. A command may have several bindings or none; conflicts are blocked and name whatever already owns the chord. Hiding a command only removes it from the picker list — its shortcut still works.', + // Carries BOTH vocabularies. This row absorbed the deleted + // 'Command Picker Visibility' row, and Settings search matches on + // keywords — so dropping that row's terms would mean a user typing + // "hide" or "visibility" (the words for the thing they want) gets no + // result at all, even though the control is right there. keywords: [ 'keybinding', 'keyboard', @@ -577,6 +582,17 @@ export function getSettingsRegistry(): SettingDefinition[] { 'rebind', 'hotkey', 'conflict', + 'command', + 'picker', + 'palette', + 'visibility', + 'visible', + 'hide', + 'hidden', + 'show', + 'advanced', + 'experimental', + 'debug', ], metadata: { scope: 'app', apply: 'immediate', storage: 'settings' }, control: { type: 'command-keybindings' }, diff --git a/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx b/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx index 5948ed57..fd6e1708 100644 --- a/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx +++ b/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx @@ -19,9 +19,14 @@ import { declaredTier, isVisibleInPicker, setPickerVisibilityOverride, + suppressingCommandGroup, } from '@renderer/features/command-palette/pickerVisibility' import type { Keybinding } from '@renderer/features/command-keybindings/normalize' -import type { CommandCategory, CommandDef } from '@renderer/features/command-palette/types' +import type { + CommandCategory, + CommandDef, + CommandGroup, +} from '@renderer/features/command-palette/types' // --------------------------------------------------------------------------- // Commands & Shortcuts: the built-in keybinding editor (governance plan §4). @@ -90,9 +95,11 @@ const CATEGORY_ORDER = (Object.keys(CATEGORY_RANK) as CommandCategory[]) * - `group-suppressed` — a member of a disabled command GROUP. The group gate * outranks per-command overrides by design (`isVisibleInPicker` step 2 * before step 3), so an editable checkbox here would be a switch that - * appears able to contradict its own disabled parent. The old list did - * exactly this: it drew all six navigation commands as ON while the palette - * omitted them, and ticking one wrote an override that changed nothing. + * appears able to contradict its own disabled parent. The old list computed + * the *checked state* correctly but still rendered an ENABLED box: clicking + * it wrote an override that changed nothing visible, because the gate + * outranks it at read time. Disabling the row and naming the parent is the + * difference. * - `editable` — the ordinary case. */ type PaletteState = @@ -100,10 +107,17 @@ type PaletteState = | { kind: 'excluded' } | { kind: 'group-suppressed'; groupLabel: string } -/** Human name for a command group, for the "off via X" explanation. Keyed by - * the group id so adding a group without a label is visible here rather than - * rendering a bare identifier at the user. */ -const COMMAND_GROUP_LABELS: Record = { +/** + * Human name for a command group, for the "off via X" explanation. + * + * EXHAUSTIVE by type, for the same reason `CATEGORY_RANK` above is: a new + * `CommandGroup` member must be a compile error here, not a row that silently + * renders its bare identifier at the user. The earlier `Record` + * version had a `?? command.commandGroup` fallback whose comment claimed to + * catch exactly that — and which TypeScript could prove unreachable, because + * the only key was also the only narrowed literal. + */ +const COMMAND_GROUP_LABELS: Record = { navigation: 'Navigation Commands', } @@ -113,11 +127,13 @@ function paletteState( navigationCommandsEnabled: boolean, ): PaletteState { if (PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(command.id)) return { kind: 'excluded' } - if (command.commandGroup === 'navigation' && !navigationCommandsEnabled) { - return { - kind: 'group-suppressed', - groupLabel: COMMAND_GROUP_LABELS[command.commandGroup] ?? command.commandGroup, - } + // Ask the shared rule which group is suppressing this command rather than + // re-testing the flag here. A local copy would not generalize: teaching + // `isVisibleInPicker` about a second gated group while this stayed literal + // would leave an enabled, unticked checkbox that snaps back when ticked. + const suppressingGroup = suppressingCommandGroup(command, { navigationCommandsEnabled }) + if (suppressingGroup !== null) { + return { kind: 'group-suppressed', groupLabel: COMMAND_GROUP_LABELS[suppressingGroup] } } return { kind: 'editable', @@ -213,7 +229,15 @@ export function CommandKeybindingsRow() { // Without this a user typing "hidden" — the whole reason they opened // this list — matches nothing. row.tier, - row.palette.kind === 'editable' && !row.palette.visible ? 'hidden' : '', + // "hidden" must cover EVERY reason a command is absent from the + // palette, not just an unticked box. Navigation commands ship + // group-suppressed on a fresh install, so they are the likeliest + // reason someone searches this list for "hidden" in the first place — + // omitting them returned every advanced/debug command except the ones + // the user was looking for. + row.palette.kind === 'excluded' || (row.palette.kind === 'editable' && row.palette.visible) + ? '' + : 'hidden', ].join(' ').toLowerCase() return haystack.includes(needle) }) @@ -399,21 +423,24 @@ export function CommandKeybindingsRow() { ) : null} - {/* Column header. Without it the checkbox column is unexplained, and the - one thing a user must not assume about it is that it disables the - command. The sub-line says so in the only place they will read it. */} -
- Command - Shortcut - - Palette - -
-
+ {/* Column header lives INSIDE the scroll container, and sticks. + Outside it, the header sits in a box that is not narrowed by the + scrollbar while the rows below it are — so on any platform with + non-overlay scrollbars the "Palette" caption drifts ~15px right of + the column it names, and 98 rows guarantee a scrollbar. Sticky keeps + it visible while scrolling, which a long list needs anyway. */} +
+ Command + Shortcut + + Palette + +
+ {grouped.map(group => (
@@ -480,6 +507,7 @@ export function CommandKeybindingsRow() { See the READ THIS block in pickerVisibility.ts. */} setPaletteVisible(row.command, visible)} />
@@ -522,9 +550,12 @@ export function CommandKeybindingsRow() { */ function PaletteToggle({ state, + commandTitle, onChange, }: { state: PaletteState + /** Needed for the accessible name — see below. */ + commandTitle: string onChange: (visible: boolean) => void }) { if (state.kind === 'excluded') { @@ -532,6 +563,7 @@ function PaletteToggle({ — @@ -539,6 +571,16 @@ function PaletteToggle({ } const suppressed = state.kind === 'group-suppressed' + // Every explanation goes in `aria-label`, not only `title`. + // + // `title` is mouse-hover-only, and a DISABLED input is out of the tab order + // entirely — so for a keyboard or screen-reader user the suppressed state was + // an inert control with no reachable reason. And because the wrapping + //