fix: roll review notes back to the persisted list

Rollback restored the pre-mutation list, which is itself optimistic when
writes queue up, so two failed queued writes left notes on screen that
never reached disk. Track the last durably persisted list per queue and
roll back to that instead.

Also enforce the DiffComment invariants in DiffCommentSchema (non-empty
identity fields, trimmed body, positive sentAt, startLine <= lineNumber)
and allow lineNumber 0 so mobile file-level notes are no longer rejected,
and parse persisted diffComments in normalizeFolderWorkspaces so
malformed stored notes are dropped instead of rehydrated.

Co-authored-by: Orca <help@stably.ai>

src/renderer/src/store/slices/diffComments.ts

@@ -154,36 +154,55 @@ function settingsForWorktreeOwner(state: AppState, worktreeId: string): AppState
 }
 
 // Why: IPC writes aren't ordered, so serialize per worktree to stop an older snapshot from overwriting a newer one on disk.
-const persistQueueByWorktree = new Map<string, Promise<void>>()
+const persistQueueByWorktree = new Map<string, Promise<PersistOutcome>>()
 
-// Why: chain each write onto the prior promise so writes land in call order; both then handlers keep the chain alive past a failure.
+// Why: the list storage actually holds, so a failed write rolls back to disk rather than to an earlier optimistic list
+// that a preceding queued write may also have failed to persist. Cleared once the queue drains and state matches disk again.
+const durableCommentsByQueue = new Map<string, DiffComment[] | undefined>()
+
+type PersistOutcome =
+  | { ok: true }
+  | { ok: false; error: unknown; durable: DiffComment[] | undefined }
+
+// Why: chain each write onto the prior promise so writes land in call order; the chain resolves outcomes instead of
+// rejecting so a failure can carry the durable list back to its own caller without breaking the chain.
 // Why: queued work reads the latest list at dequeue time, and the returned promise settles for THIS write so callers can roll back.
 function enqueuePersist(
   worktreeId: string,
   get: () => AppState,
-  folderExecutionHostId?: ReturnType<typeof getExecutionHostIdForFolderWorkspace>
-): Promise<void> {
+  mutation: CommentMutation
+): Promise<PersistOutcome> {
+  const folderExecutionHostId = mutation.folderExecutionHostId
   const queueKey = folderExecutionHostId ? `${folderExecutionHostId}\0${worktreeId}` : worktreeId
-  const prior = persistQueueByWorktree.get(queueKey) ?? Promise.resolve()
-  const run = async (): Promise<void> => {
+  // Why: an empty queue means nothing is in flight, so the pre-mutation list is what storage holds.
+  if (!durableCommentsByQueue.has(queueKey)) {
+    durableCommentsByQueue.set(queueKey, mutation.previous)
+  }
+  const prior =
+    persistQueueByWorktree.get(queueKey) ?? Promise.resolve<PersistOutcome>({ ok: true })
+  const run = async (): Promise<PersistOutcome> => {
     const scope = parseWorkspaceKey(worktreeId)
-    if (scope?.type === 'folder') {
-      const state = get()
-      const folderWorkspace = findFolderWorkspaceOwner(
-        state,
-        scope.folderWorkspaceId,
-        folderExecutionHostId
-      )
-      const latest = (folderWorkspace?.diffComments ?? []).map(normalizeDiffComment)
-      await persist(state, state.settings, worktreeId, latest, folderExecutionHostId)
-      return
-    }
-    const repoId = getRepoIdFromWorktreeId(worktreeId)
-    const repoList = get().worktreesByRepo[repoId]
-    const target = repoList?.find((w) => w.id === worktreeId)
-    const latest = (target?.diffComments ?? []).map(normalizeDiffComment)
     const state = get()
-    await persist(state, settingsForWorktreeOwner(state, worktreeId), worktreeId, latest)
+    const latest =
+      scope?.type === 'folder'
+        ? (
+            findFolderWorkspaceOwner(state, scope.folderWorkspaceId, folderExecutionHostId)
+              ?.diffComments ?? []
+          ).map(normalizeDiffComment)
+        : (
+            state.worktreesByRepo[getRepoIdFromWorktreeId(worktreeId)]?.find(
+              (w) => w.id === worktreeId
+            )?.diffComments ?? []
+          ).map(normalizeDiffComment)
+    try {
+      await (scope?.type === 'folder'
+        ? persist(state, state.settings, worktreeId, latest, folderExecutionHostId)
+        : persist(state, settingsForWorktreeOwner(state, worktreeId), worktreeId, latest))
+    } catch (error) {
+      return { ok: false, error, durable: durableCommentsByQueue.get(queueKey) }
+    }
+    durableCommentsByQueue.set(queueKey, latest)
+    return { ok: true }
   }
   const next = prior.then(run, run)
   persistQueueByWorktree.set(queueKey, next)
@@ -192,22 +211,42 @@ function enqueuePersist(
   const cleanup = (): void => {
     if (persistQueueByWorktree.get(queueKey) === next) {
       persistQueueByWorktree.delete(queueKey)
+      durableCommentsByQueue.delete(queueKey)
     }
   }
   next.then(cleanup, cleanup)
   return next
 }
 
+// Why: single commit path for every mutation so rollback always targets the durable list.
+async function commitMutation(
+  set: Parameters<StateCreator<AppState, [], [], DiffCommentsSlice>>[0],
+  get: () => AppState,
+  worktreeId: string,
+  mutation: CommentMutation
+): Promise<boolean> {
+  const outcome = await enqueuePersist(worktreeId, get, mutation)
+  if (outcome.ok) {
+    return true
+  }
+  console.error('Failed to persist diff comments:', outcome.error)
+  // Why: rollback's identity guard no-ops if a later mutation already replaced the list, so a newer write can't be lost.
+  rollback(set, worktreeId, outcome.durable, mutation.next, mutation.folderExecutionHostId)
+  return false
+}
+
+type CommentMutation = {
+  previous: DiffComment[] | undefined
+  next: DiffComment[]
+  folderExecutionHostId?: ReturnType<typeof getExecutionHostIdForFolderWorkspace>
+}
+
 // Why: derive the next list inside the `set` updater so concurrent writes can't clobber each other via a stale closure.
 function mutateComments(
   set: Parameters<StateCreator<AppState, [], [], DiffCommentsSlice>>[0],
   worktreeId: string,
   mutate: (existing: DiffComment[]) => DiffComment[] | null
-): {
-  previous: DiffComment[] | undefined
-  next: DiffComment[]
-  folderExecutionHostId?: ReturnType<typeof getExecutionHostIdForFolderWorkspace>
-} | null {
+): CommentMutation | null {
   const repoId = getRepoIdFromWorktreeId(worktreeId)
   let previous: DiffComment[] | undefined
   let next: DiffComment[] | null = null
@@ -330,17 +369,12 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return null
     }
-    try {
-      // Why: serialize through the per-worktree queue so concurrent writes can't land on disk out of call order.
-      await enqueuePersist(input.worktreeId, get, result.folderExecutionHostId)
-      get().recordFeatureInteraction?.('review-notes')
-      return comment
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      // Why: rollback's identity guard no-ops if a later mutation already replaced the list, so a newer write can't be lost.
-      rollback(set, input.worktreeId, result.previous, result.next, result.folderExecutionHostId)
+    // Why: serialize through the per-worktree queue so concurrent writes can't land on disk out of call order.
+    if (!(await commitMutation(set, get, input.worktreeId, result))) {
       return null
     }
+    get().recordFeatureInteraction?.('review-notes')
+    return comment
   },
 
   updateDiffComment: async (worktreeId, commentId, body) => {
@@ -377,14 +411,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
       // Why: comment vanished or the same body was already written between pre-check and set; treat as success so the editor closes.
       return true
     }
-    try {
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-      return true
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
-      return false
-    }
+    return commitMutation(set, get, worktreeId, result)
   },
 
   clearDeliveredDiffComments: async (worktreeId, comments) => {
@@ -403,15 +430,11 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return true
     }
-    try {
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-      get().recordFeatureInteraction?.('review-notes')
-      return true
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
+    if (!(await commitMutation(set, get, worktreeId, result))) {
       return false
     }
+    get().recordFeatureInteraction?.('review-notes')
+    return true
   },
 
   markDiffCommentsSent: async (worktreeId, commentIds, sentAt = Date.now()) => {
@@ -433,15 +456,11 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return true
     }
-    try {
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-      get().recordFeatureInteraction?.('review-notes')
-      return true
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
+    if (!(await commitMutation(set, get, worktreeId, result))) {
       return false
     }
+    get().recordFeatureInteraction?.('review-notes')
+    return true
   },
 
   deleteDiffComment: async (worktreeId, commentId) => {
@@ -452,13 +471,8 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return
     }
-    try {
-      // Why: serialize through the per-worktree queue so concurrent writes can't land out of call order.
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
-    }
+    // Why: serialize through the per-worktree queue so concurrent writes can't land out of call order.
+    await commitMutation(set, get, worktreeId, result)
   },
 
   clearDiffComments: async (worktreeId) => {
@@ -468,14 +482,7 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return true
     }
-    try {
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-      return true
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
-      return false
-    }
+    return commitMutation(set, get, worktreeId, result)
   },
 
   clearDiffCommentsForFile: async (worktreeId, filePath) => {
@@ -486,13 +493,6 @@ export const createDiffCommentsSlice: StateCreator<AppState, [], [], DiffComment
     if (!result) {
       return true
     }
-    try {
-      await enqueuePersist(worktreeId, get, result.folderExecutionHostId)
-      return true
-    } catch (err) {
-      console.error('Failed to persist diff comments:', err)
-      rollback(set, worktreeId, result.previous, result.next, result.folderExecutionHostId)
-      return false
-    }
+    return commitMutation(set, get, worktreeId, result)
   }
 })

src/renderer/src/store/slices/folder-workspace-diff-comments.test.ts

@@ -150,6 +150,94 @@ describe('folder workspace diff comments', () => {
     })
   })
 
+  it('rolls back to the persisted list when two queued writes fail', async () => {
+    const store = createTestStore()
+    const folderWorkspace = seedLocalFolderWorkspace(store)
+    const workspaceKey = folderWorkspaceKey(folderWorkspace.id)
+    const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
+    let releaseFirstWrite: (() => void) | undefined
+    const firstWrite = new Promise<void>((resolve) => {
+      releaseFirstWrite = resolve
+    })
+    folderWorkspacesUpdate.mockImplementation(async () => {
+      if (folderWorkspacesUpdate.mock.calls.length === 1) {
+        await firstWrite
+      }
+      throw new Error('disk full')
+    })
+
+    const addFirst = store.getState().addDiffComment({
+      worktreeId: workspaceKey,
+      filePath: 'README.md',
+      source: 'markdown',
+      lineNumber: 1,
+      body: 'first note',
+      side: 'modified'
+    })
+    await vi.waitFor(() => expect(folderWorkspacesUpdate).toHaveBeenCalledTimes(1))
+    const addSecond = store.getState().addDiffComment({
+      worktreeId: workspaceKey,
+      filePath: 'README.md',
+      source: 'markdown',
+      lineNumber: 2,
+      body: 'second note',
+      side: 'modified'
+    })
+    releaseFirstWrite?.()
+
+    await expect(Promise.all([addFirst, addSecond])).resolves.toEqual([null, null])
+    // Why: neither write reached disk, so the pre-mutation list is the only durable state.
+    expect(store.getState().getDiffComments(workspaceKey)).toEqual([])
+    consoleError.mockRestore()
+  })
+
+  it('rolls back to the last persisted list when a later queued write fails', async () => {
+    const store = createTestStore()
+    const folderWorkspace = seedLocalFolderWorkspace(store)
+    const workspaceKey = folderWorkspaceKey(folderWorkspace.id)
+    const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
+    let releaseFirstWrite: (() => void) | undefined
+    const firstWrite = new Promise<void>((resolve) => {
+      releaseFirstWrite = resolve
+    })
+    folderWorkspacesUpdate.mockImplementation(async ({ updates }) => {
+      if (folderWorkspacesUpdate.mock.calls.length === 1) {
+        await firstWrite
+        return { ...folderWorkspace, ...updates }
+      }
+      throw new Error('disk full')
+    })
+
+    const addFirst = store.getState().addDiffComment({
+      worktreeId: workspaceKey,
+      filePath: 'README.md',
+      source: 'markdown',
+      lineNumber: 1,
+      body: 'first note',
+      side: 'modified'
+    })
+    await vi.waitFor(() => expect(folderWorkspacesUpdate).toHaveBeenCalledTimes(1))
+    const addSecond = store.getState().addDiffComment({
+      worktreeId: workspaceKey,
+      filePath: 'README.md',
+      source: 'markdown',
+      lineNumber: 2,
+      body: 'second note',
+      side: 'modified'
+    })
+    releaseFirstWrite?.()
+
+    await expect(Promise.all([addFirst, addSecond])).resolves.toEqual([
+      expect.objectContaining({ body: 'first note' }),
+      null
+    ])
+    // Why: the first write persisted the first note, so rollback keeps it instead of reverting to the empty list.
+    expect(store.getState().getDiffComments(workspaceKey)).toEqual([
+      expect.objectContaining({ body: 'first note' })
+    ])
+    consoleError.mockRestore()
+  })
+
   it('keeps same-id writes scoped to their original hosts', async () => {
     const store = createTestStore()
     const workspaceId = 'shared-folder-id'

src/shared/diff-comment-schema.test.ts

@@ -0,0 +1,75 @@
+import { describe, expect, it } from 'vitest'
+import { DiffCommentSchema, parsePersistedDiffComments } from './diff-comment-schema'
+
+function comment(overrides: Record<string, unknown> = {}): Record<string, unknown> {
+  return {
+    id: 'note-1',
+    worktreeId: 'repo::wt',
+    filePath: 'README.md',
+    lineNumber: 3,
+    body: 'Review this',
+    createdAt: 100,
+    side: 'modified',
+    ...overrides
+  }
+}
+
+describe('DiffCommentSchema', () => {
+  it('accepts a line note and a file-level note', () => {
+    expect(DiffCommentSchema.safeParse(comment()).success).toBe(true)
+    expect(DiffCommentSchema.safeParse(comment({ lineNumber: 0 })).success).toBe(true)
+  })
+
+  it('rejects empty identity and path fields', () => {
+    for (const field of ['id', 'worktreeId', 'filePath']) {
+      expect(DiffCommentSchema.safeParse(comment({ [field]: '' })).success).toBe(false)
+    }
+  })
+
+  it('trims the body and rejects one with no content', () => {
+    const parsed = DiffCommentSchema.safeParse(comment({ body: '  Review this  ' }))
+    expect(parsed.success && parsed.data.body).toBe('Review this')
+    expect(DiffCommentSchema.safeParse(comment({ body: '   ' })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ body: '' })).success).toBe(false)
+  })
+
+  it('requires startLine to stay within the note range', () => {
+    expect(DiffCommentSchema.safeParse(comment({ startLine: 3 })).success).toBe(true)
+    expect(DiffCommentSchema.safeParse(comment({ startLine: 4 })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ startLine: 0 })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ lineNumber: 0, startLine: 1 })).success).toBe(
+      false
+    )
+  })
+
+  it('rejects a negative line number and a non-positive sentAt', () => {
+    expect(DiffCommentSchema.safeParse(comment({ lineNumber: -1 })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ lineNumber: 1.5 })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ sentAt: 1 })).success).toBe(true)
+    expect(DiffCommentSchema.safeParse(comment({ sentAt: 0 })).success).toBe(false)
+    expect(DiffCommentSchema.safeParse(comment({ sentAt: -1 })).success).toBe(false)
+  })
+})
+
+describe('parsePersistedDiffComments', () => {
+  it('keeps valid notes and drops malformed ones', () => {
+    expect(
+      parsePersistedDiffComments([
+        comment({ id: 'keep-1' }),
+        comment({ id: '' }),
+        { id: 'no-body', worktreeId: 'repo::wt', filePath: 'README.md' },
+        'not-an-object',
+        null,
+        comment({ id: 'keep-2', lineNumber: 0 })
+      ])
+    ).toEqual([
+      expect.objectContaining({ id: 'keep-1' }),
+      expect.objectContaining({ id: 'keep-2' })
+    ])
+  })
+
+  it('returns an empty list for non-array input', () => {
+    expect(parsePersistedDiffComments(undefined)).toEqual([])
+    expect(parsePersistedDiffComments({ length: 1 })).toEqual([])
+  })
+})

src/shared/diff-comment-schema.ts

@@ -1,19 +1,42 @@
 import { z } from 'zod'
+import type { DiffComment } from './types'
 
-export const DiffCommentSchema = z.object({
-  id: z.string(),
-  worktreeId: z.string(),
-  filePath: z.string(),
-  source: z.enum(['diff', 'markdown']).optional(),
-  selectedText: z.string().optional(),
-  startLine: z.number().int().positive().optional(),
-  lineNumber: z.number().int().positive(),
-  body: z.string(),
-  createdAt: z.number().finite(),
-  updatedAt: z.number().finite().optional(),
-  sentAt: z.number().finite().optional(),
-  scope: z.enum(['unstaged', 'staged', 'branch']).optional(),
-  oldPath: z.string().optional(),
-  diffIdentity: z.string().optional(),
-  side: z.literal('modified')
-})
+export const DiffCommentSchema = z
+  .object({
+    id: z.string().min(1),
+    worktreeId: z.string().min(1),
+    filePath: z.string().min(1),
+    source: z.enum(['diff', 'markdown']).optional(),
+    selectedText: z.string().optional(),
+    startLine: z.number().int().positive().optional(),
+    // Why: 0 marks a file-level note; line notes are 1-based.
+    lineNumber: z.number().int().min(0),
+    body: z.string().trim().min(1),
+    createdAt: z.number().finite(),
+    updatedAt: z.number().finite().optional(),
+    // Why: the renderer treats a non-positive sentAt as unsent, so never store one.
+    sentAt: z.number().finite().positive().optional(),
+    scope: z.enum(['unstaged', 'staged', 'branch']).optional(),
+    oldPath: z.string().optional(),
+    diffIdentity: z.string().optional(),
+    side: z.literal('modified')
+  })
+  .refine((comment) => comment.startLine === undefined || comment.startLine <= comment.lineNumber, {
+    message: 'startLine must not exceed lineNumber',
+    path: ['startLine']
+  })
+
+// Why: persisted JSON predates the schema boundary, so drop unusable notes instead of rehydrating them.
+export function parsePersistedDiffComments(value: unknown): DiffComment[] {
+  if (!Array.isArray(value)) {
+    return []
+  }
+  const comments: DiffComment[] = []
+  for (const candidate of value) {
+    const parsed = DiffCommentSchema.safeParse(candidate)
+    if (parsed.success) {
+      comments.push(parsed.data)
+    }
+  }
+  return comments
+}

src/shared/folder-workspaces.test.ts

@@ -0,0 +1,45 @@
+import { describe, expect, it } from 'vitest'
+import type { ProjectGroup } from './types'
+import { normalizeFolderWorkspaces } from './folder-workspaces'
+
+const projectGroups = [
+  { id: 'group-1', name: 'Platform', parentPath: '/workspace' } as ProjectGroup
+]
+
+function persistedWorkspace(diffComments: unknown): Record<string, unknown> {
+  return {
+    id: 'folder-1',
+    projectGroupId: 'group-1',
+    folderPath: '/workspace/platform',
+    diffComments
+  }
+}
+
+describe('normalizeFolderWorkspaces diff comments', () => {
+  it('keeps valid persisted notes and drops malformed ones', () => {
+    const valid = {
+      id: 'note-1',
+      worktreeId: 'folder::folder-1',
+      filePath: 'README.md',
+      lineNumber: 1,
+      body: 'Review this',
+      createdAt: 100,
+      side: 'modified'
+    }
+    const [workspace] = normalizeFolderWorkspaces(
+      [persistedWorkspace([valid, { ...valid, id: 'note-2', body: '   ' }, 42])],
+      projectGroups
+    )
+
+    expect(workspace.diffComments).toEqual([expect.objectContaining({ id: 'note-1' })])
+  })
+
+  it('leaves diffComments unset when the persisted value is not an array', () => {
+    const [workspace] = normalizeFolderWorkspaces(
+      [persistedWorkspace({ id: 'note-1' })],
+      projectGroups
+    )
+
+    expect(workspace.diffComments).toBeUndefined()
+  })
+})

src/shared/folder-workspaces.ts

@@ -4,6 +4,7 @@ import { normalizeStoredTaskSourceContext } from './task-source-context'
 import { normalizeWorkspaceLinkedItem } from './workspace-linked-item'
 import { isWorkspaceLinkedItemSourceContextMatch } from './workspace-linked-item-source-context'
 import { normalizeWorkspaceCreatorProvenance } from './workspace-creator-provenance'
+import { parsePersistedDiffComments } from './diff-comment-schema'
 
 export function normalizeFolderWorkspaceName(
   name: string | null | undefined,
@@ -104,7 +105,9 @@ export function normalizeFolderWorkspaces(
         typeof raw.createdAt === 'number' && Number.isFinite(raw.createdAt) ? raw.createdAt : now,
       updatedAt:
         typeof raw.updatedAt === 'number' && Number.isFinite(raw.updatedAt) ? raw.updatedAt : now,
-      ...(Array.isArray(raw.diffComments) ? { diffComments: raw.diffComments } : {})
+      ...(Array.isArray(raw.diffComments)
+        ? { diffComments: parsePersistedDiffComments(raw.diffComments) }
+        : {})
     })
   }
   return workspaces.sort(