fix(web): close tooltips on popover close and refine branch chip UX (#808)

This commit is contained in:
Carlos Florêncio
2026-05-03 22:45:52 +01:00
committed by GitHub
parent cc934adb85
commit 2732c101d9
6 changed files with 375 additions and 135 deletions
@@ -0,0 +1,189 @@
import { describe, it, expect } from "vitest";
import {
computeBranchPrefix,
computeBranchTooltip,
computeBranchDisabledReason,
} from "./task-create-dialog-branch-utils";
describe("computeBranchPrefix", () => {
it("returns empty when no branch is picked yet", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "",
currentLocalBranch: "main",
freshBranchEnabled: false,
}),
).toBe("");
});
it("returns 'current: ' for local executor when row branch matches workspace current", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "main",
currentLocalBranch: "main",
freshBranchEnabled: false,
}),
).toBe("current: ");
});
it("returns 'current: ' on detached HEAD when the picked value is the same short SHA", () => {
// Backend returns a short SHA in `current_branch` when the repo is on
// detached HEAD; the autoselect seeds row.branch with that same value
// so the chip can read "current: <sha>" instead of falling through to
// "will switch to: master".
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "4fbc5d7",
currentLocalBranch: "4fbc5d7",
freshBranchEnabled: false,
}),
).toBe("current: ");
});
it("returns 'will switch to: ' for local executor when row branch differs from current", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "develop",
currentLocalBranch: "main",
freshBranchEnabled: false,
}),
).toBe("will switch to: ");
});
it("treats unknown current branch as 'will switch to' on local executor", () => {
// Local-status fetch error leaves currentLocalBranch="". With no baseline
// we can't claim the picked branch is "current", so fall through to the
// destructive label.
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "main",
currentLocalBranch: "",
freshBranchEnabled: false,
}),
).toBe("will switch to: ");
});
it("returns 'from: ' for non-local executors regardless of current branch", () => {
expect(
computeBranchPrefix({
isLocalExecutor: false,
rowBranch: "main",
currentLocalBranch: "main",
freshBranchEnabled: false,
}),
).toBe("from: ");
expect(
computeBranchPrefix({
isLocalExecutor: false,
rowBranch: "develop",
currentLocalBranch: "",
freshBranchEnabled: false,
}),
).toBe("from: ");
});
it("returns 'from: ' when fork-a-new-branch is enabled, even on local executor", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "main",
currentLocalBranch: "main",
freshBranchEnabled: true,
}),
).toBe("from: ");
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "develop",
currentLocalBranch: "main",
freshBranchEnabled: true,
}),
).toBe("from: ");
});
});
describe("computeBranchTooltip", () => {
it('returns the "current" tooltip for "current: " prefix', () => {
expect(computeBranchTooltip("current: ")).toContain("as-is");
});
it('returns the "switch" tooltip for "will switch to: " prefix', () => {
expect(computeBranchTooltip("will switch to: ")).toContain("git checkout");
});
it('returns the "fork" tooltip for "from: " prefix', () => {
expect(computeBranchTooltip("from: ")).toContain("forked");
});
it("returns a generic tooltip for empty or undefined prefix", () => {
expect(computeBranchTooltip("")).toContain("Branch the agent will run against");
expect(computeBranchTooltip(undefined)).toContain("Branch the agent will run against");
});
it("returns a generic tooltip for an unrecognized prefix", () => {
// Defensive: unknown prefix shouldn't crash, just falls back.
expect(computeBranchTooltip("foo: ")).toContain("Branch the agent will run against");
});
});
describe("computeBranchDisabledReason", () => {
it("returns the lock reason when branchLocked is true", () => {
expect(
computeBranchDisabledReason({
branchLocked: true,
hasRepo: true,
branchesLoading: false,
optionCount: 5,
}),
).toContain("local executor");
});
it("asks to select a repo first when none is set", () => {
expect(
computeBranchDisabledReason({
branchLocked: false,
hasRepo: false,
branchesLoading: false,
optionCount: 0,
}),
).toBe("Select a repository first.");
});
it("reports loading while branches are being fetched", () => {
expect(
computeBranchDisabledReason({
branchLocked: false,
hasRepo: true,
branchesLoading: true,
optionCount: 0,
}),
).toContain("Loading");
});
it("reports no branches available when the list is empty", () => {
expect(
computeBranchDisabledReason({
branchLocked: false,
hasRepo: true,
branchesLoading: false,
optionCount: 0,
}),
).toContain("No branches");
});
it("returns undefined when the chip should be enabled", () => {
expect(
computeBranchDisabledReason({
branchLocked: false,
hasRepo: true,
branchesLoading: false,
optionCount: 5,
}),
).toBeUndefined();
});
});
@@ -0,0 +1,82 @@
/**
* Pure helpers for the branch chip in the task-create dialog.
*
* Extracted from `task-create-dialog-repo-chips.tsx` to keep that file under
* the 600-line cap. No React deps — these are string functions tested
* directly in `task-create-dialog-branch-utils.test.ts`.
*/
/**
* Decide the muted text shown before the branch value to qualify intent.
* "current: " — local exec, picked branch matches the current checkout
* (also covers detached HEAD where the "branch" is the
* short SHA returned by the backend).
* "will switch to: " — local exec, picked branch differs from current.
* Surfaces the destructive `git checkout` decision the
* backend will make on submit.
* "from: " — fork mode (forking a new branch off this base) or
* non-local executors (worktree base).
*
* Returns "" when there's no value yet, so the chip's placeholder ("branch")
* doesn't get a misleading prefix.
*/
export function computeBranchPrefix({
isLocalExecutor,
rowBranch,
currentLocalBranch,
freshBranchEnabled,
}: {
isLocalExecutor: boolean;
rowBranch: string;
currentLocalBranch: string;
freshBranchEnabled: boolean;
}): string {
if (!rowBranch) return "";
if (freshBranchEnabled) return "from: ";
if (isLocalExecutor) {
if (currentLocalBranch && rowBranch === currentLocalBranch) return "current: ";
return "will switch to: ";
}
return "from: ";
}
/**
* Branch chip hover tooltip, picked to match whatever prefix the chip is
* currently showing so the explanation lines up with the qualifier text.
* "current: " → no git ops will run; agent uses your existing checkout
* "will switch to: " → backend runs `git checkout` before the agent starts
* "from: " → a new branch / worktree is forked off this base
* "" → no branch picked yet; generic hint
*/
export function computeBranchTooltip(branchPrefix: string | undefined): string {
if (branchPrefix === "current: ") {
return "Your repository's current checkout. The agent runs against it as-is, no git operations.";
}
if (branchPrefix === "will switch to: ") {
return "The backend will run `git checkout` to switch your repository to this branch before the agent starts.";
}
if (branchPrefix === "from: ") {
return "Base branch. A new branch (or worktree) is forked from here and the agent runs there.";
}
return "Branch the agent will run against.";
}
export function computeBranchDisabledReason({
branchLocked,
hasRepo,
branchesLoading,
optionCount,
}: {
branchLocked: boolean;
hasRepo: boolean;
branchesLoading: boolean;
optionCount: number;
}): string | undefined {
if (branchLocked) {
return "The local executor uses your repository's current checkout, so the branch can't change here. Toggle 'Fork a new branch' to pick a different base.";
}
if (!hasRepo) return "Select a repository first.";
if (branchesLoading) return "Loading branches…";
if (optionCount === 0) return "No branches available for this repository.";
return undefined;
}
@@ -19,6 +19,13 @@ import { STORAGE_KEYS } from "@/lib/settings/constants";
function clearFreshBranch(fs: DialogFormState) {
fs.setFreshBranchEnabled(false);
fs.setCurrentLocalBranch("");
// Set loading=true synchronously alongside the clear so the chip's
// autoselect effect (which runs bottom-up before useCurrentLocalBranchEffect
// can re-fire and set loading itself) sees the gate and skips. Otherwise
// the autoselect lands a last-used / preferred-default branch in row.branch
// before currentLocalBranch resolves, then the prefix logic computes
// "will switch to: master" instead of "current: master".
fs.setCurrentLocalBranchLoading(true);
}
function useRepositoryHandlers(fs: DialogFormState, repositories: Repository[]) {
@@ -1,6 +1,6 @@
"use client";
import { useState } from "react";
import { useCallback, useEffect, useRef, useState } from "react";
import { cn } from "@/lib/utils";
import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover";
import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip";
@@ -133,7 +133,39 @@ export function Pill({
tooltip,
prefix,
}: PillProps) {
const [open, setOpen] = useState(false);
const [open, setOpenState] = useState(false);
// Tracks the brief window after the popover closes during which the cursor
// is still hovering the trigger. Without this, Radix's Tooltip becomes
// uncontrolled the moment `open` flips to false and pops back open from the
// lingering hover. Holding `open={false}` for ~200ms lets the user move
// away naturally and prevents the tooltip from racing back in.
const [suppressTooltip, setSuppressTooltip] = useState(false);
const suppressTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const setOpen = useCallback((next: boolean) => {
setOpenState((prev) => {
if (prev && !next) {
if (suppressTimerRef.current) clearTimeout(suppressTimerRef.current);
setSuppressTooltip(true);
suppressTimerRef.current = setTimeout(() => {
setSuppressTooltip(false);
suppressTimerRef.current = null;
}, 200);
}
return next;
});
}, []);
// Clear the suppression timer if the component unmounts while it's still
// ticking, so the eventual `setSuppressTooltip(false)` doesn't fire on a
// dead component.
useEffect(
() => () => {
if (suppressTimerRef.current) {
clearTimeout(suppressTimerRef.current);
suppressTimerRef.current = null;
}
},
[],
);
const hasValue = !!value;
// Prefix only renders alongside a real value; an empty chip with placeholder
// ("branch") doesn't need "current: " in front of it.
@@ -193,9 +225,13 @@ export function Pill({
if (!tooltip) return popover;
// Suppress the hover tooltip while the popover is open so they don't stack.
// Suppress the hover tooltip while the popover is open so they don't stack,
// and for a brief window after it closes so a lingering hover doesn't pop
// the tooltip back in (which Radix would otherwise do the moment we flip
// from controlled-closed back to uncontrolled).
const tooltipOpen = open || suppressTooltip ? false : undefined;
return (
<Tooltip open={open ? false : undefined}>
<Tooltip open={tooltipOpen}>
{popover}
<TooltipContent>{tooltip}</TooltipContent>
</Tooltip>
@@ -1,5 +1,4 @@
import { describe, it, expect, vi, afterEach } from "vitest";
import { computeBranchPrefix } from "./task-create-dialog-repo-chips";
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
import type { Branch, Repository } from "@/lib/types/http";
import type { DialogFormState, TaskRepoRow } from "./task-create-dialog-types";
@@ -374,62 +373,3 @@ describe("RepoChipsRow", () => {
mockBranches.value = { branches: [], isLoading: false };
});
});
describe("computeBranchPrefix", () => {
it("returns empty when no branch is picked yet", () => {
expect(
computeBranchPrefix({ isLocalExecutor: true, rowBranch: "", currentLocalBranch: "main" }),
).toBe("");
});
it("returns 'current: ' for local executor when row branch matches workspace current", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "main",
currentLocalBranch: "main",
}),
).toBe("current: ");
});
it("returns 'will switch to: ' for local executor when row branch differs from current", () => {
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "develop",
currentLocalBranch: "main",
}),
).toBe("will switch to: ");
});
it("treats unknown current branch as 'will switch to' on local executor", () => {
// Detached HEAD or local-status fetch error returns "" for currentLocalBranch.
// Without a known baseline we can't claim the picked branch is "current",
// so fall through to the destructive label — the user picked something
// explicit and the backend will checkout to it.
expect(
computeBranchPrefix({
isLocalExecutor: true,
rowBranch: "main",
currentLocalBranch: "",
}),
).toBe("will switch to: ");
});
it("returns 'from: ' for non-local executors regardless of current branch", () => {
expect(
computeBranchPrefix({
isLocalExecutor: false,
rowBranch: "main",
currentLocalBranch: "main",
}),
).toBe("from: ");
expect(
computeBranchPrefix({
isLocalExecutor: false,
rowBranch: "develop",
currentLocalBranch: "",
}),
).toBe("from: ");
});
});
@@ -17,6 +17,38 @@ import {
type PillOption,
} from "@/components/task-create-dialog-pill";
import { GitHubUrlSection } from "@/components/task-create-dialog-github-url";
import {
computeBranchPrefix,
computeBranchTooltip,
computeBranchDisabledReason,
} from "@/components/task-create-dialog-branch-utils";
function runAutoselect({
branches,
preferredDefaultBranch,
preferredDefaultBranchLoading,
onBranchChange,
}: {
branches: Array<{ name: string; type: "local" | "remote"; remote?: string }>;
preferredDefaultBranch: string | undefined;
preferredDefaultBranchLoading: boolean;
onBranchChange: (value: string) => void;
}) {
if (preferredDefaultBranchLoading) return;
// Local-executor flow: preferredDefaultBranch is the workspace's current
// ref (branch name OR detached-HEAD short SHA). Seed row.branch with it
// unconditionally so the chip displays "current: <ref>". When the ref is a
// SHA (detached HEAD) it won't appear in the branches list, but falling
// through to autoSelectBranch would pick main/master and surface "will
// switch to: master", which contradicts what the user actually has checked
// out. Sending the SHA back on submit is a no-op because the backend's
// skip-when-equal check compares against the same SHA.
if (preferredDefaultBranch) {
onBranchChange(preferredDefaultBranch);
return;
}
autoSelectBranch(branches, onBranchChange);
}
/**
* Chip row for the task-create dialog. Renders one chip per row in
@@ -197,6 +229,7 @@ function ChipsList({
isLocalExecutor,
rowBranch: row.branch,
currentLocalBranch: fs.currentLocalBranch,
freshBranchEnabled: !!fs.freshBranchEnabled,
})}
onRepositoryChange={(value) => onRowRepositoryChange(row.key, value)}
onBranchChange={(value) => onRowBranchChange(row.key, value)}
@@ -245,7 +278,11 @@ function FreshBranchToggle({
onClick={() => onToggle(!enabled)}
data-testid="fresh-branch-toggle"
aria-pressed={enabled}
aria-label={enabled ? "Fork a new branch" : "Use current branch"}
aria-label={
enabled
? "Fork a new branch from a base (turn off to use current checkout)"
: "Fork a new branch from a base instead of using current checkout"
}
className={cn(
"inline-flex h-7 w-7 items-center justify-center rounded-md border border-input cursor-pointer transition-colors",
enabled
@@ -256,10 +293,10 @@ function FreshBranchToggle({
<IconGitFork className="h-3.5 w-3.5" />
</button>
</TooltipTrigger>
<TooltipContent>
<TooltipContent className="max-w-xs">
{enabled
? "Forking a new branch from the selected base. Click to use the existing branch instead."
: "Use the existing branch. Click to fork a new branch from a base instead."}
? "Fork mode: a new branch will be created from the selected base before the agent runs. Click to turn off and use your repository's current checkout instead."
: "By default the local executor uses your repository's current checkout. Click to fork a new branch from a base instead, leaving your working tree untouched."}
</TooltipContent>
</Tooltip>
);
@@ -298,12 +335,15 @@ type RepoChipProps = {
*/
branchLocked?: boolean;
/**
* When set, prefer this branch as the auto-selected default for an empty
* row.branch. Used by the local-executor flow to surface the workspace's
* current branch as the chip's default value (so submit always carries
* an explicit branch and the user sees what's on disk). Falls back to the
* existing last-used / preferred-default autoselect when unset or when
* the value isn't in the loaded branch list.
* When set, seed row.branch with this value (for an empty row). Used by
* the local-executor flow to surface the workspace's current ref — either
* a branch name like "main" or, on detached HEAD, the short commit SHA
* returned by the backend. The chip displays it verbatim ("current: main"
* or "current: 4fbc5d7"); on submit the backend's skip-when-equal check
* matches the same SHA so it's a no-op.
*
* When unset, the chip falls back to the existing last-used / preferred-
* default autoselect (main / master / develop, etc.).
*/
preferredDefaultBranch?: string;
/**
@@ -403,18 +443,12 @@ function useRepoChipData({
// so the current-branch preference gets first crack at row.branch.
useEffect(() => {
if (!branchSource || branchesLoading || branches.length === 0 || row.branch) return;
if (preferredDefaultBranchLoading) return;
if (
preferredDefaultBranch &&
branches.some((b) => {
const displayName = b.type === "remote" && b.remote ? `${b.remote}/${b.name}` : b.name;
return displayName === preferredDefaultBranch || b.name === preferredDefaultBranch;
})
) {
onBranchChange(preferredDefaultBranch);
return;
}
autoSelectBranch(branches, onBranchChange);
runAutoselect({
branches,
preferredDefaultBranch,
preferredDefaultBranchLoading: !!preferredDefaultBranchLoading,
onBranchChange,
});
}, [
branchSource,
branchesLoading,
@@ -531,7 +565,7 @@ function RepoChip({
searchPlaceholder="Search branches..."
emptyMessage="No branches"
testId="branch-chip-trigger"
tooltip="Base branch"
tooltip={computeBranchTooltip(branchPrefix)}
onRefresh={refreshBranches}
refreshing={branchesLoading}
filter={scoreBranch}
@@ -555,54 +589,6 @@ function RepoChip({
);
}
/**
* Decide the muted text shown before the branch value to qualify intent.
* Local executor: "current: " when the user kept the workspace's current
* branch, "will switch to: " when they picked something different — surfaces
* the destructive `git checkout` decision the backend will make on submit.
* Non-local executors: "from: " — the picked branch is the base for the new
* worktree branch, not a switch target.
*
* Returns "" when there's no value yet, so the chip's placeholder ("branch")
* doesn't get a misleading prefix.
*/
export function computeBranchPrefix({
isLocalExecutor,
rowBranch,
currentLocalBranch,
}: {
isLocalExecutor: boolean;
rowBranch: string;
currentLocalBranch: string;
}): string {
if (!rowBranch) return "";
if (isLocalExecutor) {
if (currentLocalBranch && rowBranch === currentLocalBranch) return "current: ";
return "will switch to: ";
}
return "from: ";
}
function computeBranchDisabledReason({
branchLocked,
hasRepo,
branchesLoading,
optionCount,
}: {
branchLocked: boolean;
hasRepo: boolean;
branchesLoading: boolean;
optionCount: number;
}): string | undefined {
if (branchLocked) {
return "The local executor uses your repository's current checkout, so the branch can't change here. Toggle 'Fork a new branch' to pick a different base.";
}
if (!hasRepo) return "Select a repository first.";
if (branchesLoading) return "Loading branches…";
if (optionCount === 0) return "No branches available for this repository.";
return undefined;
}
function normalizeRepoPath(path: string): string {
return path.replace(/\\/g, "/").replace(/\/+$/g, "");
}