Skip to content

feat(ui): use backend normalize method to validate user-entered branch names #12488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nshcr wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(ui): use backend normalize method to validate user-entered branch names #12488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions apps/desktop/src/components/AddDependentBranchModal.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,18 @@

let modal = $state<Modal>();
let branchName = $state<string>();
let slugifiedRefName: string | undefined = $state();
let normalizedRefName: string | undefined = $state();
let isBranchNameValid = $state(false);

async function handleAddDependentBranch(close: () => void) {
if (!slugifiedRefName) return;
if (!normalizedRefName) return;

await createNewBranch({
projectId,
stackId,
request: {
targetPatch: undefined,
name: slugifiedRefName,
name: normalizedRefName,
},
});

Expand All @@ -52,7 +53,8 @@
placeholder="Branch name"
bind:value={branchName}
autofocus
onslugifiedvalue={(value) => (slugifiedRefName = value)}
onnormalizedvalue={(value) => (normalizedRefName = value)}
onvalidationchange={(isValid) => (isBranchNameValid = isValid)}
/>
</div>
{#snippet controls(close)}
Expand All @@ -61,7 +63,7 @@
testId={TestId.BranchHeaderAddDependanttBranchModal_ActionButton}
style="pop"
type="submit"
disabled={!slugifiedRefName}
disabled={!isBranchNameValid}
loading={branchCreation.current.isLoading}>Add branch</Button
>
{/snippet}
Expand Down
91 changes: 81 additions & 10 deletions apps/desktop/src/components/BranchNameTextbox.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,108 @@
<script lang="ts">
import { Textbox } from "@gitbutler/ui";
import { slugify } from "@gitbutler/ui/utils/string";
import { STACK_SERVICE } from "$lib/stacks/stackService.svelte";
import { debounce } from "$lib/utils/debounce";
import { inject } from "@gitbutler/core/context";
import { Icon, Textbox } from "@gitbutler/ui";

type Props = {
value?: string;
helperText?: string;
onslugifiedvalue?: (slugified: string | undefined) => void;
onnormalizedvalue?: (normalized: string | undefined) => void;
onvalidationchange?: (isValid: boolean) => void;
[key: string]: any;
};

let {
value = $bindable(),
helperText,

onslugifiedvalue,
onnormalizedvalue,
onvalidationchange,
...restProps
}: Props = $props();

const stackService = inject(STACK_SERVICE);

let textbox = $state<ReturnType<typeof Textbox>>();
let isValidating = $state(false);
let validationError = $state<string | undefined>();
let validationCounter = $state(0);

let normalizedResult = $state<{ fromValue: string; normalized: string } | undefined>();

const slugifiedName = $derived(value && slugify(value));
const namesDiverge = $derived(!!value && slugifiedName !== value);
const isValidState = $derived(
!isValidating &&
!validationError &&
!!value &&
!!normalizedResult?.normalized &&
normalizedResult.fromValue === value,
);
$effect(() => {
onvalidationchange?.(isValidState);
});

const namesDiverge = $derived(
!!normalizedResult && normalizedResult.normalized !== normalizedResult.fromValue,
);
const computedHelperText = $derived(
namesDiverge ? `Will be created as '${slugifiedName}'` : helperText,
namesDiverge && normalizedResult
? `Will be created as '${normalizedResult.normalized}'`
: helperText,
);

const debouncedNormalize = debounce(async (inputValue: string) => {
if (!inputValue) {
isValidating = false;
validationError = undefined;
normalizedResult = undefined;
onnormalizedvalue?.(undefined);
return;
}

const currentValidation = ++validationCounter;
isValidating = true;
validationError = undefined;

try {
const result = await stackService.normalizeBranchName(inputValue);
// Only update if the value hasn't changed during the async call
// and no newer validation has started
if (value === inputValue && currentValidation === validationCounter) {
normalizedResult = { fromValue: inputValue, normalized: result };
onnormalizedvalue?.(result);
validationError = undefined;
}
} catch {
if (value === inputValue && currentValidation === validationCounter) {
normalizedResult = undefined;
onnormalizedvalue?.(undefined);
validationError = "Invalid branch name";
}
Comment on lines +74 to +79
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider logging the caught error for debugging purposes. Currently, when the normalization API call fails, the error is silently caught and only a generic 'Invalid branch name' message is shown. Logging the actual error would help diagnose issues with branch name validation. For example: catch (err) { console.error('Branch name normalization failed:', err); ... }

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@nshcr nshcr Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since validation runs dynamically as the user types, I think it's acceptable to silently catch the error and display a generic error message (IPC errors are printed to the console by default and it's difficult to directly retrieve the message from the error object).
Leaving this conversation as is.

} finally {
if (currentValidation === validationCounter) {
isValidating = false;
}
}
Comment on lines +80 to +84
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential race condition in the validation state management. When a user types quickly, multiple API calls can be in flight simultaneously. When an outdated API call completes, the finally block sets isValidating = false even though a newer validation is still pending. This could cause the spinner to briefly disappear and the button to become enabled prematurely. Consider tracking a unique identifier for each validation request or canceling previous requests when a new one starts.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@nshcr nshcr Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This race condition was resolved by adding a counter to the debounced function call and checking whether the counter has changed before actually updating the state, ensuring that no additional async calls are still in progress.
I believe this implementation is sound. Leaving this thread unresolved for further review.

}, 100);

$effect(() => {
onslugifiedvalue?.(slugifiedName);
debouncedNormalize(value || "");
});
Comment on lines 87 to 89
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $effect that calls debouncedNormalize doesn't provide a cleanup function to clear pending timeouts when the component unmounts or when the value changes. This could lead to memory leaks or stale API calls completing after the component is destroyed.

Based on the pattern used elsewhere in the codebase (e.g., UnassignedViewForgePrompt.svelte:47), the effect should return a cleanup function. However, the current debounce utility doesn't expose a way to cancel pending calls.

Consider either:

  1. Enhancing the debounce utility to return an object with both the debounced function and a cancel() method, or
  2. Creating a custom debounced effect that manages its own timeout state within the component and returns a cleanup function that clears the timeout.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@nshcr nshcr Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect here doesn't need cleanup. The debounced function already has an internal mechanism to ensure that only the last invocation is executed, and the code also prevents concurrent state updates.
Adding a cleanup function would only increase code complexity in exchange for eliminating just one extra 20ms invocation.


export async function selectAll() {
await textbox?.selectAll();
}
</script>

<Textbox bind:this={textbox} bind:value helperText={computedHelperText} {...restProps} />
<Textbox
bind:this={textbox}
bind:value
helperText={computedHelperText}
error={validationError}
{...restProps}
>
{#snippet customIconRight()}
{#if isValidating}
<Icon name="spinner" />
{/if}
{/snippet}
</Textbox>
12 changes: 7 additions & 5 deletions apps/desktop/src/components/BranchRenameModal.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
const [renameBranch, renameQuery] = stackService.updateBranchName;

let newName: string | undefined = $state();
let slugifiedRefName: string | undefined = $state();
let normalizedRefName: string | undefined = $state();
let isBranchNameValid = $state(false);
let modal: Modal | undefined = $state();

let branchNameInput = $state<ReturnType<typeof BranchNameTextbox>>();
Expand All @@ -40,8 +41,8 @@
type={isPushed ? "warning" : "info"}
bind:this={modal}
onSubmit={async (close) => {
if (slugifiedRefName) {
renameBranch({ projectId, stackId, laneId, branchName, newName: slugifiedRefName });
if (normalizedRefName) {
renameBranch({ projectId, stackId, laneId, branchName, newName: normalizedRefName });
}
close();
}}
Expand All @@ -52,7 +53,8 @@
id={ElementId.NewBranchNameInput}
bind:value={newName}
autofocus
onslugifiedvalue={(value) => (slugifiedRefName = value)}
onnormalizedvalue={(value) => (normalizedRefName = value)}
onvalidationchange={(isValid) => (isBranchNameValid = isValid)}
/>

{#if isPushed}
Expand All @@ -68,7 +70,7 @@
testId={TestId.BranchHeaderRenameModal_ActionButton}
style="pop"
type="submit"
disabled={!slugifiedRefName}
disabled={!isBranchNameValid}
loading={renameQuery.current.isLoading}>Rename</Button
>
{/snippet}
Expand Down
13 changes: 8 additions & 5 deletions apps/desktop/src/components/ChangedFilesContextMenu.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@
}

let stashBranchName = $state<string>();
let slugifiedRefName: string | undefined = $state();
let normalizedRefName: string | undefined = $state();
let isStashBranchNameValid = $state(false);
let stashBranchNameInput = $state<ReturnType<typeof BranchNameTextbox>>();
let absorbPlan = $state<HunkAssignment.CommitAbsorption[]>([]);

Expand Down Expand Up @@ -639,7 +640,8 @@
type="info"
title="Stash changes into a new branch"
bind:this={stashConfirmationModal}
onSubmit={(_, item) => isChangedFilesItem(item) && confirmStashIntoBranch(item, slugifiedRefName)}
onSubmit={(_, item) =>
isChangedFilesItem(item) && confirmStashIntoBranch(item, normalizedRefName)}
>
{#snippet children(item)}
<div class="content-wrap">
Expand All @@ -649,7 +651,8 @@
placeholder="Enter your branch name..."
bind:value={stashBranchName}
autofocus
onslugifiedvalue={(value) => (slugifiedRefName = value)}
onnormalizedvalue={(value) => (normalizedRefName = value)}
onvalidationchange={(isValid) => (isStashBranchNameValid = isValid)}
/>
<div class="explanation">
<p class="primary-text">
Expand All @@ -675,9 +678,9 @@
<Button kind="outline" type="reset" onclick={close}>Cancel</Button>
<AsyncButton
style="pop"
disabled={!slugifiedRefName}
disabled={!isStashBranchNameValid}
type="submit"
action={async () => await confirmStashIntoBranch(item, slugifiedRefName)}
action={async () => await confirmStashIntoBranch(item, normalizedRefName)}
>
Stash into branch
</AsyncButton>
Expand Down
14 changes: 8 additions & 6 deletions apps/desktop/src/components/CreateBranchModal.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@
// Persisted preference for branch placement
const addToLeftmost = persisted<boolean>(false, "branch-placement-leftmost");

let slugifiedRefName: string | undefined = $state();
let normalizedRefName: string | undefined = $state();
let isBranchNameValid = $state(false);

// Get all stacks in the workspace
const allStacksQuery = $derived(stackService.stacks(projectId));
Expand Down Expand Up @@ -91,22 +92,22 @@
await createNewStack({
projectId,
branch: {
name: slugifiedRefName,
name: normalizedRefName,
// If addToLeftmost is true, place at position 0 (leftmost)
// Otherwise, leave undefined to append to the right
order: $addToLeftmost ? 0 : undefined,
},
});
createRefModal?.close();
} else {
if (!selectedStackId || !slugifiedRefName) {
if (!selectedStackId || !normalizedRefName) {
// TODO: Add input validation.
return;
}
await createNewBranch({
projectId,
stackId: selectedStackId,
request: { targetPatch: undefined, name: slugifiedRefName },
request: { targetPatch: undefined, name: normalizedRefName },
});
createRefModal?.close();
}
Expand Down Expand Up @@ -145,7 +146,8 @@
id={ElementId.NewBranchNameInput}
value={createRefName}
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value prop should be a two-way binding using bind:value={createRefName} instead of just value={createRefName}. Without the bind: directive, the createRefName variable won't update as the user types, and the component will only display the initial value fetched from the backend. This is inconsistent with other usages of BranchNameTextbox in the codebase (e.g., BranchRenameModal.svelte:54, AddDependentBranchModal.svelte:54, ChangedFilesContextMenu.svelte:652).

Suggested change
value={createRefName}
bind:value={createRefName}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@nshcr nshcr Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is mainly used to provide an initial value when opening the modal for creating a new branch. It does not participate in subsequent updates, so two-way binding isn't necessary.

autofocus
onslugifiedvalue={(value) => (slugifiedRefName = value)}
onnormalizedvalue={(value) => (normalizedRefName = value)}
onvalidationchange={(isValid) => (isBranchNameValid = isValid)}
/>

<div class="options-wrap" role="radiogroup" aria-label="Branch type selection">
Expand Down Expand Up @@ -270,7 +272,7 @@
style="pop"
type="submit"
onclick={addNew}
disabled={!createRefName || (createRefType === "dependent" && !selectedStackId)}
disabled={!isBranchNameValid || (createRefType === "dependent" && !selectedStackId)}
loading={isAddingNew}
testId={TestId.ConfirmSubmit}
>
Expand Down
Loading