diff --git a/packages/cli/src/__tests__/diff.routes.test.ts b/packages/cli/src/__tests__/diff.routes.test.ts index 2cc4c04..a221152 100644 --- a/packages/cli/src/__tests__/diff.routes.test.ts +++ b/packages/cli/src/__tests__/diff.routes.test.ts @@ -3,6 +3,7 @@ import fs from "node:fs/promises"; import http from "node:http"; import os from "node:os"; import path from "node:path"; +import type { DiffResponse } from "@stagereview/types/diff"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { closeDb, getDb } from "../db/client.js"; import { chapterRun } from "../db/schema/index.js"; @@ -140,8 +141,12 @@ function rawRequest(port: number, requestPath: string): Promise { }); } +function parseDiffResponse(body: string): DiffResponse { + return JSON.parse(body) as DiffResponse; +} + describe("diff API", () => { - it("GET /api/runs/:runId/diff.patch streams the committed-scope unified diff", async () => { + it("GET /api/runs/:runId/diff.patch returns JSON with patch and fileContents", async () => { const { baseSha, headSha } = await initRepoWithTwoCommits(); const runId = insertCommittedRun(baseSha, headSha); @@ -149,12 +154,17 @@ describe("diff API", () => { const res = await rawRequest(port, `/api/runs/${runId}/diff.patch`); expect(res.status).toBe(200); - expect(res.headers["content-type"]).toMatch(/text\/plain/); + expect(res.headers["content-type"]).toMatch(/application\/json/); expect(res.headers["cache-control"]).toBe("private, max-age=300"); - expect(res.body).toContain("diff --git a/file.txt b/file.txt"); - expect(res.body).toContain("+world"); - // --no-color should suppress ANSI escapes (literal ESC character). - expect(res.body).not.toContain(`${String.fromCharCode(27)}[`); + + const data = parseDiffResponse(res.body); + expect(data.patch).toContain("diff --git a/file.txt b/file.txt"); + expect(data.patch).toContain("+world"); + expect(data.patch).not.toContain(`${String.fromCharCode(27)}[`); + + expect(data.fileContents["file.txt"]).toBeDefined(); + expect(data.fileContents["file.txt"]?.oldContent).toBe("hello\n"); + expect(data.fileContents["file.txt"]?.newContent).toBe("hello\nworld\n"); }); it("returns the unstaged diff for workingTree/unstaged runs", async () => { @@ -167,14 +177,16 @@ describe("diff API", () => { expect(res.status).toBe(200); expect(res.headers["cache-control"]).toBe("no-store"); - expect(res.body).toContain("+unstaged"); + + const data = parseDiffResponse(res.body); + expect(data.patch).toContain("+unstaged"); + expect(data.fileContents["file.txt"]?.newContent).toBe("hello\nworld\nunstaged\n"); }); it("returns the staged diff for workingTree/staged runs", async () => { await initRepoWithTwoCommits(); await fs.writeFile(path.join(repoRoot, "file.txt"), "hello\nworld\nstaged\n"); git("add", "file.txt"); - // Add an extra unstaged change that must NOT appear in the staged diff. await fs.writeFile(path.join(repoRoot, "file.txt"), "hello\nworld\nstaged\nleak\n"); const runId = insertWorkingTreeRun(WORKING_TREE_REF.STAGED); @@ -183,16 +195,17 @@ describe("diff API", () => { expect(res.status).toBe(200); expect(res.headers["cache-control"]).toBe("no-store"); - expect(res.body).toContain("+staged"); - expect(res.body).not.toContain("+leak"); + + const data = parseDiffResponse(res.body); + expect(data.patch).toContain("+staged"); + expect(data.patch).not.toContain("+leak"); + expect(data.fileContents["file.txt"]?.newContent).toBe("hello\nworld\nstaged\n"); }); it("returns the combined diff vs HEAD for workingTree/work runs", async () => { await initRepoWithTwoCommits(); - // staged change await fs.writeFile(path.join(repoRoot, "file.txt"), "hello\nworld\nstaged\n"); git("add", "file.txt"); - // plus an unstaged change on top await fs.writeFile(path.join(repoRoot, "file.txt"), "hello\nworld\nstaged\nunstaged\n"); const runId = insertWorkingTreeRun(WORKING_TREE_REF.WORK); @@ -200,9 +213,60 @@ describe("diff API", () => { const res = await rawRequest(port, `/api/runs/${runId}/diff.patch`); expect(res.status).toBe(200); - // `git diff HEAD` includes both staged and unstaged changes. - expect(res.body).toContain("+staged"); - expect(res.body).toContain("+unstaged"); + + const data = parseDiffResponse(res.body); + expect(data.patch).toContain("+staged"); + expect(data.patch).toContain("+unstaged"); + expect(data.fileContents["file.txt"]?.newContent).toBe("hello\nworld\nstaged\nunstaged\n"); + }); + + it("returns null oldContent for added files", async () => { + git("init", "--initial-branch=main"); + git("config", "user.email", "test@example.com"); + git("config", "user.name", "Test"); + git("config", "commit.gpgsign", "false"); + + await fs.writeFile(path.join(repoRoot, "initial.txt"), "init\n"); + git("add", "initial.txt"); + git("commit", "-m", "initial"); + const baseSha = git("rev-parse", "HEAD").trim(); + + await fs.writeFile(path.join(repoRoot, "new-file.txt"), "brand new\n"); + git("add", "new-file.txt"); + git("commit", "-m", "add new file"); + const headSha = git("rev-parse", "HEAD").trim(); + + const runId = insertCommittedRun(baseSha, headSha); + const { port } = await startWithRoutes(); + const res = await rawRequest(port, `/api/runs/${runId}/diff.patch`); + + const data = parseDiffResponse(res.body); + expect(data.fileContents["new-file.txt"]?.oldContent).toBeNull(); + expect(data.fileContents["new-file.txt"]?.newContent).toBe("brand new\n"); + }); + + it("returns null newContent for deleted files", async () => { + git("init", "--initial-branch=main"); + git("config", "user.email", "test@example.com"); + git("config", "user.name", "Test"); + git("config", "commit.gpgsign", "false"); + + await fs.writeFile(path.join(repoRoot, "doomed.txt"), "goodbye\n"); + git("add", "doomed.txt"); + git("commit", "-m", "add file"); + const baseSha = git("rev-parse", "HEAD").trim(); + + git("rm", "doomed.txt"); + git("commit", "-m", "delete file"); + const headSha = git("rev-parse", "HEAD").trim(); + + const runId = insertCommittedRun(baseSha, headSha); + const { port } = await startWithRoutes(); + const res = await rawRequest(port, `/api/runs/${runId}/diff.patch`); + + const data = parseDiffResponse(res.body); + expect(data.fileContents["doomed.txt"]?.oldContent).toBe("goodbye\n"); + expect(data.fileContents["doomed.txt"]?.newContent).toBeNull(); }); it("returns 404 for unknown runId", async () => { diff --git a/packages/cli/src/routes/diff.ts b/packages/cli/src/routes/diff.ts index 4faf906..109fef4 100644 --- a/packages/cli/src/routes/diff.ts +++ b/packages/cli/src/routes/diff.ts @@ -1,14 +1,21 @@ -import { spawn } from "node:child_process"; -import type { ServerResponse } from "node:http"; +import { execFile } from "node:child_process"; +import fs from "node:fs/promises"; import path from "node:path"; +import { promisify } from "node:util"; +import type { DiffResponse, FileContentsMap } from "@stagereview/types/diff"; import { eq } from "drizzle-orm"; import type { StageDb } from "../db/client.js"; +import type { ChapterRunRow } from "../db/schema/chapter-run.js"; import { chapterRun } from "../db/schema/index.js"; import { buildDiffArgs } from "../git.js"; -import { SCOPE_KIND } from "../schema.js"; +import { SCOPE_KIND, WORKING_TREE_REF } from "../schema.js"; import type { Route } from "../server.js"; import { writeJson } from "./json.js"; +const execFileAsync = promisify(execFile); + +const MAX_FILE_BYTES = 5 * 1024 * 1024; + export function diffRoutes(db: StageDb): Route[] { return [ { @@ -27,9 +34,6 @@ export function diffRoutes(db: StageDb): Route[] { return; } - // Defense in depth: repoRoot was validated at ingest, but the diff endpoint is - // a fresh boundary. Refuse non-absolute paths or any path containing `..` - // segments so we can't be tricked into spawning git against a traversal. const repoRoot = run.repoRoot; if (!path.isAbsolute(repoRoot) || repoRoot.split(path.sep).includes("..")) { writeJson(res, 500, { @@ -42,81 +46,142 @@ export function diffRoutes(db: StageDb): Route[] { const cacheControl = run.scopeKind === SCOPE_KIND.COMMITTED ? "private, max-age=300" : "no-store"; - await streamGitDiff(res, repoRoot, args, cacheControl); + try { + const { stdout: patch } = await execFileAsync("git", args, { + cwd: repoRoot, + encoding: "utf8", + maxBuffer: 50 * 1024 * 1024, + }); + const fileContents = await buildFileContents(run, repoRoot, patch); + const body: DiffResponse = { patch, fileContents }; + res.writeHead(200, { + "Content-Type": "application/json; charset=utf-8", + "Cache-Control": cacheControl, + }); + res.end(JSON.stringify(body)); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + writeJson(res, 500, { error: message }); + } }, }, ]; } -function streamGitDiff( - res: ServerResponse, +const MINUS_RE = /^--- (?:a\/)?(.+)$/m; +const PLUS_RE = /^\+\+\+ (?:b\/)?(.+)$/m; +const BINARY_RE = /^Binary files/m; + +interface ParsedFilePaths { + oldPath: string | null; + newPath: string | null; + isBinary: boolean; +} + +function parseFilePathsFromPatch(patch: string): ParsedFilePaths[] { + if (!patch.trim()) return []; + + const segments = patch.split(/\ndiff --git /); + const results: ParsedFilePaths[] = []; + + for (let i = 0; i < segments.length; i++) { + const segment = segments[i]; + if (segment === undefined) continue; + const text = i === 0 ? segment : `diff --git ${segment}`; + if (!text.startsWith("diff --git ")) continue; + + const isBinary = BINARY_RE.test(text); + + const minus = text.match(MINUS_RE); + const plus = text.match(PLUS_RE); + + const oldPath = minus?.[1] && minus[1] !== "/dev/null" ? minus[1] : null; + const newPath = plus?.[1] && plus[1] !== "/dev/null" ? plus[1] : null; + + results.push({ oldPath, newPath, isBinary }); + } + + return results; +} + +async function getGitFileContent( cwd: string, - args: string[], - cacheControl: string, -): Promise { - return new Promise((resolve) => { - const child = spawn("git", args, { cwd, stdio: ["ignore", "pipe", "pipe"] }); - - let stderr = ""; - let settled = false; - - const settle = () => { - if (settled) return; - settled = true; - resolve(); - }; - - const writeSuccessHeaders = () => { - if (res.headersSent) return; - res.writeHead(200, { - "Content-Type": "text/plain; charset=utf-8", - "Cache-Control": cacheControl, - }); - }; - - // If the client disconnects before git finishes, kill the child so we don't leak - // a long-running diff for a request nobody is reading. - res.once("close", () => { - if (!child.killed) child.kill("SIGTERM"); + ref: string, + filePath: string, +): Promise { + try { + const { stdout } = await execFileAsync("git", ["show", `${ref}:${filePath}`], { + cwd, + encoding: "utf8", + maxBuffer: MAX_FILE_BYTES, }); + return stdout; + } catch { + return null; + } +} - child.on("error", (err) => { - if (!res.headersSent) { - writeJson(res, 500, { error: `git failed: ${err.message}` }); - } else if (!res.writableEnded) { - res.end(); - } - settle(); - }); +async function readFileContent(repoRoot: string, filePath: string): Promise { + const resolved = path.resolve(repoRoot, filePath); + const rel = path.relative(repoRoot, resolved); + if (rel.startsWith("..") || path.isAbsolute(rel)) return null; + try { + return await fs.readFile(resolved, "utf8"); + } catch { + return null; + } +} - child.stderr.on("data", (chunk: Buffer) => { - stderr += chunk.toString("utf8"); - }); +function getContentRefs(run: ChapterRunRow): { oldRef: string; newRef: string | "DISK" } { + if (run.scopeKind === SCOPE_KIND.COMMITTED) { + return { oldRef: run.baseSha, newRef: run.headSha }; + } + switch (run.workingTreeRef) { + case WORKING_TREE_REF.UNSTAGED: + return { oldRef: "", newRef: "DISK" }; + case WORKING_TREE_REF.STAGED: + return { oldRef: "HEAD", newRef: "" }; + case WORKING_TREE_REF.WORK: + return { oldRef: "HEAD", newRef: "DISK" }; + default: + return { oldRef: "HEAD", newRef: "HEAD" }; + } +} - child.stdout.on("data", (chunk: Buffer) => { - writeSuccessHeaders(); - if (!res.write(chunk)) { - child.stdout.pause(); - res.once("drain", () => child.stdout.resume()); - } - }); +function fetchContent( + repoRoot: string, + ref: string | "DISK", + filePath: string, +): Promise { + if (ref === "DISK") return readFileContent(repoRoot, filePath); + return getGitFileContent(repoRoot, ref, filePath); +} - child.on("close", (code) => { - if (settled) return; - if (code === 0) { - // Successful exit with no stdout (empty diff) still needs headers + end. - writeSuccessHeaders(); - res.end(); - } else if (!res.headersSent) { - const message = stderr.trim() || `git exited with code ${code}`; - writeJson(res, 500, { error: message }); - } else { - // Headers were already sent, so we can't change the status. Log and terminate - // the response — the client will see a truncated patch. - process.stderr.write(`git diff failed mid-stream (exit ${code}): ${stderr}\n`); - if (!res.writableEnded) res.end(); - } - settle(); - }); - }); +async function buildFileContents( + run: ChapterRunRow, + repoRoot: string, + patch: string, +): Promise { + const files = parseFilePathsFromPatch(patch); + const { oldRef, newRef } = getContentRefs(run); + + const entries = await Promise.all( + files.map(async ({ oldPath, newPath, isBinary }) => { + const key = newPath ?? oldPath; + if (!key || isBinary) return null; + + const [oldContent, newContent] = await Promise.all([ + oldPath ? fetchContent(repoRoot, oldRef, oldPath) : Promise.resolve(null), + newPath ? fetchContent(repoRoot, newRef, newPath) : Promise.resolve(null), + ]); + + return [key, { oldContent, newContent }] as const; + }), + ); + + const map: FileContentsMap = {}; + for (const entry of entries) { + if (entry) map[entry[0]] = entry[1]; + } + return map; } diff --git a/packages/types/package.json b/packages/types/package.json index 6e74e2a..d2957ca 100644 --- a/packages/types/package.json +++ b/packages/types/package.json @@ -7,6 +7,7 @@ "exports": { ".": "./src/index.ts", "./chapters": "./src/chapters.ts", + "./diff": "./src/diff.ts", "./prologue": "./src/prologue.ts", "./view-state": "./src/view-state.ts" }, diff --git a/packages/types/src/diff.ts b/packages/types/src/diff.ts new file mode 100644 index 0000000..8143b45 --- /dev/null +++ b/packages/types/src/diff.ts @@ -0,0 +1,16 @@ +export interface FileContent { + /** Full content of the file on the old/deletion side. `null` for added files. */ + oldContent: string | null; + /** Full content of the file on the new/addition side. `null` for deleted files. */ + newContent: string | null; +} + +/** Map from file path (new-side, post-rename) to full file contents. */ +export type FileContentsMap = Record; + +export interface DiffResponse { + /** Raw unified diff patch text. */ + patch: string; + /** Per-file full content for context expansion. */ + fileContents: FileContentsMap; +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 2cc9b4c..e300f69 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,3 +1,4 @@ export * from "./chapters.ts"; +export * from "./diff.ts"; export * from "./prologue.ts"; export * from "./view-state.ts"; diff --git a/packages/web/src/lib/chapter-context.tsx b/packages/web/src/lib/chapter-context.tsx index d08e011..a76026d 100644 --- a/packages/web/src/lib/chapter-context.tsx +++ b/packages/web/src/lib/chapter-context.tsx @@ -38,7 +38,7 @@ function buildChapterLineCountsMap( export function ChapterProvider({ runId, children }: { runId: string; children: ReactNode }) { const { data: chaptersData } = useChapters(runId); - const { data: patch } = useDiffPatch(runId); + const { data: diffData } = useDiffPatch(runId); const chapters = useMemo(() => { if (!chaptersData?.chapters) return []; @@ -46,8 +46,8 @@ export function ChapterProvider({ runId, children }: { runId: string; children: }, [chaptersData?.chapters]); const chapterLineCountsMap = useMemo( - () => buildChapterLineCountsMap(chapters, patch), - [chapters, patch], + () => buildChapterLineCountsMap(chapters, diffData?.patch), + [chapters, diffData?.patch], ); const value = useMemo( diff --git a/packages/web/src/lib/collapse-actions-context.tsx b/packages/web/src/lib/collapse-actions-context.tsx new file mode 100644 index 0000000..621101e --- /dev/null +++ b/packages/web/src/lib/collapse-actions-context.tsx @@ -0,0 +1,37 @@ +import { createContext, type ReactNode, use, useEffect, useMemo, useState } from "react"; +import type { CollapseState } from "@/components/files"; + +interface CollapseActionsWithCount { + collapseState: CollapseState; + fileCount: number; +} + +interface CollapseActionsContextValue { + actions: CollapseActionsWithCount | null; + setActions: (actions: CollapseActionsWithCount | null) => void; +} + +const CollapseActionsContext = createContext(null); + +export function CollapseActionsProvider({ children }: { children: ReactNode }) { + const [actions, setActions] = useState(null); + + const value = useMemo(() => ({ actions, setActions }), [actions]); + + return {children}; +} + +export function useCollapseActionsFromNav(): CollapseActionsWithCount | null { + const ctx = use(CollapseActionsContext); + return ctx?.actions ?? null; +} + +export function useProvideCollapseActions(collapseState: CollapseState, fileCount: number): void { + const ctx = use(CollapseActionsContext); + const setter = ctx?.setActions; + + useEffect(() => { + setter?.({ collapseState, fileCount }); + return () => setter?.(null); + }, [setter, collapseState, fileCount]); +} diff --git a/packages/web/src/lib/filter-files-for-chapter.ts b/packages/web/src/lib/filter-files-for-chapter.ts index 41de609..78717e7 100644 --- a/packages/web/src/lib/filter-files-for-chapter.ts +++ b/packages/web/src/lib/filter-files-for-chapter.ts @@ -1,9 +1,10 @@ -import { getSingularPatch } from "@pierre/diffs"; +import { getSingularPatch, parseDiffFromFile } from "@pierre/diffs"; import type { HunkReference } from "@stagereview/types/chapters"; +import type { FileContentsMap } from "@stagereview/types/diff"; import type { FileDiffEntry } from "./parse-diff"; import { fileDiffToPullRequestFile } from "./parse-diff"; -const HUNK_HEADER_RE = /^@@\s+-(\d+)(?:,\d+)?\s+\+\d+(?:,\d+)?\s+@@/; +const HUNK_HEADER_RE = /^@@\s+-(\d+)(?:,(\d+))?\s+\+\d+(?:,\d+)?\s+@@/; const FILE_BREAK = /\ndiff --git /g; interface FileSegment { @@ -12,16 +13,8 @@ interface FileSegment { text: string; } -/** - * Splits a `git diff` patch into per-file blocks, keyed by both the new path - * (post-rename) and the old path so a chapter `hunkRef` can match either. - * Splitting at the text level (rather than reconstructing from Pierre's parsed - * metadata) preserves the file-level headers Pierre needs to re-parse cleanly. - */ function splitPatchByFile(patch: string): FileSegment[] { if (!patch.trim()) return []; - // Split on every "\ndiff --git " boundary. Re-prepend the prefix to all - // segments after the first so each block starts with a valid file header. const parts = patch.split(FILE_BREAK); const segments: FileSegment[] = []; for (let i = 0; i < parts.length; i++) { @@ -38,8 +31,6 @@ const PLUS_NAME_RE = /^\+\+\+ (?:b\/)?(.+)$/m; const MINUS_NAME_RE = /^--- (?:a\/)?(.+)$/m; function parseFileNames(segment: string): { prevName?: string; name?: string } { - // Prefer the +++/--- lines because they're authoritative; fall back to the - // `diff --git` header for added/deleted files where one side is /dev/null. const plusMatch = segment.match(PLUS_NAME_RE); const minusMatch = segment.match(MINUS_NAME_RE); const gitMatch = segment.match(DIFF_GIT_NAMES_RE); @@ -50,44 +41,91 @@ function parseFileNames(segment: string): { prevName?: string; name?: string } { return { prevName, name }; } -/** - * Filter a per-file patch text down to only the hunks whose old-side start - * matches one of the provided line numbers. Returns null when no hunk matches. - */ -function filterHunksInFile(perFileText: string, oldStarts: ReadonlySet): string | null { - const lines = perFileText.split("\n"); - const headerLines: string[] = []; - const hunks: Array<{ oldStart: number; lines: string[] }> = []; - let current: { oldStart: number; lines: string[] } | null = null; +interface ParsedHunk { + oldStart: number; + oldLines: number; + header: string; + lines: string[]; +} + +function parseHunksFromSegment(segmentText: string): ParsedHunk[] { + const lines = segmentText.split("\n"); + const hunks: ParsedHunk[] = []; + let current: ParsedHunk | null = null; for (const line of lines) { const match = line.match(HUNK_HEADER_RE); if (match) { - if (current) hunks.push(current); - const start = match[1] === undefined ? Number.NaN : Number.parseInt(match[1], 10); - current = { oldStart: start, lines: [line] }; + if (current) { + while (current.lines.at(-1) === "") current.lines.pop(); + hunks.push(current); + } + const oldStart = match[1] === undefined ? 0 : Number.parseInt(match[1], 10); + const oldLines = match[2] === undefined ? 1 : Number.parseInt(match[2], 10); + current = { oldStart, oldLines, header: line, lines: [] }; continue; } if (current) current.lines.push(line); - else headerLines.push(line); } - if (current) hunks.push(current); + if (current) { + while (current.lines.at(-1) === "") current.lines.pop(); + hunks.push(current); + } - const matched = hunks.filter((h) => oldStarts.has(h.oldStart)); - if (matched.length === 0) return null; + return hunks; +} - return [...headerLines, ...matched.flatMap((h) => h.lines)].join("\n"); +/** + * Apply hunks to old file content, producing an intermediate file. + * + * Used in chapter view: applying all NON-chapter hunks to the old file produces + * a base where only the chapter's changes remain as the diff against the new file. + * + * Hunks are applied bottom-to-top (by descending oldStart) so that each splice + * doesn't shift the positions of earlier hunks. + */ +function applyHunksToContent(content: string, hunks: ParsedHunk[]): string { + if (hunks.length === 0) return content; + + const trailingNewline = content.endsWith("\n"); + const fileLines = + content === "" ? [] : trailingNewline ? content.slice(0, -1).split("\n") : content.split("\n"); + + const sorted = [...hunks].sort((a, b) => b.oldStart - a.oldStart); + + for (const hunk of sorted) { + const newContent: string[] = []; + for (const line of hunk.lines) { + if (line.startsWith("+")) { + newContent.push(line.slice(1)); + } else if (line.startsWith("-")) { + // deletion — skip (don't include in output) + } else if (line.startsWith(" ") || line === "") { + newContent.push(line.startsWith(" ") ? line.slice(1) : line); + } + } + + const spliceStart = hunk.oldLines === 0 ? hunk.oldStart : hunk.oldStart - 1; + fileLines.splice(spliceStart, hunk.oldLines, ...newContent); + } + + return fileLines.length > 0 ? fileLines.join("\n") + (trailingNewline ? "\n" : "") : ""; } /** * Filter a parsed PR diff to the files+hunks referenced by a chapter's hunkRefs. * + * When file contents are available, computes an intermediate file by applying + * non-chapter hunks to the old content, then diffs intermediate vs new. This + * produces a clean isPartial=false diff that supports context expansion. + * * File order follows hunkRef first-appearance — that's the LLM's intended * reading order for the chapter, not the file tree's alphabetical order. */ export function filterFilesForChapter( patch: string, hunkRefs: readonly HunkReference[], + fileContents?: FileContentsMap, ): FileDiffEntry[] { if (hunkRefs.length === 0) return []; @@ -105,22 +143,45 @@ export function filterFilesForChapter( const segmentsByPath = new Map(); for (const segment of segments) { if (segment.name) segmentsByPath.set(segment.name, segment); - // Index the previous path too so renames whose hunkRefs use the old path still resolve. if (segment.prevName && segment.prevName !== segment.name) { segmentsByPath.set(segment.prevName, segment); } } const result: FileDiffEntry[] = []; - for (const [filePath, oldStarts] of oldStartsByPath) { + for (const [filePath, chapterOldStarts] of oldStartsByPath) { const segment = segmentsByPath.get(filePath); if (!segment) continue; - const filteredText = filterHunksInFile(segment.text, oldStarts); - if (filteredText === null) continue; - - const diff = getSingularPatch(filteredText); - result.push({ file: fileDiffToPullRequestFile(diff), diff }); + const allHunks = parseHunksFromSegment(segment.text); + const chapterHunks = allHunks.filter((h) => chapterOldStarts.has(h.oldStart)); + if (chapterHunks.length === 0) continue; + + const contents = fileContents?.[segment.name ?? filePath]; + if (contents?.oldContent != null && contents?.newContent != null) { + const nonChapterHunks = allHunks.filter((h) => !chapterOldStarts.has(h.oldStart)); + const intermediateContent = applyHunksToContent(contents.oldContent, nonChapterHunks); + const oldPath = segment.prevName ?? segment.name ?? filePath; + const newPath = segment.name ?? filePath; + const diff = parseDiffFromFile( + { name: oldPath, contents: intermediateContent }, + { name: newPath, contents: contents.newContent }, + ); + result.push({ file: fileDiffToPullRequestFile(diff), diff }); + } else { + const headerLines: string[] = []; + const lines = segment.text.split("\n"); + for (const line of lines) { + if (HUNK_HEADER_RE.test(line)) break; + headerLines.push(line); + } + const filteredText = [ + ...headerLines, + ...chapterHunks.flatMap((h) => [h.header, ...h.lines]), + ].join("\n"); + const diff = getSingularPatch(filteredText); + result.push({ file: fileDiffToPullRequestFile(diff), diff }); + } } return result; diff --git a/packages/web/src/lib/parse-diff.ts b/packages/web/src/lib/parse-diff.ts index 22e155d..5dab577 100644 --- a/packages/web/src/lib/parse-diff.ts +++ b/packages/web/src/lib/parse-diff.ts @@ -1,6 +1,8 @@ -import { type FileDiffMetadata, parsePatchFiles } from "@pierre/diffs"; +import { type FileDiffMetadata, type Hunk, parsePatchFiles } from "@pierre/diffs"; +import type { FileContentsMap } from "@stagereview/types/diff"; import { useMemo } from "react"; import { FILE_STATUS, type FileStatus, type PullRequestFile } from "./diff-types"; +import { splitWithNewlines } from "./split-with-newlines"; // Flatten across ParsedPatch envelopes — `parsePatchFiles` returns one per // `From ` block, but plain `git diff` output yields a single envelope @@ -54,10 +56,67 @@ export interface FileDiffEntry { diff: FileDiffMetadata; } -export function useFileDiffEntries(patch: string | undefined): FileDiffEntry[] { +/** + * Remap a hunk's line-array indices from partial (sequential offset within the + * patch-only arrays) to full-file (actual line position). Pierre's hunk headers + * already carry the 1-based file line numbers (`deletionStart`/`additionStart`); + * converting to 0-based gives the correct index into the full-file arrays. + */ +function reindexHunk(hunk: Hunk): Hunk { + let delIdx = hunk.deletionStart - 1; + let addIdx = hunk.additionStart - 1; + + const reindexedContent = hunk.hunkContent.map((segment) => { + const patched = { ...segment, deletionLineIndex: delIdx, additionLineIndex: addIdx }; + if (segment.type === "context") { + delIdx += segment.lines; + addIdx += segment.lines; + } else { + delIdx += segment.deletions; + addIdx += segment.additions; + } + return patched; + }); + + return { + ...hunk, + deletionLineIndex: hunk.deletionStart - 1, + additionLineIndex: hunk.additionStart - 1, + hunkContent: reindexedContent, + }; +} + +export function enrichFileDiff( + diff: FileDiffMetadata, + fileContents: FileContentsMap | undefined, +): FileDiffMetadata { + if (!fileContents) return diff; + const contents = fileContents[diff.name]; + if (!contents) return diff; + + const oldLines = splitWithNewlines(contents.oldContent); + const newLines = splitWithNewlines(contents.newContent); + if (!oldLines && !newLines) return diff; + + return { + ...diff, + isPartial: false, + hunks: diff.hunks.map(reindexHunk), + ...(oldLines ? { deletionLines: oldLines } : {}), + ...(newLines ? { additionLines: newLines } : {}), + }; +} + +export function useFileDiffEntries( + patch: string | undefined, + fileContents?: FileContentsMap, +): FileDiffEntry[] { return useMemo(() => { if (!patch) return []; const diffs = parsePatchToFileDiffs(patch); - return diffs.map((diff) => ({ file: fileDiffToPullRequestFile(diff), diff })); - }, [patch]); + return diffs.map((diff) => { + const enriched = enrichFileDiff(diff, fileContents); + return { file: fileDiffToPullRequestFile(enriched), diff: enriched }; + }); + }, [patch, fileContents]); } diff --git a/packages/web/src/lib/split-with-newlines.ts b/packages/web/src/lib/split-with-newlines.ts new file mode 100644 index 0000000..dc882bc --- /dev/null +++ b/packages/web/src/lib/split-with-newlines.ts @@ -0,0 +1,11 @@ +/** + * Splits file content into lines while preserving each line's trailing "\n". + * + * Pierre expects each entry in deletionLines/additionLines to include its + * trailing newline. A plain split("\n") strips them, causing Shiki decoration + * positions to exceed the actual rendered line count. + */ +export function splitWithNewlines(content: string | null | undefined): string[] | undefined { + if (content == null) return undefined; + return content.match(/[^\n]*\n|[^\n]+/g) ?? undefined; +} diff --git a/packages/web/src/lib/use-diff-patch.ts b/packages/web/src/lib/use-diff-patch.ts index 0afb056..0ea6a43 100644 --- a/packages/web/src/lib/use-diff-patch.ts +++ b/packages/web/src/lib/use-diff-patch.ts @@ -1,3 +1,4 @@ +import type { DiffResponse } from "@stagereview/types/diff"; import { type UseQueryResult, useQuery } from "@tanstack/react-query"; const DIFF_QUERY_ROOT = "diff"; @@ -6,15 +7,13 @@ export function diffPatchQueryKey(runId: string): readonly unknown[] { return [DIFF_QUERY_ROOT, runId]; } -// Shared queryKey lets react-query dedupe the patch fetch when the same hook -// is mounted from more than one component for the same run. -export function useDiffPatch(runId: string): UseQueryResult { - return useQuery({ +export function useDiffPatch(runId: string): UseQueryResult { + return useQuery({ queryKey: diffPatchQueryKey(runId), queryFn: async () => { const res = await fetch(`/api/runs/${encodeURIComponent(runId)}/diff.patch`); if (!res.ok) throw new Error(`GET diff.patch failed: ${res.status}`); - return res.text(); + return res.json(); }, enabled: runId !== "", staleTime: Number.POSITIVE_INFINITY, diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index a096747..d5cc782 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -1,4 +1,5 @@ import type { Chapter, LineRef } from "@stagereview/types/chapters"; +import type { FileContentsMap } from "@stagereview/types/diff"; import { Link, useNavigate } from "@tanstack/react-router"; import { useCallback, useMemo, useRef, useState } from "react"; import { useHotkeys } from "react-hotkeys-hook"; @@ -12,6 +13,7 @@ import { import { Button } from "@/components/ui/button"; import { Skeleton } from "@/components/ui/skeleton"; import { useChapterContext } from "@/lib/chapter-context"; +import { useProvideCollapseActions } from "@/lib/collapse-actions-context"; import { FILE_STATUS } from "@/lib/diff-types"; import { filterFilesForChapter } from "@/lib/filter-files-for-chapter"; import { formatChapterAsMarkdown } from "@/lib/format-chapter-markdown"; @@ -38,7 +40,7 @@ interface ChapterDetailPageProps { export function ChapterDetailPage({ runId, chapterNumber }: ChapterDetailPageProps) { const { chapters } = useChapterContext(); const { isLoading: chaptersLoading, error: chaptersError } = useChapters(runId); - const { data: patch, isLoading: patchLoading, error: patchError } = useDiffPatch(runId); + const { data: diffData, isLoading: patchLoading, error: patchError } = useDiffPatch(runId); const chapter = chapterNumber === null ? undefined : chapters.find((c) => c.order === chapterNumber); @@ -51,23 +53,33 @@ export function ChapterDetailPage({ runId, chapterNumber }: ChapterDetailPagePro if (error) return ; if (isLoading) return ; if (!chapter) return ; - // Distinguish "still loading / errored" (undefined) from "valid empty diff" - // (""): a chapter with no hunks against an empty PR is rendered as the - // "No changes in this chapter" empty state by FileDiffList, not an error. - if (patch === undefined) { + if (diffData === undefined) { return ; } - return ; + return ( + + ); } interface ChapterDetailContentProps { chapter: Chapter; chapterIndex: number; patch: string; + fileContents: FileContentsMap; } -function ChapterDetailContent({ chapter, chapterIndex, patch }: ChapterDetailContentProps) { +function ChapterDetailContent({ + chapter, + chapterIndex, + patch, + fileContents, +}: ChapterDetailContentProps) { const { runId, chapters: allChapters } = useChapterContext(); const view = useViewState(runId); const [focusedKeyChangeId, setFocusedKeyChangeId] = useState(null); @@ -81,8 +93,8 @@ function ChapterDetailContent({ chapter, chapterIndex, patch }: ChapterDetailCon } const chapterEntries = useMemo( - () => filterFilesForChapter(patch, chapter.hunkRefs), - [patch, chapter.hunkRefs], + () => filterFilesForChapter(patch, chapter.hunkRefs, fileContents), + [patch, chapter.hunkRefs, fileContents], ); const allLineRefsByFile = useMemo( @@ -173,6 +185,7 @@ function ChapterDetailContent({ chapter, chapterIndex, patch }: ChapterDetailCon chapterFilePaths, collapseResetKey, ); + useProvideCollapseActions(collapseState, chapterFilePaths.length); useFileNavigationKeys(chapterFiles, activeFilePath, handleSelectFile); diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 65c2af5..1778da8 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -7,6 +7,7 @@ import { SidebarLayout, } from "@/components/files"; import { Skeleton } from "@/components/ui/skeleton"; +import { useProvideCollapseActions } from "@/lib/collapse-actions-context"; import { FILE_STATUS } from "@/lib/diff-types"; import { buildFileTree, flattenFileTree, sortFileTree } from "@/lib/file-tree"; import { KEYBOARD_SHORTCUTS } from "@/lib/keyboard-shortcuts"; @@ -23,9 +24,9 @@ interface FilesPageProps { } export function FilesPage({ runId, scrollTo }: FilesPageProps) { - const { data, isLoading, error } = useDiffPatch(runId); + const { data: diffData, isLoading, error } = useDiffPatch(runId); - const rawEntries = useFileDiffEntries(data); + const rawEntries = useFileDiffEntries(diffData?.patch, diffData?.fileContents); const entries = useMemo(() => sortFileDiffEntries(rawEntries), [rawEntries]); const files = useMemo(() => entries.map((e) => e.file), [entries]); @@ -51,6 +52,7 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { const filePaths = useMemo(() => files.map((f) => f.path), [files]); const collapseState = useFileCollapseState(defaultCollapsedFileIds, filePaths, runId); + useProvideCollapseActions(collapseState, filePaths.length); const diffListRef = useRef(null); const { activeFilePath, setActiveFileManually } = useActiveFileOnScroll(files); @@ -77,7 +79,7 @@ export function FilesPage({ runId, scrollTo }: FilesPageProps) { useFileNavigationKeys(files, activeFilePath, handleSelectFile); if (error) return ; - if (isLoading || data === undefined) return ; + if (isLoading || diffData === undefined) return ; return ( 0 && collapseState.collapsedFiles.size >= fileCount; + const handleClick = allCollapsed ? collapseState.expandAllFiles : collapseState.collapseAllFiles; + const label = allCollapsed ? "Expand all files" : "Collapse all files"; + + return ( + + + + + {label} + + ); +} + function ErrorState({ error }: { error: unknown }) { return (
@@ -90,8 +125,8 @@ export function PullRequestLayout({ runId }: { runId: string }) { // Fetched here so the Files tab's "N/M viewed" label can render before the // user clicks into the tab; react-query dedupes the same fetch from FilesPage. - const { data: patch } = useDiffPatch(runId); - const fileEntries = useFileDiffEntries(patch); + const { data: diffData } = useDiffPatch(runId); + const fileEntries = useFileDiffEntries(diffData?.patch, diffData?.fileContents); const totalFileCount = fileEntries.length; const viewedFileCount = useMemo(() => { if (totalFileCount === 0) return 0; @@ -111,7 +146,7 @@ export function PullRequestLayout({ runId }: { runId: string }) { })(); const fileCountLabel = (() => { - if (patch === undefined) return undefined; + if (diffData === undefined) return undefined; if (viewedFileCount > 0) return `${viewedFileCount}/${totalFileCount} viewed`; return String(totalFileCount); })(); @@ -128,6 +163,16 @@ export function PullRequestLayout({ runId }: { runId: string }) { return () => observer.disconnect(); }, []); + const { totalAdditions, totalDeletions } = useMemo(() => { + let additions = 0; + let deletions = 0; + for (const entry of fileEntries) { + additions += entry.file.additions; + deletions += entry.file.deletions; + } + return { totalAdditions: additions, totalDeletions: deletions }; + }, [fileEntries]); + if (error) return ; // 48 = the app-shell Topbar's `h-12`, which the picker also has to clear. @@ -137,61 +182,74 @@ export function PullRequestLayout({ runId }: { runId: string }) { } as CSSProperties; return ( -
-
-
- Run -

{data?.run.id ?? runId}

-
- - - - + +
+
+
+ Run +

+ {data?.run.id ?? runId} +

+
+ + + + +
-
+ ); }