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..5e14a229 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-unified-command-settings.md @@ -0,0 +1,152 @@ +# 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. + +**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 | + +**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). Now the row is disabled +and names the parent switch. Better affordance, not a correctness fix. + +--- + +## 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 + +- [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. +- [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. +- [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. +- [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. +- [x] **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 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 + `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 + +**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 +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.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 1f97c0c0..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( @@ -82,3 +114,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/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 ef2470e0..64b0d042 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', @@ -644,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', @@ -656,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' }, @@ -688,47 +625,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..fd6e1708 100644 --- a/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx +++ b/src/renderer/src/features/settings/ui/CommandKeybindingsRow.tsx @@ -15,8 +15,18 @@ import { resolveEffectiveKeybindings, setCommandKeybindings, } from '@renderer/features/command-keybindings/resolve' +import { + declaredTier, + isVisibleInPicker, + setPickerVisibilityOverride, + suppressingCommandGroup, +} 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, + CommandGroup, +} from '@renderer/features/command-palette/types' // --------------------------------------------------------------------------- // Commands & Shortcuts: the built-in keybinding editor (governance plan §4). @@ -70,6 +80,74 @@ 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 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 = + | { kind: 'editable'; visible: boolean } + | { kind: 'excluded' } + | { kind: 'group-suppressed'; groupLabel: string } + +/** + * 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', +} + +function paletteState( + command: CommandDef, + overrides: Record | undefined, + navigationCommandsEnabled: boolean, +): PaletteState { + if (PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(command.id)) return { kind: 'excluded' } + // 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', + // `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 +176,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 +202,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 +225,36 @@ 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, + // "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) }) - }, [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() @@ -312,6 +424,23 @@ export function CommandKeybindingsRow() { ) : null}
+ {/* 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 => (
@@ -324,9 +453,6 @@ export function CommandKeybindingsRow() { >
{row.title} - {PALETTE_SELF_EXCLUDED_COMMAND_IDS.has(row.id) ? ( - (not shown in palette) - ) : null}
@@ -370,6 +496,20 @@ 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 +517,93 @@ 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} - {/* Command-visibility control: one toggle row per command plus a - reset action. Purely presentational — every state transition - flows out through the control's callbacks (which patch the - sparse override map in settings); this branch never owns or - derives visibility itself. The toggle row reuses the same - border/checkbox styling as the `toggle` control above so the - long list reads as a single coherent group. */} - {control.type === 'command-visibility' ? ( -
-
- {control.commands.map(command => { - const visible = control.isVisible(settings, command) - return ( - - ) - })} -
- -
- ) : 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. */}