| author | |
| committer | |
| log | 9e61d0f127cad164702ca37aad6f5f38babc1ac7 |
| tree | 60948dddabada52d1012f38301ffd4dd97a6b5ce |
| parent | 06dd9993b6292e61de8890956a3ce3c50495a6eb |
| signature | Signed by PGP key B5690EEEBB952194 |
3 files changed, 132 insertions(+), 6 deletions(-)
apps/web/src/components/BranchToolbar.logic.test.ts+90| ... | ... | @@ -16,6 +16,7 @@ import { |
| 16 | 16 | resolveLocalCheckoutBranchMismatch, |
| 17 | 17 | resolvePreviousWorktreeLabel, |
| 18 | 18 | resolvePreviousWorktreeSeed, |
| 19 | sanitizeNewRefName, | |
| 19 | 20 | shouldIncludeBranchPickerItem, |
| 20 | 21 | shouldShowComposerContextStrip, |
| 21 | 22 | shouldShowEnvironmentIndicator, |
| ... | ... | @@ -729,4 +730,93 @@ describe("shouldIncludeBranchPickerItem", () => { |
| 729 | 730 | }), |
| 730 | 731 | ).toBe(false); |
| 731 | 732 | }); |
| 733 | ||
| 734 | // Typing a spaced name must still surface the ref it would have been created | |
| 735 | // as, or the picker shows nothing at all for that query. | |
| 736 | it("surfaces an existing ref matching the sanitized query", () => { | |
| 737 | expect( | |
| 738 | shouldIncludeBranchPickerItem({ | |
| 739 | itemValue: "new-branch", | |
| 740 | normalizedQuery: "new branch", | |
| 741 | createBranchItemValue: null, | |
| 742 | checkoutPullRequestItemValue: null, | |
| 743 | }), | |
| 744 | ).toBe(true); | |
| 745 | }); | |
| 746 | ||
| 747 | // A partial query has to reach the ref it would have been created as, so | |
| 748 | // searching "hello w" still finds an existing hello-world. | |
| 749 | it("surfaces a ref from a partial query containing a space", () => { | |
| 750 | expect( | |
| 751 | shouldIncludeBranchPickerItem({ | |
| 752 | itemValue: "hello-world", | |
| 753 | normalizedQuery: "hello w", | |
| 754 | createBranchItemValue: null, | |
| 755 | checkoutPullRequestItemValue: null, | |
| 756 | }), | |
| 757 | ).toBe(true); | |
| 758 | }); | |
| 759 | ||
| 760 | it("excludes refs matching neither the raw nor the sanitized query", () => { | |
| 761 | expect( | |
| 762 | shouldIncludeBranchPickerItem({ | |
| 763 | itemValue: "main", | |
| 764 | normalizedQuery: "new branch", | |
| 765 | createBranchItemValue: null, | |
| 766 | checkoutPullRequestItemValue: null, | |
| 767 | }), | |
| 768 | ).toBe(false); | |
| 769 | }); | |
| 770 | }); | |
| 771 | ||
| 772 | // Git rejects ASCII space and the ASCII control characters in ref names, so a | |
| 773 | // typed name like "new branch" can only ever fail. Replacing exactly those can | |
| 774 | // turn a failing name into a working one without touching a name git already | |
| 775 | // accepts, including one holding non-ASCII whitespace such as U+00A0. | |
| 776 | describe("sanitizeNewRefName", () => { | |
| 777 | it("replaces a space with a dash", () => { | |
| 778 | expect(sanitizeNewRefName("new branch")).toBe("new-branch"); | |
| 779 | }); | |
| 780 | ||
| 781 | it("collapses a run of whitespace into a single dash", () => { | |
| 782 | expect(sanitizeNewRefName("new branch")).toBe("new-branch"); | |
| 783 | }); | |
| 784 | ||
| 785 | it("trims surrounding whitespace instead of turning it into dashes", () => { | |
| 786 | expect(sanitizeNewRefName(" new branch ")).toBe("new-branch"); | |
| 787 | }); | |
| 788 | ||
| 789 | it("replaces tabs, which git rejects just like spaces", () => { | |
| 790 | expect(sanitizeNewRefName("new\tbranch")).toBe("new-branch"); | |
| 791 | }); | |
| 792 | ||
| 793 | // git accepts U+00A0, U+2009 and other non-ASCII whitespace in ref names, so | |
| 794 | // rewriting them would silently create a ref the user never typed. | |
| 795 | it("preserves whitespace that git accepts", () => { | |
| 796 | expect(sanitizeNewRefName("new\u00a0branch")).toBe("new\u00a0branch"); | |
| 797 | expect(sanitizeNewRefName("new\u2009branch")).toBe("new\u2009branch"); | |
| 798 | }); | |
| 799 | ||
| 800 | it("keeps slashes so nested ref names survive", () => { | |
| 801 | expect(sanitizeNewRefName("feature/new thing")).toBe("feature/new-thing"); | |
| 802 | }); | |
| 803 | ||
| 804 | it("preserves case because git ref names are case sensitive", () => { | |
| 805 | expect(sanitizeNewRefName("Feature/New Thing")).toBe("Feature/New-Thing"); | |
| 806 | }); | |
| 807 | ||
| 808 | it("leaves an already valid ref name untouched", () => { | |
| 809 | expect(sanitizeNewRefName("feature/login")).toBe("feature/login"); | |
| 810 | }); | |
| 811 | ||
| 812 | it("returns an empty string for whitespace-only input", () => { | |
| 813 | expect(sanitizeNewRefName(" ")).toBe(""); | |
| 814 | }); | |
| 815 | ||
| 816 | // Scoped deliberately to whitespace: git accepts consecutive dashes, so | |
| 817 | // collapsing them would rewrite names the user may have typed on purpose. | |
| 818 | it("does not collapse dashes the user typed", () => { | |
| 819 | expect(sanitizeNewRefName("new - branch")).toBe("new---branch"); | |
| 820 | expect(sanitizeNewRefName("foo--bar")).toBe("foo--bar"); | |
| 821 | }); | |
| 732 | 822 | }); |
apps/web/src/components/BranchToolbar.logic.ts+27-1| ... | ... | @@ -243,6 +243,19 @@ export function resolveBranchSelectionTarget(input: { |
| 243 | 243 | }; |
| 244 | 244 | } |
| 245 | 245 | |
| 246 | // Git rejects ASCII space and the ASCII control characters (tab, newline and | |
| 247 | // friends) in ref names, so the picker's "Create new ref" entry can only fail | |
| 248 | // for a typed name like "new branch". Replacing runs of those with a dash makes | |
| 249 | // the name usable without reimplementing check-ref-format: names invalid for | |
| 250 | // other reasons still surface the git error. Only the whitespace git actually | |
| 251 | // rejects is replaced — git accepts U+00A0 and friends, and rewriting those | |
| 252 | // would silently create a ref the user never asked for. Case and existing | |
| 253 | // dashes are left alone, since ref names are case sensitive and consecutive | |
| 254 | // dashes are valid. | |
| 255 | export function sanitizeNewRefName(rawName: string): string { | |
| 256 | return rawName.trim().replace(/[ \t\n\r\f\v]+/g, "-"); | |
| 257 | } | |
| 258 | ||
| 246 | 259 | export function shouldIncludeBranchPickerItem(input: { |
| 247 | 260 | itemValue: string; |
| 248 | 261 | normalizedQuery: string; |
| ... | ... | @@ -263,5 +276,18 @@ export function shouldIncludeBranchPickerItem(input: { |
| 263 | 276 | return true; |
| 264 | 277 | } |
| 265 | 278 | |
| 266 | return itemValue.toLowerCase().includes(normalizedQuery); | |
| 279 | const lowerItemValue = itemValue.toLowerCase(); | |
| 280 | if (lowerItemValue.includes(normalizedQuery)) { | |
| 281 | return true; | |
| 282 | } | |
| 283 | ||
| 284 | // A query containing whitespace can only ever match a ref under its sanitized | |
| 285 | // name, because that is the name such a ref would have been created with. | |
| 286 | // Without this, typing "new branch" hides an existing "new-branch". | |
| 287 | const sanitizedQuery = sanitizeNewRefName(normalizedQuery); | |
| 288 | return ( | |
| 289 | sanitizedQuery.length > 0 && | |
| 290 | sanitizedQuery !== normalizedQuery && | |
| 291 | lowerItemValue.includes(sanitizedQuery) | |
| 292 | ); | |
| 267 | 293 | } |
apps/web/src/components/BranchToolbarBranchSelector.tsx+15-5| ... | ... | @@ -42,6 +42,7 @@ import { |
| 42 | 42 | resolveBranchToolbarValue, |
| 43 | 43 | resolveDraftEnvModeAfterBranchChange, |
| 44 | 44 | resolveEffectiveEnvMode, |
| 45 | sanitizeNewRefName, | |
| 45 | 46 | shouldIncludeBranchPickerItem, |
| 46 | 47 | } from "./BranchToolbar.logic"; |
| 47 | 48 | import { |
| ... | ... | @@ -220,13 +221,18 @@ export function BranchToolbarBranchSelector({ |
| 220 | 221 | ); |
| 221 | 222 | const trimmedBranchQuery = branchQuery.trim(); |
| 222 | 223 | const deferredTrimmedBranchQuery = deferredBranchQuery.trim(); |
| 224 | // The server filters refs by substring, so it has to be given the sanitized | |
| 225 | // name as well: querying the raw "new branch" drops an existing new-branch | |
| 226 | // from the response entirely, which would defeat the collision check below. | |
| 227 | // Ref names cannot contain an ASCII space, so sanitizing loses no matches. | |
| 228 | const branchRefQuery = sanitizeNewRefName(deferredTrimmedBranchQuery); | |
| 223 | 229 | const branchRefTarget = useMemo( |
| 224 | 230 | () => ({ |
| 225 | 231 | environmentId, |
| 226 | 232 | cwd: branchCwd, |
| 227 | query: deferredTrimmedBranchQuery, | |
| 233 | query: branchRefQuery, | |
| 228 | 234 | }), |
| 229 | [branchCwd, deferredTrimmedBranchQuery, environmentId], | |
| 235 | [branchCwd, branchRefQuery, environmentId], | |
| 230 | 236 | ); |
| 231 | 237 | const branchRefState = usePaginatedBranches(branchRefTarget); |
| 232 | 238 | const refs = branchRefState.refs; |
| ... | ... | @@ -259,7 +265,11 @@ export function BranchToolbarBranchSelector({ |
| 259 | 265 | const checkoutPullRequestItemValue = |
| 260 | 266 | prReference && onCheckoutPullRequestRequest ? `__checkout_pull_request__:${prReference}` : null; |
| 261 | 267 | const canCreateBranch = !isSelectingWorktreeBase && trimmedBranchQuery.length > 0; |
| 262 | const hasExactBranchMatch = branchByName.has(trimmedBranchQuery); | |
| 268 | // The ref is created under its sanitized name, so the collision check has to | |
| 269 | // use that name too. Matching on the raw query would offer to create a ref | |
| 270 | // that already exists whenever sanitizing changes the name. | |
| 271 | const newRefName = sanitizeNewRefName(trimmedBranchQuery); | |
| 272 | const hasExactBranchMatch = branchByName.has(newRefName); | |
| 263 | 273 | const createBranchItemValue = canCreateBranch |
| 264 | 274 | ? `__create_new_branch__:${trimmedBranchQuery}` |
| 265 | 275 | : null; |
| ... | ... | @@ -441,7 +451,7 @@ export function BranchToolbarBranchSelector({ |
| 441 | 451 | }; |
| 442 | 452 | |
| 443 | 453 | const createRef = (rawName: string) => { |
| 444 | const name = rawName.trim(); | |
| 454 | const name = sanitizeNewRefName(rawName); | |
| 445 | 455 | if (!branchCwd || !name || isBranchActionPending) return; |
| 446 | 456 | |
| 447 | 457 | setIsBranchMenuOpen(false); |
| ... | ... | @@ -659,7 +669,7 @@ export function BranchToolbarBranchSelector({ |
| 659 | 669 | className="pe-1.5" |
| 660 | 670 | onClick={() => createRef(trimmedBranchQuery)} |
| 661 | 671 | > |
| 662 | <span className="truncate">Create new ref &quot;{trimmedBranchQuery}&quot;</span> | |
| 672 | <span className="truncate">Create new ref &quot;{newRefName}&quot;</span> | |
| 663 | 673 | </ComboboxItem> |
| 664 | 674 | ); |
| 665 | 675 | } |