From 0e8b6575a854454d3258b0da16bd4555e8bb25b9 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Thu, 3 Sep 2026 23:25:23 -0600 Subject: [PATCH] Fix create-agent git option availability --- .../components/app/create-agent-dialog.tsx | 1 - .../create-agent-worktree-section.test.tsx | 86 +++++++++++++++ .../app/create-agent-worktree-section.tsx | 35 +++--- .../app/use-create-agent-form.test.tsx | 103 +++++++----------- .../components/app/use-create-agent-form.ts | 100 ++++++----------- 5 files changed, 171 insertions(+), 154 deletions(-) create mode 100644 apps/web/src/components/app/create-agent-worktree-section.test.tsx diff --git a/apps/web/src/components/app/create-agent-dialog.tsx b/apps/web/src/components/app/create-agent-dialog.tsx index 135bb847f..ef89bbbb7 100644 --- a/apps/web/src/components/app/create-agent-dialog.tsx +++ b/apps/web/src/components/app/create-agent-dialog.tsx @@ -164,7 +164,6 @@ function CreateAgentDialogContent({ { + it("renders saved true values unchecked and disabled outside a git repo", () => { + render(); + + const worktree = checkbox("create-agent-worktree"); + const newBranch = checkbox("create-agent-new-branch"); + + expect(worktree.getAttribute("aria-checked")).toBe("false"); + expect(worktree.disabled).toBe(true); + expect(newBranch.getAttribute("aria-checked")).toBe("false"); + expect(newBranch.disabled).toBe(true); + expect( + (screen.getByTestId("create-agent-base-branch") as HTMLButtonElement) + .disabled + ).toBe(true); + expect( + (screen.getByTestId("create-agent-worktree-branch") as HTMLInputElement) + .disabled + ).toBe(true); + }); + + it("uses saved values when the cwd is a git repo", () => { + render(); + + const worktree = checkbox("create-agent-worktree"); + const newBranch = checkbox("create-agent-new-branch"); + + expect(worktree.getAttribute("aria-checked")).toBe("true"); + expect(worktree.disabled).toBe(false); + expect(newBranch.getAttribute("aria-checked")).toBe("true"); + expect(newBranch.disabled).toBe(false); + expect( + (screen.getByTestId("create-agent-base-branch") as HTMLButtonElement) + .disabled + ).toBe(false); + expect( + (screen.getByTestId("create-agent-worktree-branch") as HTMLInputElement) + .disabled + ).toBe(false); + }); + + it("renders branch controls unchecked and disabled when worktrees are off", () => { + render(); + + const worktree = checkbox("create-agent-worktree"); + const newBranch = checkbox("create-agent-new-branch"); + + expect(worktree.getAttribute("aria-checked")).toBe("false"); + expect(worktree.disabled).toBe(false); + expect(newBranch.getAttribute("aria-checked")).toBe("false"); + expect(newBranch.disabled).toBe(true); + expect( + (screen.getByTestId("create-agent-base-branch") as HTMLButtonElement) + .disabled + ).toBe(true); + expect( + (screen.getByTestId("create-agent-worktree-branch") as HTMLInputElement) + .disabled + ).toBe(true); + }); +}); diff --git a/apps/web/src/components/app/create-agent-worktree-section.tsx b/apps/web/src/components/app/create-agent-worktree-section.tsx index 8344d6428..534a474be 100644 --- a/apps/web/src/components/app/create-agent-worktree-section.tsx +++ b/apps/web/src/components/app/create-agent-worktree-section.tsx @@ -8,7 +8,6 @@ import { cn } from "@/lib/utils"; type WorktreeSectionProps = { cwd: string; worktreeAvailable: boolean; - worktreeChecked: boolean; useWorktree: boolean; onUseWorktreeChange: (value: boolean) => void; baseBranch: string; @@ -22,7 +21,6 @@ type WorktreeSectionProps = { export function WorktreeSection({ cwd, worktreeAvailable, - worktreeChecked, useWorktree, onUseWorktreeChange, baseBranch, @@ -32,9 +30,9 @@ export function WorktreeSection({ createNewBranch, onCreateNewBranchChange, }: WorktreeSectionProps): JSX.Element { - const controlsDisabled = !worktreeAvailable || !worktreeChecked; - const branchOptionsEnabled = worktreeAvailable && worktreeChecked; - const newBranchChecked = branchOptionsEnabled && createNewBranch; + const worktreeChecked = worktreeAvailable && useWorktree; + const branchControlsEnabled = worktreeChecked; + const newBranchChecked = branchControlsEnabled && createNewBranch; return (
{ - const nextUseWorktree = !useWorktree; - onUseWorktreeChange(nextUseWorktree); - }} + onCheckedChange={(checked) => onUseWorktreeChange(checked === true)} disabled={!worktreeAvailable} className="mt-0.5" title={ @@ -78,10 +73,10 @@ export function WorktreeSection({
diff --git a/apps/web/src/components/app/use-create-agent-form.test.tsx b/apps/web/src/components/app/use-create-agent-form.test.tsx index 223d7c392..865683631 100644 --- a/apps/web/src/components/app/use-create-agent-form.test.tsx +++ b/apps/web/src/components/app/use-create-agent-form.test.tsx @@ -99,10 +99,6 @@ function submitEvent(): FormEvent { // (apps/v1/system/path-info's {exists, isDirectory, isGitRepo} shape). const REPO_INFO = { exists: true, isDirectory: true, isGitRepo: true }; const NON_REPO_DIR_INFO = { exists: true, isDirectory: true, isGitRepo: false }; -// A path that doesn't exist yet also resolves isGitRepo: false — distinct -// from NON_REPO_DIR_INFO because a half-typed path shouldn't be treated as -// a deliberate "this is not a repo" signal. -const NOT_FOUND_INFO = { exists: false, isDirectory: false, isGitRepo: false }; /** The POST /api/v1/agents call, or undefined if none was made. */ function agentsPost(): [string, RequestInit] | undefined { @@ -360,18 +356,20 @@ describe("handleSubmit", () => { expect(result.current.creating).toBe(false); }); - it("clears the new branch preference when worktree creation is disabled", async () => { + it("keeps the new branch preference when worktree creation is disabled", async () => { const { result } = await setup(); + act(() => result.current.handlePathInfoChange(REPO_INFO)); expect(result.current.createNewBranch).toBe(true); act(() => result.current.setCreateUseWorktree(false)); expect(result.current.createUseWorktree).toBe(false); - expect(result.current.createNewBranch).toBe(false); + expect(result.current.createNewBranch).toBe(true); }); it("sends the default JSON payload and omits context-only fields on the config step", async () => { const { result } = await setup(); + act(() => result.current.handlePathInfoChange(REPO_INFO)); act(() => result.current.setCreateName(" my agent ")); // Prompt typed but still on the config step — must not be sent. act(() => result.current.setInitialPrompt("draft prompt")); @@ -418,6 +416,7 @@ describe("handleSubmit", () => { it("drops the worktree branch when not creating a new branch", async () => { const { result } = await setup(); + act(() => result.current.handlePathInfoChange(REPO_INFO)); act(() => result.current.setCreateNewBranch(false)); act(() => result.current.setCreateWorktreeBranch("feat/x")); @@ -445,6 +444,7 @@ describe("handleSubmit", () => { it("switches to FormData when context files or links exist, skipping empty fields", async () => { const img = file("shot.png", "image/png"); const { result } = await setup(); + act(() => result.current.handlePathInfoChange(REPO_INFO)); act(() => result.current.enterContextStep()); act(() => result.current.appendStartupFiles([img])); act(() => result.current.handleAddLink("https://example.com")); @@ -538,103 +538,74 @@ describe("handleSubmit", () => { }); describe("worktree checkbox state vs. cwd repo-ness", () => { - it("forces the worktree checkbox off once a previously-available repo becomes a confirmed non-repo directory", async () => { + it("keeps saved option state untouched while repo availability is unknown", async () => { const { result } = await setup(); - act(() => result.current.handlePathInfoChange(REPO_INFO)); + + expect(result.current.worktreeAvailable).toBe(false); expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); + act(() => result.current.handlePathInfoChange(null)); - expect(result.current.createUseWorktree).toBe(false); - expect(result.current.worktreeChecked).toBe(false); + expect(result.current.worktreeAvailable).toBe(false); + expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); }); - it("does not spuriously re-check once the cwd becomes a repo again", async () => { + it("marks worktrees available only after the current cwd is confirmed as a repo", async () => { const { result } = await setup(); - act(() => result.current.handlePathInfoChange(REPO_INFO)); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); - expect(result.current.createUseWorktree).toBe(false); - act(() => result.current.handlePathInfoChange(REPO_INFO)); expect(result.current.worktreeAvailable).toBe(true); - expect(result.current.createUseWorktree).toBe(false); - expect(result.current.worktreeChecked).toBe(false); - }); - - it("leaves the preference untouched while repo-ness is still unknown", async () => { - const { result } = await setup(); - - // handlePathInfoChange(null) is what PathInput sends while a debounced - // validation is in flight — must not be treated as "confirmed not a repo". - act(() => result.current.handlePathInfoChange(null)); - expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); }); - it("leaves the untouched default alone when the dialog opens on a non-repo cwd", async () => { - // No repo has ever been available yet (e.g. the dialog's default cwd is - // the user's home directory) — the useState(true) default must survive, - // not read as though the user explicitly unchecked it. + it("does not change either saved option when the current cwd is not a repo", async () => { const { result } = await setup(); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); + expect(result.current.worktreeAvailable).toBe(false); expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); + expect( + window.localStorage.getItem("dispatch:createNewBranch:/repo/app") + ).toBeNull(); }); - it("does not reset for a path that merely doesn't exist yet, even after a prior available repo", async () => { - const { result } = await setup(); - act(() => result.current.handlePathInfoChange(REPO_INFO)); - - // A half-typed path (mid-Tab-completion, or a pause between keystrokes) - // resolves isGitRepo: false too, via exists: false — must not clobber - // the user's choice while they're still typing toward a real path. - act(() => result.current.handlePathInfoChange(NOT_FOUND_INFO)); - - expect(result.current.createUseWorktree).toBe(true); - }); - - it("still forces off on a confirmed non-repo dir reached via an intermediate not-found path", async () => { + it("keeps option state through repo availability changes", async () => { const { result } = await setup(); act(() => result.current.handlePathInfoChange(REPO_INFO)); - act(() => result.current.handlePathInfoChange(NOT_FOUND_INFO)); - expect(result.current.createUseWorktree).toBe(true); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); + expect(result.current.worktreeAvailable).toBe(false); + expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); - expect(result.current.createUseWorktree).toBe(false); - }); - - it("never touches the per-cwd createNewBranch preference directly", async () => { - // createNewBranch's checked state cascades from worktreeChecked in the - // UI (create-agent-worktree-section.tsx), so the reset effect doesn't - // need to — and must not — write through to its own per-cwd atom. - const { result } = await setup(); act(() => result.current.handlePathInfoChange(REPO_INFO)); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); + expect(result.current.worktreeAvailable).toBe(true); + expect(result.current.createUseWorktree).toBe(true); expect(result.current.createNewBranch).toBe(true); - expect( - window.localStorage.getItem("dispatch:createNewBranch:/repo/app") - ).toBeNull(); }); - it("doesn't clobber a different cwd's saved createNewBranch pref on switch", async () => { - // A previously-visited repo with its own saved "create new branch" pref. + it("does not apply stale validation to a new cwd", async () => { window.localStorage.setItem("dispatch:createNewBranch:/repo/other", "true"); const { result } = await setup(); act(() => result.current.handlePathInfoChange(REPO_INFO)); - act(() => result.current.handlePathInfoChange(NON_REPO_DIR_INFO)); - // Switching to the other repo must not stomp its independently-saved - // preference — a standing guard against a cwd-keyed setter creeping - // back into the reset effect's deps. act(() => result.current.setCreateCwd("/repo/other")); + expect(result.current.worktreeAvailable).toBe(false); + expect(result.current.createUseWorktree).toBe(true); expect(result.current.createNewBranch).toBe(true); expect( window.localStorage.getItem("dispatch:createNewBranch:/repo/other") ).toBe("true"); + + act(() => result.current.handlePathInfoChange(REPO_INFO)); + + expect(result.current.worktreeAvailable).toBe(true); + expect(result.current.createUseWorktree).toBe(true); + expect(result.current.createNewBranch).toBe(true); }); }); diff --git a/apps/web/src/components/app/use-create-agent-form.ts b/apps/web/src/components/app/use-create-agent-form.ts index 84e110e6c..e87fe0fcd 100644 --- a/apps/web/src/components/app/use-create-agent-form.ts +++ b/apps/web/src/components/app/use-create-agent-form.ts @@ -36,11 +36,13 @@ type UseCreateAgentFormOptions = { }; type CwdPathInfo = { - exists: boolean; - isDirectory: boolean; isGitRepo: boolean; }; +type ValidatedCwd = CwdPathInfo & { + cwd: string; +}; + export function useCreateAgentForm({ enabledAgentTypes, initialAgentType, @@ -65,11 +67,8 @@ export function useCreateAgentForm({ ); const [createUseWorktree, setCreateUseWorktree] = useState(true); const [createWorktreeBranch, setCreateWorktreeBranch] = useState(""); - const [cwdPathInfo, setCwdPathInfo] = useState(null); - const cwdPathInfoRef = useRef(null); - // Flips true the first time the cwd is confirmed a git repo, and never - // resets — see the reset effect below for why. - const hasHadAvailableWorktreeRef = useRef(false); + const [validatedCwd, setValidatedCwd] = useState(null); + const validatedCwdRef = useRef(null); const [initialPrompt, setInitialPrompt] = useState(""); const { startupFiles, @@ -110,14 +109,6 @@ export function useCreateAgentForm({ loaded: modelCatalogLoaded, } = useAgentModelCatalog(createType); - const handleCreateUseWorktreeChange = useCallback( - (useWorktree: boolean) => { - setCreateUseWorktree(useWorktree); - if (!useWorktree) setCreateNewBranch(false); - }, - [setCreateNewBranch] - ); - useEffect(() => { if (!modelCatalogLoaded) return; // Anything stored that the catalog no longer offers (a retired id, or an @@ -134,43 +125,6 @@ export function useCreateAgentForm({ setCreateWorktreeBranch(""); }, [createCwd]); - const cwdIsGitRepo = cwdPathInfo ? cwdPathInfo.isGitRepo : null; - // A settled result for a path that doesn't exist yet (a half-typed path, - // mid-Tab-completion) also reports isGitRepo: false — that's not the same - // as the user actually pointing at a real non-repo directory, and acting - // on it would clobber their checkbox choice on every debounce tick while - // they're still typing toward a repo. Only a fully resolved, existing, - // non-repo directory counts. - const cwdConfirmedNonRepoDirectory = - cwdPathInfo !== null && - cwdPathInfo.exists && - cwdPathInfo.isDirectory && - !cwdPathInfo.isGitRepo; - - // The submit path already guards against sending useWorktree for a - // non-repo cwd (see handleSubmit's submitUseWorktree), and the derived - // worktreeChecked/worktreeAvailable below already keep the checkbox - // *rendering* unchecked while disabled. But without this, the underlying - // preference stays true, so flipping back to a repo dir would silently - // re-check it on its own. Force the actual state off once the cwd is - // confirmed to be a real, existing non-repo directory — but only after - // the cwd has been an available repo at least once, so opening the dialog - // on a non-repo default cwd (home dir, no last-used project) doesn't wipe - // the untouched useState(true) default before the user has touched - // anything. createNewBranch doesn't need a matching reset: its own - // checked state already cascades from worktreeChecked in - // create-agent-worktree-section.tsx, so forcing this off already hides it. - useEffect(() => { - if (cwdIsGitRepo === true) { - hasHadAvailableWorktreeRef.current = true; - } else if ( - cwdConfirmedNonRepoDirectory && - hasHadAvailableWorktreeRef.current - ) { - setCreateUseWorktree(false); - } - }, [cwdIsGitRepo, cwdConfirmedNonRepoDirectory]); - const { data: systemDefaults, isError: systemDefaultsError } = useSystemDefaults(); useEffect(() => { @@ -200,10 +154,20 @@ export function useCreateAgentForm({ } }, [setDraggingFiles, step]); - const handlePathInfoChange = useCallback((info: CwdPathInfo | null) => { - cwdPathInfoRef.current = info; - setCwdPathInfo(info); - }, []); + const handlePathInfoChange = useCallback( + (info: CwdPathInfo | null) => { + const nextValidatedCwd = info + ? { cwd: createCwd.trim(), isGitRepo: info.isGitRepo } + : null; + validatedCwdRef.current = nextValidatedCwd; + setValidatedCwd(nextValidatedCwd); + }, + [createCwd] + ); + + const currentCwd = createCwd.trim(); + const worktreeAvailable = + validatedCwd?.cwd === currentCwd && validatedCwd.isGitRepo; const handleStartupPaste = useCallback( (event: ClipboardEvent) => { @@ -249,9 +213,12 @@ export function useCreateAgentForm({ setCreating(true); try { - const latestPathInfo = cwdPathInfoRef.current; + const latestPathInfo = validatedCwdRef.current; const submitUseWorktree = - latestPathInfo?.isGitRepo === false ? false : createUseWorktree; + latestPathInfo?.cwd === cwd && + latestPathInfo.isGitRepo && + createUseWorktree; + const submitCreateNewBranch = submitUseWorktree && createNewBranch; const contextInitialPrompt = step === "context" ? initialPrompt.trim() || undefined : undefined; const payloadBase = { @@ -264,11 +231,12 @@ export function useCreateAgentForm({ fullAccess: createFullAccess, autoReview: createAutoReview, useWorktree: submitUseWorktree, - createNewBranch: submitUseWorktree ? createNewBranch : undefined, - worktreeBranch: - submitUseWorktree && createNewBranch - ? createWorktreeBranch.trim() || undefined - : undefined, + createNewBranch: submitUseWorktree + ? submitCreateNewBranch + : undefined, + worktreeBranch: submitCreateNewBranch + ? createWorktreeBranch.trim() || undefined + : undefined, baseBranch: submitUseWorktree && createBaseBranch !== "main" ? createBaseBranch @@ -338,9 +306,6 @@ export function useCreateAgentForm({ ] ); - const worktreeAvailable = cwdIsGitRepo === true; - const worktreeChecked = worktreeAvailable && createUseWorktree; - return { step, setStep, @@ -352,7 +317,7 @@ export function useCreateAgentForm({ createCwd, setCreateCwd, createUseWorktree, - setCreateUseWorktree: handleCreateUseWorktreeChange, + setCreateUseWorktree, createWorktreeBranch, setCreateWorktreeBranch, initialPrompt, @@ -381,7 +346,6 @@ export function useCreateAgentForm({ cwdHistoryMetadata, removeCwdHistory, worktreeAvailable, - worktreeChecked, startupFilePreviewsRef, handlePathInfoChange, appendStartupFiles,