diff --git a/.agents/skills/verify-mcode/references/features/turn-diff-review.md b/.agents/skills/verify-mcode/references/features/turn-diff-review.md index 3b09df3c4..417619211 100644 --- a/.agents/skills/verify-mcode/references/features/turn-diff-review.md +++ b/.agents/skills/verify-mcode/references/features/turn-diff-review.md @@ -37,3 +37,6 @@ - Missing provider access, a stale runtime, or unavailable desktop control is a coverage gap. Focused tests do not replace the Composer and Review proof. - The focused real Git/RPC test covers same-file isolation, settled service recreation, cumulative edits, and legacy fallback. The startup migration test covers an existing database. Neither proves rendered labels or an actual provider turn. - An unsupported or over-limit patch is rejected whole. Do not expect a partial native patch. +- The rendered diff surface is `@pierre/diffs` CodeView. Lines and headers mount inside `diffs-container` shadow roots; use Playwright locators (they pierce shadow DOM), not `element.textContent` scraping of the light DOM. +- Long patches collapse unchanged context behind "N unmodified lines" bands. Click a band and confirm the hidden lines render. +- Line comments open from the gutter "+" affordance on hover and land in the thread annotation bundle. Exercise both the deletions (left) and additions (right) sides. diff --git a/apps/server/src/features/projects/diffs/snapshots/__tests__/snapshot-service.test.ts b/apps/server/src/features/projects/diffs/snapshots/__tests__/snapshot-service.test.ts index d8bd83057..5a4b09aaa 100644 --- a/apps/server/src/features/projects/diffs/snapshots/__tests__/snapshot-service.test.ts +++ b/apps/server/src/features/projects/diffs/snapshots/__tests__/snapshot-service.test.ts @@ -135,18 +135,20 @@ describe("SnapshotService ref and numstat validation", () => { it("returns destination paths for simple and brace-form numstat renames", async () => { const executor: GitExecutor = { - async exec() { - return { - stdout: "0\t0\told.txt => new.txt\n1\t2\tarch/{i386 => x86}/Makefile\n", - stderr: "", - }; + async exec(args) { + return args.includes("--name-status") + ? { stdout: "R100\told.txt\tnew.txt\nM\tarch/x86/Makefile\n", stderr: "" } + : { + stdout: "0\t0\told.txt => new.txt\n1\t2\tarch/{i386 => x86}/Makefile\n", + stderr: "", + }; }, }; const service = new SnapshotService(executor); await expect(service.getDiffStats("/repo", "before", "after")).resolves.toEqual([ - { filePath: "new.txt", additions: 0, deletions: 0 }, - { filePath: "arch/x86/Makefile", additions: 1, deletions: 2 }, + { filePath: "new.txt", additions: 0, deletions: 0, changeType: "renamed" }, + { filePath: "arch/x86/Makefile", additions: 1, deletions: 2, changeType: "modified" }, ]); }); }); diff --git a/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts b/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts index 1ed5ca5ee..a85d46388 100644 --- a/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts +++ b/apps/server/src/features/projects/diffs/snapshots/snapshot-service.ts @@ -8,6 +8,7 @@ import { injectable, inject } from "tsyringe"; import * as NodeFSPromises from "node:fs/promises"; import * as NodePath from "node:path"; import * as NodeCrypto from "node:crypto"; +import type { DiffStats } from "@mcode/contracts"; import type { GitExecutor } from "../../git/execution/index.js"; import { RealGitExecutor } from "../../git/execution/real-git-executor.js"; @@ -82,7 +83,7 @@ function selectedPathGroups( return pathGroups.filter((group) => group.some((path) => connected.has(normalize(path)))); } -function parseNumstat(stdout: string): { filePath: string; additions: number; deletions: number }[] { +function parseNumstat(stdout: string): Omit[] { return stdout .trim() .split("\n") @@ -97,6 +98,28 @@ function parseNumstat(stdout: string): { filePath: string; additions: number; de }); } +/** Map a --name-status letter (A/M/D/R/C/T/U…) to the shared change type. */ +function nameStatusChangeType(letter: string): DiffStats["changeType"] { + if (letter === "A") return "added"; + if (letter === "D") return "deleted"; + if (letter === "R") return "renamed"; + if (letter === "C") return "copied"; + return "modified"; +} + +function parseNameStatus(stdout: string): Map { + const types = new Map(); + for (const line of stdout.trim().split("\n")) { + if (!line.includes("\t")) continue; + const [status, ...paths] = line.split("\t"); + // Rename/copy rows carry "old\tnew"; the destination is the diff file path. + const filePath = paths[paths.length - 1]; + if (!filePath) continue; + types.set(filePath, nameStatusChangeType(status?.[0] ?? "M")); + } + return types; +} + function numstatDestinationPath(path: string): string { const braceRename = path.match(/^(.*)\{([^{}]*) => ([^{}]*)\}(.*)$/); if (braceRename) { @@ -124,12 +147,12 @@ function getDiffPathspecBatches( function gitDiffArgs( cwd: string, - format: "unified" | "numstat", + format: "unified" | "numstat" | "name-status", refBefore: string, refAfter: string, pathspecs: string[] | undefined, ): string[] { - const formatArgs = format === "unified" ? [] : ["--numstat"]; + const formatArgs = format === "unified" ? [] : [`--${format}`]; const args = ["-C", cwd, "diff", ...formatArgs, "--find-renames", refBefore, refAfter]; if (pathspecs) args.push("--", ...pathspecs); return args; @@ -138,7 +161,7 @@ function gitDiffArgs( async function executeDiffBatches( gitExecutor: GitExecutor, cwd: string, - format: "unified" | "numstat", + format: "unified" | "numstat" | "name-status", refBefore: string, refAfter: string, pathspecBatches: (string[] | undefined)[], @@ -159,10 +182,24 @@ function limitDiffLines(diff: string, maxLines: number | undefined): string { return maxLines ? diff.split("\n").slice(0, maxLines).join("\n") : diff; } -function collectDiffStats(outputs: readonly string[]): { filePath: string; additions: number; deletions: number }[] { - const stats = new Map(); +function collectDiffStats( + outputs: readonly string[], + statusOutputs: readonly string[], +): DiffStats[] { + const changeTypes = new Map(); + for (const output of statusOutputs) { + for (const [filePath, changeType] of parseNameStatus(output)) { + changeTypes.set(filePath, changeType); + } + } + const stats = new Map(); for (const output of outputs) { - for (const entry of parseNumstat(output)) stats.set(entry.filePath, entry); + for (const entry of parseNumstat(output)) { + stats.set(entry.filePath, { + ...entry, + changeType: changeTypes.get(entry.filePath) ?? "modified", + }); + } } return [...stats.values()]; } @@ -320,28 +357,24 @@ export class SnapshotService { } } - /** Get per-file line addition/deletion counts between two refs (tree or commit SHAs). */ + /** Get per-file change classification and line counts between two refs (tree or commit SHAs). */ async getDiffStats( cwd: string, refBefore: string, refAfter: string, allowedPaths?: readonly string[], allowedPathGroups?: readonly (readonly string[])[], - ): Promise<{ filePath: string; additions: number; deletions: number }[]> { + ): Promise { if (hasUnsafeDiffRefs(refBefore, refAfter) || refBefore === refAfter) return []; const pathspecBatches = getDiffPathspecBatches(undefined, allowedPaths, allowedPathGroups); if (pathspecBatches.length === 0) return []; try { - const outputs = await executeDiffBatches( - this.gitExecutor, - cwd, - "numstat", - refBefore, - refAfter, - pathspecBatches, - ); - return collectDiffStats(outputs); + const [outputs, statusOutputs] = await Promise.all([ + executeDiffBatches(this.gitExecutor, cwd, "numstat", refBefore, refAfter, pathspecBatches), + executeDiffBatches(this.gitExecutor, cwd, "name-status", refBefore, refAfter, pathspecBatches), + ]); + return collectDiffStats(outputs, statusOutputs); } catch { return []; } diff --git a/apps/server/src/features/projects/git/git-comparison-service.ts b/apps/server/src/features/projects/git/git-comparison-service.ts index 0355d92f0..be4d4290c 100644 --- a/apps/server/src/features/projects/git/git-comparison-service.ts +++ b/apps/server/src/features/projects/git/git-comparison-service.ts @@ -145,6 +145,27 @@ export class GitComparisonService { } } + /** + * Read a file's contents at a revision. `ref` follows `git show` revision + * rules with two extensions: "" reads the staged index blob (`:path`), and + * "A...B" resolves to the merge base of A and B, matching the old side of a + * three-dot comparison diff. + */ + async readFileAtRef( + workspaceId: string, + ref: string, + filePath: string, + repoPath?: string, + ): Promise { + const cwd = repoPath ?? this.requireWorkspace(workspaceId).path; + const resolved = await this.resolveShowRef(cwd, ref); + const { stdout } = await this.gitExecutor.exec( + ["-C", cwd, "show", `${resolved}:${filePath}`], + { timeout: 10_000 }, + ); + return stdout; + } + /** List files changed on the target side of a branch comparison. */ async listBranchComparisonChangedFiles(workspaceId: string, base?: string, target?: string, repoPath?: string): Promise { const cwd = repoPath ?? this.requireWorkspace(workspaceId).path; @@ -449,6 +470,25 @@ export class GitComparisonService { return this.gitRepository.getDefaultBranchAt(repoPath); } + /** Map a `readFileAtRef` revision onto the commit git show should read. */ + private async resolveShowRef(cwd: string, ref: string): Promise { + if (ref === "") return ""; + const mergeBaseIndex = ref.indexOf("..."); + if (mergeBaseIndex >= 0) { + const base = ref.slice(0, mergeBaseIndex); + const target = ref.slice(mergeBaseIndex + 3); + assertSafeRef(base); + assertSafeRef(target || "HEAD"); + const { stdout } = await this.gitExecutor.exec( + ["-C", cwd, "merge-base", base, target || "HEAD"], + { timeout: 10_000 }, + ); + return stdout.trim(); + } + assertShowRef(ref); + return ref; + } + private requireWorkspace(workspaceId: string) { const workspace = this.workspaceRepo.findById(workspaceId); if (!workspace) throw new Error(`Workspace not found: ${workspaceId}`); @@ -734,6 +774,11 @@ function assertSafeRef(ref: string): void { if (!/^(?!-)[A-Za-z0-9._/-]+$/.test(ref)) throw new Error(`Unsafe git ref: ${ref}`); } +/** Refs passed to `git show` may carry revision suffixes like `sha~1`. */ +function assertShowRef(ref: string): void { + if (!/^(?!-)[A-Za-z0-9._/-]+(?:[~^][0-9]*)*$/.test(ref)) throw new Error(`Unsafe git ref: ${ref}`); +} + function assertSafeSha(sha: string | undefined): asserts sha is string { if (!sha || !/^[0-9a-fA-F]{4,40}$/.test(sha)) throw new Error(`Invalid or missing git SHA for commit view: ${sha}`); } diff --git a/apps/server/src/features/projects/git/transport/git-rpc.ts b/apps/server/src/features/projects/git/transport/git-rpc.ts index 9cb1d191b..cce3dc364 100644 --- a/apps/server/src/features/projects/git/transport/git-rpc.ts +++ b/apps/server/src/features/projects/git/transport/git-rpc.ts @@ -34,6 +34,7 @@ export interface GitRouterDeps { | "listCommitChangedFiles" | "listWorkingTreeChangedFiles" | "readWorkingTreeDiff" + | "readFileAtRef" | "listBranchComparisonChangedFiles" | "readBranchComparisonDiff" | "resolveBranchComparison" @@ -136,6 +137,19 @@ const gitHandlers: GitHandlerMap = { resolveThreadRepoPath(deps, params.threadId), ) : "", + // Hydration needs the real old/new contents; a soft "" here would let the + // diff renderer merge inconsistent metadata, so failures must propagate. + "git.fileAtRef": (deps, params) => { + if (!isGitWorkspace(deps, params.workspaceId)) { + throw new Error(`Workspace ${params.workspaceId} is not a git repository`); + } + return deps.gitComparison.readFileAtRef( + params.workspaceId, + params.ref, + params.filePath, + resolveThreadRepoPath(deps, params.threadId), + ); + }, "git.branchFiles": (deps, params) => isGitWorkspace(deps, params.workspaceId) ? deps.gitComparison.listBranchComparisonChangedFiles( diff --git a/apps/web/package.json b/apps/web/package.json index 39e90a6be..1bfd79e89 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -24,6 +24,7 @@ "@mcode/contracts": "workspace:*", "@mcode/shared": "workspace:*", "@phosphor-icons/react": "^2.1.10", + "@pierre/diffs": "1.4.1", "@radix-ui/react-collapsible": "^1.1.12", "@radix-ui/react-dropdown-menu": "^2.1.16", "@radix-ui/react-popover": "^1.1.15", diff --git a/apps/web/src/__tests__/TurnTimeline.expand.test.tsx b/apps/web/src/__tests__/TurnTimeline.expand.test.tsx index 2598b243c..fd3133526 100644 --- a/apps/web/src/__tests__/TurnTimeline.expand.test.tsx +++ b/apps/web/src/__tests__/TurnTimeline.expand.test.tsx @@ -5,7 +5,7 @@ import type { TurnSnapshot } from "@mcode/contracts"; import { TurnTimeline } from "../components/diff/TurnTimeline"; import { useDiffStore } from "../stores/diffStore"; -// Mock the transport so FileEntry's lazy diff load resolves immediately. +// Mock the transport so lazy diff loads resolve immediately. vi.mock("@/transport", () => ({ getTransport: () => ({ getSnapshotDiff: vi.fn().mockResolvedValue("@@ -0,0 +1 @@\n+hello\n"), @@ -14,15 +14,12 @@ vi.mock("@/transport", () => ({ }), })); -// Mock heavy diff renderers to keep tests fast and avoid unrelated failures. -vi.mock("../components/diff/UnifiedDiff", () => ({ - UnifiedDiff: ({ lines }: { lines: unknown[] }) => ( -
{lines.length} lines
+// Mock the pierre-backed diff surface to keep tests fast and avoid unrelated failures. +vi.mock("../components/diff/ReviewDiffView", () => ({ + ReviewDiffView: ({ files }: { files: { path: string }[] }) => ( +
{files.length} files
), })); -vi.mock("../components/diff/SideBySideDiff", () => ({ - SideBySideDiff: () =>
, -})); vi.mock("../components/diff/DiffPreview", () => ({ DiffPreview: () =>
, })); diff --git a/apps/web/src/__tests__/mocks/transport.ts b/apps/web/src/__tests__/mocks/transport.ts index ff1b58a9b..648ffa53d 100644 --- a/apps/web/src/__tests__/mocks/transport.ts +++ b/apps/web/src/__tests__/mocks/transport.ts @@ -236,6 +236,7 @@ export const mockTransport: McodeTransport = { refreshWorkspaceFiles: vi.fn().mockResolvedValue(undefined), getReviewComparison: vi.fn().mockResolvedValue({ files: [], additions: 0, deletions: 0 }), readFileContent: vi.fn().mockResolvedValue(""), + readFileAtRef: vi.fn().mockResolvedValue(""), listOpenInApps: vi.fn().mockResolvedValue([]), openIn: vi.fn().mockResolvedValue(undefined), getBranchPr: vi.fn().mockResolvedValue(null), diff --git a/apps/web/src/components/chat/DiffViewer.tsx b/apps/web/src/components/chat/DiffViewer.tsx index 6574206a3..032f9959a 100644 --- a/apps/web/src/components/chat/DiffViewer.tsx +++ b/apps/web/src/components/chat/DiffViewer.tsx @@ -1,7 +1,8 @@ -import { useState, useCallback, useMemo } from "react"; +import { useState, useCallback } from "react"; import { ChevronRight, FileText } from "lucide-react"; +import { PatchDiff } from "@pierre/diffs/react"; import { getTransport } from "@/transport"; -import { parseDiffLines } from "@/lib/diff-parser"; +import { useShikiTheme } from "@/hooks/useTheme"; /** Number of lines to request on the initial (truncated) fetch. */ const MAX_LINES = 500; @@ -13,38 +14,16 @@ interface DiffViewerProps { changeType?: "created" | "deleted" | "renamed" | "modified" | "binary"; } -function DiffViewerHeader({ expanded, loading, filePath, changeLabel, onToggle }: { expanded: boolean; loading: boolean; filePath: string; changeLabel: string; onToggle: () => void }) { - return ; -} - -function DiffLines({ lines, visibleLines, truncated, onShowAll }: { lines: ReturnType; visibleLines: ReturnType; truncated: boolean; onShowAll: () => void }) { - return
- {visibleLines.map((line, index) =>
- {line.type === "add" ? "+" : line.type === "remove" ? "-" : " "} - {line.content} -
)} - {truncated && } -
; -} - -function DiffViewerContent({ expanded, diff, changeType, lines, visibleLines, truncated, onShowAll }: { expanded: boolean; diff: string | null; changeType: NonNullable; lines: ReturnType; visibleLines: ReturnType; truncated: boolean; onShowAll: () => void }) { - if (!expanded) return null; - if (changeType === "binary") return
Binary file changed. No diff available.
; - return diff === null ? null : ; -} - -/** Inline unified diff renderer. Lazy-loads diff content on expand. */ +/** + * Inline diff for one file inside the transcript. Lazy-loads the patch on + * expand and renders it through pierre's PatchDiff. + */ export function DiffViewer({ snapshotId, filePath, changeType = "modified" }: DiffViewerProps) { const [expanded, setExpanded] = useState(false); const [diff, setDiff] = useState(null); const [loading, setLoading] = useState(false); - const [showAll, setShowAll] = useState(false); + const [truncated, setTruncated] = useState(false); + const shikiTheme = useShikiTheme(); const handleToggle = useCallback(async () => { if (!expanded && diff === null) { @@ -52,8 +31,9 @@ export function DiffViewer({ snapshotId, filePath, changeType = "modified" }: Di try { const result = await getTransport().getSnapshotDiff(snapshotId, filePath, MAX_LINES); setDiff(result); + setTruncated(result.split("\n").length > MAX_LINES); } catch { - setDiff("Failed to load diff"); + setDiff(""); } finally { setLoading(false); } @@ -66,16 +46,12 @@ export function DiffViewer({ snapshotId, filePath, changeType = "modified" }: Di try { const fullDiff = await getTransport().getSnapshotDiff(snapshotId, filePath); setDiff(fullDiff); - setShowAll(true); + setTruncated(false); } catch { // Keep existing truncated diff on error } }, [snapshotId, filePath]); - const lines = useMemo(() => (diff ? parseDiffLines(diff) : []), [diff]); - const truncated = !showAll && lines.length > MAX_LINES; - const visibleLines = truncated ? lines.slice(0, MAX_LINES) : lines; - const changeLabel = { created: "File created", deleted: "File deleted", @@ -85,9 +61,72 @@ export function DiffViewer({ snapshotId, filePath, changeType = "modified" }: Di }[changeType]; return ( -
- - +
+ + {expanded ? ( + + ) : null} +
+ ); +} + +/** Expanded body: binary notice, patch render, or the show-all affordance. */ +function DiffBody({ + binary, + diff, + truncated, + shikiTheme, + onShowAll, +}: { + readonly binary: boolean; + readonly diff: string | null; + readonly truncated: boolean; + readonly shikiTheme: string; + readonly onShowAll: () => void; +}) { + if (binary) { + return ( +
+ Binary file changed. No diff available. +
+ ); + } + if (diff === null) return null; + return ( +
+ {diff ? ( + + ) : ( +

No diff content

+ )} + {truncated ? ( + + ) : null}
); } diff --git a/apps/web/src/components/chat/HeaderActions.test.tsx b/apps/web/src/components/chat/HeaderActions.test.tsx index b4c13cd59..ee0de270f 100644 --- a/apps/web/src/components/chat/HeaderActions.test.tsx +++ b/apps/web/src/components/chat/HeaderActions.test.tsx @@ -1044,7 +1044,7 @@ describe("resolveThreadOverviewChangeSummary", () => { it("uses the latest turn snapshot before git views", async () => { const transport = makeSummaryTransport({ getSnapshotDiffStats: vi.fn().mockResolvedValue([ - { filePath: "src/latest.ts", additions: 8, deletions: 2 }, + { filePath: "src/latest.ts", additions: 8, deletions: 2, changeType: "modified" }, ]), getWorkingTreeFiles: vi.fn().mockResolvedValue(["src/manual.ts"]), }); diff --git a/apps/web/src/components/chat/TurnChangeSummary.tsx b/apps/web/src/components/chat/TurnChangeSummary.tsx index d84b9d91f..f2f24bdc8 100644 --- a/apps/web/src/components/chat/TurnChangeSummary.tsx +++ b/apps/web/src/components/chat/TurnChangeSummary.tsx @@ -1,6 +1,9 @@ -import { useState, useCallback, useEffect, useId } from "react"; -import { ChevronRight, FileText, ExternalLink } from "lucide-react"; +import { useState, useCallback, useEffect, useId, useMemo } from "react"; +import { ChevronRight, FileText, ExternalLink, Folder } from "lucide-react"; +import type { DiffStats } from "@mcode/contracts"; import { Button } from "@/components/ui/button"; +import { FileTypeIcon } from "@/components/ui/file-type-icon"; +import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { useDiffStore } from "@/stores/diffStore"; import { readThreadRecord } from "@/stores/thread-selectors"; import { useWorkspaceStore } from "@/features/projects/state/workspaceStore"; @@ -8,6 +11,9 @@ import { showRightPanelAdaptive } from "@/lib/right-panel-layout"; import { getTransport } from "@/transport"; import { diffCardSurfaceClass } from "@/components/diff/diff-surface"; import { DiffStat } from "@/components/diff/DiffStat"; +import { CHANGE_TYPE_GLYPHS, CHANGE_TYPE_LABELS, changeTypeTone } from "@/components/diff/change-type"; +import { cn } from "@/lib/utils"; +import { basename } from "@/lib/path"; /** Props for TurnChangeSummary. */ interface TurnChangeSummaryProps { @@ -18,36 +24,95 @@ interface TurnChangeSummaryProps { manualExpandRef?: React.RefObject>; } -/** Extract just the filename from a path for display. */ -function fileName(filePath: string): string { - const parts = filePath.split("/"); - return parts[parts.length - 1] ?? filePath; -} +/** Cap displayed files to avoid DOM bloat on massive turns. */ +const MAX_DISPLAYED_FILES = 5; -/** Extract the parent directory from a path for display. */ -function parentDir(filePath: string): string { - const parts = filePath.split("/"); - if (parts.length <= 1) return ""; - return parts.slice(0, -1).join("/"); +/** Git reports diff stats with `/` separators; snapshot file lists may carry `\`. */ +const normalizeSlashes = (p: string) => p.replace(/\\/g, "/"); + +/** One directory in the changed-file tree; single-child chains are compressed into the label. */ +interface FileTreeDir { + /** Compressed path label, e.g. "server/src" for a folder chain with no branching. */ + label: string; + dirs: FileTreeDir[]; + /** Full file paths sitting directly in this directory. */ + files: string[]; } -/** Cap displayed files to avoid DOM bloat on massive turns. */ -const MAX_DISPLAYED_FILES = 50; +/** + * Build a directory tree from changed paths. Folders that contain only a + * single subfolder and no files fold into it ("a/b/c" renders as one header), + * which keeps deep paths readable without flattening the hierarchy. + */ +function buildFileTree(paths: readonly string[]): { rootFiles: string[]; dirs: FileTreeDir[] } { + interface MutableDir { + label: string; + dirs: Map; + files: string[]; + } + const root: MutableDir = { label: "", dirs: new Map(), files: [] }; + for (const path of paths) { + let node = root; + for (const seg of path.split(/[\\/]/).slice(0, -1)) { + let next = node.dirs.get(seg); + if (!next) { + next = { label: seg, dirs: new Map(), files: [] }; + node.dirs.set(seg, next); + } + node = next; + } + node.files.push(path); + } + const finish = (node: MutableDir): FileTreeDir => { + let label = node.label; + let cur = node; + while (cur.files.length === 0 && cur.dirs.size === 1) { + const [seg, child] = [...cur.dirs.entries()][0]!; + label = `${label}/${seg}`; + cur = child; + } + const dirs = [...cur.dirs.values()].map(finish).sort((a, b) => a.label.localeCompare(b.label)); + return { label, dirs, files: cur.files }; + }; + const dirs = [...root.dirs.values()].map(finish).sort((a, b) => a.label.localeCompare(b.label)); + return { rootFiles: root.files, dirs }; +} /** * Inline banner showing files changed in an agent turn. * Collapsed: single-line bar with file count and expand chevron. - * Expanded: file list with per-file "Diff" button and a "View all diffs" button. + * Expanded: file list with type icon, change status, and per-file diff stats. */ export function TurnChangeSummary({ messageId, filesChanged, isLatestTurn, manualExpandRef }: TurnChangeSummaryProps) { // Restore manual override from ref if the virtualizer remounted this component const manualOverride = manualExpandRef?.current?.get(messageId); const contentId = useId(); const [expanded, setExpanded] = useState(manualOverride ?? isLatestTurn); - const [diffStats, setDiffStats] = useState | null>(null); + const [diffStats, setDiffStats] = useState | null>(null); const fileCount = filesChanged.length; - const displayedFiles = filesChanged.slice(0, MAX_DISPLAYED_FILES); - const hiddenCount = fileCount - displayedFiles.length; + const hiddenCount = Math.max(0, fileCount - MAX_DISPLAYED_FILES); + + // Group rows under a nested folder tree so paths read as structure, not + // noise: root files first, then directory sections sorted A-Z. + const { rootFiles, dirs } = useMemo( + () => buildFileTree(filesChanged.slice(0, MAX_DISPLAYED_FILES)), + [filesChanged], + ); + + // Aggregate stats land after the async fetch; the header shows them only + // when every displayed file resolved so partial totals never mislead. + const totals = useMemo(() => { + if (!diffStats) return null; + let additions = 0; + let deletions = 0; + for (const path of filesChanged) { + const stat = diffStats.get(normalizeSlashes(path)); + if (!stat) return null; + additions += stat.additions; + deletions += stat.deletions; + } + return { additions, deletions }; + }, [diffStats, filesChanged]); // Sync expanded state when isLatestTurn changes (auto-collapse older turns). // Prefer any stored manual override; only fall back to isLatestTurn when the @@ -76,7 +141,7 @@ export function TurnChangeSummary({ messageId, filesChanged, isLatestTurn, manua const serverMsgId = readThreadRecord(threadId).serverMessageIds[messageId] ?? messageId; - // Resolve snapshot — load from server if not cached in diffStore + // Resolve snapshot: load from server if not cached in diffStore let snapshots = useDiffStore.getState().snapshotsByThread[threadId]; if (!snapshots) { snapshots = await getTransport().listSnapshots(threadId); @@ -86,21 +151,18 @@ export function TurnChangeSummary({ messageId, filesChanged, isLatestTurn, manua if (!snapshot) return; const stats = await getTransport().getSnapshotDiffStats(snapshot.id); - const statsMap = new Map( - stats.map((s) => [s.filePath, { additions: s.additions, deletions: s.deletions }]), - ); - setDiffStats(statsMap); + setDiffStats(new Map(stats.map((s) => [normalizeSlashes(s.filePath), s]))); } catch { // Best-effort: stats are decorative, don't block the file list } })(); }, [expanded, diffStats, messageId]); - /** Open the diff panel focused on the Changes tab, scrolled to this turn's snapshot. */ - const handleViewAllDiffs = useCallback(async () => { + /** Open the diff panel focused on the Changes tab showing this turn's changes. */ + const openReviewPanel = useCallback(async () => { const { activeThreadId: threadId, activeWorkspaceId: workspaceId } = useWorkspaceStore.getState(); - if (!threadId || !workspaceId) return; + if (!threadId || !workspaceId) return null; const store = useDiffStore.getState(); showRightPanelAdaptive(workspaceId, threadId); @@ -118,79 +180,219 @@ export function TurnChangeSummary({ messageId, filesChanged, isLatestTurn, manua console.warn("[TurnChangeSummary] Failed to load snapshots:", err); } } + return threadId; }, []); + const handleViewAllDiffs = useCallback(() => { + void openReviewPanel(); + }, [openReviewPanel]); + + /** Open the Changes tab and scroll the comparison to this file. */ + const handleJumpToFile = useCallback((filePath: string) => { + void openReviewPanel().then((threadId) => { + if (threadId) useDiffStore.getState().requestReviewFileJump(threadId, filePath); + }); + }, [openReviewPanel]); + return (
{/* Header row: toggle and "View All Diffs" are siblings to avoid nested buttons */} -
- - + + + {fileCount} file{fileCount !== 1 ? "s" : ""} changed + + + + + +
- {/* File list — only rendered when expanded */} + {/* File list: only rendered when expanded */} {expanded && ( -
- {displayedFiles.map((filePath) => { - const name = fileName(filePath); - const dir = parentDir(filePath); - const stat = diffStats?.get(filePath); - return ( -
-
- {name} - {dir && ( - - {dir} - - )} -
- {stat && } -
- ); - })} - {hiddenCount > 0 && ( - - )} +
+
)}
); } + +/** Aggregate "+A | −D" shown in the header once every file's stats resolve. */ +function SummaryTotals({ totals }: { readonly totals: { additions: number; deletions: number } | null }) { + if (!totals) return null; + return ( + + {totals.additions > 0 && ( + +{totals.additions} + )} + {totals.additions > 0 && totals.deletions > 0 && ( + | + )} + {totals.deletions > 0 && ( + −{totals.deletions} + )} + + ); +} + +/** Props for the expanded, folder-grouped changed-file list. */ +interface ChangedFileListProps { + readonly rootFiles: string[]; + readonly dirs: readonly FileTreeDir[]; + readonly hiddenCount: number; + readonly diffStats: Map | null; + readonly onJump: (filePath: string) => void; + readonly onViewAll: () => void; +} + +/** Root files first, then a collapsible directory tree. */ +function ChangedFileList({ rootFiles, dirs, hiddenCount, diffStats, onJump, onViewAll }: ChangedFileListProps) { + return ( + <> + {rootFiles.length > 0 && ( +
+ {rootFiles.map((filePath) => ( + + ))} +
+ )} + {dirs.map((dir) => ( + + ))} + {hiddenCount > 0 && ( + + )} + + ); +} + +/** One collapsible folder in the changed-file tree. */ +function DirNode({ dir, diffStats, onJump }: { dir: FileTreeDir; diffStats: Map | null; onJump: (filePath: string) => void }) { + const [open, setOpen] = useState(true); + return ( +
+ + {open && ( +
+ {dir.dirs.map((child) => ( + + ))} + {dir.files.length > 0 && ( +
+ {dir.files.map((filePath) => ( + + ))} +
+ )} +
+ )} +
+ ); +} + +/** Props for one row in the changed-file list. */ +interface ChangedFileRowProps { + filePath: string; + stat: DiffStats | undefined; + onJump: (filePath: string) => void; +} + +/** One changed file: type icon, name, change glyph, and line stats. */ +function ChangedFileRow({ filePath, stat, onJump }: ChangedFileRowProps) { + const name = basename(filePath); + const changeType = stat?.changeType ?? "modified"; + + return ( + + onJump(filePath)} + aria-label={`${CHANGE_TYPE_LABELS[changeType]} ${filePath}`} + className="flex w-full items-center justify-between gap-2 px-2 py-1.5 text-xs transition-colors hover:bg-muted/40" + > + + + {name} + + + {stat && ( + + )} + + {CHANGE_TYPE_GLYPHS[changeType]} + + + + } + /> + {`${CHANGE_TYPE_LABELS[changeType]} · ${filePath}`} + + ); +} diff --git a/apps/web/src/components/chat/__tests__/TurnChangeSummary.test.tsx b/apps/web/src/components/chat/__tests__/TurnChangeSummary.test.tsx new file mode 100644 index 000000000..3f676dbfe --- /dev/null +++ b/apps/web/src/components/chat/__tests__/TurnChangeSummary.test.tsx @@ -0,0 +1,184 @@ +import { act, render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { TurnSnapshot } from "@mcode/contracts"; +import { useDiffStore } from "@/stores/diffStore"; +import { useWorkspaceStore } from "@/features/projects/state/workspaceStore"; +import { TurnChangeSummary } from "../TurnChangeSummary"; + +const transport = vi.hoisted(() => ({ + listSnapshots: vi.fn(), + getSnapshotDiffStats: vi.fn(), +})); + +vi.mock("@/transport", () => ({ + getTransport: () => transport, +})); + +vi.mock("@/stores/thread-selectors", () => ({ + readThreadRecord: () => ({ serverMessageIds: {} }), +})); + +const showRightPanelAdaptive = vi.hoisted(() => vi.fn()); +vi.mock("@/lib/right-panel-layout", () => ({ + showRightPanelAdaptive, +})); + +function snapshot(id: string, messageId: string): TurnSnapshot { + return { + id, + thread_id: "thread-1", + worktree_path: null, + message_id: messageId, + ref_before: `${id}-before`, + ref_after: `${id}-after`, + files_changed: [], + created_at: "2026-07-20T12:00:00.000Z", + }; +} + +function renderSummary( + filesChanged: string[], + overrides: { isLatestTurn?: boolean; messageId?: string; manualExpandRef?: React.RefObject> } = {}, +) { + return render( + , + ); +} + +describe("TurnChangeSummary", () => { + beforeEach(() => { + vi.clearAllMocks(); + transport.listSnapshots.mockResolvedValue([]); + transport.getSnapshotDiffStats.mockResolvedValue([]); + useWorkspaceStore.setState({ + activeThreadId: "thread-1", + activeWorkspaceId: "workspace-1", + }); + useDiffStore.setState({ + snapshotsByThread: { "thread-1": [snapshot("snap-1", "msg-1")] }, + reviewFileJumpRequest: null, + viewMode: "last-turn", + }); + }); + + it("shows per-file change glyphs and line stats from snapshot stats", async () => { + transport.getSnapshotDiffStats.mockResolvedValue([ + { filePath: "src/App.tsx", additions: 8, deletions: 2, changeType: "modified" }, + { filePath: "src/new.ts", additions: 12, deletions: 0, changeType: "added" }, + { filePath: "src/old.ts", additions: 0, deletions: 30, changeType: "deleted" }, + ]); + + renderSummary(["src/App.tsx", "src/new.ts", "src/old.ts"]); + + await waitFor(() => expect(transport.getSnapshotDiffStats).toHaveBeenCalledWith("snap-1")); + + expect(screen.getByRole("button", { name: "Modified src/App.tsx" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Added src/new.ts" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Deleted src/old.ts" })).toBeInTheDocument(); + expect(screen.getByText("+8")).toBeInTheDocument(); + expect(screen.getByText("−30")).toBeInTheDocument(); + }); + + it("opens the Changes tab and jumps to the clicked file", async () => { + transport.getSnapshotDiffStats.mockResolvedValue([ + { filePath: "src/App.tsx", additions: 8, deletions: 2, changeType: "modified" }, + ]); + const user = userEvent.setup(); + + renderSummary(["src/App.tsx"]); + + await user.click(await screen.findByRole("button", { name: "Modified src/App.tsx" })); + + await waitFor(() => { + expect(showRightPanelAdaptive).toHaveBeenCalledWith("workspace-1", "thread-1"); + }); + const jump = useDiffStore.getState().reviewFileJumpRequest; + expect(jump).toMatchObject({ scopeId: "thread-1", path: "src/App.tsx" }); + expect(useDiffStore.getState().reviewViewByThread["thread-1"]).toBe("cumulative"); + }); + + it("restores manual expansion across remounts via manualExpandRef", () => { + const manualExpandRef = { current: new Map([["msg-1", false]]) }; + + renderSummary(["src/App.tsx"], { isLatestTurn: true, manualExpandRef }); + + expect(screen.queryByRole("button", { name: /src\/App\.tsx/ })).not.toBeInTheDocument(); + }); + + it("caps the rendered list and offers a View all diffs escape", async () => { + const manyFiles = Array.from({ length: 8 }, (_, i) => `src/file-${i}.ts`); + transport.getSnapshotDiffStats.mockResolvedValue( + manyFiles.map((filePath) => ({ filePath, additions: 1, deletions: 0, changeType: "modified" })), + ); + + renderSummary(manyFiles); + + expect(await screen.findByRole("button", { name: "Modified src/file-4.ts" })).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Modified src/file-5.ts" })).not.toBeInTheDocument(); + expect(screen.getByText(/\+3 more files/)).toBeInTheDocument(); + }); + + it("groups nested files under folder headers and keeps root files flat", async () => { + renderSummary(["agent.txt", "src/util/a.ts", "src/util/b.ts", "docs/g.md"]); + + // Single-child chains compress into one header ("src/util" not "src" + "util"). + expect(await screen.findByText("src/util")).toBeInTheDocument(); + expect(screen.getByText("docs")).toBeInTheDocument(); + // Root file has no folder header; its row keeps the file aria-label. + expect(screen.getByRole("button", { name: "Modified agent.txt" })).toBeInTheDocument(); + }); + + it("nests shared parents once and collapses folder children on toggle", async () => { + const user = userEvent.setup(); + renderSummary([ + "apps/web/src/a.ts", + "apps/web/src/b.ts", + "apps/server/src/c.ts", + ]); + + // "apps" branches, so it renders once as a parent; chains below compress. + const appsHeader = await screen.findByText("apps"); + expect(screen.getByText("web/src")).toBeInTheDocument(); + expect(screen.getByText("server/src")).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Modified apps/web/src/a.ts" })).toBeInTheDocument(); + + await user.click(appsHeader); + expect(screen.queryByRole("button", { name: "Modified apps/web/src/a.ts" })).not.toBeInTheDocument(); + expect(screen.queryByText("web/src")).not.toBeInTheDocument(); + }); + + it("parses Windows-style paths into folders, basenames, and stats", async () => { + // Git stats arrive with "/" separators even when files_changed uses "\". + transport.getSnapshotDiffStats.mockResolvedValue([ + { + filePath: ".dev/task-briefs/issue-37-keygen-migration.md", + additions: 42, + deletions: 0, + changeType: "added", + }, + ]); + + renderSummary([".dev\\task-briefs\\issue-37-keygen-migration.md"]); + + expect(await screen.findByText(".dev/task-briefs")).toBeInTheDocument(); + const row = screen.getByRole("button", { + name: "Added .dev\\task-briefs\\issue-37-keygen-migration.md", + }); + expect(row).toHaveTextContent("issue-37-keygen-migration.md"); + expect(row).toHaveTextContent("+42"); + }); + + it("does not fetch stats while collapsed", async () => { + renderSummary(["src/App.tsx"], { isLatestTurn: false }); + + expect(screen.getByText("1 file changed")).toBeInTheDocument(); + await act(async () => {}); + expect(transport.getSnapshotDiffStats).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/web/src/components/diff/CommitEntry.tsx b/apps/web/src/components/diff/CommitEntry.tsx index 272cb149c..32607e19f 100644 --- a/apps/web/src/components/diff/CommitEntry.tsx +++ b/apps/web/src/components/diff/CommitEntry.tsx @@ -2,6 +2,7 @@ import { useState, useCallback } from "react"; import { getTransport } from "@/transport"; import { useWorkspaceStore } from "@/features/projects/state/workspaceStore"; import type { GitCommit } from "@mcode/contracts"; +import { pathsToReviewFiles } from "@/lib/review-comparison"; import { FileList } from "./FileList"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; @@ -129,7 +130,7 @@ export function CommitEntry({ commit, threadId }: CommitEntryProps) { No files changed

) : ( - + )}
)} diff --git a/apps/web/src/components/diff/CumulativeView.tsx b/apps/web/src/components/diff/CumulativeView.tsx index 7634fe972..0facf9a60 100644 --- a/apps/web/src/components/diff/CumulativeView.tsx +++ b/apps/web/src/components/diff/CumulativeView.tsx @@ -12,7 +12,6 @@ interface CumulativeViewProps { threadId: string; comparison?: ReviewComparison | null; cacheVersion?: string | number; - turnCount?: number; refreshing?: boolean; onRefresh?: () => void; scopeLabel?: string; @@ -25,17 +24,10 @@ function CumulativeEmptyState() {
; } -function getCumulativeScopeLabel(scopeLabel: string | undefined, fileCount: number, turnCount: number): string { - const fileLabel = `file${fileCount === 1 ? "" : "s"}`; - return scopeLabel ? `${fileLabel} · ${scopeLabel}` : `${fileLabel} · ${turnCount} turn${turnCount === 1 ? "" : "s"}`; -} - -function CumulativeHeader({ fileCount, scopeLabel, turnCount, summaryLens, summaryEnabled, onToggleSummary }: { fileCount: number; scopeLabel: string | undefined; turnCount: number; summaryLens: boolean; summaryEnabled: boolean; onToggleSummary: () => void }) { - const canSummarize = summaryEnabled && !scopeLabel; - return
- {fileCount} - {getCumulativeScopeLabel(scopeLabel, fileCount, turnCount)} - {canSummarize && } +function CumulativeHeader({ scopeLabel, summaryLens, summaryEnabled, onToggleSummary }: { scopeLabel: string | undefined; summaryLens: boolean; summaryEnabled: boolean; onToggleSummary: () => void }) { + if (!summaryEnabled || scopeLabel) return null; + return
+
; } @@ -49,7 +41,7 @@ function CumulativePendingNotice({ refreshing, onRefresh }: { refreshing: boolea
; } -function CumulativeBody({ showSummary, files, threadId, cacheVersion, refreshing, onRefresh }: { showSummary: boolean; files: string[]; threadId: string; cacheVersion: string | number; refreshing: boolean; onRefresh: () => void }) { +function CumulativeBody({ showSummary, files, threadId, cacheVersion, refreshing, onRefresh }: { showSummary: boolean; files: ReviewComparison["files"]; threadId: string; cacheVersion: string | number; refreshing: boolean; onRefresh: () => void }) { if (showSummary) return ; return ; } @@ -60,20 +52,20 @@ function getCumulativeLensState(pending: boolean, scopeLabel: string | undefined } /** Deduplicated file list across all snapshots for the "All" cumulative view. */ -export function CumulativeView({ threadId, comparison = null, cacheVersion = "", turnCount = 0, refreshing = false, onRefresh = () => {}, scopeLabel }: CumulativeViewProps) { +export function CumulativeView({ threadId, comparison = null, cacheVersion = "", refreshing = false, onRefresh = () => {}, scopeLabel }: CumulativeViewProps) { const pending = useDiffStore((s) => s.snapshotsPendingByThread[threadId] ?? false); const diffSummaryEnabled = useSettingsStore((s) => s.settings.diffSummary.enabled); const [summaryLens, setSummaryLens] = useState(false); - const files = useMemo(() => (comparison?.files ?? []).map((file) => file.path), [comparison]); + const files = useMemo(() => comparison?.files ?? [], [comparison]); if (files.length === 0) return ; const { showPendingNotice, showSummary } = getCumulativeLensState(pending, scopeLabel, summaryLens, diffSummaryEnabled); return ( -
- setSummaryLens((value) => !value)} /> +
+ setSummaryLens((value) => !value)} /> {showPendingNotice && }
diff --git a/apps/web/src/components/diff/DiffAnnotationEditor.tsx b/apps/web/src/components/diff/DiffAnnotationEditor.tsx deleted file mode 100644 index bd972bf3b..000000000 --- a/apps/web/src/components/diff/DiffAnnotationEditor.tsx +++ /dev/null @@ -1,108 +0,0 @@ -import { MessageCircle } from "lucide-react"; -import { useEffect, useRef, useState } from "react"; -import { Button } from "@/components/ui/button"; -import { Textarea } from "@/components/ui/textarea"; -import { - usePreviewAnnotationStore, - type DiffAnnotationInput, - type SavedDiffAnnotation, -} from "@/features/preview/state/previewAnnotationStore"; - -/** Props for the inline editor attached to a local diff line. */ -export interface DiffAnnotationEditorProps { - /** Thread that will receive the composer annotation. */ - readonly threadId: string; - /** Line target and source context sent to the agent. */ - readonly target: Omit; - /** Existing annotation when the user is editing a saved line note. */ - readonly annotation?: SavedDiffAnnotation; - /** Closes the inline editor and restores the diff row. */ - readonly onClose: () => void; -} - -/** Edits one Dev diff comment and adds it to the thread composer bundle. */ -export function DiffAnnotationEditor({ - threadId, - target, - annotation, - onClose, -}: DiffAnnotationEditorProps) { - const [note, setNote] = useState(annotation?.note ?? ""); - const textareaRef = useRef(null); - - useEffect(() => { - textareaRef.current?.focus(); - }, []); - - const save = (): void => { - if (!note.trim()) return; - usePreviewAnnotationStore - .getState() - .saveDiffAnnotation(threadId, { ...target, note }, annotation?.id); - onClose(); - }; - - return ( -
-
- - Code comment - - {target.filePath}:{target.line} - -
-