Shortcuts: serialized saves, typing-safe bare-key dispatch, and macOS system-shortcut warnings

vdavid · vdavid · commit a412e599cf6f · 2026-06-10T13:32:10.000+02:00
Three hardening fixes for the shortcuts subsystem: - **Saves can't interleave.** Every store mutator fires `void saveToStore()`; two rapid mutations used to run their reconcile/delete/set/save loops concurrently over the shared store. Saves now chain (`saveChain`), so each starts after the previous finished and the last one always persists the final state. Covered by a rapid-mutations round-trip test. - **Typing wins in text inputs.** A bare-key (or shift-only) Tier 1 binding — Tab → switch pane being the built-in case — used to fire mid-typing in any in-pane text input that forgot to `stopPropagation` (a per-component convention; only `NetworkLoginForm` did it). `handleGlobalKeyDown` now bails centrally when focus is in a text-editing element and the combo `isTypingKeyCombo` (new pure helper in `key-capture.ts`: no ⌘/⌃/⌥, not an F-key or Escape). ⌘-combos and F-keys stay live in inputs. Unit-tested red-first. - **Capturing a macOS-owned combo warns honestly.** Binding ⌘Space (Spotlight), ⌃↑ (Mission Control), ⌘⇧4 (screenshots), and ~12 other system defaults used to save silently and then never fire — the OS intercepts first. The capture banner now shows a soft warning (`classifySystemShortcut` + `systemShortcutMessage`, checked only when there's no in-app conflict) with "Use anyway" (the user may have freed the combo in System Settings) and Cancel. macOS-display-form keys mean the list inherently can't misfire on other platforms. - The banner's `ConflictKind` union growing a `command`-less variant forced honest narrowing: the template snapshots `conflictWarning.conflict` into an `{@const}` and branches exhaustively; the unit tests assert via `toMatchObject`.
diff --git a/apps/desktop/src/lib/settings/sections/CLAUDE.md b/apps/desktop/src/lib/settings/sections/CLAUDE.md
@@ -35,7 +35,7 @@ Parents: [`../CLAUDE.md`](../CLAUDE.md) (registry, store, applier, search) and
 | `license-section-utils.ts`         | Pure label/status formatters extracted from `LicenseSection` for testability                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               |
 | `ram-gauge-utils.ts`               | Pure stacked-bar segment math for `AiLocalSection`'s memory gauge (used → projected → free, plus warning thresholds)                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
 | `keyboard-shortcuts-grouping.ts`   | Pure scope→group logic for `KeyboardShortcutsSection` (one titled group per `CommandScope`, fixed order). Tested by the set-equality regression guard                                                                                                                                                                                                                                                                                                                                                                                                                                      |
-| `keyboard-shortcuts-banner.ts`     | Pure conflict-banner classification for `KeyboardShortcutsSection` (`classifyConflict` → native vs fixed vs normal; `reservedByMacOsMessage` / `fixedKeyMessage`). Native > fixed > normal in mixed sets. Unit-tested                                                                                                                                                                                                                                                                                                                                                                      |
+| `keyboard-shortcuts-banner.ts`     | Pure conflict-banner classification for `KeyboardShortcutsSection` (`classifyConflict` → native vs fixed vs normal, `classifySystemShortcut` → soft macOS-system warning with "Use anyway"; `reservedByMacOsMessage` / `fixedKeyMessage` / `systemShortcutMessage`). Native > fixed > normal in mixed sets; system checked only when no in-app conflict. Unit-tested                                                                                                                                                                                                                       |
 
 Each section ships with an `*.a11y.test.ts` (axe-core tier-3). `McpServerSection`, `UpdatesSection`, `SearchSection`,
 `FileSystemWatchingSection`, and `KeyboardShortcutsSection` also have functional `*.test.ts` / `*.svelte.test.ts` files;
diff --git a/apps/desktop/src/lib/settings/sections/KeyboardShortcutsSection.svelte b/apps/desktop/src/lib/settings/sections/KeyboardShortcutsSection.svelte
@@ -28,7 +28,14 @@
     import { confirmDialog } from '$lib/utils/confirm-dialog'
     import GlobalShortcutRow from '$lib/downloads/GlobalShortcutRow.svelte'
     import { groupCommandsByScope } from './keyboard-shortcuts-grouping'
-    import { classifyConflict, fixedKeyMessage, reservedByMacOsMessage, type ConflictKind } from './keyboard-shortcuts-banner'
+    import {
+        classifyConflict,
+        classifySystemShortcut,
+        fixedKeyMessage,
+        reservedByMacOsMessage,
+        systemShortcutMessage,
+        type ConflictKind,
+    } from './keyboard-shortcuts-banner'
     import { shortcutAnchorId } from '$lib/settings/settings-window'
     import {
         getPendingShortcutHighlight,
@@ -194,7 +201,9 @@
         const command = commands.find((c) => c.id === currentEditCommandId)
         if (command) {
             const conflicts = findConflictsForShortcut(combo, command.scope, command.id)
-            const conflict = classifyConflict(conflicts)
+            // In-app conflicts take priority; with none, still warn when macOS
+            // itself usually owns the combo (Spotlight, Mission Control, …).
+            const conflict = classifyConflict(conflicts) ?? classifySystemShortcut(combo)
             if (conflict) {
                 conflictWarning = { shortcut: combo, conflict }
                 return // Don't auto-save, wait for user decision
@@ -492,30 +501,42 @@
     </div>
 
     {#if conflictWarning}
+        <!-- `{@const}` snapshot so the kind checks narrow the union (a re-read of the
+             `$state` field doesn't narrow, and `SystemConflict` has no `command`). -->
+        {@const conflict = conflictWarning.conflict}
         <div class="conflict-warning">
             <span class="warning-icon">⚠️</span>
-            {#if conflictWarning.conflict.kind === 'native'}
+            {#if conflict.kind === 'native'}
                 <!-- macOS owns this combo: it can never reach Cmdr, so we don't offer
                      "Remove from other" or "Keep both" (both would be a lie). -->
                 <span class="warning-text">
-                    {reservedByMacOsMessage(conflictWarning.shortcut, conflictWarning.conflict.command)}
+                    {reservedByMacOsMessage(conflictWarning.shortcut, conflict.command)}
+                </span>
+                <div class="warning-actions">
+                    <Button variant="secondary" size="mini" onclick={cancelEdit}>Cancel</Button>
+                </div>
+            {:else if conflict.kind === 'system'}
+                <!-- macOS usually intercepts this combo before Cmdr sees it, but the user
+                     may have disabled the system shortcut — warn and let them decide. -->
+                <span class="warning-text">
+                    {systemShortcutMessage(conflictWarning.shortcut, conflict.label)}
                 </span>
                 <div class="warning-actions">
+                    <Button variant="secondary" size="mini" onclick={handleKeepBoth}>Use anyway</Button>
                     <Button variant="secondary" size="mini" onclick={cancelEdit}>Cancel</Button>
                 </div>
-            {:else if conflictWarning.conflict.kind === 'fixed'}
+            {:else if conflict.kind === 'fixed'}
                 <!-- The combo is hardcoded in a component: it can't be removed there and
                      would keep firing, so we don't offer "Remove from other" or "Keep both". -->
                 <span class="warning-text">
-                    {fixedKeyMessage(conflictWarning.shortcut, conflictWarning.conflict.command)}
+                    {fixedKeyMessage(conflictWarning.shortcut, conflict.command)}
                 </span>
                 <div class="warning-actions">
                     <Button variant="secondary" size="mini" onclick={cancelEdit}>Cancel</Button>
                 </div>
-            {:else}
+            {:else if conflict.kind === 'normal'}
                 <span class="warning-text">
-                    <strong>{conflictWarning.shortcut}</strong> is already bound to "{conflictWarning.conflict.command
-                        .name}"
+                    <strong>{conflictWarning.shortcut}</strong> is already bound to "{conflict.command.name}"
                 </span>
                 <div class="warning-actions">
                     <Button variant="secondary" size="mini" onclick={handleRemoveFromOther}>Remove from other</Button>
diff --git a/apps/desktop/src/lib/settings/sections/keyboard-shortcuts-banner.test.ts b/apps/desktop/src/lib/settings/sections/keyboard-shortcuts-banner.test.ts
@@ -1,6 +1,12 @@
 import { describe, it, expect } from 'vitest'
 import type { Command } from '$lib/commands/types'
-import { classifyConflict, fixedKeyMessage, reservedByMacOsMessage } from './keyboard-shortcuts-banner'
+import {
+  classifyConflict,
+  classifySystemShortcut,
+  fixedKeyMessage,
+  reservedByMacOsMessage,
+  systemShortcutMessage,
+} from './keyboard-shortcuts-banner'
 
 function cmd(partial: { id: string; name?: string; nativeShortcut?: true; fixedKey?: true }): Command {
   const command: Command = {
@@ -22,14 +28,12 @@ describe('classifyConflict', () => {
 
   it('classifies a purely non-native conflict as normal (first command)', () => {
     const result = classifyConflict([cmd({ id: 'file.copy' }), cmd({ id: 'file.move' })])
-    expect(result?.kind).toBe('normal')
-    expect(result?.command.id).toBe('file.copy')
+    expect(result).toMatchObject({ kind: 'normal', command: { id: 'file.copy' } })
   })
 
   it('classifies a native conflict as native', () => {
     const result = classifyConflict([cmd({ id: 'app.hide', name: 'Hide Cmdr', nativeShortcut: true })])
-    expect(result?.kind).toBe('native')
-    expect(result?.command.id).toBe('app.hide')
+    expect(result).toMatchObject({ kind: 'native', command: { id: 'app.hide' } })
   })
 
   it('lets the native command win a mixed set (native + normal)', () => {
@@ -39,8 +43,7 @@ describe('classifyConflict', () => {
       cmd({ id: 'file.copy' }),
       cmd({ id: 'app.hide', name: 'Hide Cmdr', nativeShortcut: true }),
     ])
-    expect(result?.kind).toBe('native')
-    expect(result?.command.id).toBe('app.hide')
+    expect(result).toMatchObject({ kind: 'native', command: { id: 'app.hide' } })
   })
 })
 
@@ -54,8 +57,7 @@ describe('reservedByMacOsMessage', () => {
 describe('classifyConflict (fixed-key)', () => {
   it('classifies a fixed-key conflict as fixed', () => {
     const result = classifyConflict([cmd({ id: 'nav.up', name: 'Select previous file', fixedKey: true })])
-    expect(result?.kind).toBe('fixed')
-    expect(result?.command.id).toBe('nav.up')
+    expect(result).toMatchObject({ kind: 'fixed', command: { id: 'nav.up' } })
   })
 
   it('lets the fixed command win a mixed set (fixed + normal)', () => {
@@ -65,8 +67,7 @@ describe('classifyConflict (fixed-key)', () => {
       cmd({ id: 'file.copy' }),
       cmd({ id: 'nav.up', name: 'Select previous file', fixedKey: true }),
     ])
-    expect(result?.kind).toBe('fixed')
-    expect(result?.command.id).toBe('nav.up')
+    expect(result).toMatchObject({ kind: 'fixed', command: { id: 'nav.up' } })
   })
 
   it('lets a native conflict outrank a fixed one (both in the set)', () => {
@@ -86,3 +87,26 @@ describe('fixedKeyMessage', () => {
     )
   })
 })
+
+describe('classifySystemShortcut', () => {
+  it('flags well-known macOS system combos with their feature label', () => {
+    expect(classifySystemShortcut('⌘Space')).toEqual({ kind: 'system', label: 'Spotlight' })
+    expect(classifySystemShortcut('⌃↑')).toEqual({ kind: 'system', label: 'Mission Control' })
+    expect(classifySystemShortcut('⌘⇧4')).toEqual({ kind: 'system', label: 'screenshots' })
+  })
+
+  it('returns null for ordinary combos', () => {
+    expect(classifySystemShortcut('⌘⇧P')).toBeNull()
+    expect(classifySystemShortcut('F5')).toBeNull()
+    expect(classifySystemShortcut('Ctrl+Space')).toBeNull() // non-mac display form never matches
+  })
+})
+
+describe('systemShortcutMessage', () => {
+  it('names the combo and the owning feature and points at System Settings', () => {
+    const message = systemShortcutMessage('⌘Space', 'Spotlight')
+    expect(message).toContain('⌘Space')
+    expect(message).toContain('Spotlight')
+    expect(message).toContain('System Settings')
+  })
+})
diff --git a/apps/desktop/src/lib/settings/sections/keyboard-shortcuts-banner.ts b/apps/desktop/src/lib/settings/sections/keyboard-shortcuts-banner.ts
@@ -37,7 +37,7 @@ export interface NormalConflict {
   command: Command
 }
 
-export type ConflictKind = NativeConflict | FixedConflict | NormalConflict
+export type ConflictKind = NativeConflict | FixedConflict | SystemConflict | NormalConflict
 
 /**
  * Classify a non-empty conflict set. A native command anywhere in the set makes
@@ -72,3 +72,53 @@ export function reservedByMacOsMessage(combo: string, nativeCommand: Command): s
 export function fixedKeyMessage(combo: string, fixedCommand: Command): string {
   return `${combo} is a fixed key in Cmdr (${fixedCommand.name}) and can't be reassigned. Pick a different combo.`
 }
+
+/** A system conflict: macOS itself usually owns the combo (Spotlight, Mission Control, …). */
+export interface SystemConflict {
+  kind: 'system'
+  /** The macOS feature that owns the combo (drives the banner copy). */
+  label: string
+}
+
+/**
+ * Default macOS system-wide shortcuts (display form, as `formatKeyCombo`
+ * produces them on macOS). Best-effort: users can disable or remap these in
+ * System Settings, so the banner warns and offers "Use anyway" instead of
+ * refusing. Keys only ever match on macOS — other platforms capture
+ * `Ctrl+…`-style strings.
+ */
+const macSystemShortcutToFeatureMap: Record<string, string> = {
+  '⌘Space': 'Spotlight',
+  '⌥⌘Space': 'Finder search window',
+  '⌃⌘Space': 'Character Viewer',
+  '⌃Space': 'input source switching',
+  '⌘Tab': 'the app switcher',
+  '⌃↑': 'Mission Control',
+  '⌃↓': 'App windows',
+  '⌃←': 'Spaces',
+  '⌃→': 'Spaces',
+  '⌘⇧3': 'screenshots',
+  '⌘⇧4': 'screenshots',
+  '⌘⇧5': 'screen recording',
+  '⌘⇧Q': 'logging out',
+  '⌃⌘Q': 'locking the screen',
+  '⌥⌘⎋': 'Force Quit',
+}
+
+/**
+ * Classify a combo against the macOS system-shortcut list. Checked only when
+ * there's no in-app conflict (those banners take priority); a match produces a
+ * soft warning, not a refusal.
+ */
+export function classifySystemShortcut(combo: string): SystemConflict | null {
+  const label = macSystemShortcutToFeatureMap[combo]
+  return label ? { kind: 'system', label } : null
+}
+
+/**
+ * The banner copy for a system-owned combo, like
+ * `⌘Space is usually taken by macOS (Spotlight), so it may never reach Cmdr.`
+ */
+export function systemShortcutMessage(combo: string, label: string): string {
+  return `${combo} is usually taken by macOS (${label}), so it may never reach Cmdr. You can free it up in System Settings > Keyboard.`
+}
diff --git a/apps/desktop/src/lib/shortcuts/CLAUDE.md b/apps/desktop/src/lib/shortcuts/CLAUDE.md
@@ -28,6 +28,8 @@ shortcuts:
 - Delta-only storage: only customizations, not defaults
 - Empty array means "all shortcuts removed"
 - Missing command means "use defaults from registry"
+- Saves are serialized: every mutator fires `void saveToStore()`, and the save chains onto the previous one
+  (`saveChain`) so two rapid mutations can't interleave their reconcile/delete/set/save loops over the shared store.
 - `saveToStore` reconciles disk against the in-memory map on every write: it deletes any `shortcut:*` key whose command
   no longer has a map entry, then writes the current entries. So when `resetShortcut` / `cleanupIfMatchesDefaults` drops
   an entry (e.g. a custom that's been edited back to defaults, or `app.showAll`'s `[]` default after removing an added
@@ -176,6 +178,11 @@ command palette and MCP events. Rebuilds automatically when custom shortcuts cha
 Tier 2 commands (arrows, Space, Enter, Backspace, etc.) are not in the dispatch map. Unmatched keypresses propagate
 normally to component-level handlers in DualPaneExplorer and FilePane.
 
+Typing wins in text inputs: before the lookup, `handleGlobalKeyDown` bails when focus is in a text-editing element and
+the combo `isTypingKeyCombo` (no ⌘/⌃/⌥, not an F-key or Escape — shift-only counts as typing). Without this, a bare-key
+Tier 1 binding (Tab → switch pane) fires mid-typing in any in-pane text input that forgets to `stopPropagation`. The
+guard is central so new inputs are protected by default; `NetworkLoginForm`'s own Tab shielding remains as before.
+
 ## Key decisions
 
 ### Why platform-specific storage?
diff --git a/apps/desktop/src/lib/shortcuts/key-capture.test.ts b/apps/desktop/src/lib/shortcuts/key-capture.test.ts
@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, afterEach } from 'vitest'
-import { formatKeyCombo, toPlatformShortcut } from './key-capture'
+import { formatKeyCombo, toPlatformShortcut, isTypingKeyCombo } from './key-capture'
 
 // Mock navigator to control isMacOS() behavior
 const navigatorSpy = vi.spyOn(globalThis, 'navigator', 'get')
@@ -143,3 +143,35 @@ describe('formatKeyCombo', () => {
     expect(result).toBe('⌘⌥E')
   })
 })
+
+describe('isTypingKeyCombo', () => {
+  it('treats bare keys as typing (Tab, letters, Space, Enter)', () => {
+    expect(isTypingKeyCombo('Tab')).toBe(true)
+    expect(isTypingKeyCombo('A')).toBe(true)
+    expect(isTypingKeyCombo('Space')).toBe(true)
+    expect(isTypingKeyCombo('↩')).toBe(true)
+  })
+
+  it('treats shift-only combos as typing (⇧Tab reverse-tab, ⇧A capital letter)', () => {
+    expect(isTypingKeyCombo('⇧Tab')).toBe(true)
+    expect(isTypingKeyCombo('⇧A')).toBe(true)
+    expect(isTypingKeyCombo('Shift+Tab')).toBe(true)
+  })
+
+  it('keeps command-modifier combos live (⌘, ⌃, ⌥, Ctrl, Alt, Super)', () => {
+    expect(isTypingKeyCombo('⌘C')).toBe(false)
+    expect(isTypingKeyCombo('⌃X')).toBe(false)
+    expect(isTypingKeyCombo('⌥↓')).toBe(false)
+    expect(isTypingKeyCombo('Ctrl+C')).toBe(false)
+    expect(isTypingKeyCombo('Alt+Tab')).toBe(false)
+    expect(isTypingKeyCombo('Super+Space')).toBe(false)
+  })
+
+  it('keeps F-keys and Escape live (never typing)', () => {
+    expect(isTypingKeyCombo('F5')).toBe(false)
+    expect(isTypingKeyCombo('F12')).toBe(false)
+    expect(isTypingKeyCombo('⇧F6')).toBe(false)
+    expect(isTypingKeyCombo('⎋')).toBe(false)
+    expect(isTypingKeyCombo('Esc')).toBe(false)
+  })
+})
diff --git a/apps/desktop/src/lib/shortcuts/key-capture.ts b/apps/desktop/src/lib/shortcuts/key-capture.ts
@@ -197,3 +197,22 @@ export function matchesShortcut(event: KeyboardEvent, shortcut: string): boolean
 export function isCompleteCombo(event: KeyboardEvent): boolean {
   return !isModifierKey(event.key)
 }
+
+/** Modifier tokens that signal command intent (Shift alone doesn't — it types capitals and reverse-tabs). */
+const commandModifierTokens = ['⌘', '⌃', '⌥', 'Ctrl', 'Alt', 'Super']
+
+/**
+ * True when the combo is something a user types in a text field rather than a
+ * command: no command modifier (⌘/⌃/⌥ or Ctrl/Alt/Super — Shift alone still
+ * counts as typing), and not an F-key or Escape (which never produce text).
+ * The centralized dispatch uses this to let typing win in focused text inputs:
+ * a bare-key Tier 1 binding (Tab → switch pane) must not fire mid-typing.
+ */
+export function isTypingKeyCombo(shortcut: string): boolean {
+  if (commandModifierTokens.some((token) => shortcut.includes(token))) return false
+  // Strip the shift prefix (both display forms) to inspect the base key.
+  const base = shortcut.replace(/^⇧/, '').replace(/^Shift\+/, '')
+  if (/^F\d+$/.test(base)) return false
+  if (base === '⎋' || base === 'Esc' || base === 'Escape') return false
+  return true
+}
diff --git a/apps/desktop/src/lib/shortcuts/shortcuts-store.test.ts b/apps/desktop/src/lib/shortcuts/shortcuts-store.test.ts
@@ -541,3 +541,21 @@ describe('shortcuts-store refuses to customize fixed-key commands', () => {
     expect(changedEmits()).toHaveLength(0)
   })
 })
+
+describe('shortcuts-store save serialization', () => {
+  it('rapid back-to-back mutations persist the final state of every touched command', async () => {
+    const store = await loadStore()
+    await store.initializeShortcuts()
+
+    // Three mutations in the same tick: their fire-and-forget saves chain
+    // instead of interleaving reconcile/set/save loops over the shared store.
+    store.addShortcut('app.about', 'F9')
+    store.setShortcut('app.about', 0, 'F10')
+    store.addShortcut('file.copy', '⌘5')
+    await flushSave()
+    await flushSave()
+
+    expect(disk.get('shortcut:app.about')).toEqual(['F10'])
+    expect(store.getEffectiveShortcuts('file.copy')).toContain('⌘5')
+  })
+})
diff --git a/apps/desktop/src/lib/shortcuts/shortcuts-store.ts b/apps/desktop/src/lib/shortcuts/shortcuts-store.ts
diff --git a/apps/desktop/src/routes/(main)/+page.svelte b/apps/desktop/src/routes/(main)/+page.svelte
