Skip to content

Commit 2247dac

Browse files
committed
Bugfix: Detect conflicts for compound-scope shortcuts
- `scope-hierarchy.ts` only knew simple scopes, so `getActiveScopes('Main window/File list')` (and every other compound scope) returned `[]`, and `scopesOverlap` short-circuits to false on an empty chain. The 48 compound-scope commands could never conflict with anything, including each other: binding the same combo to two File-list commands showed no warning. - Unify on one scope vocabulary: `scope-hierarchy.ts` now re-exports `CommandScope` from `commands/types.ts`, dropping the legacy simple-scope-only type (`File list`, `Navigation`, `Selection`, `Edit`, `View`, `Help`, `Settings window` — none of which the registry uses). - Give compound scopes correct ancestry chains: `Brief mode` / `Full mode` sit under `Main window/File list` (the list renders in both modes, so a mode key collides with a File-list key), while Brief and Full stay siblings (the default `←`/`→` pairs must not conflict). `Network` / `Share browser` / `Volume chooser` are siblings of the file list (a pane shows one INSTEAD of the list). - Drop the now-redundant `as CommandScope` casts in `conflict-detector.ts` and `KeyboardShortcutsSection.svelte`. - Add a regression test over the full registry: default bindings produce zero conflicts.
1 parent 6c21fd1 commit 2247dac

8 files changed

Lines changed: 221 additions & 67 deletions

File tree

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,9 @@ in `types.ts` bridges this gap. Don't expose raw pixel values in the UI.
291291

292292
### Shortcuts conflict only when scopes overlap
293293

294-
`⌘N` in "File list" scope and `⌘N` in "Settings window" scope do NOT conflict because their scope hierarchies don't
295-
overlap. Only warn when the same key combo is used in overlapping scopes (e.g., "File list" and "Main window").
294+
`⌘N` in `Main window/File list` and `⌘N` in `About window` do NOT conflict because their scope chains don't overlap.
295+
Only warn when the same key combo is used in overlapping scopes (for example, `Main window/File list` and
296+
`Main window`). See `lib/shortcuts/CLAUDE.md` § "Scope hierarchy" for the full chain model.
296297

297298
### No undo
298299

apps/desktop/src/lib/settings/sections/KeyboardShortcutsSection.svelte

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
findConflictsForShortcut,
2424
getConflictingCommandIds,
2525
getConflictCount,
26-
type CommandScope,
2726
} from '$lib/shortcuts'
2827
import { confirmDialog } from '$lib/utils/confirm-dialog'
2928
import GlobalShortcutRow from '$lib/downloads/GlobalShortcutRow.svelte'
@@ -186,7 +185,7 @@
186185
const currentEditCommandId = editingShortcut.commandId
187186
const command = commands.find((c) => c.id === currentEditCommandId)
188187
if (command) {
189-
const conflicts = findConflictsForShortcut(combo, command.scope as CommandScope, command.id)
188+
const conflicts = findConflictsForShortcut(combo, command.scope, command.id)
190189
if (conflicts.length > 0) {
191190
conflictWarning = { shortcut: combo, conflictingCommand: conflicts[0] }
192191
return // Don't auto-save, wait for user decision

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

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,17 +66,27 @@ handler) can't drift. The settings side scrolls the row into its nested list and
6666

6767
### Scope hierarchy (`scope-hierarchy.ts`)
6868

69-
Defines which shortcuts are active in each context:
70-
71-
- `App` scope: global, always active
72-
- `Main window` → inherits `App`
73-
- `File list` → inherits `Main window` → inherits `App`
74-
75-
When "File list" is focused, shortcuts from all three scopes can trigger.
76-
77-
Note: the command registry uses compound scopes like `'Main window/File list'`, `'Main window/Brief mode'`,
78-
`'Main window/Full mode'`, `'Main window/Network'` that are distinct from the simple scope hierarchy defined in
79-
`scope-hierarchy.ts`. These compound scopes represent specific UI contexts within the main window.
69+
`CommandScope` is the single scope vocabulary — `scope-hierarchy.ts` re-exports it from `../commands/types.ts`, so the
70+
registry's `scope` strings and the hierarchy keys are the same type. `scopeHierarchy` holds each scope's ancestry chain
71+
(most specific first); two scopes "overlap" (and so can conflict) when one chain contains the other.
72+
73+
The chains mirror what renders together in the app:
74+
75+
- `App` → global, always active.
76+
- `Main window` → inherits `App`.
77+
- `Main window/File list` → inherits `Main window``App`.
78+
- `Main window/Brief mode` and `Main window/Full mode` → sit UNDER `Main window/File list` (→ `Main window``App`).
79+
The file list renders in both view modes, so a mode-scoped key genuinely collides with a File-list key. Brief and Full
80+
stay siblings (neither chain contains the other), so they don't conflict with each other — the registry binds ``/``
81+
in both on purpose, and the modes never coexist.
82+
- `Main window/Network`, `Main window/Share browser`, `Main window/Volume chooser` → siblings of `Main window/File list`
83+
(under `Main window``App`, but not under the file list). A pane shows one of them INSTEAD of the file list, so
84+
their keys don't collide with File-list keys.
85+
- `Command palette` → inherits `Main window``App` (it overlays the main window).
86+
- `About window` and `Onboarding` → inherit `App` only (standalone/modal contexts).
87+
88+
`getActiveScopes(unknown)` returns `[]`, and `scopesOverlap` treats an empty chain as non-overlapping, so a typo'd scope
89+
silently can't conflict rather than throwing.
8090

8191
### Conflict detection (`conflict-detector.ts`)
8292

@@ -85,7 +95,9 @@ Two commands conflict if:
8595
1. They share the same key combo, AND
8696
2. Their scopes overlap (via hierarchy)
8797

88-
Example: `⌘N` in "File list" and `⌘N` in "Main window" conflict because "File list" inherits from "Main window".
98+
Example: `⌘N` in `Main window/File list` and `⌘N` in `Main window` conflict because the File-list chain contains
99+
`Main window`. Two `Main window/File list` commands sharing a combo also conflict (same scope). `` in
100+
`Main window/Brief mode` and `` in `Main window/Full mode` do NOT, because Brief and Full are siblings.
89101

90102
### Key capture (`key-capture.ts`)
91103

@@ -221,8 +233,8 @@ are part of the app's behavior, not user data.
221233

222234
### Scope overlap is transitive
223235

224-
If "File list" inherits "Main window" and "Main window" inherits "App", then "File list" also inherits "App". The
225-
`getActiveScopes()` function returns all ancestors, not just the immediate parent.
236+
If `Main window/File list` inherits `Main window` and `Main window` inherits `App`, then `Main window/File list` also
237+
inherits `App`. `getActiveScopes()` returns the full ancestry chain, not just the immediate parent.
226238

227239
### No chorded shortcuts
228240

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* Regression test for conflict detection over the REAL command registry with
3+
* default bindings (no mocks). Guards two things at once:
4+
*
5+
* 1. Compound-scope commands (`Main window/File list`, `Main window/Brief mode`,
6+
* …) participate in conflict detection. Before the scope hierarchy learned the
7+
* compound chains, `getActiveScopes` returned `[]` for these and the 48
8+
* compound-scope commands could never conflict with anything.
9+
* 2. The shipped default bindings don't introduce conflicts. If a future default
10+
* edit binds the same combo to two overlapping-scope commands, this test
11+
* fails and surfaces exactly which combo.
12+
*/
13+
14+
import { describe, it, expect } from 'vitest'
15+
import { getAllConflicts } from './conflict-detector'
16+
17+
describe('conflict-detector over the real registry (default bindings)', () => {
18+
it('reports no conflicts for the shipped defaults', () => {
19+
const conflicts = getAllConflicts()
20+
// Surface the offending combos in the failure message rather than a bare count.
21+
const summary = conflicts.map((c) => `${c.shortcut}: ${c.commandIds.join(', ')}`)
22+
expect(summary).toEqual([])
23+
})
24+
})

apps/desktop/src/lib/shortcuts/conflict-detector.test.ts

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,9 @@ describe('conflict-detector', () => {
7777
it('returns commands sharing the same shortcut in overlapping scopes', () => {
7878
setCommands(
7979
makeCommand({ id: 'a', shortcuts: ['⌘N'], scope: 'Main window' }),
80-
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'File list' }),
80+
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'Main window/File list' }),
8181
)
82-
const conflicts = findConflictsForShortcut('⌘N', 'File list')
82+
const conflicts = findConflictsForShortcut('⌘N', 'Main window/File list')
8383
const ids = conflicts.map((c) => c.id)
8484
expect(ids).toContain('a')
8585
expect(ids).toContain('b')
@@ -107,9 +107,9 @@ describe('conflict-detector', () => {
107107
it('returns empty array when scopes do not overlap', () => {
108108
setCommands(
109109
makeCommand({ id: 'a', shortcuts: ['⌘N'], scope: 'About window' }),
110-
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'Settings window' }),
110+
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'Command palette' }),
111111
)
112-
// About window and Settings window don't overlap
112+
// About window and Command palette don't overlap
113113
const conflicts = findConflictsForShortcut('⌘N', 'About window')
114114
const ids = conflicts.map((c) => c.id)
115115
expect(ids).toContain('a')
@@ -180,7 +180,7 @@ describe('conflict-detector', () => {
180180
it('does not report a conflict when scopes do not overlap', () => {
181181
setCommands(
182182
makeCommand({ id: 'a', shortcuts: ['⌘N'], scope: 'About window' }),
183-
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'Settings window' }),
183+
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'Command palette' }),
184184
)
185185
expect(getAllConflicts()).toEqual([])
186186
})
@@ -199,12 +199,12 @@ describe('conflict-detector', () => {
199199

200200
it('handles three commands sharing a shortcut with partial scope overlap', () => {
201201
// a: App (overlaps with everything)
202-
// b: About window (overlaps with App but not Settings window)
203-
// c: Settings window (overlaps with App but not About window)
202+
// b: About window (overlaps with App but not Command palette)
203+
// c: Command palette (overlaps with App but not About window)
204204
setCommands(
205205
makeCommand({ id: 'a', shortcuts: ['⌘N'], scope: 'App' }),
206206
makeCommand({ id: 'b', shortcuts: ['⌘N'], scope: 'About window' }),
207-
makeCommand({ id: 'c', shortcuts: ['⌘N'], scope: 'Settings window' }),
207+
makeCommand({ id: 'c', shortcuts: ['⌘N'], scope: 'Command palette' }),
208208
)
209209
const conflicts = getAllConflicts()
210210
expect(conflicts).toHaveLength(1)
@@ -226,6 +226,54 @@ describe('conflict-detector', () => {
226226
const shortcuts = conflicts.map((c) => c.shortcut).sort()
227227
expect(shortcuts).toEqual(['⌘A', '⌘B'])
228228
})
229+
230+
// ----------------------------------------------------------------------
231+
// Compound-scope conflicts (the regression these tests guard)
232+
// ----------------------------------------------------------------------
233+
234+
it('detects a conflict between two File-list commands sharing a combo', () => {
235+
setCommands(
236+
makeCommand({ id: 'a', shortcuts: ['⌘K'], scope: 'Main window/File list' }),
237+
makeCommand({ id: 'b', shortcuts: ['⌘K'], scope: 'Main window/File list' }),
238+
)
239+
const conflicts = getAllConflicts()
240+
expect(conflicts).toHaveLength(1)
241+
expect(conflicts[0].commandIds).toContain('a')
242+
expect(conflicts[0].commandIds).toContain('b')
243+
})
244+
245+
it('detects a conflict between a Main-window command and a File-list command sharing a combo', () => {
246+
setCommands(
247+
makeCommand({ id: 'a', shortcuts: ['⌘K'], scope: 'Main window' }),
248+
makeCommand({ id: 'b', shortcuts: ['⌘K'], scope: 'Main window/File list' }),
249+
)
250+
const conflicts = getAllConflicts()
251+
expect(conflicts).toHaveLength(1)
252+
expect(conflicts[0].commandIds).toContain('a')
253+
expect(conflicts[0].commandIds).toContain('b')
254+
})
255+
256+
it('does NOT report a conflict for the default Brief/Full ←/→ pairs', () => {
257+
// The registry binds ← and → in BOTH Brief and Full mode on purpose; the
258+
// modes are mutually exclusive, so they must never be flagged as conflicts.
259+
setCommands(
260+
makeCommand({ id: 'nav.left', shortcuts: ['←'], scope: 'Main window/Brief mode' }),
261+
makeCommand({ id: 'nav.right', shortcuts: ['→'], scope: 'Main window/Brief mode' }),
262+
makeCommand({ id: 'nav.firstInFull', shortcuts: ['←'], scope: 'Main window/Full mode' }),
263+
makeCommand({ id: 'nav.lastInFull', shortcuts: ['→'], scope: 'Main window/Full mode' }),
264+
)
265+
expect(getAllConflicts()).toEqual([])
266+
})
267+
268+
it('does NOT report a conflict between a Network command and a File-list command (sibling views)', () => {
269+
// Enter is bound on both network.selectHost and nav.open, but a pane shows
270+
// the network browser INSTEAD of the file list, so they never coexist.
271+
setCommands(
272+
makeCommand({ id: 'network.selectHost', shortcuts: ['Enter'], scope: 'Main window/Network' }),
273+
makeCommand({ id: 'nav.open', shortcuts: ['Enter'], scope: 'Main window/File list' }),
274+
)
275+
expect(getAllConflicts()).toEqual([])
276+
})
229277
})
230278

231279
// ========================================================================

apps/desktop/src/lib/shortcuts/conflict-detector.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ export function getAllConflicts(): ShortcutConflict[] {
4040
for (const cmd of commands) {
4141
// Filter out empty shortcuts (used during editing)
4242
const shortcuts = getEffectiveShortcuts(cmd.id).filter((s) => s)
43-
const scope = cmd.scope as CommandScope
43+
const scope = cmd.scope
4444

4545
for (const shortcut of shortcuts) {
4646
// Create a unique key for this shortcut
@@ -62,10 +62,9 @@ export function getAllConflicts(): ShortcutConflict[] {
6262
// Check for actual scope overlap between all pairs
6363
const actualConflicts: Command[] = []
6464
for (const c of conflictingCommands) {
65-
const cScope = c.scope as CommandScope
6665
// Check if this command conflicts with any other
6766
const hasOverlap = conflictingCommands.some(
68-
(other) => other.id !== c.id && scopesOverlap(cScope, other.scope),
67+
(other) => other.id !== c.id && scopesOverlap(c.scope, other.scope),
6968
)
7069
if (hasOverlap) {
7170
actualConflicts.push(c)

apps/desktop/src/lib/shortcuts/scope-hierarchy.ts

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,42 +3,50 @@
33
* Determines which scopes' shortcuts are active in a given context.
44
*/
55

6-
/** All available command scopes */
7-
export type CommandScope =
8-
| 'App' // Global, works everywhere
9-
| 'Main window' // Main window context
10-
| 'File list' // File list focused
11-
| 'Command palette' // Command palette open
12-
| 'Navigation' // Navigation context
13-
| 'Selection' // Selection operations
14-
| 'Edit' // Edit operations
15-
| 'View' // View operations
16-
| 'Help' // Help operations
17-
| 'About window' // About window context
18-
| 'Settings window' // Settings window context
6+
import type { CommandScope } from '$lib/commands/types'
7+
8+
export type { CommandScope }
199

2010
/**
2111
* Scope hierarchy - when a scope is active, these scopes' shortcuts also trigger.
22-
* Order matters: more specific scopes are listed first for priority.
12+
* Each entry is the scope's ancestry chain, most specific first. Two scopes
13+
* "overlap" (and so can conflict) when one chain contains the other.
14+
*
15+
* The chains mirror what actually renders together in the running app:
16+
*
17+
* - `Main window/Brief mode` / `Main window/Full mode` sit UNDER
18+
* `Main window/File list`: the file list renders in both view modes, so a
19+
* mode-scoped key genuinely collides with a File-list key. Brief and Full stay
20+
* siblings (neither chain contains the other), so they don't conflict with each
21+
* other — the registry binds ←/→ in both on purpose, and the modes never coexist.
22+
* - `Main window/Network`, `Main window/Share browser`, and
23+
* `Main window/Volume chooser` are siblings of `Main window/File list`: a pane
24+
* shows one of them INSTEAD of the file list, so their keys don't collide with
25+
* File-list keys (they share only `Main window` + `App`).
26+
*
27+
* `Command palette` inherits `Main window` (it overlays the main window, so its
28+
* keys can collide with Main-window keys). `Onboarding` is a modal under `App`
29+
* only. (The registry has no `Onboarding` commands today; the chain is here so
30+
* the union stays exhaustive.)
2331
*/
2432
const scopeHierarchy: Record<CommandScope, CommandScope[]> = {
2533
App: ['App'],
2634
'Main window': ['Main window', 'App'],
27-
'File list': ['File list', 'Main window', 'App'],
28-
'Command palette': ['Command palette', 'Main window', 'App'],
29-
Navigation: ['Navigation', 'Main window', 'App'],
30-
Selection: ['Selection', 'Main window', 'App'],
31-
Edit: ['Edit', 'Main window', 'App'],
32-
View: ['View', 'Main window', 'App'],
33-
Help: ['Help', 'Main window', 'App'],
35+
'Main window/File list': ['Main window/File list', 'Main window', 'App'],
36+
'Main window/Brief mode': ['Main window/Brief mode', 'Main window/File list', 'Main window', 'App'],
37+
'Main window/Full mode': ['Main window/Full mode', 'Main window/File list', 'Main window', 'App'],
38+
'Main window/Network': ['Main window/Network', 'Main window', 'App'],
39+
'Main window/Share browser': ['Main window/Share browser', 'Main window', 'App'],
40+
'Main window/Volume chooser': ['Main window/Volume chooser', 'Main window', 'App'],
3441
'About window': ['About window', 'App'],
35-
'Settings window': ['Settings window', 'App'],
42+
Onboarding: ['Onboarding', 'App'],
43+
'Command palette': ['Command palette', 'Main window', 'App'],
3644
}
3745

3846
/**
3947
* Get all scopes that are active when the given scope is current.
4048
* Returns scopes in priority order (most specific first).
41-
* Returns empty array for unknown/compound scopes (like 'Main window/File list').
49+
* Returns empty array for an unknown scope.
4250
*/
4351
export function getActiveScopes(current: string): CommandScope[] {
4452
if (current in scopeHierarchy) {
@@ -54,8 +62,8 @@ export function getActiveScopes(current: string): CommandScope[] {
5462
export function scopesOverlap(scopeA: string, scopeB: string): boolean {
5563
const activeA = getActiveScopes(scopeA)
5664
const activeB = getActiveScopes(scopeB)
57-
// They overlap if either hierarchy includes the other scope
58-
// If either scope is unknown (empty activeScopes), treat them as non-overlapping
65+
// They overlap if one scope's ancestry chain contains the other.
66+
// If either scope is unknown (empty chain), treat them as non-overlapping.
5967
if (activeA.length === 0 || activeB.length === 0) {
6068
return false
6169
}

0 commit comments

Comments
 (0)