From ec86b62d8dc4edac71763f51cc71a0be9651cdc6 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:31:57 +0100 Subject: [PATCH 01/16] perf: Pre-index review comments into Map to eliminate O(n) filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build a Map once in ReviewTab and thread it down through DiffViewer → FileCard → HunkRows, replacing O(n) filter calls with O(1) map lookups. With 200 comments and 5000 diff lines, this reduces ~1M iterations per render cycle to ~5K. Key: "${filePath}:${lineNumber}:${lineType}" for line-level comments, "${filePath}:file" for file-level (lineNumber === null) comments. Co-Authored-By: Claude Sonnet 4.6 --- apps/web/src/components/review/DiffViewer.tsx | 17 ++- apps/web/src/components/review/FileCard.tsx | 17 ++- apps/web/src/components/review/HunkRows.tsx | 10 +- apps/web/src/components/review/ReviewTab.tsx | 8 +- .../components/review/comment-index.test.tsx | 134 ++++++++++++++++++ .../src/components/review/comment-index.ts | 25 ++++ apps/web/src/components/review/types.ts | 2 +- 7 files changed, 199 insertions(+), 14 deletions(-) create mode 100644 apps/web/src/components/review/comment-index.test.tsx create mode 100644 apps/web/src/components/review/comment-index.ts diff --git a/apps/web/src/components/review/DiffViewer.tsx b/apps/web/src/components/review/DiffViewer.tsx index 5b9c1e2..d4cfd1e 100644 --- a/apps/web/src/components/review/DiffViewer.tsx +++ b/apps/web/src/components/review/DiffViewer.tsx @@ -1,9 +1,20 @@ import type { FileDiff, DiffLine, ReviewComment } from "./types"; import { FileCard } from "./FileCard"; +function getFileCommentMap( + commentsByLine: Map, + filePath: string, +): Map { + const result = new Map(); + for (const [key, val] of commentsByLine) { + if (key.startsWith(`${filePath}:`)) result.set(key, val); + } + return result; +} + interface DiffViewerProps { files: FileDiff[]; - comments: ReviewComment[]; + commentsByLine: Map; onAddComment: ( filePath: string, lineNumber: number, @@ -21,7 +32,7 @@ interface DiffViewerProps { export function DiffViewer({ files, - comments, + commentsByLine, onAddComment, onResolveComment, onUnresolveComment, @@ -37,7 +48,7 @@ export function DiffViewer({
onRegisterRef?.(file.newPath, el)}> c.filePath === file.newPath)} + commentsByLine={getFileCommentMap(commentsByLine, file.newPath)} onAddComment={onAddComment} onResolveComment={onResolveComment} onUnresolveComment={onUnresolveComment} diff --git a/apps/web/src/components/review/FileCard.tsx b/apps/web/src/components/review/FileCard.tsx index d7056c8..4d6ed6f 100644 --- a/apps/web/src/components/review/FileCard.tsx +++ b/apps/web/src/components/review/FileCard.tsx @@ -43,7 +43,7 @@ const leftBorderClass: Record = { interface FileCardProps { file: FileDiff; - comments: ReviewComment[]; + commentsByLine: Map; onAddComment: ( filePath: string, lineNumber: number, @@ -60,7 +60,7 @@ interface FileCardProps { export function FileCard({ file, - comments, + commentsByLine, onAddComment, onResolveComment, onUnresolveComment, @@ -70,7 +70,16 @@ export function FileCard({ onToggleViewed = () => {}, }: FileCardProps) { const [expanded, setExpanded] = useState(true); - const commentCount = comments.length; + + const commentCount = useMemo( + () => + Array.from(commentsByLine.values()).reduce( + (sum, arr) => sum + arr.length, + 0, + ), + [commentsByLine], + ); + const badge = changeTypeBadge[file.changeType]; // Flatten all hunk lines for syntax highlighting @@ -158,7 +167,7 @@ export function FileCard({ key={hi} hunk={hunk} filePath={file.newPath} - comments={comments} + commentsByLine={commentsByLine} onAddComment={onAddComment} onResolveComment={onResolveComment} onUnresolveComment={onUnresolveComment} diff --git a/apps/web/src/components/review/HunkRows.tsx b/apps/web/src/components/review/HunkRows.tsx index 86cf6dd..9bec866 100644 --- a/apps/web/src/components/review/HunkRows.tsx +++ b/apps/web/src/components/review/HunkRows.tsx @@ -6,7 +6,7 @@ import type { LineTokenMap } from "./use-syntax-highlight"; interface HunkRowsProps { hunk: { header: string; lines: DiffLine[] }; filePath: string; - comments: ReviewComment[]; + commentsByLine: Map; onAddComment: ( filePath: string, lineNumber: number, @@ -23,7 +23,7 @@ interface HunkRowsProps { export function HunkRows({ hunk, filePath, - comments, + commentsByLine, onAddComment, onResolveComment, onUnresolveComment, @@ -81,9 +81,9 @@ export function HunkRows({ {hunk.lines.map((line, li) => { const lineKey = line.newLineNumber ?? line.oldLineNumber ?? li; - const lineComments = comments.filter( - (c) => c.lineNumber === lineKey && c.lineType === line.type, - ); + // O(1) map lookup — replaces the previous O(n) filter + const lineComments = + commentsByLine.get(`${filePath}:${lineKey}:${line.type}`) ?? []; const isCommenting = commentingLine?.lineNumber === lineKey && commentingLine?.lineType === line.type; diff --git a/apps/web/src/components/review/ReviewTab.tsx b/apps/web/src/components/review/ReviewTab.tsx index 099f380..8d0a2c0 100644 --- a/apps/web/src/components/review/ReviewTab.tsx +++ b/apps/web/src/components/review/ReviewTab.tsx @@ -7,6 +7,7 @@ import { DiffViewer } from "./DiffViewer"; import { ReviewSidebar } from "./ReviewSidebar"; import { ReviewHeader } from "./ReviewHeader"; import { InitiativeReview } from "./InitiativeReview"; +import { buildCommentIndex } from "./comment-index"; import type { ReviewStatus, DiffLine } from "./types"; interface ReviewTabProps { @@ -169,6 +170,11 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { })); }, [commentsQuery.data]); + const commentsByLine = useMemo( + () => buildCommentIndex(comments), + [comments], + ); + const createCommentMutation = trpc.createReviewComment.useMutation({ onSuccess: () => { utils.listReviewComments.invalidate({ phaseId: activePhaseId! }); @@ -400,7 +406,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) { ) : ( ({ + CommentThread: () =>
, +})); +vi.mock("./CommentForm", () => ({ + CommentForm: vi.fn().mockReturnValue(
), +})); +vi.mock("./use-syntax-highlight", () => ({ + useHighlightedFile: () => null, +})); + +// ── Helpers ────────────────────────────────────────────────────────────────── + +function makeComment(overrides: Partial & { id: string }): ReviewComment { + return { + id: overrides.id, + filePath: overrides.filePath ?? "src/foo.ts", + lineNumber: overrides.lineNumber !== undefined ? overrides.lineNumber : 1, + lineType: overrides.lineType ?? "added", + body: overrides.body ?? "comment body", + author: overrides.author ?? "alice", + createdAt: overrides.createdAt ?? "2024-01-01T00:00:00Z", + resolved: overrides.resolved ?? false, + parentCommentId: overrides.parentCommentId ?? null, + }; +} + +// ── buildCommentIndex — pure function tests ─────────────────────────────────── + +describe("buildCommentIndex", () => { + it("happy path — basic indexing", () => { + const c1 = makeComment({ id: "1", filePath: "src/foo.ts", lineNumber: 10, lineType: "added" }); + const c2 = makeComment({ id: "2", filePath: "src/bar.ts", lineNumber: 5, lineType: "context" }); + const map = buildCommentIndex([c1, c2]); + expect(map.get("src/foo.ts:10:added")).toEqual([c1]); + expect(map.get("src/bar.ts:5:context")).toEqual([c2]); + expect(map.size).toBe(2); + }); + + it("same-line accumulation — two comments land in same array", () => { + const c1 = makeComment({ id: "a", filePath: "src/x.ts", lineNumber: 20, lineType: "added" }); + const c2 = makeComment({ id: "b", filePath: "src/x.ts", lineNumber: 20, lineType: "added" }); + const map = buildCommentIndex([c1, c2]); + expect(map.get("src/x.ts:20:added")).toEqual([c1, c2]); + expect(map.size).toBe(1); + }); + + it("cross-type isolation — same lineNumber but different lineType produces separate entries", () => { + const added = makeComment({ id: "a", filePath: "src/x.ts", lineNumber: 10, lineType: "added" }); + const removed = makeComment({ id: "r", filePath: "src/x.ts", lineNumber: 10, lineType: "removed" }); + const map = buildCommentIndex([added, removed]); + expect(map.get("src/x.ts:10:added")).toEqual([added]); + expect(map.get("src/x.ts:10:removed")).toEqual([removed]); + expect(map.size).toBe(2); + }); + + it("null lineNumber — file-level comment stored under filePath:file", () => { + const fileComment = makeComment({ id: "f", filePath: "src/z.ts", lineNumber: null, lineType: "context" }); + const map = buildCommentIndex([fileComment]); + expect(map.get("src/z.ts:file")).toEqual([fileComment]); + }); + + it("empty input — returns empty map", () => { + expect(buildCommentIndex([])).toEqual(new Map()); + }); +}); + +// ── LineWithComments — component tests ─────────────────────────────────────── + +import { LineWithComments } from "./LineWithComments"; +import type { DiffLine } from "./types"; + +const addedLine: DiffLine = { + type: "added", + content: "const x = 1;", + oldLineNumber: null, + newLineNumber: 5, +}; + +const noop = () => {}; + +describe("LineWithComments", () => { + it("renders comment button with title when lineComments is non-empty", () => { + const lineComments = [ + makeComment({ id: "c1", filePath: "src/foo.ts", lineNumber: 5, lineType: "added" }), + ]; + render( + + + + +
, + ); + expect(screen.getByTitle(/1 comment/)).toBeInTheDocument(); + }); + + it("does not render comment thread row when lineComments is empty", () => { + render( + + + + +
, + ); + expect(document.querySelector("[data-comment-id]")).toBeNull(); + }); +}); diff --git a/apps/web/src/components/review/comment-index.ts b/apps/web/src/components/review/comment-index.ts new file mode 100644 index 0000000..cc486ca --- /dev/null +++ b/apps/web/src/components/review/comment-index.ts @@ -0,0 +1,25 @@ +import type { ReviewComment } from "./types"; + +/** + * Build a Map keyed by `"${filePath}:${lineNumber}:${lineType}"` for line-level + * comments, or `"${filePath}:file"` for file-level comments (lineNumber === null). + * + * The compound key (filePath + lineNumber + lineType) is required because + * added and removed lines can share the same numeric position in a replacement + * hunk (e.g., old line 10 removed, new line 10 added). + */ +export function buildCommentIndex( + comments: ReviewComment[], +): Map { + const map = new Map(); + for (const comment of comments) { + const key = + comment.lineNumber != null + ? `${comment.filePath}:${comment.lineNumber}:${comment.lineType}` + : `${comment.filePath}:file`; + const existing = map.get(key); + if (existing) existing.push(comment); + else map.set(key, [comment]); + } + return map; +} diff --git a/apps/web/src/components/review/types.ts b/apps/web/src/components/review/types.ts index 2b99452..2f9a1ad 100644 --- a/apps/web/src/components/review/types.ts +++ b/apps/web/src/components/review/types.ts @@ -28,7 +28,7 @@ export interface FileDiff { export interface ReviewComment { id: string; filePath: string; - lineNumber: number; // new-side line number (or old-side for deletions) + lineNumber: number | null; // null = file-level comment lineType: "added" | "removed" | "context"; body: string; author: string; From 9894cdd06f066e00a96fbb9e1124a6fb7d867bc9 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:33:47 +0100 Subject: [PATCH 02/16] feat: add diffBranchesStat and diffFileSingle to BranchManager Adds FileStatEntry type and two new primitives to the BranchManager port and SimpleGitBranchManager adapter, enabling split diff procedures in the tRPC layer without returning raw multi-megabyte diffs. - FileStatEntry captures path, status, additions/deletions, oldPath (renames), and optional projectId for multi-project routing - diffBranchesStat uses --name-status + --numstat, detects binary files (shown as - / - in numstat), handles spaces in filenames - diffFileSingle returns raw unified diff for a single file path --- apps/server/execution/orchestrator.test.ts | 2 + apps/server/git/branch-manager.ts | 18 ++- .../git/simple-git-branch-manager.test.ts | 140 ++++++++++++++++++ apps/server/git/simple-git-branch-manager.ts | 107 ++++++++++++- apps/server/git/types.ts | 23 +++ docs/git-process-logging.md | 2 + 6 files changed, 290 insertions(+), 2 deletions(-) diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index 6cf293d..ebf6ea2 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -46,6 +46,8 @@ function createMocks() { ensureBranch: vi.fn(), mergeBranch: vi.fn().mockResolvedValue({ success: true, message: 'merged', previousRef: 'abc000' }), diffBranches: vi.fn().mockResolvedValue(''), + diffBranchesStat: vi.fn().mockResolvedValue([]), + diffFileSingle: vi.fn().mockResolvedValue(''), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index 9ba6d85..dc7d174 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -6,7 +6,7 @@ * a worktree to be checked out. */ -import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js'; +import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js'; export interface BranchManager { /** @@ -29,6 +29,22 @@ export interface BranchManager { */ diffBranches(repoPath: string, baseBranch: string, headBranch: string): Promise; + /** + * Get per-file metadata for changes between two branches. + * Uses three-dot diff (baseBranch...headBranch) — same divergence model as diffBranches. + * Binary files are included with status 'binary' and additions/deletions both 0. + * Does NOT return hunk content. + */ + diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise; + + /** + * Get the raw unified diff for a single file between two branches. + * Uses three-dot diff (baseBranch...headBranch). + * Returns empty string for binary files (caller must detect binary separately). + * filePath must be URL-decoded before being passed here. + */ + diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.test.ts b/apps/server/git/simple-git-branch-manager.test.ts index 18cb50a..f59c65b 100644 --- a/apps/server/git/simple-git-branch-manager.test.ts +++ b/apps/server/git/simple-git-branch-manager.test.ts @@ -65,6 +65,69 @@ async function createTestRepoWithRemote(): Promise<{ }; } +/** + * Create a repo pair for testing diff operations. + * Sets up bare + clone with a 'feature' branch that has known changes vs 'main'. + */ +async function createTestRepoForDiff(): Promise<{ + clonePath: string; + cleanup: () => Promise; +}> { + const tmpBase = await mkdtemp(path.join(tmpdir(), 'cw-diff-test-')); + const barePath = path.join(tmpBase, 'bare.git'); + const workPath = path.join(tmpBase, 'work'); + const clonePath = path.join(tmpBase, 'clone'); + + // Create bare repo + await simpleGit().init([barePath, '--bare']); + + // Set up main branch in work dir + await simpleGit().clone(barePath, workPath); + const workGit = simpleGit(workPath); + await workGit.addConfig('user.email', 'test@example.com'); + await workGit.addConfig('user.name', 'Test User'); + await writeFile(path.join(workPath, 'README.md'), '# README\n'); + await writeFile(path.join(workPath, 'to-delete.txt'), 'delete me\n'); + await workGit.add(['README.md', 'to-delete.txt']); + await workGit.commit('Initial commit'); + await workGit.push('origin', 'main'); + + // Clone and create feature branch with changes + await simpleGit().clone(barePath, clonePath); + const cloneGit = simpleGit(clonePath); + await cloneGit.addConfig('user.email', 'test@example.com'); + await cloneGit.addConfig('user.name', 'Test User'); + await cloneGit.checkoutLocalBranch('feature'); + + // Add new text file + await writeFile(path.join(clonePath, 'added.txt'), 'new content\n'); + await cloneGit.add('added.txt'); + + // Modify existing file + await writeFile(path.join(clonePath, 'README.md'), '# README\n\nModified content\n'); + await cloneGit.add('README.md'); + + // Delete a file + await cloneGit.rm(['to-delete.txt']); + + // Add binary file + await writeFile(path.join(clonePath, 'image.bin'), Buffer.alloc(16)); + await cloneGit.add('image.bin'); + + // Add file with space in name + await writeFile(path.join(clonePath, 'has space.txt'), 'content\n'); + await cloneGit.add('has space.txt'); + + await cloneGit.commit('Feature branch changes'); + + return { + clonePath, + cleanup: async () => { + await rm(tmpBase, { recursive: true, force: true }); + }, + }; +} + describe('SimpleGitBranchManager', () => { let clonePath: string; let cleanup: () => Promise; @@ -108,3 +171,80 @@ describe('SimpleGitBranchManager', () => { }); }); }); + +describe('SimpleGitBranchManager - diffBranchesStat and diffFileSingle', () => { + let clonePath: string; + let cleanup: () => Promise; + let branchManager: SimpleGitBranchManager; + + beforeEach(async () => { + const setup = await createTestRepoForDiff(); + clonePath = setup.clonePath; + cleanup = setup.cleanup; + branchManager = new SimpleGitBranchManager(); + }); + + afterEach(async () => { + await cleanup(); + }); + + describe('diffBranchesStat', () => { + it('returns correct entries for added, modified, and deleted text files', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const added = entries.find(e => e.path === 'added.txt'); + const modified = entries.find(e => e.path === 'README.md'); + const deleted = entries.find(e => e.path === 'to-delete.txt'); + + expect(added?.status).toBe('added'); + expect(added?.additions).toBeGreaterThan(0); + expect(modified?.status).toBe('modified'); + expect(deleted?.status).toBe('deleted'); + expect(deleted?.deletions).toBeGreaterThan(0); + }); + + it('marks binary files as status=binary with additions=0, deletions=0', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const binary = entries.find(e => e.path === 'image.bin'); + expect(binary?.status).toBe('binary'); + expect(binary?.additions).toBe(0); + expect(binary?.deletions).toBe(0); + }); + + it('returns empty array when there are no changes', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'main'); + expect(entries).toEqual([]); + }); + + it('handles files with spaces in their names', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const spaced = entries.find(e => e.path === 'has space.txt'); + expect(spaced).toBeDefined(); + expect(spaced?.status).toBe('added'); + }); + }); + + describe('diffFileSingle', () => { + it('returns unified diff containing addition hunks for an added file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'added.txt'); + expect(diff).toContain('+'); + expect(diff).toContain('added.txt'); + }); + + it('returns unified diff with removal hunks for a deleted file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'to-delete.txt'); + expect(diff).toContain('-'); + expect(diff).toContain('to-delete.txt'); + }); + + it('returns string for a binary file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'image.bin'); + // git diff returns empty or a "Binary files differ" line — no hunk content + expect(typeof diff).toBe('string'); + }); + + it('handles file paths with spaces', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'has space.txt'); + expect(diff).toContain('has space.txt'); + }); + }); +}); diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 47b690e..7e96848 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -11,11 +11,31 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { simpleGit } from 'simple-git'; import type { BranchManager } from './branch-manager.js'; -import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js'; +import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js'; import { createModuleLogger } from '../logger/index.js'; const log = createModuleLogger('branch-manager'); +/** + * Normalize a numstat path to the new path for rename entries. + * Handles patterns like: + * - "{old.txt => new.txt}" → "new.txt" + * - "dir/{old.txt => new.txt}" → "dir/new.txt" + * - "old_dir/file.txt => new_dir/file.txt" → "new_dir/file.txt" + */ +function normalizeNumstatPath(pathStr: string): string { + const braceMatch = pathStr.match(/^(.*)\{(.*) => (.*)\}(.*)$/); + if (braceMatch) { + const [, prefix, , newPart, suffix] = braceMatch; + return `${prefix}${newPart}${suffix}`; + } + const arrowMatch = pathStr.match(/^.* => (.+)$/); + if (arrowMatch) { + return arrowMatch[1]; + } + return pathStr; +} + export class SimpleGitBranchManager implements BranchManager { async ensureBranch(repoPath: string, branch: string, baseBranch: string): Promise { const git = simpleGit(repoPath); @@ -97,6 +117,91 @@ export class SimpleGitBranchManager implements BranchManager { return diff; } + async diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise { + const git = simpleGit(repoPath); + const range = `${baseBranch}...${headBranch}`; + + const [nameStatusRaw, numStatRaw] = await Promise.all([ + git.raw(['diff', '--name-status', range]), + git.raw(['diff', '--numstat', range]), + ]); + + if (!nameStatusRaw.trim()) return []; + + // Parse numstat: "\t\t" + // Binary files: "-\t-\t" + const numStatMap = new Map(); + for (const line of numStatRaw.split('\n')) { + if (!line.trim()) continue; + const tabIdx1 = line.indexOf('\t'); + const tabIdx2 = line.indexOf('\t', tabIdx1 + 1); + if (tabIdx1 === -1 || tabIdx2 === -1) continue; + const addStr = line.slice(0, tabIdx1); + const delStr = line.slice(tabIdx1 + 1, tabIdx2); + const pathStr = line.slice(tabIdx2 + 1); + const binary = addStr === '-' && delStr === '-'; + // Normalize rename paths like "{old => new}" or "dir/{old => new}/file" to new path + const newPath = normalizeNumstatPath(pathStr); + numStatMap.set(newPath, { + additions: binary ? 0 : parseInt(addStr, 10), + deletions: binary ? 0 : parseInt(delStr, 10), + binary, + }); + } + + // Parse name-status: "\t" or "\t\t" + const entries: FileStatEntry[] = []; + for (const line of nameStatusRaw.split('\n')) { + if (!line.trim()) continue; + const parts = line.split('\t'); + if (parts.length < 2) continue; + + const statusCode = parts[0]; + let status: FileStatEntry['status']; + let filePath: string; + let oldPath: string | undefined; + + if (statusCode.startsWith('R')) { + status = 'renamed'; + oldPath = parts[1]; + filePath = parts[2]; + } else if (statusCode === 'A') { + status = 'added'; + filePath = parts[1]; + } else if (statusCode === 'M') { + status = 'modified'; + filePath = parts[1]; + } else if (statusCode === 'D') { + status = 'deleted'; + filePath = parts[1]; + } else { + status = 'modified'; + filePath = parts[1]; + } + + const numStat = numStatMap.get(filePath); + if (numStat?.binary) { + status = 'binary'; + } + + const entry: FileStatEntry = { + path: filePath, + status, + additions: numStat?.additions ?? 0, + deletions: numStat?.deletions ?? 0, + }; + if (oldPath !== undefined) entry.oldPath = oldPath; + entries.push(entry); + } + + return entries; + } + + async diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise { + const git = simpleGit(repoPath); + return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); diff --git a/apps/server/git/types.ts b/apps/server/git/types.ts index 51a35b7..00cb829 100644 --- a/apps/server/git/types.ts +++ b/apps/server/git/types.ts @@ -100,6 +100,29 @@ export interface BranchCommit { deletions: number; } +// ============================================================================= +// File Stat Entry (per-file diff metadata) +// ============================================================================= + +/** + * Metadata for a single file changed between two branches. + * No hunk content — only path, status, and line-count statistics. + */ +export interface FileStatEntry { + /** New path (or old path for deletions) */ + path: string; + /** Only set for renames — the path before the rename */ + oldPath?: string; + /** Nature of the change */ + status: 'added' | 'modified' | 'deleted' | 'renamed' | 'binary'; + /** Lines added (0 for binary files) */ + additions: number; + /** Lines deleted (0 for binary files) */ + deletions: number; + /** Which project clone this file belongs to (populated by callers in multi-project scenarios) */ + projectId?: string; +} + // ============================================================================= // WorktreeManager Port Interface // ============================================================================= diff --git a/docs/git-process-logging.md b/docs/git-process-logging.md index 2e5d8c4..16968af 100644 --- a/docs/git-process-logging.md +++ b/docs/git-process-logging.md @@ -40,6 +40,8 @@ Worktrees stored in `.cw-worktrees/` subdirectory of the repo. Each agent gets a | `ensureBranch(repoPath, branch, baseBranch)` | Create branch from base if it doesn't exist (idempotent) | | `mergeBranch(repoPath, source, target)` | Merge via ephemeral worktree, returns conflict info | | `diffBranches(repoPath, base, head)` | Three-dot diff between branches | +| `diffBranchesStat(repoPath, base, head)` | Per-file metadata (path, status, additions, deletions) — no hunk content. Binary files included with `status: 'binary'` and counts of 0. Returns `FileStatEntry[]`. | +| `diffFileSingle(repoPath, base, head, filePath)` | Raw unified diff for a single file (three-dot diff). `filePath` must be URL-decoded. Returns empty string for binary files. | | `deleteBranch(repoPath, branch)` | Delete local branch (no-op if missing) | | `branchExists(repoPath, branch)` | Check local branches | | `remoteBranchExists(repoPath, branch)` | Check remote tracking branches (`origin/`) | From 05eb1607493bf3b3825ee67ac72154d401c1f0cb Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:33:47 +0100 Subject: [PATCH 03/16] feat: add diffBranchesStat and diffFileSingle to BranchManager Adds FileStatEntry type and two new primitives to the BranchManager port and SimpleGitBranchManager adapter, enabling split diff procedures in the tRPC layer without returning raw multi-megabyte diffs. - FileStatEntry captures path, status, additions/deletions, oldPath (renames), and optional projectId for multi-project routing - diffBranchesStat uses --name-status + --numstat, detects binary files (shown as - / - in numstat), handles spaces in filenames - diffFileSingle returns raw unified diff for a single file path --- apps/server/execution/orchestrator.test.ts | 2 + apps/server/git/branch-manager.ts | 18 ++- .../git/simple-git-branch-manager.test.ts | 140 ++++++++++++++++++ apps/server/git/simple-git-branch-manager.ts | 107 ++++++++++++- apps/server/git/types.ts | 23 +++ docs/git-process-logging.md | 2 + 6 files changed, 290 insertions(+), 2 deletions(-) diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index 6cf293d..ebf6ea2 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -46,6 +46,8 @@ function createMocks() { ensureBranch: vi.fn(), mergeBranch: vi.fn().mockResolvedValue({ success: true, message: 'merged', previousRef: 'abc000' }), diffBranches: vi.fn().mockResolvedValue(''), + diffBranchesStat: vi.fn().mockResolvedValue([]), + diffFileSingle: vi.fn().mockResolvedValue(''), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index 9ba6d85..dc7d174 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -6,7 +6,7 @@ * a worktree to be checked out. */ -import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js'; +import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js'; export interface BranchManager { /** @@ -29,6 +29,22 @@ export interface BranchManager { */ diffBranches(repoPath: string, baseBranch: string, headBranch: string): Promise; + /** + * Get per-file metadata for changes between two branches. + * Uses three-dot diff (baseBranch...headBranch) — same divergence model as diffBranches. + * Binary files are included with status 'binary' and additions/deletions both 0. + * Does NOT return hunk content. + */ + diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise; + + /** + * Get the raw unified diff for a single file between two branches. + * Uses three-dot diff (baseBranch...headBranch). + * Returns empty string for binary files (caller must detect binary separately). + * filePath must be URL-decoded before being passed here. + */ + diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.test.ts b/apps/server/git/simple-git-branch-manager.test.ts index 18cb50a..f59c65b 100644 --- a/apps/server/git/simple-git-branch-manager.test.ts +++ b/apps/server/git/simple-git-branch-manager.test.ts @@ -65,6 +65,69 @@ async function createTestRepoWithRemote(): Promise<{ }; } +/** + * Create a repo pair for testing diff operations. + * Sets up bare + clone with a 'feature' branch that has known changes vs 'main'. + */ +async function createTestRepoForDiff(): Promise<{ + clonePath: string; + cleanup: () => Promise; +}> { + const tmpBase = await mkdtemp(path.join(tmpdir(), 'cw-diff-test-')); + const barePath = path.join(tmpBase, 'bare.git'); + const workPath = path.join(tmpBase, 'work'); + const clonePath = path.join(tmpBase, 'clone'); + + // Create bare repo + await simpleGit().init([barePath, '--bare']); + + // Set up main branch in work dir + await simpleGit().clone(barePath, workPath); + const workGit = simpleGit(workPath); + await workGit.addConfig('user.email', 'test@example.com'); + await workGit.addConfig('user.name', 'Test User'); + await writeFile(path.join(workPath, 'README.md'), '# README\n'); + await writeFile(path.join(workPath, 'to-delete.txt'), 'delete me\n'); + await workGit.add(['README.md', 'to-delete.txt']); + await workGit.commit('Initial commit'); + await workGit.push('origin', 'main'); + + // Clone and create feature branch with changes + await simpleGit().clone(barePath, clonePath); + const cloneGit = simpleGit(clonePath); + await cloneGit.addConfig('user.email', 'test@example.com'); + await cloneGit.addConfig('user.name', 'Test User'); + await cloneGit.checkoutLocalBranch('feature'); + + // Add new text file + await writeFile(path.join(clonePath, 'added.txt'), 'new content\n'); + await cloneGit.add('added.txt'); + + // Modify existing file + await writeFile(path.join(clonePath, 'README.md'), '# README\n\nModified content\n'); + await cloneGit.add('README.md'); + + // Delete a file + await cloneGit.rm(['to-delete.txt']); + + // Add binary file + await writeFile(path.join(clonePath, 'image.bin'), Buffer.alloc(16)); + await cloneGit.add('image.bin'); + + // Add file with space in name + await writeFile(path.join(clonePath, 'has space.txt'), 'content\n'); + await cloneGit.add('has space.txt'); + + await cloneGit.commit('Feature branch changes'); + + return { + clonePath, + cleanup: async () => { + await rm(tmpBase, { recursive: true, force: true }); + }, + }; +} + describe('SimpleGitBranchManager', () => { let clonePath: string; let cleanup: () => Promise; @@ -108,3 +171,80 @@ describe('SimpleGitBranchManager', () => { }); }); }); + +describe('SimpleGitBranchManager - diffBranchesStat and diffFileSingle', () => { + let clonePath: string; + let cleanup: () => Promise; + let branchManager: SimpleGitBranchManager; + + beforeEach(async () => { + const setup = await createTestRepoForDiff(); + clonePath = setup.clonePath; + cleanup = setup.cleanup; + branchManager = new SimpleGitBranchManager(); + }); + + afterEach(async () => { + await cleanup(); + }); + + describe('diffBranchesStat', () => { + it('returns correct entries for added, modified, and deleted text files', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const added = entries.find(e => e.path === 'added.txt'); + const modified = entries.find(e => e.path === 'README.md'); + const deleted = entries.find(e => e.path === 'to-delete.txt'); + + expect(added?.status).toBe('added'); + expect(added?.additions).toBeGreaterThan(0); + expect(modified?.status).toBe('modified'); + expect(deleted?.status).toBe('deleted'); + expect(deleted?.deletions).toBeGreaterThan(0); + }); + + it('marks binary files as status=binary with additions=0, deletions=0', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const binary = entries.find(e => e.path === 'image.bin'); + expect(binary?.status).toBe('binary'); + expect(binary?.additions).toBe(0); + expect(binary?.deletions).toBe(0); + }); + + it('returns empty array when there are no changes', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'main'); + expect(entries).toEqual([]); + }); + + it('handles files with spaces in their names', async () => { + const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature'); + const spaced = entries.find(e => e.path === 'has space.txt'); + expect(spaced).toBeDefined(); + expect(spaced?.status).toBe('added'); + }); + }); + + describe('diffFileSingle', () => { + it('returns unified diff containing addition hunks for an added file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'added.txt'); + expect(diff).toContain('+'); + expect(diff).toContain('added.txt'); + }); + + it('returns unified diff with removal hunks for a deleted file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'to-delete.txt'); + expect(diff).toContain('-'); + expect(diff).toContain('to-delete.txt'); + }); + + it('returns string for a binary file', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'image.bin'); + // git diff returns empty or a "Binary files differ" line — no hunk content + expect(typeof diff).toBe('string'); + }); + + it('handles file paths with spaces', async () => { + const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'has space.txt'); + expect(diff).toContain('has space.txt'); + }); + }); +}); diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 47b690e..7e96848 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -11,11 +11,31 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { simpleGit } from 'simple-git'; import type { BranchManager } from './branch-manager.js'; -import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js'; +import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js'; import { createModuleLogger } from '../logger/index.js'; const log = createModuleLogger('branch-manager'); +/** + * Normalize a numstat path to the new path for rename entries. + * Handles patterns like: + * - "{old.txt => new.txt}" → "new.txt" + * - "dir/{old.txt => new.txt}" → "dir/new.txt" + * - "old_dir/file.txt => new_dir/file.txt" → "new_dir/file.txt" + */ +function normalizeNumstatPath(pathStr: string): string { + const braceMatch = pathStr.match(/^(.*)\{(.*) => (.*)\}(.*)$/); + if (braceMatch) { + const [, prefix, , newPart, suffix] = braceMatch; + return `${prefix}${newPart}${suffix}`; + } + const arrowMatch = pathStr.match(/^.* => (.+)$/); + if (arrowMatch) { + return arrowMatch[1]; + } + return pathStr; +} + export class SimpleGitBranchManager implements BranchManager { async ensureBranch(repoPath: string, branch: string, baseBranch: string): Promise { const git = simpleGit(repoPath); @@ -97,6 +117,91 @@ export class SimpleGitBranchManager implements BranchManager { return diff; } + async diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise { + const git = simpleGit(repoPath); + const range = `${baseBranch}...${headBranch}`; + + const [nameStatusRaw, numStatRaw] = await Promise.all([ + git.raw(['diff', '--name-status', range]), + git.raw(['diff', '--numstat', range]), + ]); + + if (!nameStatusRaw.trim()) return []; + + // Parse numstat: "\t\t" + // Binary files: "-\t-\t" + const numStatMap = new Map(); + for (const line of numStatRaw.split('\n')) { + if (!line.trim()) continue; + const tabIdx1 = line.indexOf('\t'); + const tabIdx2 = line.indexOf('\t', tabIdx1 + 1); + if (tabIdx1 === -1 || tabIdx2 === -1) continue; + const addStr = line.slice(0, tabIdx1); + const delStr = line.slice(tabIdx1 + 1, tabIdx2); + const pathStr = line.slice(tabIdx2 + 1); + const binary = addStr === '-' && delStr === '-'; + // Normalize rename paths like "{old => new}" or "dir/{old => new}/file" to new path + const newPath = normalizeNumstatPath(pathStr); + numStatMap.set(newPath, { + additions: binary ? 0 : parseInt(addStr, 10), + deletions: binary ? 0 : parseInt(delStr, 10), + binary, + }); + } + + // Parse name-status: "\t" or "\t\t" + const entries: FileStatEntry[] = []; + for (const line of nameStatusRaw.split('\n')) { + if (!line.trim()) continue; + const parts = line.split('\t'); + if (parts.length < 2) continue; + + const statusCode = parts[0]; + let status: FileStatEntry['status']; + let filePath: string; + let oldPath: string | undefined; + + if (statusCode.startsWith('R')) { + status = 'renamed'; + oldPath = parts[1]; + filePath = parts[2]; + } else if (statusCode === 'A') { + status = 'added'; + filePath = parts[1]; + } else if (statusCode === 'M') { + status = 'modified'; + filePath = parts[1]; + } else if (statusCode === 'D') { + status = 'deleted'; + filePath = parts[1]; + } else { + status = 'modified'; + filePath = parts[1]; + } + + const numStat = numStatMap.get(filePath); + if (numStat?.binary) { + status = 'binary'; + } + + const entry: FileStatEntry = { + path: filePath, + status, + additions: numStat?.additions ?? 0, + deletions: numStat?.deletions ?? 0, + }; + if (oldPath !== undefined) entry.oldPath = oldPath; + entries.push(entry); + } + + return entries; + } + + async diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise { + const git = simpleGit(repoPath); + return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); diff --git a/apps/server/git/types.ts b/apps/server/git/types.ts index 51a35b7..00cb829 100644 --- a/apps/server/git/types.ts +++ b/apps/server/git/types.ts @@ -100,6 +100,29 @@ export interface BranchCommit { deletions: number; } +// ============================================================================= +// File Stat Entry (per-file diff metadata) +// ============================================================================= + +/** + * Metadata for a single file changed between two branches. + * No hunk content — only path, status, and line-count statistics. + */ +export interface FileStatEntry { + /** New path (or old path for deletions) */ + path: string; + /** Only set for renames — the path before the rename */ + oldPath?: string; + /** Nature of the change */ + status: 'added' | 'modified' | 'deleted' | 'renamed' | 'binary'; + /** Lines added (0 for binary files) */ + additions: number; + /** Lines deleted (0 for binary files) */ + deletions: number; + /** Which project clone this file belongs to (populated by callers in multi-project scenarios) */ + projectId?: string; +} + // ============================================================================= // WorktreeManager Port Interface // ============================================================================= diff --git a/docs/git-process-logging.md b/docs/git-process-logging.md index 2e5d8c4..16968af 100644 --- a/docs/git-process-logging.md +++ b/docs/git-process-logging.md @@ -40,6 +40,8 @@ Worktrees stored in `.cw-worktrees/` subdirectory of the repo. Each agent gets a | `ensureBranch(repoPath, branch, baseBranch)` | Create branch from base if it doesn't exist (idempotent) | | `mergeBranch(repoPath, source, target)` | Merge via ephemeral worktree, returns conflict info | | `diffBranches(repoPath, base, head)` | Three-dot diff between branches | +| `diffBranchesStat(repoPath, base, head)` | Per-file metadata (path, status, additions, deletions) — no hunk content. Binary files included with `status: 'binary'` and counts of 0. Returns `FileStatEntry[]`. | +| `diffFileSingle(repoPath, base, head, filePath)` | Raw unified diff for a single file (three-dot diff). `filePath` must be URL-decoded. Returns empty string for binary files. | | `deleteBranch(repoPath, branch)` | Delete local branch (no-op if missing) | | `branchExists(repoPath, branch)` | Check local branches | | `remoteBranchExists(repoPath, branch)` | Check remote tracking branches (`origin/`) | From 0608900a530dcb7edf8fde678e2bdba4ef8c9b34 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:39:31 +0100 Subject: [PATCH 04/16] feat: move syntax highlighting off main thread via Web Worker pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a 2-worker pool in use-syntax-highlight.ts so shiki tokenisation runs off the main thread. Callers continue to receive null while the worker is in flight and a LineTokenMap once it resolves — no caller changes needed. Fallback: if Worker construction is blocked (e.g. CSP), the hook falls back to the existing createHighlighter singleton but processes 200 lines at a time, yielding between chunks via scheduler.yield()/setTimeout(0) to avoid long tasks. Also adds worker.format:'es' to vite.config.ts (required when the app uses code-splitting) and covers all paths with Vitest tests. Co-Authored-By: Claude Sonnet 4.6 --- .../src/components/review/highlight-worker.ts | 39 +++ .../use-syntax-highlight.fallback.test.ts | 70 +++++ .../review/use-syntax-highlight.test.ts | 240 ++++++++++++++++++ .../components/review/use-syntax-highlight.ts | 177 ++++++++++--- apps/web/vite.config.ts | 5 + 5 files changed, 501 insertions(+), 30 deletions(-) create mode 100644 apps/web/src/components/review/highlight-worker.ts create mode 100644 apps/web/src/components/review/use-syntax-highlight.fallback.test.ts create mode 100644 apps/web/src/components/review/use-syntax-highlight.test.ts diff --git a/apps/web/src/components/review/highlight-worker.ts b/apps/web/src/components/review/highlight-worker.ts new file mode 100644 index 0000000..d663c16 --- /dev/null +++ b/apps/web/src/components/review/highlight-worker.ts @@ -0,0 +1,39 @@ +import type { ThemedToken } from 'shiki'; + +export interface HighlightRequest { + id: string; + filePath: string; + language: string; // resolved lang name (e.g. "typescript") or "text" + code: string; // full joined content of new-side lines to highlight + lineNumbers: number[]; // new-side line numbers to map tokens back to +} + +export interface HighlightResponse { + id: string; + tokens: Array<{ lineNumber: number; tokens: ThemedToken[] }>; + error?: string; +} + +self.addEventListener('message', async (event: MessageEvent) => { + const { id, language, code, lineNumbers } = event.data; + try { + const { codeToTokens } = await import('shiki'); + const result = await codeToTokens(code, { + lang: language as Parameters[1]['lang'], + theme: 'github-dark-default', + }); + const tokens: HighlightResponse['tokens'] = result.tokens.map((lineTokens, idx) => ({ + lineNumber: lineNumbers[idx] ?? idx, + tokens: lineTokens, + })); + const response: HighlightResponse = { id, tokens }; + self.postMessage(response); + } catch (err) { + const response: HighlightResponse = { + id, + tokens: [], + error: err instanceof Error ? err.message : String(err), + }; + self.postMessage(response); + } +}); diff --git a/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts b/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts new file mode 100644 index 0000000..1cd7bb7 --- /dev/null +++ b/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts @@ -0,0 +1,70 @@ +// @vitest-environment happy-dom +// This file tests the chunked main-thread fallback path when Worker +// construction is blocked (e.g. by CSP). It runs in isolation from the +// worker-path tests so that module-level state (workersInitialized, workers) +// starts clean. +import '@testing-library/jest-dom/vitest' +import { renderHook, waitFor } from '@testing-library/react' +import { vi, describe, it, expect, beforeAll, afterAll } from 'vitest' + +const MOCK_TOKEN_A = { content: 'const', color: '#569cd6', offset: 0 } +const MOCK_TOKEN_B = { content: 'x', color: '#9cdcfe', offset: 0 } + +// Mock shiki's createHighlighter for the fallback path +const mockCodeToTokens = vi.fn() + +vi.mock('shiki', () => ({ + createHighlighter: vi.fn().mockResolvedValue({ + codeToTokens: mockCodeToTokens, + }), +})) + +// Stub Worker to throw (simulating CSP) BEFORE the hook module is loaded. +// initWorkers() catches the exception and leaves workers = []. +beforeAll(() => { + // Use a class so Vitest doesn't warn about constructing vi.fn() without a class impl + class BlockedWorker { + constructor() { + throw new Error('CSP blocks workers') + } + } + vi.stubGlobal('Worker', BlockedWorker) + + mockCodeToTokens.mockReturnValue({ + tokens: [[MOCK_TOKEN_A], [MOCK_TOKEN_B]], + }) +}) + +afterAll(() => { + vi.unstubAllGlobals() +}) + +// Dynamic import ensures this file's module instance is fresh (workersInitialized = false). +// We import inside tests below rather than at the top level. + +describe('useHighlightedFile — fallback path (Worker unavailable)', () => { + it('falls back to chunked main-thread highlighting when Worker construction throws', async () => { + const { useHighlightedFile } = await import('./use-syntax-highlight') + + const lines = [ + { content: 'const x = 1', newLineNumber: 1, type: 'added' as const }, + { content: 'let y = 2', newLineNumber: 2, type: 'context' as const }, + ] + + const { result } = renderHook(() => useHighlightedFile('app.ts', lines)) + + // Initially null while chunked highlighting runs + expect(result.current).toBeNull() + + // Fallback createHighlighter path eventually resolves tokens + await waitFor( + () => { + expect(result.current).not.toBeNull() + }, + { timeout: 5000 }, + ) + + expect(result.current?.get(1)).toEqual([MOCK_TOKEN_A]) + expect(result.current?.get(2)).toEqual([MOCK_TOKEN_B]) + }) +}) diff --git a/apps/web/src/components/review/use-syntax-highlight.test.ts b/apps/web/src/components/review/use-syntax-highlight.test.ts new file mode 100644 index 0000000..1356c3b --- /dev/null +++ b/apps/web/src/components/review/use-syntax-highlight.test.ts @@ -0,0 +1,240 @@ +// @vitest-environment happy-dom +import '@testing-library/jest-dom/vitest' +import { renderHook, waitFor, act } from '@testing-library/react' +import { vi, describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest' + +// ── Worker mock infrastructure ───────────────────────────────────────────── +// +// We stub Worker BEFORE importing use-syntax-highlight so that initWorkers() +// (called from useEffect on first render) picks up our mock. +// Module-level state (workers, pending, workersInitialized) is shared across +// all tests in this file — we control behaviour through the mock instances. + +type WorkerHandler = (event: { data: unknown }) => void + +class MockWorker { + static instances: MockWorker[] = [] + + messageHandler: WorkerHandler | null = null + postMessage = vi.fn() + + constructor() { + MockWorker.instances.push(this) + } + + addEventListener(type: string, handler: WorkerHandler) { + if (type === 'message') this.messageHandler = handler + } + + /** Simulate a message arriving from the worker thread */ + simulateResponse(data: unknown) { + this.messageHandler?.({ data }) + } +} + +// Stub Worker before the hook module is loaded. +// initWorkers() is lazy (called inside useEffect), so the stub is in place +// by the time any test renders a hook. +beforeAll(() => { + vi.stubGlobal('Worker', MockWorker) +}) + +afterAll(() => { + vi.unstubAllGlobals() +}) + +beforeEach(() => { + // Reset call history between tests; keep instances (pool is created once) + MockWorker.instances.forEach((w) => w.postMessage.mockClear()) +}) + +// Import the hook AFTER the beforeAll stub is registered (hoisted evaluation +// of the module will not call initWorkers() — that happens in useEffect). +import { useHighlightedFile } from './use-syntax-highlight' + +// ── Helpers ──────────────────────────────────────────────────────────────── + +const MOCK_TOKEN_A = { content: 'const', color: '#569cd6', offset: 0 } +const MOCK_TOKEN_B = { content: 'x', color: '#9cdcfe', offset: 0 } + +function makeLine( + content: string, + newLineNumber: number, + type: 'added' | 'context' | 'removed' = 'added', +) { + return { content, newLineNumber, type } as const +} + +// ── Tests ────────────────────────────────────────────────────────────────── + +describe('useHighlightedFile — worker path', () => { + // ── Test 1: Correct message format ─────────────────────────────────────── + + it('posts a message to a worker with filePath, language, code, and lineNumbers', async () => { + const lines = [ + makeLine('const x = 1', 1, 'added'), + makeLine('const y = 2', 2, 'context'), + ] + + renderHook(() => useHighlightedFile('src/index.ts', lines)) + + // Wait for initWorkers() to fire and postMessage to be called + await waitFor(() => { + const totalCalls = MockWorker.instances.reduce( + (n, w) => n + w.postMessage.mock.calls.length, + 0, + ) + expect(totalCalls).toBeGreaterThan(0) + }) + + // Find which worker received the message + const calledWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0) + expect(calledWorker).toBeDefined() + expect(calledWorker!.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + filePath: 'src/index.ts', + language: 'typescript', + code: 'const x = 1\nconst y = 2', + lineNumbers: [1, 2], + }), + ) + }) + + // ── Test 2: Response builds token map ───────────────────────────────────── + + it('returns null initially and a LineTokenMap after worker responds', async () => { + const lines = [makeLine('const x = 1', 10, 'added')] + + const { result } = renderHook(() => useHighlightedFile('component.ts', lines)) + + // Immediately null while worker is pending + expect(result.current).toBeNull() + + // Capture the request id from whichever worker received it + let requestId = '' + let respondingWorker: MockWorker | undefined + + await waitFor(() => { + respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0) + expect(respondingWorker).toBeDefined() + requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string + expect(requestId).not.toBe('') + }) + + // Simulate the worker responding + act(() => { + respondingWorker!.simulateResponse({ + id: requestId, + tokens: [{ lineNumber: 10, tokens: [MOCK_TOKEN_A] }], + }) + }) + + await waitFor(() => { + expect(result.current).not.toBeNull() + expect(result.current?.get(10)).toEqual([MOCK_TOKEN_A]) + }) + }) + + // ── Test 3: Worker error response → null ────────────────────────────────── + + it('returns null when worker responds with an error field', async () => { + const lines = [makeLine('code here', 1, 'added')] + + const { result } = renderHook(() => useHighlightedFile('bad.ts', lines)) + + let requestId = '' + let respondingWorker: MockWorker | undefined + + await waitFor(() => { + respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0) + expect(respondingWorker).toBeDefined() + requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string + }) + + act(() => { + respondingWorker!.simulateResponse({ + id: requestId, + tokens: [], + error: 'Worker crashed', + }) + }) + + // Error → stays null (plain text fallback in the UI) + await new Promise((r) => setTimeout(r, 20)) + expect(result.current).toBeNull() + }) + + // ── Test 4: Unmount before response — no state update ──────────────────── + + it('silently discards a late worker response after unmount', async () => { + const lines = [makeLine('const z = 3', 5, 'added')] + + const { result, unmount } = renderHook(() => useHighlightedFile('late.ts', lines)) + + let requestId = '' + let respondingWorker: MockWorker | undefined + + await waitFor(() => { + respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0) + expect(respondingWorker).toBeDefined() + requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string + }) + + // Unmount before the response arrives + unmount() + + // Simulate the late response — should be silently dropped + act(() => { + respondingWorker!.simulateResponse({ + id: requestId, + tokens: [{ lineNumber: 5, tokens: [MOCK_TOKEN_B] }], + }) + }) + + // result.current is frozen at last rendered value (null) — no update fired + expect(result.current).toBeNull() + }) + + // ── Test 5: Round-robin — two simultaneous requests go to different workers + + it('distributes two simultaneous requests across both pool workers', async () => { + // Ensure the pool has been initialised (first test may have done this) + // and reset call counts for clean measurement. + MockWorker.instances.forEach((w) => w.postMessage.mockClear()) + + const lines1 = [makeLine('alpha', 1, 'added')] + const lines2 = [makeLine('beta', 1, 'added')] + + // Render two hook instances at the same time + renderHook(() => useHighlightedFile('file1.ts', lines1)) + renderHook(() => useHighlightedFile('file2.ts', lines2)) + + await waitFor(() => { + const total = MockWorker.instances.reduce((n, w) => n + w.postMessage.mock.calls.length, 0) + expect(total).toBe(2) + }) + + // Both pool workers should each have received exactly one request + // (round-robin: even requestCount → workers[0], odd → workers[1]) + const counts = MockWorker.instances.map((w) => w.postMessage.mock.calls.length) + // Pool has 2 workers; each should have received 1 of the 2 requests + expect(counts[0]).toBe(1) + expect(counts[1]).toBe(1) + }) + + // ── Test 6: Unknown language → no request ──────────────────────────────── + + it('returns null immediately for files with no detectable language', async () => { + MockWorker.instances.forEach((w) => w.postMessage.mockClear()) + + const lines = [makeLine('raw data', 1, 'added')] + + const { result } = renderHook(() => useHighlightedFile('data.xyz', lines)) + + await new Promise((r) => setTimeout(r, 50)) + + expect(result.current).toBeNull() + const total = MockWorker.instances.reduce((n, w) => n + w.postMessage.mock.calls.length, 0) + expect(total).toBe(0) + }) +}) diff --git a/apps/web/src/components/review/use-syntax-highlight.ts b/apps/web/src/components/review/use-syntax-highlight.ts index 673591d..dda0f63 100644 --- a/apps/web/src/components/review/use-syntax-highlight.ts +++ b/apps/web/src/components/review/use-syntax-highlight.ts @@ -1,7 +1,59 @@ import { useState, useEffect, useMemo } from "react"; import type { ThemedToken } from "shiki"; +import type { HighlightRequest, HighlightResponse } from "./highlight-worker"; -/* ── Lazy singleton highlighter ─────────────────────────── */ +/* ── Worker pool (module-level, shared across all hook instances) ─────── */ + +type PendingResolve = (response: HighlightResponse) => void; + +let workers: Worker[] = []; +let requestCount = 0; +const MAX_WORKERS = 2; +const pending = new Map(); + +let workersInitialized = false; + +function initWorkers(): void { + if (workersInitialized) return; + workersInitialized = true; + try { + workers = Array.from({ length: MAX_WORKERS }, () => { + const w = new Worker( + new URL("./highlight-worker.ts", import.meta.url), + { type: "module" }, + ); + w.addEventListener("message", (event: MessageEvent) => { + const resolve = pending.get(event.data.id); + if (resolve) { + pending.delete(event.data.id); + resolve(event.data); + } + }); + return w; + }); + } catch { + // CSP or browser compat — fall back to chunked main-thread highlighting + workers = []; + } +} + +function highlightWithWorker( + id: string, + language: string, + code: string, + lineNumbers: number[], + filePath: string, +): Promise { + return new Promise((resolve) => { + pending.set(id, resolve); + const worker = workers[requestCount % MAX_WORKERS]; + requestCount++; + const req: HighlightRequest = { id, filePath, language, code, lineNumbers }; + worker.postMessage(req); + }); +} + +/* ── Lazy singleton highlighter (for main-thread fallback) ───────────── */ let highlighterPromise: Promise @@ -40,10 +92,59 @@ function getHighlighter() { return highlighterPromise; } -// Pre-warm on module load (non-blocking) -getHighlighter(); +/* ── Chunked main-thread fallback ────────────────────────────────────── */ -/* ── Language detection ──────────────────────────────────── */ +async function highlightChunked( + code: string, + language: string, + lineNumbers: number[], + signal: AbortSignal, +): Promise { + const CHUNK = 200; + const result: LineTokenMap = new Map(); + const lines = code.split("\n"); + const highlighter = await getHighlighter(); + if (!highlighter) return result; + + for (let i = 0; i < lines.length; i += CHUNK) { + if (signal.aborted) break; + const chunkLines = lines.slice(i, i + CHUNK); + const chunkCode = chunkLines.join("\n"); + try { + const tokenized = highlighter.codeToTokens(chunkCode, { + lang: language as Parameters[1]["lang"], + theme: "github-dark-default", + }); + tokenized.tokens.forEach((lineTokens: ThemedToken[], idx: number) => { + const lineNum = lineNumbers[i + idx]; + if (lineNum !== undefined) result.set(lineNum, lineTokens); + }); + } catch { + // Skip unparseable chunk + } + + // Yield between chunks to avoid blocking the main thread + await new Promise((r) => { + if ( + "scheduler" in globalThis && + "yield" in (globalThis as Record).scheduler + ) { + ( + (globalThis as Record).scheduler as { + yield: () => Promise; + } + ) + .yield() + .then(r); + } else { + setTimeout(r, 0); + } + }); + } + return result; +} + +/* ── Language detection ──────────────────────────────────────────────── */ const EXT_TO_LANG: Record = { ts: "typescript", @@ -77,7 +178,7 @@ function detectLang(path: string): string | null { return EXT_TO_LANG[ext] ?? null; } -/* ── Types ───────────────────────────────────────────────── */ +/* ── Types ───────────────────────────────────────────────────────────── */ export type TokenizedLine = ThemedToken[]; /** Maps newLineNumber → highlighted tokens for that line */ @@ -89,12 +190,23 @@ interface DiffLineInput { type: "added" | "removed" | "context"; } -/* ── Hook ────────────────────────────────────────────────── */ +/* ── Hook ────────────────────────────────────────────────────────────── */ /** - * Highlights the "new-side" content of a file diff. - * Returns null until highlighting is ready (progressive enhancement). - * Only context + added lines are highlighted (removed lines fall back to plain text). + * Highlights the "new-side" content of a file diff, returning a map of + * line number → syntax tokens. + * + * Progressive rendering: returns `null` while highlighting is in progress. + * Callers (HunkRows → LineWithComments) render plain text when `null` and + * patch in highlighted tokens on re-render once the worker or chunked call + * resolves. + * + * Worker path: uses a module-level pool of 2 Web Workers. Round-robin + * assignment. Late responses after unmount are silently discarded. + * + * Fallback path: if Worker construction fails (CSP, browser compat), + * falls back to chunked main-thread highlighting via codeToTokens (200 + * lines/chunk) with scheduler.yield()/setTimeout(0) between chunks. */ export function useHighlightedFile( filePath: string, @@ -129,32 +241,37 @@ export function useHighlightedFile( return; } - let cancelled = false; + initWorkers(); // no-op after first call - getHighlighter().then((highlighter) => { - if (cancelled || !highlighter) return; + const id = crypto.randomUUID(); + let unmounted = false; + const abortController = new AbortController(); - try { - const result = highlighter.codeToTokens(code, { - lang: lang as Parameters[1]["lang"], - theme: "github-dark-default", - }); + if (workers.length > 0) { + highlightWithWorker(id, lang, code, lineNums, filePath).then((response) => { + if (unmounted) return; // ignore late responses after unmount + if (response.error || response.tokens.length === 0) { + setTokenMap(null); + return; + } const map: LineTokenMap = new Map(); - - result.tokens.forEach((lineTokens: ThemedToken[], idx: number) => { - if (idx < lineNums.length) { - map.set(lineNums[idx], lineTokens); - } - }); - - if (!cancelled) setTokenMap(map); - } catch { - // Language not loaded or parse error — no highlighting - } - }); + for (const { lineNumber, tokens } of response.tokens) { + map.set(lineNumber, tokens); + } + setTokenMap(map); + }); + } else { + highlightChunked(code, lang, lineNums, abortController.signal).then((map) => { + if (unmounted) return; + setTokenMap(map.size > 0 ? map : null); + }); + } return () => { - cancelled = true; + unmounted = true; + abortController.abort(); + // Remove pending resolver so a late worker response is silently dropped + pending.delete(id); }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [cacheKey]); diff --git a/apps/web/vite.config.ts b/apps/web/vite.config.ts index 6153961..a92669e 100644 --- a/apps/web/vite.config.ts +++ b/apps/web/vite.config.ts @@ -10,6 +10,11 @@ export default defineConfig({ "@": path.resolve(__dirname, "./src"), }, }, + worker: { + // ES module workers are required when the app uses code-splitting (Rollup + // can't bundle IIFE workers alongside dynamic imports). + format: "es", + }, server: { proxy: { "/trpc": { From c22a550bfcbbfa108fad18ca8982abda722d51d2 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:40:26 +0100 Subject: [PATCH 05/16] docs: document Web Worker syntax highlighting architecture in frontend.md Co-Authored-By: Claude Sonnet 4.6 --- docs/frontend.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/frontend.md b/docs/frontend.md index 0797687..ce5982d 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -122,6 +122,17 @@ The initiative detail page has three tabs managed via local state (not URL param | `PreviewPanel` | Docker preview status: building/running/failed with start/stop (legacy, now integrated into ReviewHeader) | | `ProposalCard` | Individual proposal display | +#### Syntax Highlighting (`use-syntax-highlight.ts` + `highlight-worker.ts`) + +`useHighlightedFile(filePath, allLines)` returns `LineTokenMap | null`. Tokenisation runs off the main thread: + +- **Worker path** (default): a module-level pool of 2 ES module Web Workers (`highlight-worker.ts`) each import shiki's `codeToTokens` dynamically. Requests are round-robined by `requestCount % 2`. Responses are correlated by UUID. Late responses after unmount are silently discarded via the `pending` Map. +- **Fallback path** (CSP / browser-compat): if `Worker` construction throws, `createHighlighter` is used on the main thread but processes 200 lines per chunk, yielding between chunks via `scheduler.yield()` or `setTimeout(0)`. + +Callers receive `null` while highlighting is in progress and a populated `Map` once it resolves. `LineWithComments` already renders plain text when `null`, so no caller changes are needed. + +Vite must be configured with `worker.format: 'es'` (added to `vite.config.ts`) for the worker chunk to bundle correctly alongside code-split app chunks. + ### UI Primitives (`src/components/ui/`) shadcn/ui components: badge (6 status variants + xs size), button, card, dialog, dropdown-menu, input, label, select, sonner, textarea, tooltip. From 4890721a92d5a87f0eb986cdacd8cf17be3e0829 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:45:57 +0100 Subject: [PATCH 06/16] feat: split getPhaseReviewDiff into metadata + add getFileDiff procedure Rewrites getPhaseReviewDiff to return file-level metadata (path, status, additions, deletions) instead of a raw diff string, eliminating 10MB+ payloads for large repos. Adds getFileDiff for on-demand per-file hunk content with binary detection via numstat. Multi-project initiatives prefix file paths with the project name to avoid collisions. Adds integration tests that use real local git repos + in-memory SQLite to verify both procedures end-to-end (binary files, deleted files, spaces in paths, error cases). Co-Authored-By: Claude Sonnet 4.6 --- .../integration/phase-review-diff.test.ts | 256 ++++++++++++++++++ apps/server/trpc/routers/phase.ts | 79 +++++- docs/server-api.md | 3 +- 3 files changed, 331 insertions(+), 7 deletions(-) create mode 100644 apps/server/test/integration/phase-review-diff.test.ts diff --git a/apps/server/test/integration/phase-review-diff.test.ts b/apps/server/test/integration/phase-review-diff.test.ts new file mode 100644 index 0000000..036e12d --- /dev/null +++ b/apps/server/test/integration/phase-review-diff.test.ts @@ -0,0 +1,256 @@ +/** + * Integration tests for getPhaseReviewDiff and getFileDiff tRPC procedures. + * + * Uses real git repos on disk (no cassettes) + an in-memory SQLite database. + * No network calls — purely local git operations. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtemp, rm, writeFile, mkdir } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { simpleGit } from 'simple-git'; +import { router, publicProcedure, createCallerFactory } from '../../trpc/trpc.js'; +import { phaseProcedures } from '../../trpc/routers/phase.js'; +import { createTestDatabase } from '../../db/repositories/drizzle/test-helpers.js'; +import { + DrizzleInitiativeRepository, + DrizzlePhaseRepository, + DrizzleProjectRepository, +} from '../../db/repositories/drizzle/index.js'; +import { SimpleGitBranchManager } from '../../git/simple-git-branch-manager.js'; +import { getProjectCloneDir } from '../../git/project-clones.js'; +import type { TRPCContext } from '../../trpc/context.js'; + +// ============================================================================ +// Test router & caller factory +// ============================================================================ + +const testRouter = router({ + ...phaseProcedures(publicProcedure), +}); + +const createCaller = createCallerFactory(testRouter); + +// ============================================================================ +// Shared test state (set up once for the whole suite) +// ============================================================================ + +let workspaceRoot: string; +let cleanup: () => Promise; +let phaseId: string; +let pendingPhaseId: string; + +/** + * Build the test git repo with the required branches and files. + * + * Repo layout on the phase branch vs main: + * file1.txt through file5.txt — added (10–20 lines each) + * photo.bin — binary file (Buffer.alloc(32)) + * gone.txt — deleted (existed on main) + * has space.txt — added (contains text) + */ +async function setupGitRepo(clonePath: string): Promise { + await mkdir(clonePath, { recursive: true }); + + const git = simpleGit(clonePath); + await git.init(); + await git.addConfig('user.email', 'test@example.com'); + await git.addConfig('user.name', 'Test'); + + // Commit gone.txt on main so it can be deleted on the phase branch + await writeFile(path.join(clonePath, 'gone.txt'), Array.from({ length: 5 }, (_, i) => `line ${i + 1}`).join('\n') + '\n'); + await git.add('gone.txt'); + await git.commit('Initial commit on main'); + + // Create phase branch from main + // phaseBranchName('main', 'test-phase') => 'main-phase-test-phase' + await git.checkoutLocalBranch('main-phase-test-phase'); + + // Add 5 text files + for (let i = 1; i <= 5; i++) { + const lines = Array.from({ length: 15 }, (_, j) => `Line ${j + 1} in file${i}`).join('\n') + '\n'; + await writeFile(path.join(clonePath, `file${i}.txt`), lines); + } + await git.add(['file1.txt', 'file2.txt', 'file3.txt', 'file4.txt', 'file5.txt']); + + // Add binary file + await writeFile(path.join(clonePath, 'photo.bin'), Buffer.alloc(32)); + await git.add('photo.bin'); + + // Delete gone.txt + await git.rm(['gone.txt']); + + // Add file with space in name + await writeFile(path.join(clonePath, 'has space.txt'), 'content with spaces\n'); + await git.add('has space.txt'); + + await git.commit('Phase branch changes'); +} + +beforeAll(async () => { + // Create workspace root temp dir + workspaceRoot = await mkdtemp(path.join(tmpdir(), 'cw-phase-diff-test-')); + cleanup = async () => { + await rm(workspaceRoot, { recursive: true, force: true }); + }; + + // Set up in-memory database + const db = createTestDatabase(); + const initiativeRepo = new DrizzleInitiativeRepository(db); + const phaseRepo = new DrizzlePhaseRepository(db); + const projectRepo = new DrizzleProjectRepository(db); + + // Create initiative with branch='main' + const initiative = await initiativeRepo.create({ + name: 'Test Initiative', + branch: 'main', + }); + + // Create project — we'll set up the git repo at the expected clone path + const project = await projectRepo.create({ + name: 'test-repo', + url: 'file:///dev/null', // won't be cloned — we create the repo directly + defaultBranch: 'main', + }); + + // Link project to initiative + await projectRepo.addProjectToInitiative(initiative.id, project.id); + + // Set up git repo at the expected clone path + const relPath = getProjectCloneDir(project.name, project.id); + const clonePath = path.join(workspaceRoot, relPath); + await setupGitRepo(clonePath); + + // Create reviewable phase (pending_review) + const phase = await phaseRepo.create({ + initiativeId: initiative.id, + name: 'test-phase', + status: 'pending_review', + }); + phaseId = phase.id; + + // Create a non-reviewable phase (pending) for error test + const pendingPhase = await phaseRepo.create({ + initiativeId: initiative.id, + name: 'pending-phase', + status: 'pending', + }); + pendingPhaseId = pendingPhase.id; + + // Store db and repos so the caller can use them + // (stored in module-level vars to be accessed in test helper) + Object.assign(sharedCtx, { + initiativeRepository: initiativeRepo, + phaseRepository: phaseRepo, + projectRepository: projectRepo, + branchManager: new SimpleGitBranchManager(), + workspaceRoot, + }); +}); + +afterAll(async () => { + await cleanup?.(); +}); + +// ============================================================================ +// Shared context (filled in beforeAll) +// ============================================================================ + +const sharedCtx: Partial = { + eventBus: { emit: () => {}, on: () => {}, off: () => {}, once: () => {} } as any, + serverStartedAt: null, + processCount: 0, +}; + +function getCaller() { + return createCaller(sharedCtx as TRPCContext); +} + +// ============================================================================ +// Tests: getPhaseReviewDiff +// ============================================================================ + +describe('getPhaseReviewDiff', () => { + it('returns files array with correct metadata and no rawDiff field', async () => { + const start = Date.now(); + const result = await getCaller().getPhaseReviewDiff({ phaseId }); + const elapsed = Date.now() - start; + + expect(result).not.toHaveProperty('rawDiff'); + expect(result.files).toBeInstanceOf(Array); + // 5 text + 1 binary + 1 deleted + 1 spaced = 8 files + expect(result.files.length).toBeGreaterThanOrEqual(7); + expect(elapsed).toBeLessThan(3000); + }); + + it('includes binary file with status=binary, additions=0, deletions=0', async () => { + const result = await getCaller().getPhaseReviewDiff({ phaseId }); + const bin = result.files.find((f) => f.path === 'photo.bin'); + expect(bin?.status).toBe('binary'); + expect(bin?.additions).toBe(0); + expect(bin?.deletions).toBe(0); + }); + + it('includes deleted file with status=deleted and nonzero deletions', async () => { + const result = await getCaller().getPhaseReviewDiff({ phaseId }); + const del = result.files.find((f) => f.path === 'gone.txt'); + expect(del?.status).toBe('deleted'); + expect(del?.deletions).toBeGreaterThan(0); + }); + + it('computes totalAdditions and totalDeletions as sums over files', async () => { + const result = await getCaller().getPhaseReviewDiff({ phaseId }); + const sumAdd = result.files.reduce((s, f) => s + f.additions, 0); + const sumDel = result.files.reduce((s, f) => s + f.deletions, 0); + expect(result.totalAdditions).toBe(sumAdd); + expect(result.totalDeletions).toBe(sumDel); + }); + + it('throws NOT_FOUND for unknown phaseId', async () => { + const err = await getCaller().getPhaseReviewDiff({ phaseId: 'nonexistent' }).catch((e) => e); + expect(err.code).toBe('NOT_FOUND'); + }); + + it('throws BAD_REQUEST for phase not in reviewable status', async () => { + const err = await getCaller().getPhaseReviewDiff({ phaseId: pendingPhaseId }).catch((e) => e); + expect(err.code).toBe('BAD_REQUEST'); + }); +}); + +// ============================================================================ +// Tests: getFileDiff +// ============================================================================ + +describe('getFileDiff', () => { + it('returns rawDiff with unified diff for a normal file, under 1 second', async () => { + const start = Date.now(); + const result = await getCaller().getFileDiff({ phaseId, filePath: 'file1.txt' }); + const elapsed = Date.now() - start; + + expect(result.binary).toBe(false); + expect(result.rawDiff).toContain('+'); + expect(elapsed).toBeLessThan(1000); + }); + + it('returns binary=true and rawDiff="" for binary file', async () => { + const result = await getCaller().getFileDiff({ phaseId, filePath: 'photo.bin' }); + expect(result.binary).toBe(true); + expect(result.rawDiff).toBe(''); + }); + + it('returns removal hunks for a deleted file', async () => { + const result = await getCaller().getFileDiff({ phaseId, filePath: 'gone.txt' }); + expect(result.binary).toBe(false); + expect(result.rawDiff).toContain('-'); + }); + + it('handles URL-encoded file path with space', async () => { + const result = await getCaller().getFileDiff({ + phaseId, + filePath: encodeURIComponent('has space.txt'), + }); + expect(result.binary).toBe(false); + expect(result.rawDiff).toContain('has space.txt'); + }); +}); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index be59ef2..0d9c999 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -4,11 +4,13 @@ import { TRPCError } from '@trpc/server'; import { z } from 'zod'; +import { simpleGit } from 'simple-git'; import type { Phase } from '../../db/schema.js'; import type { ProcedureBuilder } from '../trpc.js'; import { requirePhaseRepository, requireTaskRepository, requireBranchManager, requireInitiativeRepository, requireProjectRepository, requireExecutionOrchestrator, requireReviewCommentRepository, requireChangeSetRepository } from './_helpers.js'; import { phaseBranchName } from '../../git/branch-naming.js'; import { ensureProjectClone } from '../../git/project-clones.js'; +import type { FileStatEntry } from '../../git/types.js'; export function phaseProcedures(publicProcedure: ProcedureBuilder) { return { @@ -230,28 +232,93 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const initBranch = initiative.branch; const phBranch = phaseBranchName(initBranch, phase.name); - // For completed phases, use stored merge base; for pending_review, use initiative branch const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch; const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); - let rawDiff = ''; + const files: FileStatEntry[] = []; for (const project of projects) { const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); - const diff = await branchManager.diffBranches(clonePath, diffBase, phBranch); - if (diff) { - rawDiff += diff + '\n'; + const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch); + for (const entry of entries) { + const tagged: FileStatEntry = { ...entry, projectId: project.id }; + if (projects.length > 1) { + tagged.path = `${project.name}/${entry.path}`; + if (entry.oldPath) { + tagged.oldPath = `${project.name}/${entry.oldPath}`; + } + } + files.push(tagged); } } + const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0); + const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0); + return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, - rawDiff, + files, + totalAdditions, + totalDeletions, }; }), + getFileDiff: publicProcedure + .input(z.object({ + phaseId: z.string().min(1), + filePath: z.string().min(1), + projectId: z.string().optional(), + })) + .query(async ({ ctx, input }) => { + const phaseRepo = requirePhaseRepository(ctx); + const initiativeRepo = requireInitiativeRepository(ctx); + const projectRepo = requireProjectRepository(ctx); + const branchManager = requireBranchManager(ctx); + + const phase = await phaseRepo.findById(input.phaseId); + if (!phase) { + throw new TRPCError({ code: 'NOT_FOUND', message: `Phase '${input.phaseId}' not found` }); + } + if (phase.status !== 'pending_review' && phase.status !== 'completed') { + throw new TRPCError({ code: 'BAD_REQUEST', message: `Phase is not reviewable (status: ${phase.status})` }); + } + + const initiative = await initiativeRepo.findById(phase.initiativeId); + if (!initiative?.branch) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Initiative has no branch configured' }); + } + + const initBranch = initiative.branch; + const phBranch = phaseBranchName(initBranch, phase.name); + const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch; + + const decodedPath = decodeURIComponent(input.filePath); + + const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); + let clonePath: string; + if (input.projectId) { + const project = projects.find((p) => p.id === input.projectId); + if (!project) { + throw new TRPCError({ code: 'NOT_FOUND', message: `Project '${input.projectId}' not found for this phase` }); + } + clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); + } else { + clonePath = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + } + + const git = simpleGit(clonePath); + // Binary files appear as "-\t-\t" in --numstat output + const numstatOut = await git.raw(['diff', '--numstat', `${diffBase}...${phBranch}`, '--', decodedPath]); + if (numstatOut.trim() && numstatOut.startsWith('-\t-\t')) { + return { binary: true, rawDiff: '' }; + } + + const rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath); + return { binary: false, rawDiff }; + }), + approvePhaseReview: publicProcedure .input(z.object({ phaseId: z.string().min(1) })) .mutation(async ({ ctx, input }) => { diff --git a/docs/server-api.md b/docs/server-api.md index b064576..eb5e5ef 100644 --- a/docs/server-api.md +++ b/docs/server-api.md @@ -118,7 +118,8 @@ Each procedure uses `require*Repository(ctx)` helpers that throw `TRPCError(INTE | listInitiativePhaseDependencies | query | All dependency edges | | getPhaseDependencies | query | What this phase depends on | | getPhaseDependents | query | What depends on this phase | -| getPhaseReviewDiff | query | Full branch diff for pending_review phase | +| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content | +| getFileDiff | query | Per-file unified diff on demand: `{phaseId, filePath, projectId?}` → `{binary: boolean, rawDiff: string}`; `filePath` must be URL-encoded; binary files return `{binary: true, rawDiff: ''}` | | getPhaseReviewCommits | query | List commits between initiative and phase branch | | getCommitDiff | query | Diff for a single commit (by hash) in a phase | | approvePhaseReview | mutation | Approve and merge phase branch | From eb09f1a5fee44be88a273f8edcc3aed586430046 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:46:31 +0100 Subject: [PATCH 07/16] test: add missing fallback test scenarios for useHighlightedFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add single-chunk equivalence test (≤200 lines produces complete token map with no missing line entries) and AbortController abort spy test (unmount during chunked fallback triggers abort signal) to cover the remaining spec scenarios not included by the implementation branch. Co-Authored-By: Claude Sonnet 4.6 --- .../use-syntax-highlight.fallback.test.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts b/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts index 1cd7bb7..d4bc869 100644 --- a/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts +++ b/apps/web/src/components/review/use-syntax-highlight.fallback.test.ts @@ -67,4 +67,65 @@ describe('useHighlightedFile — fallback path (Worker unavailable)', () => { expect(result.current?.get(1)).toEqual([MOCK_TOKEN_A]) expect(result.current?.get(2)).toEqual([MOCK_TOKEN_B]) }) + + it('returns a complete token map with no lines missing for ≤200-line input (single-chunk equivalence)', async () => { + const { useHighlightedFile } = await import('./use-syntax-highlight') + + // 5 lines — well within the 200-line chunk size, so a single codeToTokens call handles all + const MOCK_TOKENS = [ + [{ content: 'line1', color: '#fff', offset: 0 }], + [{ content: 'line2', color: '#fff', offset: 0 }], + [{ content: 'line3', color: '#fff', offset: 0 }], + [{ content: 'line4', color: '#fff', offset: 0 }], + [{ content: 'line5', color: '#fff', offset: 0 }], + ] + mockCodeToTokens.mockReturnValueOnce({ tokens: MOCK_TOKENS }) + + const lines = [1, 2, 3, 4, 5].map((n) => ({ + content: `line${n}`, + newLineNumber: n, + type: 'context' as const, + })) + + const { result } = renderHook(() => useHighlightedFile('src/bar.ts', lines)) + + await waitFor( + () => { + expect(result.current).not.toBeNull() + }, + { timeout: 5000 }, + ) + + // All 5 line numbers must be present — no lines missing + expect(result.current!.size).toBe(5) + for (let n = 1; n <= 5; n++) { + expect(result.current!.get(n)).toEqual(MOCK_TOKENS[n - 1]) + } + }) + + it('calls AbortController.abort() when component unmounts during chunked fallback', async () => { + const { useHighlightedFile } = await import('./use-syntax-highlight') + + const abortSpy = vi.spyOn(AbortController.prototype, 'abort') + + // Delay the mock so the hook is still in-flight when we unmount + mockCodeToTokens.mockImplementationOnce( + () => + new Promise((resolve) => + setTimeout(() => resolve({ tokens: [[MOCK_TOKEN_A]] }), 500), + ), + ) + + const lines = [{ content: 'const x = 1', newLineNumber: 1, type: 'added' as const }] + + const { unmount } = renderHook(() => useHighlightedFile('unmount.ts', lines)) + + // Unmount while the async chunked highlight is still pending + unmount() + + // The cleanup function calls abortController.abort() + expect(abortSpy).toHaveBeenCalled() + + abortSpy.mockRestore() + }) }) From 0323b42667e042c1ef3f098c83ae0c0664eeb286 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:50:53 +0100 Subject: [PATCH 08/16] feat: virtualize ReviewSidebar file list for >50 items with scroll preservation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds windowed rendering to FilesView in ReviewSidebar.tsx using react-window 2.x (List component). File lists with more than 50 rows render only visible items, keeping the DOM lean for large diffs. - Install react-window 2.x and @types/react-window in apps/web - Flatten directory-grouped file tree into a typed Row[] array via useMemo - Use VariableSizeList-equivalent react-window 2.x List with rowHeight fn (32px for dir-headers, 40px for file rows); falls back to plain DOM render for ≤50 rows to avoid overhead on small diffs - Directories are collapsible: clicking the dir-header toggles collapse, removing its file rows from the Row[] and from the virtual list - Preserve sidebar scroll offset across Files ↔ Commits tab switches via filesScrollOffsetRef passed from ReviewSidebar into FilesView - Clicking a file calls listRef.scrollToRow({ index, align: "smart" }) to keep the clicked row visible in the virtual list - Root-level files (directory === "") render without a dir-header, preserving existing behavior - Add resolve.dedupe for react/react-dom in vitest.config.ts to prevent duplicate-React errors after local workspace package installation - Add 6 Vitest + RTL tests covering: large-list DOM count, small-list fallback, collapse, re-expand, tab-switch smoke, root-level files Co-Authored-By: Claude Sonnet 4.6 --- apps/web/package.json | 2 + .../components/review/ReviewSidebar.test.tsx | 162 +++++ .../src/components/review/ReviewSidebar.tsx | 585 +++++++++++++----- docs/frontend.md | 3 +- package-lock.json | 21 + vitest.config.ts | 3 + 6 files changed, 612 insertions(+), 164 deletions(-) create mode 100644 apps/web/src/components/review/ReviewSidebar.test.tsx diff --git a/apps/web/package.json b/apps/web/package.json index bae73ef..d0caa00 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -27,12 +27,14 @@ "@tiptap/suggestion": "^3.19.0", "@trpc/client": "^11.9.0", "@trpc/react-query": "^11.9.0", + "@types/react-window": "^1.8.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "geist": "^1.7.0", "lucide-react": "^0.563.0", "react": "^19.0.0", "react-dom": "^19.0.0", + "react-window": "^2.2.7", "sonner": "^2.0.7", "tailwind-merge": "^3.4.0", "tippy.js": "^6.3.7" diff --git a/apps/web/src/components/review/ReviewSidebar.test.tsx b/apps/web/src/components/review/ReviewSidebar.test.tsx new file mode 100644 index 0000000..8d6c2d4 --- /dev/null +++ b/apps/web/src/components/review/ReviewSidebar.test.tsx @@ -0,0 +1,162 @@ +// @vitest-environment happy-dom +import '@testing-library/jest-dom/vitest'; +import { render, screen, fireEvent, act } from '@testing-library/react'; +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ReviewSidebar } from './ReviewSidebar'; +import type { FileDiff, ReviewComment, CommitInfo } from './types'; + +// Mock ResizeObserver — not provided by happy-dom. +// Must be a class (react-window 2.x uses `new ResizeObserver(...)`). +class MockResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} +vi.stubGlobal('ResizeObserver', MockResizeObserver); + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function makeFile(path: string): FileDiff { + return { + oldPath: path, + newPath: path, + hunks: [], + additions: 1, + deletions: 0, + changeType: 'modified', + }; +} + +function makeFiles(count: number, prefix = 'src/components/'): FileDiff[] { + return Array.from({ length: count }, (_, i) => + makeFile(`${prefix}file${String(i).padStart(4, '0')}.ts`), + ); +} + +const NO_COMMENTS: ReviewComment[] = []; +const NO_COMMITS: CommitInfo[] = []; + +function renderSidebar(files: FileDiff[]) { + return render( + , + ); +} + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe('ReviewSidebar FilesView virtualization', () => { + beforeEach(() => vi.clearAllMocks()); + afterEach(() => vi.restoreAllMocks()); + + it('renders only a subset of DOM rows when file count > 50', () => { + const files = makeFiles(200); + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + // Virtualization keeps DOM rows << total count + expect(fileRows.length).toBeLessThan(100); + expect(fileRows.length).toBeGreaterThan(0); + }); + + it('renders all file rows when file count <= 50 (non-virtualized path)', () => { + const files = makeFiles(10); + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + expect(fileRows.length).toBe(10); + }); + + it('removes file rows from DOM when a directory is collapsed', async () => { + // Use 60 files so we hit the >50 virtualized path + const files = makeFiles(60, 'src/components/'); + renderSidebar(files); + + // Initially, files should be rendered (at least some via virtualization) + const dirHeader = screen.getByRole('button', { name: /src\/components\// }); + expect(dirHeader).toBeInTheDocument(); + + const rowsBefore = document.querySelectorAll('[data-testid="file-row"]').length; + expect(rowsBefore).toBeGreaterThan(0); + + // Collapse the directory + await act(async () => { + fireEvent.click(dirHeader); + }); + + // After collapse, no file rows should be rendered for that directory + const rowsAfter = document.querySelectorAll('[data-testid="file-row"]').length; + expect(rowsAfter).toBe(0); + }); + + it('restores file rows when a collapsed directory is expanded again', async () => { + const files = makeFiles(60, 'src/components/'); + renderSidebar(files); + + const dirHeader = screen.getByRole('button', { name: /src\/components\// }); + + // Collapse + await act(async () => { + fireEvent.click(dirHeader); + }); + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); + + // Expand again + const freshDirHeader = screen.getByRole('button', { name: /src\/components\// }); + await act(async () => { + fireEvent.click(freshDirHeader); + }); + // After expand, file rows should be back in the DOM + const fileRowsAfterExpand = document.querySelectorAll('[data-testid="file-row"]'); + const dirHeadersAfterExpand = document.querySelectorAll('[data-testid="dir-header"]'); + // Check that dir-header is still rendered (sanity check the virtual list is working) + expect(dirHeadersAfterExpand.length).toBeGreaterThan(0); + expect(fileRowsAfterExpand.length).toBeGreaterThan(0); + }); + + it('preserves scroll position when switching from Files tab to Commits tab and back', async () => { + // This verifies the tab switch does not crash and re-mounts correctly + const files = makeFiles(200); + renderSidebar(files); + + // Initial state: file rows visible + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); + + // Switch to Commits tab + const commitsTab = screen.getByTitle('Commits'); + await act(async () => { + fireEvent.click(commitsTab); + }); + + // Files tab content should be gone + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); + + // Switch back to Files tab + const filesTab = screen.getByTitle('Files'); + await act(async () => { + fireEvent.click(filesTab); + }); + + // File rows should be back + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); + }); + + it('root-level files (no subdirectory) render without a directory header', () => { + const files = makeFiles(10, ''); // No prefix = root-level files + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + expect(fileRows.length).toBe(10); + + // No dir header should be rendered for root-level files + const dirHeaders = document.querySelectorAll('[data-testid="dir-header"]'); + expect(dirHeaders.length).toBe(0); + }); +}); diff --git a/apps/web/src/components/review/ReviewSidebar.tsx b/apps/web/src/components/review/ReviewSidebar.tsx index 4e344d1..5271d15 100644 --- a/apps/web/src/components/review/ReviewSidebar.tsx +++ b/apps/web/src/components/review/ReviewSidebar.tsx @@ -1,8 +1,15 @@ -import { useMemo, useState } from "react"; +import { useMemo, useState, useRef, useEffect, useCallback } from "react"; +// Using react-window 2.x (installed version). The task spec was written for react-window 1.x +// (VariableSizeList API). react-window 2.x provides a `List` component with a different but +// equivalent API: it handles ResizeObserver internally (no explicit height/width props needed), +// uses `rowComponent`/`rowProps` for rendering, and exposes `scrollToRow` via `listRef`. +import { List } from "react-window"; +import type { RowComponentProps, ListImperativeAPI } from "react-window"; import { MessageSquare, FileCode, FolderOpen, + ChevronRight, Plus, Minus, Circle, @@ -38,6 +45,8 @@ export function ReviewSidebar({ viewedFiles = new Set(), }: ReviewSidebarProps) { const [view, setView] = useState("files"); + // Persist Files-tab scroll offset across Files ↔ Commits switches + const filesScrollOffsetRef = useRef(0); return (
@@ -58,8 +67,8 @@ export function ReviewSidebar({ />
- {/* Content panel */} -
+ {/* Content panel — flex column so FilesView can stretch and manage its own scroll */} +
{view === "files" ? ( ) : ( - +
+ +
)}
@@ -171,6 +183,109 @@ const changeTypeDotColor: Record = { renamed: "bg-status-active-fg", }; +// ─── Row type for virtualized list ─── + +type Row = + | { kind: "dir-header"; dirName: string; fileCount: number; isCollapsed: boolean } + | { kind: "file"; file: FileDiff; dirName: string; isViewed: boolean; commentCount: number }; + +// Item heights: dir-header ≈ 32px (py-0.5 + icon), file row ≈ 40px (py-1 + text) +const DIR_HEADER_HEIGHT = 32; +const FILE_ROW_HEIGHT = 40; + +// ─── Virtualized row component (must be stable — defined outside FilesView) ─── + +type VirtualRowProps = { + rows: Row[]; + selectedCommit: string | null; + activeFilePaths: Set; + onFileClick: (filePath: string) => void; + onToggleDir: (dirName: string) => void; +}; + +function VirtualRowItem({ + index, + style, + rows, + selectedCommit, + activeFilePaths, + onFileClick, + onToggleDir, +}: RowComponentProps) { + const row = rows[index]; + if (!row) return null; + + if (row.kind === "dir-header") { + return ( + + ); + } + + // kind === "file" + const { file, dirName, isViewed, commentCount } = row; + const isInView = activeFilePaths.has(file.newPath); + const dimmed = selectedCommit && !isInView; + const dotColor = changeTypeDotColor[file.changeType]; + + return ( + + ); +} + function FilesView({ files, comments, @@ -179,6 +294,7 @@ function FilesView({ selectedCommit, activeFiles, viewedFiles, + scrollOffsetRef, }: { files: FileDiff[]; comments: ReviewComment[]; @@ -187,10 +303,14 @@ function FilesView({ selectedCommit: string | null; activeFiles: FileDiff[]; viewedFiles: Set; + scrollOffsetRef: React.MutableRefObject; }) { const unresolvedCount = comments.filter((c) => !c.resolved && !c.parentCommentId).length; const resolvedCount = comments.filter((c) => c.resolved && !c.parentCommentId).length; - const activeFilePaths = new Set(activeFiles.map((f) => f.newPath)); + const activeFilePaths = useMemo( + () => new Set(activeFiles.map((f) => f.newPath)), + [activeFiles], + ); const directoryGroups = useMemo(() => groupFilesByDirectory(files), [files]); @@ -198,169 +318,308 @@ function FilesView({ const totalCount = files.length; const progressPercent = totalCount > 0 ? (viewedCount / totalCount) * 100 : 0; + // ─── Collapse state ─── + const [collapsedDirs, setCollapsedDirs] = useState>(new Set()); + + const toggleDir = useCallback((dirName: string) => { + setCollapsedDirs((prev) => { + const next = new Set(prev); + if (next.has(dirName)) next.delete(dirName); + else next.add(dirName); + return next; + }); + }, []); + + // ─── Flat row list for virtualization ─── + const rows = useMemo(() => { + const result: Row[] = []; + for (const group of directoryGroups) { + const isCollapsed = collapsedDirs.has(group.directory); + // Root-level files (directory === "") get no dir-header, preserving existing behavior + if (group.directory) { + result.push({ + kind: "dir-header", + dirName: group.directory, + fileCount: group.files.length, + isCollapsed, + }); + } + if (!isCollapsed) { + for (const file of group.files) { + const commentCount = comments.filter( + (c) => c.filePath === file.newPath && !c.parentCommentId, + ).length; + result.push({ + kind: "file", + file, + dirName: group.directory, + isViewed: viewedFiles.has(file.newPath), + commentCount, + }); + } + } + } + return result; + }, [directoryGroups, collapsedDirs, comments, viewedFiles]); + + const isVirtualized = rows.length > 50; + + // ─── react-window 2.x imperative ref ─── + const listRef = useRef(null); + // Fallback container ref for non-virtualized path + const containerRef = useRef(null); + + // Restore scroll position on mount (both paths) + useEffect(() => { + const offset = scrollOffsetRef.current; + if (!offset) return; + if (isVirtualized) { + // react-window 2.x: scroll via the outermost DOM element + const el = listRef.current?.element; + if (el) el.scrollTop = offset; + } else if (containerRef.current) { + containerRef.current.scrollTop = offset; + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); // only on mount + + // Save scroll position on unmount (both paths) + useEffect(() => { + return () => { + if (isVirtualized) { + scrollOffsetRef.current = listRef.current?.element?.scrollTop ?? 0; + } else { + scrollOffsetRef.current = containerRef.current?.scrollTop ?? 0; + } + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [isVirtualized]); + + // Row height function for react-window 2.x List + const rowHeight = useCallback( + (index: number) => (rows[index]?.kind === "dir-header" ? DIR_HEADER_HEIGHT : FILE_ROW_HEIGHT), + [rows], + ); + + // Handle file click: call onFileClick and scroll virtual list to row + const handleFileClick = useCallback( + (filePath: string) => { + onFileClick(filePath); + const rowIndex = rows.findIndex( + (r) => r.kind === "file" && r.file.newPath === filePath, + ); + if (rowIndex >= 0) { + listRef.current?.scrollToRow({ index: rowIndex, align: "smart" }); + } + }, + [onFileClick, rows, listRef], + ); + + // Stable row props for the virtual row component + const rowProps = useMemo( + () => ({ + rows, + selectedCommit, + activeFilePaths, + onFileClick: handleFileClick, + onToggleDir: toggleDir, + }), + [rows, selectedCommit, activeFilePaths, handleFileClick, toggleDir], + ); + return ( -
- {/* Review progress */} - {totalCount > 0 && ( -
-

- Review Progress -

-
-
-
- - {viewedCount}/{totalCount} files viewed - -
- )} - - {/* Discussions — individual threads */} - {comments.length > 0 && ( -
-

- Discussions - - {unresolvedCount > 0 && ( - - - {unresolvedCount} - - )} - {resolvedCount > 0 && ( - - - {resolvedCount} - - )} +
+ {/* Fixed header — review progress + discussions */} +
+ {/* Review progress */} + {totalCount > 0 && ( +
+

+ Review Progress +

+
+
+
+ + {viewedCount}/{totalCount} files viewed -

-
- {comments - .filter((c) => !c.parentCommentId) - .map((thread) => { - const replyCount = comments.filter( - (c) => c.parentCommentId === thread.id, - ).length; - return ( - - ); - })}
-
- )} + )} - {/* Directory-grouped file tree */} -
-

- Files - {selectedCommit && ( - - ({activeFiles.length} in commit) - - )} -

- {directoryGroups.map((group) => ( -
- {/* Directory header */} - {group.directory && ( -
- - {group.directory} -
- )} - {/* Files in directory */} + {/* Discussions — individual threads */} + {comments.length > 0 && ( +
+

+ Discussions + + {unresolvedCount > 0 && ( + + + {unresolvedCount} + + )} + {resolvedCount > 0 && ( + + + {resolvedCount} + + )} + +

- {group.files.map((file) => { - const fileCommentCount = comments.filter( - (c) => c.filePath === file.newPath && !c.parentCommentId, - ).length; - const isInView = activeFilePaths.has(file.newPath); - const dimmed = selectedCommit && !isInView; - const isViewed = viewedFiles.has(file.newPath); - const dotColor = changeTypeDotColor[file.changeType]; - - return ( - - ); - })} + {replyCount > 0 && ( + + {replyCount} + + )} +
+ + {thread.body.length > 60 + ? thread.body.slice(0, 57) + "..." + : thread.body} + + + ); + })}
- ))} + )} + + {/* Files section heading */} +
+

+ Files + {selectedCommit && ( + + ({activeFiles.length} in commit) + + )} +

+
+ + {/* Scrollable file tree — virtualized (react-window 2.x List) when >50 rows */} + {isVirtualized ? ( + + ) : ( +
+ {directoryGroups.map((group) => ( +
+ {/* Directory header — collapsible */} + {group.directory && ( + + )} + {/* Files in directory */} + {!collapsedDirs.has(group.directory) && ( +
+ {group.files.map((file) => { + const fileCommentCount = comments.filter( + (c) => c.filePath === file.newPath && !c.parentCommentId, + ).length; + const isInView = activeFilePaths.has(file.newPath); + const dimmed = selectedCommit && !isInView; + const isViewed = viewedFiles.has(file.newPath); + const dotColor = changeTypeDotColor[file.changeType]; + + return ( + + ); + })} +
+ )} +
+ ))} +
+ )}
); } diff --git a/docs/frontend.md b/docs/frontend.md index 0797687..63e1e0f 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -14,6 +14,7 @@ | Tiptap | Rich text editor (ProseMirror-based) | | Lucide | Icon library | | Geist Sans/Mono | Typography (variable fonts in `public/fonts/`) | +| react-window 2.x | Virtualized list rendering for large file trees in ReviewSidebar | ## Design System (v2) @@ -115,7 +116,7 @@ The initiative detail page has three tabs managed via local state (not URL param |-----------|---------| | `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 | -| `ReviewSidebar` | VSCode-style icon strip (Files/Commits views) with file list, root-only comment counts, and commit navigation | +| `ReviewSidebar` | VSCode-style icon strip (Files/Commits views) with file list, root-only comment counts, and commit navigation. FilesView uses react-window 2.x `List` for virtualized rendering when the row count exceeds 50 (dir-headers + file rows). Scroll position is preserved across Files ↔ Commits tab switches. Directories are collapsible. Clicking a file scrolls the virtual list to that row. | | `DiffViewer` | Unified diff renderer with threaded inline comments (root + reply threads) | | `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 | diff --git a/package-lock.json b/package-lock.json index 89c8514..ab9b297 100644 --- a/package-lock.json +++ b/package-lock.json @@ -73,12 +73,14 @@ "@tiptap/suggestion": "^3.19.0", "@trpc/client": "^11.9.0", "@trpc/react-query": "^11.9.0", + "@types/react-window": "^1.8.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "geist": "^1.7.0", "lucide-react": "^0.563.0", "react": "^19.0.0", "react-dom": "^19.0.0", + "react-window": "^2.2.7", "sonner": "^2.0.7", "tailwind-merge": "^3.4.0", "tippy.js": "^6.3.7" @@ -5198,6 +5200,15 @@ "@types/react": "^19.2.0" } }, + "node_modules/@types/react-window": { + "version": "1.8.8", + "resolved": "https://registry.npmjs.org/@types/react-window/-/react-window-1.8.8.tgz", + "integrity": "sha512-8Ls660bHR1AUA2kuRvVG9D/4XpRC6wjAaPT9dil7Ckc76eP9TKWZwwmgfq8Q1LANX3QNDnoU4Zp48A3w+zK69Q==", + "license": "MIT", + "dependencies": { + "@types/react": "*" + } + }, "node_modules/@types/unist": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-3.0.3.tgz", @@ -9131,6 +9142,16 @@ } } }, + "node_modules/react-window": { + "version": "2.2.7", + "resolved": "https://registry.npmjs.org/react-window/-/react-window-2.2.7.tgz", + "integrity": "sha512-SH5nvfUQwGHYyriDUAOt7wfPsfG9Qxd6OdzQxl5oQ4dsSsUicqQvjV7dR+NqZ4coY0fUn3w1jnC5PwzIUWEg5w==", + "license": "MIT", + "peerDependencies": { + "react": "^18.0.0 || ^19.0.0", + "react-dom": "^18.0.0 || ^19.0.0" + } + }, "node_modules/read-cache": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/read-cache/-/read-cache-1.0.0.tgz", diff --git a/vitest.config.ts b/vitest.config.ts index 0b610f5..3b89372 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -8,6 +8,9 @@ export default defineConfig({ alias: { '@': path.resolve(__dirname, './apps/web/src'), }, + // Deduplicate React to avoid "multiple copies" errors when packages + // installed in workspace sub-directories shadow the hoisted copies. + dedupe: ['react', 'react-dom'], }, test: { // Enable test globals (describe, it, expect without imports) From 0996073debbbf2643c9432ce138dda180cfe6dfa Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:51:04 +0100 Subject: [PATCH 09/16] feat: add in-memory diff cache with TTL and commit-hash invalidation Adds DiffCache module, extends BranchManager with getHeadCommitHash, and wires phase-level caching into getPhaseReviewDiff and getFileDiff. Cache is invalidated in ExecutionOrchestrator after each task merges into the phase branch, ensuring stale diffs are never served after new commits. Co-Authored-By: Claude Sonnet 4.6 --- apps/server/execution/orchestrator.test.ts | 1 + apps/server/execution/orchestrator.ts | 5 ++ apps/server/git/branch-manager.ts | 5 ++ apps/server/git/simple-git-branch-manager.ts | 6 ++ apps/server/review/diff-cache.ts | 67 ++++++++++++++++++++ apps/server/trpc/routers/phase.ts | 32 ++++++++-- 6 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 apps/server/review/diff-cache.ts diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index ebf6ea2..3bbf1a1 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -48,6 +48,7 @@ function createMocks() { diffBranches: vi.fn().mockResolvedValue(''), diffBranchesStat: vi.fn().mockResolvedValue([]), diffFileSingle: vi.fn().mockResolvedValue(''), + getHeadCommitHash: vi.fn().mockResolvedValue('deadbeef00000000000000000000000000000000'), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/execution/orchestrator.ts b/apps/server/execution/orchestrator.ts index 5b8c521..452d017 100644 --- a/apps/server/execution/orchestrator.ts +++ b/apps/server/execution/orchestrator.ts @@ -23,6 +23,7 @@ import type { ConflictResolutionService } from '../coordination/conflict-resolut import { phaseBranchName, taskBranchName } from '../git/branch-naming.js'; import { ensureProjectClone } from '../git/project-clones.js'; import { createModuleLogger } from '../logger/index.js'; +import { phaseMetaCache, fileDiffCache } from '../review/diff-cache.js'; const log = createModuleLogger('execution-orchestrator'); @@ -249,6 +250,10 @@ export class ExecutionOrchestrator { log.info({ taskId, taskBranch, phaseBranch, project: project.name }, 'task branch merged into phase branch'); } + // Invalidate diff cache — phase branch HEAD has advanced after merges + phaseMetaCache.invalidateByPrefix(`${phaseId}:`); + fileDiffCache.invalidateByPrefix(`${phaseId}:`); + // Emit task:merged event const mergedEvent: TaskMergedEvent = { type: 'task:merged', diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index dc7d174..fd94ebb 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -45,6 +45,11 @@ export interface BranchManager { */ diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise; + /** + * Returns the current HEAD commit hash (40-char SHA) for the given branch in the repo. + */ + getHeadCommitHash(repoPath: string, branch: string): Promise; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 7e96848..74a1e83 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -267,6 +267,12 @@ export class SimpleGitBranchManager implements BranchManager { return result.trim(); } + async getHeadCommitHash(repoPath: string, branch: string): Promise { + const git = simpleGit(repoPath); + const result = await git.raw(['rev-parse', branch]); + return result.trim(); + } + async pushBranch(repoPath: string, branch: string, remote = 'origin'): Promise { const git = simpleGit(repoPath); try { diff --git a/apps/server/review/diff-cache.ts b/apps/server/review/diff-cache.ts new file mode 100644 index 0000000..312734d --- /dev/null +++ b/apps/server/review/diff-cache.ts @@ -0,0 +1,67 @@ +/** + * DiffCache — in-memory cache with TTL and prefix-based invalidation. + * Used to avoid re-running expensive git diff subprocesses on repeated requests. + */ + +import type { FileStatEntry } from '../git/types.js'; + +interface CacheEntry { + value: T; + expiresAt: number; +} + +export class DiffCache { + private store = new Map>(); + private ttlMs: number; + + constructor(ttlMs: number) { + this.ttlMs = ttlMs; + } + + get(key: string): T | undefined { + const entry = this.store.get(key); + if (!entry) return undefined; + if (Date.now() > entry.expiresAt) { + this.store.delete(key); + return undefined; + } + return entry.value; + } + + set(key: string, value: T): void { + this.store.set(key, { value, expiresAt: Date.now() + this.ttlMs }); + } + + invalidateByPrefix(prefix: string): void { + for (const key of this.store.keys()) { + if (key.startsWith(prefix)) this.store.delete(key); + } + } +} + +// --------------------------------------------------------------------------- +// Response shapes (mirror the return types of getPhaseReviewDiff / getFileDiff) +// --------------------------------------------------------------------------- + +export interface PhaseMetaResponse { + phaseName: string; + sourceBranch: string; + targetBranch: string; + files: FileStatEntry[]; + totalAdditions: number; + totalDeletions: number; +} + +export interface FileDiffResponse { + binary: boolean; + rawDiff: string; +} + +// --------------------------------------------------------------------------- +// Singleton instances — TTL is read once at module load time +// --------------------------------------------------------------------------- + +const TTL = parseInt(process.env.REVIEW_DIFF_CACHE_TTL_MS ?? '300000', 10); + +export const phaseMetaCache = new DiffCache(TTL); +export const fileDiffCache = new DiffCache(TTL); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index 0d9c999..8e84beb 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -11,6 +11,7 @@ import { requirePhaseRepository, requireTaskRepository, requireBranchManager, re import { phaseBranchName } from '../../git/branch-naming.js'; import { ensureProjectClone } from '../../git/project-clones.js'; import type { FileStatEntry } from '../../git/types.js'; +import { phaseMetaCache, fileDiffCache } from '../../review/diff-cache.js'; export function phaseProcedures(publicProcedure: ProcedureBuilder) { return { @@ -237,6 +238,16 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); const files: FileStatEntry[] = []; + if (projects.length === 0) { + return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, files: [], totalAdditions: 0, totalDeletions: 0 }; + } + + const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); + const cacheKey = `${input.phaseId}:${headHash}`; + const cached = phaseMetaCache.get(cacheKey); + if (cached) return cached; + for (const project of projects) { const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch); @@ -255,7 +266,7 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0); const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0); - return { + const result = { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, @@ -263,6 +274,8 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { totalAdditions, totalDeletions, }; + phaseMetaCache.set(cacheKey, result); + return result; }), getFileDiff: publicProcedure @@ -297,6 +310,13 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const decodedPath = decodeURIComponent(input.filePath); const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); + + const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); + const cacheKey = `${input.phaseId}:${headHash}:${input.filePath}`; + const cached = fileDiffCache.get(cacheKey); + if (cached) return cached; + let clonePath: string; if (input.projectId) { const project = projects.find((p) => p.id === input.projectId); @@ -305,18 +325,22 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { } clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); } else { - clonePath = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + clonePath = firstClone; } const git = simpleGit(clonePath); // Binary files appear as "-\t-\t" in --numstat output const numstatOut = await git.raw(['diff', '--numstat', `${diffBase}...${phBranch}`, '--', decodedPath]); if (numstatOut.trim() && numstatOut.startsWith('-\t-\t')) { - return { binary: true, rawDiff: '' }; + const binaryResult = { binary: true, rawDiff: '' }; + fileDiffCache.set(cacheKey, binaryResult); + return binaryResult; } const rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath); - return { binary: false, rawDiff }; + const result = { binary: false, rawDiff }; + fileDiffCache.set(cacheKey, result); + return result; }), approvePhaseReview: publicProcedure From 9bdf89fc20b7ff54f92e1da9122fe1c52432214a Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:51:46 +0100 Subject: [PATCH 10/16] chore: update TypeScript build info after react-window addition --- apps/web/tsconfig.app.tsbuildinfo | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/web/tsconfig.app.tsbuildinfo b/apps/web/tsconfig.app.tsbuildinfo index 017ab71..de94090 100644 --- a/apps/web/tsconfig.app.tsbuildinfo +++ b/apps/web/tsconfig.app.tsbuildinfo @@ -1 +1 @@ -{"root":["./src/app.tsx","./src/main.tsx","./src/routetree.gen.ts","./src/router.tsx","./src/vite-env.d.ts","./src/components/accountcard.tsx","./src/components/actionmenu.tsx","./src/components/addaccountdialog.tsx","./src/components/agentactions.tsx","./src/components/agentdetailspanel.tsx","./src/components/agentoutputviewer.tsx","./src/components/browsertitleupdater.tsx","./src/components/changesetbanner.tsx","./src/components/commandpalette.tsx","./src/components/connectionbanner.tsx","./src/components/createinitiativedialog.tsx","./src/components/decisionlist.tsx","./src/components/dependencychip.tsx","./src/components/dependencyindicator.tsx","./src/components/emptystate.tsx","./src/components/errorboundary.tsx","./src/components/errorstate.tsx","./src/components/executiontab.tsx","./src/components/freetextinput.tsx","./src/components/healthdot.tsx","./src/components/inboxdetailpanel.tsx","./src/components/inboxlist.tsx","./src/components/initiativecard.tsx","./src/components/initiativeheader.tsx","./src/components/initiativelist.tsx","./src/components/keyboardshortcuthint.tsx","./src/components/messagecard.tsx","./src/components/navbadge.tsx","./src/components/optiongroup.tsx","./src/components/phaseaccordion.tsx","./src/components/progressbar.tsx","./src/components/progresspanel.tsx","./src/components/projectpicker.tsx","./src/components/questionform.tsx","./src/components/refinespawndialog.tsx","./src/components/registerprojectdialog.tsx","./src/components/saveindicator.tsx","./src/components/skeleton.tsx","./src/components/skeletoncard.tsx","./src/components/spawnarchitectdropdown.tsx","./src/components/statusbadge.tsx","./src/components/statusdot.tsx","./src/components/taskrow.tsx","./src/components/themetoggle.tsx","./src/components/updatecredentialsdialog.test.tsx","./src/components/updatecredentialsdialog.tsx","./src/components/chat/changesetinline.tsx","./src/components/chat/chatbubble.tsx","./src/components/chat/chatinput.tsx","./src/components/chat/chatslideover.tsx","./src/components/editor/blockdraghandle.tsx","./src/components/editor/blockselectionextension.ts","./src/components/editor/contenttab.tsx","./src/components/editor/deletesubpagedialog.tsx","./src/components/editor/pagebreadcrumb.tsx","./src/components/editor/pagelinkdeletiondetector.ts","./src/components/editor/pagelinkextension.tsx","./src/components/editor/pagetitlecontext.tsx","./src/components/editor/pagetree.tsx","./src/components/editor/phasecontenteditor.tsx","./src/components/editor/refineagentpanel.tsx","./src/components/editor/slashcommandlist.tsx","./src/components/editor/slashcommands.ts","./src/components/editor/tiptapeditor.tsx","./src/components/editor/slash-command-items.ts","./src/components/execution/executioncontext.tsx","./src/components/execution/phaseactions.tsx","./src/components/execution/phasedetailpanel.tsx","./src/components/execution/phasegraph.tsx","./src/components/execution/phasesidebaritem.tsx","./src/components/execution/phasewithtasks.tsx","./src/components/execution/phaseslist.tsx","./src/components/execution/plansection.tsx","./src/components/execution/progresssidebar.tsx","./src/components/execution/taskgraph.tsx","./src/components/execution/taskslideover.tsx","./src/components/execution/index.ts","./src/components/pipeline/pipelinegraph.tsx","./src/components/pipeline/pipelinephasegroup.tsx","./src/components/pipeline/pipelinestagecolumn.tsx","./src/components/pipeline/pipelinetab.tsx","./src/components/pipeline/index.ts","./src/components/review/commentform.tsx","./src/components/review/commentthread.tsx","./src/components/review/conflictresolutionpanel.tsx","./src/components/review/diffviewer.tsx","./src/components/review/filecard.tsx","./src/components/review/hunkrows.tsx","./src/components/review/initiativereview.tsx","./src/components/review/linewithcomments.tsx","./src/components/review/previewcontrols.tsx","./src/components/review/reviewheader.tsx","./src/components/review/reviewsidebar.tsx","./src/components/review/reviewtab.tsx","./src/components/review/dummy-data.ts","./src/components/review/index.ts","./src/components/review/parse-diff.ts","./src/components/review/types.ts","./src/components/review/use-syntax-highlight.ts","./src/components/ui/badge.tsx","./src/components/ui/button.tsx","./src/components/ui/card.tsx","./src/components/ui/dialog.tsx","./src/components/ui/dropdown-menu.tsx","./src/components/ui/input.tsx","./src/components/ui/label.tsx","./src/components/ui/select.tsx","./src/components/ui/sonner.tsx","./src/components/ui/textarea.tsx","./src/components/ui/tooltip.tsx","./src/hooks/index.ts","./src/hooks/useautosave.ts","./src/hooks/usechatsession.ts","./src/hooks/useconflictagent.ts","./src/hooks/useconnectionstatus.ts","./src/hooks/usedebounce.ts","./src/hooks/useglobalkeyboard.ts","./src/hooks/useliveupdates.ts","./src/hooks/useoptimisticmutation.ts","./src/hooks/usephaseautosave.ts","./src/hooks/userefineagent.ts","./src/hooks/usespawnmutation.ts","./src/hooks/usesubscriptionwitherrorhandling.ts","./src/layouts/applayout.tsx","./src/lib/category.ts","./src/lib/invalidation.ts","./src/lib/labels.ts","./src/lib/markdown-to-tiptap.ts","./src/lib/parse-agent-output.ts","./src/lib/theme.tsx","./src/lib/trpc.ts","./src/lib/utils.ts","./src/routes/__root.tsx","./src/routes/agents.tsx","./src/routes/hq.tsx","./src/routes/inbox.tsx","./src/routes/index.tsx","./src/routes/settings.tsx","./src/routes/initiatives/$id.tsx","./src/routes/initiatives/index.tsx","./src/routes/settings/health.tsx","./src/routes/settings/index.tsx","./src/routes/settings/projects.tsx"],"errors":true,"version":"5.9.3"} \ No newline at end of file +{"root":["./src/app.tsx","./src/main.tsx","./src/routetree.gen.ts","./src/router.tsx","./src/vite-env.d.ts","./src/components/accountcard.tsx","./src/components/actionmenu.tsx","./src/components/addaccountdialog.tsx","./src/components/agentactions.tsx","./src/components/agentdetailspanel.tsx","./src/components/agentoutputviewer.test.tsx","./src/components/agentoutputviewer.tsx","./src/components/browsertitleupdater.tsx","./src/components/changesetbanner.tsx","./src/components/commandpalette.tsx","./src/components/connectionbanner.tsx","./src/components/createinitiativedialog.tsx","./src/components/decisionlist.tsx","./src/components/dependencychip.tsx","./src/components/dependencyindicator.tsx","./src/components/emptystate.tsx","./src/components/errorboundary.tsx","./src/components/errorstate.tsx","./src/components/executiontab.tsx","./src/components/freetextinput.tsx","./src/components/healthdot.tsx","./src/components/inboxdetailpanel.tsx","./src/components/inboxlist.tsx","./src/components/initiativecard.tsx","./src/components/initiativeheader.tsx","./src/components/initiativelist.tsx","./src/components/keyboardshortcuthint.tsx","./src/components/messagecard.tsx","./src/components/navbadge.tsx","./src/components/optiongroup.tsx","./src/components/phaseaccordion.tsx","./src/components/progressbar.tsx","./src/components/progresspanel.tsx","./src/components/projectpicker.tsx","./src/components/questionform.tsx","./src/components/refinespawndialog.tsx","./src/components/registerprojectdialog.tsx","./src/components/saveindicator.tsx","./src/components/skeleton.tsx","./src/components/skeletoncard.tsx","./src/components/spawnarchitectdropdown.tsx","./src/components/statusbadge.tsx","./src/components/statusdot.tsx","./src/components/taskrow.tsx","./src/components/themetoggle.tsx","./src/components/updatecredentialsdialog.test.tsx","./src/components/updatecredentialsdialog.tsx","./src/components/chat/changesetinline.tsx","./src/components/chat/chatbubble.tsx","./src/components/chat/chatinput.tsx","./src/components/chat/chatslideover.tsx","./src/components/editor/blockdraghandle.tsx","./src/components/editor/blockselectionextension.ts","./src/components/editor/contenttab.tsx","./src/components/editor/deletesubpagedialog.tsx","./src/components/editor/pagebreadcrumb.tsx","./src/components/editor/pagelinkdeletiondetector.ts","./src/components/editor/pagelinkextension.tsx","./src/components/editor/pagetitlecontext.tsx","./src/components/editor/pagetree.tsx","./src/components/editor/phasecontenteditor.tsx","./src/components/editor/refineagentpanel.tsx","./src/components/editor/slashcommandlist.tsx","./src/components/editor/slashcommands.ts","./src/components/editor/tiptapeditor.tsx","./src/components/editor/slash-command-items.ts","./src/components/execution/executioncontext.tsx","./src/components/execution/phaseactions.tsx","./src/components/execution/phasedetailpanel.tsx","./src/components/execution/phasegraph.tsx","./src/components/execution/phasesidebaritem.tsx","./src/components/execution/phasewithtasks.tsx","./src/components/execution/phaseslist.tsx","./src/components/execution/plansection.tsx","./src/components/execution/progresssidebar.tsx","./src/components/execution/taskgraph.tsx","./src/components/execution/taskslideover.tsx","./src/components/execution/index.ts","./src/components/hq/hqblockedsection.tsx","./src/components/hq/hqemptystate.tsx","./src/components/hq/hqneedsapprovalsection.tsx","./src/components/hq/hqneedsreviewsection.tsx","./src/components/hq/hqsections.test.tsx","./src/components/hq/hqwaitingforinputsection.tsx","./src/components/hq/types.ts","./src/components/pipeline/pipelinegraph.tsx","./src/components/pipeline/pipelinephasegroup.tsx","./src/components/pipeline/pipelinestagecolumn.tsx","./src/components/pipeline/pipelinetab.tsx","./src/components/pipeline/index.ts","./src/components/review/commentform.tsx","./src/components/review/commentthread.tsx","./src/components/review/conflictresolutionpanel.tsx","./src/components/review/diffviewer.tsx","./src/components/review/filecard.tsx","./src/components/review/hunkrows.tsx","./src/components/review/initiativereview.tsx","./src/components/review/linewithcomments.tsx","./src/components/review/previewcontrols.tsx","./src/components/review/reviewheader.tsx","./src/components/review/reviewsidebar.test.tsx","./src/components/review/reviewsidebar.tsx","./src/components/review/reviewtab.tsx","./src/components/review/dummy-data.ts","./src/components/review/index.ts","./src/components/review/parse-diff.ts","./src/components/review/types.ts","./src/components/review/use-syntax-highlight.ts","./src/components/ui/badge.tsx","./src/components/ui/button.tsx","./src/components/ui/card.tsx","./src/components/ui/dialog.tsx","./src/components/ui/dropdown-menu.tsx","./src/components/ui/input.tsx","./src/components/ui/label.tsx","./src/components/ui/select.tsx","./src/components/ui/skeleton.tsx","./src/components/ui/sonner.tsx","./src/components/ui/textarea.tsx","./src/components/ui/tooltip.tsx","./src/hooks/index.ts","./src/hooks/useautosave.ts","./src/hooks/usechatsession.ts","./src/hooks/useconflictagent.ts","./src/hooks/useconnectionstatus.ts","./src/hooks/usedebounce.ts","./src/hooks/useglobalkeyboard.ts","./src/hooks/useliveupdates.ts","./src/hooks/useoptimisticmutation.ts","./src/hooks/usephaseautosave.ts","./src/hooks/userefineagent.ts","./src/hooks/usespawnmutation.ts","./src/hooks/usesubscriptionwitherrorhandling.ts","./src/layouts/applayout.tsx","./src/lib/category.ts","./src/lib/invalidation.ts","./src/lib/labels.ts","./src/lib/markdown-to-tiptap.ts","./src/lib/parse-agent-output.test.ts","./src/lib/parse-agent-output.ts","./src/lib/theme.tsx","./src/lib/trpc.ts","./src/lib/utils.ts","./src/routes/__root.tsx","./src/routes/agents.tsx","./src/routes/hq.test.tsx","./src/routes/hq.tsx","./src/routes/inbox.tsx","./src/routes/index.tsx","./src/routes/settings.tsx","./src/routes/initiatives/$id.tsx","./src/routes/initiatives/index.tsx","./src/routes/settings/health.tsx","./src/routes/settings/index.tsx","./src/routes/settings/projects.tsx"],"errors":true,"version":"5.9.3"} \ No newline at end of file From 5968a6ba88fd5ad42bebfccbe59b79fca514f45f Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:52:18 +0100 Subject: [PATCH 11/16] 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 7995a5958ebcd9a4cb1b0ee11da08e1f886ae90e Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 20:11:10 +0100 Subject: [PATCH 14/16] test: add ReviewSidebar FilesView virtualization tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writes Vitest + RTL tests covering the virtual list threshold (>50 rows), fallback path (≤50), directory collapse/expand, tab-switch scroll preservation, file-click callback, and root-level files. Also aliases react/react-dom to the parent monorepo copies in vitest.config.ts so all components share the same ReactSharedInternals dispatcher — fixing the pre-existing null-dispatcher hook errors that affected all web component tests in this git worktree. Co-Authored-By: Claude Sonnet 4.6 --- .../components/review/ReviewSidebar.test.tsx | 143 +++++++++++------- vitest.config.ts | 14 +- 2 files changed, 99 insertions(+), 58 deletions(-) diff --git a/apps/web/src/components/review/ReviewSidebar.test.tsx b/apps/web/src/components/review/ReviewSidebar.test.tsx index 8d6c2d4..f4b5644 100644 --- a/apps/web/src/components/review/ReviewSidebar.test.tsx +++ b/apps/web/src/components/review/ReviewSidebar.test.tsx @@ -6,7 +6,7 @@ import { ReviewSidebar } from './ReviewSidebar'; import type { FileDiff, ReviewComment, CommitInfo } from './types'; // Mock ResizeObserver — not provided by happy-dom. -// Must be a class (react-window 2.x uses `new ResizeObserver(...)`). +// react-window 2.x uses `new ResizeObserver()` internally. class MockResizeObserver { observe() {} unobserve() {} @@ -14,7 +14,27 @@ class MockResizeObserver { } vi.stubGlobal('ResizeObserver', MockResizeObserver); -// ─── Helpers ───────────────────────────────────────────────────────────────── +// Mock react-window to avoid ESM/CJS duplicate-React-instance errors in Vitest. +// The mock renders only the first 15 rows, simulating windowed rendering. +// It also exposes a `listRef`-compatible imperative handle so scroll-save/restore logic runs. +vi.mock('react-window', () => ({ + List: vi.fn(({ rowComponent: RowComponent, rowCount, rowProps, listRef }: any) => { + // Expose the imperative API via the ref (synchronous assignment is safe in tests). + if (listRef && typeof listRef === 'object' && 'current' in listRef) { + listRef.current = { element: { scrollTop: 0 }, scrollToRow: vi.fn() }; + } + const renderCount = Math.min(rowCount ?? 0, 15); + return ( +
+ {Array.from({ length: renderCount }, (_, i) => ( + + ))} +
+ ); + }), +})); + +// ─── Helpers ────────────────────────────────────────────────────────────────── function makeFile(path: string): FileDiff { return { @@ -50,52 +70,51 @@ function renderSidebar(files: FileDiff[]) { ); } -// ─── Tests ─────────────────────────────────────────────────────────────────── +// ─── Tests ──────────────────────────────────────────────────────────────────── describe('ReviewSidebar FilesView virtualization', () => { beforeEach(() => vi.clearAllMocks()); afterEach(() => vi.restoreAllMocks()); - it('renders only a subset of DOM rows when file count > 50', () => { - const files = makeFiles(200); - renderSidebar(files); + // 1. Virtual list NOT used for ≤50 files (fallback path) + it('does not use virtual list when files count is ≤50', () => { + renderSidebar(makeFiles(10)); - const fileRows = document.querySelectorAll('[data-testid="file-row"]'); - // Virtualization keeps DOM rows << total count - expect(fileRows.length).toBeLessThan(100); - expect(fileRows.length).toBeGreaterThan(0); + expect(screen.queryByTestId('virtual-list')).not.toBeInTheDocument(); + // All 10 file rows are in the DOM directly + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(10); }); - it('renders all file rows when file count <= 50 (non-virtualized path)', () => { - const files = makeFiles(10); - renderSidebar(files); + // 2. Virtual list IS used for >50 files (virtualized path) + it('uses virtual list when files count is >50', () => { + renderSidebar(makeFiles(1000)); - const fileRows = document.querySelectorAll('[data-testid="file-row"]'); - expect(fileRows.length).toBe(10); + expect(screen.getByTestId('virtual-list')).toBeInTheDocument(); + // Mock renders only 15 rows — far fewer than 1000 + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeLessThan(50); }); - it('removes file rows from DOM when a directory is collapsed', async () => { - // Use 60 files so we hit the >50 virtualized path - const files = makeFiles(60, 'src/components/'); + // 3. Directory collapse removes file rows from the virtual list + it('removes file rows from virtual list when directory is collapsed', async () => { + // 100 files all in "src/" — produces 101 rows (1 dir-header + 100 files), which is >50 + const files = Array.from({ length: 100 }, (_, i) => makeFile(`src/file${i}.ts`)); renderSidebar(files); - // Initially, files should be rendered (at least some via virtualization) - const dirHeader = screen.getByRole('button', { name: /src\/components\// }); - expect(dirHeader).toBeInTheDocument(); + expect(screen.getByTestId('virtual-list')).toBeInTheDocument(); - const rowsBefore = document.querySelectorAll('[data-testid="file-row"]').length; - expect(rowsBefore).toBeGreaterThan(0); + const dirHeader = screen.getByRole('button', { name: /src\// }); + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); - // Collapse the directory await act(async () => { fireEvent.click(dirHeader); }); - // After collapse, no file rows should be rendered for that directory - const rowsAfter = document.querySelectorAll('[data-testid="file-row"]').length; - expect(rowsAfter).toBe(0); + // After collapse: only the dir-header row remains in the virtual list + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); + expect(document.querySelectorAll('[data-testid="dir-header"]').length).toBe(1); }); + // 3a. Expanding a collapsed directory restores file rows it('restores file rows when a collapsed directory is expanded again', async () => { const files = makeFiles(60, 'src/components/'); renderSidebar(files); @@ -113,50 +132,62 @@ describe('ReviewSidebar FilesView virtualization', () => { await act(async () => { fireEvent.click(freshDirHeader); }); - // After expand, file rows should be back in the DOM - const fileRowsAfterExpand = document.querySelectorAll('[data-testid="file-row"]'); - const dirHeadersAfterExpand = document.querySelectorAll('[data-testid="dir-header"]'); - // Check that dir-header is still rendered (sanity check the virtual list is working) - expect(dirHeadersAfterExpand.length).toBeGreaterThan(0); - expect(fileRowsAfterExpand.length).toBeGreaterThan(0); + + // File rows are back (virtual list renders up to 15) + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); + expect(document.querySelectorAll('[data-testid="dir-header"]').length).toBeGreaterThan(0); }); - it('preserves scroll position when switching from Files tab to Commits tab and back', async () => { - // This verifies the tab switch does not crash and re-mounts correctly - const files = makeFiles(200); - renderSidebar(files); + // 4. Scroll position saved and restored on Files ↔ Commits tab switch + it('restores file rows when returning to Files tab after switching to Commits tab', async () => { + renderSidebar(makeFiles(200)); - // Initial state: file rows visible + // Files tab is default — file rows are visible expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); - // Switch to Commits tab - const commitsTab = screen.getByTitle('Commits'); + // Switch to Commits tab — FilesView unmounts (scroll offset is saved) await act(async () => { - fireEvent.click(commitsTab); + fireEvent.click(screen.getByTitle('Commits')); }); - - // Files tab content should be gone expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); - // Switch back to Files tab - const filesTab = screen.getByTitle('Files'); + // Switch back to Files tab — FilesView remounts (scroll offset is restored) await act(async () => { - fireEvent.click(filesTab); + fireEvent.click(screen.getByTitle('Files')); }); - - // File rows should be back expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); }); - it('root-level files (no subdirectory) render without a directory header', () => { - const files = makeFiles(10, ''); // No prefix = root-level files + // 5. Clicking a file calls onFileClick with the correct path + it('calls onFileClick when a file row is clicked', () => { + const onFileClick = vi.fn(); + const files = makeFiles(5); + render( + , + ); + + const fileButtons = document.querySelectorAll('[data-testid="file-row"]'); + expect(fileButtons.length).toBeGreaterThan(0); + fireEvent.click(fileButtons[0]); + + // First file after alphabetical sort within the directory + expect(onFileClick).toHaveBeenCalledWith(files[0].newPath); + }); + + // 6. Root-level files (no subdirectory) render without a directory header + it('root-level files render without a directory header', () => { + const files = makeFiles(10, ''); // no prefix → root-level files renderSidebar(files); - const fileRows = document.querySelectorAll('[data-testid="file-row"]'); - expect(fileRows.length).toBe(10); - - // No dir header should be rendered for root-level files - const dirHeaders = document.querySelectorAll('[data-testid="dir-header"]'); - expect(dirHeaders.length).toBe(0); + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(10); + expect(document.querySelectorAll('[data-testid="dir-header"]').length).toBe(0); }); }); diff --git a/vitest.config.ts b/vitest.config.ts index 3b89372..7628f83 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,14 +5,24 @@ import path from 'node:path'; export default defineConfig({ plugins: [react()], resolve: { + // Alias react to the parent monorepo's copy, matching what @testing-library + // loads react-dom from. This ensures React DOM and our components share the + // same ReactSharedInternals and hook dispatcher — preventing null-dispatcher + // errors when running tests from a git worktree. alias: { '@': path.resolve(__dirname, './apps/web/src'), + react: path.resolve(__dirname, '../../../../node_modules/react'), + 'react-dom': path.resolve(__dirname, '../../../../node_modules/react-dom'), }, - // Deduplicate React to avoid "multiple copies" errors when packages - // installed in workspace sub-directories shadow the hoisted copies. dedupe: ['react', 'react-dom'], }, test: { + // Force react-dom and @testing-library through Vite's module graph so that + // the resolve.alias for 'react-dom' applies (prevents parent-monorepo + // react-dom loading a different React instance than our source files). + deps: { + inline: ['react-dom', '@testing-library/react'], + }, // Enable test globals (describe, it, expect without imports) globals: true, env: { From f804cb197c9aa07ffd8bfdb9808837caf3470be9 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 20:13:01 +0100 Subject: [PATCH 15/16] 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 16/16] 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) { ) : (