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
92 changes: 92 additions & 0 deletions apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { describe, expect, test } from "bun:test";
import { isUpstreamMissingError } from "./git-utils";

describe("git-operations error handling", () => {
describe("isUpstreamMissingError", () => {
const upstreamDeletedMessages = [
"Your configuration specifies to merge with the ref 'refs/heads/feature-branch' from the remote, but no such ref was fetched.",
"fatal: couldn't find remote ref refs/heads/deleted-branch",
"There is no tracking information for the current branch",
];

const otherErrorMessages = [
"fatal: not a git repository",
"error: failed to push some refs",
"CONFLICT (content): Merge conflict in file.ts",
"fatal: refusing to merge unrelated histories",
];

test("detects upstream deleted errors", () => {
for (const message of upstreamDeletedMessages) {
expect(isUpstreamMissingError(message)).toBe(true);
}
});

test("does not falsely detect other errors as upstream deleted", () => {
for (const message of otherErrorMessages) {
expect(isUpstreamMissingError(message)).toBe(false);
}
});
});

describe("error message patterns", () => {
test("commit with no staged changes", () => {
const message = "nothing to commit, working tree clean";
expect(message.includes("nothing to commit")).toBe(true);
});

test("push rejected - needs pull first", () => {
const message =
"error: failed to push some refs to 'origin'\nhint: Updates were rejected because the remote contains work";
expect(message.includes("failed to push")).toBe(true);
expect(message.includes("rejected")).toBe(true);
});

test("push rejected - no permission", () => {
const message = "remote: Permission to user/repo.git denied to otheruser";
expect(message.includes("Permission")).toBe(true);
expect(message.includes("denied")).toBe(true);
});

test("merge conflict during pull", () => {
const message =
"CONFLICT (content): Merge conflict in src/file.ts\nAutomatic merge failed";
expect(message.includes("CONFLICT")).toBe(true);
expect(message.includes("Merge conflict")).toBe(true);
});

test("detached HEAD state", () => {
const message = "HEAD detached at abc1234";
expect(message.includes("detached")).toBe(true);
});

test("no remote configured", () => {
const message = "fatal: 'origin' does not appear to be a git repository";
expect(message.includes("does not appear to be a git repository")).toBe(
true,
);
});
});
Comment on lines +32 to +69

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.

🛠️ Refactor suggestion | 🟠 Major

Remove trivial string-matching tests.

These tests check obvious string containment (e.g., verifying that "nothing to commit, working tree clean" includes "nothing to commit"). They don't test business logic or behavior and would always pass, providing no value.

Consider removing this entire section or replacing with meaningful integration tests

If you want to test error handling for these scenarios, test the actual git operation procedures (commit, push, pull) with mocked git responses, rather than testing string literals.

Example:

test("commit with no staged changes throws appropriate error", async () => {
  // Mock git.commit to throw "nothing to commit" error
  // Call the commit procedure
  // Assert the error message is user-friendly
});

Based on learnings, tests should be meaningful and test actual behavior rather than trivial assertions.

🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts around lines
32-69 the tests are trivial string-containment checks that provide no value;
remove this block or replace each trivial case with a meaningful
unit/integration test that exercises the git-handling logic: mock the git
client/commands to return the specific error strings (or throw errors), call the
actual commit/push/pull wrapper functions, and assert the procedure converts
those git errors into the expected user-facing errors or control flow (e.g.,
throws a mapped Error, returns a specific result, or triggers the correct
retry/prompt behavior); ensure tests cover both success and error paths and
avoid bare string includes.

});

describe("sync operation logic", () => {
test("should push with set-upstream when pull fails due to deleted upstream", () => {
// This tests the logic flow:
// 1. Pull fails with "no such ref was fetched"
// 2. Should fall back to push with --set-upstream

const pullError = new Error(
"Your configuration specifies to merge with the ref 'refs/heads/feature' from the remote, but no such ref was fetched.",
);

expect(isUpstreamMissingError(pullError.message)).toBe(true);
});

test("should re-throw other pull errors", () => {
const pullError = new Error(
"CONFLICT (content): Merge conflict in file.ts",
);

expect(isUpstreamMissingError(pullError.message)).toBe(false);
});
});
169 changes: 169 additions & 0 deletions apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
import { writeFile } from "node:fs/promises";
import { resolve } from "node:path";
import { shell } from "electron";
import simpleGit from "simple-git";
import { z } from "zod";
import { publicProcedure, router } from "../..";
import { isUpstreamMissingError } from "./git-utils";

export { isUpstreamMissingError };

async function hasUpstreamBranch(
git: ReturnType<typeof simpleGit>,
): Promise<boolean> {
try {
await git.raw(["rev-parse", "--abbrev-ref", "@{upstream}"]);
return true;
} catch {
return false;
}
}

export const createGitOperationsRouter = () => {
return router({
saveFile: publicProcedure
.input(
z.object({
worktreePath: z.string(),
filePath: z.string(),
content: z.string(),
}),
)
.mutation(async ({ input }): Promise<{ success: boolean }> => {
const resolvedWorktree = resolve(input.worktreePath);
const fullPath = resolve(resolvedWorktree, input.filePath);

if (!fullPath.startsWith(`${resolvedWorktree}/`)) {
throw new Error("Invalid file path: path traversal detected");
}

await writeFile(fullPath, input.content, "utf-8");
return { success: true };
}),
Comment thread
Kitenite marked this conversation as resolved.
Resolved

commit: publicProcedure
.input(
z.object({
worktreePath: z.string(),
message: z.string(),
}),
)
.mutation(
async ({ input }): Promise<{ success: boolean; hash: string }> => {
const git = simpleGit(input.worktreePath);
const result = await git.commit(input.message);
return { success: true, hash: result.commit };
},
),

push: publicProcedure
.input(
z.object({
worktreePath: z.string(),
setUpstream: z.boolean().optional(),
}),
)
.mutation(async ({ input }): Promise<{ success: boolean }> => {
const git = simpleGit(input.worktreePath);
const hasUpstream = await hasUpstreamBranch(git);

if (input.setUpstream && !hasUpstream) {
const branch = await git.revparse(["--abbrev-ref", "HEAD"]);
await git.push(["--set-upstream", "origin", branch.trim()]);
} else {
await git.push();
}
await git.fetch();
return { success: true };
}),

pull: publicProcedure
.input(
z.object({
worktreePath: z.string(),
}),
)
.mutation(async ({ input }): Promise<{ success: boolean }> => {
const git = simpleGit(input.worktreePath);
try {
await git.pull(["--rebase"]);
} catch (error) {
const message =
error instanceof Error ? error.message : String(error);
if (isUpstreamMissingError(message)) {
throw new Error(
"No upstream branch to pull from. The remote branch may have been deleted.",
);
}
throw error;
}
return { success: true };
}),

sync: publicProcedure
.input(
z.object({
worktreePath: z.string(),
}),
)
.mutation(async ({ input }): Promise<{ success: boolean }> => {
const git = simpleGit(input.worktreePath);
try {
await git.pull(["--rebase"]);
} catch (error) {
const message =
error instanceof Error ? error.message : String(error);
if (isUpstreamMissingError(message)) {
const branch = await git.revparse(["--abbrev-ref", "HEAD"]);
await git.push(["--set-upstream", "origin", branch.trim()]);
await git.fetch();
return { success: true };
}
throw error;
}
await git.push();
await git.fetch();
return { success: true };
}),

createPR: publicProcedure
.input(
z.object({
worktreePath: z.string(),
}),
)
.mutation(
async ({ input }): Promise<{ success: boolean; url: string }> => {
const git = simpleGit(input.worktreePath);
const branch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim();
const hasUpstream = await hasUpstreamBranch(git);

// Ensure branch is pushed first
if (!hasUpstream) {
await git.push(["--set-upstream", "origin", branch]);
} else {
// Push any unpushed commits
await git.push();
}

// Get the remote URL to construct the GitHub compare URL
const remoteUrl = (await git.remote(["get-url", "origin"])) || "";
const repoMatch = remoteUrl
.trim()
.match(/github\.com[:/](.+?)(?:\.git)?$/);

if (!repoMatch) {
throw new Error("Could not determine GitHub repository URL");
}

const repo = repoMatch[1].replace(/\.git$/, "");
const url = `https://github.com/${repo}/compare/${branch}?expand=1`;

await shell.openExternal(url);
await git.fetch();

return { success: true, url };
},
),
});
};
10 changes: 10 additions & 0 deletions apps/desktop/src/lib/trpc/routers/changes/git-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Check if the error message indicates the upstream branch is missing/deleted
*/
export function isUpstreamMissingError(message: string): boolean {
return (
message.includes("no such ref was fetched") ||
message.includes("no tracking information") ||
message.includes("couldn't find remote ref")
);
}
5 changes: 5 additions & 0 deletions apps/desktop/src/lib/trpc/routers/changes/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { router } from "../..";
import { createBranchesRouter } from "./branches";
import { createFileContentsRouter } from "./file-contents";
import { createGitOperationsRouter } from "./git-operations";
import { createStagingRouter } from "./staging";
import { createStatusRouter } from "./status";

Expand All @@ -9,6 +10,7 @@ export const createChangesRouter = () => {
const statusRouter = createStatusRouter();
const fileContentsRouter = createFileContentsRouter();
const stagingRouter = createStagingRouter();
const gitOperationsRouter = createGitOperationsRouter();

return router({
// Branch operations
Expand All @@ -22,5 +24,8 @@ export const createChangesRouter = () => {

// Staging operations
...stagingRouter._def.procedures,

// Git operations (commit, push, pull, sync, createPR)
...gitOperationsRouter._def.procedures,
});
};
40 changes: 40 additions & 0 deletions apps/desktop/src/lib/trpc/routers/changes/status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const createStatusRouter = () => {
const parsed = parseGitStatus(status);

const branchComparison = await getBranchComparison(git, defaultBranch);
const trackingStatus = await getTrackingBranchStatus(git);

await applyNumstatToFiles(git, parsed.staged, [
"diff",
Expand All @@ -49,6 +50,9 @@ export const createStatusRouter = () => {
untracked: parsed.untracked,
ahead: branchComparison.ahead,
behind: branchComparison.behind,
pushCount: trackingStatus.pushCount,
pullCount: trackingStatus.pullCount,
hasUpstream: trackingStatus.hasUpstream,
};
}),

Expand Down Expand Up @@ -151,3 +155,39 @@ async function applyUntrackedLineCount(
} catch {}
}
}

interface TrackingStatus {
pushCount: number;
pullCount: number;
hasUpstream: boolean;
}

async function getTrackingBranchStatus(
git: ReturnType<typeof simpleGit>,
): Promise<TrackingStatus> {
try {
const upstream = await git.raw([
"rev-parse",
"--abbrev-ref",
"@{upstream}",
]);
if (!upstream.trim()) {
return { pushCount: 0, pullCount: 0, hasUpstream: false };
}

const tracking = await git.raw([
"rev-list",
"--left-right",
"--count",
"@{upstream}...HEAD",
]);
const [pullStr, pushStr] = tracking.trim().split(/\s+/);
return {
pushCount: Number.parseInt(pushStr || "0", 10),
pullCount: Number.parseInt(pullStr || "0", 10),
hasUpstream: true,
};
} catch {
return { pushCount: 0, pullCount: 0, hasUpstream: false };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ import type { ChangedFile } from "shared/changes-types";
import type { SimpleGit } from "simple-git";
import { parseDiffNumstat } from "./parse-status";

/**
* Fetches numstat for given diff arguments and applies stats to files
*/
export async function applyNumstatToFiles(
git: SimpleGit,
files: ChangedFile[],
Expand Down
Loading