Skip to content

Commit 6c21fd1

Browse files
committed
Bugfix: Custom shortcut removals survive reopening Settings
- Load persisted empty arrays at init (`Array.isArray` instead of `length > 0`), so removing all of a command's shortcuts stays removed across a webview reload instead of silently falling back to the registry default. Still skips non-array garbage. - Reconcile disk on every save: `saveToStore` now deletes any `shortcut:*` key whose command no longer has an in-memory entry, then writes the current entries. Previously a stale key survived when `resetShortcut`/`cleanupIfMatchesDefaults` dropped a map entry, so a removed or reset customization resurrected on next load. - `resetAllShortcuts` now clears the map and delegates to `saveToStore`, dropping its duplicate delete-loop. - Add round-trip persistence tests covering both fixes, reset/reset-all, normal customs, and garbage skipping.
1 parent 1db537f commit 6c21fd1

4 files changed

Lines changed: 217 additions & 16 deletions

File tree

apps/desktop/coverage-allowlist.json

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -244,9 +244,6 @@
244244
"shortcuts/mcp-shortcuts-listener.ts": {
245245
"reason": "MCP listener, depends on Tauri events"
246246
},
247-
"shortcuts/shortcuts-store.ts": {
248-
"reason": "Depends on Tauri store APIs"
249-
},
250247
"stores/volume-store.svelte.ts": {
251248
"reason": "Depends on Tauri listen/invoke APIs, integration point between backend events and frontend state"
252249
},

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ shortcuts:
2828
- Delta-only storage: only customizations, not defaults
2929
- Empty array means "all shortcuts removed"
3030
- Missing command means "use defaults from registry"
31+
- `saveToStore` reconciles disk against the in-memory map on every write: it deletes any `shortcut:*` key whose command
32+
no longer has a map entry, then writes the current entries. So when `resetShortcut` / `cleanupIfMatchesDefaults` drops
33+
an entry (e.g. a custom that's been edited back to defaults, or `app.showAll`'s `[]` default after removing an added
34+
shortcut), the stale disk key goes too. Without this the old value resurrects at next load. `resetAllShortcuts` relies
35+
on the same step — it just clears the map and saves.
36+
- `initializeShortcuts` loads any array, including `[]`, and skips only non-array garbage (`Array.isArray` check). The
37+
empty array is the persisted "removed all shortcuts" state, so it must survive a reload, not be treated as absent.
3138
- `initializeShortcuts` notifies `onShortcutChange` listeners for every loaded customization, so components that mounted
3239
before the async init finished (reactive shortcut reads, the dispatch map) catch up instead of showing registry
3340
defaults. The notification path also syncs menu accelerators (`updateMenuAccelerator` no-ops for commands without a
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
/**
2+
* Round-trip persistence tests for the shortcuts store.
3+
*
4+
* These guard the two disk-state invariants documented in `CLAUDE.md`
5+
* § "Empty array vs missing key":
6+
*
7+
* - A persisted empty array (`"shortcut:<id>": []`) means "user removed all
8+
* shortcuts, don't use defaults" and must survive a reload.
9+
* - A `shortcut:<id>` key with no in-memory entry is stale and must be deleted
10+
* on save, so a removed/reset customization can't resurrect at next load.
11+
*
12+
* "Reload" is simulated with `vi.resetModules()`: the store's in-memory map is
13+
* module-scoped, so re-importing the module after a reset mimics a fresh webview
14+
* re-reading `shortcuts.json` from disk. The mock `load()` is backed by a single
15+
* shared `Map` (`disk`) that persists across resets, standing in for the file on
16+
* disk.
17+
*/
18+
import { describe, it, expect, vi, beforeEach } from 'vitest'
19+
// Static import so the file genuinely exercises source (not just its mocks).
20+
// `getDefaultShortcuts` reads the registry, not module-scoped store state, so
21+
// it stays valid across the `vi.resetModules()` reloads below.
22+
import { getDefaultShortcuts } from './shortcuts-store'
23+
24+
// Shared backing store for the fake plugin-store, persisting across
25+
// `vi.resetModules()` to stand in for the on-disk `shortcuts.json`. A `Map`
26+
// avoids dynamic property delete. Declared via `vi.hoisted` so the hoisted
27+
// `vi.mock` factory can capture it.
28+
const disk = vi.hoisted(() => new Map<string, unknown>())
29+
30+
vi.mock('@tauri-apps/plugin-store', () => ({
31+
load: vi.fn((_path: string, opts?: { defaults?: Record<string, unknown> }) => {
32+
// Apply defaults only for keys not already present (matches plugin-store).
33+
for (const [k, v] of Object.entries(opts?.defaults ?? {})) {
34+
if (!disk.has(k)) disk.set(k, v)
35+
}
36+
return Promise.resolve({
37+
get: (key: string) => Promise.resolve(disk.get(key)),
38+
set: (key: string, value: unknown) => {
39+
disk.set(key, value)
40+
return Promise.resolve()
41+
},
42+
delete: (key: string) => Promise.resolve(disk.delete(key)),
43+
keys: () => Promise.resolve([...disk.keys()]),
44+
save: () => Promise.resolve(),
45+
})
46+
}),
47+
}))
48+
49+
vi.mock('$lib/settings/store-path', () => ({
50+
resolveStorePath: (name: string) => Promise.resolve(name),
51+
}))
52+
53+
vi.mock('$lib/ipc/bindings', () => ({
54+
commands: {
55+
updateMenuAccelerator: () => Promise.resolve({ status: 'ok' as const, data: null }),
56+
},
57+
}))
58+
59+
// Import fresh after a module reset so the store's module-scoped map starts empty,
60+
// then re-reads `disk`. Returns the relevant store functions.
61+
async function loadStore() {
62+
return await import('./shortcuts-store')
63+
}
64+
65+
// The mutating store APIs (`setShortcut`, `addShortcut`, `removeShortcut`,
66+
// `resetShortcut`) are synchronous and fire `void saveToStore()`. Flush a few
67+
// microtask turns so the async write to `disk` lands before we assert on it.
68+
async function flushSave() {
69+
for (let i = 0; i < 5; i++) await Promise.resolve()
70+
}
71+
72+
beforeEach(() => {
73+
// Fresh disk per test; resetModules so each test starts with an uninitialized store.
74+
disk.clear()
75+
vi.resetModules()
76+
})
77+
78+
describe('shortcuts-store persistence round-trips', () => {
79+
it('keeps a removed-only-default shortcut removed across a reload (RC2)', async () => {
80+
// `app.hide` defaults to ['⌘H']. Remove the only shortcut, leaving [].
81+
let store = await loadStore()
82+
await store.initializeShortcuts()
83+
84+
store.removeShortcut('app.hide', 0)
85+
await flushSave()
86+
expect(store.getEffectiveShortcuts('app.hide')).toEqual([])
87+
// Disk must hold the empty array, not the absence of the key.
88+
expect(disk.get('shortcut:app.hide')).toEqual([])
89+
90+
// Reload (fresh webview re-reads disk).
91+
vi.resetModules()
92+
store = await loadStore()
93+
await store.initializeShortcuts()
94+
95+
expect(store.getEffectiveShortcuts('app.hide')).toEqual([])
96+
})
97+
98+
it('does not resurrect a removed shortcut on a default-[] command (RC3)', async () => {
99+
// `app.showAll` defaults to []. Add a custom, then remove it.
100+
let store = await loadStore()
101+
await store.initializeShortcuts()
102+
103+
store.addShortcut('app.showAll', 'F7')
104+
await flushSave()
105+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(['F7'])
106+
expect(disk.get('shortcut:app.showAll')).toEqual(['F7'])
107+
108+
store.removeShortcut('app.showAll', 0)
109+
await flushSave()
110+
// Now matches the [] default, so the map entry is cleaned up and the stale
111+
// disk key must be deleted.
112+
expect(disk.has('shortcut:app.showAll')).toBe(false)
113+
114+
vi.resetModules()
115+
store = await loadStore()
116+
await store.initializeShortcuts()
117+
118+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual([])
119+
})
120+
121+
it('reset-to-default survives a reload (RC3)', async () => {
122+
// Customize `app.hide` away from its default, then reset it.
123+
let store = await loadStore()
124+
await store.initializeShortcuts()
125+
126+
store.setShortcut('app.hide', 0, '⌃X')
127+
await flushSave()
128+
expect(disk.get('shortcut:app.hide')).toEqual(['⌃X'])
129+
130+
store.resetShortcut('app.hide')
131+
await flushSave()
132+
// After reset the stale disk key must be gone.
133+
expect(disk.has('shortcut:app.hide')).toBe(false)
134+
135+
vi.resetModules()
136+
store = await loadStore()
137+
await store.initializeShortcuts()
138+
139+
expect(store.getEffectiveShortcuts('app.hide')).toEqual(getDefaultShortcuts('app.hide'))
140+
})
141+
142+
it('persists and reloads a normal customization (regression)', async () => {
143+
let store = await loadStore()
144+
await store.initializeShortcuts()
145+
146+
store.setShortcut('app.showAll', 0, 'F9')
147+
await flushSave()
148+
expect(disk.get('shortcut:app.showAll')).toEqual(['F9'])
149+
150+
vi.resetModules()
151+
store = await loadStore()
152+
await store.initializeShortcuts()
153+
154+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(['F9'])
155+
})
156+
157+
it('resetAllShortcuts clears every customization across a reload', async () => {
158+
let store = await loadStore()
159+
await store.initializeShortcuts()
160+
161+
store.setShortcut('app.showAll', 0, 'F9')
162+
await flushSave()
163+
store.setShortcut('app.hide', 0, '⌃X')
164+
await flushSave()
165+
expect(disk.get('shortcut:app.showAll')).toEqual(['F9'])
166+
expect(disk.get('shortcut:app.hide')).toEqual(['⌃X'])
167+
168+
await store.resetAllShortcuts()
169+
expect(disk.has('shortcut:app.showAll')).toBe(false)
170+
expect(disk.has('shortcut:app.hide')).toBe(false)
171+
172+
vi.resetModules()
173+
store = await loadStore()
174+
await store.initializeShortcuts()
175+
176+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(getDefaultShortcuts('app.showAll'))
177+
expect(store.getEffectiveShortcuts('app.hide')).toEqual(getDefaultShortcuts('app.hide'))
178+
})
179+
180+
it('ignores non-array (garbage) values at load', async () => {
181+
// Simulate a corrupted entry on disk.
182+
disk.set('shortcut:app.showAll', 'not-an-array')
183+
184+
const store = await loadStore()
185+
await store.initializeShortcuts()
186+
187+
// Garbage is skipped, so the command falls back to its registry default ([]).
188+
expect(store.getEffectiveShortcuts('app.showAll')).toEqual(getDefaultShortcuts('app.showAll'))
189+
expect(store.isShortcutModified('app.showAll')).toBe(false)
190+
})
191+
})

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

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,10 @@ export async function initializeShortcuts(): Promise<void> {
7575
for (const key of shortcutKeys) {
7676
const commandId = key.replace('shortcut:', '')
7777
const shortcuts = await store.get<string[]>(key)
78-
if (shortcuts && shortcuts.length > 0) {
78+
// An empty array is a real, persisted state ("user removed all shortcuts,
79+
// don't fall back to defaults") and must load, so we accept any array and
80+
// only skip non-array garbage. See CLAUDE.md § "Empty array vs missing key".
81+
if (Array.isArray(shortcuts)) {
7982
customShortcuts.set(commandId, shortcuts)
8083
}
8184
}
@@ -318,19 +321,11 @@ export function resetShortcut(commandId: string): void {
318321
export async function resetAllShortcuts(): Promise<void> {
319322
const modifiedIds = [...customShortcuts.keys()]
320323

321-
// Clear all customizations
324+
// Clear all customizations. With the map empty, saveToStore's reconcile step
325+
// deletes every stale `shortcut:*` key from disk, so we don't duplicate the
326+
// delete-loop here.
322327
customShortcuts.clear()
323-
324-
// Delete all shortcut keys from store
325-
const store = await getStore()
326-
const keys = await store.keys()
327-
for (const key of keys) {
328-
if (key.startsWith('shortcut:')) {
329-
await store.delete(key)
330-
}
331-
}
332-
await store.set('_schemaVersion', SCHEMA_VERSION)
333-
await store.save()
328+
await saveToStore()
334329

335330
// Notify listeners for all modified commands
336331
for (const id of modifiedIds) {
@@ -347,6 +342,17 @@ async function saveToStore(): Promise<void> {
347342
const store = await getStore()
348343
log.debug('Saving shortcuts: {shortcuts}', { shortcuts: JSON.stringify(Object.fromEntries(customShortcuts)) })
349344

345+
// Reconcile disk with the in-memory map so the save reflects the full current
346+
// state, not just additions. Delete every `shortcut:*` key that no longer has
347+
// a map entry: when `resetShortcut` or `cleanupIfMatchesDefaults` drops an
348+
// entry, the matching disk key must go too, or it resurrects on next load.
349+
const keys = await store.keys()
350+
for (const key of keys) {
351+
if (key.startsWith('shortcut:') && !customShortcuts.has(key.replace('shortcut:', ''))) {
352+
await store.delete(key)
353+
}
354+
}
355+
350356
// Store each command's shortcuts at top level (like settings-store does)
351357
// This avoids potential issues with nested objects
352358
for (const [commandId, shortcuts] of customShortcuts) {

0 commit comments

Comments
 (0)