Skip to content

Commit add4db8

Browse files
committed
Bugfix: Native macOS shortcuts are read-only
- The four macOS-native commands (`app.quit`/`hide`/`hideOthers`/`showAll`) are AppKit `PredefinedMenuItem`s: macOS owns both the behavior and the accelerator, so editing them in Settings was a double illusion (removal didn't disable the OS accelerator; a new binding dispatched into a void). - Registry: add a `nativeShortcut?: true` flag on the `Command` interface, set on exactly those four entries. New `NATIVE_SHORTCUT_COMMAND_IDS` export is the single source of truth; `command-handlers/types.ts` now sources its Family-1 dispatch-exempt list from it. A `command-registry.test.ts` set-equality test pins the flag to the list and proves the dispatch-exempt round-trip. - Editor: native rows render read-only — plain static pills, a small "macOS" badge with the tooltip "macOS handles this shortcut. Cmdr can't change it.", and no `+`/`×`/reset/add-slot. `Show all` shows `(none)` unframed plus the badge. Normal rows keep every affordance. - Honest conflict capture: when a captured combo conflicts with a native command (native wins even in a mixed set), the banner reads `⌘H is reserved by macOS (Hide Cmdr) and won't reach Cmdr. Pick a different combo.` and offers ONLY Cancel — no "Remove from other" / "Keep both" (both would be a lie). Purely non-native conflicts keep the resolvable banner. Classification extracted to the pure `keyboard-shortcuts-banner.ts`. - Store: `initializeShortcuts` drops any persisted native customization on load (the dev `shortcuts.json` had `app.hide: []`); `setShortcut`/`addShortcut`/`removeShortcut` no-op with a `log.warn` for native commands (defense in depth — MCP shortcut edits route through these mutators too). `resetShortcut` stays permissive (delete-only). New `isNativeShortcutCommand` predicate. - Scrollbar gutter: reserve `scrollbar-gutter: stable` on `.commands-list` so the trailing `+`/badge controls never sit under an overlay scrollbar when the list scrolls. - Migrated palette / store tests that used native ids as plain rebindable fixtures to non-native commands; added store-guard, healing, read-only-row, reserved-banner, and conflict-detection tests. Updated colocated CLAUDE.md docs.
1 parent d1e43f8 commit add4db8

16 files changed

Lines changed: 568 additions & 123 deletions

apps/desktop/src/lib/command-palette/CommandPalette.test.ts

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -487,8 +487,10 @@ describe('CommandPalette', () => {
487487
})
488488

489489
it('renders the effective custom binding, not the registry default', async () => {
490-
// app.quit defaults to ⌘Q; rebind it and the row must show the custom combo.
491-
setShortcut('app.quit', 0, '⌘0')
490+
// view.showHidden defaults to ⌘⇧.; rebind it and the row must show the custom
491+
// combo. (A non-native command: the macOS-native commands like app.quit can't
492+
// be rebound and aren't shown in the palette anyway.)
493+
setShortcut('view.showHidden', 0, '⌘0')
492494

493495
const target = document.createElement('div')
494496
mount(CommandPalette, {
@@ -497,7 +499,7 @@ describe('CommandPalette', () => {
497499
})
498500
await tick()
499501

500-
expect(shortcutTextsFor(target, 'app.quit')).toEqual(['⌘0'])
502+
expect(shortcutTextsFor(target, 'view.showHidden')).toEqual(['⌘0'])
501503
})
502504

503505
it('caps the shown shortcuts at three', async () => {
@@ -519,8 +521,9 @@ describe('CommandPalette', () => {
519521

520522
it('updates a rendered row live when the binding changes while the palette is open', async () => {
521523
// Custom bindings are stored verbatim (no platform conversion), so they assert
522-
// cleanly regardless of the test platform.
523-
setShortcut('app.quit', 0, '⌘7')
524+
// cleanly regardless of the test platform. (view.showHidden is a normal,
525+
// rebindable command — unlike the macOS-native ones.)
526+
setShortcut('view.showHidden', 0, '⌘7')
524527

525528
const target = document.createElement('div')
526529
mount(CommandPalette, {
@@ -529,13 +532,13 @@ describe('CommandPalette', () => {
529532
})
530533
await tick()
531534

532-
expect(shortcutTextsFor(target, 'app.quit')).toEqual(['⌘7'])
535+
expect(shortcutTextsFor(target, 'view.showHidden')).toEqual(['⌘7'])
533536

534537
// Simulate a rebind (the MCP / Settings path) while the palette stays open.
535-
setShortcut('app.quit', 0, '⌘8')
538+
setShortcut('view.showHidden', 0, '⌘8')
536539
flushSync()
537540

538-
expect(shortcutTextsFor(target, 'app.quit')).toEqual(['⌘8'])
541+
expect(shortcutTextsFor(target, 'view.showHidden')).toEqual(['⌘8'])
539542
})
540543

541544
it('does not throw if the previously focused element is no longer in the DOM', async () => {

apps/desktop/src/lib/commands/CLAUDE.md

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ interface Command {
2727
scope: CommandScope // hierarchical, display-only (does not enforce routing)
2828
showInPalette: boolean
2929
shortcuts: string[] // e.g. ['⌘Q'], ['Backspace', '⌘↑']
30+
nativeShortcut?: true // macOS owns behavior AND accelerator (PredefinedMenuItem); read-only in the editor
3031
description?: string
3132
}
3233

@@ -186,10 +187,16 @@ scope enforcement centralized would require the command module to know about all
186187
conflict detection in the shortcuts system and for display in settings; the actual dispatch is the responsibility of
187188
each component's keydown handler or the centralized dispatch map.
188189
189-
**Decision**: `showInPalette: false` for native macOS commands (quit, hide, hide others, show all). **Why**: These
190-
commands are handled by macOS via `PredefinedMenuItems` with native selectors (`terminate:`, `hide:`, etc.). The native
191-
menu accelerators handle the keyboard shortcuts directly. Including them in the JS shortcut dispatch map would cause
192-
double-execution: the native handler fires AND the JS handler fires.
190+
**Decision**: `showInPalette: false` AND `nativeShortcut: true` for native macOS commands (quit, hide, hide others, show
191+
all). **Why**: These commands are handled by macOS via `PredefinedMenuItems` with native selectors (`terminate:`,
192+
`hide:`, etc.). The native menu accelerators handle the keyboard shortcuts directly. Including them in the JS shortcut
193+
dispatch map would cause double-execution: the native handler fires AND the JS handler fires. AppKit owns BOTH the
194+
behavior and the accelerator, so Cmdr can neither rebind nor intercept them — editing them in Settings would be a double
195+
illusion (removal doesn't disable the OS accelerator; a new binding dispatches into a void). `nativeShortcut: true` (set
196+
on exactly the `NATIVE_SHORTCUT_COMMAND_IDS` exported from `command-registry.ts`) is the single source of truth that
197+
makes the shortcuts editor render these rows read-only and makes the store mutators refuse to write them.
198+
`command-handlers/types.ts` sources its Family-1 dispatch-exempt list from the same `NATIVE_SHORTCUT_COMMAND_IDS`, so
199+
the "AppKit owns this" fact lives in one place; `command-registry.test.ts` pins the flag set-equal to that list.
193200
194201
**Decision**: `isMacOS()` called at module load time for platform-specific command names and visibility. **Why**: Some
195202
commands only make sense on macOS (`Get info`, `Quick look`, `Show in Finder`). Rather than filtering at render time,

apps/desktop/src/lib/commands/command-registry.test.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { describe, it, expect } from 'vitest'
2-
import { commands, getPaletteCommands, updateLicenseCommandName } from './command-registry'
2+
import { commands, getPaletteCommands, updateLicenseCommandName, NATIVE_SHORTCUT_COMMAND_IDS } from './command-registry'
33
import { COMMAND_IDS, isCommandId, type CommandId } from './command-ids'
44
import type { CommandArgs, CommandDispatchArgs } from './types'
5+
import { DISPATCH_EXEMPT_IDS } from '../../routes/(main)/command-handlers/types'
56

67
/**
78
* The exact set of commands the palette shows the user. Pinned so a new
@@ -129,6 +130,45 @@ describe('command-registry id sync', () => {
129130
})
130131
})
131132

133+
describe('nativeShortcut flag', () => {
134+
it('the source list names the four macOS-native commands', () => {
135+
// Sanity guard against a silently-failing import: the set-equality below
136+
// would falsely pass if `NATIVE_SHORTCUT_COMMAND_IDS` resolved to undefined.
137+
expect([...NATIVE_SHORTCUT_COMMAND_IDS].sort()).toEqual(
138+
['app.hide', 'app.hideOthers', 'app.quit', 'app.showAll'].sort(),
139+
)
140+
})
141+
142+
it('marks exactly the commands in NATIVE_SHORTCUT_COMMAND_IDS', () => {
143+
// The `nativeShortcut` registry flag and `NATIVE_SHORTCUT_COMMAND_IDS` are
144+
// two views of the same set: macOS owns both the behavior AND the accelerator
145+
// (PredefinedMenuItems), so the editor must render them read-only and the
146+
// store must refuse to rebind them. Keying the flag off the list keeps the
147+
// sites from drifting.
148+
const flagged = new Set(commands.filter((c) => c.nativeShortcut).map((c) => c.id))
149+
const expected = new Set<string>(NATIVE_SHORTCUT_COMMAND_IDS)
150+
expect(flagged).toEqual(expected)
151+
})
152+
153+
it('every native command is also dispatch-exempt (Family 1)', () => {
154+
// The native commands are handler-less by design (AppKit runs them). The
155+
// dispatch-exempt list sources Family 1 from the same registry list, so this
156+
// also proves that single-source wiring round-trips.
157+
const exempt = new Set<string>(DISPATCH_EXEMPT_IDS)
158+
for (const id of NATIVE_SHORTCUT_COMMAND_IDS) {
159+
expect(exempt.has(id), `${id} must be dispatch-exempt`).toBe(true)
160+
}
161+
})
162+
163+
it('only ever uses `true` for the flag (never `false`/`undefined` noise)', () => {
164+
for (const cmd of commands) {
165+
if (cmd.nativeShortcut !== undefined) {
166+
expect(cmd.nativeShortcut).toBe(true)
167+
}
168+
}
169+
})
170+
})
171+
132172
describe('palette-visible command set', () => {
133173
it('shows exactly the pinned set of commands (no silent additions or removals)', () => {
134174
const paletteIds = getPaletteCommands().map((c) => c.id)

apps/desktop/src/lib/commands/command-registry.ts

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,17 @@
1111
import type { Command } from './types'
1212
import { isMacOS } from '$lib/shortcuts/key-capture'
1313

14+
/**
15+
* The macOS-native commands: AppKit `PredefinedMenuItem`s own BOTH the behavior
16+
* and the accelerator (`terminate:`, `hide:`, `hideOtherApplications:`,
17+
* `unhideAllApplications:`). Cmdr can neither rebind nor intercept them, so the
18+
* shortcuts editor renders them read-only and the store refuses to customize
19+
* them. Single source of truth: the registry entries below carry
20+
* `nativeShortcut: true` for exactly these ids (pinned by `command-registry.test.ts`),
21+
* and `command-handlers/types.ts` sources its Family-1 dispatch-exempt list from here.
22+
*/
23+
export const NATIVE_SHORTCUT_COMMAND_IDS = ['app.quit', 'app.hide', 'app.hideOthers', 'app.showAll'] as const
24+
1425
// `Command.id` is the `CommandId` union derived from `COMMAND_IDS` in
1526
// `command-ids.ts`. Adding an entry here whose id isn't in that tuple is a
1627
// compile error; a tuple id with no entry here is caught by the set-equality
@@ -23,11 +34,19 @@ export const commands: Command[] = [
2334
// ============================================================================
2435
// Native-only: handled by PredefinedMenuItems via macOS selectors (hide:, hideOtherApplications:,
2536
// unhideAllApplications:, terminate:). showInPalette: false keeps them out of the JS shortcut
26-
// dispatch map; the native menu accelerators handle the keyboard shortcuts directly.
27-
{ id: 'app.quit', name: 'Quit Cmdr', scope: 'App', showInPalette: false, shortcuts: ['⌘Q'] },
28-
{ id: 'app.hide', name: 'Hide Cmdr', scope: 'App', showInPalette: false, shortcuts: ['⌘H'] },
29-
{ id: 'app.hideOthers', name: 'Hide others', scope: 'App', showInPalette: false, shortcuts: ['⌥⌘H'] },
30-
{ id: 'app.showAll', name: 'Show all', scope: 'App', showInPalette: false, shortcuts: [] },
37+
// dispatch map; the native menu accelerators handle the keyboard shortcuts directly. `nativeShortcut`
38+
// makes the editor read-only and the store refuse to rebind them (NATIVE_SHORTCUT_COMMAND_IDS above).
39+
{ id: 'app.quit', name: 'Quit Cmdr', scope: 'App', showInPalette: false, shortcuts: ['⌘Q'], nativeShortcut: true },
40+
{ id: 'app.hide', name: 'Hide Cmdr', scope: 'App', showInPalette: false, shortcuts: ['⌘H'], nativeShortcut: true },
41+
{
42+
id: 'app.hideOthers',
43+
name: 'Hide others',
44+
scope: 'App',
45+
showInPalette: false,
46+
shortcuts: ['⌥⌘H'],
47+
nativeShortcut: true,
48+
},
49+
{ id: 'app.showAll', name: 'Show all', scope: 'App', showInPalette: false, shortcuts: [], nativeShortcut: true },
3150
{ id: 'app.about', name: 'About Cmdr', scope: 'App', showInPalette: true, shortcuts: [] },
3251
{ id: 'app.licenseKey', name: 'See license details', scope: 'App', showInPalette: true, shortcuts: [] },
3352
{

apps/desktop/src/lib/commands/types.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,14 @@ export interface Command {
132132
showInPalette: boolean
133133
/** Keyboard shortcuts (like ['⌘⇧P', 'F1']) */
134134
shortcuts: string[]
135+
/**
136+
* macOS owns this command's behavior AND its accelerator via a
137+
* `PredefinedMenuItem` (`terminate:`, `hide:`, `hideOtherApplications:`,
138+
* `unhideAllApplications:`). Cmdr can neither rebind nor intercept it, so the
139+
* shortcuts editor renders it read-only and the store refuses to customize it.
140+
* Set only on the four `NATIVE_SHORTCUT_COMMAND_IDS` (Family 1 dispatch-exempt).
141+
*/
142+
nativeShortcut?: true
135143
/** Optional description for long-form help */
136144
description?: string
137145
/**

apps/desktop/src/lib/settings/sections/CLAUDE.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ Parents: [`../CLAUDE.md`](../CLAUDE.md) (registry, store, applier, search) and
3535
| `license-section-utils.ts` | Pure label/status formatters extracted from `LicenseSection` for testability |
3636
| `ram-gauge-utils.ts` | Pure stacked-bar segment math for `AiLocalSection`'s memory gauge (used → projected → free, plus warning thresholds) |
3737
| `keyboard-shortcuts-grouping.ts` | Pure scope→group logic for `KeyboardShortcutsSection` (one titled group per `CommandScope`, fixed order). Tested by the set-equality regression guard |
38+
| `keyboard-shortcuts-banner.ts` | Pure conflict-banner classification for `KeyboardShortcutsSection` (`classifyConflict` → native vs normal; `reservedByMacOsMessage`). Native conflicts win even in a mixed set. Unit-tested |
3839

3940
Each section ships with an `*.a11y.test.ts` (axe-core tier-3). `McpServerSection`, `UpdatesSection`, `SearchSection`,
4041
`FileSystemWatchingSection`, and `KeyboardShortcutsSection` also have functional `*.test.ts` / `*.svelte.test.ts` files;
@@ -142,6 +143,33 @@ after that lands as an overwrite of that pill instead of an append. It needs a p
142143
command, the result is visible immediately, and any fix (re-deriving the slot on `shortcutChangeCounter` bumps) costs
143144
more state than the race is worth. Revisit only if it shows up in practice.
144145

146+
## macOS-native rows are read-only
147+
148+
The four `nativeShortcut` commands (`app.quit`/`hide`/`hideOthers`/`showAll`) render read-only: their combos show as
149+
plain `.shortcut-pill.static` spans (no click-to-edit), with no `+` add, no `×` remove, no reset button, and never the
150+
add slot. Each native row also carries a small "macOS" badge (`.macos-badge`) with a tooltip: "macOS handles this
151+
shortcut. Cmdr can't change it." (`Show all` has no default binding, so it renders its `(none)` unframed plus the
152+
badge.) The branch is keyed off `isNativeShortcutCommand(command.id)` from `$lib/shortcuts`. This is honest: AppKit owns
153+
both the behavior and the accelerator (see `lib/shortcuts/CLAUDE.md` § "macOS-native commands are not customizable"), so
154+
an editable control here would be a double illusion. The store also refuses these writes as defense in depth, so the UI
155+
and the store agree.
156+
157+
## Conflict banner: native conflicts are honest (reserved by macOS), others are resolvable
158+
159+
`handleKeyCapture` classifies the captured combo's conflicts via the pure `classifyConflict`
160+
(`keyboard-shortcuts-banner.ts`):
161+
162+
- If the conflict set includes a `nativeShortcut` command (even in a MIXED set with a normal command — the native wins,
163+
because the combo is unusable regardless), the banner reads
164+
`⌘H is reserved by macOS (Hide Cmdr) and won't reach Cmdr. Pick a different combo.` (`reservedByMacOsMessage`) and
165+
offers ONLY Cancel. No "Remove from other" (removing Cmdr's binding doesn't free the OS accelerator) and no "Keep
166+
both" (the user's binding would never fire) — both would be a lie.
167+
- A purely non-native conflict keeps the resolvable banner (Remove-from-other / Keep-both / Cancel).
168+
169+
`conflictWarning` carries `{ shortcut, conflict: ConflictKind }`; the template branches on `conflict.kind`. The
170+
classification logic is extracted to the pure `keyboard-shortcuts-banner.ts` (unit-tested) to keep the section component
171+
lean.
172+
145173
## Conflict banner: the editing pill reads as a pending decision
146174

147175
When a captured combo conflicts, `handleKeyCapture` sets `conflictWarning` and returns without saving (the banner offers

0 commit comments

Comments
 (0)