From 5968a6ba88fd5ad42bebfccbe59b79fca514f45f Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:52:18 +0100 Subject: [PATCH 1/4] feat: split FileDiff into metadata FileDiff + hunk-bearing FileDiffDetail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prepares the review components for the backend phase that returns metadata-only file lists from getPhaseReviewDiff. FileDiff now holds only path/status/additions/deletions; FileDiffDetail extends it with hunks. Renames changeType→status and adds 'binary' to the union. Also fixes two pre-existing TypeScript errors: InitiativeReview was passing an unknown `comments` prop to DiffViewer (should be commentsByLine), and ConflictResolutionPanel destructured an unused `agent` variable. Co-Authored-By: Claude Sonnet 4.6 --- .../review/ConflictResolutionPanel.tsx | 2 +- apps/web/src/components/review/DiffViewer.tsx | 4 +-- apps/web/src/components/review/FileCard.tsx | 14 +++++---- .../components/review/InitiativeReview.tsx | 2 +- .../src/components/review/ReviewSidebar.tsx | 6 ++-- apps/web/src/components/review/parse-diff.ts | 22 +++++++------- apps/web/src/components/review/types.test.ts | 29 +++++++++++++++++++ apps/web/src/components/review/types.ts | 13 ++++++--- 8 files changed, 65 insertions(+), 27 deletions(-) create mode 100644 apps/web/src/components/review/types.test.ts diff --git a/apps/web/src/components/review/ConflictResolutionPanel.tsx b/apps/web/src/components/review/ConflictResolutionPanel.tsx index ea2ff36..95dc750 100644 --- a/apps/web/src/components/review/ConflictResolutionPanel.tsx +++ b/apps/web/src/components/review/ConflictResolutionPanel.tsx @@ -11,7 +11,7 @@ interface ConflictResolutionPanelProps { } export function ConflictResolutionPanel({ initiativeId, conflicts, onResolved }: ConflictResolutionPanelProps) { - const { state, agent, questions, spawn, resume, stop, dismiss } = useConflictAgent(initiativeId); + const { state, agent: _agent, questions, spawn, resume, stop, dismiss } = useConflictAgent(initiativeId); const [showManual, setShowManual] = useState(false); const prevStateRef = useRef(null); diff --git a/apps/web/src/components/review/DiffViewer.tsx b/apps/web/src/components/review/DiffViewer.tsx index d4cfd1e..1ffbe22 100644 --- a/apps/web/src/components/review/DiffViewer.tsx +++ b/apps/web/src/components/review/DiffViewer.tsx @@ -1,4 +1,4 @@ -import type { FileDiff, DiffLine, ReviewComment } from "./types"; +import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { FileCard } from "./FileCard"; function getFileCommentMap( @@ -13,7 +13,7 @@ function getFileCommentMap( } interface DiffViewerProps { - files: FileDiff[]; + files: FileDiffDetail[]; commentsByLine: Map; onAddComment: ( filePath: string, diff --git a/apps/web/src/components/review/FileCard.tsx b/apps/web/src/components/review/FileCard.tsx index 4d6ed6f..4c84769 100644 --- a/apps/web/src/components/review/FileCard.tsx +++ b/apps/web/src/components/review/FileCard.tsx @@ -8,12 +8,12 @@ import { Circle, } from "lucide-react"; import { Badge } from "@/components/ui/badge"; -import type { FileDiff, FileChangeType, DiffLine, ReviewComment } from "./types"; +import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { HunkRows } from "./HunkRows"; import { useHighlightedFile } from "./use-syntax-highlight"; const changeTypeBadge: Record< - FileChangeType, + FileDiffDetail['status'], { label: string; classes: string } | null > = { added: { @@ -32,17 +32,19 @@ const changeTypeBadge: Record< "bg-status-active-bg text-status-active-fg border-status-active-border", }, modified: null, + binary: null, }; -const leftBorderClass: Record = { +const leftBorderClass: Record = { added: "border-l-2 border-l-status-success-fg", deleted: "border-l-2 border-l-status-error-fg", renamed: "border-l-2 border-l-status-active-fg", modified: "border-l-2 border-l-primary/40", + binary: "border-l-2 border-l-primary/40", }; interface FileCardProps { - file: FileDiff; + file: FileDiffDetail; commentsByLine: Map; onAddComment: ( filePath: string, @@ -80,7 +82,7 @@ export function FileCard({ [commentsByLine], ); - const badge = changeTypeBadge[file.changeType]; + const badge = changeTypeBadge[file.status]; // Flatten all hunk lines for syntax highlighting const allLines = useMemo( @@ -93,7 +95,7 @@ export function FileCard({
{/* File header — sticky so it stays visible when scrolling */} + )} {/* Preview controls */} {preview && } diff --git a/apps/web/src/components/review/ReviewSidebar.tsx b/apps/web/src/components/review/ReviewSidebar.tsx index 4e344d1..c3b8c53 100644 --- a/apps/web/src/components/review/ReviewSidebar.tsx +++ b/apps/web/src/components/review/ReviewSidebar.tsx @@ -310,7 +310,7 @@ function FilesView({ const isInView = activeFilePaths.has(file.newPath); const dimmed = selectedCommit && !isInView; const isViewed = viewedFiles.has(file.newPath); - const dotColor = changeTypeDotColor[file.changeType]; + const dotColor = changeTypeDotColor[file.status]; return ( + )} +
+ ), +})); + +vi.mock("./InitiativeReview", () => ({ + InitiativeReview: () =>
, +})); + +vi.mock("./comment-index", () => ({ + buildCommentIndex: vi.fn(() => new Map()), +})); + +vi.mock("sonner", () => ({ + toast: { success: vi.fn(), error: vi.fn() }, +})); + +// ── parseUnifiedDiff spy ─────────────────────────────────────────────────────── +const mockParseUnifiedDiff = vi.fn((_raw: string) => [ + { + oldPath: "a.ts", + newPath: "a.ts", + status: "modified" as const, + additions: 3, + deletions: 1, + hunks: [], + }, +]); + +vi.mock("./parse-diff", () => ({ + get parseUnifiedDiff() { + return mockParseUnifiedDiff; + }, +})); + +// ── tRPC mock factory ───────────────────────────────────────────────────────── + +const noopMutation = () => ({ + mutate: vi.fn(), + isPending: false, +}); + +const noopQuery = (data: unknown = undefined) => ({ + data, + isLoading: false, + isError: false, + refetch: vi.fn(), +}); + +const mockUtils = { + listReviewComments: { invalidate: vi.fn() }, +}; + +// Server format (FileStatEntry): uses `path` not `newPath` +const PHASE_FILES = [ + { + path: "a.ts", + status: "modified" as const, + additions: 5, + deletions: 2, + }, +]; + +// trpcMock is a let so tests can override it. The getter in the mock reads the current value. +let trpcMock = buildTrpcMock(); + +function buildTrpcMock(overrides: Record = {}) { + return { + getInitiative: { useQuery: vi.fn(() => noopQuery({ status: "in_progress" })) }, + listPhases: { + useQuery: vi.fn(() => + noopQuery([{ id: "phase-1", name: "Phase 1", status: "pending_review" }]) + ), + }, + getInitiativeProjects: { useQuery: vi.fn(() => noopQuery([{ id: "proj-1" }])) }, + getPhaseReviewDiff: { + useQuery: vi.fn(() => + noopQuery({ + phaseName: "Phase 1", + sourceBranch: "cw/phase-1", + targetBranch: "main", + files: PHASE_FILES, + totalAdditions: 5, + totalDeletions: 2, + }) + ), + }, + getPhaseReviewCommits: { + useQuery: vi.fn(() => + noopQuery({ commits: [], sourceBranch: "cw/phase-1", targetBranch: "main" }) + ), + }, + getCommitDiff: { + useQuery: vi.fn(() => noopQuery({ rawDiff: "" })), + }, + listPreviews: { useQuery: vi.fn(() => noopQuery([])) }, + getPreviewStatus: { useQuery: vi.fn(() => noopQuery(null)) }, + listReviewComments: { useQuery: vi.fn(() => noopQuery([])) }, + startPreview: { useMutation: vi.fn(() => noopMutation()) }, + stopPreview: { useMutation: vi.fn(() => noopMutation()) }, + createReviewComment: { useMutation: vi.fn(() => noopMutation()) }, + resolveReviewComment: { useMutation: vi.fn(() => noopMutation()) }, + unresolveReviewComment: { useMutation: vi.fn(() => noopMutation()) }, + replyToReviewComment: { useMutation: vi.fn(() => noopMutation()) }, + updateReviewComment: { useMutation: vi.fn(() => noopMutation()) }, + approvePhaseReview: { useMutation: vi.fn(() => noopMutation()) }, + requestPhaseChanges: { useMutation: vi.fn(() => noopMutation()) }, + useUtils: vi.fn(() => mockUtils), + ...overrides, + }; +} + +vi.mock("@/lib/trpc", () => ({ + get trpc() { + return trpcMock; + }, +})); + +// ── Import component after mocks ────────────────────────────────────────────── +import { ReviewTab } from "./ReviewTab"; + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("ReviewTab", () => { + beforeEach(() => { + diffViewerProps = {}; + reviewSidebarProps = {}; + mockParseUnifiedDiff.mockClear(); + trpcMock = buildTrpcMock(); + }); + + it("1. phase diff loads metadata: DiffViewer receives files array and commitMode=false", () => { + render(); + + expect(screen.getByTestId("diff-viewer")).toBeInTheDocument(); + const files = diffViewerProps.files as unknown[]; + expect(files).toHaveLength(1); + expect(diffViewerProps.commitMode).toBe(false); + }); + + it("2. no rawDiff parsing in phase mode: parseUnifiedDiff is NOT called", () => { + render(); + + expect(mockParseUnifiedDiff).not.toHaveBeenCalled(); + }); + + it("3. commit view parses rawDiff: parseUnifiedDiff called and DiffViewer gets commitMode=true", async () => { + trpcMock = buildTrpcMock({ + getCommitDiff: { + useQuery: vi.fn(() => + noopQuery({ rawDiff: "diff --git a/a.ts b/a.ts\nindex 000..111 100644\n--- a/a.ts\n+++ b/a.ts\n@@ -1,1 +1,1 @@\n-old\n+new\n" }) + ), + }, + }); + + render(); + + // Select a commit via the sidebar stub's onSelectCommit prop + const { onSelectCommit } = reviewSidebarProps as { + onSelectCommit: (hash: string | null) => void; + }; + + await act(async () => { + onSelectCommit("abc123"); + }); + + expect(diffViewerProps.commitMode).toBe(true); + expect(mockParseUnifiedDiff).toHaveBeenCalled(); + }); + + it("4. allFiles uses metadata for sidebar: ReviewSidebar receives files from diffQuery.data.files", () => { + render(); + + const sidebarFiles = reviewSidebarProps.files as Array<{ newPath: string }>; + expect(sidebarFiles).toHaveLength(1); + expect(sidebarFiles[0].newPath).toBe("a.ts"); + }); + + it("5. expandAll prop passed: clicking Expand all button causes DiffViewer to receive expandAll=true", async () => { + render(); + + // Before clicking, expandAll should be false + expect(diffViewerProps.expandAll).toBe(false); + + const expandBtn = screen.getByTestId("expand-all-btn"); + await act(async () => { + expandBtn.click(); + }); + + expect(diffViewerProps.expandAll).toBe(true); + }); +}); diff --git a/apps/web/src/components/review/ReviewTab.tsx b/apps/web/src/components/review/ReviewTab.tsx index 8d0a2c0..5d51b20 100644 --- a/apps/web/src/components/review/ReviewTab.tsx +++ b/apps/web/src/components/review/ReviewTab.tsx @@ -8,7 +8,7 @@ import { ReviewSidebar } from "./ReviewSidebar"; import { ReviewHeader } from "./ReviewHeader"; import { InitiativeReview } from "./InitiativeReview"; import { buildCommentIndex } from "./comment-index"; -import type { ReviewStatus, DiffLine } from "./types"; +import type { ReviewStatus, DiffLine, FileDiff, FileDiffDetail } from "./types"; interface ReviewTabProps { initiativeId: string; @@ -18,6 +18,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { const [status, setStatus] = useState("pending"); const [selectedCommit, setSelectedCommit] = useState(null); const [viewedFiles, setViewedFiles] = useState>(new Set()); + const [expandAll, setExpandAll] = useState(false); const fileRefs = useRef>(new Map()); const headerRef = useRef(null); const [headerHeight, setHeaderHeight] = useState(0); @@ -74,7 +75,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { const projectsQuery = trpc.getInitiativeProjects.useQuery({ initiativeId }); const firstProjectId = projectsQuery.data?.[0]?.id ?? null; - // Fetch full branch diff for active phase + // Fetch full branch diff for active phase (metadata only, no rawDiff) const diffQuery = trpc.getPhaseReviewDiff.useQuery( { phaseId: activePhaseId! }, { enabled: !!activePhaseId && !isInitiativePendingReview }, @@ -96,7 +97,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { // Preview state const previewsQuery = trpc.listPreviews.useQuery({ initiativeId }); const existingPreview = previewsQuery.data?.find( - (p) => p.phaseId === activePhaseId || p.initiativeId === initiativeId, + (p: { phaseId?: string; initiativeId?: string }) => p.phaseId === activePhaseId || p.initiativeId === initiativeId, ); const [activePreviewId, setActivePreviewId] = useState(null); const previewStatusQuery = trpc.getPreviewStatus.useQuery( @@ -107,12 +108,12 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { const sourceBranch = diffQuery.data?.sourceBranch ?? commitsQuery.data?.sourceBranch ?? ""; const startPreview = trpc.startPreview.useMutation({ - onSuccess: (data) => { + onSuccess: (data: { id: string; url: string }) => { setActivePreviewId(data.id); previewsQuery.refetch(); toast.success(`Preview running at ${data.url}`); }, - onError: (err) => toast.error(`Preview failed: ${err.message}`), + onError: (err: { message: string }) => toast.error(`Preview failed: ${err.message}`), }); const stopPreview = trpc.stopPreview.useMutation({ @@ -121,7 +122,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { toast.success("Preview stopped"); previewsQuery.refetch(); }, - onError: (err) => toast.error(`Failed to stop: ${err.message}`), + onError: (err: { message: string }) => toast.error(`Failed to stop: ${err.message}`), }); const previewState = firstProjectId && sourceBranch @@ -157,7 +158,17 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { { enabled: !!activePhaseId && !isInitiativePendingReview }, ); const comments = useMemo(() => { - return (commentsQuery.data ?? []).map((c) => ({ + return (commentsQuery.data ?? []).map((c: { + id: string; + filePath: string; + lineNumber: number | null; + lineType: string; + body: string; + author: string; + createdAt: string | number; + resolved: boolean; + parentCommentId?: string | null; + }) => ({ id: c.id, filePath: c.filePath, lineNumber: c.lineNumber, @@ -179,7 +190,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { onSuccess: () => { utils.listReviewComments.invalidate({ phaseId: activePhaseId! }); }, - onError: (err) => toast.error(`Failed to save comment: ${err.message}`), + onError: (err: { message: string }) => toast.error(`Failed to save comment: ${err.message}`), }); const resolveCommentMutation = trpc.resolveReviewComment.useMutation({ @@ -198,14 +209,14 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { onSuccess: () => { utils.listReviewComments.invalidate({ phaseId: activePhaseId! }); }, - onError: (err) => toast.error(`Failed to post reply: ${err.message}`), + onError: (err: { message: string }) => toast.error(`Failed to post reply: ${err.message}`), }); const editCommentMutation = trpc.updateReviewComment.useMutation({ onSuccess: () => { utils.listReviewComments.invalidate({ phaseId: activePhaseId! }); }, - onError: (err) => toast.error(`Failed to update comment: ${err.message}`), + onError: (err: { message: string }) => toast.error(`Failed to update comment: ${err.message}`), }); const approveMutation = trpc.approvePhaseReview.useMutation({ @@ -214,23 +225,48 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { toast.success("Phase approved and merged"); phasesQuery.refetch(); }, - onError: (err) => toast.error(err.message), + onError: (err: { message: string }) => toast.error(err.message), }); - // Determine which diff to display - const activeDiffRaw = selectedCommit - ? commitDiffQuery.data?.rawDiff - : diffQuery.data?.rawDiff; + // Phase branch diff — metadata only, no parsing + const phaseFiles: FileDiff[] = useMemo( + () => { + const serverFiles = diffQuery.data?.files ?? []; + // Map server FileStatEntry (path) to frontend FileDiff (newPath) + return serverFiles.map((f: { + path: string; + oldPath?: string; + status: FileDiff['status']; + additions: number; + deletions: number; + projectId?: string; + }) => ({ + newPath: f.path, + oldPath: f.oldPath ?? f.path, + status: f.status, + additions: f.additions, + deletions: f.deletions, + projectId: f.projectId, + })); + }, + [diffQuery.data?.files], + ); - const files = useMemo(() => { - if (!activeDiffRaw) return []; - return parseUnifiedDiff(activeDiffRaw); - }, [activeDiffRaw]); + // Commit diff — still raw, parse client-side + const commitFiles: FileDiffDetail[] = useMemo(() => { + if (!commitDiffQuery.data?.rawDiff) return []; + return parseUnifiedDiff(commitDiffQuery.data.rawDiff); + }, [commitDiffQuery.data?.rawDiff]); const isDiffLoading = selectedCommit ? commitDiffQuery.isLoading : diffQuery.isLoading; + // All files for sidebar — always from phase metadata + const allFiles = phaseFiles; + + const activeFiles: FileDiff[] | FileDiffDetail[] = selectedCommit ? commitFiles : phaseFiles; + const handleAddComment = useCallback( (filePath: string, lineNumber: number, lineType: DiffLine["type"], body: string) => { if (!activePhaseId) return; @@ -273,7 +309,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { toast.success("Changes requested — revision task dispatched"); phasesQuery.refetch(); }, - onError: (err) => toast.error(err.message), + onError: (err: { message: string }) => toast.error(err.message), }); const handleRequestChanges = useCallback(() => { @@ -303,6 +339,11 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { setSelectedCommit(null); setStatus("pending"); setViewedFiles(new Set()); + setExpandAll(false); + }, []); + + const handleExpandAll = useCallback(() => { + setExpandAll(v => !v); }, []); const unresolvedCount = comments.filter((c) => !c.resolved && !c.parentCommentId).length; @@ -312,12 +353,6 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { reviewablePhases.find((p) => p.id === activePhaseId)?.name ?? "Phase"; - // All files from the full branch diff (for sidebar file list) - const allFiles = useMemo(() => { - if (!diffQuery.data?.rawDiff) return []; - return parseUnifiedDiff(diffQuery.data.rawDiff); - }, [diffQuery.data?.rawDiff]); - // Initiative-level review takes priority if (isInitiativePendingReview) { return ( @@ -363,6 +398,9 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { preview={previewState} viewedCount={viewedFiles.size} totalCount={allFiles.length} + totalAdditions={selectedCommit ? undefined : diffQuery.data?.totalAdditions} + totalDeletions={selectedCommit ? undefined : diffQuery.data?.totalDeletions} + onExpandAll={handleExpandAll} /> {/* Main content area — sidebar always rendered to preserve state */} @@ -382,7 +420,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { onFileClick={handleFileClick} onCommentClick={handleCommentClick} selectedCommit={selectedCommit} - activeFiles={files} + activeFiles={activeFiles} commits={commits} onSelectCommit={setSelectedCommit} viewedFiles={viewedFiles} @@ -397,7 +435,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { Loading diff...
- ) : files.length === 0 ? ( + ) : activeFiles.length === 0 ? (
{selectedCommit ? "No changes in this commit" @@ -405,7 +443,10 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
) : ( )} diff --git a/apps/web/src/components/review/parse-diff.ts b/apps/web/src/components/review/parse-diff.ts index 8a5697e..7e5013f 100644 --- a/apps/web/src/components/review/parse-diff.ts +++ b/apps/web/src/components/review/parse-diff.ts @@ -1,10 +1,10 @@ -import type { FileDiff, FileChangeType, DiffHunk, DiffLine } from "./types"; +import type { FileDiffDetail, DiffHunk, DiffLine } from "./types"; /** - * Parse a unified diff string into structured FileDiff objects. + * Parse a unified diff string into structured FileDiffDetail objects. */ -export function parseUnifiedDiff(raw: string): FileDiff[] { - const files: FileDiff[] = []; +export function parseUnifiedDiff(raw: string): FileDiffDetail[] { + const files: FileDiffDetail[] = []; const fileChunks = raw.split(/^diff --git /m).filter(Boolean); for (const chunk of fileChunks) { @@ -90,19 +90,19 @@ export function parseUnifiedDiff(raw: string): FileDiff[] { hunks.push({ header, oldStart, oldCount, newStart, newCount, lines: hunkLines }); } - // Derive changeType from header markers and path comparison - let changeType: FileChangeType; + // Derive status from header markers and path comparison + let status: FileDiffDetail['status']; if (hasOldDevNull) { - changeType = "added"; + status = "added"; } else if (hasNewDevNull) { - changeType = "deleted"; + status = "deleted"; } else if (oldPath !== newPath) { - changeType = "renamed"; + status = "renamed"; } else { - changeType = "modified"; + status = "modified"; } - files.push({ oldPath, newPath, hunks, additions, deletions, changeType }); + files.push({ oldPath, newPath, hunks, additions, deletions, status }); } return files; diff --git a/apps/web/src/components/review/types.ts b/apps/web/src/components/review/types.ts index 2f9a1ad..248933c 100644 --- a/apps/web/src/components/review/types.ts +++ b/apps/web/src/components/review/types.ts @@ -14,15 +14,22 @@ export interface DiffLine { newLineNumber: number | null; } +/** @deprecated Use FileDiff.status instead */ export type FileChangeType = 'added' | 'modified' | 'deleted' | 'renamed'; +/** Metadata returned by getPhaseReviewDiff — no hunk content */ export interface FileDiff { oldPath: string; newPath: string; - hunks: DiffHunk[]; + status: 'added' | 'modified' | 'deleted' | 'renamed' | 'binary'; additions: number; deletions: number; - changeType: FileChangeType; + projectId?: string; +} + +/** Full diff with parsed hunks — returned by getFileDiff and parsed client-side */ +export interface FileDiffDetail extends FileDiff { + hunks: DiffHunk[]; } export interface ReviewComment { diff --git a/docs/frontend.md b/docs/frontend.md index 0797687..ae7e448 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -113,10 +113,10 @@ The initiative detail page has three tabs managed via local state (not URL param ### Review Components (`src/components/review/`) | Component | Purpose | |-----------|---------| -| `ReviewTab` | Review tab container — orchestrates header, diff, sidebar, and preview. Phase-level review has threaded inline comments (with reply support) + Request Changes; initiative-level review has Request Changes (summary prompt) + Push Branch / Merge & Push | -| `ReviewHeader` | Consolidated toolbar: phase selector pills, branch info, stats, preview controls, approve/reject actions | +| `ReviewTab` | Review tab container — orchestrates header, diff, sidebar, and preview. Phase diff uses metadata-only `FileDiff[]` from `getPhaseReviewDiff` (no rawDiff parsing). Commit diff parses `rawDiff` via `parseUnifiedDiff` → `FileDiffDetail[]`. Passes `commitMode`, `phaseId`, `expandAll` to DiffViewer | +| `ReviewHeader` | Consolidated toolbar: phase selector pills, branch info, stats (uses `totalAdditions`/`totalDeletions` props when available, falls back to summing files), preview controls, Expand all button, approve/reject actions | | `ReviewSidebar` | VSCode-style icon strip (Files/Commits views) with file list, root-only comment counts, and commit navigation | -| `DiffViewer` | Unified diff renderer with threaded inline comments (root + reply threads) | +| `DiffViewer` | Unified diff renderer with threaded inline comments (root + reply threads). Accepts `FileDiff[] | FileDiffDetail[]`, `phaseId`, `commitMode`, `expandAll` props | | `CommentThread` | Renders root comment with resolve/reopen + nested reply threads (agent replies styled with primary border). Inline reply form | | `ConflictResolutionPanel` | Merge conflict detection + agent resolution in initiative review. Shows conflict files, spawns conflict agent, inline questions, re-check on completion | | `PreviewPanel` | Docker preview status: building/running/failed with start/stop (legacy, now integrated into ReviewHeader) | From f804cb197c9aa07ffd8bfdb9808837caf3470be9 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 20:13:01 +0100 Subject: [PATCH 3/4] feat: viewport virtualization for DiffViewer + lazy per-file hunk loading in FileCard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DiffViewer now uses IntersectionObserver to replace off-viewport FileCards with 48px placeholder divs, eliminating thousands of DOM nodes at initial render for large diffs. Files within 1× viewport buffer are rendered as real FileCards. FileCard defaults to collapsed state and only fires getFileDiff when expanded, with staleTime: Infinity to avoid re-fetches. Handles loading/error/binary/ no-hunks states. Commit mode passes detail prop to skip lazy loading entirely. DiffViewer batches expand-all in chunks of 10, prefetching via tRPC utils to saturate the network without blocking the UI. Co-Authored-By: Claude Sonnet 4.6 --- apps/web/src/components/review/DiffViewer.tsx | 173 ++++++++++++++++-- apps/web/src/components/review/FileCard.tsx | 166 ++++++++++++----- apps/web/src/components/review/ReviewTab.tsx | 2 + 3 files changed, 280 insertions(+), 61 deletions(-) diff --git a/apps/web/src/components/review/DiffViewer.tsx b/apps/web/src/components/review/DiffViewer.tsx index 1ffbe22..5e9c536 100644 --- a/apps/web/src/components/review/DiffViewer.tsx +++ b/apps/web/src/components/review/DiffViewer.tsx @@ -1,5 +1,8 @@ -import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; +import { useCallback, useEffect, useRef, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { FileCard } from "./FileCard"; +import { trpc } from "@/lib/trpc"; function getFileCommentMap( commentsByLine: Map, @@ -13,7 +16,9 @@ function getFileCommentMap( } interface DiffViewerProps { - files: FileDiffDetail[]; + files: (FileDiff | FileDiffDetail)[]; + phaseId: string; + commitMode: boolean; commentsByLine: Map; onAddComment: ( filePath: string, @@ -28,10 +33,13 @@ interface DiffViewerProps { viewedFiles?: Set; onToggleViewed?: (filePath: string) => void; onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void; + expandAll?: boolean; } export function DiffViewer({ files, + phaseId, + commitMode, commentsByLine, onAddComment, onResolveComment, @@ -41,24 +49,155 @@ export function DiffViewer({ viewedFiles, onToggleViewed, onRegisterRef, + expandAll, }: DiffViewerProps) { + // Set of file paths currently intersecting (or near) the viewport + const visibleFiles = useRef>(new Set()); + // Map from filePath → wrapper div ref + const wrapperRefs = useRef>(new Map()); + // Increment to trigger re-render when visibility changes + const [visibilityVersion, setVisibilityVersion] = useState(0); + + // Single IntersectionObserver for all wrappers + const observerRef = useRef(null); + + useEffect(() => { + if (files.length === 1) return; // skip for single file + + observerRef.current = new IntersectionObserver( + (entries) => { + let changed = false; + for (const entry of entries) { + const filePath = (entry.target as HTMLDivElement).dataset['filePath']; + if (!filePath) continue; + if (entry.isIntersecting) { + if (!visibleFiles.current.has(filePath)) { + visibleFiles.current.add(filePath); + changed = true; + } + } else { + if (visibleFiles.current.has(filePath)) { + visibleFiles.current.delete(filePath); + changed = true; + } + } + } + if (changed) setVisibilityVersion((v) => v + 1); + }, + { rootMargin: '100% 0px 100% 0px' }, // 1× viewport above and below + ); + + // Observe all current wrapper divs + for (const el of wrapperRefs.current.values()) { + observerRef.current.observe(el); + } + + return () => { + observerRef.current?.disconnect(); + }; + }, [files]); // re-create observer when file list changes + + // Register wrapper ref — observes the div, registers with parent + const registerWrapper = useCallback( + (filePath: string, el: HTMLDivElement | null) => { + if (el) { + wrapperRefs.current.set(filePath, el); + observerRef.current?.observe(el); + } else { + const prev = wrapperRefs.current.get(filePath); + if (prev) observerRef.current?.unobserve(prev); + wrapperRefs.current.delete(filePath); + } + onRegisterRef?.(filePath, el); + }, + [onRegisterRef], + ); + + // expandAll batch loading + const [expandedFiles, setExpandedFiles] = useState>(new Set()); + const queryClient = useQueryClient(); + const utils = trpc.useUtils(); + + useEffect(() => { + if (!expandAll || files.length === 0) return; + + const BATCH = 10; + let cancelled = false; + + async function batchExpand() { + const chunks: (FileDiff | FileDiffDetail)[][] = []; + for (let i = 0; i < files.length; i += BATCH) { + chunks.push(files.slice(i, i + BATCH)); + } + + for (const chunk of chunks) { + if (cancelled) break; + // Mark this batch as expanded (triggers FileCard renders + queries) + setExpandedFiles((prev) => { + const next = new Set(prev); + for (const f of chunk) { + if (f.status !== 'binary') next.add(f.newPath); + } + return next; + }); + // Eagerly prefetch via React Query to saturate network + await Promise.all( + chunk + .filter((f) => f.status !== 'binary' && !('hunks' in f)) + .map((f) => + utils.getFileDiff + .fetch({ phaseId, filePath: encodeURIComponent(f.newPath) }) + .catch(() => null), // swallow per-file errors; FileCard shows its own error state + ), + ); + } + } + + batchExpand(); + return () => { + cancelled = true; + }; + }, [expandAll]); // only re-run when expandAll toggles + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally only on expandAll + + // Suppress unused variable warning — used only to force re-render on visibility change + void visibilityVersion; + + const isSingleFile = files.length === 1; + return (
- {files.map((file) => ( -
onRegisterRef?.(file.newPath, el)}> - onToggleViewed?.(file.newPath)} - /> -
- ))} + {files.map((file) => { + const isVisible = isSingleFile || visibleFiles.current.has(file.newPath); + const isExpandedOverride = expandedFiles.has(file.newPath) ? true : undefined; + return ( +
registerWrapper(file.newPath, el)} + data-file-path={file.newPath} + > + {isVisible ? ( + onToggleViewed?.(file.newPath)} + /> + ) : ( +
+ )} +
+ ); + })}
); } diff --git a/apps/web/src/components/review/FileCard.tsx b/apps/web/src/components/review/FileCard.tsx index 4c84769..12cfb14 100644 --- a/apps/web/src/components/review/FileCard.tsx +++ b/apps/web/src/components/review/FileCard.tsx @@ -6,16 +6,16 @@ import { Minus, CheckCircle2, Circle, + Loader2, } from "lucide-react"; import { Badge } from "@/components/ui/badge"; -import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; +import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { HunkRows } from "./HunkRows"; import { useHighlightedFile } from "./use-syntax-highlight"; +import { parseUnifiedDiff } from "./parse-diff"; +import { trpc } from "@/lib/trpc"; -const changeTypeBadge: Record< - FileDiffDetail['status'], - { label: string; classes: string } | null -> = { +const statusBadge: Record = { added: { label: "NEW", classes: @@ -32,10 +32,13 @@ const changeTypeBadge: Record< "bg-status-active-bg text-status-active-fg border-status-active-border", }, modified: null, - binary: null, + binary: { + label: "BINARY", + classes: "bg-muted text-muted-foreground border-border", + }, }; -const leftBorderClass: Record = { +const leftBorderClass: Record = { added: "border-l-2 border-l-status-success-fg", deleted: "border-l-2 border-l-status-error-fg", renamed: "border-l-2 border-l-status-active-fg", @@ -44,8 +47,12 @@ const leftBorderClass: Record = { }; interface FileCardProps { - file: FileDiffDetail; + file: FileDiff; + detail?: FileDiffDetail; + phaseId: string; + commitMode: boolean; commentsByLine: Map; + isExpandedOverride?: boolean; onAddComment: ( filePath: string, lineNumber: number, @@ -62,7 +69,11 @@ interface FileCardProps { export function FileCard({ file, + detail, + phaseId, + commitMode, commentsByLine, + isExpandedOverride, onAddComment, onResolveComment, onUnresolveComment, @@ -71,35 +82,65 @@ export function FileCard({ isViewed = false, onToggleViewed = () => {}, }: FileCardProps) { - const [expanded, setExpanded] = useState(true); + // Uncontrolled expand for normal file clicks. + // Start expanded if detail prop is provided (commit mode). + const [isExpandedLocal, setIsExpandedLocal] = useState(() => !!detail); - const commentCount = useMemo( - () => - Array.from(commentsByLine.values()).reduce( - (sum, arr) => sum + arr.length, - 0, - ), - [commentsByLine], + // Merge with override from DiffViewer expandAll + const isExpanded = isExpandedOverride ?? isExpandedLocal; + + const fileDiffQuery = trpc.getFileDiff.useQuery( + { phaseId, filePath: encodeURIComponent(file.newPath) }, + { + enabled: isExpanded && !commitMode && file.status !== 'binary' && !detail, + staleTime: Infinity, + }, ); - const badge = changeTypeBadge[file.status]; + // Compute hunks from query data (phase mode) + const parsedHunks = useMemo(() => { + if (!fileDiffQuery.data?.rawDiff) return null; + const parsed = parseUnifiedDiff(fileDiffQuery.data.rawDiff); + return parsed[0] ?? null; + }, [fileDiffQuery.data]); + + // Collect all lines for syntax highlighting + const allLines = useMemo(() => { + if (detail) return detail.hunks.flatMap((h) => h.lines); + if (parsedHunks) return parsedHunks.hunks.flatMap((h) => h.lines); + return []; + }, [detail, parsedHunks]); - // Flatten all hunk lines for syntax highlighting - const allLines = useMemo( - () => file.hunks.flatMap((h) => h.lines), - [file.hunks], - ); const tokenMap = useHighlightedFile(file.newPath, allLines); + const commentCount = useMemo(() => { + let count = 0; + for (const [key, arr] of commentsByLine) { + if (key.startsWith(`${file.newPath}:`)) count += arr.length; + } + return count; + }, [commentsByLine, file.newPath]); + + const badge = statusBadge[file.status]; + + const handlers = { + onAddComment, + onResolveComment, + onUnresolveComment, + onReplyComment, + onEditComment, + tokenMap, + }; + return (
- {/* File header — sticky so it stays visible when scrolling */} + {/* File header */} {/* Diff content */} - {expanded && ( + {isExpanded && (
- - - {file.hunks.map((hunk, hi) => ( - - ))} - -
+ {detail ? ( + // Commit mode: pre-parsed hunks from detail prop + detail.hunks.length === 0 ? ( +
No content changes
+ ) : ( + + + {detail.hunks.map((hunk, hi) => ( + + ))} + +
+ ) + ) : file.status === 'binary' ? ( +
Binary file — diff not shown
+ ) : fileDiffQuery.isLoading ? ( +
+ + Loading diff… +
+ ) : fileDiffQuery.isError ? ( +
+ Failed to load diff. + +
+ ) : fileDiffQuery.data ? ( + !parsedHunks || parsedHunks.hunks.length === 0 ? ( +
No content changes
+ ) : ( + + + {parsedHunks.hunks.map((hunk, hi) => ( + + ))} + +
+ ) + ) : null}
)}
diff --git a/apps/web/src/components/review/ReviewTab.tsx b/apps/web/src/components/review/ReviewTab.tsx index 8d0a2c0..df91c58 100644 --- a/apps/web/src/components/review/ReviewTab.tsx +++ b/apps/web/src/components/review/ReviewTab.tsx @@ -406,6 +406,8 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { ) : ( Date: Fri, 6 Mar 2026 20:25:48 +0100 Subject: [PATCH 4/4] test: add DiffViewer and FileCard viewport virtualization tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers IntersectionObserver placeholder rendering for 300-file diffs, single-file bypass, sidebar ref registration, expandAll batch fetching (25 files → 3 batches of 10), and all FileCard lazy-load states: default collapsed, loading, success with HunkRows, error+retry, binary, no-hunks, detail-prop pre-expanded, and collapse/re-expand UX. Cherry-picks viewport virtualization implementation commit (f804cb19) onto this branch so the tests run against the actual new code. Co-Authored-By: Claude Sonnet 4.6 --- .../src/components/review/DiffViewer.test.tsx | 192 +++++++++++++ apps/web/src/components/review/DiffViewer.tsx | 173 +++++++++-- .../src/components/review/FileCard.test.tsx | 270 ++++++++++++++++++ apps/web/src/components/review/FileCard.tsx | 166 ++++++++--- apps/web/src/components/review/ReviewTab.tsx | 2 + 5 files changed, 742 insertions(+), 61 deletions(-) create mode 100644 apps/web/src/components/review/DiffViewer.test.tsx create mode 100644 apps/web/src/components/review/FileCard.test.tsx diff --git a/apps/web/src/components/review/DiffViewer.test.tsx b/apps/web/src/components/review/DiffViewer.test.tsx new file mode 100644 index 0000000..062de65 --- /dev/null +++ b/apps/web/src/components/review/DiffViewer.test.tsx @@ -0,0 +1,192 @@ +// @vitest-environment happy-dom +import "@testing-library/jest-dom/vitest"; +import { render, screen, act } from "@testing-library/react"; +import { vi, describe, it, expect, beforeEach, afterEach } from "vitest"; +import { DiffViewer } from "./DiffViewer"; +import type { FileDiff } from "./types"; + +// ── Module mocks ────────────────────────────────────────────────────────────── + +vi.mock("./FileCard", () => ({ + FileCard: ({ file }: { file: FileDiff }) => ( +
+ ), +})); + +// Hoist the fetch mock so it can be referenced inside vi.mock factories +const { mockGetFileDiffFetch } = vi.hoisted(() => ({ + mockGetFileDiffFetch: vi.fn().mockResolvedValue({ rawDiff: "" }), +})); + +vi.mock("@/lib/trpc", () => ({ + trpc: { + useUtils: () => ({ + getFileDiff: { fetch: mockGetFileDiffFetch }, + }), + }, +})); + +// DiffViewer calls useQueryClient() (even though the return value is unused). +// Provide a minimal mock so the hook doesn't throw outside a QueryClientProvider. +vi.mock("@tanstack/react-query", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, useQueryClient: () => ({}) }; +}); + +// ── IntersectionObserver mock ───────────────────────────────────────────────── + +let observerCallback: IntersectionObserverCallback | null = null; +const observedElements = new Set(); + +// Class (not arrow function) so it can be used with `new IntersectionObserver(...)` +class MockIntersectionObserver { + constructor(cb: IntersectionObserverCallback) { + observerCallback = cb; + } + observe(el: Element) { + observedElements.add(el); + } + unobserve(el: Element) { + observedElements.delete(el); + } + disconnect() { + observedElements.clear(); + } +} + +beforeEach(() => { + vi.stubGlobal("IntersectionObserver", MockIntersectionObserver); + observedElements.clear(); + observerCallback = null; + mockGetFileDiffFetch.mockClear(); + mockGetFileDiffFetch.mockResolvedValue({ rawDiff: "" }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +/** + * Fire the IntersectionObserver callback with a set of intersecting and + * non-intersecting file paths. The target element is simulated by an object + * whose dataset.filePath matches the DiffViewer's data-file-path attribute. + */ +function fireIntersection( + intersectingPaths: string[], + nonIntersectingPaths: string[] = [], +) { + if (!observerCallback) return; + const entries = [ + ...intersectingPaths.map((p) => ({ + isIntersecting: true, + target: { dataset: { filePath: p } } as unknown as Element, + })), + ...nonIntersectingPaths.map((p) => ({ + isIntersecting: false, + target: { dataset: { filePath: p } } as unknown as Element, + })), + ] as IntersectionObserverEntry[]; + act(() => { + observerCallback!(entries, {} as IntersectionObserver); + }); +} + +function makeFiles(count: number): FileDiff[] { + return Array.from({ length: count }, (_, i) => ({ + oldPath: `file${i}.ts`, + newPath: `file${i}.ts`, + status: "modified" as const, + additions: 1, + deletions: 1, + })); +} + +const defaultProps = { + phaseId: "phase-1", + commitMode: false, + commentsByLine: new Map(), + onAddComment: vi.fn(), + onResolveComment: vi.fn(), + onUnresolveComment: vi.fn(), +}; + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("DiffViewer", () => { + it("renders all FileCards when 5 files are all in viewport", () => { + const files = makeFiles(5); + render(); + + // Trigger all five as intersecting + fireIntersection(files.map((f) => f.newPath)); + + expect(screen.getAllByTestId("file-card")).toHaveLength(5); + }); + + it("shows only intersecting FileCards for 300 files, placeholders for the rest", () => { + const files = makeFiles(300); + render(); + + // Only first 5 files enter the viewport + fireIntersection(files.slice(0, 5).map((f) => f.newPath)); + + expect(screen.getAllByTestId("file-card")).toHaveLength(5); + + // The remaining 295 should be 48px placeholder divs marked aria-hidden + const placeholders = document.querySelectorAll( + '[aria-hidden][style*="height: 48px"]', + ); + expect(placeholders.length).toBeGreaterThanOrEqual(295); + }); + + it("skips IntersectionObserver for single-file diff and renders FileCard directly", () => { + render(); + + // Single-file path: isVisible is always true, no intersection event needed + expect(screen.getAllByTestId("file-card")).toHaveLength(1); + }); + + it("calls scrollIntoView on the wrapper div when onRegisterRef is used for sidebar navigation", () => { + const files = makeFiles(5); + const registeredRefs = new Map(); + const onRegisterRef = (filePath: string, el: HTMLDivElement | null) => { + if (el) registeredRefs.set(filePath, el); + }; + + render(); + + // All wrapper divs should have been registered (including the last one) + const targetFile = files[4].newPath; + expect(registeredRefs.has(targetFile)).toBe(true); + + const wrapperEl = registeredRefs.get(targetFile)!; + const scrollSpy = vi.fn(); + Object.defineProperty(wrapperEl, "scrollIntoView", { value: scrollSpy }); + + // Simulate a sidebar click that calls scrollIntoView on the wrapper + act(() => { + wrapperEl.scrollIntoView({ behavior: "smooth", block: "start" }); + }); + expect(scrollSpy).toHaveBeenCalledOnce(); + }); + + it("fires getFileDiff queries in batches of 10 when expandAll is toggled", async () => { + const files = makeFiles(25); // 3 batches: 10, 10, 5 + const { rerender } = render( + , + ); + + rerender(); + + // Wait for all async batch iterations to complete + await act(async () => { + await new Promise((r) => setTimeout(r, 100)); + }); + + // All 25 non-binary files should have been prefetched + expect(mockGetFileDiffFetch).toHaveBeenCalledTimes(25); + }); +}); diff --git a/apps/web/src/components/review/DiffViewer.tsx b/apps/web/src/components/review/DiffViewer.tsx index 1ffbe22..5e9c536 100644 --- a/apps/web/src/components/review/DiffViewer.tsx +++ b/apps/web/src/components/review/DiffViewer.tsx @@ -1,5 +1,8 @@ -import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; +import { useCallback, useEffect, useRef, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { FileCard } from "./FileCard"; +import { trpc } from "@/lib/trpc"; function getFileCommentMap( commentsByLine: Map, @@ -13,7 +16,9 @@ function getFileCommentMap( } interface DiffViewerProps { - files: FileDiffDetail[]; + files: (FileDiff | FileDiffDetail)[]; + phaseId: string; + commitMode: boolean; commentsByLine: Map; onAddComment: ( filePath: string, @@ -28,10 +33,13 @@ interface DiffViewerProps { viewedFiles?: Set; onToggleViewed?: (filePath: string) => void; onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void; + expandAll?: boolean; } export function DiffViewer({ files, + phaseId, + commitMode, commentsByLine, onAddComment, onResolveComment, @@ -41,24 +49,155 @@ export function DiffViewer({ viewedFiles, onToggleViewed, onRegisterRef, + expandAll, }: DiffViewerProps) { + // Set of file paths currently intersecting (or near) the viewport + const visibleFiles = useRef>(new Set()); + // Map from filePath → wrapper div ref + const wrapperRefs = useRef>(new Map()); + // Increment to trigger re-render when visibility changes + const [visibilityVersion, setVisibilityVersion] = useState(0); + + // Single IntersectionObserver for all wrappers + const observerRef = useRef(null); + + useEffect(() => { + if (files.length === 1) return; // skip for single file + + observerRef.current = new IntersectionObserver( + (entries) => { + let changed = false; + for (const entry of entries) { + const filePath = (entry.target as HTMLDivElement).dataset['filePath']; + if (!filePath) continue; + if (entry.isIntersecting) { + if (!visibleFiles.current.has(filePath)) { + visibleFiles.current.add(filePath); + changed = true; + } + } else { + if (visibleFiles.current.has(filePath)) { + visibleFiles.current.delete(filePath); + changed = true; + } + } + } + if (changed) setVisibilityVersion((v) => v + 1); + }, + { rootMargin: '100% 0px 100% 0px' }, // 1× viewport above and below + ); + + // Observe all current wrapper divs + for (const el of wrapperRefs.current.values()) { + observerRef.current.observe(el); + } + + return () => { + observerRef.current?.disconnect(); + }; + }, [files]); // re-create observer when file list changes + + // Register wrapper ref — observes the div, registers with parent + const registerWrapper = useCallback( + (filePath: string, el: HTMLDivElement | null) => { + if (el) { + wrapperRefs.current.set(filePath, el); + observerRef.current?.observe(el); + } else { + const prev = wrapperRefs.current.get(filePath); + if (prev) observerRef.current?.unobserve(prev); + wrapperRefs.current.delete(filePath); + } + onRegisterRef?.(filePath, el); + }, + [onRegisterRef], + ); + + // expandAll batch loading + const [expandedFiles, setExpandedFiles] = useState>(new Set()); + const queryClient = useQueryClient(); + const utils = trpc.useUtils(); + + useEffect(() => { + if (!expandAll || files.length === 0) return; + + const BATCH = 10; + let cancelled = false; + + async function batchExpand() { + const chunks: (FileDiff | FileDiffDetail)[][] = []; + for (let i = 0; i < files.length; i += BATCH) { + chunks.push(files.slice(i, i + BATCH)); + } + + for (const chunk of chunks) { + if (cancelled) break; + // Mark this batch as expanded (triggers FileCard renders + queries) + setExpandedFiles((prev) => { + const next = new Set(prev); + for (const f of chunk) { + if (f.status !== 'binary') next.add(f.newPath); + } + return next; + }); + // Eagerly prefetch via React Query to saturate network + await Promise.all( + chunk + .filter((f) => f.status !== 'binary' && !('hunks' in f)) + .map((f) => + utils.getFileDiff + .fetch({ phaseId, filePath: encodeURIComponent(f.newPath) }) + .catch(() => null), // swallow per-file errors; FileCard shows its own error state + ), + ); + } + } + + batchExpand(); + return () => { + cancelled = true; + }; + }, [expandAll]); // only re-run when expandAll toggles + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally only on expandAll + + // Suppress unused variable warning — used only to force re-render on visibility change + void visibilityVersion; + + const isSingleFile = files.length === 1; + return (
- {files.map((file) => ( -
onRegisterRef?.(file.newPath, el)}> - onToggleViewed?.(file.newPath)} - /> -
- ))} + {files.map((file) => { + const isVisible = isSingleFile || visibleFiles.current.has(file.newPath); + const isExpandedOverride = expandedFiles.has(file.newPath) ? true : undefined; + return ( +
registerWrapper(file.newPath, el)} + data-file-path={file.newPath} + > + {isVisible ? ( + onToggleViewed?.(file.newPath)} + /> + ) : ( +
+ )} +
+ ); + })}
); } diff --git a/apps/web/src/components/review/FileCard.test.tsx b/apps/web/src/components/review/FileCard.test.tsx new file mode 100644 index 0000000..56f73d5 --- /dev/null +++ b/apps/web/src/components/review/FileCard.test.tsx @@ -0,0 +1,270 @@ +// @vitest-environment happy-dom +import "@testing-library/jest-dom/vitest"; +import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { vi, describe, it, expect, beforeEach } from "vitest"; +import { FileCard } from "./FileCard"; +import type { FileDiff, FileDiffDetail } from "./types"; + +// ── Module mocks ────────────────────────────────────────────────────────────── + +vi.mock("./HunkRows", () => ({ + HunkRows: ({ hunk }: { hunk: { header: string } }) => ( + + {hunk.header} + + ), +})); + +vi.mock("./use-syntax-highlight", () => ({ + useHighlightedFile: () => null, +})); + +// Hoist mocks so they can be referenced in vi.mock factories +const { mockGetFileDiff, mockParseUnifiedDiff } = vi.hoisted(() => ({ + mockGetFileDiff: vi.fn(), + mockParseUnifiedDiff: vi.fn(), +})); + +vi.mock("@/lib/trpc", () => ({ + trpc: { + getFileDiff: { + useQuery: ( + input: unknown, + opts: { enabled: boolean; staleTime?: number }, + ) => mockGetFileDiff(input, opts), + }, + }, +})); + +vi.mock("./parse-diff", () => ({ + parseUnifiedDiff: (rawDiff: string) => mockParseUnifiedDiff(rawDiff), +})); + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +function makeFile(overrides: Partial = {}): FileDiff { + return { + oldPath: "src/foo.ts", + newPath: "src/foo.ts", + status: "modified", + additions: 10, + deletions: 5, + ...overrides, + }; +} + +const defaultProps = { + phaseId: "phase-1", + commitMode: false, + commentsByLine: new Map(), + onAddComment: vi.fn(), + onResolveComment: vi.fn(), + onUnresolveComment: vi.fn(), +}; + +beforeEach(() => { + mockGetFileDiff.mockReturnValue({ + data: undefined, + isLoading: false, + isError: false, + refetch: vi.fn(), + }); + // Default: return empty parse result + mockParseUnifiedDiff.mockReturnValue([]); +}); + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe("FileCard", () => { + it("starts collapsed and does not enable getFileDiff query", () => { + render(); + + // Query must be called with enabled: false while card is collapsed + expect(mockGetFileDiff).toHaveBeenCalledWith( + expect.objectContaining({ + filePath: encodeURIComponent("src/foo.ts"), + }), + expect.objectContaining({ enabled: false }), + ); + + // No hunk rows rendered in the collapsed state + expect(screen.queryByTestId("hunk-row")).toBeNull(); + }); + + it("enables query and shows loading spinner when expanded", () => { + mockGetFileDiff.mockReturnValue({ + data: undefined, + isLoading: true, + isError: false, + refetch: vi.fn(), + }); + + render(); + fireEvent.click(screen.getByRole("button")); + + // After expanding, query should be called with enabled: true + expect(mockGetFileDiff).toHaveBeenLastCalledWith( + expect.anything(), + expect.objectContaining({ enabled: true }), + ); + + // Loading spinner should be visible + expect(screen.getByText(/Loading diff/i)).toBeInTheDocument(); + }); + + it("renders HunkRows when query succeeds", async () => { + mockGetFileDiff.mockReturnValue({ + data: { + binary: false, + rawDiff: + "diff --git a/src/foo.ts b/src/foo.ts\n@@ -1,3 +1,3 @@\n context\n", + }, + isLoading: false, + isError: false, + refetch: vi.fn(), + }); + + mockParseUnifiedDiff.mockReturnValue([ + { + oldPath: "src/foo.ts", + newPath: "src/foo.ts", + status: "modified", + additions: 0, + deletions: 0, + hunks: [ + { + header: "@@ -1,3 +1,3 @@", + oldStart: 1, + oldCount: 3, + newStart: 1, + newCount: 3, + lines: [], + }, + ], + }, + ]); + + render(); + fireEvent.click(screen.getByRole("button")); + + await waitFor(() => { + expect(screen.getByTestId("hunk-row")).toBeInTheDocument(); + }); + }); + + it("shows error state with Retry button; clicking retry calls refetch", () => { + const refetch = vi.fn(); + mockGetFileDiff.mockReturnValue({ + data: undefined, + isLoading: false, + isError: true, + refetch, + }); + + render(); + fireEvent.click(screen.getByRole("button")); + + expect(screen.getByText(/Failed to load diff/i)).toBeInTheDocument(); + const retryBtn = screen.getByRole("button", { name: /retry/i }); + fireEvent.click(retryBtn); + expect(refetch).toHaveBeenCalledOnce(); + }); + + it("shows binary message on expand and does not enable getFileDiff query", () => { + render(); + fireEvent.click(screen.getByRole("button")); + + expect(screen.getByText(/Binary file/i)).toBeInTheDocument(); + + // Query must never be enabled for binary files + expect(mockGetFileDiff).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ enabled: false }), + ); + }); + + it("shows No content changes when parsed hunks array is empty", async () => { + mockGetFileDiff.mockReturnValue({ + data: { + binary: false, + rawDiff: "diff --git a/src/foo.ts b/src/foo.ts\nsome content\n", + }, + isLoading: false, + isError: false, + refetch: vi.fn(), + }); + + mockParseUnifiedDiff.mockReturnValue([ + { + oldPath: "src/foo.ts", + newPath: "src/foo.ts", + status: "modified", + additions: 0, + deletions: 0, + hunks: [], + }, + ]); + + render(); + fireEvent.click(screen.getByRole("button")); + + await waitFor(() => { + expect(screen.getByText(/No content changes/i)).toBeInTheDocument(); + }); + }); + + it("renders pre-parsed hunks from detail prop without fetching", () => { + const detail: FileDiffDetail = { + oldPath: "src/foo.ts", + newPath: "src/foo.ts", + status: "modified", + additions: 5, + deletions: 2, + hunks: [ + { + header: "@@ -1 +1 @@", + oldStart: 1, + oldCount: 1, + newStart: 1, + newCount: 1, + lines: [], + }, + ], + }; + + render(); + + // Should start expanded because detail prop is provided + expect(screen.getByTestId("hunk-row")).toBeInTheDocument(); + + // Query must not be enabled when detail prop is present + expect(mockGetFileDiff).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ enabled: false }), + ); + }); + + it("does not refetch when collapsing and re-expanding", () => { + // Simulate data already available (as if previously fetched and cached) + mockGetFileDiff.mockReturnValue({ + data: { binary: false, rawDiff: "" }, + isLoading: false, + isError: false, + refetch: vi.fn(), + }); + + render(); + const headerBtn = screen.getByRole("button"); + + // Expand: query enabled, data shown immediately (no loading) + fireEvent.click(headerBtn); + expect(screen.queryByText(/Loading diff/i)).toBeNull(); + + // Collapse + fireEvent.click(headerBtn); + + // Re-expand: should not enter loading state (data still available) + fireEvent.click(headerBtn); + expect(screen.queryByText(/Loading diff/i)).toBeNull(); + }); +}); diff --git a/apps/web/src/components/review/FileCard.tsx b/apps/web/src/components/review/FileCard.tsx index 4c84769..12cfb14 100644 --- a/apps/web/src/components/review/FileCard.tsx +++ b/apps/web/src/components/review/FileCard.tsx @@ -6,16 +6,16 @@ import { Minus, CheckCircle2, Circle, + Loader2, } from "lucide-react"; import { Badge } from "@/components/ui/badge"; -import type { FileDiffDetail, DiffLine, ReviewComment } from "./types"; +import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { HunkRows } from "./HunkRows"; import { useHighlightedFile } from "./use-syntax-highlight"; +import { parseUnifiedDiff } from "./parse-diff"; +import { trpc } from "@/lib/trpc"; -const changeTypeBadge: Record< - FileDiffDetail['status'], - { label: string; classes: string } | null -> = { +const statusBadge: Record = { added: { label: "NEW", classes: @@ -32,10 +32,13 @@ const changeTypeBadge: Record< "bg-status-active-bg text-status-active-fg border-status-active-border", }, modified: null, - binary: null, + binary: { + label: "BINARY", + classes: "bg-muted text-muted-foreground border-border", + }, }; -const leftBorderClass: Record = { +const leftBorderClass: Record = { added: "border-l-2 border-l-status-success-fg", deleted: "border-l-2 border-l-status-error-fg", renamed: "border-l-2 border-l-status-active-fg", @@ -44,8 +47,12 @@ const leftBorderClass: Record = { }; interface FileCardProps { - file: FileDiffDetail; + file: FileDiff; + detail?: FileDiffDetail; + phaseId: string; + commitMode: boolean; commentsByLine: Map; + isExpandedOverride?: boolean; onAddComment: ( filePath: string, lineNumber: number, @@ -62,7 +69,11 @@ interface FileCardProps { export function FileCard({ file, + detail, + phaseId, + commitMode, commentsByLine, + isExpandedOverride, onAddComment, onResolveComment, onUnresolveComment, @@ -71,35 +82,65 @@ export function FileCard({ isViewed = false, onToggleViewed = () => {}, }: FileCardProps) { - const [expanded, setExpanded] = useState(true); + // Uncontrolled expand for normal file clicks. + // Start expanded if detail prop is provided (commit mode). + const [isExpandedLocal, setIsExpandedLocal] = useState(() => !!detail); - const commentCount = useMemo( - () => - Array.from(commentsByLine.values()).reduce( - (sum, arr) => sum + arr.length, - 0, - ), - [commentsByLine], + // Merge with override from DiffViewer expandAll + const isExpanded = isExpandedOverride ?? isExpandedLocal; + + const fileDiffQuery = trpc.getFileDiff.useQuery( + { phaseId, filePath: encodeURIComponent(file.newPath) }, + { + enabled: isExpanded && !commitMode && file.status !== 'binary' && !detail, + staleTime: Infinity, + }, ); - const badge = changeTypeBadge[file.status]; + // Compute hunks from query data (phase mode) + const parsedHunks = useMemo(() => { + if (!fileDiffQuery.data?.rawDiff) return null; + const parsed = parseUnifiedDiff(fileDiffQuery.data.rawDiff); + return parsed[0] ?? null; + }, [fileDiffQuery.data]); + + // Collect all lines for syntax highlighting + const allLines = useMemo(() => { + if (detail) return detail.hunks.flatMap((h) => h.lines); + if (parsedHunks) return parsedHunks.hunks.flatMap((h) => h.lines); + return []; + }, [detail, parsedHunks]); - // Flatten all hunk lines for syntax highlighting - const allLines = useMemo( - () => file.hunks.flatMap((h) => h.lines), - [file.hunks], - ); const tokenMap = useHighlightedFile(file.newPath, allLines); + const commentCount = useMemo(() => { + let count = 0; + for (const [key, arr] of commentsByLine) { + if (key.startsWith(`${file.newPath}:`)) count += arr.length; + } + return count; + }, [commentsByLine, file.newPath]); + + const badge = statusBadge[file.status]; + + const handlers = { + onAddComment, + onResolveComment, + onUnresolveComment, + onReplyComment, + onEditComment, + tokenMap, + }; + return (
- {/* File header — sticky so it stays visible when scrolling */} + {/* File header */} {/* Diff content */} - {expanded && ( + {isExpanded && (
- - - {file.hunks.map((hunk, hi) => ( - - ))} - -
+ {detail ? ( + // Commit mode: pre-parsed hunks from detail prop + detail.hunks.length === 0 ? ( +
No content changes
+ ) : ( + + + {detail.hunks.map((hunk, hi) => ( + + ))} + +
+ ) + ) : file.status === 'binary' ? ( +
Binary file — diff not shown
+ ) : fileDiffQuery.isLoading ? ( +
+ + Loading diff… +
+ ) : fileDiffQuery.isError ? ( +
+ Failed to load diff. + +
+ ) : fileDiffQuery.data ? ( + !parsedHunks || parsedHunks.hunks.length === 0 ? ( +
No content changes
+ ) : ( + + + {parsedHunks.hunks.map((hunk, hi) => ( + + ))} + +
+ ) + ) : null}
)}
diff --git a/apps/web/src/components/review/ReviewTab.tsx b/apps/web/src/components/review/ReviewTab.tsx index 8d0a2c0..df91c58 100644 --- a/apps/web/src/components/review/ReviewTab.tsx +++ b/apps/web/src/components/review/ReviewTab.tsx @@ -406,6 +406,8 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { ) : (