fix(undo-redo): evict furthest-from-use first so redo order survives trimming

Review caught that ordering eviction purely by createdAt is correct for the
undo stack but inverted for redo. Both stacks are consumed from their end,
so redo's next entry is its oldest, and trimming oldest-first removed the
operation redo needed next while keeping the later ones that depend on it.
After a reload redo could skip a step or replay against the wrong graph.

Evict by depth from the end of each array instead, which drops the front
first and matches the capacity policy that keeps the tail via slice(-capacity).
createdAt now only breaks ties.

Also type the test fixtures against PersistedUndoRedoState and OperationEntry
instead of any, and add a regression test asserting the next redo operation
survives while the front of the redo stack is evicted.

Author: rohitsudhakar1

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

@@ -21,12 +21,13 @@ import {
   createUpdateParentEntry,
 } from '@sim/testing'
 import { beforeEach, describe, expect, it } from 'vitest'
+import type { PersistedUndoRedoState } from '@/stores/undo-redo/store'
 import {
   runWithUndoRedoRecordingSuspended,
   trimPersistedStateToBudget,
   useUndoRedoStore,
 } from '@/stores/undo-redo/store'
-import type { UpdateParentOperation } from '@/stores/undo-redo/types'
+import type { OperationEntry, UpdateParentOperation } from '@/stores/undo-redo/types'
 
 describe('useUndoRedoStore', () => {
   const workflowId = 'wf-test'
@@ -827,66 +828,80 @@ describe('useUndoRedoStore', () => {
   })
 
   describe('trimPersistedStateToBudget', () => {
-    const entryOfSize = (createdAt: number, chars: number) => ({
-      id: `e-${createdAt}`,
-      createdAt,
-      operation: { id: `op-${createdAt}`, data: { blob: 'x'.repeat(chars) } },
-      inverse: { id: `inv-${createdAt}`, data: {} },
+    const entryOfSize = (createdAt: number, chars: number): OperationEntry =>
+      ({
+        id: `e-${createdAt}`,
+        createdAt,
+        operation: { id: `op-${createdAt}`, data: { blob: 'x'.repeat(chars) } },
+        inverse: { id: `inv-${createdAt}`, data: {} },
+      }) as unknown as OperationEntry
+
+    const persisted = (stacks: PersistedUndoRedoState['stacks']): PersistedUndoRedoState => ({
+      capacity: 100,
+      stacks,
     })
 
+    const createdAtsOf = (state: PersistedUndoRedoState): number[] =>
+      Object.values(state.stacks)
+        .flatMap((stack) => [...stack.undo, ...stack.redo])
+        .map((entry) => entry.createdAt)
+        .sort((x, y) => x - y)
+
     it('returns the state unchanged when already within budget', () => {
-      const state = {
-        capacity: 100,
-        stacks: { 'wf:user': { undo: [entryOfSize(1, 10)], redo: [], lastUpdated: 1 } },
-      } as any
+      const state = persisted({
+        'wf:user': { undo: [entryOfSize(1, 10)], redo: [], lastUpdated: 1 },
+      })
       expect(trimPersistedStateToBudget(state, 1024 * 1024)).toBe(state)
     })
 
-    it('evicts oldest-first across stacks until under budget', () => {
-      const state = {
-        capacity: 100,
-        stacks: {
-          a: {
-            undo: [entryOfSize(1, 5000), entryOfSize(4, 5000)],
-            redo: [entryOfSize(2, 5000)],
-            lastUpdated: 4,
-          },
-          b: { undo: [entryOfSize(3, 5000)], redo: [], lastUpdated: 3 },
+    it('evicts the entries furthest from the next use until under budget', () => {
+      const state = persisted({
+        a: {
+          undo: [entryOfSize(1, 5000), entryOfSize(4, 5000)],
+          redo: [entryOfSize(2, 5000)],
+          lastUpdated: 4,
         },
-      } as any
+        b: { undo: [entryOfSize(3, 5000)], redo: [], lastUpdated: 3 },
+      })
 
       const budget = 12_000
       const trimmed = trimPersistedStateToBudget(state, budget)
 
       expect(JSON.stringify(trimmed).length).toBeLessThanOrEqual(budget)
-      // The oldest entries (createdAt 1, 2) are gone; the newest (3, 4) survive.
-      const survivors = Object.values(trimmed.stacks)
-        .flatMap((s: any) => [...s.undo, ...s.redo])
-        .map((e: any) => e.createdAt)
-        .sort((x, y) => x - y)
-      expect(survivors).toEqual([3, 4])
+      expect(createdAtsOf(trimmed)).toEqual([3, 4])
     })
 
-    it('does not mutate the input state', () => {
-      const state = {
-        capacity: 100,
-        stacks: {
-          a: { undo: [entryOfSize(1, 5000), entryOfSize(2, 5000)], redo: [], lastUpdated: 2 },
+    it('keeps the next redo operation and evicts from the front of the redo stack', () => {
+      // redo is replayed from the end, so redo[length - 1] (createdAt 1) is next.
+      const state = persisted({
+        a: {
+          undo: [],
+          redo: [entryOfSize(3, 5000), entryOfSize(2, 5000), entryOfSize(1, 5000)],
+          lastUpdated: 3,
         },
-      } as any
+      })
+
+      const trimmed = trimPersistedStateToBudget(state, 11_000)
+      const redo = trimmed.stacks.a.redo
+
+      expect(redo.map((entry) => entry.createdAt)).toEqual([2, 1])
+      expect(redo[redo.length - 1].createdAt).toBe(1)
+    })
+
+    it('does not mutate the input state', () => {
+      const state = persisted({
+        a: { undo: [entryOfSize(1, 5000), entryOfSize(2, 5000)], redo: [], lastUpdated: 2 },
+      })
 
       trimPersistedStateToBudget(state, 6000)
       expect(state.stacks.a.undo).toHaveLength(2)
     })
 
     it('drops stacks emptied by eviction', () => {
-      const state = {
-        capacity: 100,
-        stacks: {
-          old: { undo: [entryOfSize(1, 8000)], redo: [], lastUpdated: 1 },
-          fresh: { undo: [entryOfSize(2, 100)], redo: [], lastUpdated: 2 },
-        },
-      } as any
+      const state = persisted({
+        old: { undo: [entryOfSize(1, 8000)], redo: [], lastUpdated: 1 },
+        fresh: { undo: [entryOfSize(2, 100)], redo: [], lastUpdated: 2 },
+      })
 
       const trimmed = trimPersistedStateToBudget(state, 4000)
       expect(trimmed.stacks.old).toBeUndefined()

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

@@ -138,61 +138,83 @@ function isOperationApplicable(
   }
 }
 
-type PersistedUndoRedoState = Pick<UndoRedoState, 'stacks' | 'capacity'>
+/** The slice of {@link UndoRedoState} that `persist` writes to storage. */
+export type PersistedUndoRedoState = Pick<UndoRedoState, 'stacks' | 'capacity'>
 
+/** Serialized length of `value`, or 0 when it is not serializable. */
 function serializedLength(value: unknown): number {
   return JSON.stringify(value)?.length ?? 0
 }
 
 /**
- * Returns a copy of the persisted state trimmed so its serialized size fits
- * `maxBytes`, evicting the oldest operations (by `createdAt`) across every stack
- * first and dropping any stack left empty. The input is never mutated, so the
- * live in-memory history is unaffected — only what gets written to storage shrinks.
+ * Trims a copy of the persisted state so its serialized size fits `maxBytes`.
+ *
+ * Both stacks are consumed from their end: `undo()` takes `undo[undo.length - 1]`
+ * and `redo()` takes `redo[redo.length - 1]`. Eviction therefore removes entries
+ * furthest from the next use first, which is the front of each array, matching the
+ * capacity policy that keeps the tail via `slice(-capacity)`. Ordering purely by
+ * `createdAt` would be correct for `undo` but inverted for `redo`, whose next entry
+ * is its oldest, and would drop the operation redo needs next while keeping the
+ * later ones that depend on it.
+ *
+ * Any stack emptied by eviction is removed so its key and overhead are reclaimed.
+ * The input is never mutated, so the live in-memory history is unaffected and only
+ * what gets written to storage shrinks.
+ *
+ * @param state - The persisted slice to trim. Left untouched.
+ * @param maxBytes - Serialized-size ceiling for the returned state.
+ * @returns `state` itself when already within budget, otherwise a trimmed copy.
  */
 export function trimPersistedStateToBudget(
   state: PersistedUndoRedoState,
   maxBytes: number
 ): PersistedUndoRedoState {
   if (serializedLength(state) <= maxBytes) return state
 
-  // Clone stacks (and their arrays) so eviction never touches the live state.
   const stacks: PersistedUndoRedoState['stacks'] = {}
   for (const [key, stack] of Object.entries(state.stacks)) {
     stacks[key] = { ...stack, undo: [...stack.undo], redo: [...stack.redo] }
   }
 
-  // Every removable entry, oldest first.
   const removable = Object.entries(stacks)
-    .flatMap(([key, stack]) => [
-      ...stack.undo.map((entry) => ({ key, list: 'undo' as const, entry })),
-      ...stack.redo.map((entry) => ({ key, list: 'redo' as const, entry })),
-    ])
-    .sort((a, b) => a.entry.createdAt - b.entry.createdAt)
+    .flatMap(([key, stack]) =>
+      (['undo', 'redo'] as const).flatMap((list) =>
+        stack[list].map((entry, index) => ({
+          key,
+          list,
+          entry,
+          depth: stack[list].length - 1 - index,
+        }))
+      )
+    )
+    .sort((a, b) => b.depth - a.depth || a.entry.createdAt - b.entry.createdAt)
 
   const drop = ({ key, list, entry }: (typeof removable)[number]): void => {
     const stack = stacks[key]
     if (!stack) return
     const arr = stack[list]
     const idx = arr.indexOf(entry)
     if (idx !== -1) arr.splice(idx, 1)
-    // Reclaim the key/overhead of a stack emptied by eviction.
     if (stack.undo.length === 0 && stack.redo.length === 0) delete stacks[key]
   }
 
-  // Pass 1: estimate-based bulk eviction. Subtracting each entry's own serialized
-  // length (plus a separating comma) avoids re-serializing the whole payload on
-  // every eviction, which would be O(n²) over the megabytes involved.
+  /*
+   * Pass 1 evicts in bulk against an estimate. Subtracting each entry's own
+   * serialized length (plus a separating comma) avoids re-serializing the whole
+   * payload on every eviction, which would be O(n^2) over the megabytes involved.
+   */
   let approxBytes = serializedLength(state)
   let i = 0
   for (; i < removable.length && approxBytes > maxBytes; i++) {
     approxBytes -= serializedLength(removable[i].entry) + 1
     drop(removable[i])
   }
 
-  // Pass 2: the estimate can undershoot the true reduction (structural overhead it
-  // doesn't attribute to entries), so verify exactly and keep dropping the oldest
-  // survivors until the invariant holds. In practice this runs zero or one times.
+  /*
+   * The estimate can undershoot the true reduction because it does not attribute
+   * structural overhead to entries, so pass 2 verifies exactly and keeps dropping
+   * survivors until the invariant holds. In practice it runs zero or one times.
+   */
   while (
     i < removable.length &&
     serializedLength({ stacks, capacity: state.capacity }) > maxBytes