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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { formatRelativeTime } from "renderer/lib/formatRelativeTime";
import { electronTrpcClient as trpcClient } from "renderer/lib/trpc-client";
import { useCreateWorkspace } from "renderer/react-query/workspaces";
import { NotFound } from "renderer/routes/not-found";
import { sanitizeSegment } from "shared/utils/branch";

export const Route = createFileRoute(
"/_authenticated/_dashboard/project/$projectId/",
Expand Down Expand Up @@ -53,17 +54,7 @@ function generateBranchFromTitle({
title: string;
authorPrefix?: string;
}): string {
if (!title.trim()) return "";

const slug = title
.toLowerCase()
.trim()
.replace(/[^a-z0-9\s-]/g, "")
.replace(/\s+/g, "-")
.replace(/-+/g, "-")
.replace(/^-|-$/g, "")
.slice(0, 50);

const slug = sanitizeSegment(title);
if (!slug) return "";

if (authorPrefix) {
Expand Down
54 changes: 52 additions & 2 deletions apps/desktop/src/shared/utils/branch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,41 @@ describe("sanitizeSegment", () => {
expect(sanitizeSegment("Hello's World!")).toBe("hellos-world");
});

test("preserves underscores", () => {
expect(sanitizeSegment("hello_world")).toBe("hello_world");
});

test("preserves dots", () => {
expect(sanitizeSegment("v1.0.0")).toBe("v1.0.0");
});

test("preserves plus signs", () => {
expect(sanitizeSegment("c++fix")).toBe("c++fix");
});

test("preserves at signs", () => {
expect(sanitizeSegment("user@feature")).toBe("user@feature");
});

test("strips @{ sequence", () => {
expect(sanitizeSegment("test@{0}")).toBe("test@0");
});

test("collapses consecutive dots", () => {
expect(sanitizeSegment("hello..world")).toBe("hello.world");
});

test("removes .lock suffix", () => {
expect(sanitizeSegment("hello.lock")).toBe("hello");
});

test("collapses multiple hyphens", () => {
expect(sanitizeSegment("hello--world")).toBe("hello-world");
});

test("removes leading/trailing hyphens", () => {
test("removes leading/trailing hyphens and dots", () => {
expect(sanitizeSegment("-hello-")).toBe("hello");
expect(sanitizeSegment(".hello.")).toBe("hello");
});

test("respects maxLength", () => {
Expand All @@ -44,8 +73,9 @@ describe("sanitizeAuthorPrefix", () => {
expect(sanitizeAuthorPrefix("John Doe")).toBe("john-doe");
});

test("removes special characters", () => {
test("removes special characters but keeps underscores and dots", () => {
expect(sanitizeAuthorPrefix("John's Name!")).toBe("johns-name");
expect(sanitizeAuthorPrefix("user_name")).toBe("user_name");
});

test("collapses multiple hyphens", () => {
Expand Down Expand Up @@ -78,6 +108,26 @@ describe("sanitizeBranchName", () => {
expect(sanitizeBranchName("John's/Feature!")).toBe("johns/feature");
});

test("preserves underscores and dots in branch names", () => {
expect(sanitizeBranchName("user/fix_bug")).toBe("user/fix_bug");
expect(sanitizeBranchName("release/v1.0.0")).toBe("release/v1.0.0");
});

test("preserves plus and at signs", () => {
expect(sanitizeBranchName("user/c++fix")).toBe("user/c++fix");
expect(sanitizeBranchName("user@org/feature")).toBe("user@org/feature");
});

test("strips git-forbidden characters", () => {
expect(sanitizeBranchName("feat/test~1")).toBe("feat/test1");
expect(sanitizeBranchName("feat/test^2")).toBe("feat/test2");
expect(sanitizeBranchName("feat/test:foo")).toBe("feat/testfoo");
expect(sanitizeBranchName("feat/test?foo")).toBe("feat/testfoo");
expect(sanitizeBranchName("feat/test*foo")).toBe("feat/testfoo");
expect(sanitizeBranchName("feat/test[foo")).toBe("feat/testfoo");
expect(sanitizeBranchName("feat/test\\foo")).toBe("feat/testfoo");
});

test("handles empty string", () => {
expect(sanitizeBranchName("")).toBe("");
});
Expand Down
7 changes: 5 additions & 2 deletions apps/desktop/src/shared/utils/branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ export function sanitizeSegment(text: string, maxLength = 50): string {
return text
.toLowerCase()
.trim()
.replace(/[^a-z0-9\s-]/g, "")
.replace(/\s+/g, "-")
.replace(/[^a-z0-9._+@-]/g, "")
.replace(/\.{2,}/g, ".")
.replace(/@\{/g, "@")
Comment on lines +6 to +8

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.

⚠️ Potential issue | 🟡 Minor

@{ replacement on line 8 is dead code — { is already stripped by line 6.

The allowlist on line 6 ([^a-z0-9._+@-]) does not include {, so { is removed before the @{ replacement on line 8 ever runs. For example, "test@{0}" becomes "test@0" at line 6 already.

The test still passes because line 6 handles it, but line 8 is unreachable. Either remove line 8 or reorder lines 7-8 before line 6 so the @{ → @ normalization runs on the original input first.

Suggested fix: remove the dead line
 		.replace(/[^a-z0-9._+@-]/g, "")
 		.replace(/\.{2,}/g, ".")
-		.replace(/@\{/g, "@")
 		.replace(/-+/g, "-")
🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` around lines 6 - 8, The final
.replace(/@\{/g, "@") call in the string normalization chain is dead code
because the earlier .replace(/[^a-z0-9._+@-]/g, "") already strips the "{"
character; remove the unreachable .replace(/@\{/g, "@") from the replace chain
in apps/desktop/src/shared/utils/branch.ts (the normalization pipeline that
applies .replace(/[^a-z0-9._+@-]/g, "") and .replace(/\.{2,}/g, ".")). This
keeps behavior identical and removes the redundant operation; alternatively, if
you prefer normalization of the original input first, move the @\{→@ replacement
to run before the allowlist replace, but prefer simply deleting the dead
.replace(/@\{/g, "@") call.

.replace(/-+/g, "-")
.replace(/^-|-$/g, "")
.replace(/^[-.]|[-.]+$/g, "")

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.

⚠️ Potential issue | 🟡 Minor

Leading-character strip only removes a single [-.], not a run of mixed chars.

/^[-.]|[-.]+$/g — the ^[-.] (no +) matches exactly one character. If the input after earlier steps has a sequence like .-hello, only the . is removed, leaving -hello. The trailing side correctly uses [-.]+$. The leading side should be symmetrical:

Suggested fix
-		.replace(/^[-.]|[-.]+$/g, "")
+		.replace(/^[-.]+|[-.]+$/g, "")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.replace(/^[-.]|[-.]+$/g, "")
.replace(/^[-.]+|[-.]+$/g, "")
🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` at line 10, The leading-character
strip regex in the replace call (.replace(/^[-.]|[-.]+$/g, "")) only removes a
single leading dot or dash; update the pattern used in that replace to remove
runs of leading mixed dots/dashes symmetrically (e.g., change the left
alternation to use a + like the right side) so inputs like ".-hello" have all
leading punctuation removed; locate the replace invocation in branch.ts and
replace the regex /^[-.]|[-.]+$/g with a regex that uses + on the leading side
(making it /^[-.]+|[-.]+$/g).

.replace(/\.lock$/g, "")

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.

⚠️ Potential issue | 🟡 Minor

Repeated .lock suffix is not fully stripped.

A single .replace(/\.lock$/g, "") pass turns foo.lock.lock into foo.lock, which still violates git check-ref-format. Consider a greedy pattern:

Suggested fix
-		.replace(/\.lock$/g, "")
+		.replace(/(\.lock)+$/g, "")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.replace(/\.lock$/g, "")
.replace(/(\.lock)+$/g, "")
🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` at line 11, The current
.replace(/\.lock$/g, "") only strips one trailing ".lock" so names like
"foo.lock.lock" remain invalid; update the .replace call in branch.ts to use a
pattern that matches one or more repeated ".lock" suffixes at the end (i.e., a
greedy/quantified pattern that removes all consecutive ".lock" occurrences) so
the sanitization fully strips any repeated ".lock" endings.

.slice(0, maxLength);
}

Expand Down
Loading