From 89c5712472e180f99c4a05456466f5da2e519bb2 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 16 Jul 2026 16:41:37 -0400 Subject: [PATCH] feat: group review files by type Generated-By: PostHog Code Task-Id: 633af2d8-9f13-4a43-ac8e-673fc1d367d8 --- .../task-detail/components/ChangesPanel.tsx | 114 +++++++-- .../components/ChangesTreeView.test.ts | 60 +++++ .../components/ChangesTreeView.tsx | 98 ++++---- .../task-detail/components/changesTree.ts | 216 ++++++++++++++++++ .../ui/src/primitives/TreeDirectoryRow.tsx | 4 +- 5 files changed, 424 insertions(+), 68 deletions(-) create mode 100644 packages/ui/src/features/task-detail/components/ChangesTreeView.test.ts create mode 100644 packages/ui/src/features/task-detail/components/changesTree.ts diff --git a/packages/ui/src/features/task-detail/components/ChangesPanel.tsx b/packages/ui/src/features/task-detail/components/ChangesPanel.tsx index cfc14c578b..9832662485 100644 --- a/packages/ui/src/features/task-detail/components/ChangesPanel.tsx +++ b/packages/ui/src/features/task-detail/components/ChangesPanel.tsx @@ -5,6 +5,8 @@ import { FilePlus, MinusIcon, PlusIcon, + StackIcon, + TreeStructure, } from "@phosphor-icons/react"; import { getFileExtension } from "@posthog/shared"; import { @@ -19,6 +21,7 @@ import { DropdownMenu, Flex, IconButton, + SegmentedControl, Spinner, Text, } from "@radix-ui/themes"; @@ -47,12 +50,17 @@ import { useCloudChangedFiles } from "../hooks/useCloudChangedFiles"; import { useDiscardFile } from "../hooks/useDiscardFile"; import { useStageToggle } from "../hooks/useStageToggle"; import { ChangesTreeView } from "./ChangesTreeView"; +import type { ChangesGrouping } from "./changesTree"; interface ChangesPanelProps { taskId: string; task: Task; } +interface GroupedChangesPanelProps extends ChangesPanelProps { + grouping: ChangesGrouping; +} + interface ChangedFileItemProps { file: ChangedFile; taskId: string; @@ -63,6 +71,7 @@ interface ChangedFileItemProps { onStageToggle?: (file: ChangedFile) => void; onDiscard?: (file: ChangedFile, fileName: string) => void; depth?: number; + showFullPath?: boolean; } function CompactIconButton({ @@ -99,6 +108,7 @@ function ChangedFileItem({ onStageToggle, onDiscard, depth = 0, + showFullPath = false, }: ChangedFileItemProps) { const requestScrollToFile = useReviewNavigationStore( (state) => state.requestScrollToFile, @@ -283,6 +293,7 @@ function ChangedFileItem({ ( + (file: ChangedFile, depth: number, showFullPath: boolean) => ( ), [taskId, activeFilePath], @@ -379,7 +395,11 @@ function CloudChangesPanel({ taskId, task }: ChangesPanelProps) { return ( - + {isRunActive && ( @@ -395,15 +415,55 @@ function CloudChangesPanel({ taskId, task }: ChangesPanelProps) { export function ChangesPanel({ taskId, task }: ChangesPanelProps) { const isCloud = useIsCloudTask(taskId); + const isExpanded = useReviewNavigationStore( + (state) => state.reviewModes[taskId] === "expanded", + ); + const [grouping, setGrouping] = useState("directory"); - if (isCloud) { - return ; + const content = isCloud ? ( + + ) : ( + + ); + + if (!isExpanded) { + return content; } - return ; + return ( + + + setGrouping(value as ChangesGrouping)} + aria-label="Changed files grouping" + className="w-full" + > + + + + Folders + + + + + + File type + + + + + {content} + + ); } -function LocalChangesPanel({ taskId, task }: ChangesPanelProps) { +function LocalChangesPanel({ + taskId, + task, + grouping, +}: GroupedChangesPanelProps) { const { effectiveSource, prUrl, linkedBranch } = useEffectiveDiffSource(taskId); const repoPath = useCwd(taskId); @@ -414,21 +474,29 @@ function LocalChangesPanel({ taskId, task }: ChangesPanelProps) { taskId={taskId} repoPath={repoPath} branch={linkedBranch} + grouping={grouping} /> ); } if (effectiveSource === "pr") { - return ; + return ; } - return ; + return ( + + ); } function LocalWorkingTreeChangesPanel({ taskId, task: _task, -}: ChangesPanelProps) { + grouping, +}: GroupedChangesPanelProps) { const workspace = useWorkspace(taskId); const repoPath = useCwd(taskId); const activeFilePath = useReviewNavigationStore( @@ -446,7 +514,7 @@ function LocalWorkingTreeChangesPanel({ const hasStagedFiles = stagedFiles.length > 0; const renderLocalFile = useCallback( - (file: ChangedFile, depth: number) => { + (file: ChangedFile, depth: number, showFullPath: boolean) => { const key = makeFileKey(file.staged, file.path); return ( ); }, @@ -512,7 +581,11 @@ function LocalWorkingTreeChangesPanel({ )} - + ))} @@ -526,6 +599,7 @@ interface RemoteChangesListProps { isLoading: boolean; emptyMessage: string; panelId: string; + grouping: ChangesGrouping; } function RemoteChangesList({ @@ -534,13 +608,14 @@ function RemoteChangesList({ isLoading, emptyMessage, panelId, + grouping, }: RemoteChangesListProps) { const activeFilePath = useReviewNavigationStore( (s) => s.activeFilePaths[taskId] ?? null, ); const renderFile = useCallback( - (file: ChangedFile, depth: number) => ( + (file: ChangedFile, depth: number, showFullPath: boolean) => ( ), [taskId, activeFilePath], @@ -564,7 +640,11 @@ function RemoteChangesList({ return ( - + ); @@ -574,10 +654,12 @@ function BranchChangesPanel({ taskId, repoPath, branch, + grouping, }: { taskId: string; repoPath: string | undefined; branch: string | null; + grouping: ChangesGrouping; }) { const { data: files = [], isLoading } = useLocalBranchChangedFiles( repoPath ?? null, @@ -595,6 +677,7 @@ function BranchChangesPanel({ isLoading={isLoading} emptyMessage="No file changes in branch" panelId="changes-panel-branch" + grouping={grouping} /> ); } @@ -602,9 +685,11 @@ function BranchChangesPanel({ function PrChangesPanel({ taskId, prUrl, + grouping, }: { taskId: string; prUrl: string | null; + grouping: ChangesGrouping; }) { const { data: files = [], isLoading } = usePrChangedFiles(prUrl); @@ -619,6 +704,7 @@ function PrChangesPanel({ isLoading={isLoading} emptyMessage="No file changes in pull request" panelId="changes-panel-pr" + grouping={grouping} /> ); } diff --git a/packages/ui/src/features/task-detail/components/ChangesTreeView.test.ts b/packages/ui/src/features/task-detail/components/ChangesTreeView.test.ts new file mode 100644 index 0000000000..d2a23f5d38 --- /dev/null +++ b/packages/ui/src/features/task-detail/components/ChangesTreeView.test.ts @@ -0,0 +1,60 @@ +import type { ChangedFile } from "@posthog/shared/domain-types"; +import { describe, expect, it } from "vitest"; +import { + classifyChangedFile, + type FileTypeCategory, + groupChangesByFileType, +} from "./changesTree"; + +const changedFile = (path: string): ChangedFile => ({ + path, + status: "modified", +}); + +describe("classifyChangedFile", () => { + it.each<[string, FileTypeCategory]>([ + ["packages/core/src/service.ts", "Implementation"], + ["packages/core/src/service.test.ts", "Tests"], + ["tests/e2e/review.spec.ts", "Tests"], + ["packages/core/service_test.go", "Tests"], + ["packages/core/test_service.py", "Tests"], + ["packages/api/src/__generated__/schema.ts", "Generated"], + ["packages/api/src/schema.generated.ts", "Generated"], + ["packages/api/src/messages.pb.go", "Generated"], + ["pnpm-lock.yaml", "Generated"], + ["docs/code-review.md", "Documentation"], + ["README.md", "Documentation"], + [".github/workflows/ci.yml", "Configuration"], + ["package.json", "Configuration"], + ["requirements.txt", "Configuration"], + ["packages/ui/src/assets/review.png", "Assets"], + ["Makefile", "Other"], + ])("classifies %s as %s", (path, expected) => { + expect(classifyChangedFile(path)).toBe(expected); + }); +}); + +describe("groupChangesByFileType", () => { + it("orders categories and files consistently", () => { + const groups = groupChangesByFileType([ + changedFile("docs/z-last.md"), + changedFile("src/z-last.ts"), + changedFile("src/a-first.ts"), + changedFile("src/service.test.ts"), + changedFile("src/schema.generated.ts"), + changedFile("package.json"), + ]); + + expect(groups.map((group) => group.category)).toEqual([ + "Implementation", + "Tests", + "Generated", + "Documentation", + "Configuration", + ]); + expect(groups[0]?.files.map((file) => file.path)).toEqual([ + "src/a-first.ts", + "src/z-last.ts", + ]); + }); +}); diff --git a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx index e21aad27cd..1e7d7c06ce 100644 --- a/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx +++ b/packages/ui/src/features/task-detail/components/ChangesTreeView.tsx @@ -1,62 +1,24 @@ 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; -} - -/** 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 }; -} +import { + buildChangesTree, + type ChangesGrouping, + compactTree, + groupChangesByFileType, + type TreeNode, +} from "./changesTree"; interface ChangesTreeNodeProps { node: TreeNode; depth: number; collapsedDirs: Set; onToggleDir: (path: string) => void; - renderFile: (file: ChangedFile, depth: number) => React.ReactNode; + renderFile: ( + file: ChangedFile, + depth: number, + showFullPath: boolean, + ) => React.ReactNode; } function ChangesTreeNode({ @@ -105,7 +67,7 @@ function ChangesTreeNode({ /> ))} {sortedFiles.map((file) => - renderFile(file, node.path ? depth + 1 : depth), + renderFile(file, node.path ? depth + 1 : depth, false), )} )} @@ -115,11 +77,21 @@ function ChangesTreeNode({ interface ChangesTreeViewProps { files: ChangedFile[]; - renderFile: (file: ChangedFile, depth: number) => React.ReactNode; + grouping: ChangesGrouping; + renderFile: ( + file: ChangedFile, + depth: number, + showFullPath: boolean, + ) => React.ReactNode; } -export function ChangesTreeView({ files, renderFile }: ChangesTreeViewProps) { +export function ChangesTreeView({ + files, + grouping, + renderFile, +}: ChangesTreeViewProps) { const tree = useMemo(() => compactTree(buildChangesTree(files)), [files]); + const fileTypeGroups = useMemo(() => groupChangesByFileType(files), [files]); const [collapsedDirs, setCollapsedDirs] = useState>(new Set()); const handleToggleDir = useCallback((path: string) => { @@ -134,6 +106,26 @@ export function ChangesTreeView({ files, renderFile }: ChangesTreeViewProps) { }); }, []); + if (grouping === "file-type") { + return fileTypeGroups.map(({ category, files: categoryFiles }) => { + const categoryPath = `file-type:${category}`; + const isCollapsed = collapsedDirs.has(categoryPath); + + return ( +
+ handleToggleDir(categoryPath)} + /> + {!isCollapsed && + categoryFiles.map((file) => renderFile(file, 1, true))} +
+ ); + }); + } + return ( + ["__tests__", "e2e", "spec", "specs", "test", "tests"].includes(segment), + ) || + /(?:^|\.)((?:e2e|integration|spec|test))\.[^.]+$/.test(fileName) || + /(?:^test_.+|.+_test)\.[^.]+$/.test(fileName) || + fileName.endsWith(".snap") + ) { + return "Tests"; + } + + if ( + GENERATED_FILE_NAMES.has(fileName) || + segments.some((segment) => + ["__generated__", "dist", "generated"].includes(segment), + ) || + /(?:^|[._-])generated(?:[._-]|$)/.test(fileName) || + /(?:\.g|\.pb|\.designer)\.[^.]+$/.test(fileName) || + /_pb2(?:_grpc)?\.py$/.test(fileName) + ) { + return "Generated"; + } + + if (CONFIG_FILE_NAMES.has(fileName)) { + return "Configuration"; + } + + if ( + segments.some((segment) => + ["doc", "docs", "documentation"].includes(segment), + ) || + DOCUMENTATION_EXTENSIONS.has(extension ?? "") || + /^(changelog|contributing|license|readme)(\.|$)/.test(fileName) + ) { + return "Documentation"; + } + + if ( + segments.some((segment) => + [".github", ".husky", ".vscode", "config", "configs"].includes(segment), + ) || + CONFIG_EXTENSIONS.has(extension ?? "") || + fileName.startsWith(".") || + /(?:^|\.)config\.[^.]+$/.test(fileName) + ) { + return "Configuration"; + } + + if ( + segments.some((segment) => + [ + "asset", + "assets", + "font", + "fonts", + "image", + "images", + "static", + ].includes(segment), + ) || + ASSET_EXTENSIONS.has(extension ?? "") + ) { + return "Assets"; + } + + if (extension) { + return "Implementation"; + } + + return "Other"; +} + +export function groupChangesByFileType( + files: ChangedFile[], +): { category: FileTypeCategory; files: ChangedFile[] }[] { + const groups = new Map(); + + for (const file of files) { + const category = classifyChangedFile(file.path); + const categoryFiles = groups.get(category) ?? []; + categoryFiles.push(file); + groups.set(category, categoryFiles); + } + + return FILE_TYPE_CATEGORY_ORDER.flatMap((category) => { + const categoryFiles = groups.get(category); + if (!categoryFiles) return []; + return [ + { + category, + files: categoryFiles.sort((a, b) => a.path.localeCompare(b.path)), + }, + ]; + }); +} + +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 index = 0; index < parts.length - 1; index++) { + const part = parts[index]; + if (!node.children.has(part)) { + node.children.set(part, { + name: part, + path: parts.slice(0, index + 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 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 }; +} diff --git a/packages/ui/src/primitives/TreeDirectoryRow.tsx b/packages/ui/src/primitives/TreeDirectoryRow.tsx index a5cc5d6756..cf5c8552ea 100644 --- a/packages/ui/src/primitives/TreeDirectoryRow.tsx +++ b/packages/ui/src/primitives/TreeDirectoryRow.tsx @@ -69,6 +69,7 @@ export function TreeDirectoryRow({ interface TreeFileRowProps { fileName: string; + displayName?: string; depth: number; isActive?: boolean; title?: string; @@ -83,6 +84,7 @@ interface TreeFileRowProps { export function TreeFileRow({ fileName, + displayName = fileName, depth, isActive = false, title, @@ -118,7 +120,7 @@ export function TreeFileRow({ /> - {fileName} + {displayName} {trailing}