fix(code-review): order diff review files to match the changes file tree

packages/core/src/git-interaction/changeTree.test.ts

@@ -0,0 +1,173 @@
+import type { ChangedFile } from "@posthog/shared/domain-types";
+import { describe, expect, it } from "vitest";
+import {
+  buildChangeTree,
+  compactChangeTree,
+  flattenChangeTree,
+  orderPathsLikeChangeTree,
+  sortByChangeTreeOrder,
+} from "./changeTree";
+
+const file = (path: string, over: Partial<ChangedFile> = {}): ChangedFile => ({
+  path,
+  status: "modified",
+  ...over,
+});
+
+const paths = (files: ChangedFile[]) => files.map((f) => f.path);
+
+describe("buildChangeTree", () => {
+  it("nests files under their directory parts", () => {
+    const tree = buildChangeTree([
+      file("src/a.ts"),
+      file("src/utils/b.ts"),
+      file("root.ts"),
+    ]);
+    expect(tree.files.map((f) => f.path)).toEqual(["root.ts"]);
+    expect([...tree.children.keys()]).toEqual(["src"]);
+    const src = tree.children.get("src");
+    expect(src?.files.map((f) => f.path)).toEqual(["src/a.ts"]);
+    expect([...(src?.children.keys() ?? [])]).toEqual(["utils"]);
+  });
+});
+
+describe("compactChangeTree", () => {
+  it("collapses single-child directory chains into one node", () => {
+    const tree = compactChangeTree(buildChangeTree([file("src/a/b/c.ts")]));
+    const src = tree.children.get("src");
+    expect(src?.name).toBe("src/a/b");
+    expect(src?.files.map((f) => f.path)).toEqual(["src/a/b/c.ts"]);
+  });
+
+  it("does not collapse a directory that holds files", () => {
+    const tree = compactChangeTree(
+      buildChangeTree([file("src/a/b.ts"), file("src/c.ts")]),
+    );
+    const src = tree.children.get("src");
+    expect(src?.name).toBe("src");
+    expect([...(src?.children.keys() ?? [])]).toEqual(["a"]);
+  });
+});
+
+describe("flattenChangeTree", () => {
+  it("groups a directory's files together ahead of later siblings", () => {
+    const ordered = flattenChangeTree([
+      file("zeta.txt"),
+      file("alpha.txt"),
+      file("src/mid.ts"),
+      file("src/beta.ts"),
+    ]);
+    // Directories sort before sibling files at every node, so the src/ group
+    // precedes root-level alpha.txt and zeta.txt.
+    expect(paths(ordered)).toEqual([
+      "src/beta.ts",
+      "src/mid.ts",
+      "alpha.txt",
+      "zeta.txt",
+    ]);
+  });
+
+  it("sorts case-insensitively, matching the file tree (not git byte order)", () => {
+    const ordered = flattenChangeTree([
+      file("Beta.txt"),
+      file("ZETA_CAPS.txt"),
+      file("alpha.txt"),
+      file("zeta.txt"),
+    ]);
+    expect(paths(ordered)).toEqual([
+      "alpha.txt",
+      "Beta.txt",
+      "ZETA_CAPS.txt",
+      "zeta.txt",
+    ]);
+  });
+
+  it("interleaves untracked files with modified ones by path", () => {
+    const ordered = flattenChangeTree([
+      file("a.ts", { status: "modified" }),
+      file("untracked.ts", { status: "untracked" }),
+      file("b.ts", { status: "modified" }),
+    ]);
+    expect(paths(ordered)).toEqual(["a.ts", "b.ts", "untracked.ts"]);
+  });
+
+  it("sorts files within a directory by basename", () => {
+    const ordered = flattenChangeTree([
+      file("src/mid.ts"),
+      file("src/Apple.ts"),
+      file("src/beta.ts"),
+    ]);
+    expect(paths(ordered)).toEqual([
+      "src/Apple.ts",
+      "src/beta.ts",
+      "src/mid.ts",
+    ]);
+  });
+
+  it("renders directories before sibling files at every level", () => {
+    const ordered = flattenChangeTree([
+      file("root_file.ts"),
+      file("dir/inside.ts"),
+    ]);
+    expect(paths(ordered)).toEqual(["dir/inside.ts", "root_file.ts"]);
+  });
+
+  it("returns an empty list for no files", () => {
+    expect(flattenChangeTree([])).toEqual([]);
+  });
+});
+
+describe("orderPathsLikeChangeTree", () => {
+  it("orders paths like the file tree, not git byte order", () => {
+    expect(
+      orderPathsLikeChangeTree([
+        "Beta.txt",
+        "ZETA_CAPS.txt",
+        "alpha.txt",
+        "src/Apple.ts",
+        "src/beta.ts",
+        "zeta.txt",
+      ]),
+    ).toEqual([
+      "src/Apple.ts",
+      "src/beta.ts",
+      "alpha.txt",
+      "Beta.txt",
+      "ZETA_CAPS.txt",
+      "zeta.txt",
+    ]);
+  });
+
+  it("returns an empty list for no paths", () => {
+    expect(orderPathsLikeChangeTree([])).toEqual([]);
+  });
+});
+
+describe("sortByChangeTreeOrder", () => {
+  const item = (key: string, filePaths: string[]) => ({ key, filePaths });
+
+  it("reorders items to match the given tree order", () => {
+    const items = [
+      item("a", ["zeta.txt"]),
+      item("b", ["alpha.txt"]),
+      item("c", ["src/x.ts"]),
+    ];
+    const ordered = sortByChangeTreeOrder(items, [
+      "src/x.ts",
+      "alpha.txt",
+      "zeta.txt",
+    ]);
+    expect(ordered.map((i) => i.key)).toEqual(["c", "b", "a"]);
+  });
+
+  it("keeps items whose path is not in the order last", () => {
+    const items = [item("a", ["unknown.ts"]), item("b", ["alpha.txt"])];
+    const ordered = sortByChangeTreeOrder(items, ["alpha.txt"]);
+    expect(ordered.map((i) => i.key)).toEqual(["b", "a"]);
+  });
+
+  it("returns the same array reference when there is no tree order", () => {
+    const items = [item("a", ["zeta.txt"]), item("b", ["alpha.txt"])];
+    expect(sortByChangeTreeOrder(items, [])).toBe(items);
+  });
+});

packages/core/src/git-interaction/changeTree.ts

@@ -0,0 +1,112 @@
+import type { ChangedFile } from "@posthog/shared/domain-types";
+
+export interface ChangeTreeNode {
+  name: string;
+  path: string;
+  children: Map<string, ChangeTreeNode>;
+  files: ChangedFile[];
+}
+
+export function buildChangeTree(files: ChangedFile[]): ChangeTreeNode {
+  const root: ChangeTreeNode = {
+    name: "",
+    path: "",
+    children: new Map(),
+    files: [],
+  };
+  for (const file of files) {
+    const parts = file.path.split("/");
+    let node = root;
+    for (let i = 0; i < parts.length - 1; i++) {
+      const part = parts[i];
+      if (!node.children.has(part)) {
+        node.children.set(part, {
+          name: part,
+          path: parts.slice(0, i + 1).join("/"),
+          children: new Map(),
+          files: [],
+        });
+      }
+      const child = node.children.get(part);
+      if (!child) break;
+      node = child;
+    }
+    node.files.push(file);
+  }
+  return root;
+}
+
+export function compactChangeTree(node: ChangeTreeNode): ChangeTreeNode {
+  const compacted = new Map<string, ChangeTreeNode>();
+  for (const [key, child] of node.children) {
+    let current = child;
+    let label = current.name;
+    while (current.children.size === 1 && current.files.length === 0) {
+      const [, only] = [...current.children.entries()][0];
+      label = `${label}/${only.name}`;
+      current = only;
+    }
+    const result = compactChangeTree(current);
+    result.name = label;
+    compacted.set(key, result);
+  }
+  return { ...node, children: compacted };
+}
+
+const compareLocale = (a: string, b: string) => a.localeCompare(b);
+
+export function orderedTreeDirs(node: ChangeTreeNode): ChangeTreeNode[] {
+  return [...node.children.values()].sort((a, b) =>
+    compareLocale(a.name, b.name),
+  );
+}
+
+export function orderedTreeFiles(node: ChangeTreeNode): ChangedFile[] {
+  return [...node.files].sort((a, b) => {
+    const aName = a.path.split("/").pop() ?? "";
+    const bName = b.path.split("/").pop() ?? "";
+    return compareLocale(aName, bName);
+  });
+}
+
+function flattenNode(node: ChangeTreeNode, out: ChangedFile[]) {
+  for (const child of orderedTreeDirs(node)) {
+    flattenNode(child, out);
+  }
+  for (const file of orderedTreeFiles(node)) {
+    out.push(file);
+  }
+}
+
+export function flattenChangeTree(files: ChangedFile[]): ChangedFile[] {
+  const tree = compactChangeTree(buildChangeTree(files));
+  const out: ChangedFile[] = [];
+  flattenNode(tree, out);
+  return out;
+}
+
+export function orderPathsLikeChangeTree(paths: string[]): string[] {
+  const file = (path: string): ChangedFile => ({ path, status: "modified" });
+  return flattenChangeTree(paths.map(file)).map((f) => f.path);
+}
+
+export function sortByChangeTreeOrder<T extends { filePaths?: string[] }>(
+  items: T[],
+  orderedPaths: string[],
+): T[] {
+  if (orderedPaths.length === 0) return items;
+
+  const order = new Map<string, number>();
+  for (let i = 0; i < orderedPaths.length; i++) {
+    order.set(orderedPaths[i], i);
+  }
+
+  const rank = (item: T): number => {
+    const path = item.filePaths?.find((p) => order.has(p));
+    return path !== undefined
+      ? (order.get(path) ?? Number.MAX_SAFE_INTEGER)
+      : Number.MAX_SAFE_INTEGER;
+  };
+
+  return [...items].sort((a, b) => rank(a) - rank(b));
+}