fix: improve error handling and add rollback test

- Add try-catch isolation in restoreOptimisticState to prevent one bad
  transaction from breaking all restoration
- Add defensive null check for mutation.collection in cleanup methods
- Add test for rollback of restored optimistic state on permanent failure

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: KyleAMathews

packages/offline-transactions/src/OfflineExecutor.ts

@@ -509,6 +509,11 @@ export class OfflineExecutor {
     // Remove from each collection's transaction map and recompute
     const touchedCollections = new Set<string>()
     for (const mutation of restorationTx.mutations) {
+      // Defensive check for corrupted deserialized data
+      // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+      if (!mutation.collection) {
+        continue
+      }
       const collectionId = mutation.collection.id
       if (touchedCollections.has(collectionId)) {
         continue

packages/offline-transactions/src/executor/TransactionExecutor.ts

@@ -260,36 +260,48 @@ export class TransactionExecutor {
         continue
       }
 
-      // Create a restoration transaction that holds mutations for optimistic state display.
-      // It will never commit - the real mutation is handled by the offline executor.
-      const restorationTx = createTransaction({
-        id: offlineTx.id,
-        autoCommit: false,
-        mutationFn: async () => {},
-      })
-
-      restorationTx.applyMutations(offlineTx.mutations)
+      try {
+        // Create a restoration transaction that holds mutations for optimistic state display.
+        // It will never commit - the real mutation is handled by the offline executor.
+        const restorationTx = createTransaction({
+          id: offlineTx.id,
+          autoCommit: false,
+          mutationFn: async () => {},
+        })
+
+        restorationTx.applyMutations(offlineTx.mutations)
+
+        // Register with each affected collection's state manager
+        const touchedCollections = new Set<string>()
+        for (const mutation of offlineTx.mutations) {
+          // Defensive check for corrupted deserialized data
+          // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+          if (!mutation.collection) {
+            continue
+          }
+          const collectionId = mutation.collection.id
+          if (touchedCollections.has(collectionId)) {
+            continue
+          }
+          touchedCollections.add(collectionId)
 
-      // Register with each affected collection's state manager
-      const touchedCollections = new Set<string>()
-      for (const mutation of offlineTx.mutations) {
-        const collectionId = mutation.collection.id
-        if (touchedCollections.has(collectionId)) {
-          continue
+          mutation.collection._state.transactions.set(
+            restorationTx.id,
+            restorationTx,
+          )
+          mutation.collection._state.recomputeOptimisticState(true)
         }
-        touchedCollections.add(collectionId)
 
-        mutation.collection._state.transactions.set(
-          restorationTx.id,
+        this.offlineExecutor.registerRestorationTransaction(
+          offlineTx.id,
           restorationTx,
         )
-        mutation.collection._state.recomputeOptimisticState(true)
+      } catch (error) {
+        console.warn(
+          `Failed to restore optimistic state for transaction ${offlineTx.id}:`,
+          error,
+        )
       }
-
-      this.offlineExecutor.registerRestorationTransaction(
-        offlineTx.id,
-        restorationTx,
-      )
     }
   }
 

packages/offline-transactions/tests/offline-e2e.test.ts

@@ -582,4 +582,85 @@ describe(`offline executor end-to-end`, () => {
 
     secondEnv.executor.dispose()
   })
+
+  it(`should rollback restored optimistic state on permanent failure after page refresh`, async () => {
+    // This test verifies that optimistic state restored from persisted transactions
+    // is properly rolled back when the transaction fails permanently (NonRetriableError).
+    //
+    // Scenario: User creates transaction offline, refreshes page, transaction fails permanently
+    // Expected: Optimistic state should be visible initially, then removed after failure
+
+    const storage = new FakeStorageAdapter()
+
+    // First environment: Create a transaction that gets stuck (simulating offline)
+    const mutationPromise = () => new Promise<void>(() => {})
+
+    const firstEnv = createTestOfflineEnvironment({
+      storage,
+      mutationFn: async () => {
+        await mutationPromise()
+        throw new Error(`Should not reach here in first env`)
+      },
+    })
+
+    await firstEnv.waitForLeader()
+
+    const offlineTx = firstEnv.executor.createOfflineTransaction({
+      mutationFnName: firstEnv.mutationFnName,
+      autoCommit: false,
+    })
+
+    // Apply optimistic update
+    offlineTx.mutate(() => {
+      firstEnv.collection.insert({
+        id: `ghost-item`,
+        value: `will-fail`,
+        completed: false,
+        updatedAt: new Date(),
+      })
+    })
+
+    // Verify the optimistic update is visible
+    expect(firstEnv.collection.get(`ghost-item`)?.value).toBe(`will-fail`)
+
+    // Start commit - it will persist to outbox
+    offlineTx.commit()
+
+    // Wait for the transaction to be persisted
+    await waitUntil(async () => {
+      const pendingEntries = await firstEnv.executor.peekOutbox()
+      return pendingEntries.length === 1
+    }, 5000)
+
+    // Dispose first environment (simulating page refresh)
+    firstEnv.executor.dispose()
+
+    // Create a new environment where the mutation fails permanently
+    const secondEnv = createTestOfflineEnvironment({
+      storage,
+      mutationFn: () => {
+        throw new NonRetriableError(`Server rejected mutation permanently`)
+      },
+    })
+
+    await secondEnv.waitForLeader()
+
+    // Optimistic state should be restored immediately after initialization
+    expect(secondEnv.collection.get(`ghost-item`)?.value).toBe(`will-fail`)
+
+    // Wait for the transaction to fail and be removed from outbox
+    await waitUntil(async () => {
+      const entries = await secondEnv.executor.peekOutbox()
+      return entries.length === 0
+    }, 5000)
+
+    // After permanent failure, the optimistic state should be rolled back
+    // The item should no longer exist in the collection
+    expect(secondEnv.collection.get(`ghost-item`)).toBeUndefined()
+
+    // And it should not exist on the server either
+    expect(secondEnv.serverState.get(`ghost-item`)).toBeUndefined()
+
+    secondEnv.executor.dispose()
+  })
 })