diff --git a/src/main/folder-workspace-diff-comments.ts b/src/main/folder-workspace-diff-comments.ts new file mode 100644 index 00000000000..1a7435e8930 --- /dev/null +++ b/src/main/folder-workspace-diff-comments.ts @@ -0,0 +1,37 @@ +import type { DiffComment, FolderWorkspace } from '../shared/types' + +// Why shape-only: this replaces folder-workspaces.ts's verbatim `Array.isArray(raw.diffComments)` read. +// Filtering members would make the fix itself a new deletion path for user-authored prose. +export function normalizeFolderWorkspaceDiffComments( + value: unknown +): Record | undefined { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return undefined + } + const normalized: Record = {} + let kept = false + for (const [id, comments] of Object.entries(value)) { + if (!Array.isArray(comments)) { + continue + } + normalized[id] = comments as DiffComment[] + kept = true + } + return kept ? normalized : undefined +} + +// Why derive on every write: the map self-GCs, so delete paths need no pruning code. +export function collectFolderWorkspaceDiffComments( + workspaces: readonly FolderWorkspace[] | undefined +): Record | undefined { + const collected: Record = {} + let kept = false + for (const workspace of workspaces ?? []) { + const comments = workspace.diffComments + if (Array.isArray(comments) && comments.length > 0) { + collected[workspace.id] = comments + kept = true + } + } + return kept ? collected : undefined +} diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index b0a1ee2ac92..7bf6e635a01 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -36,6 +36,7 @@ import { ONBOARDING_FLOW_VERSION } from '../shared/constants' import { folderWorkspaceKey, worktreeWorkspaceKey } from '../shared/workspace-scope' +import { folderWorkspaceToWorktree } from '../shared/folder-workspace-worktree' import { toRuntimeExecutionHostId, toSshExecutionHostId } from '../shared/execution-host' import { SshConnectionStore } from './ssh/ssh-connection-store' import { setSourceControlActionDefault } from '../shared/source-control-ai-actions' @@ -5418,6 +5419,369 @@ describe('Store', () => { expect(session.browserPagesByWorkspace?.['browser-workspace']).toBeUndefined() }) + // ── 8b. Folder-workspace review notes across a build rollback ── + + function makeFolderNote(id: string, body: string, workspaceId = 'fw-1') { + return { + id, + worktreeId: folderWorkspaceKey(workspaceId), + filePath: 'README.md', + source: 'markdown' as const, + lineNumber: 1, + body, + createdAt: 100, + side: 'modified' as const + } + } + + function folderWorkspaceRecord(overrides: Record = {}) { + return { + id: 'fw-1', + projectGroupId: 'root', + name: 'Refund fix', + folderPath: '/workspace/platform', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 10, + lastActivityAt: 5, + createdAt: 2, + updatedAt: 3, + ...overrides + } + } + + function writeFolderWorkspaceProfile(options: { + workspaces: Record[] + diffCommentsMap?: unknown + connectionId?: string | null + extraTopLevel?: Record + }): void { + writeDataFile({ + schemaVersion: 1, + repos: [], + worktreeMeta: {}, + settings: {}, + ui: {}, + githubCache: { pr: {}, issue: {} }, + projectGroups: [ + { + id: 'root', + name: 'Platform', + parentPath: '/workspace/platform', + parentGroupId: null, + connectionId: options.connectionId ?? null, + createdFrom: 'folder-scan', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 1, + updatedAt: 1 + } + ], + folderWorkspaces: options.workspaces, + ...('diffCommentsMap' in options + ? { folderWorkspaceDiffComments: options.diffCommentsMap } + : {}), + ...options.extraTopLevel + }) + } + + // The previous build's normalizeFolderWorkspaces has no diffComments projection (the line this + // checkout carries at src/shared/folder-workspaces.ts:107 does not exist on v1.4.179–v1.4.181), + // and its full-state write re-serializes everything else verbatim (the `{ ...defaults, ...parsed }` + // load spread and the omit-style getDurableState()). + function simulatePreviousBuildLoadAndFlush(onDisk: PersistedState): PersistedState { + return { + ...onDisk, + folderWorkspaces: onDisk.folderWorkspaces.map( + ({ diffComments: _droppedByOldNormalizer, ...rest }) => rest + ) + } + } + + it('keeps folder-workspace review notes across a previous-build rollback and re-upgrade', async () => { + const store = await createStore() + const group = store.createProjectGroup({ + name: 'Platform', + parentPath: '/workspace/platform', + createdFrom: 'folder-scan' + }) + const workspace = store.createFolderWorkspace({ + projectGroupId: group.id, + name: 'Refund fix' + }) + const note = makeFolderNote('note-1', 'Review this paragraph', workspace.id) + store.updateFolderWorkspace(workspace.id, { diffComments: [note] }) + store.flush() + + const persisted = readDataFile() as PersistedState + expect(persisted.folderWorkspaceDiffComments?.[workspace.id]).toEqual([note]) + expect(persisted.folderWorkspaces[0]).not.toHaveProperty('diffComments') + // The strip lives at the serialization boundary only; live records keep their notes. + expect(store.getFolderWorkspace(workspace.id)?.diffComments).toEqual([note]) + expect(folderWorkspaceToWorktree(store.getFolderWorkspace(workspace.id)!).diffComments).toEqual( + [note] + ) + + writeDataFile(simulatePreviousBuildLoadAndFlush(persisted)) + + const restored = await createStore() + expect(restored.getFolderWorkspace(workspace.id)?.diffComments).toEqual([note]) + restored.flush() + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments?.[workspace.id]).toEqual([ + note + ]) + }) + + it('survives repeated rollback / re-upgrade hops', async () => { + const store = await createStore() + const group = store.createProjectGroup({ + name: 'Platform', + parentPath: '/workspace/platform', + createdFrom: 'folder-scan' + }) + const workspace = store.createFolderWorkspace({ projectGroupId: group.id, name: 'Refund fix' }) + const note = makeFolderNote('note-1', 'Review this paragraph', workspace.id) + store.updateFolderWorkspace(workspace.id, { diffComments: [note] }) + store.flush() + + for (let hop = 0; hop < 2; hop++) { + writeDataFile(simulatePreviousBuildLoadAndFlush(readDataFile() as PersistedState)) + const reupgraded = await createStore() + expect(reupgraded.getFolderWorkspace(workspace.id)?.diffComments).toEqual([note]) + reupgraded.flush() + } + }) + + it('keeps notes and remote provenance for an SSH folder workspace across a rollback', async () => { + const store = await createStore() + const group = store.createProjectGroup({ + name: 'Platform', + parentPath: '/workspace/platform', + connectionId: 'ssh-1', + createdFrom: 'folder-scan' + }) + const workspace = store.createFolderWorkspace({ projectGroupId: group.id, name: 'Remote fix' }) + expect(workspace.connectionId).toBe('ssh-1') + const note = makeFolderNote('note-remote', 'Remote review', workspace.id) + store.updateFolderWorkspace(workspace.id, { diffComments: [note] }) + store.flush() + + writeDataFile(simulatePreviousBuildLoadAndFlush(readDataFile() as PersistedState)) + + const restored = await createStore() + expect(restored.getFolderWorkspace(workspace.id)?.diffComments).toEqual([note]) + expect(restored.getFolderWorkspace(workspace.id)?.connectionId).toBe('ssh-1') + }) + + it('round-trips unknown top-level state keys through load and flush', async () => { + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord({ unknownNestedField: 'from-a-newer-build' })], + extraTopLevel: { unknownTopLevelKey: { kept: 'verbatim' } } + }) + + const store = await createStore() + store.updateFolderWorkspace('fw-1', { comment: 'touch' }) + store.flush() + + const persisted = readDataFile() as PersistedState & { unknownTopLevelKey?: unknown } + expect( + persisted.unknownTopLevelKey, + 'Unknown top-level keys must survive load + flush. Note preservation relies on this: converting the ' + + '`{ ...defaults, ...parsed }` load spread or the omit-style getDurableState() into an ' + + 'allowlist re-opens folder-workspace note loss for the then-previous build.' + ).toEqual({ kept: 'verbatim' }) + expect( + persisted.folderWorkspaces[0], + 'Documents existing behavior, not a guarantee: normalizeFolderWorkspaces rebuilds each ' + + 'record field-by-field, so nested unknown fields are dropped. That is why notes moved ' + + 'to a top-level key.' + ).not.toHaveProperty('unknownNestedField') + }) + + it('migrates legacy inline folder-workspace notes into the top-level map', async () => { + const note = makeFolderNote('note-1', 'Legacy inline note') + writeFolderWorkspaceProfile({ workspaces: [folderWorkspaceRecord({ diffComments: [note] })] }) + + const store = await createStore() + expect(store.getFolderWorkspace('fw-1')?.diffComments).toEqual([note]) + store.flush() + + const persisted = readDataFile() as PersistedState + expect(persisted.folderWorkspaceDiffComments?.['fw-1']).toEqual([note]) + expect(persisted.folderWorkspaces[0]).not.toHaveProperty('diffComments') + }) + + it('keeps notes across a rollback when the session never edits anything', async () => { + const note = makeFolderNote('note-1', 'Legacy inline note') + writeFolderWorkspaceProfile({ workspaces: [folderWorkspaceRecord({ diffComments: [note] })] }) + + // Launch and quit on the fixed build, with no mutation: loadNeedsSave must make the + // relocation durable on its own. This is the reported P0 sequence. + const upgraded = await createStore() + upgraded.flush() + + writeDataFile(simulatePreviousBuildLoadAndFlush(readDataFile() as PersistedState)) + + const restored = await createStore() + expect(restored.getFolderWorkspace('fw-1')?.diffComments).toEqual([note]) + }) + + it('keeps notes authored inline on a rolled-back #14112 build over a staler map entry', async () => { + const mapped = makeFolderNote('note-mapped', 'Note from the fixed build') + const authored = makeFolderNote('note-inline', 'Note authored while rolled back') + + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord()], + diffCommentsMap: { 'fw-1': [mapped] } + }) + // #14112 persists notes inline and never learned about the top-level map, so a note authored + // there lands inline while the untouched map entry goes stale. Inline is the newer write. + const rolledBack = readDataFile() as PersistedState + writeDataFile({ + ...rolledBack, + folderWorkspaces: rolledBack.folderWorkspaces.map((workspace) => ({ + ...workspace, + diffComments: [authored] + })) + }) + + const store = await createStore() + expect(store.getFolderWorkspace('fw-1')?.diffComments).toEqual([authored]) + store.flush() + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments).toEqual({ + 'fw-1': [authored] + }) + }) + + it('never lets an empty or unrelated map entry delete inline notes', async () => { + const inline = makeFolderNote('note-inline', 'Inline note') + const mapped = makeFolderNote('note-mapped', 'Mapped note') + + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord({ diffComments: [inline] })], + diffCommentsMap: { 'fw-1': [] } + }) + expect((await createStore()).getFolderWorkspace('fw-1')?.diffComments).toEqual([inline]) + + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord({ diffComments: [inline] })], + diffCommentsMap: { other: [mapped] } + }) + const store = await createStore() + expect(store.getFolderWorkspace('fw-1')?.diffComments).toEqual([inline]) + store.flush() + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments).toEqual({ + 'fw-1': [inline] + }) + }) + + it('drops orphaned note entries and prunes them when workspaces are deleted', async () => { + const note = makeFolderNote('note-1', 'Kept note') + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord()], + diffCommentsMap: { 'fw-1': [note], ghost: [makeFolderNote('note-ghost', 'Orphan', 'ghost')] } + }) + + const store = await createStore() + store.flush() + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments).toEqual({ + 'fw-1': [note] + }) + + const reloaded = await createStore() + reloaded.flush() + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments).not.toHaveProperty( + 'ghost' + ) + + // Delete paths carry no pruning code: the map is derived from live workspaces on every write. + expect(reloaded.removeFolderWorkspace('fw-1')).toBe(true) + reloaded.flush() + expect(readDataFile() as PersistedState).not.toHaveProperty('folderWorkspaceDiffComments') + + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord()], + diffCommentsMap: { 'fw-1': [note] } + }) + const groupDelete = await createStore() + expect(groupDelete.deleteProjectGroup('root')).toBe(true) + groupDelete.flush() + expect(readDataFile() as PersistedState).not.toHaveProperty('folderWorkspaceDiffComments') + }) + + it('writes no folderWorkspaceDiffComments key for note-free profiles', async () => { + const store = await createStore() + const group = store.createProjectGroup({ + name: 'Platform', + parentPath: '/workspace/platform', + createdFrom: 'folder-scan' + }) + const workspace = store.createFolderWorkspace({ projectGroupId: group.id, name: 'No notes' }) + store.flush() + expect('folderWorkspaceDiffComments' in (readDataFile() as object)).toBe(false) + + store.updateFolderWorkspace(workspace.id, { diffComments: [] }) + store.flush() + expect('folderWorkspaceDiffComments' in (readDataFile() as object)).toBe(false) + }) + + it.each([ + ['null root', null, undefined], + ['string root', 'oops', undefined], + ['array root', [], undefined], + ['non-array entry value', { 'fw-1': 'oops' }, undefined], + ['array of non-DiffComment members', { 'fw-1': [7] }, [7]] + ])( + 'tolerates a corrupt folderWorkspaceDiffComments map (%s)', + async (_label, diffCommentsMap, expected) => { + writeFolderWorkspaceProfile({ workspaces: [folderWorkspaceRecord()], diffCommentsMap }) + + const store = await createStore() + + expect(store.getFolderWorkspace('fw-1')?.diffComments).toEqual(expected) + } + ) + + it('passes note members through verbatim on load and re-write', async () => { + // Shape-only guard: the moment member filtering is added here, the fix itself becomes a new + // deletion path for user-authored prose. + const noteWithExtras = { ...makeFolderNote('note-1', 'Body'), unknownNoteField: 'kept' } + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord(), folderWorkspaceRecord({ id: 'fw-2', name: 'Second' })], + diffCommentsMap: { 'fw-1': [7], 'fw-2': [noteWithExtras] } + }) + + const store = await createStore() + store.flush() + + const persisted = readDataFile() as PersistedState + expect(persisted.folderWorkspaceDiffComments).toEqual({ + 'fw-1': [7], + 'fw-2': [noteWithExtras] + }) + }) + + it('derives the written note map from live workspaces, never a stale loaded copy', async () => { + const loadedNote = makeFolderNote('note-loaded', 'Loaded note') + const editedNote = makeFolderNote('note-edited', 'Edited note') + writeFolderWorkspaceProfile({ + workspaces: [folderWorkspaceRecord()], + diffCommentsMap: { 'fw-1': [loadedNote] } + }) + + const store = await createStore() + store.updateFolderWorkspace('fw-1', { diffComments: [editedNote] }) + store.flush() + + expect((readDataFile() as PersistedState).folderWorkspaceDiffComments).toEqual({ + 'fw-1': [editedNote] + }) + }) + // ── 9. Settings: get/update ──────────────────────────────────────── it('updateSettings merges partial updates', async () => { diff --git a/src/main/persistence.ts b/src/main/persistence.ts index fbb4a2089aa..f9fee511d39 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -256,6 +256,10 @@ import { normalizeUiLanguage } from '../shared/ui-language' import { normalizeBrowserPageZoomLevel } from '../shared/browser-page-zoom' import { persistedUIValuesEqual } from '../shared/persisted-ui-equality' import { ActiveViewPreference } from './active-view-preference' +import { + collectFolderWorkspaceDiffComments, + normalizeFolderWorkspaceDiffComments +} from './folder-workspace-diff-comments' import { normalizeFolderWorkspaceName, normalizeFolderWorkspaces @@ -2858,6 +2862,7 @@ export class Store { // profile avoids serializing the multi-MB recovery store on navigation. this.activeViewPreference = new ActiveViewPreference(this.dataFile, this.state.ui?.activeView) const adaptedProjectGroups = this.adaptFlatFolderScanProjectGroups() + this.hydrateFolderWorkspaceDiffComments() for (const entry of normalized.migrationUnsupportedEntries) { setMigrationUnsupportedPty(entry) } @@ -2878,6 +2883,33 @@ export class Store { } } + // Why: notes live top-level on disk so an older build's field-by-field + // normalizeFolderWorkspaces can't drop them; re-attach them to the in-memory records here. + private hydrateFolderWorkspaceDiffComments(): void { + const stored = this.state.folderWorkspaceDiffComments + let relocatedInline = false + for (const workspace of this.state.folderWorkspaces ?? []) { + if (Array.isArray(workspace.diffComments) && workspace.diffComments.length > 0) { + // Inline wins: an intervening rollback to a #14112 build writes notes inline and leaves the + // older map untouched, so inline is the last notes-aware write. Also makes the relocation + // durable even if the user never edits anything this session. + relocatedInline = true + continue + } + const comments = stored?.[workspace.id] + // Not `??`: a degenerate `{ id: [] }` entry must not delete an intact inline value. + if (Array.isArray(comments) && comments.length > 0) { + workspace.diffComments = comments + } + } + if (relocatedInline) { + this.loadNeedsSave = true + } + // Write-only projection: buildStateToSave() is the only producer, so leaving the loaded map in + // state would make it a stale second source of truth that getDurableState() spreads back out. + delete this.state.folderWorkspaceDiffComments + } + private adaptFlatFolderScanProjectGroups(): boolean { // Why: older folder imports kept a real parent path but flat repos; upgrade that shape into v1 sparse folder scopes. const groups = this.state.projectGroups ?? [] @@ -3427,6 +3459,9 @@ export class Store { parsed.folderWorkspaces, normalizedProjectGroups ), + folderWorkspaceDiffComments: normalizeFolderWorkspaceDiffComments( + parsed.folderWorkspaceDiffComments + ), worktreeLineageById: parsed.worktreeLineageById ?? {}, mobileClientTabSelectionsByDeviceId: normalizePersistedMobileClientTabSelections( parsed.mobileClientTabSelectionsByDeviceId @@ -4021,6 +4056,13 @@ export class Store { // Why: clone before encrypting secrets so in-memory this.state stays plaintext. const stateToSave = { ...this.getDurableState(), + // Why both keys unconditionally: the explicit keys always win over the spread, and + // JSON.stringify drops the `undefined` value so a note-free profile gains no key on disk. + // The strip builds a new array here only; this.state records keep their notes in memory. + folderWorkspaces: (this.state.folderWorkspaces ?? []).map( + ({ diffComments: _relocated, ...rest }) => rest + ), + folderWorkspaceDiffComments: collectFolderWorkspaceDiffComments(this.state.folderWorkspaces), sshPtyConsumerRecoveries: (this.state.sshPtyConsumerRecoveries ?? []).map((record) => ({ ...record, ownerLease: encryptToSentinel( diff --git a/src/shared/folder-workspaces.ts b/src/shared/folder-workspaces.ts index 8850083d672..e5badf0d8d6 100644 --- a/src/shared/folder-workspaces.ts +++ b/src/shared/folder-workspaces.ts @@ -104,6 +104,8 @@ 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, + // Legacy read: unreleased #14112 builds wrote notes inline. Canonical home is + // PersistedState.folderWorkspaceDiffComments. ...(Array.isArray(raw.diffComments) ? { diffComments: raw.diffComments } : {}) }) } diff --git a/src/shared/types.ts b/src/shared/types.ts index e83b9367a8c..4d9a2e9b810 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -3694,6 +3694,14 @@ export type PersistedState = { projectHostSetups: ProjectHostSetup[] projectGroups: ProjectGroup[] folderWorkspaces: FolderWorkspace[] + /** Folder-workspace review notes, keyed by FolderWorkspace.id. Top-level, NOT nested in + * folderWorkspaces[]: normalizeFolderWorkspaces rebuilds each record field-by-field, so an + * older build drops nested fields, while unknown top-level keys round-trip untouched. + * + * WRITE-ONLY PROJECTION. FolderWorkspace.diffComments is the single in-memory home; load() + * hydrates from this key and then deletes it from Store state, and buildStateToSave() is the + * only producer of it. Never read Store.state.folderWorkspaceDiffComments outside load(). */ + folderWorkspaceDiffComments?: Record /** Sparse-checkout presets keyed by repoId. */ sparsePresetsByRepo: Record /** Per paired device last tab selection by worktree; keeps mobile navigation across host restarts. */