feat(undo-redo): migrate persistence from localStorage to IndexedDB#4738
feat(undo-redo): migrate persistence from localStorage to IndexedDB#4738Sprexatura wants to merge 5 commits into
Conversation
Add apps/sim/stores/undo-redo/storage.ts wrapping idb-keyval, mirroring the pattern of apps/sim/stores/terminal/console/storage.ts. Includes a one-time localStorage -> IndexedDB migration that runs on first module load and removes the legacy localStorage key after copying. storage.test.ts covers migration idempotency, graceful failure on IndexedDB errors, and basic get/set/remove behavior (10 cases). No behavioral change in this commit -- the adapter is unused until the follow-up swaps the store's persist middleware. Refs simstudioai#4737 Signed-off-by: JaeHyung Jang <jaehyung.jang@navercorp.com>
Replace inline safeStorageAdapter with indexedDBStorage. Workflow undo/ redo persistence moves from localStorage (~5MB origin cap) to IndexedDB (~GB), eliminating QuotaExceededError that can surface from any small persisted Zustand store (notification-storage, panel-editor-state, etc.) once workflow-undo-redo occupies the bulk of the origin's localStorage budget. Same pattern as PR simstudioai#2812 (terminal-console -> IndexedDB) and aligns with PR simstudioai#497's policy of minimizing localStorage usage. The earlier safeStorageAdapter (PR simstudioai#2079) silently swallowed quota throws in this store but did not free the origin budget that the other small UI stores share, leaving them to throw uncaught QuotaExceededError. Fixes simstudioai#4737 Signed-off-by: JaeHyung Jang <jaehyung.jang@navercorp.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview On first load, a one-time migration copies any existing The undo/redo Zustand store drops the inline Reviewed by Cursor Bugbot for commit 2a8bf5f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
|
||
| try { | ||
| const migrated = await get<boolean>(MIGRATION_KEY) | ||
| if (migrated) return |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit d9201d8. Configure here.
| } catch (error) { | ||
| logger.warn('IndexedDB read failed', { name, error }) | ||
| return null | ||
| } |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit d9201d8. Configure here.
Greptile SummaryThis PR migrates the
Confidence Score: 4/5Safe to merge; the migration is idempotent and adapter errors are all caught, so there is no data-loss path. The migration logic is sound — IDB writes are awaited before the localStorage key is removed, the sentinel prevents re-runs, and failures leave localStorage intact for a retry. The only open questions are a minor SSR-guard naming inconsistency with the sibling storage file and the absence of a migration-gate in apps/sim/stores/undo-redo/storage.ts — the Important Files Changed
Sequence DiagramsequenceDiagram
participant Module as storage.ts (module load)
participant IDB as IndexedDB (idb-keyval)
participant LS as localStorage
participant Store as useUndoRedoStore (Zustand persist)
Module->>IDB: get(MIGRATION_KEY)
alt already migrated
IDB-->>Module: true → return early
else first load
IDB-->>Module: undefined
Module->>LS: getItem("workflow-undo-redo")
alt data exists
LS-->>Module: legacyPayload
Module->>IDB: set("workflow-undo-redo", legacyPayload)
Module->>LS: removeItem("workflow-undo-redo")
end
Module->>IDB: set(MIGRATION_KEY, true)
end
Note over Module: migrationPromiseInternal resolves → set to null
Store->>Module: indexedDBStorage.getItem("workflow-undo-redo")
Module->>Module: await migrationPromiseInternal (if still pending)
Module->>IDB: get("workflow-undo-redo")
IDB-->>Store: persisted JSON string (or null)
Store->>Module: indexedDBStorage.setItem("workflow-undo-redo", newState)
Module->>IDB: set("workflow-undo-redo", newState)
Reviews (1): Last reviewed commit: "feat(undo-redo): migrate persistence fro..." | Re-trigger Greptile |
| async function migrateFromLocalStorage(): Promise<void> { | ||
| if (typeof localStorage === 'undefined') return |
There was a problem hiding this comment.
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.
| 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!
| setItem: async (name: string, value: string): Promise<void> => { | ||
| if (typeof localStorage === 'undefined') return | ||
| try { | ||
| await set(name, value) |
There was a problem hiding this comment.
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.
| 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) |
|
|
||
| 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 |
There was a problem hiding this comment.
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.
- 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>
`clearUserData` previously cleared `localStorage` only. After moving workflow-undo-redo persistence to IndexedDB, undo/redo history could survive across user sessions on the same device. Add `clearPersistedUndoRedo()` to the adapter and invoke it from `clearUserData` so sign-out removes the IndexedDB entry as well. The migration flag is left intact so the one-time migration does not re-run on the next sign-in. Signed-off-by: JaeHyung Jang <jaehyung.jang@navercorp.com>
- 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>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2a8bf5f. Configure here.
| logger.warn('Failed to clear persisted undo-redo', { error }) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Sign-out races undo hydration
Medium Severity
clearUserData deletes the persisted undo payload from IndexedDB while Zustand may still be rehydrating over async storage. If the initial getItem read started before deletion but completes afterward with the previous snapshot, persist can merge that undo history back into the in-memory store after storage was cleared. Sign-out navigates to login without a full reload, so the next session in the same tab can retain the prior user’s stacks in memory even though IndexedDB was wiped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2a8bf5f. Configure here.


Summary
Workflow undo/redo persistence currently lives in
localStorageand stores operation snapshots that include full block state. Active editing of multiple workflows quickly fills the per-stack capacity with snapshots that can be several KB each, occupying the bulk of the origin's ~5 MBlocalStoragebudget. Once the budget is exhausted, any other persisted Zustand store whosesetItemis called next throwsQuotaExceededError.This PR moves
workflow-undo-redoto IndexedDB using the sameidb-keyvaladapter pattern asterminal-console-store(#2812), aligning with the policy stated in #497 to minimizelocalStorageusage.The earlier
safeStorageAdapter(#2079) silently swallowed quota throws inside this store but did not free the origin budget that other small UI stores share, leaving them to throw uncaughtQuotaExceededError. Migrating to IndexedDB removes the underlying contention.Changes
apps/sim/stores/undo-redo/storage.tswrappingidb-keyval, mirroringapps/sim/stores/terminal/console/storage.ts. Includes a one-timelocalStorage→ IndexedDB migration that copies any existing data and removes the legacylocalStoragekey on first module load.apps/sim/stores/undo-redo/storage.test.tscovering migration idempotency, graceful failure on IndexedDB errors, and basic get/set/remove behavior (10 cases).apps/sim/stores/undo-redo/store.ts: drop the inlinesafeStorageAdapterand use the newindexedDBStorage.Structured as two commits — first commit adds the adapter with no behavioral change, second commit swaps the store's persist middleware.
Verification
Detailed reproduction steps are in #4737. Local verification:
setItemon another small store (e.g., dismiss a notification) —QuotaExceededErrorthrown.workflow-undo-redopayload to IndexedDB and removing thelocalStoragekey.Fixes #4737