diff --git a/apps/web/src/components/BranchToolbar.logic.test.ts b/apps/web/src/components/BranchToolbar.logic.test.ts index 36d42a60fa81..251b07688121 100644 --- a/apps/web/src/components/BranchToolbar.logic.test.ts +++ b/apps/web/src/components/BranchToolbar.logic.test.ts @@ -16,6 +16,7 @@ import { resolveLocalCheckoutBranchMismatch, resolvePreviousWorktreeLabel, resolvePreviousWorktreeSeed, + sanitizeNewRefName, shouldIncludeBranchPickerItem, shouldShowComposerContextStrip, shouldShowEnvironmentIndicator, @@ -729,4 +730,93 @@ describe("shouldIncludeBranchPickerItem", () => { }), ).toBe(false); }); + + // Typing a spaced name must still surface the ref it would have been created + // as, or the picker shows nothing at all for that query. + it("surfaces an existing ref matching the sanitized query", () => { + expect( + shouldIncludeBranchPickerItem({ + itemValue: "new-branch", + normalizedQuery: "new branch", + createBranchItemValue: null, + checkoutPullRequestItemValue: null, + }), + ).toBe(true); + }); + + // A partial query has to reach the ref it would have been created as, so + // searching "hello w" still finds an existing hello-world. + it("surfaces a ref from a partial query containing a space", () => { + expect( + shouldIncludeBranchPickerItem({ + itemValue: "hello-world", + normalizedQuery: "hello w", + createBranchItemValue: null, + checkoutPullRequestItemValue: null, + }), + ).toBe(true); + }); + + it("excludes refs matching neither the raw nor the sanitized query", () => { + expect( + shouldIncludeBranchPickerItem({ + itemValue: "main", + normalizedQuery: "new branch", + createBranchItemValue: null, + checkoutPullRequestItemValue: null, + }), + ).toBe(false); + }); +}); + +// Git rejects ASCII space and the ASCII control characters in ref names, so a +// typed name like "new branch" can only ever fail. Replacing exactly those can +// turn a failing name into a working one without touching a name git already +// accepts, including one holding non-ASCII whitespace such as U+00A0. +describe("sanitizeNewRefName", () => { + it("replaces a space with a dash", () => { + expect(sanitizeNewRefName("new branch")).toBe("new-branch"); + }); + + it("collapses a run of whitespace into a single dash", () => { + expect(sanitizeNewRefName("new branch")).toBe("new-branch"); + }); + + it("trims surrounding whitespace instead of turning it into dashes", () => { + expect(sanitizeNewRefName(" new branch ")).toBe("new-branch"); + }); + + it("replaces tabs, which git rejects just like spaces", () => { + expect(sanitizeNewRefName("new\tbranch")).toBe("new-branch"); + }); + + // git accepts U+00A0, U+2009 and other non-ASCII whitespace in ref names, so + // rewriting them would silently create a ref the user never typed. + it("preserves whitespace that git accepts", () => { + expect(sanitizeNewRefName("new\u00a0branch")).toBe("new\u00a0branch"); + expect(sanitizeNewRefName("new\u2009branch")).toBe("new\u2009branch"); + }); + + it("keeps slashes so nested ref names survive", () => { + expect(sanitizeNewRefName("feature/new thing")).toBe("feature/new-thing"); + }); + + it("preserves case because git ref names are case sensitive", () => { + expect(sanitizeNewRefName("Feature/New Thing")).toBe("Feature/New-Thing"); + }); + + it("leaves an already valid ref name untouched", () => { + expect(sanitizeNewRefName("feature/login")).toBe("feature/login"); + }); + + it("returns an empty string for whitespace-only input", () => { + expect(sanitizeNewRefName(" ")).toBe(""); + }); + + // Scoped deliberately to whitespace: git accepts consecutive dashes, so + // collapsing them would rewrite names the user may have typed on purpose. + it("does not collapse dashes the user typed", () => { + expect(sanitizeNewRefName("new - branch")).toBe("new---branch"); + expect(sanitizeNewRefName("foo--bar")).toBe("foo--bar"); + }); }); diff --git a/apps/web/src/components/BranchToolbar.logic.ts b/apps/web/src/components/BranchToolbar.logic.ts index 485ffbf8d37f..0a8e07d1958b 100644 --- a/apps/web/src/components/BranchToolbar.logic.ts +++ b/apps/web/src/components/BranchToolbar.logic.ts @@ -243,6 +243,19 @@ export function resolveBranchSelectionTarget(input: { }; } +// Git rejects ASCII space and the ASCII control characters (tab, newline and +// friends) in ref names, so the picker's "Create new ref" entry can only fail +// for a typed name like "new branch". Replacing runs of those with a dash makes +// the name usable without reimplementing check-ref-format: names invalid for +// other reasons still surface the git error. Only the whitespace git actually +// rejects is replaced — git accepts U+00A0 and friends, and rewriting those +// would silently create a ref the user never asked for. Case and existing +// dashes are left alone, since ref names are case sensitive and consecutive +// dashes are valid. +export function sanitizeNewRefName(rawName: string): string { + return rawName.trim().replace(/[ \t\n\r\f\v]+/g, "-"); +} + export function shouldIncludeBranchPickerItem(input: { itemValue: string; normalizedQuery: string; @@ -263,5 +276,18 @@ export function shouldIncludeBranchPickerItem(input: { return true; } - return itemValue.toLowerCase().includes(normalizedQuery); + const lowerItemValue = itemValue.toLowerCase(); + if (lowerItemValue.includes(normalizedQuery)) { + return true; + } + + // A query containing whitespace can only ever match a ref under its sanitized + // name, because that is the name such a ref would have been created with. + // Without this, typing "new branch" hides an existing "new-branch". + const sanitizedQuery = sanitizeNewRefName(normalizedQuery); + return ( + sanitizedQuery.length > 0 && + sanitizedQuery !== normalizedQuery && + lowerItemValue.includes(sanitizedQuery) + ); } diff --git a/apps/web/src/components/BranchToolbarBranchSelector.tsx b/apps/web/src/components/BranchToolbarBranchSelector.tsx index b3c1c08eb730..5fcad2f741db 100644 --- a/apps/web/src/components/BranchToolbarBranchSelector.tsx +++ b/apps/web/src/components/BranchToolbarBranchSelector.tsx @@ -42,6 +42,7 @@ import { resolveBranchToolbarValue, resolveDraftEnvModeAfterBranchChange, resolveEffectiveEnvMode, + sanitizeNewRefName, shouldIncludeBranchPickerItem, } from "./BranchToolbar.logic"; import { @@ -220,13 +221,18 @@ export function BranchToolbarBranchSelector({ ); const trimmedBranchQuery = branchQuery.trim(); const deferredTrimmedBranchQuery = deferredBranchQuery.trim(); + // The server filters refs by substring, so it has to be given the sanitized + // name as well: querying the raw "new branch" drops an existing new-branch + // from the response entirely, which would defeat the collision check below. + // Ref names cannot contain an ASCII space, so sanitizing loses no matches. + const branchRefQuery = sanitizeNewRefName(deferredTrimmedBranchQuery); const branchRefTarget = useMemo( () => ({ environmentId, cwd: branchCwd, - query: deferredTrimmedBranchQuery, + query: branchRefQuery, }), - [branchCwd, deferredTrimmedBranchQuery, environmentId], + [branchCwd, branchRefQuery, environmentId], ); const branchRefState = usePaginatedBranches(branchRefTarget); const refs = branchRefState.refs; @@ -259,7 +265,11 @@ export function BranchToolbarBranchSelector({ const checkoutPullRequestItemValue = prReference && onCheckoutPullRequestRequest ? `__checkout_pull_request__:${prReference}` : null; const canCreateBranch = !isSelectingWorktreeBase && trimmedBranchQuery.length > 0; - const hasExactBranchMatch = branchByName.has(trimmedBranchQuery); + // The ref is created under its sanitized name, so the collision check has to + // use that name too. Matching on the raw query would offer to create a ref + // that already exists whenever sanitizing changes the name. + const newRefName = sanitizeNewRefName(trimmedBranchQuery); + const hasExactBranchMatch = branchByName.has(newRefName); const createBranchItemValue = canCreateBranch ? `__create_new_branch__:${trimmedBranchQuery}` : null; @@ -441,7 +451,7 @@ export function BranchToolbarBranchSelector({ }; const createRef = (rawName: string) => { - const name = rawName.trim(); + const name = sanitizeNewRefName(rawName); if (!branchCwd || !name || isBranchActionPending) return; setIsBranchMenuOpen(false); @@ -659,7 +669,7 @@ export function BranchToolbarBranchSelector({ className="pe-1.5" onClick={() => createRef(trimmedBranchQuery)} > - Create new ref "{trimmedBranchQuery}" + Create new ref "{newRefName}" ); }