From f6ebdc27073f4db642c6b6558e10e1a8d865d65e Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:27:02 -0700 Subject: [PATCH] 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 --- src/renderer/src/store/slices/diffComments.ts | 162 +++++++++--------- .../folder-workspace-diff-comments.test.ts | 88 ++++++++++ src/shared/diff-comment-schema.test.ts | 75 ++++++++ src/shared/diff-comment-schema.ts | 57 ++++-- src/shared/folder-workspaces.test.ts | 45 +++++ src/shared/folder-workspaces.ts | 5 +- 6 files changed, 333 insertions(+), 99 deletions(-) create mode 100644 src/shared/diff-comment-schema.test.ts create mode 100644 src/shared/folder-workspaces.test.ts diff --git a/src/renderer/src/store/slices/diffComments.ts b/src/renderer/src/store/slices/diffComments.ts index 3af0505b031..32f33f7fb9c 100644 --- a/src/renderer/src/store/slices/diffComments.ts +++ b/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>() +const persistQueueByWorktree = new Map>() -// 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() + +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 -): Promise { + mutation: CommentMutation +): Promise { + const folderExecutionHostId = mutation.folderExecutionHostId const queueKey = folderExecutionHostId ? `${folderExecutionHostId}\0${worktreeId}` : worktreeId - const prior = persistQueueByWorktree.get(queueKey) ?? Promise.resolve() - const run = async (): Promise => { + // 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({ ok: true }) + const run = async (): Promise => { 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>[0], + get: () => AppState, + worktreeId: string, + mutation: CommentMutation +): Promise { + 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 +} + // 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>[0], worktreeId: string, mutate: (existing: DiffComment[]) => DiffComment[] | null -): { - previous: DiffComment[] | undefined - next: DiffComment[] - folderExecutionHostId?: ReturnType -} | null { +): CommentMutation | null { const repoId = getRepoIdFromWorktreeId(worktreeId) let previous: DiffComment[] | undefined let next: DiffComment[] | null = null @@ -330,17 +369,12 @@ export const createDiffCommentsSlice: StateCreator { @@ -377,14 +411,7 @@ export const createDiffCommentsSlice: StateCreator { @@ -403,15 +430,11 @@ export const createDiffCommentsSlice: StateCreator { @@ -433,15 +456,11 @@ export const createDiffCommentsSlice: StateCreator { @@ -452,13 +471,8 @@ export const createDiffCommentsSlice: StateCreator { @@ -468,14 +482,7 @@ export const createDiffCommentsSlice: StateCreator { @@ -486,13 +493,6 @@ export const createDiffCommentsSlice: StateCreator { }) }) + 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((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((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' diff --git a/src/shared/diff-comment-schema.test.ts b/src/shared/diff-comment-schema.test.ts new file mode 100644 index 00000000000..751d8b897eb --- /dev/null +++ b/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 = {}): Record { + 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([]) + }) +}) diff --git a/src/shared/diff-comment-schema.ts b/src/shared/diff-comment-schema.ts index 369dda682c6..444a313c537 100644 --- a/src/shared/diff-comment-schema.ts +++ b/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 +} diff --git a/src/shared/folder-workspaces.test.ts b/src/shared/folder-workspaces.test.ts new file mode 100644 index 00000000000..16ed380b70d --- /dev/null +++ b/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 { + 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() + }) +}) diff --git a/src/shared/folder-workspaces.ts b/src/shared/folder-workspaces.ts index 8850083d672..99716f71810 100644 --- a/src/shared/folder-workspaces.ts +++ b/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(