Skip to content

Commit 2a8bf5f

Browse files
committed
fix(undo-redo): close two more review findings
- Block setItem/removeItem on the first getItem (hydration read) so a state change cannot persist an empty in-memory snapshot over the IndexedDB payload before zustand's persist middleware finishes rehydration. Without this guard, switching from synchronous localStorage to async IndexedDB introduced a race that could wipe undo history after reload (Cursor Bugbot, medium). - Propagate IndexedDB errors from clearPersistedUndoRedo so a delete failure surfaces to clearUserData's outer try/catch instead of silently leaving a previous user's payload behind (Cursor Bugbot, medium). - Add tests for the hydration-read ordering and the error propagation. Signed-off-by: JaeHyung Jang <jaehyung.jang@navercorp.com>
1 parent 3f0e896 commit 2a8bf5f

2 files changed

Lines changed: 70 additions & 9 deletions

File tree

apps/sim/stores/undo-redo/storage.test.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,12 +176,41 @@ describe('undo-redo IndexedDB storage adapter', () => {
176176
expect(idbStore.get(MIGRATION_KEY)).toBe(true)
177177
})
178178

179-
it('swallows IndexedDB errors so sign-out does not crash', async () => {
179+
it('propagates IndexedDB errors so callers can surface the failure', async () => {
180180
const { clearPersistedUndoRedo, migrationReady } = await loadFreshModule()
181181
await migrationReady
182182

183183
idbDel.mockRejectedValueOnce(new Error('idb delete failed'))
184-
await expect(clearPersistedUndoRedo()).resolves.toBeUndefined()
184+
await expect(clearPersistedUndoRedo()).rejects.toThrow('idb delete failed')
185+
})
186+
})
187+
188+
describe('hydration race', () => {
189+
it('blocks setItem until the first getItem resolves', async () => {
190+
const { indexedDBStorage, migrationReady } = await loadFreshModule()
191+
await migrationReady
192+
idbStore.set(STORE_KEY, 'persisted-snapshot')
193+
194+
let releaseRead: ((value: 'persisted-snapshot') => void) | null = null
195+
idbGet.mockImplementationOnce(
196+
() =>
197+
new Promise<'persisted-snapshot'>((resolve) => {
198+
releaseRead = resolve
199+
})
200+
)
201+
202+
const readPromise = indexedDBStorage.getItem(STORE_KEY)
203+
const writePromise = indexedDBStorage.setItem(STORE_KEY, 'empty-state')
204+
205+
// Give the microtask queue a chance to process; the write must still be pending.
206+
await Promise.resolve()
207+
expect(idbStore.get(STORE_KEY)).toBe('persisted-snapshot')
208+
209+
releaseRead?.('persisted-snapshot')
210+
await readPromise
211+
await writePromise
212+
213+
expect(idbStore.get(STORE_KEY)).toBe('empty-state')
185214
})
186215
})
187216
})

apps/sim/stores/undo-redo/storage.ts

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,14 @@ const MIGRATION_KEY = 'workflow-undo-redo-migrated'
99

1010
let migrationPromiseInternal: Promise<void> | null = null
1111

12+
/**
13+
* Resolves with the first `getItem` result that goes through the adapter.
14+
* Used to gate writes until the initial rehydration read completes — without
15+
* this, a `setItem` triggered before the async `getItem` returns would
16+
* overwrite the IndexedDB snapshot with an empty in-memory state.
17+
*/
18+
let hydrationReadPromise: Promise<string | null> | null = null
19+
1220
/**
1321
* Migrates existing undo/redo data from localStorage to IndexedDB.
1422
* Runs once on first load; subsequent loads short-circuit on MIGRATION_KEY.
@@ -55,11 +63,23 @@ async function awaitMigration(): Promise<void> {
5563
}
5664
}
5765

66+
async function awaitHydrationRead(): Promise<void> {
67+
if (hydrationReadPromise) {
68+
try {
69+
await hydrationReadPromise
70+
} catch {
71+
// The read promise already swallowed its own errors; ignore here.
72+
}
73+
}
74+
}
75+
5876
/**
5977
* Removes the persisted undo/redo payload from IndexedDB.
6078
*
6179
* Called from `clearUserData` on sign-out so undo history does not
62-
* survive across user sessions on the same device.
80+
* survive across user sessions on the same device. Errors are
81+
* propagated so callers can decide how to react (the default
82+
* `clearUserData` already wraps this call in a try/catch).
6383
*/
6484
export async function clearPersistedUndoRedo(): Promise<void> {
6585
if (typeof window === 'undefined') return
@@ -69,6 +89,7 @@ export async function clearPersistedUndoRedo(): Promise<void> {
6989
await del(STORE_KEY)
7090
} catch (error) {
7191
logger.warn('Failed to clear persisted undo-redo', { error })
92+
throw error
7293
}
7394
}
7495

@@ -77,18 +98,28 @@ export const indexedDBStorage: StateStorage = {
7798
if (typeof window === 'undefined') return null
7899
await awaitMigration()
79100

80-
try {
81-
const value = await get<string>(name)
82-
return value ?? null
83-
} catch (error) {
84-
logger.warn('IndexedDB read failed', { name, error })
85-
return null
101+
const readPromise = (async () => {
102+
try {
103+
const value = await get<string>(name)
104+
return value ?? null
105+
} catch (error) {
106+
logger.warn('IndexedDB read failed', { name, error })
107+
return null
108+
}
109+
})()
110+
111+
// Record the first read so concurrent writes can wait for it to complete.
112+
if (!hydrationReadPromise) {
113+
hydrationReadPromise = readPromise
86114
}
115+
116+
return await readPromise
87117
},
88118

89119
setItem: async (name: string, value: string): Promise<void> => {
90120
if (typeof window === 'undefined') return
91121
await awaitMigration()
122+
await awaitHydrationRead()
92123

93124
try {
94125
await set(name, value)
@@ -100,6 +131,7 @@ export const indexedDBStorage: StateStorage = {
100131
removeItem: async (name: string): Promise<void> => {
101132
if (typeof window === 'undefined') return
102133
await awaitMigration()
134+
await awaitHydrationRead()
103135

104136
try {
105137
await del(name)

0 commit comments

Comments
 (0)