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(