From dfc2a43a91427a0e4b0c87b1424c45c233ea7524 Mon Sep 17 00:00:00 2001 From: iliya Date: Wed, 4 Mar 2026 14:36:05 +0200 Subject: [PATCH] fix: stabilize diff review apply state and missing-file UX - Invalidate main-process file content cache after applying review decisions - Clear stale per-file review state after instant-apply to prevent re-apply conflicts - Disable accept/reject for preview-only files and surface deleted/renamed/moved badges - Persist per-file collapse state across reopen Made-with: Cursor --- src/main/ipc/review.ts | 14 +- .../team/review/ChangeReviewDialog.tsx | 180 +++++++++++++++++- .../team/review/ContinuousScrollView.tsx | 41 ++-- .../team/review/FileSectionDiff.tsx | 2 +- .../team/review/FileSectionHeader.tsx | 109 +++++++---- .../components/team/review/ReviewFileTree.tsx | 22 +++ .../store/slices/changeReviewSlice.ts | 43 +++++ 7 files changed, 358 insertions(+), 53 deletions(-) diff --git a/src/main/ipc/review.ts b/src/main/ipc/review.ts index 9663c5dc..1869e9a6 100644 --- a/src/main/ipc/review.ts +++ b/src/main/ipc/review.ts @@ -257,7 +257,19 @@ async function handleApplyDecisions( fileContents.set(d.filePath, resolved); } - return getApplier().applyReviewDecisions(request, fileContents); + const result = await getApplier().applyReviewDecisions(request, fileContents); + + // Invalidate resolved file content cache after applying decisions so subsequent + // diff operations read the latest disk state (avoids "stuck" decisions in instant-apply flows). + try { + for (const d of request.decisions) { + getContentResolver().invalidateFile(d.filePath); + } + } catch (error) { + logger.debug('applyDecisions cache invalidation failed:', error); + } + + return result; }); } diff --git a/src/renderer/components/team/review/ChangeReviewDialog.tsx b/src/renderer/components/team/review/ChangeReviewDialog.tsx index 4ffcb471..d99de220 100644 --- a/src/renderer/components/team/review/ChangeReviewDialog.tsx +++ b/src/renderer/components/team/review/ChangeReviewDialog.tsx @@ -83,6 +83,7 @@ export const ChangeReviewDialog = ({ applySingleFileDecision, removeReviewFile, addReviewFile, + clearReviewStateForFile, editedContents, updateEditedContent, discardFileEdits, @@ -98,11 +99,33 @@ export const ChangeReviewDialog = ({ hunkContextHashesByFile, } = useStore(); + // Build scope keys (pure values — safe to compute before hooks that depend on them) + const scopeKey = mode === 'task' ? `task:${taskId ?? ''}` : `agent:${memberName ?? ''}`; + // Filesystem-safe: use `-` instead of `:` for decision persistence key + const decisionScopeKey = mode === 'task' ? `task-${taskId ?? ''}` : `agent-${memberName ?? ''}`; + // Active file from scroll-spy (replaces selectedReviewFilePath for continuous scroll) const [activeFilePath, setActiveFilePath] = useState(null); const [autoViewed, setAutoViewed] = useState(true); const [timelineOpen, setTimelineOpen] = useState(false); const [discardCounters, setDiscardCounters] = useState>({}); + const collapseStorageKey = useMemo( + () => `review:collapsed:${teamName}:${decisionScopeKey}`, + [teamName, decisionScopeKey] + ); + const [collapsedFiles, setCollapsedFiles] = useState>(() => { + if (typeof window === 'undefined') return new Set(); + try { + const raw = window.localStorage.getItem(collapseStorageKey); + const parsed = raw ? (JSON.parse(raw) as unknown) : null; + if (Array.isArray(parsed)) { + return new Set(parsed.filter((v): v is string => typeof v === 'string')); + } + } catch { + // ignore + } + return new Set(); + }); // Selection menu state const [selectionInfo, setSelectionInfo] = useState(null); @@ -148,18 +171,92 @@ export const ChangeReviewDialog = ({ scrollContainerRef, }); - // Build scope key for viewed storage - const scopeKey = mode === 'task' ? `task:${taskId ?? ''}` : `agent:${memberName ?? ''}`; - - // Build scope key for decision persistence (filesystem-safe: use `-` instead of `:`) - const decisionScopeKey = mode === 'task' ? `task-${taskId ?? ''}` : `agent-${memberName ?? ''}`; - // File paths for viewed tracking const allFilePaths = useMemo( () => (activeChangeSet?.files ?? []).map((f) => f.filePath), [activeChangeSet] ); + const pathChangeLabels = useMemo(() => { + if (!activeChangeSet) + return {} as Record< + string, + | { kind: 'deleted' } + | { kind: 'moved' | 'renamed'; direction: 'from' | 'to'; otherPath: string } + >; + + const normalize = (s: string): string => + s.replace(/\r\n/g, '\n').replace(/\r/g, '\n').trimEnd(); + const hashFull = (s: string): string => { + // DJB2 (full string) — good enough for heuristic rename/move pairing + let h = 5381; + for (let i = 0; i < s.length; i++) h = ((h << 5) + h + s.charCodeAt(i)) | 0; + return (h >>> 0).toString(36); + }; + const baseName = (p: string): string => p.split(/[\\/]/).filter(Boolean).pop() ?? p; + + type Label = + | { kind: 'deleted' } + | { kind: 'moved' | 'renamed'; direction: 'from' | 'to'; otherPath: string }; + + const out: Record = {}; + + const deletedCandidates: { file: FileChangeSummary; hash: string }[] = []; + const newCandidates: { file: FileChangeSummary; hash: string }[] = []; + + for (const f of activeChangeSet.files) { + const content = fileContents[f.filePath]; + if (!content) continue; + + const modified = content.modifiedFullContent; + const original = content.originalFullContent; + + if (!f.isNewFile && modified == null) { + if (original != null) { + deletedCandidates.push({ file: f, hash: hashFull(normalize(original)) }); + } else { + out[f.filePath] = { kind: 'deleted' }; + } + } + + if (f.isNewFile && modified != null) { + newCandidates.push({ file: f, hash: hashFull(normalize(modified)) }); + } + } + + const deletedByHash = new Map(); + for (const d of deletedCandidates) { + const prev = deletedByHash.get(d.hash); + deletedByHash.set(d.hash, { file: d.file, count: (prev?.count ?? 0) + 1 }); + } + + const usedDeleted = new Set(); + for (const n of newCandidates) { + const entry = deletedByHash.get(n.hash); + if (!entry) continue; + if (entry.count !== 1) continue; // ambiguous + const oldFile = entry.file; + if (usedDeleted.has(oldFile.filePath)) continue; + usedDeleted.add(oldFile.filePath); + + const oldName = baseName(oldFile.relativePath); + const newName = baseName(n.file.relativePath); + const kind: 'moved' | 'renamed' = + oldName === newName && oldFile.relativePath !== n.file.relativePath ? 'moved' : 'renamed'; + + out[n.file.filePath] = { kind, direction: 'from', otherPath: oldFile.relativePath }; + out[oldFile.filePath] = { kind, direction: 'to', otherPath: n.file.relativePath }; + } + + for (const d of deletedCandidates) { + if (!usedDeleted.has(d.file.filePath) && !(d.file.filePath in out)) { + out[d.file.filePath] = { kind: 'deleted' }; + } + } + + return out; + }, [activeChangeSet, fileContents]); + const { viewedSet, isViewed, @@ -296,6 +393,15 @@ export const ChangeReviewDialog = ({ lastNewFileRemoveAtRef.current = Date.now(); removeReviewFile(filePath); } + } else { + const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath); + if (result && !hasErrorForFile) { + // Disk state is now authoritative. Clear stale decisions/cache so reopening + // doesn't try to re-apply and the diff can re-resolve from disk. + clearReviewStateForFile(filePath); + setDiscardCounters((prev) => ({ ...prev, [filePath]: (prev[filePath] ?? 0) + 1 })); + void fetchFileContent(teamName, memberName, filePath); + } } } } finally { @@ -311,6 +417,8 @@ export const ChangeReviewDialog = ({ memberName, removeReviewFile, fileContents, + clearReviewStateForFile, + fetchFileContent, ] ); @@ -336,10 +444,25 @@ export const ChangeReviewDialog = ({ } hunkDecisionUndoStackRef.current[filePath].push(originalIndex); if (REVIEW_INSTANT_APPLY) { - void applySingleFileDecision(teamName, filePath, taskId, memberName); + void applySingleFileDecision(teamName, filePath, taskId, memberName).then((result) => { + const hasErrorForFile = !!result?.errors.some((e) => e.filePath === filePath); + if (result && !hasErrorForFile) { + clearReviewStateForFile(filePath); + setDiscardCounters((prev) => ({ ...prev, [filePath]: (prev[filePath] ?? 0) + 1 })); + void fetchFileContent(teamName, memberName, filePath); + } + }); } }, - [setHunkDecision, applySingleFileDecision, teamName, taskId, memberName] + [ + setHunkDecision, + applySingleFileDecision, + teamName, + taskId, + memberName, + clearReviewStateForFile, + fetchFileContent, + ] ); const handleContentChanged = useCallback( @@ -490,6 +613,42 @@ export const ChangeReviewDialog = ({ getFileHunkCount(filePath, fallbackSnippetsLength, fileChunkCounts) ); + const toggleCollapsedFile = useCallback((filePath: string) => { + setCollapsedFiles((prev) => { + const next = new Set(prev); + if (next.has(filePath)) next.delete(filePath); + else next.add(filePath); + return next; + }); + }, []); + + // Persist collapsed state (best-effort) + useEffect(() => { + if (!open) return; + if (typeof window === 'undefined') return; + const id = window.setTimeout(() => { + try { + window.localStorage.setItem(collapseStorageKey, JSON.stringify([...collapsedFiles])); + } catch { + // ignore + } + }, 200); + return () => window.clearTimeout(id); + }, [open, collapseStorageKey, collapsedFiles]); + + // Prune collapsed entries to only current files to avoid stale growth + useEffect(() => { + if (!activeChangeSet) return; + const allowed = new Set(activeChangeSet.files.map((f) => f.filePath)); + setCollapsedFiles((prev) => { + const next = new Set(); + for (const fp of prev) { + if (allowed.has(fp)) next.add(fp); + } + return next.size === prev.size ? prev : next; + }); + }, [activeChangeSet]); + // Load data on open useEffect(() => { if (!open) return; @@ -902,6 +1061,8 @@ export const ChangeReviewDialog = ({
void; onRejectFile: (filePath: string) => void; onRestoreMissingFile?: (filePath: string, content: string) => void; + pathChangeLabels?: Record< + string, + { kind: 'deleted' } | { kind: 'moved' | 'renamed'; direction: 'from' | 'to'; otherPath: string } + >; + /** Controlled collapsed state (persisted by parent). If omitted, component manages it locally. */ + collapsedFiles?: Set; + onToggleCollapse?: (filePath: string) => void; onVisibleFileChange: (filePath: string) => void; scrollContainerRef: React.RefObject; editorViewMapRef: React.MutableRefObject>; @@ -77,6 +84,9 @@ export const ContinuousScrollView = ({ onAcceptFile, onRejectFile, onRestoreMissingFile, + pathChangeLabels, + collapsedFiles: collapsedFilesProp, + onToggleCollapse: onToggleCollapseProp, onVisibleFileChange, scrollContainerRef, editorViewMapRef, @@ -87,19 +97,27 @@ export const ContinuousScrollView = ({ onSelectionChange, }: ContinuousScrollViewProps): React.ReactElement => { const setFileChunkCount = useStore((s) => s.setFileChunkCount); - const [collapsedFiles, setCollapsedFiles] = useState>(new Set()); + const [localCollapsedFiles, setLocalCollapsedFiles] = useState>(() => new Set()); + const collapsedFiles = collapsedFilesProp ?? localCollapsedFiles; - const handleToggleCollapse = useCallback((filePath: string) => { - setCollapsedFiles((prev) => { - const next = new Set(prev); - if (next.has(filePath)) { - next.delete(filePath); - } else { - next.add(filePath); + const handleToggleCollapse = useCallback( + (filePath: string) => { + if (onToggleCollapseProp) { + onToggleCollapseProp(filePath); + return; } - return next; - }); - }, []); + setLocalCollapsedFiles((prev) => { + const next = new Set(prev); + if (next.has(filePath)) { + next.delete(filePath); + } else { + next.add(filePath); + } + return next; + }); + }, + [onToggleCollapseProp] + ); const filePaths = useMemo(() => files.map((f) => f.filePath), [files]); @@ -222,6 +240,7 @@ export const ContinuousScrollView = ({ file={file} fileContent={content} fileDecision={decision} + pathChangeLabel={pathChangeLabels?.[filePath]} hasEdits={hasEdits} applying={applying} isCollapsed={isCollapsed} diff --git a/src/renderer/components/team/review/FileSectionDiff.tsx b/src/renderer/components/team/review/FileSectionDiff.tsx index e4a109cb..a2ca0f63 100644 --- a/src/renderer/components/team/review/FileSectionDiff.tsx +++ b/src/renderer/components/team/review/FileSectionDiff.tsx @@ -93,7 +93,7 @@ export const FileSectionDiff = ({ })(); const resolvedOriginal = fileContent?.originalFullContent ?? null; - const isMissingOnDisk = fileContent?.contentSource === 'unavailable'; + const isMissingOnDisk = fileContent ? fileContent.modifiedFullContent == null : false; // Show CodeMirror only when we have a trustworthy original baseline: // - new files: original is legitimately empty diff --git a/src/renderer/components/team/review/FileSectionHeader.tsx b/src/renderer/components/team/review/FileSectionHeader.tsx index 64278f2f..4c99455c 100644 --- a/src/renderer/components/team/review/FileSectionHeader.tsx +++ b/src/renderer/components/team/review/FileSectionHeader.tsx @@ -19,6 +19,9 @@ interface FileSectionHeaderProps { file: FileChangeSummary; fileContent: FileChangeWithContent | null; fileDecision: HunkDecision | undefined; + pathChangeLabel?: + | { kind: 'deleted' } + | { kind: 'moved' | 'renamed'; direction: 'from' | 'to'; otherPath: string }; hasEdits: boolean; applying: boolean; isCollapsed: boolean; @@ -34,6 +37,7 @@ export const FileSectionHeader = ({ file, fileContent, fileDecision, + pathChangeLabel, hasEdits, applying, isCollapsed, @@ -44,7 +48,8 @@ export const FileSectionHeader = ({ onAcceptFile, onRejectFile, }: FileSectionHeaderProps): React.ReactElement => { - const isMissingOnDisk = fileContent?.contentSource === 'unavailable'; + const isMissingOnDisk = fileContent ? fileContent.modifiedFullContent == null : false; + const isPreviewOnly = isMissingOnDisk || fileContent?.contentSource === 'unavailable'; const restoreContent = fileContent?.modifiedFullContent ?? (() => { @@ -54,8 +59,7 @@ export const FileSectionHeader = ({ if (writeSnippets.length === 0) return null; return writeSnippets[writeSnippets.length - 1].newString; })(); - const canRestore = - !!onRestoreMissingFile && isMissingOnDisk && !hasEdits && restoreContent != null; + const canRestore = !!onRestoreMissingFile && isPreviewOnly && !hasEdits && restoreContent != null; const handleHeaderClick = (e: React.MouseEvent): void => { // Don't collapse when clicking action buttons @@ -89,22 +93,41 @@ export const FileSectionHeader = ({ )} + {pathChangeLabel?.kind === 'deleted' && ( + + DELETED + + )} + + {pathChangeLabel && pathChangeLabel.kind !== 'deleted' && ( + + + + {pathChangeLabel.kind.toUpperCase()} + + + + {pathChangeLabel.direction === 'from' ? 'From' : 'To'} {pathChangeLabel.otherPath} + + + )} + {fileContent?.contentSource && ( - {CONTENT_SOURCE_LABELS[fileContent.contentSource] ?? fileContent.contentSource} + {isPreviewOnly + ? 'Missing on disk' + : (CONTENT_SOURCE_LABELS[fileContent.contentSource] ?? fileContent.contentSource)} - {fileContent.contentSource === 'unavailable' ? ( + {isPreviewOnly ? (
File is missing on disk
@@ -148,32 +171,54 @@ export const FileSectionHeader = ({ {(onAcceptFile || onRejectFile) && (
{onAcceptFile && ( - + + + + + + + {isPreviewOnly && ( + + Accept/Reject is disabled while the file is missing on disk. + + )} + )} {onRejectFile && ( - + + + + + + + {isPreviewOnly && ( + + Accept/Reject is disabled while the file is missing on disk. + + )} + )}
)} diff --git a/src/renderer/components/team/review/ReviewFileTree.tsx b/src/renderer/components/team/review/ReviewFileTree.tsx index 5d2fedd8..c7c38b34 100644 --- a/src/renderer/components/team/review/ReviewFileTree.tsx +++ b/src/renderer/components/team/review/ReviewFileTree.tsx @@ -20,10 +20,16 @@ import { import type { TreeNode } from '@renderer/utils/fileTreeBuilder'; import type { HunkDecision } from '@shared/types'; +import type { FileChangeWithContent } from '@shared/types'; import type { FileChangeSummary } from '@shared/types/review'; interface ReviewFileTreeProps { files: FileChangeSummary[]; + fileContents?: Record; + pathChangeLabels?: Record< + string, + { kind: 'deleted' } | { kind: 'moved' | 'renamed'; direction: 'from' | 'to'; otherPath: string } + >; selectedFilePath: string | null; onSelectFile: (filePath: string) => void; viewedSet?: Set; @@ -108,6 +114,7 @@ const TreeItem = ({ viewedSet, collapsedFolders, onToggleFolder, + pathChangeLabels, }: { node: TreeNode; selectedFilePath: string | null; @@ -120,11 +127,13 @@ const TreeItem = ({ viewedSet?: Set; collapsedFolders: Set; onToggleFolder: (fullPath: string) => void; + pathChangeLabels?: ReviewFileTreeProps['pathChangeLabels']; }): JSX.Element => { if (node.isFile && node.data) { const isSelected = node.data.filePath === selectedFilePath; const isActive = node.data.filePath === activeFilePath && !isSelected; const status = getFileStatus(node.data, hunkDecisions, fileDecisions, fileChunkCounts); + const label = pathChangeLabels?.[node.data.filePath]; return (
@@ -248,6 +268,7 @@ function getAncestorFolderPaths(tree: TreeNode[], filePath: s export const ReviewFileTree = ({ files, + pathChangeLabels, selectedFilePath, onSelectFile, viewedSet, @@ -441,6 +462,7 @@ export const ReviewFileTree = ({ viewedSet={viewedSet} collapsedFolders={collapsedFolders} onToggleFolder={toggleFolder} + pathChangeLabels={pathChangeLabels} /> ))}
diff --git a/src/renderer/store/slices/changeReviewSlice.ts b/src/renderer/store/slices/changeReviewSlice.ts index 2c7fbbf3..3e7b366a 100644 --- a/src/renderer/store/slices/changeReviewSlice.ts +++ b/src/renderer/store/slices/changeReviewSlice.ts @@ -131,6 +131,11 @@ export interface ChangeReviewSlice { file: FileChangeSummary, options?: { index?: number; content?: FileChangeWithContent } ) => void; + /** + * Clear in-memory review state for a single file after applying changes to disk. + * Prevents stale decisions from being re-applied later and forces fresh content resolve. + */ + clearReviewStateForFile: (filePath: string) => void; invalidateChangeStats: (teamName: string) => void; // Editable diff actions @@ -889,6 +894,44 @@ export const createChangeReviewSlice: StateCreator { + set((s) => { + const nextHunkDecisions = { ...s.hunkDecisions }; + const prefix = `${filePath}:`; + for (const key of Object.keys(nextHunkDecisions)) { + if (key.startsWith(prefix) && nextHunkDecisions[key] === 'rejected') { + delete nextHunkDecisions[key]; + } + } + + const nextFileDecisions = { ...s.fileDecisions }; + if (nextFileDecisions[filePath] === 'rejected') { + delete nextFileDecisions[filePath]; + } + + const nextFileChunkCounts = { ...s.fileChunkCounts }; + delete nextFileChunkCounts[filePath]; + + const nextFileContents = { ...s.fileContents }; + delete nextFileContents[filePath]; + + const nextFileContentsLoading = { ...s.fileContentsLoading }; + delete nextFileContentsLoading[filePath]; + + const nextEditedContents = { ...s.editedContents }; + delete nextEditedContents[filePath]; + + return { + hunkDecisions: nextHunkDecisions, + fileDecisions: nextFileDecisions, + fileChunkCounts: nextFileChunkCounts, + fileContents: nextFileContents, + fileContentsLoading: nextFileContentsLoading, + editedContents: nextEditedContents, + }; + }); + }, + // ── Editable diff actions ── updateEditedContent: (filePath: string, content: string) => {