feat(web/review): group file tree by repository for multi-repo task changes
The review file tree now renders per-repo top-level nodes when a task's changes span multiple repositories, so the same file path in two repos no longer collapses into a single ambiguous entry. - ReviewFile.repository_id and ReviewFile.repository_name (optional) identify which repo each file belongs to. Single-repo callers leave them unset and the tree falls back to the existing flat grouping. - buildFileTree splits into buildMultiRepoTree (when 2+ distinct repository_names are present) and buildFlatTree (the original logic). Repo roots have isRepoRoot=true and a repositoryId for downstream filters; their children are full path subtrees per repo. - collapse() skips repo roots so a repo with a single changed file still shows its repo header. - ReviewFileTree renders repo roots with a stronger label, a top border, and an "N files" count badge; data-testid="repo-root-node" for E2E coverage. File-level rendering is unchanged. - Tests cover the flat-tree fallback (no repo info, single repo), multi-repo grouping, repo-root pinning, and same-filename-different- repos isolation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -165,11 +165,16 @@ function TreeNode({
|
||||
};
|
||||
|
||||
if (node.isDir) {
|
||||
// Repo roots get a stronger label so the user sees the multi-repo grouping.
|
||||
const fileCount = node.isRepoRoot ? countLeafFiles(node) : 0;
|
||||
return (
|
||||
<div>
|
||||
<div data-testid={node.isRepoRoot ? "repo-root-node" : "dir-node"}>
|
||||
<button
|
||||
type="button"
|
||||
className="flex items-center w-full gap-1 px-2 py-1 hover:bg-muted/50 transition-colors cursor-pointer"
|
||||
className={cn(
|
||||
"flex items-center w-full gap-1 px-2 py-1 hover:bg-muted/50 transition-colors cursor-pointer",
|
||||
node.isRepoRoot && "border-t border-border/40 first:border-t-0 mt-1 first:mt-0",
|
||||
)}
|
||||
style={{ paddingLeft: `${depth * 12 + 8}px` }}
|
||||
onClick={handleToggle}
|
||||
>
|
||||
@@ -178,7 +183,17 @@ function TreeNode({
|
||||
) : (
|
||||
<IconChevronRight className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
||||
)}
|
||||
<span className="text-[13px] text-muted-foreground truncate">{node.name}</span>
|
||||
<span
|
||||
className={cn(
|
||||
"text-[13px] truncate",
|
||||
node.isRepoRoot ? "font-medium text-foreground" : "text-muted-foreground",
|
||||
)}
|
||||
>
|
||||
{node.name}
|
||||
</span>
|
||||
{node.isRepoRoot && fileCount > 0 && (
|
||||
<span className="ml-auto text-[10px] text-muted-foreground/70">{fileCount}</span>
|
||||
)}
|
||||
</button>
|
||||
{expanded && node.children && (
|
||||
<div>
|
||||
@@ -193,3 +208,12 @@ function TreeNode({
|
||||
|
||||
return <FileNode node={node} {...sharedProps} depth={depth} />;
|
||||
}
|
||||
|
||||
/** Counts leaf (file) nodes anywhere under the given node. Used for the
|
||||
* "N files" badge on multi-repo repo-root headers. */
|
||||
function countLeafFiles(node: FileTreeNode): number {
|
||||
if (!node.isDir) return 1;
|
||||
let total = 0;
|
||||
for (const child of node.children ?? []) total += countLeafFiles(child);
|
||||
return total;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { buildFileTree, type ReviewFile } from "./types";
|
||||
|
||||
function file(overrides: Partial<ReviewFile>): ReviewFile {
|
||||
return {
|
||||
path: "src/app.tsx",
|
||||
diff: "",
|
||||
status: "modified",
|
||||
additions: 1,
|
||||
deletions: 0,
|
||||
staged: false,
|
||||
source: "uncommitted",
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
describe("buildFileTree — multi-repo", () => {
|
||||
it("falls back to flat tree when files have no repository_name", () => {
|
||||
const tree = buildFileTree([file({ path: "src/a.ts" }), file({ path: "src/b.ts" })]);
|
||||
expect(tree[0].isRepoRoot).toBeUndefined();
|
||||
expect(tree[0].name).toContain("src");
|
||||
});
|
||||
|
||||
it("falls back to flat tree when only one repository is present", () => {
|
||||
const tree = buildFileTree([
|
||||
file({ path: "src/a.ts", repository_name: "frontend", repository_id: "f" }),
|
||||
file({ path: "src/b.ts", repository_name: "frontend", repository_id: "f" }),
|
||||
]);
|
||||
expect(tree[0].isRepoRoot).toBeUndefined();
|
||||
});
|
||||
|
||||
it("groups by repo when 2+ distinct repositories are present", () => {
|
||||
const tree = buildFileTree([
|
||||
file({ path: "src/app.tsx", repository_name: "frontend", repository_id: "f" }),
|
||||
file({ path: "src/api.ts", repository_name: "frontend", repository_id: "f" }),
|
||||
file({ path: "handlers/task.go", repository_name: "backend", repository_id: "b" }),
|
||||
]);
|
||||
expect(tree).toHaveLength(2);
|
||||
expect(tree.map((n) => n.name).sort()).toEqual(["backend", "frontend"]);
|
||||
for (const root of tree) {
|
||||
expect(root.isRepoRoot).toBe(true);
|
||||
expect(root.repositoryId).toBeDefined();
|
||||
}
|
||||
});
|
||||
|
||||
it("repo roots are not collapsed even when they have a single child", () => {
|
||||
const tree = buildFileTree([
|
||||
file({ path: "lonely.ts", repository_name: "shared", repository_id: "s" }),
|
||||
file({ path: "src/x.ts", repository_name: "main", repository_id: "m" }),
|
||||
]);
|
||||
const shared = tree.find((n) => n.name === "shared");
|
||||
expect(shared).toBeDefined();
|
||||
expect(shared?.isRepoRoot).toBe(true);
|
||||
expect(shared?.children).toHaveLength(1);
|
||||
expect(shared?.children?.[0].name).toBe("lonely.ts");
|
||||
});
|
||||
|
||||
it("preserves file paths inside each repo (no leakage between repos)", () => {
|
||||
const tree = buildFileTree([
|
||||
file({ path: "README.md", repository_name: "frontend", repository_id: "f" }),
|
||||
file({ path: "README.md", repository_name: "backend", repository_id: "b" }),
|
||||
]);
|
||||
const frontend = tree.find((n) => n.name === "frontend");
|
||||
const backend = tree.find((n) => n.name === "backend");
|
||||
expect(frontend?.children?.[0].name).toBe("README.md");
|
||||
expect(backend?.children?.[0].name).toBe("README.md");
|
||||
// File node back-refs point to the right repo.
|
||||
expect(frontend?.children?.[0].file?.repository_id).toBe("f");
|
||||
expect(backend?.children?.[0].file?.repository_id).toBe("b");
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,14 @@ export type ReviewFile = {
|
||||
staged: boolean;
|
||||
source: "uncommitted" | "committed" | "pr";
|
||||
diff_skip_reason?: "too_large" | "binary" | "truncated" | "budget_exceeded";
|
||||
/**
|
||||
* Repository this file belongs to. Set on multi-repo task changes so the
|
||||
* file tree groups files under per-repo top-level nodes. Optional for
|
||||
* single-repo tasks; the tree falls back to flat path-only grouping.
|
||||
*/
|
||||
repository_id?: string;
|
||||
/** Human-readable repo name for tree node labels (e.g. "frontend"). */
|
||||
repository_name?: string;
|
||||
};
|
||||
|
||||
export function diffSkipReasonLabel(reason?: string): string {
|
||||
@@ -32,6 +40,14 @@ export type FileTreeNode = {
|
||||
isDir: boolean;
|
||||
children?: FileTreeNode[];
|
||||
file?: ReviewFile;
|
||||
/**
|
||||
* True when this node represents a repository root in a multi-repo file
|
||||
* tree. Repo roots are pinned (never collapsed into their first child) so
|
||||
* the user always sees the repo grouping at the top level.
|
||||
*/
|
||||
isRepoRoot?: boolean;
|
||||
/** Repository id this node represents (only set when isRepoRoot is true). */
|
||||
repositoryId?: string;
|
||||
};
|
||||
|
||||
/**
|
||||
@@ -61,8 +77,54 @@ export const hashDiff = djb2Hash;
|
||||
/**
|
||||
* Build a hierarchical file tree from flat file paths.
|
||||
* Collapses single-child directories (e.g., `src/components` as one node if `src` has no other children).
|
||||
*
|
||||
* Multi-repo: when files carry repository_name and 2+ distinct repos are
|
||||
* present, the tree gets a top-level repo node per repository so the user
|
||||
* sees per-repo grouping. Repo roots are pinned (never collapsed) and the
|
||||
* file paths inside them stay relative to the repo root.
|
||||
*/
|
||||
export function buildFileTree(files: ReviewFile[]): FileTreeNode[] {
|
||||
const repoNames = new Set<string>();
|
||||
for (const f of files) {
|
||||
if (f.repository_name) repoNames.add(f.repository_name);
|
||||
}
|
||||
const isMultiRepo = repoNames.size > 1;
|
||||
|
||||
if (isMultiRepo) return buildMultiRepoTree(files);
|
||||
return buildFlatTree(files);
|
||||
}
|
||||
|
||||
function buildMultiRepoTree(files: ReviewFile[]): FileTreeNode[] {
|
||||
// Group by repo first, then build a sub-tree for each.
|
||||
const byRepo = new Map<string, { name: string; id: string; files: ReviewFile[] }>();
|
||||
for (const f of files) {
|
||||
const name = f.repository_name ?? "(unspecified)";
|
||||
const id = f.repository_id ?? name;
|
||||
const key = id;
|
||||
let entry = byRepo.get(key);
|
||||
if (!entry) {
|
||||
entry = { name, id, files: [] };
|
||||
byRepo.set(key, entry);
|
||||
}
|
||||
entry.files.push(f);
|
||||
}
|
||||
const repoRoots: FileTreeNode[] = [];
|
||||
for (const [, entry] of byRepo) {
|
||||
const subtree = buildFlatTree(entry.files);
|
||||
repoRoots.push({
|
||||
name: entry.name,
|
||||
path: `__repo__:${entry.id}`,
|
||||
isDir: true,
|
||||
isRepoRoot: true,
|
||||
repositoryId: entry.id,
|
||||
children: subtree,
|
||||
});
|
||||
}
|
||||
// Stable alphabetical order by repo name.
|
||||
return repoRoots.sort((a, b) => a.name.localeCompare(b.name));
|
||||
}
|
||||
|
||||
function buildFlatTree(files: ReviewFile[]): FileTreeNode[] {
|
||||
const root: FileTreeNode = { name: "", path: "", isDir: true, children: [] };
|
||||
|
||||
for (const file of files) {
|
||||
@@ -92,13 +154,20 @@ export function buildFileTree(files: ReviewFile[]): FileTreeNode[] {
|
||||
}
|
||||
}
|
||||
|
||||
// Collapse single-child directories
|
||||
// Collapse single-child directories. Repo roots are never collapsed —
|
||||
// they encode user-meaningful grouping that survives even when the repo
|
||||
// has only one changed file.
|
||||
function collapse(node: FileTreeNode): FileTreeNode {
|
||||
if (!node.isDir || !node.children) return node;
|
||||
|
||||
node.children = node.children.map(collapse);
|
||||
|
||||
if (node.children.length === 1 && node.children[0].isDir && node.name !== "") {
|
||||
if (
|
||||
node.children.length === 1 &&
|
||||
node.children[0].isDir &&
|
||||
node.name !== "" &&
|
||||
!node.isRepoRoot
|
||||
) {
|
||||
const child = node.children[0];
|
||||
return {
|
||||
...child,
|
||||
|
||||
Reference in New Issue
Block a user