From fd387de6ebb9e3b9bdfb2556a5725eb98ddf3876 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 14:47:10 -0700 Subject: [PATCH 01/12] feat: bundle file contents with diff response for context expansion Change the diff endpoint from streaming plain text to returning JSON with both the patch and full file contents, enabling Pierre's expandUnchanged feature for context line expansion in diff views. --- .../cli/src/__tests__/diff.routes.test.ts | 94 ++++++-- packages/cli/src/routes/diff.ts | 217 ++++++++++++------ packages/types/package.json | 1 + packages/types/src/diff.ts | 16 ++ packages/types/src/index.ts | 1 + packages/web/src/lib/chapter-context.tsx | 6 +- .../web/src/lib/filter-files-for-chapter.ts | 8 +- packages/web/src/lib/parse-diff.ts | 34 ++- packages/web/src/lib/split-with-newlines.ts | 11 + packages/web/src/lib/use-diff-patch.ts | 9 +- .../web/src/routes/chapter-detail-page.tsx | 29 ++- packages/web/src/routes/files-page.tsx | 6 +- .../web/src/routes/pull-request-layout.tsx | 6 +- 13 files changed, 329 insertions(+), 109 deletions(-) create mode 100644 packages/types/src/diff.ts create mode 100644 packages/web/src/lib/split-with-newlines.ts diff --git a/packages/cli/src/__tests__/diff.routes.test.ts b/packages/cli/src/__tests__/diff.routes.test.ts index 2cc4c04..7aef1a0 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 "@stage-cli/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..b2d9c45 100644 --- a/packages/cli/src/routes/diff.ts +++ b/packages/cli/src/routes/diff.ts @@ -1,14 +1,19 @@ -import { spawn } from "node:child_process"; -import type { ServerResponse } from "node:http"; +import { execFile, spawn } 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 "@stage-cli/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); + export function diffRoutes(db: StageDb): Route[] { return [ { @@ -27,9 +32,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 +44,166 @@ 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 patch = await collectGitDiff(repoRoot, args); + 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, - cwd: string, - args: string[], - cacheControl: string, -): Promise { - return new Promise((resolve) => { +function collectGitDiff(cwd: string, args: string[]): Promise { + return new Promise((resolve, reject) => { const child = spawn("git", args, { cwd, stdio: ["ignore", "pipe", "pipe"] }); - + const stdoutChunks: Buffer[] = []; 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"); - }); - - child.on("error", (err) => { - if (!res.headersSent) { - writeJson(res, 500, { error: `git failed: ${err.message}` }); - } else if (!res.writableEnded) { - res.end(); - } - settle(); - }); + child.stdout.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)); child.stderr.on("data", (chunk: Buffer) => { stderr += chunk.toString("utf8"); }); - child.stdout.on("data", (chunk: Buffer) => { - writeSuccessHeaders(); - if (!res.write(chunk)) { - child.stdout.pause(); - res.once("drain", () => child.stdout.resume()); - } - }); - + child.on("error", reject); 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 }); + resolve(Buffer.concat(stdoutChunks).toString("utf8")); } 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(); + reject(new Error(stderr.trim() || `git exited with code ${code}`)); } - settle(); }); }); } + +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, + ref: string, + filePath: string, +): Promise { + try { + const { stdout } = await execFileAsync("git", ["show", `${ref}:${filePath}`], { + cwd, + encoding: "utf8", + maxBuffer: 5 * 1024 * 1024, + }); + return stdout; + } catch { + return null; + } +} + +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; + } +} + +function getOldRef(run: ChapterRunRow): string { + if (run.scopeKind === SCOPE_KIND.COMMITTED) return run.baseSha; + switch (run.workingTreeRef) { + case WORKING_TREE_REF.UNSTAGED: + return ""; + case WORKING_TREE_REF.STAGED: + case WORKING_TREE_REF.WORK: + return "HEAD"; + default: + return "HEAD"; + } +} + +function getNewRef(run: ChapterRunRow): string | "DISK" { + if (run.scopeKind === SCOPE_KIND.COMMITTED) return run.headSha; + switch (run.workingTreeRef) { + case WORKING_TREE_REF.UNSTAGED: + case WORKING_TREE_REF.WORK: + return "DISK"; + case WORKING_TREE_REF.STAGED: + return ""; + default: + return "HEAD"; + } +} + +async function buildFileContents( + run: ChapterRunRow, + repoRoot: string, + patch: string, +): Promise { + const files = parseFilePathsFromPatch(patch); + const oldRef = getOldRef(run); + const newRef = getNewRef(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 ? getGitFileContent(repoRoot, oldRef, oldPath) : Promise.resolve(null), + newPath + ? newRef === "DISK" + ? readFileContent(repoRoot, newPath) + : getGitFileContent(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/filter-files-for-chapter.ts b/packages/web/src/lib/filter-files-for-chapter.ts index 41de609..7dd592c 100644 --- a/packages/web/src/lib/filter-files-for-chapter.ts +++ b/packages/web/src/lib/filter-files-for-chapter.ts @@ -1,7 +1,8 @@ import { getSingularPatch } 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"; +import { enrichFileDiff, fileDiffToPullRequestFile } from "./parse-diff"; const HUNK_HEADER_RE = /^@@\s+-(\d+)(?:,\d+)?\s+\+\d+(?:,\d+)?\s+@@/; const FILE_BREAK = /\ndiff --git /g; @@ -88,6 +89,7 @@ function filterHunksInFile(perFileText: string, oldStarts: ReadonlySet): export function filterFilesForChapter( patch: string, hunkRefs: readonly HunkReference[], + fileContents?: FileContentsMap, ): FileDiffEntry[] { if (hunkRefs.length === 0) return []; @@ -105,7 +107,6 @@ 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); } @@ -119,7 +120,8 @@ export function filterFilesForChapter( const filteredText = filterHunksInFile(segment.text, oldStarts); if (filteredText === null) continue; - const diff = getSingularPatch(filteredText); + const baseDiff = getSingularPatch(filteredText); + const diff = enrichFileDiff(baseDiff, fileContents); result.push({ file: fileDiffToPullRequestFile(diff), diff }); } diff --git a/packages/web/src/lib/parse-diff.ts b/packages/web/src/lib/parse-diff.ts index 22e155d..b15124d 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 { FileContentsMap } from "@stage-cli/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,36 @@ export interface FileDiffEntry { diff: FileDiffMetadata; } -export function useFileDiffEntries(patch: string | undefined): FileDiffEntry[] { +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, + ...(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..b399e7f 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 "@stage-cli/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..fef61b4 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"; @@ -38,7 +39,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 +52,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 +92,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( diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 65c2af5..70b60af 100644 --- a/packages/web/src/routes/files-page.tsx +++ b/packages/web/src/routes/files-page.tsx @@ -23,9 +23,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]); @@ -77,7 +77,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 ( { if (totalFileCount === 0) return 0; @@ -111,7 +111,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); })(); From b3e73c1bb4136ac973d7d4c1587a20d0c1e32347 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 14:56:20 -0700 Subject: [PATCH 02/12] fix: reindex hunk line-array offsets when enriching with full file content Pierre's hunk metadata has deletionLineIndex/additionLineIndex pointing into the partial (patch-only) line arrays. When switching to full-file arrays (isPartial=false), remap these indices to the actual file line positions derived from the hunk's deletionStart/additionStart headers. --- packages/web/src/lib/parse-diff.ts | 33 +++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/web/src/lib/parse-diff.ts b/packages/web/src/lib/parse-diff.ts index b15124d..1a83211 100644 --- a/packages/web/src/lib/parse-diff.ts +++ b/packages/web/src/lib/parse-diff.ts @@ -1,4 +1,4 @@ -import { type FileDiffMetadata, parsePatchFiles } from "@pierre/diffs"; +import { type FileDiffMetadata, type Hunk, parsePatchFiles } from "@pierre/diffs"; import type { FileContentsMap } from "@stage-cli/types/diff"; import { useMemo } from "react"; import { FILE_STATUS, type FileStatus, type PullRequestFile } from "./diff-types"; @@ -56,6 +56,36 @@ export interface FileDiffEntry { diff: FileDiffMetadata; } +/** + * 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, @@ -71,6 +101,7 @@ export function enrichFileDiff( return { ...diff, isPartial: false, + hunks: diff.hunks.map(reindexHunk), ...(oldLines ? { deletionLines: oldLines } : {}), ...(newLines ? { additionLines: newLines } : {}), }; From 89f63dca9f2ef43c1488d986ceca2ff2c7c3b4fb Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 18:07:42 -0700 Subject: [PATCH 03/12] feat: add collapse/expand all files button to sticky nav bar Add a button in the sticky tab navigation that toggles collapse/expand for all file diffs. Visible on both the Files Changed and Chapter Detail pages via a context that child pages register their collapse state into. --- .../web/src/lib/collapse-actions-context.tsx | 32 ++++++++++++++ .../web/src/routes/chapter-detail-page.tsx | 2 + packages/web/src/routes/files-page.tsx | 2 + .../web/src/routes/pull-request-layout.tsx | 42 +++++++++++++++++-- 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 packages/web/src/lib/collapse-actions-context.tsx 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..f558c85 --- /dev/null +++ b/packages/web/src/lib/collapse-actions-context.tsx @@ -0,0 +1,32 @@ +import { createContext, type ReactNode, use, useEffect, useMemo, useState } from "react"; +import type { CollapseState } from "@/components/files"; + +interface CollapseActionsContextValue { + collapseState: CollapseState | null; + setCollapseState: (state: CollapseState | null) => void; +} + +const CollapseActionsContext = createContext(null); + +export function CollapseActionsProvider({ children }: { children: ReactNode }) { + const [collapseState, setCollapseState] = useState(null); + + const value = useMemo(() => ({ collapseState, setCollapseState }), [collapseState]); + + return {children}; +} + +export function useCollapseActionsFromNav(): CollapseState | null { + const ctx = use(CollapseActionsContext); + return ctx?.collapseState ?? null; +} + +export function useProvideCollapseActions(collapseState: CollapseState): void { + const ctx = use(CollapseActionsContext); + const setter = ctx?.setCollapseState; + + useEffect(() => { + setter?.(collapseState); + return () => setter?.(null); + }, [setter, collapseState]); +} diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index fef61b4..cb9beb1 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -13,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"; @@ -184,6 +185,7 @@ function ChapterDetailContent({ chapterFilePaths, collapseResetKey, ); + useProvideCollapseActions(collapseState); useFileNavigationKeys(chapterFiles, activeFilePath, handleSelectFile); diff --git a/packages/web/src/routes/files-page.tsx b/packages/web/src/routes/files-page.tsx index 70b60af..0b63ef9 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"; @@ -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); const diffListRef = useRef(null); const { activeFilePath, setActiveFileManually } = useActiveFileOnScroll(files); diff --git a/packages/web/src/routes/pull-request-layout.tsx b/packages/web/src/routes/pull-request-layout.tsx index 1a6d56b..5eb56ba 100644 --- a/packages/web/src/routes/pull-request-layout.tsx +++ b/packages/web/src/routes/pull-request-layout.tsx @@ -1,5 +1,5 @@ import { Link, Outlet, useRouterState } from "@tanstack/react-router"; -import { BookOpen, FileText, Settings2 } from "lucide-react"; +import { BookOpen, FileText, FoldVertical, Settings2, UnfoldVertical } from "lucide-react"; import { type CSSProperties, useEffect, useMemo, useRef, useState } from "react"; import { DiffSettingsForm } from "@/components/diff/diff-settings-form"; import { SectionLabel } from "@/components/pull-request/section-label"; @@ -7,6 +7,7 @@ import { Button } from "@/components/ui/button"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { ChapterProvider } from "@/lib/chapter-context"; +import { CollapseActionsProvider, useCollapseActionsFromNav } from "@/lib/collapse-actions-context"; import { useFileDiffEntries } from "@/lib/parse-diff"; import { useChapters } from "@/lib/use-chapters"; import { useDiffPatch } from "@/lib/use-diff-patch"; @@ -58,6 +59,36 @@ function TabLink({ tab, runId, isActive, countLabel }: TabLinkProps) { ); } +function CollapseExpandAllButton() { + const collapseState = useCollapseActionsFromNav(); + if (!collapseState) return null; + + const allCollapsed = collapseState.collapsedFiles.size > 0; + const handleClick = allCollapsed ? collapseState.expandAllFiles : collapseState.collapseAllFiles; + const label = allCollapsed ? "Expand all files" : "Collapse all files"; + + return ( + + + + + {label} + + ); +} + function ErrorState({ error }: { error: unknown }) { return (
@@ -165,6 +196,7 @@ export function PullRequestLayout({ runId }: { runId: string }) { ))}
+ @@ -188,9 +220,11 @@ export function PullRequestLayout({ runId }: { runId: string }) {
- - - + + + + + ); From 6d63bc731346b646b9307e8b980051ff8c542150 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 18:25:42 -0700 Subject: [PATCH 04/12] fix: hoist CollapseActionsProvider above nav so button renders The provider must wrap both the nav (where the button reads context) and the Outlet (where pages register their collapse state). --- .../web/src/routes/pull-request-layout.tsx | 112 +++++++++--------- 1 file changed, 57 insertions(+), 55 deletions(-) diff --git a/packages/web/src/routes/pull-request-layout.tsx b/packages/web/src/routes/pull-request-layout.tsx index 5eb56ba..b99c59e 100644 --- a/packages/web/src/routes/pull-request-layout.tsx +++ b/packages/web/src/routes/pull-request-layout.tsx @@ -168,64 +168,66 @@ export function PullRequestLayout({ runId }: { runId: string }) { } as CSSProperties; return ( -
-
-
- Run -

{data?.run.id ?? runId}

-
- - + +
+
+
+ Run +

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

+
+ - +
-
+ ); } From c537bc4e2eb041d3b513a2c8de7a63edc9e567a7 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 18:45:42 -0700 Subject: [PATCH 05/12] =?UTF-8?q?feat:=20match=20hosted=20stage=20nav=20st?= =?UTF-8?q?yling=20=E2=80=94=20line=20counts,=20opacity,=20responsive=20te?= =?UTF-8?q?xt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove backdrop-blur and opacity from sticky nav (use solid bg-background) - Add @container to layout wrapper for container query breakpoints - Add total +additions/-deletions display (visible at @5xl) - Collapse/expand and Display buttons show text labels at @7xl - Right action area uses responsive gap (@xl:gap-6) --- .../web/src/routes/pull-request-layout.tsx | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/packages/web/src/routes/pull-request-layout.tsx b/packages/web/src/routes/pull-request-layout.tsx index b99c59e..2efe045 100644 --- a/packages/web/src/routes/pull-request-layout.tsx +++ b/packages/web/src/routes/pull-request-layout.tsx @@ -59,11 +59,11 @@ function TabLink({ tab, runId, isActive, countLabel }: TabLinkProps) { ); } -function CollapseExpandAllButton() { +function CollapseExpandAllButton({ fileCount }: { fileCount: number }) { const collapseState = useCollapseActionsFromNav(); if (!collapseState) return null; - const allCollapsed = collapseState.collapsedFiles.size > 0; + const allCollapsed = fileCount > 0 && collapseState.collapsedFiles.size >= fileCount; const handleClick = allCollapsed ? collapseState.expandAllFiles : collapseState.collapseAllFiles; const label = allCollapsed ? "Expand all files" : "Collapse all files"; @@ -82,6 +82,9 @@ function CollapseExpandAllButton() { ) : ( )} + + {allCollapsed ? "Expand all" : "Collapse all"} + {label} @@ -159,6 +162,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. @@ -169,7 +182,7 @@ export function PullRequestLayout({ runId }: { runId: string }) { return ( -
+
Run @@ -179,7 +192,7 @@ export function PullRequestLayout({ runId }: { runId: string }) {
From 84b0fa7ba0d47492d0255316e64fc738c5e544c0 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 20:25:16 -0700 Subject: [PATCH 10/12] fix: don't enrich chapter-filtered diffs with full file content Pierre requires all hunks to be present when isPartial=false (trailing context must match on both sides). Chapter pages show a subset of hunks, so enrichment is only correct on the Files page where the full per-file diff is used. --- .../web/src/lib/filter-files-for-chapter.ts | 7 ++----- .../web/src/routes/chapter-detail-page.tsx | 20 ++++--------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/packages/web/src/lib/filter-files-for-chapter.ts b/packages/web/src/lib/filter-files-for-chapter.ts index 7dd592c..8ffc28c 100644 --- a/packages/web/src/lib/filter-files-for-chapter.ts +++ b/packages/web/src/lib/filter-files-for-chapter.ts @@ -1,8 +1,7 @@ import { getSingularPatch } from "@pierre/diffs"; import type { HunkReference } from "@stagereview/types/chapters"; -import type { FileContentsMap } from "@stagereview/types/diff"; import type { FileDiffEntry } from "./parse-diff"; -import { enrichFileDiff, fileDiffToPullRequestFile } from "./parse-diff"; +import { fileDiffToPullRequestFile } from "./parse-diff"; const HUNK_HEADER_RE = /^@@\s+-(\d+)(?:,\d+)?\s+\+\d+(?:,\d+)?\s+@@/; const FILE_BREAK = /\ndiff --git /g; @@ -89,7 +88,6 @@ function filterHunksInFile(perFileText: string, oldStarts: ReadonlySet): export function filterFilesForChapter( patch: string, hunkRefs: readonly HunkReference[], - fileContents?: FileContentsMap, ): FileDiffEntry[] { if (hunkRefs.length === 0) return []; @@ -120,8 +118,7 @@ export function filterFilesForChapter( const filteredText = filterHunksInFile(segment.text, oldStarts); if (filteredText === null) continue; - const baseDiff = getSingularPatch(filteredText); - const diff = enrichFileDiff(baseDiff, fileContents); + const diff = getSingularPatch(filteredText); result.push({ file: fileDiffToPullRequestFile(diff), diff }); } diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index d5cc782..646bf9c 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -1,5 +1,4 @@ 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"; @@ -58,12 +57,7 @@ export function ChapterDetailPage({ runId, chapterNumber }: ChapterDetailPagePro } return ( - + ); } @@ -71,15 +65,9 @@ interface ChapterDetailContentProps { chapter: Chapter; chapterIndex: number; patch: string; - fileContents: FileContentsMap; } -function ChapterDetailContent({ - chapter, - chapterIndex, - patch, - fileContents, -}: ChapterDetailContentProps) { +function ChapterDetailContent({ chapter, chapterIndex, patch }: ChapterDetailContentProps) { const { runId, chapters: allChapters } = useChapterContext(); const view = useViewState(runId); const [focusedKeyChangeId, setFocusedKeyChangeId] = useState(null); @@ -93,8 +81,8 @@ function ChapterDetailContent({ } const chapterEntries = useMemo( - () => filterFilesForChapter(patch, chapter.hunkRefs, fileContents), - [patch, chapter.hunkRefs, fileContents], + () => filterFilesForChapter(patch, chapter.hunkRefs), + [patch, chapter.hunkRefs], ); const allLineRefsByFile = useMemo( From 47d9996959f84def93d3079a7ebdcbdc99af82a1 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 20:46:37 -0700 Subject: [PATCH 11/12] feat: enable context expansion on chapter pages via intermediate file Port the hosted stage's approach: apply non-chapter hunks to the old file content to produce an intermediate, then diff intermediate vs new using parseDiffFromFile. This produces a clean isPartial=false diff showing only the chapter's changes while supporting full context expansion. --- .../web/src/lib/filter-files-for-chapter.ts | 124 +++++++++++++----- .../web/src/routes/chapter-detail-page.tsx | 20 ++- 2 files changed, 107 insertions(+), 37 deletions(-) diff --git a/packages/web/src/lib/filter-files-for-chapter.ts b/packages/web/src/lib/filter-files-for-chapter.ts index 8ffc28c..aa52e30 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,84 @@ 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; + 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] }; + 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, lines: [] }; continue; } if (current) current.lines.push(line); - else headerLines.push(line); } if (current) hunks.push(current); - const matched = hunks.filter((h) => oldStarts.has(h.oldStart)); - if (matched.length === 0) return null; + return hunks; +} + +/** + * 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 [...headerLines, ...matched.flatMap((h) => h.lines)].join("\n"); + 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 []; @@ -111,15 +142,42 @@ export function filterFilesForChapter( } 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) => { + const header = `@@ -${h.oldStart},${h.oldLines} +0,0 @@`; + return [header, ...h.lines]; + }), + ].join("\n"); + const diff = getSingularPatch(filteredText); + result.push({ file: fileDiffToPullRequestFile(diff), diff }); + } } return result; diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index 646bf9c..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"; @@ -57,7 +58,12 @@ export function ChapterDetailPage({ runId, chapterNumber }: ChapterDetailPagePro } return ( - + ); } @@ -65,9 +71,15 @@ 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( From c54519edf117ae077798119b154556237fd7c687 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 21:04:17 -0700 Subject: [PATCH 12/12] fix: preserve original hunk headers and strip trailing empty lines - Use original @@ header line in fallback path instead of reconstructing with hardcoded +0,0 - Strip trailing empty strings from parsed hunk lines (split artifact) --- .../web/src/lib/filter-files-for-chapter.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/web/src/lib/filter-files-for-chapter.ts b/packages/web/src/lib/filter-files-for-chapter.ts index aa52e30..78717e7 100644 --- a/packages/web/src/lib/filter-files-for-chapter.ts +++ b/packages/web/src/lib/filter-files-for-chapter.ts @@ -44,6 +44,7 @@ function parseFileNames(segment: string): { prevName?: string; name?: string } { interface ParsedHunk { oldStart: number; oldLines: number; + header: string; lines: string[]; } @@ -55,15 +56,21 @@ function parseHunksFromSegment(segmentText: string): ParsedHunk[] { for (const line of lines) { const match = line.match(HUNK_HEADER_RE); if (match) { - if (current) hunks.push(current); + 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, lines: [] }; + current = { oldStart, oldLines, header: line, lines: [] }; continue; } if (current) current.lines.push(line); } - if (current) hunks.push(current); + if (current) { + while (current.lines.at(-1) === "") current.lines.pop(); + hunks.push(current); + } return hunks; } @@ -170,10 +177,7 @@ export function filterFilesForChapter( } const filteredText = [ ...headerLines, - ...chapterHunks.flatMap((h) => { - const header = `@@ -${h.oldStart},${h.oldLines} +0,0 @@`; - return [header, ...h.lines]; - }), + ...chapterHunks.flatMap((h) => [h.header, ...h.lines]), ].join("\n"); const diff = getSingularPatch(filteredText); result.push({ file: fileDiffToPullRequestFile(diff), diff });