Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apps/code/src/main/services/git/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,12 @@ export const getBranchChangedFilesInput = z.object({
});
export const getBranchChangedFilesOutput = z.array(changedFileSchema);

export const getLocalBranchChangedFilesInput = z.object({
directoryPath: z.string(),
branch: z.string(),
});
export const getLocalBranchChangedFilesOutput = z.array(changedFileSchema);

export const generateCommitMessageInput = z.object({
directoryPath: z.string(),
conversationContext: z.string().optional(),
Expand Down
35 changes: 35 additions & 0 deletions apps/code/src/main/services/git/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const execFileAsync = promisify(execFile);
import { execGh } from "@posthog/git/gh";
import {
getAllBranches,
getBranchDiffPatchesByPath,
getChangedFilesBetweenBranches,
getChangedFilesDetailed,
getCommitConventions,
getCommitsBetweenBranches,
Expand Down Expand Up @@ -1237,6 +1239,39 @@ export class GitService extends TypedEventEmitter<GitServiceEvents> {
});
}

public async getLocalBranchChangedFiles(
directoryPath: string,
branch: string,
): Promise<ChangedFile[]> {
await this.fetchIfStale(directoryPath);

const defaultBranch = await getDefaultBranch(directoryPath);
if (!defaultBranch) return [];

const files = await getChangedFilesBetweenBranches(
directoryPath,
defaultBranch,
branch,
{ excludePatterns: [".claude", "CLAUDE.local.md"] },
);
if (files.length === 0) return [];

const patchByPath = await getBranchDiffPatchesByPath(
directoryPath,
defaultBranch,
branch,
);

return files.map((f) => ({
path: f.path,
status: f.status,
originalPath: f.originalPath,
linesAdded: f.linesAdded,
linesRemoved: f.linesRemoved,
patch: patchByPath.get(f.path),
}));
}

public async generateCommitMessage(
directoryPath: string,
conversationContext?: string,
Expand Down
12 changes: 12 additions & 0 deletions apps/code/src/main/trpc/routers/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ import {
getGitSyncStatusOutput,
getLatestCommitInput,
getLatestCommitOutput,
getLocalBranchChangedFilesInput,
getLocalBranchChangedFilesOutput,
getPrChangedFilesInput,
getPrChangedFilesOutput,
getPrDetailsByUrlInput,
Expand Down Expand Up @@ -367,6 +369,16 @@ export const gitRouter = router({
getService().getBranchChangedFiles(input.repo, input.branch),
),

getLocalBranchChangedFiles: publicProcedure
.input(getLocalBranchChangedFilesInput)
.output(getLocalBranchChangedFilesOutput)
.query(({ input }) =>
getService().getLocalBranchChangedFiles(
input.directoryPath,
input.branch,
),
),

generateCommitMessage: publicProcedure
.input(generateCommitMessageInput)
.output(generateCommitMessageOutput)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { Tooltip } from "@components/ui/Tooltip";
import {
useBranchChangedFiles,
useLocalBranchChangedFiles,
usePrChangedFiles,
} from "@features/git-interaction/hooks/useGitQueries";
import {
computeDiffStats,
type DiffStats,
} from "@features/git-interaction/utils/diffStats";
import { useCwd } from "@features/sidebar/hooks/useCwd";
import { useCloudChangedFiles } from "@features/task-detail/hooks/useCloudChangedFiles";
import { useWorkspace } from "@features/workspace/hooks/useWorkspace";
import { GitDiff } from "@phosphor-icons/react";
Expand Down Expand Up @@ -44,16 +45,16 @@ function CloudDiffStatsBadge({ task }: { task: Task }) {

function LocalDiffStatsBadge({ task }: { task: Task }) {
const taskId = task.id;
const repoPath = useCwd(taskId);
const {
effectiveSource,
repoSlug,
linkedBranch,
prUrl,
diffStats: localDiffStats,
} = useEffectiveDiffSource(taskId);

const { data: branchFiles } = useBranchChangedFiles(
effectiveSource === "branch" ? repoSlug : null,
const { data: branchFiles } = useLocalBranchChangedFiles(
effectiveSource === "branch" ? (repoPath ?? null) : null,
effectiveSource === "branch" ? linkedBranch : null,
);
const { data: prFiles } = usePrChangedFiles(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useDiffViewerStore } from "@features/code-editor/stores/diffViewerStore";
import {
useBranchChangedFiles,
useLocalBranchChangedFiles,
usePrChangedFiles,
} from "@features/git-interaction/hooks/useGitQueries";
import { usePrDetails } from "@features/git-interaction/hooks/usePrDetails";
Expand Down Expand Up @@ -52,7 +52,6 @@ export function ReviewPage({ task }: ReviewPageProps) {
prUrl,
linkedBranch,
defaultBranch,
repoSlug,
branchSourceAvailable,
prSourceAvailable,
} = useEffectiveDiffSource(taskId);
Expand Down Expand Up @@ -113,7 +112,7 @@ export function ReviewPage({ task }: ReviewPageProps) {
<BranchReviewPage
task={task}
branch={linkedBranch as string}
repoSlug={repoSlug}
repoPath={repoPath}
defaultBranch={defaultBranch}
isReviewOpen={isReviewOpen}
effectiveSource={effectiveSource}
Expand Down Expand Up @@ -211,7 +210,7 @@ export function ReviewPage({ task }: ReviewPageProps) {
function BranchReviewPage({
task,
branch,
repoSlug,
repoPath,
defaultBranch,
isReviewOpen,
effectiveSource,
Expand All @@ -222,7 +221,7 @@ function BranchReviewPage({
}: {
task: Task;
branch: string;
repoSlug: string | null;
repoPath: string | null;
defaultBranch: string | null;
isReviewOpen: boolean;
effectiveSource: ResolvedDiffSource;
Expand All @@ -233,10 +232,11 @@ function BranchReviewPage({
}) {
const taskId = task.id;

const { data: files = EMPTY_BRANCH_FILES, isLoading } = useBranchChangedFiles(
isReviewOpen ? repoSlug : null,
isReviewOpen ? branch : null,
);
const { data: files = EMPTY_BRANCH_FILES, isLoading } =
useLocalBranchChangedFiles(
isReviewOpen ? repoPath : null,
isReviewOpen ? branch : null,
);

const allPaths = useMemo(() => files.map((f) => f.path), [files]);

Expand All @@ -249,7 +249,7 @@ function BranchReviewPage({
linesAdded={reviewState.linesAdded}
linesRemoved={reviewState.linesRemoved}
isLoading={
(isLoading || (!repoSlug && isReviewOpen)) && files.length === 0
(isLoading || (!repoPath && isReviewOpen)) && files.length === 0
}
isEmpty={files.length === 0}
allExpanded={reviewState.collapsedFiles.size === 0}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,24 @@ export function useBranchChangedFiles(
),
);
}

export function useLocalBranchChangedFiles(
directoryPath: string | null,
branch: string | null,
) {
const trpc = useTRPC();
return useQuery(
trpc.git.getLocalBranchChangedFiles.queryOptions(
{
directoryPath: directoryPath as string,
branch: branch as string,
},
{
enabled: !!directoryPath && !!branch,
staleTime: 30_000,
refetchOnMount: "always",
retry: 1,
},
),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ export function invalidateGitBranchQueries(repoPath: string) {
queryClient.invalidateQueries(trpc.git.getLatestCommit.queryFilter(input));
queryClient.invalidateQueries(trpc.git.getPrStatus.queryFilter(input));
queryClient.invalidateQueries(trpc.git.getFileAtHead.pathFilter());
queryClient.invalidateQueries(
trpc.git.getLocalBranchChangedFiles.pathFilter(),
);
}
152 changes: 150 additions & 2 deletions packages/git/src/queries.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { mkdtemp, rm, writeFile } from "node:fs/promises";
import { mkdtemp, rm, unlink, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import path from "node:path";
import { afterEach, describe, expect, it } from "vitest";
import { createGitClient } from "./client";
import { detectDefaultBranch } from "./queries";
import {
detectDefaultBranch,
getBranchDiffPatchesByPath,
splitUnifiedDiffByFile,
} from "./queries";

async function setupRepo(defaultBranch = "main"): Promise<string> {
const dir = await mkdtemp(path.join(tmpdir(), "posthog-code-queries-"));
Expand Down Expand Up @@ -94,3 +98,147 @@ describe("detectDefaultBranch", () => {
await rm(remoteDir, { recursive: true, force: true });
});
});

describe("splitUnifiedDiffByFile", () => {
it("returns an empty map for empty input", () => {
expect(splitUnifiedDiffByFile("")).toEqual(new Map());
});

it("splits a two-file diff keyed by post-image path", () => {
const raw = [
"diff --git a/one.txt b/one.txt",
"index 0000000..1111111 100644",
"--- a/one.txt",
"+++ b/one.txt",
"@@ -1 +1 @@",
"-hello",
"+hello world",
"diff --git a/two.txt b/two.txt",
"new file mode 100644",
"--- /dev/null",
"+++ b/two.txt",
"@@ -0,0 +1 @@",
"+brand new",
"",
].join("\n");

const result = splitUnifiedDiffByFile(raw);

expect([...result.keys()]).toEqual(["one.txt", "two.txt"]);
expect(result.get("one.txt")).toContain("diff --git a/one.txt b/one.txt");
expect(result.get("one.txt")).toContain("+hello world");
expect(result.get("two.txt")).toContain("diff --git a/two.txt b/two.txt");
expect(result.get("two.txt")).toContain("+brand new");
});

it("keys renames by the post-rename (b/) path", () => {
const raw = [
"diff --git a/old.txt b/new.txt",
"similarity index 100%",
"rename from old.txt",
"rename to new.txt",
"",
].join("\n");

const result = splitUnifiedDiffByFile(raw);
expect(result.has("new.txt")).toBe(true);
expect(result.has("old.txt")).toBe(false);
Comment on lines +101 to +145
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer parameterised tests for splitUnifiedDiffByFile

The four cases for splitUnifiedDiffByFile all follow the same pattern — supply a raw diff string, check keys and values in the returned map — and could be collapsed into a single it.each table per the team's preference for parameterised tests. The test for the two-file case is the outlier (multiple assertions), but even that can live as a row with a callback to an inline assertion helper.

it.each([
  ["empty input", "", []],
  ["binary diff", "diff --git a/image.png b/image.png\nBinary files ...", ["image.png"]],
  ["rename keyed by new path", "diff --git a/old.txt b/new.txt\nsimilarity index 100%\n...", ["new.txt"]],
] as const)("splitUnifiedDiffByFile: %s", (_label, raw, expectedKeys) => {
  const result = splitUnifiedDiffByFile(raw);
  expect([...result.keys()]).toEqual(expectedKeys);
});
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/git/src/queries.test.ts
Line: 101-145

Comment:
**Prefer parameterised tests for `splitUnifiedDiffByFile`**

The four cases for `splitUnifiedDiffByFile` all follow the same pattern — supply a raw diff string, check keys and values in the returned map — and could be collapsed into a single `it.each` table per the team's preference for parameterised tests. The test for the two-file case is the outlier (multiple assertions), but even that can live as a row with a callback to an inline assertion helper.

```ts
it.each([
  ["empty input", "", []],
  ["binary diff", "diff --git a/image.png b/image.png\nBinary files ...", ["image.png"]],
  ["rename keyed by new path", "diff --git a/old.txt b/new.txt\nsimilarity index 100%\n...", ["new.txt"]],
] as const)("splitUnifiedDiffByFile: %s", (_label, raw, expectedKeys) => {
  const result = splitUnifiedDiffByFile(raw);
  expect([...result.keys()]).toEqual(expectedKeys);
});
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

expect(result.get("new.txt")).toContain("rename from old.txt");
});

it("handles binary diffs", () => {
const raw = [
"diff --git a/image.png b/image.png",
"Binary files a/image.png and b/image.png differ",
"",
].join("\n");

const result = splitUnifiedDiffByFile(raw);
expect(result.get("image.png")).toContain("Binary files");
});
});

describe("getBranchDiffPatchesByPath", () => {
let repoDir: string | undefined;

afterEach(async () => {
if (repoDir) {
await rm(repoDir, { recursive: true, force: true });
repoDir = undefined;
}
});

async function setupBranchWithCommits(): Promise<{
repoDir: string;
remoteDir: string;
}> {
const workDir = await mkdtemp(path.join(tmpdir(), "posthog-code-branch-"));
const remoteDir = await mkdtemp(path.join(tmpdir(), "posthog-code-bare-"));

const remoteGit = createGitClient(remoteDir);
await remoteGit.init(["--bare", "--initial-branch", "main"]);

const git = createGitClient(workDir);
await git.init(["--initial-branch", "main"]);
await git.addConfig("user.name", "Test");
await git.addConfig("user.email", "test@example.com");
await git.addConfig("commit.gpgsign", "false");
await git.addRemote("origin", remoteDir);

await writeFile(path.join(workDir, "file.txt"), "line1\nline2\n");
await git.add(["file.txt"]);
await git.commit("initial");
await git.push(["origin", "main"]);

await git.checkoutLocalBranch("feature");
await writeFile(path.join(workDir, "file.txt"), "line1\nchanged\n");
await writeFile(path.join(workDir, "added.txt"), "new file\n");
await git.add(["file.txt", "added.txt"]);
await git.commit("feature work, not pushed");

return { repoDir: workDir, remoteDir };
}

it("returns per-file patches for commits not yet pushed", async () => {
const { repoDir: workDir, remoteDir } = await setupBranchWithCommits();
repoDir = workDir;

try {
const patches = await getBranchDiffPatchesByPath(
workDir,
"main",
"feature",
);

expect(patches.has("file.txt")).toBe(true);
expect(patches.has("added.txt")).toBe(true);
expect(patches.get("file.txt")).toContain("-line2");
expect(patches.get("file.txt")).toContain("+changed");
expect(patches.get("added.txt")).toContain("+new file");
} finally {
await rm(remoteDir, { recursive: true, force: true });
}
});

it("returns deletions keyed by their path", async () => {
const { repoDir: workDir, remoteDir } = await setupBranchWithCommits();
repoDir = workDir;

try {
const git = createGitClient(workDir);
await unlink(path.join(workDir, "file.txt"));
await git.add(["file.txt"]);
await git.commit("delete file.txt");

const patches = await getBranchDiffPatchesByPath(
workDir,
"main",
"feature",
);

expect(patches.get("file.txt")).toContain("deleted file mode");
} finally {
await rm(remoteDir, { recursive: true, force: true });
}
});
});
Loading
Loading