feat(web): show file statuses in review (#1683)
* feat(web): show file statuses in review * fix(web): address review status feedback * fix(web): deduplicate review sources * fix(web): align review progress identities * fix(web): guard review status labels * fix(web): enforce file status exhaustiveness * perf(web): index review progress sources * fix(web): preserve files during status hydration * fix(web): align multi-repo review progress
This commit is contained in:
José Almeida
@@ -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>): 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 = [
|
||||
|
||||
@@ -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<string, ReviewFile>,
|
||||
files: CumulativeDiff["files"],
|
||||
gitStatusFiles: Record<string, FileInfo> | null,
|
||||
useRepositoryKeys: boolean,
|
||||
) {
|
||||
// Multi-repo: backend stamps each per-file payload with `repository_name`
|
||||
// + the repo-relative `path`, and uses a NUL-composite `<repo>\x00<path>`
|
||||
@@ -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<string, FileInfo> | 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<string, ReviewFile>,
|
||||
gitStatusFiles: Record<string, FileInfo>,
|
||||
@@ -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<string, ReviewFile>, files: PRDiffFile[]) {
|
||||
function addPRFiles(fileMap: Map<string, ReviewFile>, 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<string, FileInfo> | null,
|
||||
cumulativeDiff: CumulativeDiff | null,
|
||||
prDiffFiles?: PRDiffFile[],
|
||||
prRepoName?: string,
|
||||
useRepositoryKeys = true,
|
||||
): ReviewFile[] {
|
||||
const fileMap = new Map<string, ReviewFile>();
|
||||
// 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<string, FileInfo> | 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<string | null>(null);
|
||||
const [splitView, setSplitView] = useState(() =>
|
||||
@@ -361,8 +357,8 @@ function useReviewDialogState(props: ReviewDialogProps) {
|
||||
|
||||
const [filter, setFilter] = useState("");
|
||||
const allFiles = useMemo<ReviewFile[]>(
|
||||
() => 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(
|
||||
|
||||
@@ -68,6 +68,20 @@ function withTooltips(node: ReactNode) {
|
||||
return <TooltipProvider>{node}</TooltipProvider>;
|
||||
}
|
||||
|
||||
function renderSingleFile(reviewFile: ReviewFile, selected = true) {
|
||||
const refs = new Map([[reviewFile.path, createRef<HTMLDivElement>()]]);
|
||||
render(
|
||||
withTooltips(
|
||||
<ReviewDiffList
|
||||
{...baseProps}
|
||||
files={[reviewFile]}
|
||||
selectedFile={selected ? reviewFile.path : null}
|
||||
fileRefs={refs}
|
||||
/>,
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
describe("ReviewDiffList — multi-repo grouping", () => {
|
||||
it("renders no repo header when all files lack a repository_name", () => {
|
||||
const refs = new Map([["a.ts", createRef<HTMLDivElement>()]]);
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 (
|
||||
<div className="sticky top-0 z-10 flex items-center gap-2 px-4 py-2 bg-card/95 backdrop-blur-sm border-b border-border/50">
|
||||
<div
|
||||
data-testid="review-file-header"
|
||||
data-file-path={file.path}
|
||||
className="sticky top-0 z-10 flex items-center gap-2 px-4 py-2 bg-card/95 backdrop-blur-sm border-b border-border/50"
|
||||
>
|
||||
<Checkbox
|
||||
checked={isReviewed}
|
||||
onCheckedChange={onCheckboxChange}
|
||||
@@ -262,6 +272,7 @@ function FileDiffHeader({
|
||||
)}
|
||||
<span className="text-[13px] font-medium truncate">{file.path}</span>
|
||||
</button>
|
||||
<FileStatusIcon status={file.status} oldPath={file.old_path} className="sm:hidden" />
|
||||
{isStale && (
|
||||
<span className="flex items-center gap-1 text-xs text-yellow-500">
|
||||
<IconAlertTriangle className="h-3.5 w-3.5" />
|
||||
@@ -438,7 +449,8 @@ function renderDiffContent(opts: {
|
||||
onCommentRun,
|
||||
onToggleExpandUnchanged,
|
||||
} = opts;
|
||||
if (shouldRender && file.diff) {
|
||||
const hasText = hasTextualDiff(file);
|
||||
if (shouldRender && hasText) {
|
||||
return (
|
||||
<>
|
||||
<DiffErrorBoundary filePath={file.path}>
|
||||
@@ -468,9 +480,12 @@ function renderDiffContent(opts: {
|
||||
</>
|
||||
);
|
||||
}
|
||||
const message = hasText
|
||||
? diffSkipReasonLabel(file.diff_skip_reason)
|
||||
: reviewDiffUnavailableLabel(file);
|
||||
return (
|
||||
<div className="flex items-center justify-center py-12 text-muted-foreground text-sm">
|
||||
{diffSkipReasonLabel(file.diff_skip_reason)}
|
||||
{message}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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"]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 (
|
||||
<div
|
||||
data-testid="review-file-row"
|
||||
data-file-path={file.path}
|
||||
data-repository-name={file.repository_name}
|
||||
className={cn(
|
||||
"flex items-center gap-1.5 px-2 py-1 cursor-pointer transition-colors group",
|
||||
isSelected ? "bg-accent/50" : "hover:bg-muted/50",
|
||||
@@ -196,14 +200,17 @@ function ReviewFileRow({
|
||||
className="h-3.5 w-3.5"
|
||||
/>
|
||||
<FileIcon fileName={row.node.name} className="h-4 w-4 shrink-0" />
|
||||
<span className="text-[13px] truncate flex-1">{row.node.name}</span>
|
||||
<span data-review-file-name className="text-[13px] truncate flex-1 min-w-0">
|
||||
{row.node.name}
|
||||
</span>
|
||||
{isStale && <IconAlertTriangle className="h-3 w-3 text-yellow-500 shrink-0" />}
|
||||
{commentCount > 0 && (
|
||||
<span className="flex items-center gap-0.5 text-[10px] text-blue-500">
|
||||
<span className="flex items-center gap-0.5 text-[10px] text-blue-500 shrink-0">
|
||||
<IconMessage className="h-3 w-3" />
|
||||
{commentCount}
|
||||
</span>
|
||||
)}
|
||||
<FileStatusIcon status={file.status} oldPath={file.old_path} />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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" });
|
||||
|
||||
@@ -0,0 +1,122 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { hasTextualDiff, reviewDiffUnavailableLabel, type ReviewFile } from "./types";
|
||||
|
||||
function patchlessFile(overrides: Partial<ReviewFile>): 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<ReviewFile>]>([
|
||||
["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);
|
||||
});
|
||||
});
|
||||
@@ -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<string>,
|
||||
): boolean {
|
||||
if (taskRepositoryCount > 1) return true;
|
||||
const namedRepositories = new Set<string>();
|
||||
for (const name of repositoryNames) {
|
||||
if (name) namedRepositories.add(name);
|
||||
if (namedRepositories.size > 1) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
export function getCumulativeReviewRepositoryNames(
|
||||
files: Record<string, { repository_name?: string }> | null | undefined,
|
||||
): string[] {
|
||||
const names = new Set<string>();
|
||||
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<ReviewFile, "diff" | "status" | "additions" | "deletions">,
|
||||
): 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;
|
||||
|
||||
@@ -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(<FileStatusIcon status={status} />);
|
||||
|
||||
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(<FileStatusIcon status="renamed" oldPath="src/old-name.ts" />);
|
||||
|
||||
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(<FileStatusIcon status="modified" className="custom-marker" />);
|
||||
|
||||
const marker = screen.getByRole("img", { name: "Modified" });
|
||||
expect(marker.className).toContain("shrink-0");
|
||||
expect(marker.className).toContain("custom-marker");
|
||||
});
|
||||
});
|
||||
@@ -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<FileChangeStatus, string> = {
|
||||
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 <IconPlus aria-hidden="true" className="h-2 w-2" />;
|
||||
case "modified":
|
||||
return <IconCircleFilled aria-hidden="true" className="h-1 w-1" />;
|
||||
case "deleted":
|
||||
return <IconMinus aria-hidden="true" className="h-2 w-2" />;
|
||||
case "renamed":
|
||||
return <IconArrowRight aria-hidden="true" className="h-2 w-2" />;
|
||||
default: {
|
||||
const exhaustiveStatus: never = status;
|
||||
return exhaustiveStatus;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export function FileStatusIcon({ status, oldPath, className }: FileStatusIconProps) {
|
||||
const label = fileChangeStatusLabel(status, oldPath);
|
||||
|
||||
return (
|
||||
<span
|
||||
role="img"
|
||||
aria-label={label}
|
||||
title={label}
|
||||
data-file-status={status}
|
||||
className={cn(
|
||||
"flex h-3 w-3 shrink-0 items-center justify-center rounded border",
|
||||
markerClassName[status],
|
||||
className,
|
||||
)}
|
||||
>
|
||||
<StatusGlyph status={status} />
|
||||
</span>
|
||||
);
|
||||
}
|
||||
@@ -8,13 +8,17 @@ import { useEnvironmentSessionId } from "@/hooks/use-environment-session-id";
|
||||
import { useToast } from "@/components/toast-provider";
|
||||
import { useVcsDialogs } from "@/components/vcs/vcs-dialogs";
|
||||
import type { PRChangedFile } from "./changes-panel-timeline";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
import { useChangesGitHandlers, useChangesDialogHandlers } from "./changes-panel-hooks";
|
||||
import { useRepoDisplayName } from "@/hooks/domains/session/use-repo-display-name";
|
||||
import { useBaseBranchByRepo } from "@/hooks/domains/session/use-base-branch-by-repo";
|
||||
import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr";
|
||||
import { useActiveTaskPRsWithFiles } from "@/hooks/domains/github/use-active-task-pr-files";
|
||||
import { usePRCommits } from "@/hooks/domains/github/use-pr-commits";
|
||||
import {
|
||||
getCumulativeReviewRepositoryNames,
|
||||
isReviewMultiRepo,
|
||||
resolvePRReviewRepositoryName,
|
||||
} from "@/components/review/types";
|
||||
import {
|
||||
type ChangedFile,
|
||||
computePRGroupStamp,
|
||||
@@ -25,6 +29,8 @@ import {
|
||||
mapToChangedFiles,
|
||||
buildPrByRepoMap,
|
||||
buildRepoNameById,
|
||||
selectPRFilesForReviewProgress,
|
||||
type ReviewProgressPRSource,
|
||||
} from "./changes-panel-helpers";
|
||||
import type { OpenDiffOptions } from "./changes-diff-target";
|
||||
|
||||
@@ -148,16 +154,18 @@ function usePerRepoCallbacks(
|
||||
);
|
||||
}
|
||||
|
||||
function useChangesPanelPRData() {
|
||||
function useChangesPanelPRData(repositoryNames: string[]) {
|
||||
const { prs, filesByPRKey } = useActiveTaskPRsWithFiles();
|
||||
const reposByWorkspace = useAppStore((s) => s.repositories.itemsByWorkspaceId);
|
||||
const repoNameById = useMemo(() => buildRepoNameById(reposByWorkspace), [reposByWorkspace]);
|
||||
const taskHasMultipleRepos = useAppStore((s) => {
|
||||
const taskRepositoryCount = useAppStore((s) => {
|
||||
const taskId = s.tasks.activeTaskId;
|
||||
if (!taskId) return false;
|
||||
if (!taskId) return 0;
|
||||
const task = s.kanban.tasks.find((t: { id: string }) => t.id === taskId);
|
||||
return (task?.repositories?.length ?? 0) > 1;
|
||||
return task?.repositories?.length ?? 0;
|
||||
});
|
||||
const taskHasMultipleRepos = taskRepositoryCount > 1;
|
||||
const useRepositoryKeys = isReviewMultiRepo(taskRepositoryCount, repositoryNames);
|
||||
const taskPR = useActiveTaskPR();
|
||||
const refreshKey = taskPR?.last_synced_at ?? null;
|
||||
const { commits: prCommitsList } = usePRCommits(
|
||||
@@ -168,7 +176,7 @@ function useChangesPanelPRData() {
|
||||
);
|
||||
const { prFiles, prDiffFiles } = useMemo(() => {
|
||||
const merged: PRChangedFile[] = [];
|
||||
const flat: PRDiffFile[] = [];
|
||||
const progressSources = new Map<string, ReviewProgressPRSource>();
|
||||
// Multi-branch tasks open multiple PRs on the same repo — files must
|
||||
// split by PR so the agent's two branches don't get conflated under one
|
||||
// "kandev" header. We stamp each row with a label unique per PR.
|
||||
@@ -200,13 +208,28 @@ function useChangesPanelPRData() {
|
||||
prNumber: pr.pr_number,
|
||||
});
|
||||
merged.push(...mapPRFilesToChangedFiles(files, stamp));
|
||||
flat.push(...files);
|
||||
progressSources.set(pr.id, {
|
||||
repositoryName: resolvePRReviewRepositoryName(pr, repoName || undefined) ?? pr.repo,
|
||||
files,
|
||||
});
|
||||
}
|
||||
return { prFiles: merged, prDiffFiles: flat };
|
||||
}, [prs, filesByPRKey, repoNameById, taskHasMultipleRepos]);
|
||||
const progressFiles = selectPRFilesForReviewProgress(
|
||||
progressSources,
|
||||
taskPR?.id,
|
||||
useRepositoryKeys,
|
||||
);
|
||||
return { prFiles: merged, prDiffFiles: progressFiles };
|
||||
}, [prs, filesByPRKey, repoNameById, taskHasMultipleRepos, taskPR?.id, useRepositoryKeys]);
|
||||
const hasPRFiles = prFiles.length > 0;
|
||||
const hasPRCommits = prCommitsList.length > 0;
|
||||
return { prDiffFiles, prCommitsList, hasPRFiles, hasPRCommits, prFiles };
|
||||
return {
|
||||
prDiffFiles,
|
||||
prCommitsList,
|
||||
hasPRFiles,
|
||||
hasPRCommits,
|
||||
prFiles,
|
||||
useRepositoryKeys,
|
||||
};
|
||||
}
|
||||
|
||||
function hasCumulativeFiles(files: Record<string, unknown> | null | undefined): boolean {
|
||||
@@ -219,14 +242,25 @@ export function useChangesPanelData() {
|
||||
const git = useSessionGit(activeSessionId);
|
||||
const { toast } = useToast();
|
||||
const { reviews } = useSessionFileReviews(activeSessionId);
|
||||
const prData = useChangesPanelPRData();
|
||||
const reviewRepositoryNames = useMemo(
|
||||
() => [...git.repoNames, ...getCumulativeReviewRepositoryNames(git.cumulativeDiff?.files)],
|
||||
[git.repoNames, git.cumulativeDiff],
|
||||
);
|
||||
const prData = useChangesPanelPRData(reviewRepositoryNames);
|
||||
const vcsDialogs = useVcsDialogs();
|
||||
const baseBranchDisplay = useMemo(() => getBaseBranchDisplay(baseBranch), [baseBranch]);
|
||||
const unstagedFiles = useMemo(() => mapToChangedFiles(git.unstagedFiles), [git.unstagedFiles]);
|
||||
const stagedFiles = useMemo(() => mapToChangedFiles(git.stagedFiles), [git.stagedFiles]);
|
||||
const { reviewedCount, totalFileCount } = useMemo(
|
||||
() => computeReviewProgress(git.allFiles, git.cumulativeDiff, reviews, prData.prDiffFiles),
|
||||
[git.allFiles, git.cumulativeDiff, reviews, prData.prDiffFiles],
|
||||
() =>
|
||||
computeReviewProgress(
|
||||
git.allFiles,
|
||||
git.cumulativeDiff,
|
||||
reviews,
|
||||
prData.prDiffFiles,
|
||||
prData.useRepositoryKeys,
|
||||
),
|
||||
[git.allFiles, git.cumulativeDiff, reviews, prData.prDiffFiles, prData.useRepositoryKeys],
|
||||
);
|
||||
const staged = useMemo(() => computeStagedStats(git.stagedFiles), [git.stagedFiles]);
|
||||
const gitHandlers = useChangesGitHandlers(git, toast, baseBranch);
|
||||
|
||||
@@ -143,6 +143,35 @@ describe("FileRow truncation (regression: path overlaps diff stats in narrow pan
|
||||
});
|
||||
});
|
||||
|
||||
describe("FileRow status marker", () => {
|
||||
it("shows previous-path context for a moved file", () => {
|
||||
const { container } = render(
|
||||
<TooltipProvider>
|
||||
<ul>
|
||||
<FileRow
|
||||
file={{
|
||||
...baseFile,
|
||||
path: "src/new-name.ts",
|
||||
status: "renamed",
|
||||
oldPath: "src/old-name.ts",
|
||||
}}
|
||||
isPending={false}
|
||||
onSelect={noopSelect}
|
||||
onOpenDiff={noop}
|
||||
onStage={noop}
|
||||
onUnstage={noop}
|
||||
onDiscard={noop}
|
||||
onEditFile={noop}
|
||||
/>
|
||||
</ul>
|
||||
</TooltipProvider>,
|
||||
);
|
||||
|
||||
const marker = container.querySelector("[data-file-status='renamed']");
|
||||
expect(marker?.getAttribute("aria-label")).toBe("Moved from src/old-name.ts");
|
||||
});
|
||||
});
|
||||
|
||||
describe("FileRow hover swap (stats <-> actions occupy same cell)", () => {
|
||||
// Goal: hovering must not shift the file name. The stats group and the
|
||||
// hover-actions group live in the same CSS grid cell — stats fade out and
|
||||
|
||||
@@ -13,9 +13,9 @@ import { Button } from "@kandev/ui/button";
|
||||
import { cn } from "@/lib/utils";
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip";
|
||||
import { LineStat } from "@/components/diff-stat";
|
||||
import { FileStatusIcon } from "@/components/shared/file-status-icon";
|
||||
import { FileIcon } from "@/components/ui/file-icon";
|
||||
import { getFileCategory } from "@/lib/utils/file-types";
|
||||
import { FileStatusIcon } from "./file-status-icon";
|
||||
import type { ChangedFile } from "./changes-panel-helpers";
|
||||
import type { OpenDiffOptions } from "./changes-diff-target";
|
||||
|
||||
@@ -139,7 +139,7 @@ export function FileRow({
|
||||
<div className="grid items-center shrink-0 [&>*]:col-start-1 [&>*]:row-start-1">
|
||||
<div className="flex items-center gap-2 justify-end transition-opacity group-hover:opacity-0 pointer-events-none">
|
||||
<LineStat added={file.plus} removed={file.minus} />
|
||||
<FileStatusIcon status={file.status} />
|
||||
<FileStatusIcon status={file.status} oldPath={file.oldPath} />
|
||||
</div>
|
||||
<FileRowActions
|
||||
path={file.path}
|
||||
|
||||
@@ -1,7 +1,17 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { buildPrByRepoMap, mapPRFilesToChangedFiles } from "./changes-panel-helpers";
|
||||
import {
|
||||
buildPrByRepoMap,
|
||||
computeReviewProgress,
|
||||
mapPRFilesToChangedFiles,
|
||||
selectPRFilesForReviewProgress,
|
||||
type ReviewProgressPRFile,
|
||||
} from "./changes-panel-helpers";
|
||||
import { hashDiff, resolvePRReviewRepositoryName, reviewFileKey } from "@/components/review/types";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
|
||||
const PRIMARY_PR_ID = "primary-pr";
|
||||
const RENAMED_PATH = "src/renamed.ts";
|
||||
|
||||
function diffFile(overrides: Partial<PRDiffFile>): PRDiffFile {
|
||||
return {
|
||||
filename: "src/app.ts",
|
||||
@@ -13,6 +23,28 @@ function diffFile(overrides: Partial<PRDiffFile>): PRDiffFile {
|
||||
} as PRDiffFile;
|
||||
}
|
||||
|
||||
function progressPRFile(
|
||||
overrides: Partial<PRDiffFile> & { repository_name?: string },
|
||||
): ReviewProgressPRFile {
|
||||
return {
|
||||
...diffFile(overrides),
|
||||
repository_name: overrides.repository_name,
|
||||
};
|
||||
}
|
||||
|
||||
function progressPRSource(repositoryName: string, filename: string) {
|
||||
return { repositoryName, files: [diffFile({ filename })] };
|
||||
}
|
||||
|
||||
type ProgressLocalFile = Parameters<typeof computeReviewProgress>[0][number];
|
||||
|
||||
function progressLocalFile(
|
||||
path: string,
|
||||
overrides: Partial<ProgressLocalFile> = {},
|
||||
): ProgressLocalFile {
|
||||
return { path, status: "modified", staged: false, diff: "", ...overrides };
|
||||
}
|
||||
|
||||
describe("mapPRFilesToChangedFiles", () => {
|
||||
it("maps GitHub status strings to FileInfo statuses", () => {
|
||||
const out = mapPRFilesToChangedFiles([
|
||||
@@ -20,8 +52,10 @@ describe("mapPRFilesToChangedFiles", () => {
|
||||
diffFile({ filename: "b.ts", status: "removed" }),
|
||||
diffFile({ filename: "c.ts", status: "renamed", old_path: "old/c.ts" }),
|
||||
diffFile({ filename: "d.ts", status: "modified" }),
|
||||
// Anything not in the explicit set should fall through to "modified".
|
||||
diffFile({ filename: "e.ts", status: "weird" as PRDiffFile["status"] }),
|
||||
diffFile({ filename: "e.ts", status: "copied" }),
|
||||
diffFile({ filename: "f.ts", status: "changed" }),
|
||||
diffFile({ filename: "g.ts", status: "unchanged" }),
|
||||
diffFile({ filename: "h.ts", status: "weird" as PRDiffFile["status"] }),
|
||||
]);
|
||||
expect(out.map((f) => f.status)).toEqual([
|
||||
"added",
|
||||
@@ -29,6 +63,9 @@ describe("mapPRFilesToChangedFiles", () => {
|
||||
"renamed",
|
||||
"modified",
|
||||
"modified",
|
||||
"modified",
|
||||
"modified",
|
||||
"modified",
|
||||
]);
|
||||
});
|
||||
|
||||
@@ -69,6 +106,285 @@ describe("mapPRFilesToChangedFiles", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("selectPRFilesForReviewProgress", () => {
|
||||
it("selects only the primary PR and qualifies it in a multi-repo task", () => {
|
||||
const repositoryName = resolvePRReviewRepositoryName(
|
||||
{ repository_id: "repo-frontend", repo: "widgets" },
|
||||
"acme/widgets",
|
||||
);
|
||||
const files = selectPRFilesForReviewProgress(
|
||||
new Map([
|
||||
[PRIMARY_PR_ID, progressPRSource(repositoryName ?? "widgets", "README.md")],
|
||||
["secondary-pr", progressPRSource("backend", "README.md")],
|
||||
]),
|
||||
PRIMARY_PR_ID,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(files.map((file) => reviewFileKey({ path: file.filename, ...file }))).toEqual([
|
||||
"acme-widgets\u0000README.md",
|
||||
]);
|
||||
});
|
||||
|
||||
it("keeps the selected primary PR bare in a legacy single-repo task", () => {
|
||||
const files = selectPRFilesForReviewProgress(
|
||||
new Map([[PRIMARY_PR_ID, progressPRSource("kandev", "README.md")]]),
|
||||
PRIMARY_PR_ID,
|
||||
false,
|
||||
);
|
||||
|
||||
expect(files.map((file) => reviewFileKey({ path: file.filename, ...file }))).toEqual([
|
||||
"README.md",
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeReviewProgress PR files", () => {
|
||||
it("counts same-path PR reviews per repository using each patch hash", () => {
|
||||
const path = "README.md";
|
||||
const frontendPatch = "@@ -1 +1 @@\n-frontend old\n+frontend new";
|
||||
const backendPatch = "@@ -1 +1 @@\n-backend old\n+backend new";
|
||||
const frontendKey = reviewFileKey({ path, repository_name: "frontend" });
|
||||
const backendKey = reviewFileKey({ path, repository_name: "backend" });
|
||||
|
||||
const progress = computeReviewProgress(
|
||||
[],
|
||||
null,
|
||||
new Map([
|
||||
[frontendKey, { reviewed: true, diffHash: hashDiff(frontendPatch) }],
|
||||
[backendKey, { reviewed: true, diffHash: hashDiff(backendPatch) }],
|
||||
]),
|
||||
[
|
||||
progressPRFile({ filename: path, patch: frontendPatch, repository_name: "frontend" }),
|
||||
progressPRFile({ filename: path, patch: backendPatch, repository_name: "backend" }),
|
||||
],
|
||||
);
|
||||
|
||||
expect(progress).toEqual({ reviewedCount: 2, totalFileCount: 2 });
|
||||
});
|
||||
|
||||
it("keeps a same-repo uncommitted file over PR data without dropping another repo", () => {
|
||||
const path = "README.md";
|
||||
const frontendDiff = "@@ -1 +1 @@\n-local old\n+local new";
|
||||
const backendPatch = "@@ -1 +1 @@\n-backend old\n+backend new";
|
||||
const frontendKey = reviewFileKey({ path, repository_name: "frontend" });
|
||||
const backendKey = reviewFileKey({ path, repository_name: "backend" });
|
||||
|
||||
const progress = computeReviewProgress(
|
||||
[progressLocalFile(path, { diff: frontendDiff, repository_name: "frontend" })],
|
||||
null,
|
||||
new Map([
|
||||
[frontendKey, { reviewed: true, diffHash: hashDiff(frontendDiff) }],
|
||||
[backendKey, { reviewed: true, diffHash: hashDiff(backendPatch) }],
|
||||
]),
|
||||
[
|
||||
progressPRFile({
|
||||
filename: path,
|
||||
patch: "@@ -1 +1 @@\n-pr old\n+pr new",
|
||||
repository_name: "frontend",
|
||||
}),
|
||||
progressPRFile({ filename: path, patch: backendPatch, repository_name: "backend" }),
|
||||
],
|
||||
);
|
||||
|
||||
expect(progress).toEqual({ reviewedCount: 2, totalFileCount: 2 });
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeReviewProgress rejected source precedence", () => {
|
||||
const uncommittedWinner: Parameters<typeof computeReviewProgress>[0] = [
|
||||
progressLocalFile(RENAMED_PATH, { status: "renamed", old_path: "src/old.ts" }),
|
||||
];
|
||||
const cumulativeWinner: Parameters<typeof computeReviewProgress>[1] = {
|
||||
files: { [RENAMED_PATH]: { path: RENAMED_PATH, status: "renamed", diff: "" } },
|
||||
};
|
||||
|
||||
it.each([
|
||||
{ name: "uncommitted", uncommittedFiles: uncommittedWinner, cumulativeDiff: null },
|
||||
{ name: "cumulative", uncommittedFiles: [], cumulativeDiff: cumulativeWinner },
|
||||
])("does not hash a rejected PR patch past a patchless $name winner", (winner) => {
|
||||
const progress = computeReviewProgress(
|
||||
winner.uncommittedFiles,
|
||||
winner.cumulativeDiff,
|
||||
new Map([[RENAMED_PATH, { reviewed: true, diffHash: hashDiff("") }]]),
|
||||
[diffFile({ filename: RENAMED_PATH, patch: "@@rejected@@" })],
|
||||
);
|
||||
|
||||
expect(progress).toEqual({ reviewedCount: 1, totalFileCount: 1 });
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeReviewProgress source precedence", () => {
|
||||
it("keeps bare uncommitted precedence over a stamped cumulative file at the same path", () => {
|
||||
const path = RENAMED_PATH;
|
||||
const progress = computeReviewProgress(
|
||||
[
|
||||
{
|
||||
path,
|
||||
status: "renamed",
|
||||
staged: false,
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: "src/old.ts",
|
||||
diff: "",
|
||||
},
|
||||
],
|
||||
{
|
||||
files: {
|
||||
[`frontend\u0000${path}`]: {
|
||||
path,
|
||||
repository_name: "frontend",
|
||||
diff: "@@ -1 +1 @@\n-old\n+new",
|
||||
},
|
||||
},
|
||||
},
|
||||
new Map([[path, { reviewed: true }]]),
|
||||
);
|
||||
|
||||
expect(progress).toEqual({ reviewedCount: 1, totalFileCount: 1 });
|
||||
});
|
||||
|
||||
it("keeps same-path composite keys from different repositories distinct", () => {
|
||||
const path = "README.md";
|
||||
const progress = computeReviewProgress(
|
||||
[
|
||||
{
|
||||
path,
|
||||
status: "modified",
|
||||
staged: false,
|
||||
diff: "",
|
||||
repository_name: "frontend",
|
||||
},
|
||||
],
|
||||
{
|
||||
files: {
|
||||
[`backend\u0000${path}`]: {
|
||||
path,
|
||||
repository_name: "backend",
|
||||
diff: "",
|
||||
},
|
||||
},
|
||||
},
|
||||
new Map(),
|
||||
);
|
||||
|
||||
expect(progress.totalFileCount).toBe(2);
|
||||
});
|
||||
|
||||
it("counts a reviewed patchless file under its repository-qualified review key", () => {
|
||||
const file = {
|
||||
path: RENAMED_PATH,
|
||||
status: "renamed" as const,
|
||||
staged: false,
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: "src/old.ts",
|
||||
diff: "",
|
||||
repository_name: "frontend",
|
||||
};
|
||||
const key = reviewFileKey(file);
|
||||
|
||||
expect(computeReviewProgress([file], null, new Map([[key, { reviewed: true }]]))).toEqual({
|
||||
reviewedCount: 1,
|
||||
totalFileCount: 1,
|
||||
});
|
||||
});
|
||||
|
||||
it("counts patchless-only status and PR files", () => {
|
||||
const progress = computeReviewProgress(
|
||||
[
|
||||
{
|
||||
path: "src/local-renamed.ts",
|
||||
status: "renamed",
|
||||
staged: false,
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: "src/local-old.ts",
|
||||
diff: "",
|
||||
},
|
||||
],
|
||||
null,
|
||||
new Map(),
|
||||
[
|
||||
diffFile({
|
||||
filename: "src/pr-renamed.ts",
|
||||
status: "renamed",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: "src/pr-old.ts",
|
||||
patch: "",
|
||||
}),
|
||||
],
|
||||
);
|
||||
|
||||
expect(progress.totalFileCount).toBe(2);
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeReviewProgress repository key mode", () => {
|
||||
it("uses bare local, cumulative, and PR keys for a single named repository", () => {
|
||||
const localPath = "src/local.ts";
|
||||
const cumulativePath = "src/cumulative.ts";
|
||||
const prPath = "src/pr.ts";
|
||||
const progress = computeReviewProgress(
|
||||
[progressLocalFile(localPath, { repository_name: "frontend", diff: "@@local@@" })],
|
||||
{
|
||||
files: {
|
||||
[`frontend\u0000${cumulativePath}`]: {
|
||||
path: cumulativePath,
|
||||
repository_name: "frontend",
|
||||
diff: "@@cumulative@@",
|
||||
},
|
||||
},
|
||||
},
|
||||
new Map([
|
||||
[localPath, { reviewed: true }],
|
||||
[cumulativePath, { reviewed: true }],
|
||||
[prPath, { reviewed: true }],
|
||||
]),
|
||||
[progressPRFile({ filename: prPath, patch: "@@pr@@" })],
|
||||
false,
|
||||
);
|
||||
|
||||
expect(progress).toEqual({ reviewedCount: 3, totalFileCount: 3 });
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeReviewProgress scalability", () => {
|
||||
it("indexes cumulative file identities once for large review sets", () => {
|
||||
const fileCount = 80;
|
||||
let pathReads = 0;
|
||||
const files = Object.fromEntries(
|
||||
Array.from({ length: fileCount }, (_, index) => {
|
||||
const path = `src/file-${index}.ts`;
|
||||
return [
|
||||
`frontend\u0000${path}`,
|
||||
{
|
||||
get path() {
|
||||
pathReads++;
|
||||
return path;
|
||||
},
|
||||
repository_name: "frontend",
|
||||
diff: `@@ -1 +1 @@\n-old-${index}\n+new-${index}`,
|
||||
},
|
||||
];
|
||||
}),
|
||||
);
|
||||
const reviews = new Map(
|
||||
Array.from({ length: fileCount }, (_, index) => [
|
||||
`frontend\u0000src/file-${index}.ts`,
|
||||
{ reviewed: true },
|
||||
]),
|
||||
);
|
||||
|
||||
expect(computeReviewProgress([], { files }, reviews)).toEqual({
|
||||
reviewedCount: fileCount,
|
||||
totalFileCount: fileCount,
|
||||
});
|
||||
expect(pathReads).toBeLessThanOrEqual(fileCount * 4);
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildPrByRepoMap", () => {
|
||||
it("does not copy a multi-repo TaskPR URL into the empty-key fallback", () => {
|
||||
const map = buildPrByRepoMap(
|
||||
|
||||
@@ -1,13 +1,19 @@
|
||||
"use client";
|
||||
|
||||
import { hashDiff, normalizeDiffContent } from "@/components/review/types";
|
||||
import {
|
||||
hashDiff,
|
||||
normalizeDiffContent,
|
||||
reviewFileKey,
|
||||
splitReviewFileKey,
|
||||
} from "@/components/review/types";
|
||||
import type { FileInfo } from "@/lib/state/store";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
import { normalizeFileChangeStatus, type FileChangeStatus } from "@/lib/utils/file-change-status";
|
||||
import type { PRChangedFile } from "./changes-panel-timeline";
|
||||
|
||||
export type ChangedFile = {
|
||||
path: string;
|
||||
status: FileInfo["status"];
|
||||
status: FileChangeStatus;
|
||||
staged: boolean;
|
||||
plus: number | undefined;
|
||||
minus: number | undefined;
|
||||
@@ -59,65 +65,137 @@ export function mapToChangedFiles(files: FileInfo[]): ChangedFile[] {
|
||||
|
||||
type CumulativeDiffFiles = Record<
|
||||
string,
|
||||
{ diff?: string; status?: string; additions?: number; deletions?: number }
|
||||
{
|
||||
path?: string;
|
||||
repository_name?: string;
|
||||
diff?: string;
|
||||
status?: string;
|
||||
additions?: number;
|
||||
deletions?: number;
|
||||
}
|
||||
>;
|
||||
|
||||
function collectReviewPaths(
|
||||
function cumulativeFileIdentity(
|
||||
mapKey: string,
|
||||
file: CumulativeDiffFiles[string],
|
||||
useRepositoryKeys: boolean,
|
||||
) {
|
||||
const path = file.path ?? splitReviewFileKey(mapKey).path;
|
||||
const repositoryName = useRepositoryKeys ? file.repository_name : undefined;
|
||||
return {
|
||||
key: reviewFileKey({ path, repository_name: repositoryName }),
|
||||
path,
|
||||
repositoryName,
|
||||
};
|
||||
}
|
||||
|
||||
export type ReviewProgressPRFile = PRDiffFile & { repository_name?: string };
|
||||
|
||||
export type ReviewProgressPRSource = {
|
||||
repositoryName: string;
|
||||
files: PRDiffFile[];
|
||||
};
|
||||
|
||||
export function selectPRFilesForReviewProgress(
|
||||
sourcesByPRID: ReadonlyMap<string, ReviewProgressPRSource>,
|
||||
primaryPRID: string | undefined,
|
||||
useRepositoryKeys: boolean,
|
||||
): ReviewProgressPRFile[] {
|
||||
if (!primaryPRID) return [];
|
||||
const source = sourcesByPRID.get(primaryPRID);
|
||||
if (!source) return [];
|
||||
return source.files.map((file) => ({
|
||||
...file,
|
||||
repository_name: useRepositoryKeys ? source.repositoryName : undefined,
|
||||
}));
|
||||
}
|
||||
|
||||
function addReviewSource(
|
||||
winningDiffs: Map<string, string | undefined>,
|
||||
paths: Set<string>,
|
||||
source: {
|
||||
key: string;
|
||||
path: string;
|
||||
repositoryName?: string;
|
||||
diff?: string;
|
||||
},
|
||||
): void {
|
||||
const collidesWithHigherPriority = source.repositoryName
|
||||
? winningDiffs.has(source.path)
|
||||
: paths.has(source.path);
|
||||
if (winningDiffs.has(source.key) || collidesWithHigherPriority) return;
|
||||
winningDiffs.set(source.key, source.diff);
|
||||
paths.add(source.path);
|
||||
}
|
||||
|
||||
function buildReviewProgressIndex(
|
||||
uncommittedFiles: FileInfo[],
|
||||
cumulativeDiffFiles: CumulativeDiffFiles | undefined,
|
||||
prFiles?: PRDiffFile[],
|
||||
): Set<string> {
|
||||
prFiles?: ReviewProgressPRFile[],
|
||||
useRepositoryKeys = true,
|
||||
): Map<string, string | undefined> {
|
||||
const winningDiffs = new Map<string, string | undefined>();
|
||||
const paths = new Set<string>();
|
||||
for (const file of uncommittedFiles) {
|
||||
if (file.diff && normalizeDiffContent(file.diff)) paths.add(file.path);
|
||||
const repositoryName = useRepositoryKeys ? file.repository_name : undefined;
|
||||
const key = reviewFileKey({ path: file.path, repository_name: repositoryName });
|
||||
addReviewSource(winningDiffs, paths, {
|
||||
key,
|
||||
path: file.path,
|
||||
repositoryName,
|
||||
diff: file.diff,
|
||||
});
|
||||
}
|
||||
if (cumulativeDiffFiles) {
|
||||
for (const [path, file] of Object.entries(cumulativeDiffFiles)) {
|
||||
if (!paths.has(path) && file.diff && normalizeDiffContent(file.diff)) paths.add(path);
|
||||
for (const [mapKey, file] of Object.entries(cumulativeDiffFiles)) {
|
||||
const identity = cumulativeFileIdentity(mapKey, file, useRepositoryKeys);
|
||||
addReviewSource(winningDiffs, paths, {
|
||||
...identity,
|
||||
diff: file.diff,
|
||||
});
|
||||
}
|
||||
}
|
||||
if (prFiles) {
|
||||
for (const file of prFiles) {
|
||||
if (!paths.has(file.filename) && file.patch) paths.add(file.filename);
|
||||
const repositoryName = useRepositoryKeys ? file.repository_name : undefined;
|
||||
const key = reviewFileKey({
|
||||
path: file.filename,
|
||||
repository_name: repositoryName,
|
||||
});
|
||||
addReviewSource(winningDiffs, paths, {
|
||||
key,
|
||||
path: file.filename,
|
||||
repositoryName,
|
||||
diff: file.patch,
|
||||
});
|
||||
}
|
||||
}
|
||||
return paths;
|
||||
}
|
||||
|
||||
function getDiffForPath(
|
||||
path: string,
|
||||
uncommittedFiles: FileInfo[],
|
||||
cumulativeDiffFiles: CumulativeDiffFiles | undefined,
|
||||
prFiles?: PRDiffFile[],
|
||||
): string {
|
||||
const uncommitted = uncommittedFiles.find((f) => f.path === path);
|
||||
if (uncommitted?.diff) return normalizeDiffContent(uncommitted.diff);
|
||||
const cumDiff = cumulativeDiffFiles?.[path]?.diff;
|
||||
if (cumDiff) return normalizeDiffContent(cumDiff);
|
||||
if (prFiles) {
|
||||
const prFile = prFiles.find((f) => f.filename === path);
|
||||
if (prFile?.patch) return normalizeDiffContent(prFile.patch);
|
||||
}
|
||||
return "";
|
||||
return winningDiffs;
|
||||
}
|
||||
|
||||
export function computeReviewProgress(
|
||||
uncommittedFiles: FileInfo[],
|
||||
cumulativeDiff: { files?: CumulativeDiffFiles } | null,
|
||||
reviews: Map<string, { reviewed: boolean; diffHash?: string }>,
|
||||
prFiles?: PRDiffFile[],
|
||||
prFiles?: ReviewProgressPRFile[],
|
||||
useRepositoryKeys = true,
|
||||
) {
|
||||
const cumulativeDiffFiles = cumulativeDiff?.files;
|
||||
const paths = collectReviewPaths(uncommittedFiles, cumulativeDiffFiles, prFiles);
|
||||
const winningDiffs = buildReviewProgressIndex(
|
||||
uncommittedFiles,
|
||||
cumulativeDiffFiles,
|
||||
prFiles,
|
||||
useRepositoryKeys,
|
||||
);
|
||||
let reviewed = 0;
|
||||
for (const path of paths) {
|
||||
const state = reviews.get(path);
|
||||
for (const [key, diff] of winningDiffs) {
|
||||
const state = reviews.get(key);
|
||||
if (!state?.reviewed) continue;
|
||||
const diffContent = getDiffForPath(path, uncommittedFiles, cumulativeDiffFiles, prFiles);
|
||||
const diffContent = normalizeDiffContent(diff ?? "");
|
||||
if (diffContent && state.diffHash && state.diffHash !== hashDiff(diffContent)) continue;
|
||||
reviewed++;
|
||||
}
|
||||
return { reviewedCount: reviewed, totalFileCount: paths.size };
|
||||
return { reviewedCount: reviewed, totalFileCount: winningDiffs.size };
|
||||
}
|
||||
|
||||
export function computeStagedStats(stagedFiles: FileInfo[]) {
|
||||
@@ -130,30 +208,14 @@ export function mapPRFilesToChangedFiles(
|
||||
files: PRDiffFile[],
|
||||
repositoryName?: string,
|
||||
): PRChangedFile[] {
|
||||
return files.map((file) => {
|
||||
let status: FileInfo["status"];
|
||||
switch (file.status) {
|
||||
case "added":
|
||||
status = "added";
|
||||
break;
|
||||
case "removed":
|
||||
status = "deleted";
|
||||
break;
|
||||
case "renamed":
|
||||
status = "renamed";
|
||||
break;
|
||||
default:
|
||||
status = "modified";
|
||||
}
|
||||
return {
|
||||
path: file.filename,
|
||||
status,
|
||||
plus: file.additions,
|
||||
minus: file.deletions,
|
||||
oldPath: file.old_path,
|
||||
repository_name: repositoryName ?? "",
|
||||
};
|
||||
});
|
||||
return files.map((file) => ({
|
||||
path: file.filename,
|
||||
status: normalizeFileChangeStatus(file.status),
|
||||
plus: file.additions,
|
||||
minus: file.deletions,
|
||||
oldPath: file.old_path,
|
||||
repository_name: repositoryName ?? "",
|
||||
}));
|
||||
}
|
||||
|
||||
export function filterUnpushedCommits<T extends { commit_sha: string }>(
|
||||
|
||||
@@ -54,4 +54,24 @@ describe("PRFilesGroupedList", () => {
|
||||
repositoryName: undefined,
|
||||
});
|
||||
});
|
||||
|
||||
it("shows previous-path context for a moved PR file", () => {
|
||||
render(
|
||||
<PRFilesGroupedList
|
||||
files={[
|
||||
{
|
||||
path: "src/new-name.ts",
|
||||
status: "renamed",
|
||||
plus: 0,
|
||||
minus: 0,
|
||||
oldPath: "src/old-name.ts",
|
||||
repository_name: "",
|
||||
},
|
||||
]}
|
||||
onOpenDiff={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.getByRole("img", { name: "Moved from src/old-name.ts" })).toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -3,9 +3,9 @@
|
||||
import { useState } from "react";
|
||||
import { IconChevronDown, IconChevronRight, IconGitPullRequest } from "@tabler/icons-react";
|
||||
import { LineStat } from "@/components/diff-stat";
|
||||
import { FileStatusIcon } from "@/components/shared/file-status-icon";
|
||||
import { cn } from "@/lib/utils";
|
||||
import { useDockviewStore } from "@/lib/state/dockview-store";
|
||||
import { FileStatusIcon } from "./file-status-icon";
|
||||
import { groupByRepositoryName } from "@/lib/group-by-repo";
|
||||
import type { PRChangedFile } from "./changes-panel-timeline";
|
||||
import type { OpenDiffOptions } from "./changes-diff-target";
|
||||
@@ -145,7 +145,7 @@ function PRFileRow({
|
||||
</div>
|
||||
<div className="flex items-center gap-2">
|
||||
<LineStat added={file.plus} removed={file.minus} />
|
||||
<FileStatusIcon status={file.status} />
|
||||
<FileStatusIcon status={file.status} oldPath={file.oldPath} />
|
||||
</div>
|
||||
</li>
|
||||
);
|
||||
|
||||
@@ -116,6 +116,7 @@ export function ChangesTab(props: IDockviewPanelHeaderProps) {
|
||||
<ContextMenu>
|
||||
<ContextMenuTrigger
|
||||
className="flex h-full items-center cursor-pointer select-none"
|
||||
data-testid="dockview-tab-changes"
|
||||
onDoubleClick={onDoubleClick}
|
||||
>
|
||||
<div className={cn("relative", isFlashing && "animate-changes-flash")}>
|
||||
|
||||
@@ -150,6 +150,7 @@ function ChangesTopBarRight({
|
||||
size="sm"
|
||||
variant="ghost"
|
||||
className="px-1.5 h-5 cursor-pointer"
|
||||
aria-label="Expand review"
|
||||
onClick={() => window.dispatchEvent(new CustomEvent("open-review-dialog"))}
|
||||
>
|
||||
<IconArrowsMaximize className="h-3.5 w-3.5" />
|
||||
|
||||
@@ -24,6 +24,8 @@ export function DockviewReviewDialog({ sessionId, review }: DockviewReviewDialog
|
||||
gitStatusFiles={review.reviewGitStatusFiles}
|
||||
cumulativeDiff={review.reviewCumulativeDiff}
|
||||
prDiffFiles={review.reviewPRDiffFiles}
|
||||
prRepoName={review.reviewPRRepoName}
|
||||
useRepositoryKeys={review.reviewUseRepositoryKeys}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,44 +0,0 @@
|
||||
"use client";
|
||||
|
||||
import { IconPlus, IconCircleFilled, IconMinus } from "@tabler/icons-react";
|
||||
import type { FileInfo } from "@/lib/state/store";
|
||||
|
||||
type FileStatusIconProps = {
|
||||
status: FileInfo["status"];
|
||||
};
|
||||
|
||||
export function FileStatusIcon({ status }: FileStatusIconProps) {
|
||||
switch (status) {
|
||||
case "added":
|
||||
case "untracked":
|
||||
return (
|
||||
<div className="flex items-center justify-center h-3 w-3 rounded border border-emerald-600">
|
||||
<IconPlus className="h-2 w-2 text-emerald-600" />
|
||||
</div>
|
||||
);
|
||||
case "modified":
|
||||
return (
|
||||
<div className="flex items-center justify-center h-3 w-3 rounded border border-yellow-600">
|
||||
<IconCircleFilled className="h-1 w-1 text-yellow-600" />
|
||||
</div>
|
||||
);
|
||||
case "deleted":
|
||||
return (
|
||||
<div className="flex items-center justify-center h-3 w-3 rounded border border-rose-600">
|
||||
<IconMinus className="h-2 w-2 text-rose-600" />
|
||||
</div>
|
||||
);
|
||||
case "renamed":
|
||||
return (
|
||||
<div className="flex items-center justify-center h-3 w-3 rounded border border-purple-600">
|
||||
<IconCircleFilled className="h-1 w-1 text-purple-600" />
|
||||
</div>
|
||||
);
|
||||
default:
|
||||
return (
|
||||
<div className="flex items-center justify-center h-3 w-3 rounded border border-muted-foreground">
|
||||
<IconCircleFilled className="h-1 w-1 text-muted-foreground" />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -255,6 +255,8 @@ function MobileReviewDialogMount({
|
||||
gitStatusFiles={review.reviewGitStatusFiles}
|
||||
cumulativeDiff={review.reviewCumulativeDiff}
|
||||
prDiffFiles={review.reviewPRDiffFiles}
|
||||
prRepoName={review.reviewPRRepoName}
|
||||
useRepositoryKeys={review.reviewUseRepositoryKeys}
|
||||
/>
|
||||
<WalkthroughOverlay
|
||||
taskId={activeTaskId}
|
||||
|
||||
@@ -0,0 +1,109 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { buildReviewGitStatusFiles } from "./use-review-dialog";
|
||||
import type { FileInfo, GitStatusEntry } from "@/lib/state/slices/session-runtime/types";
|
||||
|
||||
type GitStatusByRepo = Array<{ repository_name: string; status: GitStatusEntry }>;
|
||||
|
||||
const README_PATH = "README.md";
|
||||
|
||||
function file(path = README_PATH): FileInfo {
|
||||
return { path, status: "modified", staged: false };
|
||||
}
|
||||
|
||||
function status(files: Record<string, FileInfo>): GitStatusEntry {
|
||||
return { files } as GitStatusEntry;
|
||||
}
|
||||
|
||||
describe("buildReviewGitStatusFiles", () => {
|
||||
it("keeps legacy bare files for a single-repository task", () => {
|
||||
const legacyFiles = { [README_PATH]: file() };
|
||||
const perRepoFiles = { [README_PATH]: file() };
|
||||
|
||||
const result = buildReviewGitStatusFiles(
|
||||
status(legacyFiles),
|
||||
[{ repository_name: "frontend", status: status(perRepoFiles) }],
|
||||
1,
|
||||
);
|
||||
|
||||
expect(result).toEqual({ files: legacyFiles, isMultiRepo: false });
|
||||
expect(result.files?.[README_PATH]?.repository_name).toBeUndefined();
|
||||
});
|
||||
|
||||
it("uses one named status as a bare fallback while legacy status hydrates", () => {
|
||||
const perRepoFiles = { [README_PATH]: file() };
|
||||
|
||||
const result = buildReviewGitStatusFiles(
|
||||
undefined,
|
||||
[{ repository_name: "frontend", status: status(perRepoFiles) }],
|
||||
1,
|
||||
);
|
||||
|
||||
expect(result).toEqual({ files: perRepoFiles, isMultiRepo: false });
|
||||
expect(result.files?.[README_PATH]?.repository_name).toBeUndefined();
|
||||
});
|
||||
|
||||
it("uses stable composite keys while multi-repo statuses hydrate", () => {
|
||||
const frontend = { [README_PATH]: file() };
|
||||
const backend = { [README_PATH]: file() };
|
||||
|
||||
const partiallyHydrated = buildReviewGitStatusFiles(
|
||||
status(frontend),
|
||||
[{ repository_name: "frontend", status: status(frontend) }],
|
||||
2,
|
||||
);
|
||||
const fullyHydrated = buildReviewGitStatusFiles(
|
||||
status(backend),
|
||||
[
|
||||
{ repository_name: "frontend", status: status(frontend) },
|
||||
{ repository_name: "backend", status: status(backend) },
|
||||
],
|
||||
2,
|
||||
);
|
||||
|
||||
expect(partiallyHydrated.isMultiRepo).toBe(true);
|
||||
expect(Object.keys(partiallyHydrated.files ?? {})).toEqual(["frontend\u0000README.md"]);
|
||||
expect(Object.keys(fullyHydrated.files ?? {}).sort()).toEqual([
|
||||
"backend\u0000README.md",
|
||||
"frontend\u0000README.md",
|
||||
]);
|
||||
});
|
||||
|
||||
it("keeps legacy files while multi-repo statuses have not hydrated", () => {
|
||||
const legacyFiles = { [README_PATH]: file() };
|
||||
|
||||
const result = buildReviewGitStatusFiles(status(legacyFiles), [], 2);
|
||||
|
||||
expect(result).toEqual({ files: legacyFiles, isMultiRepo: true });
|
||||
});
|
||||
|
||||
it("falls back to per-repo status count when task repositories are not hydrated", () => {
|
||||
const frontend = { "src/app.ts": file("src/app.ts") };
|
||||
const backend = { "cmd/main.go": file("cmd/main.go") };
|
||||
const statuses: GitStatusByRepo = [
|
||||
{ repository_name: "frontend", status: status(frontend) },
|
||||
{ repository_name: "backend", status: status(backend) },
|
||||
];
|
||||
|
||||
const result = buildReviewGitStatusFiles(status(frontend), statuses, 0);
|
||||
|
||||
expect(result.isMultiRepo).toBe(true);
|
||||
expect(Object.keys(result.files ?? {}).sort()).toEqual([
|
||||
"backend\u0000cmd/main.go",
|
||||
"frontend\u0000src/app.ts",
|
||||
]);
|
||||
});
|
||||
|
||||
it("uses cumulative repositories while task and status metadata hydrate", () => {
|
||||
const frontend = { [README_PATH]: file() };
|
||||
|
||||
const result = buildReviewGitStatusFiles(
|
||||
undefined,
|
||||
[{ repository_name: "frontend", status: status(frontend) }],
|
||||
0,
|
||||
["frontend", "backend"],
|
||||
);
|
||||
|
||||
expect(result.isMultiRepo).toBe(true);
|
||||
expect(Object.keys(result.files ?? {})).toEqual(["frontend\u0000README.md"]);
|
||||
});
|
||||
});
|
||||
@@ -10,11 +10,60 @@ import { useCumulativeDiff } from "@/hooks/domains/session/use-cumulative-diff";
|
||||
import { useFileEditors } from "@/hooks/use-file-editors";
|
||||
import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr";
|
||||
import { usePRDiff } from "@/hooks/domains/github/use-pr-diff";
|
||||
import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories";
|
||||
import { useRepository } from "@/hooks/domains/workspace/use-repository";
|
||||
import { formatReviewCommentsAsMarkdown } from "@/components/task/chat/messages/review-comments-attachment";
|
||||
import {
|
||||
getCumulativeReviewRepositoryNames,
|
||||
isReviewMultiRepo,
|
||||
resolvePRReviewRepositoryName,
|
||||
} from "@/components/review/types";
|
||||
import { getWebSocketClient } from "@/lib/ws/connection";
|
||||
import { useToast } from "@/components/toast-provider";
|
||||
import type { DiffComment } from "@/lib/diff/types";
|
||||
import type { FileInfo } from "@/lib/state/slices/session-runtime/types";
|
||||
import type { FileInfo, GitStatusEntry } from "@/lib/state/slices/session-runtime/types";
|
||||
|
||||
type ReviewGitStatusFiles = {
|
||||
files: Record<string, FileInfo> | null;
|
||||
isMultiRepo: boolean;
|
||||
};
|
||||
|
||||
export function buildReviewGitStatusFiles(
|
||||
reviewGitStatus: GitStatusEntry | undefined,
|
||||
statusByRepo: Array<{ repository_name: string; status: GitStatusEntry }>,
|
||||
taskRepositoryCount: number,
|
||||
cumulativeRepositoryNames: Iterable<string> = [],
|
||||
): ReviewGitStatusFiles {
|
||||
const named = statusByRepo.filter((entry) => entry.repository_name !== "");
|
||||
const isMultiRepo = isReviewMultiRepo(
|
||||
taskRepositoryCount,
|
||||
named.map((entry) => entry.repository_name).concat(Array.from(cumulativeRepositoryNames)),
|
||||
);
|
||||
if (!isMultiRepo) {
|
||||
return {
|
||||
files: reviewGitStatus?.files ?? named[0]?.status.files ?? null,
|
||||
isMultiRepo: false,
|
||||
};
|
||||
}
|
||||
if (named.length === 0) {
|
||||
return {
|
||||
files: reviewGitStatus?.files ?? null,
|
||||
isMultiRepo: true,
|
||||
};
|
||||
}
|
||||
|
||||
const files: Record<string, FileInfo> = {};
|
||||
for (const { repository_name, status } of named) {
|
||||
if (!status?.files) continue;
|
||||
for (const [path, file] of Object.entries(status.files)) {
|
||||
files[`${repository_name}\u0000${path}`] = { ...file, repository_name };
|
||||
}
|
||||
}
|
||||
return {
|
||||
files: Object.keys(files).length > 0 ? files : null,
|
||||
isMultiRepo: true,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the unified gitStatus.files map fed into the ReviewDialog. Multi-repo
|
||||
@@ -23,35 +72,51 @@ import type { FileInfo } from "@/lib/state/slices/session-runtime/types";
|
||||
* every FileInfo is stamped with its `repository_name`. Single-repo tasks keep
|
||||
* the legacy path-only keying.
|
||||
*/
|
||||
function useReviewGitStatusFiles(sessionId: string | null): Record<string, FileInfo> | null {
|
||||
function useReviewGitStatusFiles(
|
||||
sessionId: string | null,
|
||||
taskRepositoryCount: number,
|
||||
cumulativeRepositoryNames: string[],
|
||||
): ReviewGitStatusFiles {
|
||||
const reviewGitStatus = useSessionGitStatus(sessionId);
|
||||
const statusByRepo = useSessionGitStatusByRepo(sessionId);
|
||||
return useMemo(() => {
|
||||
const named = statusByRepo.filter((s) => s.repository_name !== "");
|
||||
if (named.length === 0) return reviewGitStatus?.files ?? null;
|
||||
const out: Record<string, FileInfo> = {};
|
||||
for (const { repository_name, status } of named) {
|
||||
if (!status?.files) continue;
|
||||
for (const [path, file] of Object.entries(status.files)) {
|
||||
out[`${repository_name}\u0000${path}`] = { ...file, repository_name };
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}, [reviewGitStatus, statusByRepo]);
|
||||
return useMemo(
|
||||
() =>
|
||||
buildReviewGitStatusFiles(
|
||||
reviewGitStatus,
|
||||
statusByRepo,
|
||||
taskRepositoryCount,
|
||||
cumulativeRepositoryNames,
|
||||
),
|
||||
[reviewGitStatus, statusByRepo, taskRepositoryCount, cumulativeRepositoryNames],
|
||||
);
|
||||
}
|
||||
|
||||
export function useReviewDialog(effectiveSessionId: string | null) {
|
||||
const [reviewDialogOpen, setReviewDialogOpen] = useState(false);
|
||||
const { toast } = useToast();
|
||||
const activeTaskId = useAppStore((state) => state.tasks.activeTaskId);
|
||||
const taskRepositories = useTaskRepositories(activeTaskId);
|
||||
const baseBranch = useAppStore((state) => {
|
||||
if (!effectiveSessionId) return undefined;
|
||||
return state.taskSessions.items[effectiveSessionId]?.base_branch;
|
||||
});
|
||||
const reviewGitStatusFiles = useReviewGitStatusFiles(effectiveSessionId);
|
||||
const { diff: reviewCumulativeDiff } = useCumulativeDiff(effectiveSessionId);
|
||||
const cumulativeRepositoryNames = useMemo(
|
||||
() => getCumulativeReviewRepositoryNames(reviewCumulativeDiff?.files),
|
||||
[reviewCumulativeDiff],
|
||||
);
|
||||
const reviewGitStatus = useReviewGitStatusFiles(
|
||||
effectiveSessionId,
|
||||
taskRepositories.length,
|
||||
cumulativeRepositoryNames,
|
||||
);
|
||||
const { openFile: reviewOpenFile } = useFileEditors();
|
||||
const reviewTaskPR = useActiveTaskPR();
|
||||
const reviewPRRepository = useRepository(reviewTaskPR?.repository_id ?? null);
|
||||
const reviewPRRepositoryName = resolvePRReviewRepositoryName(
|
||||
reviewTaskPR,
|
||||
reviewPRRepository?.name,
|
||||
);
|
||||
const { files: reviewPRDiffFiles } = usePRDiff(
|
||||
reviewTaskPR?.owner ?? null,
|
||||
reviewTaskPR?.repo ?? null,
|
||||
@@ -88,9 +153,11 @@ export function useReviewDialog(effectiveSessionId: string | null) {
|
||||
reviewDialogOpen,
|
||||
setReviewDialogOpen,
|
||||
baseBranch,
|
||||
reviewGitStatusFiles,
|
||||
reviewGitStatusFiles: reviewGitStatus.files,
|
||||
reviewCumulativeDiff,
|
||||
reviewPRDiffFiles,
|
||||
reviewPRRepoName: reviewGitStatus.isMultiRepo ? reviewPRRepositoryName : undefined,
|
||||
reviewUseRepositoryKeys: reviewGitStatus.isMultiRepo,
|
||||
reviewOpenFile,
|
||||
handleReviewSendComments,
|
||||
};
|
||||
|
||||
@@ -1003,6 +1003,7 @@ export class ApiClient {
|
||||
additions: number;
|
||||
deletions: number;
|
||||
patch?: string;
|
||||
old_path?: string;
|
||||
}>,
|
||||
): Promise<void> {
|
||||
await this.request("POST", "/api/v1/github/mock/files", {
|
||||
|
||||
@@ -0,0 +1,135 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { GitHelper, makeGitEnv } from "../../helpers/git-helper";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
import path from "node:path";
|
||||
|
||||
const MOBILE_FILE = "mobile-review-status-added.ts";
|
||||
const MOBILE_MOVED_FROM_FILE = "mobile-review-status-old-name.ts";
|
||||
const MOBILE_MOVED_FILE = "mobile-review-status-new-name.ts";
|
||||
|
||||
test.describe("Review file status on mobile", () => {
|
||||
test.describe.configure({ timeout: 120_000 });
|
||||
|
||||
test("shows status in the sticky header without horizontal overflow", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
await apiClient.mockGitHubReset();
|
||||
await apiClient.mockGitHubSetUser("reviewer");
|
||||
await apiClient.mockGitHubAddPRs([
|
||||
{
|
||||
number: 72,
|
||||
title: "Mobile review status cues",
|
||||
state: "open",
|
||||
head_branch: "feat/mobile-review-status",
|
||||
base_branch: "main",
|
||||
author_login: "reviewer",
|
||||
repo_owner: "testorg",
|
||||
repo_name: "testrepo",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
},
|
||||
]);
|
||||
await apiClient.mockGitHubAddPRFiles("testorg", "testrepo", 72, [
|
||||
{
|
||||
filename: MOBILE_MOVED_FILE,
|
||||
status: "renamed",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: MOBILE_MOVED_FROM_FILE,
|
||||
},
|
||||
]);
|
||||
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Mobile Review File Status E2E",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:simple-message",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
await apiClient.mockGitHubAssociateTaskPR({
|
||||
task_id: task.id,
|
||||
owner: "testorg",
|
||||
repo: "testrepo",
|
||||
pr_number: 72,
|
||||
pr_url: "https://github.com/testorg/testrepo/pull/72",
|
||||
pr_title: "Mobile review status cues",
|
||||
head_branch: "feat/mobile-review-status",
|
||||
base_branch: "main",
|
||||
author_login: "reviewer",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
});
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await session.waitForChatIdle();
|
||||
|
||||
const git = new GitHelper(
|
||||
path.join(backend.tmpDir, "repos", "e2e-repo"),
|
||||
makeGitEnv(backend.tmpDir),
|
||||
);
|
||||
git.createFile(MOBILE_FILE, "mobile added file\n");
|
||||
git.stageFile(MOBILE_FILE);
|
||||
|
||||
await testPage.getByRole("button", { name: "Changes" }).tap();
|
||||
const changesPanel = testPage.getByTestId("mobile-changes-panel");
|
||||
await expect(changesPanel).toBeVisible();
|
||||
await expect(testPage.getByTestId(`file-row-${MOBILE_FILE}`)).toBeVisible({ timeout: 20_000 });
|
||||
await changesPanel.getByRole("button", { name: "Review", exact: true }).tap();
|
||||
|
||||
const dialog = testPage.getByRole("dialog", { name: "Review Changes" });
|
||||
await expect(dialog).toBeVisible();
|
||||
const header = dialog.getByTestId("review-file-header").filter({ hasText: MOBILE_FILE });
|
||||
await expect(header).toBeVisible();
|
||||
const marker = header.getByRole("img", { name: "Added" });
|
||||
await expect(marker).toBeVisible();
|
||||
|
||||
const movedHeader = dialog
|
||||
.getByTestId("review-file-header")
|
||||
.filter({ hasText: MOBILE_MOVED_FILE });
|
||||
await expect(movedHeader).toBeVisible();
|
||||
await expect(
|
||||
movedHeader.getByRole("img", { name: `Moved from ${MOBILE_MOVED_FROM_FILE}` }),
|
||||
).toBeVisible();
|
||||
await expect(
|
||||
dialog.getByText(`Moved from ${MOBILE_MOVED_FROM_FILE}; no textual changes`),
|
||||
).toBeVisible();
|
||||
|
||||
const headerGeometry = await header.evaluate((element) => {
|
||||
const markerElement = element.querySelector<HTMLElement>('[data-file-status="added"]');
|
||||
if (!markerElement) return null;
|
||||
const headerBounds = element.getBoundingClientRect();
|
||||
const markerBounds = markerElement.getBoundingClientRect();
|
||||
return {
|
||||
scrollWidth: element.scrollWidth,
|
||||
clientWidth: element.clientWidth,
|
||||
headerLeft: headerBounds.left,
|
||||
headerRight: headerBounds.right,
|
||||
markerLeft: markerBounds.left,
|
||||
markerRight: markerBounds.right,
|
||||
};
|
||||
});
|
||||
if (!headerGeometry) throw new Error("Mobile Review header geometry is unavailable");
|
||||
expect(headerGeometry.scrollWidth).toBeLessThanOrEqual(headerGeometry.clientWidth);
|
||||
expect(headerGeometry.markerLeft).toBeGreaterThanOrEqual(headerGeometry.headerLeft);
|
||||
expect(headerGeometry.markerRight).toBeLessThanOrEqual(headerGeometry.headerRight);
|
||||
|
||||
const checkbox = header.getByRole("checkbox");
|
||||
await checkbox.tap();
|
||||
await expect(checkbox).toHaveAttribute("aria-checked", "true");
|
||||
|
||||
const overflow = await testPage.evaluate(() => ({
|
||||
viewport: window.innerWidth,
|
||||
document: document.documentElement.scrollWidth,
|
||||
}));
|
||||
expect(overflow.document).toBeLessThanOrEqual(overflow.viewport + 1);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,162 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { GitHelper, makeGitEnv } from "../../helpers/git-helper";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
import { REVIEW_SIDEBAR_LIMITS } from "../../../hooks/use-review-sidebar-resize";
|
||||
import path from "node:path";
|
||||
|
||||
const ADDED_PATH = "review-status-added.ts";
|
||||
const MODIFIED_PATH = "review-status-modified.ts";
|
||||
const DELETED_PATH = "review-status-deleted.ts";
|
||||
const MOVED_FROM_PATH = "review-status-old-name.ts";
|
||||
const MOVED_PATH = "review-status-a-very-long-new-name-that-must-truncate.ts";
|
||||
|
||||
test.describe("Review file status", () => {
|
||||
test.describe.configure({ timeout: 120_000 });
|
||||
|
||||
test("shows every status, keeps the marker visible at minimum width, and explains a pure move", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
await testPage.addInitScript(({ key, width }) => sessionStorage.setItem(key, width), {
|
||||
key: REVIEW_SIDEBAR_LIMITS.storageKey,
|
||||
width: String(REVIEW_SIDEBAR_LIMITS.minWidth),
|
||||
});
|
||||
|
||||
await apiClient.mockGitHubReset();
|
||||
await apiClient.mockGitHubSetUser("reviewer");
|
||||
await apiClient.mockGitHubAddPRs([
|
||||
{
|
||||
number: 71,
|
||||
title: "Review status cues",
|
||||
state: "open",
|
||||
head_branch: "feat/review-status",
|
||||
base_branch: "main",
|
||||
author_login: "reviewer",
|
||||
repo_owner: "testorg",
|
||||
repo_name: "testrepo",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
},
|
||||
]);
|
||||
const movedFiles = [
|
||||
{
|
||||
filename: MOVED_PATH,
|
||||
status: "renamed",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
old_path: MOVED_FROM_PATH,
|
||||
},
|
||||
];
|
||||
await apiClient.mockGitHubAddPRFiles("testorg", "testrepo", 71, movedFiles);
|
||||
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Review File Status E2E",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:simple-message",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
await apiClient.mockGitHubAssociateTaskPR({
|
||||
task_id: task.id,
|
||||
owner: "testorg",
|
||||
repo: "testrepo",
|
||||
pr_number: 71,
|
||||
pr_url: "https://github.com/testorg/testrepo/pull/71",
|
||||
pr_title: "Review status cues",
|
||||
head_branch: "feat/review-status",
|
||||
base_branch: "main",
|
||||
author_login: "reviewer",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
});
|
||||
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await session.waitForChatIdle();
|
||||
|
||||
const git = new GitHelper(
|
||||
path.join(backend.tmpDir, "repos", "e2e-repo"),
|
||||
makeGitEnv(backend.tmpDir),
|
||||
);
|
||||
git.createFile(MODIFIED_PATH, "before\n");
|
||||
git.createFile(DELETED_PATH, "remove me\n");
|
||||
git.stageAll();
|
||||
git.commit("seed review status files");
|
||||
git.createFile(ADDED_PATH, "added\n");
|
||||
git.stageFile(ADDED_PATH);
|
||||
git.modifyFile(MODIFIED_PATH, "after\n");
|
||||
git.deleteFile(DELETED_PATH);
|
||||
|
||||
const changesTab = testPage.getByTestId("dockview-tab-changes");
|
||||
await expect(changesTab).toBeVisible();
|
||||
await changesTab.click();
|
||||
for (const filePath of [ADDED_PATH, MODIFIED_PATH, DELETED_PATH]) {
|
||||
await expect(testPage.getByTestId(`file-row-${filePath}`)).toBeVisible({ timeout: 20_000 });
|
||||
}
|
||||
|
||||
await testPage
|
||||
.getByTestId("changes-panel")
|
||||
.getByRole("button", { name: "Diff", exact: true })
|
||||
.click();
|
||||
await testPage.getByRole("button", { name: "Expand review" }).click();
|
||||
const dialog = testPage.getByRole("dialog", { name: "Review Changes" });
|
||||
await expect(dialog).toBeVisible();
|
||||
const sidebar = dialog.getByTestId("review-dialog-sidebar");
|
||||
await expect(sidebar).toBeVisible();
|
||||
|
||||
for (const name of ["Added", "Modified", "Deleted"]) {
|
||||
await expect(sidebar.getByRole("img", { name })).toBeVisible();
|
||||
}
|
||||
await expect(sidebar.getByRole("img", { name: `Moved from ${MOVED_FROM_PATH}` })).toBeVisible({
|
||||
timeout: 20_000,
|
||||
});
|
||||
|
||||
const movedRow = sidebar.locator(
|
||||
`[data-testid="review-file-row"][data-file-path="${MOVED_PATH}"]`,
|
||||
);
|
||||
const movedMarker = movedRow.locator('[data-file-status="renamed"]');
|
||||
await expect(movedMarker).toBeVisible();
|
||||
await expect
|
||||
.poll(async () => Math.round((await sidebar.boundingBox())?.width ?? 0))
|
||||
.toBeGreaterThanOrEqual(REVIEW_SIDEBAR_LIMITS.minWidth - 2);
|
||||
const sidebarBox = await sidebar.boundingBox();
|
||||
if (!sidebarBox) throw new Error("Review sidebar has no bounding box");
|
||||
expect(sidebarBox.width).toBeLessThanOrEqual(REVIEW_SIDEBAR_LIMITS.minWidth + 2);
|
||||
|
||||
const geometry = await movedRow.evaluate((row) => {
|
||||
const name = row.querySelector<HTMLElement>("[data-review-file-name]");
|
||||
const marker = row.querySelector<HTMLElement>('[data-file-status="renamed"]');
|
||||
const rowBounds = row.getBoundingClientRect();
|
||||
const sidebarBounds = row
|
||||
.closest<HTMLElement>('[data-testid="review-dialog-sidebar"]')
|
||||
?.getBoundingClientRect();
|
||||
if (!name || !marker || !sidebarBounds) return null;
|
||||
const nameBounds = name.getBoundingClientRect();
|
||||
const markerBounds = marker.getBoundingClientRect();
|
||||
return {
|
||||
nameRight: nameBounds.right,
|
||||
markerLeft: markerBounds.left,
|
||||
markerRight: markerBounds.right,
|
||||
rowRight: rowBounds.right,
|
||||
sidebarRight: sidebarBounds.right,
|
||||
};
|
||||
});
|
||||
if (!geometry) throw new Error("Moved row geometry is unavailable");
|
||||
expect(geometry.nameRight).toBeLessThanOrEqual(geometry.markerLeft);
|
||||
expect(geometry.markerRight).toBeLessThanOrEqual(geometry.rowRight);
|
||||
expect(geometry.markerRight).toBeLessThanOrEqual(geometry.sidebarRight);
|
||||
|
||||
await movedRow.click();
|
||||
await expect(
|
||||
dialog.getByText(`Moved from ${MOVED_FROM_PATH}; no textual changes`),
|
||||
).toBeVisible();
|
||||
await expect(dialog.getByText("Loading diff...")).toHaveCount(0);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,99 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { reviewFileKey } from "@/components/review/types";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
import { buildReviewSources, normalizeReviewStatusSources } from "./use-review-sources";
|
||||
|
||||
const namedStatus = {
|
||||
repository_name: "frontend",
|
||||
status: {
|
||||
files: {
|
||||
"src/local.ts": { diff: "@@local@@", status: "modified", additions: 1, deletions: 0 },
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const prDiffFiles = [
|
||||
{ filename: "src/pr.ts", status: "modified", patch: "@@pr@@" },
|
||||
] as PRDiffFile[];
|
||||
|
||||
const cumulativeDiff = {
|
||||
files: {
|
||||
["frontend\u0000src/committed.ts"]: {
|
||||
path: "src/committed.ts",
|
||||
repository_name: "frontend",
|
||||
diff: "@@committed@@",
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
function reviewKeysFor(taskRepositoryCount: number) {
|
||||
const normalized = normalizeReviewStatusSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: [namedStatus],
|
||||
taskRepositoryCount,
|
||||
resolvedPRRepoName: "frontend",
|
||||
});
|
||||
const result = buildReviewSources({
|
||||
gitStatus: normalized.normalizedGitStatus,
|
||||
statusByRepo: normalized.normalizedStatusByRepo,
|
||||
cumulativeDiff,
|
||||
prDiffFiles,
|
||||
prRepoName: normalized.prRepoName,
|
||||
useRepositoryKeys: normalized.useRepositoryKeys,
|
||||
});
|
||||
return result.allFiles.map(reviewFileKey).sort();
|
||||
}
|
||||
|
||||
describe("review source key mode", () => {
|
||||
it.each([
|
||||
{
|
||||
name: "bare for one named status in a single-repo task",
|
||||
taskRepositoryCount: 1,
|
||||
expected: ["src/committed.ts", "src/local.ts", "src/pr.ts"],
|
||||
},
|
||||
{
|
||||
name: "composite for a true multi-repo task",
|
||||
taskRepositoryCount: 2,
|
||||
expected: [
|
||||
"frontend\u0000src/committed.ts",
|
||||
"frontend\u0000src/local.ts",
|
||||
"frontend\u0000src/pr.ts",
|
||||
],
|
||||
},
|
||||
])("keeps local, cumulative, and PR keys $name", ({ taskRepositoryCount, expected }) => {
|
||||
expect(reviewKeysFor(taskRepositoryCount)).toEqual(expected);
|
||||
});
|
||||
|
||||
it("preserves same-path cumulative files before task and status metadata hydrate", () => {
|
||||
const normalized = normalizeReviewStatusSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: [namedStatus],
|
||||
taskRepositoryCount: 0,
|
||||
resolvedPRRepoName: "frontend",
|
||||
cumulativeRepositoryNames: ["frontend", "backend"],
|
||||
});
|
||||
const samePathCumulativeDiff = {
|
||||
files: Object.fromEntries(
|
||||
["frontend", "backend"].map((repositoryName) => [
|
||||
`${repositoryName}\u0000README.md`,
|
||||
{ path: "README.md", repository_name: repositoryName, diff: `@@${repositoryName}@@` },
|
||||
]),
|
||||
),
|
||||
};
|
||||
|
||||
const result = buildReviewSources({
|
||||
gitStatus: normalized.normalizedGitStatus,
|
||||
statusByRepo: normalized.normalizedStatusByRepo,
|
||||
cumulativeDiff: samePathCumulativeDiff,
|
||||
prDiffFiles: undefined,
|
||||
prRepoName: normalized.prRepoName,
|
||||
useRepositoryKeys: normalized.useRepositoryKeys,
|
||||
});
|
||||
|
||||
expect(result.allFiles.map(reviewFileKey).sort()).toEqual([
|
||||
"backend\u0000README.md",
|
||||
"frontend\u0000README.md",
|
||||
"frontend\u0000src/local.ts",
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -1,8 +1,12 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { buildReviewSources } from "./use-review-sources";
|
||||
import { resolvePRReviewRepositoryName } from "@/components/review/types";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
|
||||
/* eslint-disable max-lines-per-function -- multi-case describe block */
|
||||
const RENAMED_PATH = "src/new-name.ts";
|
||||
const PREVIOUS_PATH = "src/old-name.ts";
|
||||
|
||||
/* eslint-disable max-lines-per-function -- comprehensive source merge cases */
|
||||
describe("buildReviewSources", () => {
|
||||
it("returns empty result when no inputs", () => {
|
||||
const result = buildReviewSources({
|
||||
@@ -76,9 +80,98 @@ describe("buildReviewSources", () => {
|
||||
});
|
||||
expect(result.allFiles).toHaveLength(1);
|
||||
expect(result.allFiles[0].source).toBe("pr");
|
||||
expect(result.allFiles[0].repository_name).toBeUndefined();
|
||||
expect(result.sourceCounts).toEqual({ uncommitted: 0, committed: 0, pr: 1 });
|
||||
});
|
||||
|
||||
it("keeps a patchless PR rename with its previous path", () => {
|
||||
const result = buildReviewSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: undefined,
|
||||
cumulativeDiff: null,
|
||||
prDiffFiles: [
|
||||
{
|
||||
filename: RENAMED_PATH,
|
||||
old_path: PREVIOUS_PATH,
|
||||
status: "renamed",
|
||||
patch: "",
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
expect(result.allFiles).toEqual([
|
||||
expect.objectContaining({
|
||||
path: RENAMED_PATH,
|
||||
old_path: PREVIOUS_PATH,
|
||||
status: "renamed",
|
||||
diff: "",
|
||||
source: "pr",
|
||||
}),
|
||||
]);
|
||||
expect(result.sourceCounts).toEqual({ uncommitted: 0, committed: 0, pr: 1 });
|
||||
});
|
||||
|
||||
it("normalizes removed and unknown PR statuses", () => {
|
||||
const result = buildReviewSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: undefined,
|
||||
cumulativeDiff: null,
|
||||
prDiffFiles: [
|
||||
{ filename: "gone.ts", status: "removed", patch: "", additions: 0, deletions: 1 },
|
||||
{ filename: "copied.ts", status: "copied", patch: "", additions: 1, deletions: 0 },
|
||||
],
|
||||
});
|
||||
|
||||
expect(Object.fromEntries(result.allFiles.map((file) => [file.path, file.status]))).toEqual({
|
||||
"copied.ts": "modified",
|
||||
"gone.ts": "deleted",
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps patchless uncommitted and cumulative files with metadata", () => {
|
||||
const result = buildReviewSources({
|
||||
gitStatus: {
|
||||
files: {
|
||||
"assets/logo.png": {
|
||||
status: "modified",
|
||||
diff_skip_reason: "binary",
|
||||
},
|
||||
},
|
||||
},
|
||||
statusByRepo: undefined,
|
||||
cumulativeDiff: {
|
||||
files: {
|
||||
[RENAMED_PATH]: {
|
||||
status: "renamed",
|
||||
old_path: PREVIOUS_PATH,
|
||||
},
|
||||
},
|
||||
},
|
||||
prDiffFiles: undefined,
|
||||
});
|
||||
|
||||
expect(result.allFiles).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.sourceCounts).toEqual({ uncommitted: 1, committed: 1, pr: 0 });
|
||||
});
|
||||
|
||||
it("dedupes by path: uncommitted wins over committed wins over PR", () => {
|
||||
const prFiles: PRDiffFile[] = [
|
||||
{
|
||||
@@ -217,12 +310,16 @@ describe("buildReviewSources", () => {
|
||||
expect(result.sourceCounts).toEqual({ uncommitted: 1, committed: 0, pr: 0 });
|
||||
});
|
||||
|
||||
it("multi-repo uncommitted + PR overlap: file appears once as uncommitted (same repo)", () => {
|
||||
it("dedupes PR files using the canonical workspace repo rather than provider repo name", () => {
|
||||
const prRepoName = resolvePRReviewRepositoryName(
|
||||
{ repository_id: "repo-1", repo: "widgets" },
|
||||
"acme/widgets",
|
||||
);
|
||||
const result = buildReviewSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: [
|
||||
{
|
||||
repository_name: "frontend",
|
||||
repository_name: "acme-widgets",
|
||||
status: {
|
||||
files: {
|
||||
"src/shared.ts": {
|
||||
@@ -245,7 +342,7 @@ describe("buildReviewSources", () => {
|
||||
deletions: 0,
|
||||
},
|
||||
],
|
||||
prRepoName: "frontend",
|
||||
prRepoName,
|
||||
});
|
||||
expect(result.allFiles).toHaveLength(1);
|
||||
expect(result.allFiles[0].source).toBe("uncommitted");
|
||||
@@ -413,6 +510,42 @@ describe("buildReviewSources", () => {
|
||||
expect(result.sourceCounts).toEqual({ uncommitted: 2, committed: 0, pr: 0 });
|
||||
});
|
||||
|
||||
it("keeps repo and path combinations containing colons distinct", () => {
|
||||
const result = buildReviewSources({
|
||||
gitStatus: undefined,
|
||||
statusByRepo: [
|
||||
{
|
||||
repository_name: "frontend:src",
|
||||
status: {
|
||||
files: {
|
||||
"app.ts": { diff: "@@first@@", status: "modified", additions: 1, deletions: 0 },
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
repository_name: "frontend",
|
||||
status: {
|
||||
files: {
|
||||
"src:app.ts": {
|
||||
diff: "@@second@@",
|
||||
status: "modified",
|
||||
additions: 1,
|
||||
deletions: 0,
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
],
|
||||
cumulativeDiff: null,
|
||||
prDiffFiles: undefined,
|
||||
});
|
||||
|
||||
expect(result.allFiles.map((file) => [file.repository_name, file.path])).toEqual([
|
||||
["frontend", "src:app.ts"],
|
||||
["frontend:src", "app.ts"],
|
||||
]);
|
||||
});
|
||||
|
||||
// Regression for the multi-repo cumulative-diff bug: backend's
|
||||
// `mergeCumulativeFiles` uses a `<repo>\x00<path>` map key and stamps the
|
||||
// clean repo-relative path on `file.path`. The old code read the path from
|
||||
|
||||
@@ -1,12 +1,23 @@
|
||||
import { useEffect, useMemo } from "react";
|
||||
import { useAppStore } from "@/components/state-provider";
|
||||
import { useSessionGitStatus, useSessionGitStatusByRepo } from "./use-session-git-status";
|
||||
import { useCumulativeDiff } from "./use-cumulative-diff";
|
||||
import { useActiveTaskPR } from "@/hooks/domains/github/use-task-pr";
|
||||
import { usePRDiff } from "@/hooks/domains/github/use-pr-diff";
|
||||
import { normalizeDiffContent } from "@/components/review/types";
|
||||
import { useTaskRepositories } from "@/hooks/domains/kanban/use-task-repositories";
|
||||
import { useRepository } from "@/hooks/domains/workspace/use-repository";
|
||||
import {
|
||||
getCumulativeReviewRepositoryNames,
|
||||
isReviewMultiRepo,
|
||||
normalizeDiffContent,
|
||||
resolvePRReviewRepositoryName,
|
||||
reviewFileKey,
|
||||
splitReviewFileKey,
|
||||
} from "@/components/review/types";
|
||||
import { createDebugLogger } from "@/lib/debug/log";
|
||||
import type { ReviewFile } from "@/components/review/types";
|
||||
import type { PRDiffFile } from "@/lib/types/github";
|
||||
import { normalizeFileChangeStatus } from "@/lib/utils/file-change-status";
|
||||
|
||||
const debug = createDebugLogger("review:sources");
|
||||
|
||||
@@ -18,6 +29,7 @@ type UncommittedFile = {
|
||||
diff?: string;
|
||||
diff_skip_reason?: ReviewFile["diff_skip_reason"];
|
||||
status?: string;
|
||||
old_path?: string;
|
||||
additions?: number;
|
||||
deletions?: number;
|
||||
staged?: boolean;
|
||||
@@ -25,7 +37,9 @@ type UncommittedFile = {
|
||||
|
||||
type CumulativeFile = {
|
||||
diff?: string;
|
||||
diff_skip_reason?: ReviewFile["diff_skip_reason"];
|
||||
status?: string;
|
||||
old_path?: string;
|
||||
additions?: number;
|
||||
deletions?: number;
|
||||
repository_name?: string;
|
||||
@@ -45,16 +59,16 @@ function addUncommittedFiles(
|
||||
for (const [path, file] of Object.entries(files)) {
|
||||
const diff = file.diff ? normalizeDiffContent(file.diff) : "";
|
||||
const skipReason = file.diff_skip_reason;
|
||||
if (!diff && !skipReason) continue;
|
||||
const key = repositoryName ? `${repositoryName}:${path}` : path;
|
||||
const key = reviewFileKey({ path, repository_name: repositoryName });
|
||||
fileMap.set(key, {
|
||||
path,
|
||||
diff,
|
||||
status: file.status ?? "modified",
|
||||
status: normalizeFileChangeStatus(file.status),
|
||||
additions: file.additions ?? 0,
|
||||
deletions: file.deletions ?? 0,
|
||||
staged: file.staged ?? false,
|
||||
source: "uncommitted",
|
||||
old_path: file.old_path,
|
||||
diff_skip_reason: skipReason,
|
||||
repository_name: repositoryName,
|
||||
});
|
||||
@@ -65,6 +79,7 @@ function addCumulativeFiles(
|
||||
fileMap: Map<string, ReviewFile>,
|
||||
files: Record<string, CumulativeFile>,
|
||||
uncommittedPaths: Set<string>,
|
||||
useRepositoryKeys: boolean,
|
||||
) {
|
||||
for (const [mapKey, file] of Object.entries(files)) {
|
||||
// Multi-repo cumulative payloads use a NUL-composite `<repo>\x00<path>`
|
||||
@@ -72,31 +87,27 @@ function addCumulativeFiles(
|
||||
// Single-repo keeps the bare path on the map key with no `file.path`.
|
||||
// Prefer the stamped value so the composite key doesn't bleed into the
|
||||
// displayed path; fall back to the map key for single-repo.
|
||||
const path = file.path ?? mapKey;
|
||||
const key = file.repository_name ? `${file.repository_name}:${path}` : path;
|
||||
const path = file.path ?? splitReviewFileKey(mapKey).path;
|
||||
const repositoryName = useRepositoryKeys ? file.repository_name : undefined;
|
||||
const key = reviewFileKey({ path, repository_name: repositoryName });
|
||||
const hasRepoUnawareCollision = key !== path && fileMap.has(path);
|
||||
if (fileMap.has(key) || uncommittedPaths.has(key) || hasRepoUnawareCollision) continue;
|
||||
const diff = file.diff ? normalizeDiffContent(file.diff) : "";
|
||||
if (!diff) continue;
|
||||
fileMap.set(key, {
|
||||
path,
|
||||
diff,
|
||||
status: file.status || "modified",
|
||||
status: normalizeFileChangeStatus(file.status),
|
||||
additions: file.additions ?? 0,
|
||||
deletions: file.deletions ?? 0,
|
||||
staged: false,
|
||||
source: "committed",
|
||||
repository_name: file.repository_name,
|
||||
old_path: file.old_path,
|
||||
diff_skip_reason: file.diff_skip_reason,
|
||||
repository_name: repositoryName,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
function prFileStatus(status: string): "added" | "deleted" | "modified" {
|
||||
if (status === "added") return "added";
|
||||
if (status === "removed") return "deleted";
|
||||
return "modified";
|
||||
}
|
||||
|
||||
function addPRFiles(
|
||||
fileMap: Map<string, ReviewFile>,
|
||||
files: PRDiffFile[],
|
||||
@@ -104,19 +115,19 @@ function addPRFiles(
|
||||
repoName?: string,
|
||||
) {
|
||||
for (const file of files) {
|
||||
const key = repoName ? `${repoName}:${file.filename}` : file.filename;
|
||||
const key = reviewFileKey({ path: file.filename, repository_name: repoName });
|
||||
const hasRepoUnawareCollision = key !== file.filename && fileMap.has(file.filename);
|
||||
if (fileMap.has(key) || uncommittedPaths.has(key) || hasRepoUnawareCollision) continue;
|
||||
const diff = file.patch ? normalizeDiffContent(file.patch) : "";
|
||||
if (!diff) continue;
|
||||
fileMap.set(key, {
|
||||
path: file.filename,
|
||||
diff,
|
||||
status: prFileStatus(file.status),
|
||||
status: normalizeFileChangeStatus(file.status),
|
||||
additions: file.additions ?? 0,
|
||||
deletions: file.deletions ?? 0,
|
||||
staged: false,
|
||||
source: "pr",
|
||||
old_path: file.old_path,
|
||||
repository_name: repoName,
|
||||
});
|
||||
}
|
||||
@@ -127,15 +138,12 @@ function collectPathsFromFiles(
|
||||
files: Record<string, UncommittedFile>,
|
||||
repositoryName?: string,
|
||||
): void {
|
||||
for (const [path, file] of Object.entries(files)) {
|
||||
const diff = file.diff ? normalizeDiffContent(file.diff) : "";
|
||||
if (diff || file.diff_skip_reason) {
|
||||
// Always add bare path (for deduping repo-unaware sources like cumulative
|
||||
// diffs that may not carry repository_name).
|
||||
paths.add(path);
|
||||
// Also add composite key (for repo-aware dedup when sources carry repo info).
|
||||
if (repositoryName) paths.add(`${repositoryName}:${path}`);
|
||||
}
|
||||
for (const path of Object.keys(files)) {
|
||||
// Always add bare path (for deduping repo-unaware sources like cumulative
|
||||
// diffs that may not carry repository_name).
|
||||
paths.add(path);
|
||||
// Also add composite key (for repo-aware dedup when sources carry repo info).
|
||||
if (repositoryName) paths.add(reviewFileKey({ path, repository_name: repositoryName }));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -169,6 +177,9 @@ export type BuildReviewSourcesInput = {
|
||||
/** Repository name for the primary PR — used to composite-key PR files so
|
||||
* same-named files in different repos are not incorrectly deduped. */
|
||||
prRepoName?: string;
|
||||
/** Whether review identities include repository_name. False keeps every
|
||||
* source bare during legacy/single-repository status hydration. */
|
||||
useRepositoryKeys?: boolean;
|
||||
};
|
||||
|
||||
export type BuildReviewSourcesResult = {
|
||||
@@ -176,16 +187,57 @@ export type BuildReviewSourcesResult = {
|
||||
sourceCounts: SourceCounts;
|
||||
};
|
||||
|
||||
type NormalizeReviewStatusSourcesInput = {
|
||||
gitStatus: BuildReviewSourcesInput["gitStatus"];
|
||||
statusByRepo: NonNullable<BuildReviewSourcesInput["statusByRepo"]>;
|
||||
taskRepositoryCount: number;
|
||||
resolvedPRRepoName?: string;
|
||||
cumulativeRepositoryNames?: Iterable<string>;
|
||||
};
|
||||
|
||||
export function normalizeReviewStatusSources(input: NormalizeReviewStatusSourcesInput) {
|
||||
const namedStatuses = input.statusByRepo.filter((entry) => entry.repository_name !== "");
|
||||
const useRepositoryKeys = isReviewMultiRepo(
|
||||
input.taskRepositoryCount,
|
||||
namedStatuses
|
||||
.map((entry) => entry.repository_name)
|
||||
.concat(Array.from(input.cumulativeRepositoryNames ?? [])),
|
||||
);
|
||||
if (useRepositoryKeys) {
|
||||
return {
|
||||
useRepositoryKeys: true,
|
||||
prRepoName: input.resolvedPRRepoName,
|
||||
normalizedGitStatus: input.gitStatus,
|
||||
normalizedStatusByRepo: namedStatuses.length > 0 ? namedStatuses : undefined,
|
||||
};
|
||||
}
|
||||
return {
|
||||
useRepositoryKeys: false,
|
||||
prRepoName: undefined,
|
||||
normalizedGitStatus: input.gitStatus?.files ? input.gitStatus : namedStatuses[0]?.status,
|
||||
normalizedStatusByRepo: undefined,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Pure helper that merges the three diff sources into one sorted, deduped
|
||||
* list and computes per-source counts. Uncommitted files write first under
|
||||
* composite `repo:path` keys (multi-repo) or simple `path` keys (single-repo).
|
||||
* Committed and PR files write next under simple `path` keys but skip any
|
||||
* path that already appears in the uncommitted set — preserving dedup
|
||||
* priority (uncommitted > committed > PR) across the key-shape mismatch.
|
||||
* shared `reviewFileKey` composite keys (multi-repo) or bare paths
|
||||
* (single-repo). Committed and PR files write next under the same key shape
|
||||
* but skip any path already present in the uncommitted set — preserving
|
||||
* dedup priority (uncommitted > committed > PR).
|
||||
* Keep priority, dedup keys, and sorting aligned with `buildAllFiles` in
|
||||
* `components/review/review-dialog.tsx`.
|
||||
*/
|
||||
export function buildReviewSources(input: BuildReviewSourcesInput): BuildReviewSourcesResult {
|
||||
const { gitStatus, statusByRepo, cumulativeDiff, prDiffFiles, prRepoName } = input;
|
||||
const {
|
||||
gitStatus,
|
||||
statusByRepo,
|
||||
cumulativeDiff,
|
||||
prDiffFiles,
|
||||
prRepoName,
|
||||
useRepositoryKeys = true,
|
||||
} = input;
|
||||
const fileMap = new Map<string, ReviewFile>();
|
||||
|
||||
const uncommittedPaths = collectUncommittedPaths(statusByRepo, gitStatus);
|
||||
@@ -204,7 +256,9 @@ export function buildReviewSources(input: BuildReviewSourcesInput): BuildReviewS
|
||||
addUncommittedFiles(fileMap, gitStatus.files as Record<string, UncommittedFile>);
|
||||
}
|
||||
|
||||
if (cumulativeDiff?.files) addCumulativeFiles(fileMap, cumulativeDiff.files, uncommittedPaths);
|
||||
if (cumulativeDiff?.files) {
|
||||
addCumulativeFiles(fileMap, cumulativeDiff.files, uncommittedPaths, useRepositoryKeys);
|
||||
}
|
||||
|
||||
if (prDiffFiles && prDiffFiles.length > 0)
|
||||
addPRFiles(fileMap, prDiffFiles, uncommittedPaths, prRepoName);
|
||||
@@ -244,6 +298,29 @@ export type UseReviewSourcesResult = {
|
||||
rawPRFiles: ReviewFile[];
|
||||
};
|
||||
|
||||
function useReviewRepositoryContext(
|
||||
pr: ReturnType<typeof useActiveTaskPR>,
|
||||
gitStatus: ReturnType<typeof useSessionGitStatus>,
|
||||
statusByRepo: ReturnType<typeof useSessionGitStatusByRepo>,
|
||||
cumulativeDiff: ReturnType<typeof useCumulativeDiff>["diff"],
|
||||
) {
|
||||
const activeTaskId = useAppStore((state) => state.tasks.activeTaskId);
|
||||
const taskRepositories = useTaskRepositories(activeTaskId);
|
||||
const prRepository = useRepository(pr?.repository_id ?? null);
|
||||
const resolvedPRRepoName = resolvePRReviewRepositoryName(pr, prRepository?.name);
|
||||
return useMemo(
|
||||
() =>
|
||||
normalizeReviewStatusSources({
|
||||
gitStatus,
|
||||
statusByRepo,
|
||||
taskRepositoryCount: taskRepositories.length,
|
||||
resolvedPRRepoName,
|
||||
cumulativeRepositoryNames: getCumulativeReviewRepositoryNames(cumulativeDiff?.files),
|
||||
}),
|
||||
[gitStatus, statusByRepo, taskRepositories.length, resolvedPRRepoName, cumulativeDiff],
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Multi-source merge hook. Aggregates uncommitted / committed / PR diffs
|
||||
* into one sorted ReviewFile[] tagged with `.source`. Shared by
|
||||
@@ -257,6 +334,8 @@ export function useReviewSources(sessionId: string | null | undefined): UseRevie
|
||||
const statusByRepo = useSessionGitStatusByRepo(sessionId ?? null);
|
||||
const { diff: cumulativeDiff, loading: cumulativeLoading } = useCumulativeDiff(sessionId ?? null);
|
||||
const pr = useActiveTaskPR();
|
||||
const { normalizedGitStatus, normalizedStatusByRepo, prRepoName, useRepositoryKeys } =
|
||||
useReviewRepositoryContext(pr, gitStatus, statusByRepo, cumulativeDiff);
|
||||
const { files: prDiffFiles, loading: prDiffLoading } = usePRDiff(
|
||||
pr?.owner ?? null,
|
||||
pr?.repo ?? null,
|
||||
@@ -266,21 +345,29 @@ export function useReviewSources(sessionId: string | null | undefined): UseRevie
|
||||
const { allFiles, sourceCounts } = useMemo(
|
||||
() =>
|
||||
buildReviewSources({
|
||||
gitStatus,
|
||||
statusByRepo,
|
||||
gitStatus: normalizedGitStatus,
|
||||
statusByRepo: normalizedStatusByRepo,
|
||||
cumulativeDiff: cumulativeDiff as { files?: Record<string, CumulativeFile> } | null,
|
||||
prDiffFiles: prDiffFiles.length > 0 ? prDiffFiles : undefined,
|
||||
prRepoName: pr?.repo || undefined,
|
||||
prRepoName,
|
||||
useRepositoryKeys,
|
||||
}),
|
||||
[gitStatus, statusByRepo, cumulativeDiff, prDiffFiles, pr?.repo],
|
||||
[
|
||||
normalizedGitStatus,
|
||||
normalizedStatusByRepo,
|
||||
cumulativeDiff,
|
||||
prDiffFiles,
|
||||
prRepoName,
|
||||
useRepositoryKeys,
|
||||
],
|
||||
);
|
||||
|
||||
const rawPRFiles = useMemo(() => {
|
||||
if (!prDiffFiles || prDiffFiles.length === 0) return [] as ReviewFile[];
|
||||
const fileMap = new Map<string, ReviewFile>();
|
||||
addPRFiles(fileMap, prDiffFiles, new Set(), pr?.repo || undefined);
|
||||
addPRFiles(fileMap, prDiffFiles, new Set(), prRepoName);
|
||||
return Array.from(fileMap.values());
|
||||
}, [prDiffFiles, pr?.repo]);
|
||||
}, [prDiffFiles, prRepoName]);
|
||||
|
||||
// Keep `hasPR` keyed on the existence of a TaskPR row, not on whether the
|
||||
// PR diff files have loaded yet — avoids the tab bar reflowing the moment
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
fileChangeStatusLabel,
|
||||
normalizeFileChangeStatus,
|
||||
type FileChangeStatus,
|
||||
} from "./file-change-status";
|
||||
|
||||
describe("normalizeFileChangeStatus", () => {
|
||||
it.each<FileChangeStatus>(["added", "modified", "deleted", "untracked", "renamed"])(
|
||||
"preserves local status %s",
|
||||
(status) => {
|
||||
expect(normalizeFileChangeStatus(status)).toBe(status);
|
||||
},
|
||||
);
|
||||
|
||||
it("maps GitHub removed status to deleted", () => {
|
||||
expect(normalizeFileChangeStatus("removed")).toBe("deleted");
|
||||
});
|
||||
|
||||
it.each([undefined, "copied", "changed", "unexpected"])(
|
||||
"falls back to modified for %s",
|
||||
(status) => {
|
||||
expect(normalizeFileChangeStatus(status)).toBe("modified");
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe("fileChangeStatusLabel", () => {
|
||||
it.each<[FileChangeStatus, string]>([
|
||||
["added", "Added"],
|
||||
["untracked", "Untracked"],
|
||||
["modified", "Modified"],
|
||||
["deleted", "Deleted"],
|
||||
["renamed", "Moved"],
|
||||
])("labels %s as %s", (status, label) => {
|
||||
expect(fileChangeStatusLabel(status)).toBe(label);
|
||||
});
|
||||
|
||||
it("includes previous path for moved files", () => {
|
||||
expect(fileChangeStatusLabel("renamed", "src/old-name.ts")).toBe("Moved from src/old-name.ts");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,35 @@
|
||||
export type FileChangeStatus = "modified" | "added" | "deleted" | "untracked" | "renamed";
|
||||
|
||||
export function normalizeFileChangeStatus(status?: string): FileChangeStatus {
|
||||
switch (status) {
|
||||
case "added":
|
||||
case "modified":
|
||||
case "deleted":
|
||||
case "untracked":
|
||||
case "renamed":
|
||||
return status;
|
||||
case "removed":
|
||||
return "deleted";
|
||||
default:
|
||||
return "modified";
|
||||
}
|
||||
}
|
||||
|
||||
export function fileChangeStatusLabel(status: FileChangeStatus, oldPath?: string): string {
|
||||
switch (status) {
|
||||
case "added":
|
||||
return "Added";
|
||||
case "untracked":
|
||||
return "Untracked";
|
||||
case "modified":
|
||||
return "Modified";
|
||||
case "deleted":
|
||||
return "Deleted";
|
||||
case "renamed":
|
||||
return oldPath ? `Moved from ${oldPath}` : "Moved";
|
||||
default: {
|
||||
const exhaustiveStatus: never = status;
|
||||
return exhaustiveStatus;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,72 @@
|
||||
---
|
||||
spec: docs/specs/ui/review-file-status.md
|
||||
created: 2026-07-14
|
||||
status: building
|
||||
---
|
||||
|
||||
# Implementation Plan: Review File Status Cues
|
||||
|
||||
## Overview
|
||||
|
||||
First extract the Changes panel's status mapping and marker into shared frontend primitives. Then preserve status-only files through Review's source builders and render the marker on desktop and mobile with honest patchless states. Finish with focused production-build Playwright coverage. No backend or API changes are required: `FileInfo` and `PRDiffFile` already carry status and previous-path metadata.
|
||||
|
||||
## Frontend
|
||||
|
||||
### Shared status model and marker
|
||||
|
||||
- Add `normalizeFileChangeStatus` and the canonical `FileChangeStatus`/label mapping in `apps/web/lib/utils/file-change-status.ts`; map GitHub `removed` to `deleted`, preserve `renamed`, and fall back to `modified`.
|
||||
- Move `FileStatusIcon` from `apps/web/components/task/file-status-icon.tsx` to `apps/web/components/shared/file-status-icon.tsx`. Preserve the Changes palette, use a distinct arrow for `renamed`, and expose `aria-label`, `title`, `data-file-status`, `oldPath`, and shrink-safe styling.
|
||||
- Update `changes-panel-file-row.tsx`, `changes-panel-pr-files.tsx`, and `mapPRFilesToChangedFiles` in `changes-panel-helpers.ts` to use the shared primitive and normalizer without changing current Changes interactions.
|
||||
|
||||
### Review source completeness and UI
|
||||
|
||||
- Narrow `ReviewFile.status` to the shared status union and add `old_path` in `apps/web/components/review/types.ts`; add status-aware no-diff copy while keeping `diff_skip_reason` authoritative.
|
||||
- Update `buildAllFiles` helpers in `apps/web/components/review/review-dialog.tsx` and `buildReviewSources` helpers in `apps/web/hooks/domains/session/use-review-sources.ts` to normalize every source, preserve `old_path`/`diff_skip_reason`, and retain status-only entries. Keep uncommitted > cumulative > PR precedence and `reviewFileKey` identity.
|
||||
- Render the shared marker as the final `shrink-0` item in `review-file-tree.tsx`, after existing stale/comment metadata.
|
||||
- Render a mobile-only marker in `FileDiffHeader` and status-specific patchless content in `review-diff-list.tsx`.
|
||||
|
||||
## Tests
|
||||
|
||||
- `apps/web/components/shared/file-status-icon.test.tsx`: table-driven visual and accessible contracts for added, untracked, modified, deleted, renamed, and unknown input.
|
||||
- `apps/web/components/task/changes-panel-helpers.test.ts`: shared normalization and previous-path regression coverage.
|
||||
- `apps/web/components/review/review-dialog.build-files.test.ts`: all PR statuses, pure rename inclusion, metadata propagation, and source precedence.
|
||||
- `apps/web/hooks/domains/session/use-review-sources.test.ts`: status-only inclusion and matching normalization across Review/Changes sources.
|
||||
- `apps/web/components/review/review-file-tree.test.tsx` and a focused status-empty-state test beside `types.ts`: marker semantics/layout and skip-reason precedence.
|
||||
|
||||
## E2E Tests
|
||||
|
||||
- `apps/web/e2e/tests/review/review-file-status.spec.ts`: create added, modified, deleted, and renamed files with `GitHelper`, hydrate Changes, open Review, and verify markers, narrow-sidebar layout, and the pure-rename empty state.
|
||||
- `apps/web/e2e/tests/review/mobile-review-file-status.spec.ts`: open Review through the mobile Changes surface and verify the sticky-header marker remains usable without horizontal overflow.
|
||||
|
||||
## Implementation Waves
|
||||
|
||||
Wave 1:
|
||||
|
||||
- [x] [task-01-status-model](task-01-status-model.md) — done
|
||||
|
||||
Wave 2 (depends on Wave 1):
|
||||
|
||||
- [x] [task-02-review-ui](task-02-review-ui.md) — done
|
||||
|
||||
Wave 3 (depends on Wave 2):
|
||||
|
||||
- [x] [task-03-e2e](task-03-e2e.md) — done
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
make fmt
|
||||
make typecheck test lint
|
||||
(cd apps && pnpm --filter @kandev/web test components/shared/file-status-icon.test.tsx components/task/changes-panel-helpers.test.ts components/review/review-file-tree.test.tsx components/review/types.test.ts components/review/review-dialog.build-files.test.ts hooks/domains/session/use-review-sources.test.ts)
|
||||
(cd apps && pnpm --filter @kandev/web e2e:run tests/review/review-file-status.spec.ts)
|
||||
(cd apps && pnpm --filter @kandev/web e2e:run --project mobile-chrome tests/review/mobile-review-file-status.spec.ts)
|
||||
(cd apps/web && pnpm run typecheck)
|
||||
```
|
||||
|
||||
## Risks
|
||||
|
||||
- Including patchless files changes review totals and empty-diff hashes; regression tests must cover marking them reviewed.
|
||||
- Unknown provider statuses must degrade to modified rather than inventing categories.
|
||||
- `old_path` is display metadata only; current path plus repository remains the review identity.
|
||||
- Binary/large/truncated/budget messages must outrank generic patchless copy.
|
||||
- The shared extraction must not change Changes rows' existing hover actions or status/stat visibility.
|
||||
@@ -0,0 +1,49 @@
|
||||
---
|
||||
id: "01-status-model"
|
||||
title: "Shared file status model"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/ui/review-file-status.md"
|
||||
---
|
||||
|
||||
# Task 01: Shared file status model
|
||||
|
||||
Extract the existing Changes status language into a domain-neutral, accessible primitive.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- `normalizeFileChangeStatus` preserves local statuses, maps GitHub `removed` to `deleted`, preserves `renamed`, and maps unknown values to `modified`.
|
||||
- `FileStatusIcon` renders plus/dot/minus/arrow markers with semantic labels and optional previous-path context; untracked remains semantically distinct from added.
|
||||
- Both Changes file-row variants use the shared component without changing row actions or layout.
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps && pnpm --filter @kandev/web test components/shared/file-status-icon.test.tsx components/task/changes-panel-helpers.test.ts`
|
||||
- `cd apps/web && pnpm run typecheck`
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/lib/utils/file-change-status.ts`
|
||||
- `apps/web/components/shared/file-status-icon.tsx`
|
||||
- `apps/web/components/shared/file-status-icon.test.tsx`
|
||||
- `apps/web/components/task/changes-panel-file-row.tsx`
|
||||
- `apps/web/components/task/changes-panel-pr-files.tsx`
|
||||
- `apps/web/components/task/changes-panel-helpers.ts`
|
||||
- `apps/web/components/task/changes-panel-helpers.test.ts`
|
||||
- remove `apps/web/components/task/file-status-icon.tsx`
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec: `What`, marker semantics and accessibility.
|
||||
- Pattern: current `FileStatusIcon` and its call sites in the Changes panel.
|
||||
- Constraint: preserve Changes hover/action behavior.
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the files changed, focused tests/typecheck run, blockers, and residual risks; set this task to `done` and tick it in `plan.md`.
|
||||
@@ -0,0 +1,50 @@
|
||||
---
|
||||
id: "02-review-ui"
|
||||
title: "Review status UI"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on: ["01-status-model"]
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/ui/review-file-status.md"
|
||||
---
|
||||
|
||||
# Task 02: Review status UI
|
||||
|
||||
Carry canonical status metadata through Review and render it on desktop, mobile, and patchless files.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Review retains and normalizes every changed file, including patchless pure renames, while preserving source precedence, multi-repository identity, previous path, and explicit skip reason.
|
||||
- Desktop tree rows show a fixed trailing marker without displacing reviewed, stale, or comment metadata; mobile sticky headers expose the same status without hover.
|
||||
- Patchless files show status-specific explanatory text, while explicit binary/size/truncation/budget reasons take precedence.
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps && pnpm --filter @kandev/web test components/review/review-file-tree.test.tsx components/review/types.test.ts components/review/review-dialog.build-files.test.ts hooks/domains/session/use-review-sources.test.ts`
|
||||
- `cd apps/web && pnpm run typecheck`
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/components/review/types.ts`
|
||||
- `apps/web/components/review/types.test.ts`
|
||||
- `apps/web/components/review/review-dialog.tsx`
|
||||
- `apps/web/components/review/review-dialog.build-files.test.ts`
|
||||
- `apps/web/hooks/domains/session/use-review-sources.ts`
|
||||
- `apps/web/hooks/domains/session/use-review-sources.test.ts`
|
||||
- `apps/web/components/review/review-file-tree.tsx`
|
||||
- `apps/web/components/review/review-file-tree.test.tsx`
|
||||
- `apps/web/components/review/review-diff-list.tsx`
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec: all scenarios.
|
||||
- Task 01: `normalizeFileChangeStatus`, `FileChangeStatus`, and shared `FileStatusIcon`.
|
||||
- Preserve `reviewFileKey`, uncommitted > cumulative > PR precedence, and current `diff_skip_reason` copy.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the files changed, focused tests/typecheck run, blockers, and residual risks; set this task to `done` and tick it in `plan.md`.
|
||||
@@ -0,0 +1,43 @@
|
||||
---
|
||||
id: "03-e2e"
|
||||
title: "Review status E2E coverage"
|
||||
status: done
|
||||
wave: 3
|
||||
depends_on: ["02-review-ui"]
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/ui/review-file-status.md"
|
||||
---
|
||||
|
||||
# Task 03: Review status E2E coverage
|
||||
|
||||
Verify the complete Review workflow against a production Vite build on desktop and mobile.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Desktop Review visibly distinguishes added, modified, deleted, and renamed files and keeps the marker visible at the 160 px sidebar minimum.
|
||||
- A pure rename with no textual patch remains in Review and shows the moved-without-textual-changes state.
|
||||
- Mobile Review exposes the status in the sticky diff header, keeps controls usable, and introduces no document-level horizontal overflow.
|
||||
|
||||
## Verification
|
||||
|
||||
- `cd apps && pnpm --filter @kandev/web e2e:run tests/review/review-file-status.spec.ts`
|
||||
- `cd apps && pnpm --filter @kandev/web e2e:run --project mobile-chrome tests/review/mobile-review-file-status.spec.ts`
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/e2e/tests/review/review-file-status.spec.ts`
|
||||
- `apps/web/e2e/tests/review/mobile-review-file-status.spec.ts`
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec: desktop, patchless, narrow-sidebar, and mobile scenarios.
|
||||
- Task 02 DOM/accessibility contracts; select markers by semantic label or stable data attribute, never by color class.
|
||||
- Patterns: `GitHelper` in `apps/web/e2e/helpers/git-helper.ts`, Review opening in `review-cumulative-diff.spec.ts`, sidebar sizing in `review-sidebar-resize.spec.ts`, and mobile Review entry in `task/mobile-changes-panel.spec.ts`.
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 02.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the files changed, exact E2E command/result, failure artifacts or blockers, and residual risks; set this task to `done` and tick it in `plan.md`.
|
||||
@@ -98,6 +98,7 @@ Subscription quota tracking and per-agent cheap-model profile routing.
|
||||
| [comment-markdown](ui/comment-markdown.md) | shipped |
|
||||
| [empty-turn-notice](ui/empty-turn-notice.md) | shipped |
|
||||
| [acp-shell-command-output](ui/acp-shell-command-output.md) | shipped |
|
||||
| [review-file-status](ui/review-file-status.md) | building |
|
||||
| [slash-command-composer](ui/slash-command-composer.md) | shipped |
|
||||
|
||||
## system-page/ — operational diagnostics & maintenance UI
|
||||
|
||||
@@ -0,0 +1,40 @@
|
||||
---
|
||||
status: building
|
||||
created: 2026-07-14
|
||||
owner: kandev
|
||||
---
|
||||
|
||||
# Review File Status Cues
|
||||
|
||||
## Why
|
||||
|
||||
Reviewers can see changed filenames in Review's file tree, but cannot tell whether a file was added, modified, deleted, or moved without opening its diff. This slows navigation and makes patchless changes, especially pure renames, easy to miss.
|
||||
|
||||
## What
|
||||
|
||||
- Every changed file in Review shows a persistent trailing status marker: green plus for added, amber dot for modified, rose minus for deleted, and purple arrow for moved/renamed.
|
||||
- Untracked files use the added visual language while retaining the accessible label `Untracked`.
|
||||
- The marker does not rely on color alone. Its accessible name and native title describe the status; a moved file may include its previous path when available.
|
||||
- The desktop marker occupies a fixed trailing column. At narrow sidebar widths, the filename truncates before the marker, stale warning, or comment count overlap.
|
||||
- Below the desktop breakpoint, where the file tree is hidden, the same status marker appears in each sticky diff header. No required cue depends on hover.
|
||||
- Review includes changed files even when they have no textual patch, including pure renames. Their diff body shows status-specific explanatory text instead of an indefinite loading message.
|
||||
- Existing explicit skip reasons for binary, oversized, truncated, or budget-exceeded diffs take precedence over generic status-specific text.
|
||||
- Existing file icons, tree hierarchy, filtering, review checkboxes, stale indicators, comment counts, source precedence, and multi-repository identity remain unchanged.
|
||||
|
||||
## Scenarios
|
||||
|
||||
- **GIVEN** added, modified, deleted, and renamed files, **WHEN** Review opens, **THEN** its desktop tree shows four distinct markers aligned at the trailing edge of their rows.
|
||||
- **GIVEN** an untracked file, **WHEN** Review opens, **THEN** it uses the added plus marker and its accessible name says `Untracked`.
|
||||
- **GIVEN** a pure rename with no textual patch, **WHEN** Review opens, **THEN** the file remains reviewable, uses the moved marker, and its body explains that the move has no textual changes.
|
||||
- **GIVEN** another changed file with no textual patch, **WHEN** its diff body renders, **THEN** Review shows status-appropriate empty-state text rather than `Loading diff...`.
|
||||
- **GIVEN** a file with an explicit diff skip reason, **WHEN** its body renders, **THEN** the skip-reason message is shown instead of the generic status empty state.
|
||||
- **GIVEN** a 160 px Review sidebar and a long filename, **WHEN** the row renders, **THEN** the filename truncates while the status marker, stale warning, and comment count remain visible without overlap.
|
||||
- **GIVEN** a mobile viewport, **WHEN** the user reviews a changed file, **THEN** its sticky diff header exposes the status marker without horizontal page overflow.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Grouping or filtering Review files by status.
|
||||
- Replacing compact markers with visible `A`/`M`/`D`/`R` badges or filename tinting.
|
||||
- A new mobile file navigator.
|
||||
- Status animations.
|
||||
- Backend or API changes; Review consumes existing git and pull-request status metadata.
|
||||
Reference in New Issue
Block a user