diff --git a/apps/web/src/components/review/DiffViewer.tsx b/apps/web/src/components/review/DiffViewer.tsx index d4cfd1e..872fb5a 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 { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types"; import { FileCard } from "./FileCard"; function getFileCommentMap( @@ -13,7 +13,10 @@ function getFileCommentMap( } interface DiffViewerProps { - files: FileDiff[]; + files: FileDiff[] | FileDiffDetail[]; + phaseId?: string; + commitMode?: boolean; + comments?: ReviewComment[]; commentsByLine: Map; onAddComment: ( filePath: string, @@ -28,6 +31,7 @@ interface DiffViewerProps { viewedFiles?: Set; onToggleViewed?: (filePath: string) => void; onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void; + expandAll?: boolean; } export function DiffViewer({ @@ -44,7 +48,7 @@ export function DiffViewer({ }: DiffViewerProps) { return (
- {files.map((file) => ( + {(files as FileDiffDetail[]).map((file) => (
onRegisterRef?.(file.newPath, el)}> = { added: { @@ -32,17 +34,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-muted-foreground/40", }; interface FileCardProps { - file: FileDiff; + file: FileDiffDetail; commentsByLine: Map; onAddComment: ( filePath: string, @@ -80,7 +84,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 +97,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) |