From 725e73d32f0f4ef617171b35826228a2fd546cb9 Mon Sep 17 00:00:00 2001 From: Adam Bowker Date: Wed, 22 Jul 2026 19:35:11 -0400 Subject: [PATCH 1/2] fix(code-review): order diff review files to match the changes file tree The diff review panel listed files in git-diff order (byte/ASCII sort: uppercase first) while the changes file tree beside it sorts with localeCompare and groups directory contents together, so the two views diverged as you scrolled. Untracked files also clustered at the end of "Changes" in the review instead of interleaving by path like the tree. Extract the tree's build/compact/flatten logic into a shared @posthog/core helper and have both the changes tree and the review list consume it, so the review mirrors the tree's leaf order (staged group and the merged unstaged+untracked "Changes" group) by construction. Generated-By: PostHog Code Task-Id: 4eb23888-6afa-4fb9-9b7a-22d6a48e1721 --- .../src/git-interaction/changeTree.test.ts | 173 ++++++++++++++++++ .../core/src/git-interaction/changeTree.ts | 107 +++++++++++ .../code-review/components/ReviewPage.tsx | 169 ++++++++++------- .../components/reviewItemBuilders.test.ts | 63 ++++++- .../components/reviewItemBuilders.tsx | 7 + .../components/ChangesTreeView.tsx | 55 +----- 6 files changed, 457 insertions(+), 117 deletions(-) create mode 100644 packages/core/src/git-interaction/changeTree.test.ts create mode 100644 packages/core/src/git-interaction/changeTree.ts diff --git a/packages/core/src/git-interaction/changeTree.test.ts b/packages/core/src/git-interaction/changeTree.test.ts new file mode 100644 index 0000000000..b8d1e33dd9 --- /dev/null +++ b/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 => ({ + 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); + }); +}); diff --git a/packages/core/src/git-interaction/changeTree.ts b/packages/core/src/git-interaction/changeTree.ts new file mode 100644 index 0000000000..c4090b6f22 --- /dev/null +++ b/packages/core/src/git-interaction/changeTree.ts @@ -0,0 +1,107 @@ +import type { ChangedFile } from "@posthog/shared/domain-types"; + +export interface ChangeTreeNode { + name: string; + path: string; + children: Map; + 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(); + 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); + +function flattenNode(node: ChangeTreeNode, out: ChangedFile[]) { + const sortedDirs = [...node.children.values()].sort((a, b) => + compareLocale(a.name, b.name), + ); + const sortedFiles = [...node.files].sort((a, b) => { + const aName = a.path.split("/").pop() ?? ""; + const bName = b.path.split("/").pop() ?? ""; + return compareLocale(aName, bName); + }); + + for (const child of sortedDirs) { + flattenNode(child, out); + } + for (const file of sortedFiles) { + 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( + items: T[], + orderedPaths: string[], +): T[] { + if (orderedPaths.length === 0) return items; + + const order = new Map(); + 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)); +} diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index 4954d2ccb4..b6863133ce 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -33,7 +33,9 @@ import { buildRemoteReviewItems, buildUntrackedReviewItems, changedFileSignature, + orderPathsLikeTree, patchFileSignature, + sortReviewItemsByTreeOrder, } from "./reviewItemBuilders"; const EMPTY_CHANGED_FILES: ChangedFile[] = []; @@ -344,16 +346,10 @@ function LocalReviewContent({ return map; }, [stagedParsedFiles, unstagedParsedFiles, untrackedFiles]); - const items = useMemo(() => { - const reviewItems: ReviewListItem[] = []; - - if (hasStagedFiles && stagedParsedFiles.length > 0) { - reviewItems.push({ - key: "section:staged", - node: , - }); - reviewItems.push( - ...buildPatchReviewItems({ + const stagedItems = useMemo( + () => + sortReviewItemsByTreeOrder( + buildPatchReviewItems({ files: stagedParsedFiles, staged: true, repoPath, @@ -367,66 +363,98 @@ function LocalReviewContent({ prUrl, commentThreads, }), - ); + orderPathsLikeTree( + stagedParsedFiles.map((f) => f.name ?? f.prevName ?? ""), + ), + ), + [ + collapsedFiles, + commentThreads, + diffOptions, + onDiscardFile, + onStageFile, + openFile, + prUrl, + repoPath, + stagedParsedFiles, + taskId, + toggleFile, + ], + ); + + const changesItems = useMemo( + () => + sortReviewItemsByTreeOrder( + [ + ...buildPatchReviewItems({ + files: unstagedParsedFiles, + alsoStagedPaths: stagedPathSet, + repoPath, + taskId, + diffOptions, + collapsedFiles, + toggleFile, + openFile, + onDiscardFile, + onStageFile, + prUrl, + commentThreads, + }), + ...buildUntrackedReviewItems({ + files: untrackedFiles, + repoPath, + taskId, + diffOptions, + collapsedFiles, + toggleFile, + onDiscardFile, + onStageFile, + }), + ], + orderPathsLikeTree([ + ...unstagedParsedFiles.map((f) => f.name ?? f.prevName ?? ""), + ...untrackedFiles.map((f) => f.path), + ]), + ), + [ + collapsedFiles, + commentThreads, + diffOptions, + onDiscardFile, + onStageFile, + openFile, + prUrl, + repoPath, + stagedPathSet, + taskId, + toggleFile, + untrackedFiles, + unstagedParsedFiles, + ], + ); + + const items = useMemo(() => { + const reviewItems: ReviewListItem[] = []; + + if (hasStagedFiles && stagedItems.length > 0) { + reviewItems.push({ + key: "section:staged", + node: , + }); + reviewItems.push(...stagedItems); } - if ( - hasStagedFiles && - (unstagedParsedFiles.length > 0 || untrackedFiles.length > 0) - ) { + if (hasStagedFiles && changesItems.length > 0) { reviewItems.push({ key: "section:changes", node: , }); } - reviewItems.push( - ...buildPatchReviewItems({ - files: unstagedParsedFiles, - alsoStagedPaths: stagedPathSet, - repoPath, - taskId, - diffOptions, - collapsedFiles, - toggleFile, - openFile, - onDiscardFile, - onStageFile, - prUrl, - commentThreads, - }), - ); - reviewItems.push( - ...buildUntrackedReviewItems({ - files: untrackedFiles, - repoPath, - taskId, - diffOptions, - collapsedFiles, - toggleFile, - onDiscardFile, - onStageFile, - }), - ); + reviewItems.push(...changesItems); return reviewItems; - }, [ - collapsedFiles, - commentThreads, - diffOptions, - hasStagedFiles, - onDiscardFile, - onStageFile, - openFile, - prUrl, - repoPath, - stagedParsedFiles, - stagedPathSet, - taskId, - toggleFile, - untrackedFiles, - unstagedParsedFiles, - ]); + }, [changesItems, hasStagedFiles, stagedItems]); return ( - buildRemoteReviewItems({ - files, - taskId, - prUrl, - options: reviewState.diffOptions, - collapsedFiles: reviewState.collapsedFiles, - toggleFile: reviewState.toggleFile, - commentThreads, - }), + sortReviewItemsByTreeOrder( + buildRemoteReviewItems({ + files, + taskId, + prUrl, + options: reviewState.diffOptions, + collapsedFiles: reviewState.collapsedFiles, + toggleFile: reviewState.toggleFile, + commentThreads, + }), + orderPathsLikeTree(files.map((f) => f.path)), + ), [ commentThreads, files, diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts index a9a1bf4a56..6bdd65ffbe 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts @@ -1,6 +1,12 @@ import type { ChangedFile } from "@posthog/shared/domain-types"; import { describe, expect, it } from "vitest"; -import { changedFileSignature, patchFileSignature } from "./reviewItemBuilders"; +import type { ReviewListItem } from "../commentFileFilter"; +import { + changedFileSignature, + orderPathsLikeTree, + patchFileSignature, + sortReviewItemsByTreeOrder, +} from "./reviewItemBuilders"; const changedFile = (over: Partial): ChangedFile => ({ path: "a.ts", @@ -75,3 +81,58 @@ describe("patchFileSignature", () => { expect(a).not.toBe(b); }); }); + +describe("orderPathsLikeTree", () => { + it("orders paths like the file tree, not git byte order", () => { + expect( + orderPathsLikeTree([ + "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", + ]); + }); +}); + +describe("sortReviewItemsByTreeOrder", () => { + const item = (key: string, filePaths: string[]): ReviewListItem => ({ + key, + filePaths, + node: null, + }); + + it("reorders file items to match the given tree order", () => { + const items = [ + item("a", ["zeta.txt"]), + item("b", ["alpha.txt"]), + item("c", ["src/x.ts"]), + ]; + const ordered = sortReviewItemsByTreeOrder(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 = sortReviewItemsByTreeOrder(items, ["alpha.txt"]); + expect(ordered.map((i) => i.key)).toEqual(["b", "a"]); + }); + + it("returns the same array when there is no tree order", () => { + const items = [item("a", ["zeta.txt"]), item("b", ["alpha.txt"])]; + expect(sortReviewItemsByTreeOrder(items, [])).toBe(items); + }); +}); diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index 4a77b6711e..971f2e1baa 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -5,12 +5,19 @@ import { computeSkipExpansion, } from "@posthog/core/code-review/reviewItemKeys"; import type { PrCommentThread } from "@posthog/core/code-review/types"; +import { + orderPathsLikeChangeTree, + sortByChangeTreeOrder, +} from "@posthog/core/git-interaction/changeTree"; import type { ChangedFile } from "@posthog/shared/domain-types"; import { makeFileKey } from "../../git-interaction/utils/fileKey"; import type { ReviewListItem } from "../commentFileFilter"; import type { DiffOptions } from "../types"; import { PatchRow, RemoteRow, UntrackedRow } from "./ReviewRows"; +export const orderPathsLikeTree = orderPathsLikeChangeTree; +export const sortReviewItemsByTreeOrder = sortByChangeTreeOrder; + export function changedFileSignature(file: ChangedFile): string | null { if (file.patch) return contentHash(file.patch); if (file.sha) return `${file.status}:${file.sha}`; diff --git a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx index e21aad27cd..2d9e5906cd 100644 --- a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx +++ b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx @@ -1,55 +1,16 @@ +import { + buildChangeTree, + type ChangeTreeNode, + compactChangeTree, +} from "@posthog/core/git-interaction/changeTree"; import type { ChangedFile } from "@posthog/shared/domain-types"; import { TreeDirectoryRow } from "@posthog/ui/primitives/TreeDirectoryRow"; import { useCallback, useMemo, useState } from "react"; -export interface TreeNode { - name: string; - path: string; - children: Map; - files: ChangedFile[]; -} - -export function buildChangesTree(files: ChangedFile[]): TreeNode { - const root: TreeNode = { 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 type TreeNode = ChangeTreeNode; -/** Collapse single-child directory chains into one node (e.g. "src/utils") */ -export function compactTree(node: TreeNode): TreeNode { - const compacted = new Map(); - 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 = compactTree(current); - result.name = label; - compacted.set(key, result); - } - return { ...node, children: compacted }; -} +export const buildChangesTree = buildChangeTree; +export const compactTree = compactChangeTree; interface ChangesTreeNodeProps { node: TreeNode; From b5907d7ff2acaea80b651787f7cc2354cdfe26d0 Mon Sep 17 00:00:00 2001 From: Adam Bowker Date: Wed, 22 Jul 2026 19:46:09 -0400 Subject: [PATCH 2/2] refactor(code-review): share tree traversal order, drop re-export indirection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cleanup pass on the diff-review-matches-file-tree change: - Extract orderedTreeDirs/orderedTreeFiles into the shared changeTree helper and have both flattenNode (review ordering) and ChangesTreeView (the rendered tree) consume them. The two surfaces now share one traversal, so they can't drift on sort order again — the original bug. - Drop the orderPathsLikeTree/sortReviewItemsByTreeOrder re-export aliases from reviewItemBuilders; ReviewPage imports the core helpers directly. Removes a layer of indirection with no consumer relying on the renamed names. - Drop the unused TreeNode/buildChangesTree/compactTree back-compat aliases from ChangesTreeView; it uses the core names directly. Net -73 lines; behavior unchanged. Generated-By: PostHog Code Task-Id: 4eb23888-6afa-4fb9-9b7a-22d6a48e1721 --- .../core/src/git-interaction/changeTree.ts | 15 +++-- .../code-review/components/ReviewPage.tsx | 18 +++--- .../components/reviewItemBuilders.test.ts | 63 +------------------ .../components/reviewItemBuilders.tsx | 7 --- .../components/ChangesTreeView.tsx | 30 +++------ 5 files changed, 30 insertions(+), 103 deletions(-) diff --git a/packages/core/src/git-interaction/changeTree.ts b/packages/core/src/git-interaction/changeTree.ts index c4090b6f22..e40e82e6c5 100644 --- a/packages/core/src/git-interaction/changeTree.ts +++ b/packages/core/src/git-interaction/changeTree.ts @@ -55,20 +55,25 @@ export function compactChangeTree(node: ChangeTreeNode): ChangeTreeNode { const compareLocale = (a: string, b: string) => a.localeCompare(b); -function flattenNode(node: ChangeTreeNode, out: ChangedFile[]) { - const sortedDirs = [...node.children.values()].sort((a, b) => +export function orderedTreeDirs(node: ChangeTreeNode): ChangeTreeNode[] { + return [...node.children.values()].sort((a, b) => compareLocale(a.name, b.name), ); - const sortedFiles = [...node.files].sort((a, b) => { +} + +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); }); +} - for (const child of sortedDirs) { +function flattenNode(node: ChangeTreeNode, out: ChangedFile[]) { + for (const child of orderedTreeDirs(node)) { flattenNode(child, out); } - for (const file of sortedFiles) { + for (const file of orderedTreeFiles(node)) { out.push(file); } } diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index b6863133ce..998ec35844 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -1,6 +1,10 @@ import type { parsePatchFiles } from "@pierre/diffs"; import type { ResolvedDiffSource } from "@posthog/core/code-review/resolveDiffSource"; import type { PrCommentThread } from "@posthog/core/code-review/types"; +import { + orderPathsLikeChangeTree, + sortByChangeTreeOrder, +} from "@posthog/core/git-interaction/changeTree"; import { useHostTRPC } from "@posthog/host-router/react"; import type { ChangedFile, Task } from "@posthog/shared/domain-types"; import { Flex, Text } from "@radix-ui/themes"; @@ -33,9 +37,7 @@ import { buildRemoteReviewItems, buildUntrackedReviewItems, changedFileSignature, - orderPathsLikeTree, patchFileSignature, - sortReviewItemsByTreeOrder, } from "./reviewItemBuilders"; const EMPTY_CHANGED_FILES: ChangedFile[] = []; @@ -348,7 +350,7 @@ function LocalReviewContent({ const stagedItems = useMemo( () => - sortReviewItemsByTreeOrder( + sortByChangeTreeOrder( buildPatchReviewItems({ files: stagedParsedFiles, staged: true, @@ -363,7 +365,7 @@ function LocalReviewContent({ prUrl, commentThreads, }), - orderPathsLikeTree( + orderPathsLikeChangeTree( stagedParsedFiles.map((f) => f.name ?? f.prevName ?? ""), ), ), @@ -384,7 +386,7 @@ function LocalReviewContent({ const changesItems = useMemo( () => - sortReviewItemsByTreeOrder( + sortByChangeTreeOrder( [ ...buildPatchReviewItems({ files: unstagedParsedFiles, @@ -411,7 +413,7 @@ function LocalReviewContent({ onStageFile, }), ], - orderPathsLikeTree([ + orderPathsLikeChangeTree([ ...unstagedParsedFiles.map((f) => f.name ?? f.prevName ?? ""), ...untrackedFiles.map((f) => f.path), ]), @@ -552,7 +554,7 @@ function RemoteReviewPage({ const items = useMemo( () => - sortReviewItemsByTreeOrder( + sortByChangeTreeOrder( buildRemoteReviewItems({ files, taskId, @@ -562,7 +564,7 @@ function RemoteReviewPage({ toggleFile: reviewState.toggleFile, commentThreads, }), - orderPathsLikeTree(files.map((f) => f.path)), + orderPathsLikeChangeTree(files.map((f) => f.path)), ), [ commentThreads, diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts index 6bdd65ffbe..a9a1bf4a56 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts @@ -1,12 +1,6 @@ import type { ChangedFile } from "@posthog/shared/domain-types"; import { describe, expect, it } from "vitest"; -import type { ReviewListItem } from "../commentFileFilter"; -import { - changedFileSignature, - orderPathsLikeTree, - patchFileSignature, - sortReviewItemsByTreeOrder, -} from "./reviewItemBuilders"; +import { changedFileSignature, patchFileSignature } from "./reviewItemBuilders"; const changedFile = (over: Partial): ChangedFile => ({ path: "a.ts", @@ -81,58 +75,3 @@ describe("patchFileSignature", () => { expect(a).not.toBe(b); }); }); - -describe("orderPathsLikeTree", () => { - it("orders paths like the file tree, not git byte order", () => { - expect( - orderPathsLikeTree([ - "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", - ]); - }); -}); - -describe("sortReviewItemsByTreeOrder", () => { - const item = (key: string, filePaths: string[]): ReviewListItem => ({ - key, - filePaths, - node: null, - }); - - it("reorders file items to match the given tree order", () => { - const items = [ - item("a", ["zeta.txt"]), - item("b", ["alpha.txt"]), - item("c", ["src/x.ts"]), - ]; - const ordered = sortReviewItemsByTreeOrder(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 = sortReviewItemsByTreeOrder(items, ["alpha.txt"]); - expect(ordered.map((i) => i.key)).toEqual(["b", "a"]); - }); - - it("returns the same array when there is no tree order", () => { - const items = [item("a", ["zeta.txt"]), item("b", ["alpha.txt"])]; - expect(sortReviewItemsByTreeOrder(items, [])).toBe(items); - }); -}); diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index 971f2e1baa..4a77b6711e 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -5,19 +5,12 @@ import { computeSkipExpansion, } from "@posthog/core/code-review/reviewItemKeys"; import type { PrCommentThread } from "@posthog/core/code-review/types"; -import { - orderPathsLikeChangeTree, - sortByChangeTreeOrder, -} from "@posthog/core/git-interaction/changeTree"; import type { ChangedFile } from "@posthog/shared/domain-types"; import { makeFileKey } from "../../git-interaction/utils/fileKey"; import type { ReviewListItem } from "../commentFileFilter"; import type { DiffOptions } from "../types"; import { PatchRow, RemoteRow, UntrackedRow } from "./ReviewRows"; -export const orderPathsLikeTree = orderPathsLikeChangeTree; -export const sortReviewItemsByTreeOrder = sortByChangeTreeOrder; - export function changedFileSignature(file: ChangedFile): string | null { if (file.patch) return contentHash(file.patch); if (file.sha) return `${file.status}:${file.sha}`; diff --git a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx index 2d9e5906cd..3bf3abe4c8 100644 --- a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx +++ b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx @@ -2,18 +2,15 @@ import { buildChangeTree, type ChangeTreeNode, compactChangeTree, + orderedTreeDirs, + orderedTreeFiles, } from "@posthog/core/git-interaction/changeTree"; import type { ChangedFile } from "@posthog/shared/domain-types"; import { TreeDirectoryRow } from "@posthog/ui/primitives/TreeDirectoryRow"; import { useCallback, useMemo, useState } from "react"; -export type TreeNode = ChangeTreeNode; - -export const buildChangesTree = buildChangeTree; -export const compactTree = compactChangeTree; - interface ChangesTreeNodeProps { - node: TreeNode; + node: ChangeTreeNode; depth: number; collapsedDirs: Set; onToggleDir: (path: string) => void; @@ -28,20 +25,8 @@ function ChangesTreeNode({ renderFile, }: ChangesTreeNodeProps) { const isCollapsed = collapsedDirs.has(node.path); - const sortedDirs = useMemo( - () => - [...node.children.values()].sort((a, b) => a.name.localeCompare(b.name)), - [node.children], - ); - const sortedFiles = useMemo( - () => - [...node.files].sort((a, b) => { - const aName = a.path.split("/").pop() || ""; - const bName = b.path.split("/").pop() || ""; - return aName.localeCompare(bName); - }), - [node.files], - ); + const sortedDirs = useMemo(() => orderedTreeDirs(node), [node]); + const sortedFiles = useMemo(() => orderedTreeFiles(node), [node]); return ( <> @@ -80,7 +65,10 @@ interface ChangesTreeViewProps { } export function ChangesTreeView({ files, renderFile }: ChangesTreeViewProps) { - const tree = useMemo(() => compactTree(buildChangesTree(files)), [files]); + const tree = useMemo( + () => compactChangeTree(buildChangeTree(files)), + [files], + ); const [collapsedDirs, setCollapsedDirs] = useState>(new Set()); const handleToggleDir = useCallback((path: string) => {