diff --git a/apps/web/components/review/review-dialog.build-files.test.ts b/apps/web/components/review/review-dialog.build-files.test.ts index cd04a79d03..c3f05ca113 100644 --- a/apps/web/components/review/review-dialog.build-files.test.ts +++ b/apps/web/components/review/review-dialog.build-files.test.ts @@ -1,8 +1,15 @@ import { describe, it, expect } from "vitest"; import { buildAllFiles, filterPendingDiffCommentsForSession } from "./review-dialog"; +import { reviewFileKey } from "./types"; import type { CumulativeDiff } from "@/lib/state/slices/session-runtime/types"; import type { Comment } from "@/lib/state/slices/comments"; +const RENAMED_PATH = "src/new-name.ts"; +const PREVIOUS_PATH = "src/old-name.ts"; +const README_PATH = "README.md"; +const FRONTEND_REPO = "frontend"; +const BACKEND_REPO = "backend"; + function pendingDiffComment(overrides: Partial): Comment { return { id: "c1", @@ -139,6 +146,278 @@ describe("buildAllFiles (review dialog)", () => { }); }); +describe("buildAllFiles source collisions", () => { + it("deduplicates a repo-stamped cumulative file against repo-unaware git status", () => { + const path = "src/shared.ts"; + const gitStatusFiles = { + [path]: { + path, + status: "modified" as const, + staged: false, + additions: 1, + deletions: 1, + diff: "@@ -1 +1 @@\n-local\n+fresh\n", + }, + }; + const cumulativeDiff = { + session_id: "s1", + base_commit: "abc", + head_commit: "def", + total_commits: 1, + files: { + [`${FRONTEND_REPO}\u0000${path}`]: { + path, + repository_name: FRONTEND_REPO, + status: "modified", + staged: false, + additions: 1, + deletions: 1, + diff: "@@ -1 +1 @@\n-stale\n+snapshot\n", + }, + }, + } as unknown as CumulativeDiff; + + const result = buildAllFiles(gitStatusFiles, cumulativeDiff); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ path, source: "uncommitted" }); + }); + + it("uses a bare cumulative key in single-repository review mode", () => { + const path = "src/committed.ts"; + const cumulativeDiff = { + session_id: "s1", + base_commit: "abc", + head_commit: "def", + total_commits: 1, + files: { + [`${FRONTEND_REPO}\u0000${path}`]: { + path, + repository_name: FRONTEND_REPO, + status: "modified", + staged: false, + diff: "@@committed@@", + }, + }, + } as CumulativeDiff; + + const result = buildAllFiles(null, cumulativeDiff, undefined, undefined, false); + + expect(result).toHaveLength(1); + expect(reviewFileKey(result[0])).toBe(path); + expect(result[0].repository_name).toBeUndefined(); + }); +}); + +describe("buildAllFiles sort order", () => { + it("sorts multi-repo files by repository name and then path", () => { + const gitStatusFiles = { + "frontend\u0000src/a.ts": { + path: "src/a.ts", + status: "modified" as const, + staged: false, + repository_name: FRONTEND_REPO, + }, + "backend\u0000src/b.ts": { + path: "src/b.ts", + status: "modified" as const, + staged: false, + repository_name: BACKEND_REPO, + }, + }; + + const result = buildAllFiles(gitStatusFiles, null); + + expect(result.map((file) => `${file.repository_name}:${file.path}`)).toEqual([ + "backend:src/b.ts", + "frontend:src/a.ts", + ]); + }); +}); + +describe("buildAllFiles patchless status files", () => { + it("keeps a patchless PR rename with its previous path", () => { + const result = buildAllFiles(null, null, [ + { + filename: RENAMED_PATH, + old_path: PREVIOUS_PATH, + status: "renamed", + patch: "", + additions: 0, + deletions: 0, + }, + ]); + + expect(result).toEqual([ + expect.objectContaining({ + path: RENAMED_PATH, + old_path: PREVIOUS_PATH, + status: "renamed", + diff: "", + source: "pr", + }), + ]); + expect(reviewFileKey(result[0])).toBe(RENAMED_PATH); + expect(result[0].repository_name).toBeUndefined(); + }); + + it("keeps patchless uncommitted and cumulative files with metadata", () => { + const gitStatusFiles = { + "assets/logo.png": { + path: "assets/logo.png", + status: "modified" as const, + staged: false, + diff_skip_reason: "binary" as const, + }, + }; + const cumulativeDiff = { + session_id: "s1", + base_commit: "abc", + head_commit: "def", + total_commits: 1, + files: { + [RENAMED_PATH]: { + path: RENAMED_PATH, + old_path: PREVIOUS_PATH, + status: "renamed", + staged: false, + additions: 0, + deletions: 0, + diff: "", + }, + }, + } as CumulativeDiff; + + const result = buildAllFiles(gitStatusFiles, cumulativeDiff); + + expect(result).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + path: "assets/logo.png", + diff_skip_reason: "binary", + diff: "", + source: "uncommitted", + }), + expect.objectContaining({ + path: RENAMED_PATH, + old_path: PREVIOUS_PATH, + status: "renamed", + diff: "", + source: "committed", + }), + ]), + ); + expect(result).toHaveLength(2); + }); +}); + +describe("buildAllFiles patchless source identity", () => { + it("keeps patchless uncommitted precedence over a same-repo PR file", () => { + const gitStatusFiles = { + [`${FRONTEND_REPO}\u0000${README_PATH}`]: { + path: README_PATH, + status: "modified" as const, + staged: false, + repository_name: FRONTEND_REPO, + }, + }; + + const result = buildAllFiles( + gitStatusFiles, + null, + [ + { + filename: README_PATH, + status: "modified", + patch: "", + additions: 0, + deletions: 0, + }, + ], + FRONTEND_REPO, + ); + + expect(result).toEqual([ + expect.objectContaining({ + path: README_PATH, + source: "uncommitted", + repository_name: FRONTEND_REPO, + }), + ]); + }); + + it("keeps patchless cumulative precedence over a same-repo PR file", () => { + const cumulativeDiff = { + session_id: "s1", + base_commit: "abc", + head_commit: "def", + total_commits: 1, + files: { + [`${FRONTEND_REPO}\u0000${README_PATH}`]: { + path: README_PATH, + status: "modified", + staged: false, + repository_name: FRONTEND_REPO, + }, + }, + } as CumulativeDiff; + + const result = buildAllFiles( + null, + cumulativeDiff, + [ + { + filename: README_PATH, + status: "modified", + patch: "", + additions: 0, + deletions: 0, + }, + ], + FRONTEND_REPO, + ); + + expect(result).toEqual([ + expect.objectContaining({ + path: README_PATH, + source: "committed", + repository_name: FRONTEND_REPO, + }), + ]); + }); + + it("keeps same-path files from different repositories distinct", () => { + const gitStatusFiles = { + [`${BACKEND_REPO}\u0000${README_PATH}`]: { + path: README_PATH, + status: "modified" as const, + staged: false, + repository_name: BACKEND_REPO, + }, + }; + + const result = buildAllFiles( + gitStatusFiles, + null, + [ + { + filename: README_PATH, + status: "modified", + patch: "", + additions: 0, + deletions: 0, + }, + ], + FRONTEND_REPO, + ); + + expect(result.map(reviewFileKey).sort()).toEqual([ + "backend\u0000README.md", + "frontend\u0000README.md", + ]); + }); +}); + describe("filterPendingDiffCommentsForSession", () => { it("keeps only diff comments from the active session", () => { const comments = [ diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index e5ca74dc59..5771a2aaaa 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -14,6 +14,7 @@ import { useReviewSidebarResize } from "@/hooks/use-review-sidebar-resize"; import { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; import { DEFAULT_DIFF_WORD_WRAP } from "@/components/diff/diff-defaults"; +import { normalizeFileChangeStatus } from "@/lib/utils/file-change-status"; import { useRequestChangesWalkthrough } from "@/hooks/domains/session/use-request-changes-walkthrough"; import { ReviewTopBar } from "./review-top-bar"; import { ReviewFileTree } from "./review-file-tree"; @@ -42,7 +43,7 @@ function fileMapKey(path: string, repositoryName?: string): string { function addCumulativeDiffFiles( fileMap: Map, files: CumulativeDiff["files"], - gitStatusFiles: Record | null, + useRepositoryKeys: boolean, ) { // Multi-repo: backend stamps each per-file payload with `repository_name` // + the repo-relative `path`, and uses a NUL-composite `\x00` @@ -51,45 +52,28 @@ function addCumulativeDiffFiles( // value so the composite key doesn't bleed into the displayed path, and // fall back to the map key so single-repo files aren't silently dropped. for (const [mapKey, file] of Object.entries(files)) { - const repoName = file.repository_name; - const path = file.path ?? mapKey; + const repoName = useRepositoryKeys ? file.repository_name : undefined; + const path = file.path ?? splitFileKey(mapKey).path; if (!path) continue; const key = fileMapKey(path, repoName); - if (fileMap.has(key)) continue; + const hasRepoUnawareCollision = key !== path && fileMap.has(path); + if (fileMap.has(key) || hasRepoUnawareCollision) continue; const diff = file.diff ? normalizeDiffContent(file.diff) : ""; - if (diff) { - const matchingUncommitted = findUncommittedByPathAndRepo(gitStatusFiles, path, repoName); - fileMap.set(key, { - path, - diff, - status: file.status || "modified", - additions: file.additions ?? 0, - deletions: file.deletions ?? 0, - staged: matchingUncommitted?.staged ?? false, - source: matchingUncommitted ? "uncommitted" : "committed", - repository_name: repoName, - }); - } + fileMap.set(key, { + path, + diff, + status: normalizeFileChangeStatus(file.status), + additions: file.additions ?? 0, + deletions: file.deletions ?? 0, + staged: false, + source: "committed", + old_path: file.old_path, + diff_skip_reason: file.diff_skip_reason, + repository_name: repoName, + }); } } -/** Looks up a FileInfo in the (possibly composite-keyed) gitStatus map by - * (path, repository_name). When repo is undefined or empty (single-repo), - * returns the first match by path. */ -function findUncommittedByPathAndRepo( - gitStatusFiles: Record | null, - path: string, - repositoryName: string | undefined, -): FileInfo | undefined { - if (!gitStatusFiles) return undefined; - for (const file of Object.values(gitStatusFiles)) { - if (file.path !== path) continue; - if (!repositoryName) return file; - if (file.repository_name === repositoryName) return file; - } - return undefined; -} - function addUncommittedFiles( fileMap: Map, gitStatusFiles: Record, @@ -98,40 +82,39 @@ function addUncommittedFiles( const key = fileMapKey(file.path, file.repository_name); if (fileMap.has(key)) continue; const diff = file.diff ? normalizeDiffContent(file.diff) : ""; - if (diff) - fileMap.set(key, { - path: file.path, - diff, - status: file.status, - additions: file.additions ?? 0, - deletions: file.deletions ?? 0, - staged: file.staged, - source: "uncommitted", - repository_name: file.repository_name, - }); + fileMap.set(key, { + path: file.path, + diff, + status: normalizeFileChangeStatus(file.status), + additions: file.additions ?? 0, + deletions: file.deletions ?? 0, + staged: file.staged, + source: "uncommitted", + old_path: file.old_path, + diff_skip_reason: file.diff_skip_reason, + repository_name: file.repository_name, + }); } } -function prFileStatus(status: string): "added" | "deleted" | "modified" { - if (status === "added") return "added"; - if (status === "removed") return "deleted"; - return "modified"; -} - -function addPRFiles(fileMap: Map, files: PRDiffFile[]) { +function addPRFiles(fileMap: Map, files: PRDiffFile[], repoName?: string) { + const repositoryName = repoName || undefined; for (const file of files) { - if (fileMap.has(file.filename)) continue; + const key = fileMapKey(file.filename, repositoryName); + const hasRepoUnawareCollision = !!repositoryName && fileMap.has(file.filename); + if (fileMap.has(key) || hasRepoUnawareCollision) continue; const diff = file.patch ? normalizeDiffContent(file.patch) : ""; - if (diff) - fileMap.set(file.filename, { - path: file.filename, - diff, - status: prFileStatus(file.status), - additions: file.additions ?? 0, - deletions: file.deletions ?? 0, - staged: false, - source: "pr", - }); + fileMap.set(key, { + path: file.filename, + diff, + status: normalizeFileChangeStatus(file.status), + additions: file.additions ?? 0, + deletions: file.deletions ?? 0, + staged: false, + source: "pr", + old_path: file.old_path, + repository_name: repositoryName, + }); } } @@ -139,19 +122,28 @@ export function buildAllFiles( gitStatusFiles: Record | null, cumulativeDiff: CumulativeDiff | null, prDiffFiles?: PRDiffFile[], + prRepoName?: string, + useRepositoryKeys = true, ): ReviewFile[] { const fileMap = new Map(); - // Order matters and must match `buildReviewSources` (the Changes panel - // builder): uncommitted FIRST so its always-fresh WS-pushed diff content - // wins over the polled cumulative diff for files that exist in both. Before + // Keep priority, dedup keys, and sorting aligned with `buildReviewSources` + // in `hooks/domains/session/use-review-sources.ts`. Uncommitted goes first, + // so its always-fresh WS-pushed diff content wins over the polled cumulative + // diff for files that exist in both. Before // this, the dialog and the panel disagreed on `datasource.ts`-style files // (panel: fresh worktree content from `git-status`, dialog: stale cumulative // diff snapshot from the last fetch) — the dialog appeared to show outdated // content even though the cumulative-diff hook was successfully refetching. if (gitStatusFiles) addUncommittedFiles(fileMap, gitStatusFiles); - if (cumulativeDiff?.files) addCumulativeDiffFiles(fileMap, cumulativeDiff.files, gitStatusFiles); - if (prDiffFiles) addPRFiles(fileMap, prDiffFiles); - return Array.from(fileMap.values()).sort((a, b) => a.path.localeCompare(b.path)); + if (cumulativeDiff?.files) { + addCumulativeDiffFiles(fileMap, cumulativeDiff.files, useRepositoryKeys); + } + if (prDiffFiles) addPRFiles(fileMap, prDiffFiles, prRepoName); + return Array.from(fileMap.values()).sort((a, b) => { + const repoCmp = (a.repository_name ?? "").localeCompare(b.repository_name ?? ""); + if (repoCmp !== 0) return repoCmp; + return a.path.localeCompare(b.path); + }); } export function filterPendingDiffCommentsForSession( @@ -173,6 +165,8 @@ type ReviewDialogProps = { gitStatusFiles: Record | null; cumulativeDiff: CumulativeDiff | null; prDiffFiles?: PRDiffFile[]; + prRepoName?: string; + useRepositoryKeys?: boolean; }; function computeReviewSets( @@ -343,6 +337,8 @@ function useReviewDialogState(props: ReviewDialogProps) { gitStatusFiles, cumulativeDiff, prDiffFiles, + prRepoName, + useRepositoryKeys = true, } = props; const [selectedFile, setSelectedFile] = useState(null); const [splitView, setSplitView] = useState(() => @@ -361,8 +357,8 @@ function useReviewDialogState(props: ReviewDialogProps) { const [filter, setFilter] = useState(""); const allFiles = useMemo( - () => buildAllFiles(gitStatusFiles, cumulativeDiff, prDiffFiles), - [gitStatusFiles, cumulativeDiff, prDiffFiles], + () => buildAllFiles(gitStatusFiles, cumulativeDiff, prDiffFiles, prRepoName, useRepositoryKeys), + [gitStatusFiles, cumulativeDiff, prDiffFiles, prRepoName, useRepositoryKeys], ); const filteredFiles = useFilteredReviewFiles(allFiles, filter); const { reviewedFiles, staleFiles } = useMemo( diff --git a/apps/web/components/review/review-diff-list-grouping.test.tsx b/apps/web/components/review/review-diff-list-grouping.test.tsx index 5e8da6efae..b7b214d68b 100644 --- a/apps/web/components/review/review-diff-list-grouping.test.tsx +++ b/apps/web/components/review/review-diff-list-grouping.test.tsx @@ -68,6 +68,20 @@ function withTooltips(node: ReactNode) { return {node}; } +function renderSingleFile(reviewFile: ReviewFile, selected = true) { + const refs = new Map([[reviewFile.path, createRef()]]); + render( + withTooltips( + , + ), + ); +} + describe("ReviewDiffList — multi-repo grouping", () => { it("renders no repo header when all files lack a repository_name", () => { const refs = new Map([["a.ts", createRef()]]); @@ -121,3 +135,68 @@ describe("ReviewDiffList — multi-repo grouping", () => { expect(yGroup?.textContent).not.toContain("a.ts"); }); }); + +describe("ReviewDiffList — file status rendering", () => { + it("shows moved status in the mobile header and honest copy for a patchless rename", () => { + const movedFile = { + ...file("new-name.ts"), + diff: "", + status: "renamed", + old_path: "old-name.ts", + } as ReviewFile; + renderSingleFile(movedFile); + + const marker = screen.getByRole("img", { name: "Moved from old-name.ts" }); + expect(marker.className).toContain("sm:hidden"); + expect(screen.getByText("Moved from old-name.ts; no textual changes")).toBeTruthy(); + }); + + it("treats a nonempty 100%-rename metadata diff as patchless", () => { + const movedFile = { + ...file("new-name.ts"), + diff: [ + "diff --git a/old-name.ts b/new-name.ts", + "similarity index 100%", + "rename from old-name.ts", + "rename to new-name.ts", + ].join("\n"), + status: "renamed", + old_path: "old-name.ts", + } as ReviewFile; + renderSingleFile(movedFile); + + expect(screen.getByText("Moved from old-name.ts; no textual changes")).toBeTruthy(); + expect(screen.queryByTestId("diff-stub")).toBeNull(); + }); + + it("treats a synthetic added-file hunk for a zero-stat rename as patchless", () => { + const movedFile = { + ...file("new-name.ts"), + diff: [ + "diff --git a/new-name.ts b/new-name.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/new-name.ts", + "@@ -0,0 +1,2 @@", + "+first line", + "+second line", + ].join("\n"), + status: "renamed", + additions: 0, + deletions: 0, + old_path: "old-name.ts", + } as ReviewFile; + renderSingleFile(movedFile); + + expect(screen.getByText("Moved from old-name.ts; no textual changes")).toBeTruthy(); + expect(screen.queryByTestId("diff-stub")).toBeNull(); + }); + + it("keeps the loading state for a nonempty diff deferred by lazy rendering", () => { + const deferredFile = file("deferred.ts"); + renderSingleFile(deferredFile, false); + + expect(screen.getByText("Loading diff...")).toBeTruthy(); + expect(screen.queryByText("No textual diff available")).toBeNull(); + }); +}); diff --git a/apps/web/components/review/review-diff-list.tsx b/apps/web/components/review/review-diff-list.tsx index 36d01bbe28..f3ba81adcf 100644 --- a/apps/web/components/review/review-diff-list.tsx +++ b/apps/web/components/review/review-diff-list.tsx @@ -4,6 +4,7 @@ import { memo, useEffect, useMemo, useRef, useState, useCallback } from "react"; import { IconAlertTriangle, IconChevronDown, IconChevronRight } from "@tabler/icons-react"; import { Checkbox } from "@kandev/ui/checkbox"; import { FileDiffViewer, DiffErrorBoundary } from "@/components/diff"; +import { FileStatusIcon } from "@/components/shared/file-status-icon"; import type { RevertBlockInfo } from "@/components/diff"; import { getWebSocketClient } from "@/lib/ws/connection"; import { requestFileContent, updateFileContent } from "@/lib/ws/workspace-files"; @@ -13,7 +14,12 @@ import { useToast } from "@/components/toast-provider"; import { useRunComment } from "@/hooks/domains/comments/use-run-comment"; import { useBaseBranchByRepo } from "@/hooks/domains/session/use-base-branch-by-repo"; import type { DiffComment } from "@/lib/diff/types"; -import { diffSkipReasonLabel, reviewFileKey } from "./types"; +import { + diffSkipReasonLabel, + hasTextualDiff, + reviewDiffUnavailableLabel, + reviewFileKey, +} from "./types"; import type { ReviewFile } from "./types"; import { RepoGroupHeader } from "./review-diff-list-groups"; import { FileDiffToolbar } from "./review-diff-toolbar"; @@ -245,7 +251,11 @@ function FileDiffHeader({ onToggleWordWrap, }: FileDiffHeaderProps) { return ( -
+
{file.path} + {isStale && ( @@ -438,7 +449,8 @@ function renderDiffContent(opts: { onCommentRun, onToggleExpandUnchanged, } = opts; - if (shouldRender && file.diff) { + const hasText = hasTextualDiff(file); + if (shouldRender && hasText) { return ( <> @@ -468,9 +480,12 @@ function renderDiffContent(opts: { ); } + const message = hasText + ? diffSkipReasonLabel(file.diff_skip_reason) + : reviewDiffUnavailableLabel(file); return (
- {diffSkipReasonLabel(file.diff_skip_reason)} + {message}
); } diff --git a/apps/web/components/review/review-file-tree.test.tsx b/apps/web/components/review/review-file-tree.test.tsx index a06daf72e2..aa5d745981 100644 --- a/apps/web/components/review/review-file-tree.test.tsx +++ b/apps/web/components/review/review-file-tree.test.tsx @@ -43,6 +43,23 @@ function renderTree(files: ReviewFile[], overrides: RenderOpts = {}) { return { onFilterChange, onSelectFile, onToggleReviewed }; } +describe("ReviewFileTree status markers", () => { + it("renders each file status as the final fixed row item", () => { + renderTree([ + file({ path: "src/added.ts", status: "added" }), + file({ path: "src/modified.ts", status: "modified" }), + file({ path: "src/deleted.ts", status: "deleted" }), + file({ path: "src/moved.ts", status: "renamed" }), + ]); + + for (const label of ["Added", "Modified", "Deleted", "Moved"]) { + const marker = screen.getByRole("img", { name: label }); + expect(marker.parentElement?.lastElementChild).toBe(marker); + expect(marker.className).toContain("shrink-0"); + } + }); +}); + describe("ReviewFileTree", () => { it("renders one row per file", () => { renderTree([file({ path: "src/a.ts" }), file({ path: "src/b.ts" })]); @@ -153,7 +170,9 @@ describe("ReviewFileTree", () => { // backend's child is still visible. expect(screen.getByText("task.go")).toBeTruthy(); }); +}); +describe("ReviewFileTree multi-repo identity", () => { it("disambiguates same-named files in different repos via composite key", () => { const fA = file({ path: "README.md", repository_name: "frontend", repository_id: "f" }); const fB = file({ path: "README.md", repository_name: "backend", repository_id: "b" }); @@ -169,4 +188,17 @@ describe("ReviewFileTree", () => { expect(calls).toContain(reviewFileKey(fA)); expect(calls).toContain(reviewFileKey(fB)); }); + + it("exposes a stable repository discriminator for same-path rows", () => { + renderTree([ + file({ path: "README.md", repository_name: "frontend", repository_id: "f" }), + file({ path: "README.md", repository_name: "backend", repository_id: "b" }), + ]); + + const identities = screen + .getAllByTestId("review-file-row") + .map((row) => `${row.dataset.repositoryName}:${row.dataset.filePath}`) + .sort(); + expect(identities).toEqual(["backend:README.md", "frontend:README.md"]); + }); }); diff --git a/apps/web/components/review/review-file-tree.tsx b/apps/web/components/review/review-file-tree.tsx index 218d5eba0d..67277040dd 100644 --- a/apps/web/components/review/review-file-tree.tsx +++ b/apps/web/components/review/review-file-tree.tsx @@ -11,6 +11,7 @@ import { } from "@tabler/icons-react"; import { Checkbox } from "@kandev/ui/checkbox"; import { cn } from "@kandev/ui/lib/utils"; +import { FileStatusIcon } from "@/components/shared/file-status-icon"; import { FileIcon } from "@/components/ui/file-icon"; import { useTree, type VisibleRow } from "@/hooks/use-tree"; import type { ReviewFile, FileTreeNode } from "./types"; @@ -182,6 +183,9 @@ function ReviewFileRow({ const isSelected = selectedFile === key; return (
- {row.node.name} + + {row.node.name} + {isStale && } {commentCount > 0 && ( - + {commentCount} )} +
); } diff --git a/apps/web/components/review/types.multi-repo.test.ts b/apps/web/components/review/types.multi-repo.test.ts index e1aa9362b8..14539d097a 100644 --- a/apps/web/components/review/types.multi-repo.test.ts +++ b/apps/web/components/review/types.multi-repo.test.ts @@ -1,5 +1,14 @@ import { describe, it, expect } from "vitest"; -import { buildFileTree, reviewFileKey, splitReviewFileKey, type ReviewFile } from "./types"; +import { + buildFileTree, + getCumulativeReviewRepositoryNames, + isReviewMultiRepo, + resolvePRReviewRepositoryName, + reviewFileKey, + sanitizeReviewRepositoryName, + splitReviewFileKey, + type ReviewFile, +} from "./types"; const SEP = "\u0000"; @@ -105,6 +114,62 @@ describe("reviewFileKey", () => { }); }); +describe("resolvePRReviewRepositoryName", () => { + it("uses the sanitized workspace repository name instead of the provider repo name", () => { + expect( + resolvePRReviewRepositoryName({ repository_id: "repo-1", repo: "widgets" }, "acme/widgets"), + ).toBe("acme-widgets"); + }); + + it("falls back to the provider repo name for legacy or unhydrated workspace data", () => { + expect(resolvePRReviewRepositoryName({ repo: "widgets" }, "acme/widgets")).toBe("widgets"); + expect(resolvePRReviewRepositoryName({ repository_id: "repo-1", repo: "widgets" })).toBe( + "widgets", + ); + }); +}); + +describe("sanitizeReviewRepositoryName", () => { + it.each([ + ["acme/widgets", "acme-widgets"], + ["owner\\repo", "owner-repo"], + ["weird:name space", "weird-name-space"], + ["with..dots", "with..dots"], + ["--a///b..", "a-b"], + ["修复/登录", ""], + ])("mirrors the backend repository directory sanitizer for %s", (input, expected) => { + expect(sanitizeReviewRepositoryName(input)).toBe(expected); + }); +}); + +describe("isReviewMultiRepo", () => { + it("detects multiple named git-status repositories before task links hydrate", () => { + expect(isReviewMultiRepo(0, ["frontend", "backend"])).toBe(true); + }); + + it("uses task links during partial status hydration", () => { + expect(isReviewMultiRepo(2, ["frontend"])).toBe(true); + }); + + it("keeps one named or legacy repository in single-repo mode", () => { + expect(isReviewMultiRepo(1, ["frontend"])).toBe(false); + expect(isReviewMultiRepo(1, [""])).toBe(false); + }); +}); + +describe("getCumulativeReviewRepositoryNames", () => { + it("returns distinct stamped repository names", () => { + expect( + getCumulativeReviewRepositoryNames({ + a: { repository_name: "frontend" }, + b: { repository_name: "backend" }, + c: { repository_name: "frontend" }, + d: {}, + }), + ).toEqual(["frontend", "backend"]); + }); +}); + describe("splitReviewFileKey", () => { it("returns empty repositoryName for legacy bare-path keys", () => { expect(splitReviewFileKey("README.md")).toEqual({ repositoryName: "", path: "README.md" }); diff --git a/apps/web/components/review/types.test.ts b/apps/web/components/review/types.test.ts new file mode 100644 index 0000000000..d84bb43e41 --- /dev/null +++ b/apps/web/components/review/types.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it } from "vitest"; +import { hasTextualDiff, reviewDiffUnavailableLabel, type ReviewFile } from "./types"; + +function patchlessFile(overrides: Partial): ReviewFile { + return { + path: "src/new-name.ts", + diff: "", + status: "renamed", + additions: 0, + deletions: 0, + staged: false, + source: "pr", + ...overrides, + }; +} + +describe("reviewDiffUnavailableLabel", () => { + it("describes a pure move using its previous path", () => { + const file = patchlessFile({ old_path: "src/old-name.ts" }); + expect(reviewDiffUnavailableLabel(file)).toBe("Moved from src/old-name.ts; no textual changes"); + }); + + it("prefers an explicit diff skip reason over move text", () => { + const file = patchlessFile({ + old_path: "src/old-name.ts", + diff_skip_reason: "binary", + }); + expect(reviewDiffUnavailableLabel(file)).toBe("Binary file — not diffable"); + }); + + it.each([ + ["added", "Added file has no textual diff"], + ["untracked", "Untracked file has no textual diff"], + ["deleted", "Deleted file has no textual diff"], + ] as const)("describes a patchless %s file", (status, expected) => { + const file = patchlessFile({ status, old_path: undefined }); + expect(reviewDiffUnavailableLabel(file)).toBe(expected); + }); +}); + +describe("hasTextualDiff", () => { + it.each<[string, Partial]>([ + ["empty input", { diff: "", status: "modified" }], + [ + "pure rename metadata", + { + diff: [ + "diff --git a/src/old.ts b/src/new.ts", + "similarity index 100%", + "rename from src/old.ts", + "rename to src/new.ts", + ].join("\n"), + status: "renamed", + }, + ], + [ + "empty added-file metadata", + { + diff: [ + "diff --git a/empty.txt b/empty.txt", + "new file mode 100644", + "index 0000000..e69de29", + ].join("\n"), + status: "added", + }, + ], + [ + "zero-stat rename with a synthetic added-file hunk", + { + diff: [ + "diff --git a/src/new.ts b/src/new.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/src/new.ts", + "@@ -0,0 +1,2 @@", + "+first line", + "+second line", + ].join("\n"), + status: "renamed", + additions: 0, + deletions: 0, + }, + ], + [ + "empty untracked zero-count hunk", + { + diff: [ + "diff --git a/empty.txt b/empty.txt", + "new file mode 100644", + "--- /dev/null", + "+++ b/empty.txt", + "@@ -0,0 +1,0 @@", + ].join("\n"), + status: "untracked", + additions: 0, + deletions: 0, + }, + ], + ])("returns false for %s", (_name, overrides) => { + expect(hasTextualDiff(patchlessFile(overrides))).toBe(false); + }); + + it.each([ + ["modified file", "modified", 1, 1], + ["renamed file with a real delta", "renamed", 1, 1], + ] as const)("returns true for a %s hunk", (_name, status, additions, deletions) => { + const file = patchlessFile({ + diff: [ + "diff --git a/src/app.ts b/src/app.ts", + "--- a/src/app.ts", + "+++ b/src/app.ts", + "@@ -1 +1 @@", + "-before", + "+after", + ].join("\n"), + status, + additions, + deletions, + }); + expect(hasTextualDiff(file)).toBe(true); + }); +}); diff --git a/apps/web/components/review/types.ts b/apps/web/components/review/types.ts index b7479b5c23..97194ed6a0 100644 --- a/apps/web/components/review/types.ts +++ b/apps/web/components/review/types.ts @@ -1,13 +1,15 @@ import { djb2Hash } from "@/lib/utils/hash"; +import type { FileChangeStatus } from "@/lib/utils/file-change-status"; export type ReviewFile = { path: string; diff: string; - status: string; + status: FileChangeStatus; additions: number; deletions: number; staged: boolean; source: "uncommitted" | "committed" | "pr"; + old_path?: string; diff_skip_reason?: "too_large" | "binary" | "truncated" | "budget_exceeded"; /** * Repository this file belongs to. Set on multi-repo task changes so the @@ -34,6 +36,57 @@ export function reviewFileKey(file: { path: string; repository_name?: string }): return file.repository_name ? `${file.repository_name}${FILE_KEY_SEP}${file.path}` : file.path; } +/** Mirrors backend `worktree.SanitizeRepoDirName`, which defines the + * repository_name stamped on multi-repo agentctl events. */ +export function sanitizeReviewRepositoryName(name: string): string { + let sanitized = ""; + for (const character of name) { + const code = character.charCodeAt(0); + const isASCIIAlphaNumeric = + (code >= 48 && code <= 57) || (code >= 65 && code <= 90) || (code >= 97 && code <= 122); + sanitized += + isASCIIAlphaNumeric || character === "_" || character === "." || character === "-" + ? character + : "-"; + } + return sanitized.replace(/-+/g, "-").replace(/^[-.]+|[-.]+$/g, ""); +} + +export function resolvePRReviewRepositoryName( + pr: { repository_id?: string; repo: string } | null | undefined, + workspaceRepositoryName?: string | null, +): string | undefined { + if (!pr) return undefined; + if (pr.repository_id && workspaceRepositoryName) { + const canonicalName = sanitizeReviewRepositoryName(workspaceRepositoryName); + if (canonicalName) return canonicalName; + } + return pr.repo || undefined; +} + +export function isReviewMultiRepo( + taskRepositoryCount: number, + repositoryNames: Iterable, +): boolean { + if (taskRepositoryCount > 1) return true; + const namedRepositories = new Set(); + for (const name of repositoryNames) { + if (name) namedRepositories.add(name); + if (namedRepositories.size > 1) return true; + } + return false; +} + +export function getCumulativeReviewRepositoryNames( + files: Record | null | undefined, +): string[] { + const names = new Set(); + for (const file of Object.values(files ?? {})) { + if (file.repository_name) names.add(file.repository_name); + } + return Array.from(names); +} + export function splitReviewFileKey(key: string): { repositoryName: string; path: string } { const sep = key.indexOf(FILE_KEY_SEP); if (sep < 0) return { repositoryName: "", path: key }; @@ -55,6 +108,48 @@ export function diffSkipReasonLabel(reason?: string): string { } } +const TEXTUAL_HUNK_HEADER = /^@@+ (.+?) @@+/; +const HUNK_RANGE = /[+-]\d+(?:,(\d+))?/g; + +function hunkHeaderHasLines(line: string): boolean { + const header = line.match(TEXTUAL_HUNK_HEADER); + if (!header) return false; + return [...header[1].matchAll(HUNK_RANGE)].some((range) => Number(range[1] ?? "1") > 0); +} + +/** True when a file has a real textual delta, excluding git's synthetic rename/empty hunks. */ +export function hasTextualDiff( + file: Pick, +): boolean { + // GitHub sends 0+0 stats for pure renames even when the diff contains a + // synthetic new-file hunk. Trust the stats because that hunk represents the + // same file at a new path with no textual change. + if (file.status === "renamed" && file.additions === 0 && file.deletions === 0) return false; + return file.diff.split("\n").some(hunkHeaderHasLines); +} + +export function reviewDiffUnavailableLabel(file: ReviewFile): string { + if (file.diff_skip_reason) return diffSkipReasonLabel(file.diff_skip_reason); + switch (file.status) { + case "added": + return "Added file has no textual diff"; + case "untracked": + return "Untracked file has no textual diff"; + case "deleted": + return "Deleted file has no textual diff"; + case "renamed": + return file.old_path + ? `Moved from ${file.old_path}; no textual changes` + : "File moved; no textual changes"; + case "modified": + return "No textual diff available"; + default: { + const exhaustiveStatus: never = file.status; + return exhaustiveStatus; + } + } +} + export type FileTreeNode = { name: string; path: string; diff --git a/apps/web/components/shared/file-status-icon.test.tsx b/apps/web/components/shared/file-status-icon.test.tsx new file mode 100644 index 0000000000..601ec41859 --- /dev/null +++ b/apps/web/components/shared/file-status-icon.test.tsx @@ -0,0 +1,38 @@ +import { cleanup, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it } from "vitest"; +import type { FileChangeStatus } from "@/lib/utils/file-change-status"; +import { FileStatusIcon } from "./file-status-icon"; + +afterEach(cleanup); + +describe("FileStatusIcon", () => { + it.each<[FileChangeStatus, string, string]>([ + ["added", "Added", "tabler-icon-plus"], + ["untracked", "Untracked", "tabler-icon-plus"], + ["modified", "Modified", "tabler-icon-circle-filled"], + ["deleted", "Deleted", "tabler-icon-minus"], + ["renamed", "Moved", "tabler-icon-arrow-right"], + ])("renders an accessible %s marker", (status, label, glyphClass) => { + render(); + + const marker = screen.getByRole("img", { name: label }); + expect(marker.getAttribute("data-file-status")).toBe(status); + expect(marker.getAttribute("title")).toBe(label); + expect(marker.querySelector(`.${glyphClass}`)).not.toBeNull(); + }); + + it("exposes previous-path context for a moved file", () => { + render(); + + const marker = screen.getByRole("img", { name: "Moved from src/old-name.ts" }); + expect(marker.getAttribute("title")).toBe("Moved from src/old-name.ts"); + }); + + it("merges caller classes into the fixed-size marker", () => { + render(); + + const marker = screen.getByRole("img", { name: "Modified" }); + expect(marker.className).toContain("shrink-0"); + expect(marker.className).toContain("custom-marker"); + }); +}); diff --git a/apps/web/components/shared/file-status-icon.tsx b/apps/web/components/shared/file-status-icon.tsx new file mode 100644 index 0000000000..bc8e6d5199 --- /dev/null +++ b/apps/web/components/shared/file-status-icon.tsx @@ -0,0 +1,57 @@ +"use client"; + +import { IconArrowRight, IconCircleFilled, IconMinus, IconPlus } from "@tabler/icons-react"; +import { cn } from "@/lib/utils"; +import { fileChangeStatusLabel, type FileChangeStatus } from "@/lib/utils/file-change-status"; + +type FileStatusIconProps = { + status: FileChangeStatus; + oldPath?: string; + className?: string; +}; + +const markerClassName: Record = { + added: "border-emerald-600 text-emerald-600", + untracked: "border-emerald-600 text-emerald-600", + modified: "border-amber-600 text-amber-600", + deleted: "border-rose-600 text-rose-600", + renamed: "border-purple-600 text-purple-600", +}; + +function StatusGlyph({ status }: { status: FileChangeStatus }) { + switch (status) { + case "added": + case "untracked": + return