diff --git a/apps/web/components/task-create-dialog-effects.test.ts b/apps/web/components/task-create-dialog-effects.test.ts index 1b299e87bd..a99ed5a8cd 100644 --- a/apps/web/components/task-create-dialog-effects.test.ts +++ b/apps/web/components/task-create-dialog-effects.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook } from "@testing-library/react"; import { + useAutoFillTaskNameFromPR, useBranchAutoSelectEffect, useWorkflowAgentProfileEffect, } from "./task-create-dialog-effects"; @@ -187,3 +188,132 @@ describe("useBranchAutoSelectEffect", () => { expect(fs.setGitHubBranch).not.toHaveBeenCalled(); }); }); + +const PR_1_TITLE = "PR #1: first"; + +type AutoFillFake = Pick; +function makeAutoFillFs(overrides: Partial = {}): DialogFormState { + return { + taskName: "", + setTaskName: vi.fn(), + setHasTitle: vi.fn(), + ...overrides, + } as unknown as DialogFormState; +} + +describe("useAutoFillTaskNameFromPR", () => { + it("fills the task name with 'PR #N: ' when the title is empty", () => { + const fs = makeAutoFillFs({ taskName: "" }); + const { result } = renderHook(() => useAutoFillTaskNameFromPR(fs)); + result.current(971, "feat/omp-acp-agent"); + expect(fs.setTaskName).toHaveBeenCalledWith("PR #971: feat/omp-acp-agent"); + expect(fs.setHasTitle).toHaveBeenCalledWith(true); + }); + + it("fills when the title contains only whitespace", () => { + // Regression guard: hasTitle treats whitespace-only as empty, so the + // auto-fill check must too — otherwise pasting a PR URL after typing a + // stray space would silently keep the spaces. + const fs = makeAutoFillFs({ taskName: " " }); + const { result } = renderHook(() => useAutoFillTaskNameFromPR(fs)); + result.current(7, "fix"); + expect(fs.setTaskName).toHaveBeenCalledWith("PR #7: fix"); + }); + + it("does NOT overwrite a title the user typed themselves", () => { + const fs = makeAutoFillFs({ taskName: "my custom title" }); + const { result } = renderHook(() => useAutoFillTaskNameFromPR(fs)); + result.current(42, "something"); + expect(fs.setTaskName).not.toHaveBeenCalled(); + expect(fs.setHasTitle).not.toHaveBeenCalled(); + }); + + it("replaces a previously auto-filled title when a different PR URL is pasted", () => { + // Re-paste flow: the user pastes PR #1 (autofills "PR #1: foo"), then + // realizes wrong URL and pastes PR #2. Without the lastAutoFilled-ref + // tracking, the second paste would see a non-empty title and bail. + let currentName = ""; + const fs = makeAutoFillFs({ + taskName: currentName, + setTaskName: vi.fn((v: string) => { + currentName = v; + }), + }); + const { result, rerender } = renderHook(() => { + // Re-read taskName from the closure on every render so the ref-mirror + // effect sees the latest value. + fs.taskName = currentName; + return useAutoFillTaskNameFromPR(fs); + }); + result.current(1, "first"); + expect(currentName).toBe(PR_1_TITLE); + + rerender(); + result.current(2, "second"); + expect(currentName).toBe("PR #2: second"); + expect(fs.setTaskName).toHaveBeenCalledTimes(2); + }); + + it("does NOT replace once the user edits the auto-filled title", () => { + let currentName = ""; + const fs = makeAutoFillFs({ + taskName: currentName, + setTaskName: vi.fn((v: string) => { + currentName = v; + }), + }); + const { result, rerender } = renderHook(() => { + fs.taskName = currentName; + return useAutoFillTaskNameFromPR(fs); + }); + result.current(1, "first"); + expect(currentName).toBe(PR_1_TITLE); + + // User edits the title manually. + currentName = "my edits"; + rerender(); + + result.current(2, "second"); + // Setter is still only called once (the initial auto-fill); the manual + // edit is preserved. + expect(fs.setTaskName).toHaveBeenCalledTimes(1); + expect(currentName).toBe("my edits"); + }); + + it("clears the sentinel when taskName resets to empty, preventing a re-typed auto-fill from being overwritten", () => { + // Regression guard for the mounted-across-open/close edge case: + // after a dialog reset clears taskName, the user manually types the same + // text that was previously auto-filled. A second PR paste must NOT + // overwrite it because the sentinel was cleared on the empty transition. + let currentName = ""; + const fs = makeAutoFillFs({ + taskName: currentName, + setTaskName: vi.fn((v: string) => { + currentName = v; + }), + }); + const { result, rerender } = renderHook(() => { + fs.taskName = currentName; + return useAutoFillTaskNameFromPR(fs); + }); + result.current(1, "first"); + expect(currentName).toBe(PR_1_TITLE); + + // Simulate React flushing the setTaskName state update back into the hook + // so the ref-mirror effect sees taskName = PR_1_TITLE. + rerender(); + + // Dialog resets taskName to empty (sentinel should clear). + currentName = ""; + rerender(); + + // User re-types the exact previously auto-filled value. + currentName = PR_1_TITLE; + rerender(); + + // A second paste with a different PR should NOT overwrite. + result.current(2, "second"); + expect(fs.setTaskName).toHaveBeenCalledTimes(1); // only the first auto-fill + expect(currentName).toBe(PR_1_TITLE); + }); +}); diff --git a/apps/web/components/task-create-dialog-effects.ts b/apps/web/components/task-create-dialog-effects.ts index b6d16133ef..b1f29030d5 100644 --- a/apps/web/components/task-create-dialog-effects.ts +++ b/apps/web/components/task-create-dialog-effects.ts @@ -408,6 +408,39 @@ function parseGitHubUrl(url: string): { owner: string; repo: string; prNumber?: return { owner: match[1], repo: match[2] }; } +/** + * Returns a callback that auto-fills the task name with `PR #N: <title>` when + * a PR URL is pasted, leaving anything the user typed themselves alone. The + * callback reads the latest taskName via a ref so the fetch effect doesn't + * need to list taskName as a dep (which would re-fire the branches/PR fetch on + * every keystroke in the title input). + * + * NOTE: Callers must omit the returned callback from `useEffect` dependency + * arrays — including it causes the fetch to re-fire on every keystroke, + * defeating the purpose of the ref. The call site uses + * `eslint-disable react-hooks/exhaustive-deps` intentionally. + */ +export function useAutoFillTaskNameFromPR(fs: DialogFormState) { + const { taskName, setTaskName, setHasTitle } = fs; + const lastAutoFilledTitleRef = useRef(""); + const taskNameRef = useRef(taskName); + useEffect(() => { + taskNameRef.current = taskName; + if (!taskName.trim()) { + lastAutoFilledTitleRef.current = ""; + } + }, [taskName]); + return (prNumber: number, prTitle: string) => { + const next = `PR #${prNumber}: ${prTitle}`; + const current = taskNameRef.current; + if (!current.trim() || current === lastAutoFilledTitleRef.current) { + lastAutoFilledTitleRef.current = next; + setTaskName(next); + setHasTitle(true); + } + }; +} + export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { const { useGitHubUrl, @@ -418,6 +451,7 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { setGitHubPrHeadBranch, setGitHubPrBaseBranch, } = fs; + const autoFillTitle = useAutoFillTaskNameFromPR(fs); useEffect(() => { if (!open || !useGitHubUrl) { setGitHubBranchesLoading(false); @@ -464,6 +498,7 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { if (prInfo) { setGitHubPrHeadBranch(prInfo.head_branch); setGitHubPrBaseBranch(prInfo.base_branch); + autoFillTitle(prInfo.number, prInfo.title); } }) .catch((err) => { @@ -488,6 +523,10 @@ export function useGitHubUrlBranchesEffect(fs: DialogFormState, open: boolean) { clearTimeout(timeoutId); controller.abort(); }; + // autoFillTitle is intentionally omitted: it's a fresh closure each render + // but reads the latest taskName via ref, so excluding it keeps the fetch + // from re-firing on every keystroke. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ open, useGitHubUrl,