Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 149 additions & 0 deletions apps/sim/stores/undo-redo/storage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/**
* @vitest-environment node
*/
import { beforeEach, describe, expect, it, vi } from 'vitest'

const { idbStore, idbGet, idbSet, idbDel } = vi.hoisted(() => {
const store = new Map<string, unknown>()
return {
idbStore: store,
idbGet: vi.fn(async (key: string) => store.get(key) ?? undefined),
idbSet: vi.fn(async (key: string, value: unknown) => {
store.set(key, value)
}),
idbDel: vi.fn(async (key: string) => {
store.delete(key)
}),
}
})

vi.mock('idb-keyval', () => ({
get: idbGet,
set: idbSet,
del: idbDel,
}))

const STORE_KEY = 'workflow-undo-redo'
const MIGRATION_KEY = 'workflow-undo-redo-migrated'

async function loadFreshModule() {
vi.resetModules()
return await import('@/stores/undo-redo/storage')
}

describe('undo-redo IndexedDB storage adapter', () => {
beforeEach(() => {
idbStore.clear()
idbGet.mockClear()
idbSet.mockClear()
idbDel.mockClear()
localStorage.clear()
vi.mocked(localStorage.getItem).mockClear()
vi.mocked(localStorage.setItem).mockClear()
vi.mocked(localStorage.removeItem).mockClear()
})

describe('migration', () => {
it('copies localStorage data into IndexedDB and removes the localStorage key on first load', async () => {
const legacyPayload = JSON.stringify({ state: { stacks: {} }, version: 0 })
localStorage.setItem(STORE_KEY, legacyPayload)
idbSet.mockClear()

const { migrationReady } = await loadFreshModule()
await migrationReady

expect(idbSet).toHaveBeenCalledWith(STORE_KEY, legacyPayload)
expect(idbStore.get(STORE_KEY)).toBe(legacyPayload)
expect(localStorage.getItem(STORE_KEY)).toBeNull()
expect(idbStore.get(MIGRATION_KEY)).toBe(true)
})

it('skips data copy when localStorage is empty but still marks migration complete', async () => {
const { migrationReady } = await loadFreshModule()
await migrationReady

expect(idbSet).toHaveBeenCalledWith(MIGRATION_KEY, true)
expect(idbSet).not.toHaveBeenCalledWith(STORE_KEY, expect.anything())
expect(idbStore.get(MIGRATION_KEY)).toBe(true)
})

it('does not re-run when MIGRATION_KEY is already set', async () => {
idbStore.set(MIGRATION_KEY, true)
const legacyPayload = JSON.stringify({ state: { stacks: { foo: {} } }, version: 0 })
localStorage.setItem(STORE_KEY, legacyPayload)

const { migrationReady } = await loadFreshModule()
await migrationReady

expect(idbSet).not.toHaveBeenCalledWith(STORE_KEY, expect.anything())
expect(localStorage.getItem(STORE_KEY)).toBe(legacyPayload)
})

it('does not throw when IndexedDB set fails — leaves localStorage intact for retry', async () => {
idbSet.mockRejectedValueOnce(new Error('idb write failed'))
const legacyPayload = JSON.stringify({ state: { stacks: {} }, version: 0 })
localStorage.setItem(STORE_KEY, legacyPayload)

const { migrationReady } = await loadFreshModule()
await expect(migrationReady).resolves.toBeUndefined()

expect(localStorage.getItem(STORE_KEY)).toBe(legacyPayload)
})
})

describe('storage adapter', () => {
it('getItem awaits migration completion before reading', async () => {
const legacyPayload = JSON.stringify({ state: { stacks: { a: {} } }, version: 0 })
localStorage.setItem(STORE_KEY, legacyPayload)

const { indexedDBStorage, migrationReady } = await loadFreshModule()
const readPromise = indexedDBStorage.getItem(STORE_KEY)
await migrationReady
const value = await readPromise

expect(value).toBe(legacyPayload)
})

it('getItem returns null when key is absent', async () => {
const { indexedDBStorage, migrationReady } = await loadFreshModule()
await migrationReady

const value = await indexedDBStorage.getItem('does-not-exist')
expect(value).toBeNull()
})

it('setItem writes through to IndexedDB', async () => {
const { indexedDBStorage, migrationReady } = await loadFreshModule()
await migrationReady

await indexedDBStorage.setItem(STORE_KEY, 'new-value')
expect(idbStore.get(STORE_KEY)).toBe('new-value')
})

it('setItem swallows IndexedDB errors so the store never crashes the app', async () => {
const { indexedDBStorage, migrationReady } = await loadFreshModule()
await migrationReady

idbSet.mockRejectedValueOnce(new Error('idb quota'))
await expect(indexedDBStorage.setItem(STORE_KEY, 'x')).resolves.toBeUndefined()
})

it('removeItem deletes from IndexedDB', async () => {
const { indexedDBStorage, migrationReady } = await loadFreshModule()
await migrationReady
Comment on lines +126 to +133
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing error-swallowing test for removeItem

getItem and setItem each have a dedicated test that verifies IndexedDB errors are caught and don't propagate. removeItem has only a success-path test. A test that mocks idbDel to reject would mirror the existing pattern and guard against a future regression where the catch block is accidentally removed.

idbStore.set(STORE_KEY, 'present')

await indexedDBStorage.removeItem(STORE_KEY)
expect(idbStore.has(STORE_KEY)).toBe(false)
})

it('getItem swallows IndexedDB read errors and returns null', async () => {
const { indexedDBStorage, migrationReady } = await loadFreshModule()
await migrationReady

idbGet.mockRejectedValueOnce(new Error('idb read failed'))
const value = await indexedDBStorage.getItem(STORE_KEY)
expect(value).toBeNull()
})
})
})
86 changes: 86 additions & 0 deletions apps/sim/stores/undo-redo/storage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { createLogger } from '@sim/logger'
import { del, get, set } from 'idb-keyval'
import type { StateStorage } from 'zustand/middleware'

const logger = createLogger('UndoRedoStorage')

const STORE_KEY = 'workflow-undo-redo'
const MIGRATION_KEY = 'workflow-undo-redo-migrated'

let migrationPromiseInternal: Promise<void> | null = null

/**
* Migrates existing undo/redo data from localStorage to IndexedDB.
* Runs once on first load; subsequent loads short-circuit on MIGRATION_KEY.
*
* On success the localStorage key is removed, freeing origin storage quota
* for the other persisted Zustand stores that share it.
*/
async function migrateFromLocalStorage(): Promise<void> {
if (typeof localStorage === 'undefined') return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The SSR guard throughout this file checks typeof localStorage === 'undefined', but the sibling terminal/console/storage.ts (and general Next.js convention) uses typeof window === 'undefined'. Both are equivalent in practice today, but diverging from the established pattern makes it harder to do a grep-based audit of all SSR guards in the codebase.

Suggested change
async function migrateFromLocalStorage(): Promise<void> {
if (typeof localStorage === 'undefined') return
async function migrateFromLocalStorage(): Promise<void> {
if (typeof window === 'undefined') return

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


try {
const migrated = await get<boolean>(MIGRATION_KEY)
if (migrated) return
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orphan localStorage not cleared

Medium Severity

The migrateFromLocalStorage function short-circuits if MIGRATION_KEY is already set in IndexedDB. This prevents localStorage.removeItem(STORE_KEY) from executing, allowing the workflow-undo-redo key to persist in localStorage and consume origin storage quota.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d9201d8. Configure here.


const localData = localStorage.getItem(STORE_KEY)
if (localData) {
await set(STORE_KEY, localData)
localStorage.removeItem(STORE_KEY)
logger.info('Migrated undo-redo store from localStorage to IndexedDB')
}

await set(MIGRATION_KEY, true)
} catch (error) {
logger.warn('Migration from localStorage failed', { error })
}
}

if (typeof localStorage !== 'undefined') {
migrationPromiseInternal = migrateFromLocalStorage().finally(() => {
migrationPromiseInternal = null
})
}

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

export const indexedDBStorage: StateStorage = {
getItem: async (name: string): Promise<string | null> => {
if (typeof localStorage === 'undefined') return null

if (migrationPromiseInternal) {
await migrationPromiseInternal
}

try {
const value = await get<string>(name)
return value ?? null
} catch (error) {
logger.warn('IndexedDB read failed', { name, error })
return null
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads ignore legacy localStorage

Medium Severity

After a failed localStorage→IndexedDB migration, indexedDBStorage.getItem still reads only IndexedDB and returns null even when the legacy workflow-undo-redo payload remains in localStorage. The previous safeStorageAdapter read from localStorage directly, so persisted undo stacks could still hydrate on that load.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d9201d8. Configure here.

},

setItem: async (name: string, value: string): Promise<void> => {
if (typeof localStorage === 'undefined') return
try {
await set(name, value)
Comment on lines +119 to +125
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 setItem writes to IndexedDB without waiting for migration to finish. In the normal Zustand persist flow this is safe (Zustand calls getItem before any setItem), but if the adapter is ever called directly while migration is still in progress, a newly-written state snapshot could be silently overwritten when migration copies the old localStorage value. Adding the same migration gate that getItem already has makes the contract explicit and future-proof.

Suggested change
setItem: async (name: string, value: string): Promise<void> => {
if (typeof localStorage === 'undefined') return
try {
await set(name, value)
setItem: async (name: string, value: string): Promise<void> => {
if (typeof localStorage === 'undefined') return
if (migrationPromiseInternal) {
await migrationPromiseInternal
}
try {
await set(name, value)

} catch (error) {
logger.warn('IndexedDB write failed', { name, error })
}
},
Comment thread
cursor[bot] marked this conversation as resolved.

removeItem: async (name: string): Promise<void> => {
if (typeof localStorage === 'undefined') return
try {
await del(name)
} catch (error) {
logger.warn('IndexedDB delete failed', { name, error })
}
},
}
38 changes: 2 additions & 36 deletions apps/sim/stores/undo-redo/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { UNDO_REDO_OPERATIONS } from '@sim/realtime-protocol/constants'
import type { Edge } from 'reactflow'
import { create } from 'zustand'
import { createJSONStorage, persist } from 'zustand/middleware'
import { indexedDBStorage } from '@/stores/undo-redo/storage'
import type {
BatchAddBlocksOperation,
BatchAddEdgesOperation,
Expand Down Expand Up @@ -47,41 +48,6 @@ function getStackKey(workflowId: string, userId: string): string {
return `${workflowId}:${userId}`
}

/**
* Custom storage adapter for Zustand's persist middleware.
* We need this wrapper to gracefully handle 'QuotaExceededError' when localStorage is full,
* Without this, the default storage engine would throw and crash the application.
* and to properly handle SSR/Node.js environments.
*/
const safeStorageAdapter = {
getItem: (name: string): string | null => {
if (typeof localStorage === 'undefined') return null
try {
return localStorage.getItem(name)
} catch (e) {
logger.warn('Failed to read from localStorage', e)
return null
}
},
setItem: (name: string, value: string): void => {
if (typeof localStorage === 'undefined') return
try {
localStorage.setItem(name, value)
} catch (e) {
// Log warning but don't crash - this handles QuotaExceededError
logger.warn('Failed to save to localStorage', e)
}
},
removeItem: (name: string): void => {
if (typeof localStorage === 'undefined') return
try {
localStorage.removeItem(name)
} catch (e) {
logger.warn('Failed to remove from localStorage', e)
}
},
}

function isOperationApplicable(
operation: Operation,
graph: { blocksById: Record<string, BlockState>; edgesById: Record<string, Edge> }
Expand Down Expand Up @@ -501,7 +467,7 @@ export const useUndoRedoStore = create<UndoRedoState>()(
}),
{
name: 'workflow-undo-redo',
storage: createJSONStorage(() => safeStorageAdapter),
storage: createJSONStorage(() => indexedDBStorage),
Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
partialize: (state) => ({
stacks: state.stacks,
capacity: state.capacity,
Expand Down