Skip to content

Commit da57056

Browse files
committed
Bugfix: Shortcut rebinds reach other windows live
- A Settings-window rebind now propagates to the main window's shortcut map, its `onShortcutChange` consumers (reactive chips, F-key bar, palette, sort tooltips), AND its keyboard dispatch map — no restart needed. - Every store mutation (`setShortcut`/`addShortcut`/`removeShortcut`/`resetShortcut`/`resetAllShortcuts`) emits a `shortcuts:changed` Tauri event after updating local state, mirroring settings-store's `settings:changed` pattern. - `initializeShortcuts` installs a once-per-window listener that applies remote changes to the local map and notifies listeners, without re-saving or re-emitting. - Loop guard: each window stamps emits with a per-window `senderId` and drops events that match its own (shortcut payloads are fresh-reference arrays, so settings-store's value-equality guard doesn't apply). - The capability-restricted viewer never subscribes (it doesn't call `initializeShortcuts`); module eval adds no Tauri listener.
1 parent 2247dac commit da57056

3 files changed

Lines changed: 310 additions & 2 deletions

File tree

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

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,31 @@ shortcuts:
4040
defaults. The notification path also syncs menu accelerators (`updateMenuAccelerator` no-ops for commands without a
4141
menu item).
4242

43+
### Cross-window propagation (`shortcuts:changed`)
44+
45+
The store is per-webview module state, so a rebind in the Settings window must reach the main window's `customShortcuts`
46+
map, its `onShortcutChange` consumers (reactive chips, F-key bar, palette, sort tooltips), AND its dispatch map —
47+
otherwise they stay stale until restart and the new key doesn't actually work. This mirrors settings-store's
48+
`settings:changed` pattern.
49+
50+
- **Every mutation** (`setShortcut` / `addShortcut` / `removeShortcut` / `resetShortcut` / `resetAllShortcuts`) emits a
51+
`shortcuts:changed` Tauri event AFTER updating local state and saving. A single-command change carries
52+
`{ senderId, commandId, shortcuts }` where `shortcuts` is the new custom list, or `null` when the command reverted to
53+
its registry default (cleanup/reset dropped the map entry). `resetAllShortcuts` emits `{ senderId, resetAll: true }`.
54+
- **`initializeShortcuts` installs a listener** (`setupCrossWindowListener`) that, on a remote change, updates the local
55+
map directly and calls `notifyListeners(commandId)` so reactive consumers + the dispatch map rebuild. It does NOT save
56+
to disk (the writer already saved) and does NOT re-emit (that would loop). A reset-all clears the whole map and
57+
notifies each previously-customized id (computed from the map before clearing). The listener is installed once per
58+
window — guarded by both `initialized` and a non-null `crossWindowUnlisten`, so a re-init can't double-subscribe.
59+
- **Loop guard:** each window stamps every emit with a per-window `SENDER_ID` (a `crypto.randomUUID()` generated once at
60+
module load); the listener drops any event whose `senderId` matches its own. Settings-store dedupes instead via a
61+
strict-equality idempotency guard on the cached value, but shortcut payloads are arrays that arrive as fresh
62+
references (nothing to compare by identity), so the explicit sender id is the clean guard here.
63+
- **The viewer never subscribes.** It's capability-restricted and never calls `initializeShortcuts`, so no
64+
`shortcuts:changed` listener is installed there. Importing `shortcuts-store` at module eval (the viewer pulls it in
65+
transitively via the literal-mode `ShortcutChip`) only runs `crypto.randomUUID()` and declares functions — no
66+
`listen()` call — so the capability boundary holds.
67+
4368
### Reactive reads (`reactive-shortcuts.svelte.ts`)
4469

4570
Two readers over one module-level `$state` version (bumped on every `onShortcutChange`):
@@ -168,8 +193,13 @@ hurt (adding F8 to the registry but forgetting to add it to the keydown handler)
168193

169194
### Why separate MCP listener for main window?
170195

171-
The settings window has a full MCP bridge that syncs all state. The main window only needs to react to shortcut changes.
172-
A lightweight listener keeps concerns separated and reduces overhead.
196+
`mcp-shortcuts-listener.ts` listens for the backend's `mcp-shortcuts-set` / `-remove` / `-reset` events (a distinct
197+
backend→main channel) so MCP tools can rebind even when the Settings window is closed. It calls the same store mutators
198+
(`setShortcut` / `removeShortcut` / `resetShortcut`), so an MCP-driven change in the main window now also rides the
199+
`shortcuts:changed` cross-window event to the Settings window for free — no special handling needed. The MCP listener
200+
stays separate because its trigger (a backend event with a different payload shape) is unrelated to the window-to-window
201+
`shortcuts:changed` channel; folding them together would conflate two transports. (If a future change makes the two
202+
channels share a payload, revisit — but today it's not a trivial merge, so leave it.)
173203

174204
## Gotchas
175205

apps/desktop/src/lib/shortcuts/shortcuts-store.test.ts

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,46 @@ vi.mock('@tauri-apps/plugin-store', () => ({
4646
}),
4747
}))
4848

49+
// Fake `@tauri-apps/api/event` bus. `listen` records each (name, handler) pair
50+
// in a shared registry so a test can drive a "remote window" by invoking the
51+
// captured handler; `emit` records each (name, payload) so a test can assert
52+
// what a mutation broadcast. Declared via `vi.hoisted` so the registries survive
53+
// the `vi.resetModules()` reloads (they stand in for the cross-window OS bus,
54+
// which a webview reload doesn't reset).
55+
const eventBus = vi.hoisted(() => ({
56+
listeners: new Map<string, Array<(event: { payload: unknown }) => void>>(),
57+
emits: [] as Array<{ name: string; payload: unknown }>,
58+
}))
59+
60+
vi.mock('@tauri-apps/api/event', () => ({
61+
listen: vi.fn((name: string, handler: (event: { payload: unknown }) => void) => {
62+
const arr = eventBus.listeners.get(name) ?? []
63+
arr.push(handler)
64+
eventBus.listeners.set(name, arr)
65+
return Promise.resolve(() => {
66+
const cur = eventBus.listeners.get(name) ?? []
67+
eventBus.listeners.set(
68+
name,
69+
cur.filter((h) => h !== handler),
70+
)
71+
})
72+
}),
73+
emit: vi.fn((name: string, payload: unknown) => {
74+
eventBus.emits.push({ name, payload })
75+
return Promise.resolve()
76+
}),
77+
}))
78+
79+
// Deliver an event to every handler registered in THIS window for `name`. Stands
80+
// in for the OS delivering a cross-window broadcast. The originating window also
81+
// receives its own emits on the real bus, so tests use this to prove the
82+
// loop-guard drops self-originated events.
83+
function deliver(name: string, payload: unknown): void {
84+
for (const handler of eventBus.listeners.get(name) ?? []) {
85+
handler({ payload })
86+
}
87+
}
88+
4989
vi.mock('$lib/settings/store-path', () => ({
5090
resolveStorePath: (name: string) => Promise.resolve(name),
5191
}))
@@ -72,9 +112,16 @@ async function flushSave() {
72112
beforeEach(() => {
73113
// Fresh disk per test; resetModules so each test starts with an uninitialized store.
74114
disk.clear()
115+
eventBus.listeners.clear()
116+
eventBus.emits.length = 0
75117
vi.resetModules()
76118
})
77119

120+
// All `shortcuts:changed` emit payloads recorded so far.
121+
function changedEmits(): unknown[] {
122+
return eventBus.emits.filter((e) => e.name === 'shortcuts:changed').map((e) => e.payload)
123+
}
124+
78125
describe('shortcuts-store persistence round-trips', () => {
79126
it('keeps a removed-only-default shortcut removed across a reload (RC2)', async () => {
80127
// `app.hide` defaults to ['⌘H']. Remove the only shortcut, leaving [].
@@ -189,3 +236,134 @@ describe('shortcuts-store persistence round-trips', () => {
189236
expect(store.isShortcutModified('app.showAll')).toBe(false)
190237
})
191238
})
239+
240+
describe('shortcuts-store cross-window propagation (RC1)', () => {
241+
it('setShortcut emits a shortcuts:changed event with the command id and new shortcuts', async () => {
242+
const store = await loadStore()
243+
await store.initializeShortcuts()
244+
245+
store.setShortcut('app.showAll', 0, 'F9')
246+
await flushSave()
247+
248+
const emits = changedEmits() as Array<{ commandId?: string; shortcuts?: unknown }>
249+
const forCmd = emits.find((p) => p.commandId === 'app.showAll')
250+
expect(forCmd).toBeDefined()
251+
expect(forCmd?.shortcuts).toEqual(['F9'])
252+
})
253+
254+
it('applying a received remote change updates effective shortcuts AND fires listeners, without saving or re-emitting', async () => {
255+
const store = await loadStore()
256+
await store.initializeShortcuts()
257+
258+
const seen: string[] = []
259+
store.onShortcutChange((id) => seen.push(id))
260+
261+
// A second window rebound app.showAll to F9 and broadcast it. The emit
262+
// carries that window's senderId, so it differs from ours.
263+
deliver('shortcuts:changed', { senderId: 'other-window', commandId: 'app.showAll', shortcuts: ['F9'] })
264+
await flushSave()
265+
266+
// Local effective state reflects the remote change.
267+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(['F9'])
268+
// Local reactive consumers were notified.
269+
expect(seen).toContain('app.showAll')
270+
// The writer already persisted; we must NOT write disk again here.
271+
expect(disk.has('shortcut:app.showAll')).toBe(false)
272+
// And we must NOT re-broadcast (that would loop).
273+
expect(changedEmits()).toHaveLength(0)
274+
})
275+
276+
it('propagates the "removed all shortcuts" empty-array state as [], not as a reset', async () => {
277+
// `app.hide` defaults to ['⌘H']. Removing its only shortcut leaves [], which
278+
// means "user removed all bindings, don't fall back to defaults" — distinct
279+
// from a reset (which would send null and revert to ['⌘H']).
280+
const store = await loadStore()
281+
await store.initializeShortcuts()
282+
283+
store.removeShortcut('app.hide', 0)
284+
await flushSave()
285+
286+
const emits = changedEmits() as Array<{ commandId?: string; shortcuts?: unknown }>
287+
const forCmd = emits.find((p) => p.commandId === 'app.hide')
288+
expect(forCmd?.shortcuts).toEqual([])
289+
290+
// A receiving window applies [] (removed-all), not the ⌘H default.
291+
deliver('shortcuts:changed', { senderId: 'other-window', commandId: 'app.hide', shortcuts: [] })
292+
expect(store.getEffectiveShortcuts('app.hide')).toEqual([])
293+
})
294+
295+
it('a received reset (null shortcuts) clears the local custom entry and notifies', async () => {
296+
const store = await loadStore()
297+
await store.initializeShortcuts()
298+
299+
// Local window has a customization first.
300+
store.setShortcut('app.hide', 0, '⌃X')
301+
await flushSave()
302+
expect(store.getEffectiveShortcuts('app.hide')).toEqual(['⌃X'])
303+
304+
const seen: string[] = []
305+
store.onShortcutChange((id) => seen.push(id))
306+
307+
// Another window reset app.hide to its default.
308+
deliver('shortcuts:changed', { senderId: 'other-window', commandId: 'app.hide', shortcuts: null })
309+
310+
expect(store.getEffectiveShortcuts('app.hide')).toEqual(getDefaultShortcuts('app.hide'))
311+
expect(store.isShortcutModified('app.hide')).toBe(false)
312+
expect(seen).toContain('app.hide')
313+
})
314+
315+
it('reset-all round-trips: a received reset-all clears every local customization and notifies each', async () => {
316+
const store = await loadStore()
317+
await store.initializeShortcuts()
318+
319+
store.setShortcut('app.showAll', 0, 'F9')
320+
await flushSave()
321+
store.setShortcut('app.hide', 0, '⌃X')
322+
await flushSave()
323+
324+
const seen: string[] = []
325+
store.onShortcutChange((id) => seen.push(id))
326+
327+
deliver('shortcuts:changed', { senderId: 'other-window', resetAll: true })
328+
329+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(getDefaultShortcuts('app.showAll'))
330+
expect(store.getEffectiveShortcuts('app.hide')).toEqual(getDefaultShortcuts('app.hide'))
331+
expect(seen).toContain('app.showAll')
332+
expect(seen).toContain('app.hide')
333+
})
334+
335+
it('resetAllShortcuts emits a reset-all marker', async () => {
336+
const store = await loadStore()
337+
await store.initializeShortcuts()
338+
339+
store.setShortcut('app.showAll', 0, 'F9')
340+
await flushSave()
341+
eventBus.emits.length = 0 // ignore the setShortcut emit
342+
343+
await store.resetAllShortcuts()
344+
345+
const emits = changedEmits() as Array<{ resetAll?: boolean }>
346+
expect(emits.some((p) => p.resetAll === true)).toBe(true)
347+
})
348+
349+
it('loop guard: the originating window ignores its own broadcast (no double-apply, no notify)', async () => {
350+
const store = await loadStore()
351+
await store.initializeShortcuts()
352+
353+
store.setShortcut('app.showAll', 0, 'F9')
354+
await flushSave()
355+
356+
// Grab the payload this window actually emitted, including its own senderId.
357+
const ownEmit = changedEmits()[0] as { senderId: string; commandId: string; shortcuts: unknown }
358+
expect(ownEmit.senderId).toBeTruthy()
359+
360+
const seen: string[] = []
361+
store.onShortcutChange((id) => seen.push(id))
362+
363+
// The OS echoes our own emit back to us. The loop guard must drop it.
364+
deliver('shortcuts:changed', ownEmit)
365+
366+
expect(seen).not.toContain('app.showAll')
367+
expect(changedEmits()).toHaveLength(1) // still just the original, no re-emit
368+
})
369+
})

0 commit comments

Comments
 (0)