Skip to content
Closed
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
18 changes: 18 additions & 0 deletions apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { track } from "main/lib/analytics";
import { localDb } from "main/lib/local-db";
import { workspaceInitManager } from "main/lib/workspace-init-manager";
import { SUPERSET_DIR_NAME, WORKTREES_DIR_NAME } from "shared/constants";
import { findBranchPathConflict } from "shared/utils/branch";
import { z } from "zod";
import { publicProcedure, router } from "../../..";
import {
Expand Down Expand Up @@ -328,6 +329,23 @@ export const createCreateProcedures = () => {
branch = generateBranchName({ existingBranches, authorPrefix });
}

// Check for Git ref path conflicts when creating a new branch
// e.g., can't create "release/v61" if "release" exists (or vice versa)
if (!existingBranchName) {
const conflictingBranch = findBranchPathConflict(
branch,
existingBranches,
);
if (conflictingBranch) {
const suggestedName = branch.replace(/\//g, "-");
throw new Error(
`Cannot create branch "${branch}" because branch "${conflictingBranch}" already exists. ` +
`Git cannot have branches where one is a path prefix of another. ` +
`Try using "${suggestedName}" instead.`,
);
}
}
Comment on lines +332 to +347
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

Use TRPCError instead of generic Error for consistent API error handling.

The conflict detection logic is correctly placed before any database writes. However, per coding guidelines for tRPC procedures, validation errors should throw TRPCError with code BAD_REQUEST instead of generic Error.

🔧 Proposed fix using TRPCError

First, ensure TRPCError is imported:

import { TRPCError } from "@trpc/server";

Then update the throw statement:

 				if (!existingBranchName) {
 					const conflictingBranch = findBranchPathConflict(
 						branch,
 						existingBranches,
 					);
 					if (conflictingBranch) {
 						const suggestedName = branch.replace(/\//g, "-");
-						throw new Error(
-							`Cannot create branch "${branch}" because branch "${conflictingBranch}" already exists. ` +
-								`Git cannot have branches where one is a path prefix of another. ` +
-								`Try using "${suggestedName}" instead.`,
-						);
+						throw new TRPCError({
+							code: "BAD_REQUEST",
+							message:
+								`Cannot create branch "${branch}" because branch "${conflictingBranch}" already exists. ` +
+								`Git cannot have branches where one is a path prefix of another. ` +
+								`Try using "${suggestedName}" instead.`,
+						});
 					}
 				}
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around
lines 332 - 347, Replace the generic Error throw in the branch conflict check
with a TRPCError so tRPC handlers return a proper BAD_REQUEST; import TRPCError
from "@trpc/server" at the top of the file and change the throw inside the block
that uses findBranchPathConflict (the branch conflict logic in create branch
procedure) to: throw new TRPCError({ code: "BAD_REQUEST", message: /* same
message */ }); keeping the original message text (including suggestedName)
unchanged.


const worktreePath = join(
homedir(),
SUPERSET_DIR_NAME,
Expand Down
52 changes: 52 additions & 0 deletions apps/desktop/src/shared/utils/branch.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, expect, test } from "bun:test";
import {
findBranchPathConflict,
sanitizeAuthorPrefix,
sanitizeBranchName,
sanitizeSegment,
Expand Down Expand Up @@ -86,3 +87,54 @@ describe("sanitizeBranchName", () => {
expect(sanitizeBranchName("///")).toBe("");
});
});

describe("findBranchPathConflict", () => {
test("detects conflict when new branch is child of existing", () => {
// Creating release/v61 when "release" exists
expect(findBranchPathConflict("release/v61", ["release", "main"])).toBe(
"release",
);
});

test("detects conflict when new branch is parent of existing", () => {
// Creating "release" when "release/v61" exists
expect(findBranchPathConflict("release", ["release/v61", "main"])).toBe(
"release/v61",
);
});

test("detects deep nested conflicts", () => {
// Creating feature/auth/oauth when feature/auth exists
expect(
findBranchPathConflict("feature/auth/oauth", ["feature/auth", "main"]),
).toBe("feature/auth");
});

test("returns null when no conflict exists", () => {
expect(findBranchPathConflict("feature/new", ["release", "main"])).toBe(
null,
);
});

test("returns null for sibling branches", () => {
// release-v61 is not a child of release
expect(findBranchPathConflict("release-v61", ["release", "main"])).toBe(
null,
);
});

test("handles case insensitive comparison", () => {
expect(findBranchPathConflict("Release/V61", ["release", "main"])).toBe(
"release",
);
});

test("returns null for empty existing branches", () => {
expect(findBranchPathConflict("release/v61", [])).toBe(null);
});

test("does not match exact same branch name", () => {
// Exact match is not a path conflict (it's a duplicate, handled elsewhere)
expect(findBranchPathConflict("release", ["release"])).toBe(null);
});
});
38 changes: 38 additions & 0 deletions apps/desktop/src/shared/utils/branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,41 @@ export function sanitizeBranchName(name: string): string {
.filter(Boolean)
.join("/");
}

/**
* Checks if a new branch name would conflict with existing branches due to
* Git's ref storage using file/directory structure.
*
* Git stores branches as files in .git/refs/heads/. This means:
* - If "release" exists as a branch (file at refs/heads/release), you cannot
* create "release/v1" (which would need refs/heads/release/ as a directory)
* - If "release/v1" exists, you cannot create "release" (same conflict)
*
* @param newBranch - The branch name to create
* @param existingBranches - List of existing branch names
* @returns The conflicting branch name if found, null otherwise
*/
export function findBranchPathConflict(
newBranch: string,
existingBranches: string[],
): string | null {
const newBranchLower = newBranch.toLowerCase();

for (const existing of existingBranches) {
const existingLower = existing.toLowerCase();

// Check if the new branch would be a "child" of an existing branch
// e.g., creating "release/v61" when "release" exists
if (newBranchLower.startsWith(`${existingLower}/`)) {
return existing;
}

// Check if the new branch would be a "parent" of an existing branch
// e.g., creating "release" when "release/v61" exists
if (existingLower.startsWith(`${newBranchLower}/`)) {
return existing;
}
}

return null;
}
Loading