Skip to content

Commit 54a70cc

Browse files
committed
fix(undo-redo): address review feedback
- Gate setItem/removeItem on migration completion via shared awaitMigration() helper. Previously only getItem awaited the one-time localStorage -> IndexedDB migration; a setItem racing the migration could be overwritten when migration completed (Cursor Bugbot, high). - Use `typeof window` for the SSR guard to match apps/sim/stores/terminal/console/storage.ts (Greptile). - Add error-swallowing test for removeItem to mirror the existing coverage on getItem and setItem (Greptile). - Switch storage.test.ts to `@vitest-environment jsdom` since the module-load IIFE depends on `typeof window`. Signed-off-by: JaeHyung Jang <jaehyung.jang@navercorp.com>
1 parent d9201d8 commit 54a70cc

2 files changed

Lines changed: 27 additions & 12 deletions

File tree

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* @vitest-environment node
2+
* @vitest-environment jsdom
33
*/
44
import { beforeEach, describe, expect, it, vi } from 'vitest'
55

@@ -137,6 +137,14 @@ describe('undo-redo IndexedDB storage adapter', () => {
137137
expect(idbStore.has(STORE_KEY)).toBe(false)
138138
})
139139

140+
it('removeItem swallows IndexedDB errors', async () => {
141+
const { indexedDBStorage, migrationReady } = await loadFreshModule()
142+
await migrationReady
143+
144+
idbDel.mockRejectedValueOnce(new Error('idb delete failed'))
145+
await expect(indexedDBStorage.removeItem(STORE_KEY)).resolves.toBeUndefined()
146+
})
147+
140148
it('getItem swallows IndexedDB read errors and returns null', async () => {
141149
const { indexedDBStorage, migrationReady } = await loadFreshModule()
142150
await migrationReady

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

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ let migrationPromiseInternal: Promise<void> | null = null
1717
* for the other persisted Zustand stores that share it.
1818
*/
1919
async function migrateFromLocalStorage(): Promise<void> {
20-
if (typeof localStorage === 'undefined') return
20+
if (typeof window === 'undefined') return
2121

2222
try {
2323
const migrated = await get<boolean>(MIGRATION_KEY)
@@ -36,26 +36,29 @@ async function migrateFromLocalStorage(): Promise<void> {
3636
}
3737
}
3838

39-
if (typeof localStorage !== 'undefined') {
39+
if (typeof window !== 'undefined') {
4040
migrationPromiseInternal = migrateFromLocalStorage().finally(() => {
4141
migrationPromiseInternal = null
4242
})
4343
}
4444

4545
/**
4646
* Resolves when the one-time localStorage → IndexedDB migration finishes.
47-
* Exposed for tests; production code reads through `indexedDBStorage.getItem`
48-
* which already awaits this promise.
47+
* Exposed for tests; production code reads through `indexedDBStorage`
48+
* methods which already await this promise.
4949
*/
5050
export const migrationReady: Promise<void> = migrationPromiseInternal ?? Promise.resolve()
5151

52+
async function awaitMigration(): Promise<void> {
53+
if (migrationPromiseInternal) {
54+
await migrationPromiseInternal
55+
}
56+
}
57+
5258
export const indexedDBStorage: StateStorage = {
5359
getItem: async (name: string): Promise<string | null> => {
54-
if (typeof localStorage === 'undefined') return null
55-
56-
if (migrationPromiseInternal) {
57-
await migrationPromiseInternal
58-
}
60+
if (typeof window === 'undefined') return null
61+
await awaitMigration()
5962

6063
try {
6164
const value = await get<string>(name)
@@ -67,7 +70,9 @@ export const indexedDBStorage: StateStorage = {
6770
},
6871

6972
setItem: async (name: string, value: string): Promise<void> => {
70-
if (typeof localStorage === 'undefined') return
73+
if (typeof window === 'undefined') return
74+
await awaitMigration()
75+
7176
try {
7277
await set(name, value)
7378
} catch (error) {
@@ -76,7 +81,9 @@ export const indexedDBStorage: StateStorage = {
7681
},
7782

7883
removeItem: async (name: string): Promise<void> => {
79-
if (typeof localStorage === 'undefined') return
84+
if (typeof window === 'undefined') return
85+
await awaitMigration()
86+
8087
try {
8188
await del(name)
8289
} catch (error) {

0 commit comments

Comments
 (0)