diff --git a/.gitignore b/.gitignore index 0413086153..3b81cf1a12 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ node_modules/ **/node_modules/ dist/ dist-preview/ +dist-flow-preview/ packages/paperclip-runner/runner/target/ ui/storybook-static/ .env diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index bcc985de3f..2c6824387f 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -117,7 +117,7 @@ test.describe("Onboarding wizard", () => { expect(pageErrors, pageErrors.join("\n")).toHaveLength(0); }); - test("connect step starts the sign-in on Connect, rather than hiring, when the signal reports no credential", async ({ + test("connect step starts the sign-in when the source is chosen, rather than hiring, when the signal reports no credential", async ({ page, }) => { const pageErrors: string[] = []; @@ -273,29 +273,32 @@ test.describe("Onboarding wizard", () => { await page.locator("#onboarding-agent-name").fill("Ada"); await page.getByRole("button", { name: "Next" }).click(); - // Step 4 (Connect a model): pick a source. Nothing to assert yet — the card - // *is* the sign-in now, so it does not exist until Connect is pressed. By - // role rather than by label, because which adapters the row offers depends - // on the registry this environment reports. + // Step 4 (Connect a model). By role rather than by label, because which + // adapters the row offers depends on the registry this environment reports. const source = page.getByRole("radio").first(); await source.waitFor({ timeout: 30_000 }); + + // Nothing before the row is answered: the card is the answer to the tile, + // and the button has nothing to do until there is a source to do it with. + const cardInstruction = page.getByText("then come back and enter authorization code"); + await expect(cardInstruction).toHaveCount(0); + await expect(page.getByRole("button", { name: "Next", exact: true })).toBeDisabled(); + + // Answering the row is what starts the sign-in — this is the ordering the + // step exists to enforce, since a hire here would file an agent with no + // credential to run on. It is also the whole of the interaction: there is + // no second press between choosing a source and being signed in. await source.click(); - const cardInstruction = page.getByText("Open Claude link then come back and enter code"); - await expect(cardInstruction).toHaveCount(0); - - // Connect starts the sign-in instead of hiring, which is the ordering the - // step exists to enforce: a hire here would file an agent with no - // credential to run on. Waited on for *enabled* rather than for visible — - // it is already on screen, and clicking a disabled button does nothing. - const connect = page.getByRole("button", { name: "Connect", exact: true }); - await expect(connect).toBeEnabled({ timeout: 30_000 }); - await connect.click(); - - await expect(cardInstruction).toBeVisible({ timeout: 15_000 }); + await expect(cardInstruction).toBeVisible({ timeout: 30_000 }); + // One destination, two ways to it: the card's own link for anyone + // finishing in another browser, and the step's button for the flow. + const cardLink = page.getByRole("link", { name: /^Sign in to / }); + await expect(cardLink).toBeVisible({ timeout: 15_000 }); + await expect(cardLink).toHaveAttribute("href", /claude\.ai\/oauth\/authorize/); await expect( - page.getByRole("link", { name: /claude\.ai\/oauth\/authorize/ }), - ).toBeVisible({ timeout: 15_000 }); + page.getByRole("button", { name: /^Sign in to / }), + ).toBeEnabled({ timeout: 15_000 }); // No "Use saved login" here: the hire step applies a stored login itself. await expect(page.getByRole("button", { name: "Use saved login" })).toHaveCount(0); expect(hireCalled).toBe(false); diff --git a/ui/src/components/AdapterLoginChrome.test.tsx b/ui/src/components/AdapterLoginChrome.test.tsx index 6d2499135c..00147d52bc 100644 --- a/ui/src/components/AdapterLoginChrome.test.tsx +++ b/ui/src/components/AdapterLoginChrome.test.tsx @@ -4,12 +4,12 @@ import { createRoot, type Root } from "react-dom/client"; import { flushSync } from "react-dom"; import { afterEach, describe, expect, it } from "vitest"; +import type React from "react"; import { OnboardingLoginCard, - OnboardingLoginCodeInput, + OnboardingCardField, onboardingCardInputClass, } from "./AdapterLoginChrome"; -import { ApiKeyField } from "./onboarding/ConnectInputCanvas"; /** * The connect step's canvas holds one of two cards, and the credential switch @@ -52,53 +52,92 @@ async function render(node: React.ReactNode): Promise { return container; } -describe("the connect step's two cards", () => { - it("gives the key field and the sign-in field the same input, from one declaration", async () => { - // The assertion that would have caught the drift this file exists for: not - // "both look like X", which passes right up until one of them is restyled, - // but that the two carry the byte-identical class string. - const signIn = await render( - - {}} onSubmit={() => {}} /> - , - ); - const keyCard = await render( - {}} />, - ); +describe("the connect step's cards", () => { + let container: HTMLDivElement; + let root: Root | null = null; - const codeInput = signIn.querySelector("input")!; - const keyInput = keyCard.querySelector("input")!; - expect(codeInput.className).toBe(onboardingCardInputClass); - expect(keyInput.className).toBe(codeInput.className); + afterEach(() => { + if (root) flushSync(() => root!.unmount()); + root = null; + document.body.innerHTML = ""; }); - it("wraps the key field in the same card shell as the sign-in", async () => { - // The shell, not just the input. The key field used to draw its own - // bordered box, so flipping the credential switch changed the shape of the - // step rather than its content. - const signIn = await render( - - {}} onSubmit={() => {}} /> - , - ); - const keyCard = await render( - {}} />, + function render(node: React.ReactNode) { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + flushSync(() => root!.render(node)); + } + + it("gives every card field the same input, from one declaration", () => { + // The step asks for three different things in this row — a browser code, a + // key — and they sit one toggle apart in the same canvas, so a divergence + // between them is visible by flipping a switch. Sharing the declaration is + // what stops that; this is the assertion that the sharing is real. + render( + <> + {}} onSubmit={() => {}} /> + {}} + onSubmit={() => {}} + /> + , ); - expect(keyCard.firstElementChild!.className).toBe(signIn.firstElementChild!.className); + const [code, key] = [...container.querySelectorAll("input")]; + expect(code!.className).toBe(onboardingCardInputClass); + expect(key!.className).toBe(code!.className); }); - it("labels the key field with the variable it will be written to", async () => { - // The name answers what a paster cannot answer for themselves — where this - // step puts the key — so it is the label rather than a sentence about it, - // and it reaches assistive tech as the field's name too. - const keyCard = await render( - {}} />, + it("masks a key and does not mask a one-time code", () => { + // A provider key is a credential that goes on living; a browser code is + // single-use and about to be pasted somewhere the customer can see. + render( + <> + {}} onSubmit={() => {}} /> + {}} + onSubmit={() => {}} + /> + , ); - expect(keyCard.textContent).toContain("ANTHROPIC_API_KEY"); - expect(keyCard.querySelector("input")!.getAttribute("aria-label")).toBe("ANTHROPIC_API_KEY"); - // Still a password field: the key is a secret even while being pasted. - expect(keyCard.querySelector("input")!.getAttribute("type")).toBe("password"); + const [code, key] = [...container.querySelectorAll("input")]; + expect(code!.getAttribute("type")).toBe("text"); + expect(code!.getAttribute("aria-label")).toBe("Authorization code"); + expect(key!.getAttribute("type")).toBe("password"); + expect(key!.getAttribute("aria-label")).toBe("API key"); + }); + + it("holds one height across the card's waiting and ready states", () => { + // The card opens on a spinner and then fills. Both states share a floor, so + // the footer below is pushed down once for one event rather than twice — + // the loaded card growing into place would be a second shove. + render( + +
+ , + ); + const waiting = container.firstElementChild!.className; + + flushSync(() => root!.unmount()); + root = null; + document.body.innerHTML = ""; + render( + + {}} onSubmit={() => {}} /> + , + ); + const ready = container.firstElementChild!.className; + + expect(waiting).toContain("min-h-(--sz-108px)"); + expect(ready).toContain("min-h-(--sz-108px)"); }); }); diff --git a/ui/src/components/AdapterLoginChrome.tsx b/ui/src/components/AdapterLoginChrome.tsx index cbf38220bd..f4dbc472f5 100644 --- a/ui/src/components/AdapterLoginChrome.tsx +++ b/ui/src/components/AdapterLoginChrome.tsx @@ -1,8 +1,17 @@ import { useEffect, useRef, useState, type ReactNode } from "react"; -import { Copy, Check } from "lucide-react"; +import { AnimatePresence, motion } from "motion/react"; +import { Copy, Check, Loader2 } from "lucide-react"; import { Button } from "./ui/button"; import { copyTextToClipboard } from "../lib/clipboard"; +import { + CARD_REVEAL_FIELD, + CARD_REVEAL_INSTRUCTION, + CARD_REVEAL_TRAVEL, + COPIED_REVEAL, + COPIED_REVEAL_DELAY_MS, + COPIED_REVEAL_TRAVEL, +} from "./onboarding/onboarding-motion"; /** * Which shell a login panel draws itself in. @@ -21,6 +30,32 @@ import { copyTextToClipboard } from "../lib/clipboard"; */ export type AdapterLoginChrome = "panel" | "onboarding"; +/** + * What the connect step calls each source. + * + * One map, read by the tile row and by the sign-in card's sentence, so the row + * and the card cannot end up calling the same provider different things. + * + * Deliberately not the display registry's label, which ten other surfaces read + * and which names the tool that runs ("Codex CLI was not found on this host"). + * Also not `ADAPTER_LOGIN_PROVIDER`, which names the account being signed in to + * — "Anthropic" is right in a settings panel listing credentials and wrong in a + * sentence that reads "Sign in to Claude". + * + * Three names for two adapters is a tension worth stating rather than hiding. + * The concepts differ — vendor, tool, account — but if the product decides + * otherwise, this is the one to delete. + */ +export const CONNECT_SOURCE_NAMES: Record = { + claude_local: "Claude", + codex_local: "OpenAI", +}; + +/** The provider name for a source, falling back to the type when unlisted. */ +export function connectSourceName(adapterType: string): string { + return CONNECT_SOURCE_NAMES[adapterType] ?? adapterType; +} + /** * The connect step's login card: an instruction with a Cancel beside it, then * the rows the customer works through. @@ -33,6 +68,7 @@ export type AdapterLoginChrome = "panel" | "onboarding"; export function OnboardingLoginCard({ instruction, onCancel, + loading = false, children, }: { /** @@ -42,10 +78,32 @@ export function OnboardingLoginCard({ */ instruction: ReactNode; onCancel?: () => void; + /** + * Show a spinner instead of the contents, at the same height. + * + * The card opens before the sign-in has anything to put in it. Rendering it + * empty and growing later would push the footer twice for one event, so both + * states share a floor — see `--sz-108px`, which is exactly the height of an + * instruction over one row. A card holding more than that (the settings + * chrome's link *and* field) still grows past it. + */ + loading?: boolean; children: ReactNode; }) { + if (loading) { + return ( +
+ +
+ ); + } + return ( -
+
{/* No `gap`: `justify-between` already holds the two apart, and the eight pixels a gap reserves are eight the instruction does not have. The longest of these strings needs the full width between the inset and @@ -64,7 +122,11 @@ export function OnboardingLoginCard({ to give and wraps on the rounding. Matching the width the design actually renders is the closer reading of it than matching a nominal size in a font it was not drawn in. */} -
+ {instruction} {onCancel && (
- {children} + + {/* A beat behind the sentence above it, so the card reads as one thing + unfolding and the instruction has been read by the time the field is + ready to be pasted into. */} + + {children} +
); } @@ -129,7 +199,7 @@ function LoginCardCopyButton({ size="icon-xs" aria-label={label} title={label} - className="size-6 shrink-0 text-muted-foreground hover:text-foreground [&_svg]:size-4" + className="size-6 shrink-0 text-muted-foreground hover:text-foreground [&_svg]:size-3" onClick={async () => { try { await copyTextToClipboard(value); @@ -147,62 +217,78 @@ function LoginCardCopyButton({ ); } -/** - * The authentication link. - * - * Underlined and an actual anchor, because the instruction above it says to - * open it — the copy button beside it is for the case where the login is being - * finished in another browser, not the primary path. It truncates rather than - * wrapping: these URLs carry a query string long enough to push the row to - * three lines, and none of that tail tells the reader anything. - */ -export function OnboardingLoginUrlRow({ url }: { url: string }) { - return ( - - - {url} - - - - ); -} - /** * The one-time code, for the login that hands one out. * - * The word beside the button rather than only the mark on it: this code is - * carried to another device, so the confirmation has to survive being read from - * a step away. + * `autoCopy` puts it on the clipboard as the card lands and says so. The code + * is going to be pasted somewhere else — that is its whole purpose — so making + * the customer press a button first is a step that exists only to be completed. + * + * The claim is made only when the write actually succeeded. A clipboard write + * needs transient user activation, and this one happens a beat after the press + * that started the sign-in, so a browser may well refuse it; "Copied!" over an + * empty clipboard would send someone to paste nothing. The button beside it is + * the path that always works, and is why the failure is quiet rather than an + * error. */ -export function OnboardingLoginCodeRow({ code }: { code: string }) { +export function OnboardingLoginCodeRow({ + code, + autoCopy = false, +}: { + code: string; + autoCopy?: boolean; +}) { const [copied, setCopied] = useState(false); const timeoutRef = useRef | null>(null); + const autoCopiedRef = useRef(false); useEffect(() => () => { if (timeoutRef.current) clearTimeout(timeoutRef.current); }, []); + useEffect(() => { + if (!autoCopy || autoCopiedRef.current) return; + autoCopiedRef.current = true; + void copyTextToClipboard(code) + .then(() => { + // Written now, said later: the clipboard should be ready the instant + // the code is readable, but the claim waits for the rest of the card to + // stop moving — see COPIED_REVEAL_DELAY_MS. + if (timeoutRef.current) clearTimeout(timeoutRef.current); + timeoutRef.current = setTimeout(() => setCopied(true), COPIED_REVEAL_DELAY_MS); + }) + .catch(() => { + // Refused, most likely for want of user activation. The button stays. + }); + }, [autoCopy, code]); + return ( - - - {code} - - {copied && copied!} +
+ {code} + + {copied && ( + + Copied! + + )} + { - setCopied(true); + // No wait here. A press is a direct action, and delaying its + // acknowledgement would read as the button having missed. if (timeoutRef.current) clearTimeout(timeoutRef.current); - timeoutRef.current = setTimeout(() => setCopied(false), 1500); + setCopied(true); }} /> - +
); } @@ -220,39 +306,63 @@ export const onboardingCardInputClass = "outline-none focus-visible:ring-ring/50 focus-visible:ring-(length:--rad-3)"; /** - * The field the browser code is pasted back into. + * The card's single-line field, whatever the card is asking for. * - * No Submit button beside it: the code arrives in one piece, off the clipboard, - * so the paste is the answer and a press after it confirms nothing the paste - * did not already say. + * Three cards use it and they want different things: a browser code pasted + * back, and an API key typed or pasted in. Same row, same measurements — what + * changes is the label, the placeholder, and whether the value should be masked. * - * `onPaste` is what the caller submits on, and it is separate from `onChange` - * on purpose. There is no shape that says "this code is complete" — - * `isValidBrowserCode` accepts any run of printable ASCII from one character up, - * deliberately, because the provider's exact format is not pinned down — so a - * submit driven by the value alone fires on the first keystroke of anyone who - * types instead of pasting. Enter stays for them. + * No Submit button beside it in the code case: the code arrives in one piece, + * off the clipboard, so the paste is the answer and a press after it confirms + * nothing the paste did not already say. + * + * `onPaste` is what that case submits on, and it is separate from `onChange` on + * purpose. There is no shape that says "this code is complete" — + * `isValidBrowserCode` accepts any run of printable ASCII from one character + * up, deliberately, because the provider's exact format is not pinned down — so + * a submit driven by the value alone fires on the first keystroke of anyone who + * types instead of pasting. Enter stays for them. A key field simply omits it: + * a key is not submitted by arriving, it is submitted by the step's own button. */ -export function OnboardingLoginCodeInput({ +export function OnboardingCardField({ value, onChange, onSubmit, onPaste, disabled, + label = "Authorization code", + placeholder = "Paste authorization code here", + masked = false, + autoFocus = false, }: { value: string; onChange: (value: string) => void; onSubmit: () => void; onPaste?: () => void; disabled?: boolean; + label?: string; + placeholder?: string; + /** A provider key is a credential; a one-time browser code is not. */ + masked?: boolean; + /** + * Take focus when the card opens. + * + * For the key card, where the field is the only thing being asked for and the + * card only opens because it was asked for. The code cards do not: their + * customer is on their way to another tab, and a caret waiting behind them is + * not where the next action is. + */ + autoFocus?: boolean; }) { return ( onChange(event.target.value)} diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 11a07b8f22..e13eefd2ba 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -36,10 +36,10 @@ import { FolderOpen, Heart, ChevronDown, X, Copy, Check, ExternalLink, Loader2, import { asBoolean, asFiniteNumber, asObject, cn } from "../lib/utils"; import { copyTextToClipboard } from "../lib/clipboard"; import { + connectSourceName, OnboardingLoginCard, - OnboardingLoginCodeInput, + OnboardingCardField, OnboardingLoginCodeRow, - OnboardingLoginUrlRow, type AdapterLoginChrome, } from "./AdapterLoginChrome"; import { @@ -2040,6 +2040,17 @@ export type AdapterLoginPanelProps = AdapterLoginDescriptor & { // it would appear on is already gone. onConnected?: () => void; chrome?: AdapterLoginChrome; + /** + * The address the customer has to open, once the server has produced one. + * + * The one fact about a running login that the step needs outside the card: + * its own button is what sends the customer there, and a prompt arriving is + * what moves the step from waiting to ready. Everything else it needs the + * panel already does — the paste submits itself, success is reported through + * `onConnected`, and an unmount releases the session — so this stays a single + * value rather than a whole session handed upward. + */ + onPromptReady?: (authorizationUrl: string | null) => void; }; // The login panel dispatcher. It picks the panel from the projected panel mode, @@ -2082,6 +2093,7 @@ function DisplayedCodeLoginPanel({ onCancel, onConnected, chrome = "panel", + onPromptReady, }: AdapterLoginPanelProps) { const [sessionId, setSessionId] = useState(null); const [startError, setStartError] = useState(null); @@ -2203,55 +2215,49 @@ function DisplayedCodeLoginPanel({ onConnectedRef.current?.(); }, [status]); + // Report the prompt's URL upward, the way the submitted-browser-code panel + // does. The caller's loading beat ends when this arrives, so without it the + // onboarding step waits on a card that has already opened: the code is on + // screen and the button stays disabled. Fires with null on mount, before the + // one-time prompt lands, which is the same null the caller starts from. + const onPromptReadyRef = useRef(onPromptReady); + onPromptReadyRef.current = onPromptReady; + useEffect(() => { + onPromptReadyRef.current?.(prompt?.url ?? null); + }, [prompt]); + const handleCancel = () => { cancelLogin.mutate(); onCancel?.(); }; if (chrome === "onboarding") { + const failed = isTerminal && status && status !== "authenticated"; return ( + {/* The same destination as the step's own button. Two ways to one + link: the button for the customer following the flow, the anchor + for anyone finishing in another browser. */} + + Sign in to {connectSourceName(adapterType)} + + {" by providing the authorization code below"} + } - onCancel={isActive ? handleCancel : undefined} > - {/* The two rows are the whole card once the prompt lands. Before it - does there is nothing to show but the wait, and after a failure - there is nothing to act on — so both of those are a line of text, - not a row. */} - {startError && ( + {startError ? (

{startError}

- )} - {isActive && !prompt && !startError && ( -

- - Preparing… -

- )} - {/* Code above link, which is the reverse of the settings panel below. - The panel numbers its two rows and its instruction reads "open the - page and enter the code", so there the order follows the sentence. - - Here the order follows the hands. The link is the last thing touched - and the first thing that takes attention away — press it and the - next screen is a device-code form in another tab, wanting the code - that was on this one. Putting the code above it means it has already - been read, and copied, before the link is there to be pressed. */} - {prompt && ( - <> - - - - )} - {isTerminal && status && status !== "authenticated" && ( + ) : failed ? (

{status === "timed_out" ? "The login timed out. Start it again." @@ -2259,6 +2265,8 @@ function DisplayedCodeLoginPanel({ ? "The login was cancelled." : "The login did not finish. Start it again."}

+ ) : ( + )} ); @@ -2424,6 +2432,7 @@ function SubmittedBrowserCodeLoginPanel({ onCancel, onConnected, chrome = "panel", + onPromptReady, }: AdapterLoginPanelProps) { const [sessionId, setSessionId] = useState(null); const [startError, setStartError] = useState(null); @@ -2799,25 +2808,35 @@ function SubmittedBrowserCodeLoginPanel({ onCancel?.(); }; + const onPromptReadyRef = useRef(onPromptReady); + onPromptReadyRef.current = onPromptReady; + useEffect(() => { + onPromptReadyRef.current?.(authorizationUrl); + }, [authorizationUrl]); + if (chrome === "onboarding") { + const failedNow = isFailure || timedOut; return ( + + Sign in to {connectSourceName(adapterType)} + + {" then come back and enter authorization code"} + } - onCancel={isActive ? handleCancel : undefined} > - {startError && ( -

- {startError} -

- )} - {/* The plain-HTTP advisory survives the redesign. It is the one thing - on this card that is not about getting the login done, and dropping - it to keep the card tidy would remove a warning about a code - travelling in clear text. */} + {/* The plain-HTTP advisory survives the redesign. It is the one thing on + this card not about getting the login done, and dropping it to keep + the card tidy would remove a warning about a code travelling in + clear text. */} {transportInsecure && (

@@ -2825,35 +2844,24 @@ function SubmittedBrowserCodeLoginPanel({ network. Continue only on a network you trust.

)} - {isActive && !authorizationUrl && !startError && ( -

- - Preparing… -

- )} - {authorizationUrl && ( - <> - - {/* Disabled while the submitted code is in flight and while the - completion read runs, so a second paste cannot land on top of a - login that is already finishing. */} - { - pastedRef.current = true; - }} - disabled={submitCode.isPending || isCompleting} - /> - - )} - {(isFailure || timedOut) && ( + {startError ? (

- {timedOut && !isFailure - ? CLAUDE_LOGIN_TIMED_OUT_MESSAGE - : CLAUDE_LOGIN_FAILED_MESSAGE} + {startError}

+ ) : failedNow ? ( +

+ {timedOut && !isFailure ? CLAUDE_LOGIN_TIMED_OUT_MESSAGE : CLAUDE_LOGIN_FAILED_MESSAGE} +

+ ) : ( + { + pastedRef.current = true; + }} + disabled={submitCode.isPending || isCompleting} + /> )} ); diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index 4c2445ccb4..e6069faba8 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -229,6 +229,7 @@ import { queryKeys } from "../lib/queryKeys"; import { ADAPTER_AUTH_MISSING_CHECK_CODE, getEnvironmentCapabilities } from "@paperclipai/shared"; import { CLAUDE_OAUTH_TOKEN_ENV_KEY } from "./environment-variables-editor/model"; import { ONBOARDING_STORAGE_KEY, OnboardingWizard } from "./OnboardingWizard"; +import { CONNECTED_HOLD_MS } from "./onboarding/onboarding-motion"; // eslint-disable-next-line @typescript-eslint/no-explicit-any (globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; @@ -650,7 +651,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( describe("hire gate: adapter authentication (claude_local, the default onboarding adapter)", () => { /** Drives the wizard to the Connect step, agent name already filled in. */ - async function openConnectStep() { + async function openConnectStep({ useApiKeys = false } = {}) { // The tile row is built from this registry, and the suite's default is // empty. That was survivable while the step preselected a source; now that // nothing is chosen until a tile is pressed, a step with no tiles is a step @@ -697,6 +698,14 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await clickByText((t) => isArcPrimary(t)); expect(document.body.textContent).toContain("Connect a model"); + // The credential mode is chosen *before* a source, because picking a + // source starts the sequence and the mode link fades out with the row — + // after that it is inert, and switching would mean changing the card out + // from under a running sign-in. + if (useApiKeys) { + await clickByText((t) => t.startsWith("Use API key")); + } + // Pick a source. The step arrives with nothing chosen — `adapterType` // carries a value for the hire, but that is not the same as the customer // having answered — so the input surface stays closed and the step will @@ -784,8 +793,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); async function connectWithApiKey() { - const handles = await openConnectStep(); - await handles.clickByText((t) => t.startsWith("Use API key")); + const handles = await openConnectStep({ useApiKeys: true }); const field = document.body.querySelector( 'input[type="password"]', ) as HTMLInputElement; @@ -899,10 +907,25 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(1); - // Switch to API keys, which changes the configuration the hire will send. + // Switching the credential mode means backing out first: the mode link + // fades away with the row once a source is chosen, and is inert after + // that, so it cannot be used to change the card out from under a running + // sign-in. Back unwinds to the question, and the answer is given again. + await clickByText((t) => t.startsWith("Back")); await clickByText((t) => t.startsWith("Use API key")); + await pickFirstSource(clickByText); + + const field = document.body.querySelector( + 'input[type="password"]', + ) as HTMLInputElement; + await act(async () => { + setControlledValue(field, "sk-ant-rekey"); + }); + await flushReact(); await clickByText((t) => isArcPrimary(t)); + // A different configuration, so the passing probe from before it cannot + // stand in for one against this one. expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(2); await act(async () => root.unmount()); @@ -2061,6 +2084,22 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await act(async () => root.unmount()); }); + /** + * Press a source tile. This is what starts the sign-in — the row is the + * question, and answering it is the whole of the trigger. Nothing is + * selected on arrival, including from a draft that names an adapter. + */ + async function pickSource(match: RegExp) { + const tile = [...document.body.querySelectorAll('[role="radio"]')].find((t) => + match.test(t.textContent ?? ""), + ); + expect(tile, "the row should offer that source").toBeTruthy(); + await act(async () => { + tile!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 10; i++) await flushReact(); + } + /** Press the step's forward button and let the sign-in queries settle. */ async function pressArcPrimary() { const cta = [...document.body.querySelectorAll("button")].find((b) => @@ -2082,23 +2121,70 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // difference between this step and the one it replaced. expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); - await pressArcPrimary(); + await pickSource(/Claude/); expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalled(); // Connect started a sign-in rather than hiring. The ordering is the point: // a hire here would create an agent with no credential to run on. expect(mockAgentsApi.hire).not.toHaveBeenCalled(); expect(document.body.textContent).toContain( - "Open Claude link then come back and enter code", + "Sign in to Claude then come back and enter authorization code", ); await act(async () => root.unmount()); }); + it("collapses the row to the chosen source and walks the button through the sign-in", async () => { + // The sequence, end to end, as the step actually runs it: the row is the + // question, answering it starts the sign-in, and the button reports where + // that has got to rather than offering an action it cannot perform. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + // Collapse is read off the row's own layout, not off how many tiles are + // in the DOM: the leaving tile exits through AnimatePresence, and in + // jsdom an exit never completes, so it stays mounted either way. + const rowCentred = () => + document.body + .querySelector('[role="radiogroup"]')! + .className.includes("justify-center"); + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + + // Nothing chosen yet on a fresh arrival: the row is a question. + expect(rowCentred()).toBe(false); + + await pickSource(/Claude/); + + // The row has been answered, so it now shows only the answer — leaving + // the alternative up would invite a press that has to cancel a live + // session to honour. + expect(rowCentred()).toBe(true); + expect(mockAgentsApi.startClaudeSetupTokenLogin).toHaveBeenCalled(); + + // And the button has become the sign-in rather than a step advance. + expect(cta()).toBe("Sign in to Claude"); + + // Back unwinds rather than leaving the step: the row is a question again. + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 10; i++) await flushReact(); + + expect(rowCentred()).toBe(false); + expect(cta()).toBe("Next"); + expect(document.body.textContent).toContain("Connect a model"); + + await act(async () => root.unmount()); + }); + it("submits the browser code on the paste, not on the first keystroke", async () => { mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); const { root } = await openStep4({ adapterType: "claude_local" }); - await pressArcPrimary(); + await pickSource(/Claude/); const field = document.body.querySelector( 'input[aria-label="Authorization code"]', @@ -2132,33 +2218,201 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await act(async () => root.unmount()); }); + it("starts the sign-in on the first press, even when it changes the adapter", async () => { + // The regression this is here for. Picking a source sets the phase *and* + // the adapter, and a reset keyed on the adapter then put the phase + // straight back — so the first press did nothing and only a second one, + // which changed no adapter and so woke no effect, was allowed to stand. + // + // It only showed when the chosen source differed from the one the draft + // carried: picking the adapter already in state is a no-op React bails + // out of, so every existing case here happened to miss it. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "unknown" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/OpenAI/); + + expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalledTimes(1); + expect( + document.body + .querySelector('[role="radiogroup"]')! + .className.includes("justify-center"), + "one press should have answered the row", + ).toBe(true); + + await act(async () => root.unmount()); + }); + + it("opens the OpenAI card's sign-in once the prompt lands", async () => { + // The panel that shows a code owns its session and the wizard learns the + // prompt's URL only by being told. When it was not told, the step waited + // on a card that had already opened: the code sat on screen under a + // button still reading "Next", disabled, with nothing left to wait for. + // + // The sibling test above stops at the login having started, which is why + // this went unseen — and the harness could not see it either, since it + // supplies the prompt itself rather than going through the panel. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "unknown" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/OpenAI/); + for (let i = 0; i < 6; i++) await flushReact(); + + // The displayed-code panel is the one under test: if the row picked a + // source the other panel serves, the rest of this proves nothing. + expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalled(); + expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); + + const cta = [...document.body.querySelectorAll("button")].pop()!; + expect(cta.textContent?.trim()).toBe("Sign in to OpenAI"); + // The code is the card's own content, and it arrives with the URL. + expect(document.body.textContent).toContain("Q2RJ-E1YIF"); + // The button is the whole point: its destination is the URL the panel + // reports, so an unreported prompt leaves it reading like an offer and + // refusing the press. The label alone does not catch that — the phase can + // reach `ready` before the signal resolves, which names the button + // without enabling it. + expect(cta.hasAttribute("disabled"), "the sign-in should be pressable").toBe( + false, + ); + + await act(async () => root.unmount()); + }); + + it("does not hire after Back interrupts the hold before step 5", async () => { + // "Connecting" is held for two seconds so it reads as a state rather than + // a flicker, and Back stays live throughout. The hire behind that hold + // knows nothing about the phase, so a timer left running took a customer + // who had just backed out to Review with an agent hired anyway. + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + // A session the server has already authenticated, so the panel completes + // and reports success on its own. The paste that normally gets it there + // is the sibling test's subject; this one is about what the success does. + mockAgentsApi.getClaudeSetupTokenLoginStatus.mockResolvedValue({ + sessionId: "claude-session-1", + status: "authenticated", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }); + const { root } = await openStep4({ adapterType: "claude_local" }); + await pickSource(/Claude/); + for (let i = 0; i < 8; i++) await flushReact(); + + const cta = () => + [...document.body.querySelectorAll("button")].pop()?.textContent?.trim(); + expect(cta(), "success should have taken the button to the hold").toBe( + "Connecting", + ); + + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 10; i++) await flushReact(); + + // Past the hold: whatever it was going to do, it has had its chance. + await act(async () => { + await new Promise((resolve) => window.setTimeout(resolve, CONNECTED_HOLD_MS + 400)); + }); + for (let i = 0; i < 6; i++) await flushReact(); + + expect(mockAgentsApi.hire).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain("Connect a model"); + + await act(async () => root.unmount()); + }); + + it("starts no login when Back interrupts the collapse", async () => { + // Backing out before the card has opened has nothing to close. Unwinding + // through the card beat regardless mounted the panel — which starts a + // server login on mount — only for the unmount to cancel it, and a cancel + // that fails holds the per-owner reservation until the server deadline, + // so the retry the customer is about to make cannot start. + // + // The beats collapse to zero without a `matchMedia` to ask, which is why + // this stubs one: the collapse has to still be running when Back lands. + const realMatchMedia = window.matchMedia; + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: (query: string) => ({ + matches: false, + media: query, + onchange: null, + addListener: () => {}, + removeListener: () => {}, + addEventListener: () => {}, + removeEventListener: () => {}, + dispatchEvent: () => false, + }), + }); + try { + mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); + const { root } = await openStep4({ adapterType: "claude_local" }); + + await pickSource(/Claude/); + // Still collapsing: the flushes above are microtasks and 0ms timers, + // far inside the collapse's own duration. + expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); + + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.trim().startsWith("Back"), + ); + await act(async () => { + back!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 10; i++) await flushReact(); + await act(async () => { + await new Promise((resolve) => window.setTimeout(resolve, 1200)); + }); + for (let i = 0; i < 6; i++) await flushReact(); + + expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain("Connect a model"); + + await act(async () => root.unmount()); + } finally { + Object.defineProperty(window, "matchMedia", { + configurable: true, + writable: true, + value: realMatchMedia, + }); + } + }); + it("starts the codex_local sign-in on Connect when the signal cannot decide", async () => { mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "unknown" }); const { root } = await openStep4({ adapterType: "codex_local" }); expect(mockAgentsApi.startAdapterAuthLogin).not.toHaveBeenCalled(); - await pressArcPrimary(); + await pickSource(/OpenAI/); expect(mockAgentsApi.startAdapterAuthLogin).toHaveBeenCalled(); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); // The displayed-code login hands a code over rather than taking one back, // so both halves of it have to reach the screen. - expect(document.body.textContent).toContain("Copy this code then open the authentication link"); + expect(document.body.textContent).toContain( + "Sign in to OpenAI by providing the authorization code below", + ); expect(document.body.textContent).toContain("Q2RJ-E1YIF"); - // Code above link, deliberately. Pressing the link is what takes the - // customer to another tab, and it wants the code that was on this one — - // so the code has to have been read before the link is there to press. + // The link lives inside the sentence now rather than in a row of its own, + // and points at the same address the step's button opens — it is the + // path for anyone finishing the sign-in in another browser. + const link = document.body.querySelector('a[href*="auth.openai.com"]'); + expect(link, "the sign-in link should be in the instruction").toBeTruthy(); + expect(link!.textContent).toBe("Sign in to OpenAI"); + + // And the code sits below the sentence carrying it. const code = [...document.body.querySelectorAll("span")].find( (el) => el.textContent?.trim() === "Q2RJ-E1YIF", ); - const link = document.body.querySelector('a[href*="auth.openai.com"]'); expect(code, "the code row should render").toBeTruthy(); - expect(link, "the link row should render").toBeTruthy(); expect( - code!.compareDocumentPosition(link!) & Node.DOCUMENT_POSITION_FOLLOWING, - "the code should come before the link", + link!.compareDocumentPosition(code!) & Node.DOCUMENT_POSITION_FOLLOWING, + "the code should follow the sentence that links to the sign-in", ).toBeTruthy(); await act(async () => root.unmount()); @@ -2168,6 +2422,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "present" }); const { root } = await openStep4({ adapterType: "claude_local" }); + // Answer the row, then press: with a credential already in place there is + // no sign-in to run, so the button goes straight to the hire. + await pickSource(/Claude/); await pressArcPrimary(); // The positive half is what makes this a test: a source that is already diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index f9f092d792..830ee6f0d0 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -11,6 +11,38 @@ import type { } from "@paperclipai/shared"; import { AGENT_ROLES, AGENT_ROLE_LABELS, ADAPTER_AUTH_MISSING_CHECK_CODE } from "@paperclipai/shared"; import { AdapterLoginPanel } from "./AgentConfigForm"; +import { + CONNECT_SOURCE_NAMES, + OnboardingCardField, + OnboardingLoginCard, +} from "./AdapterLoginChrome"; +import { + beatDelay, + CARD_ENTER, + CARD_EXIT, + CARD_EXIT_MS, + CONNECTED_HOLD_MS, + MAKE_ROOM, + MAKE_ROOM_MS, + SOURCE_COLLAPSE_MS, + SOURCE_LINK_EXIT, +} from "./onboarding/onboarding-motion"; + +/** + * Where the connect step's sign-in sequence is. Space and visibility land on + * different beats, which is why there are more of these than there are things + * on screen — see the derived state in the step itself. + */ +type ConnectPhase = + | "idle" + | "collapsing" + | "loading" + | "ready" + | "waiting" + | "connecting" + | "unwindCard" + | "unwindRoom" + | "unwindRow"; import { secretsApi } from "../api/secrets"; import { Label } from "./ui/label"; import { Input } from "./ui/input"; @@ -96,8 +128,7 @@ import { import { AgentPreview } from "./onboarding/AgentPreview"; import { ModelSourceTiles, type CredentialMode } from "./onboarding/ModelSourceTiles"; import { CredentialModeLink } from "./onboarding/CredentialModeLink"; -import { ApiKeyField, ConnectInputCanvas } from "./onboarding/ConnectInputCanvas"; -import { FooterNav } from "./onboarding/FooterNav"; +import { FooterNav, type FooterPrimaryIcon } from "./onboarding/FooterNav"; import { OnboardingHeading } from "./onboarding/OnboardingPrimitives"; import { DEFAULT_AGENT_ROLE } from "../lib/onboarding-agent-role"; import { capsuleHeroMotion } from "./onboarding/onboarding-motion"; @@ -203,25 +234,6 @@ const MODEL_SOURCE_BRAND_MARKS: Record = { claude_local: "/brands/claude-color.svg", }; -/** - * What the connect step calls each source. - * - * Deliberately not the display registry's label, which ten other surfaces read. - * This step asks which *provider* you are signing in with — the panel under the - * row says "Sign in to Anthropic" and "Sign in to OpenAI" — while the agent - * config screens name the tool that runs ("Codex CLI was not found on this - * host"). One rename in the registry would make that message say OpenAI, which - * is vaguer, not clearer. - * - * It is a tension worth naming rather than hiding: DESIGN.md asks for one name - * per concept, and this is two names for one adapter. The concepts are - * different — vendor here, tool there — but if the product decides otherwise, - * this map is the thing to delete. - */ -const MODEL_SOURCE_NAMES: Record = { - claude_local: "Claude", - codex_local: "OpenAI", -}; /** * OpenAI's blossom, inline rather than served from `/brands`. @@ -629,12 +641,19 @@ function OnboardingWizardInner({ * defaulted. A customer who never touched the row could reach the end of the * step having chosen nothing. * - * Restored true when the draft names a source. Someone returning here has - * already answered, and asking again would throw that answer away. + * Always false on arrival, including from a draft that names a source. + * + * It used to restore, on the reasoning that someone returning had already + * answered and asking again threw that answer away. Picking a source is what + * starts the sign-in now, so a restored selection is not an answer the step + * can act on — it is a lit tile with nothing behind it, and the sequence has + * no way to begin from there without either starting a server session + * unbidden or leaving the button to do a job the row is supposed to do. + * + * `adapterType` still restores; it is what the hire needs. This is only about + * whether the row has been *answered* on this visit. */ - const [sourcePicked, setSourcePicked] = useState( - () => typeof saved?.adapterType === "string" && saved.adapterType.length > 0, - ); + const [sourcePicked, setSourcePicked] = useState(false); const savedNativeRunnerDraft = saved?.adapterType === "paperclip_runner"; const [cwd, setCwd] = useState((saved?.cwd as string) ?? ""); // Native drafts may carry provider-specific configuration that is invalid @@ -671,26 +690,27 @@ function OnboardingWizardInner({ (saved?.credentialMode as CredentialMode) ?? "subscription", ); /** - * Whether Connect has been pressed for the current source. + * Where the connect step's sign-in sequence is. * - * The step's footer button is what starts the sign-in now, so this is the - * whole of the difference between the card being absent and the card running: - * the panel is mounted with `autoStart` the moment this is true, and it - * cancels back to false. Deliberately not in the draft — a login is a live - * server session with a deadline on it, and restoring a wizard an hour later - * into "connecting" would be describing a session that is long gone. + * Picking a source starts it now, rather than a press of the footer button: + * the row collapses, the card opens, and the button becomes the sign-in. The + * beats are ordered rather than concurrent, and each waits for the animation + * before it — see `onboarding-motion`, where the durations these timers use + * live beside the transitions they mirror. + * + * Deliberately not in the draft. A login is a live server session with a + * deadline on it, and restoring a wizard an hour later into "waiting for a + * code" would describe a session that is long gone. */ - const [loginStarted, setLoginStarted] = useState(false); + const [connectPhase, setConnectPhase] = useState("idle"); /** - * Whether that sign-in reached its success state. + * The address the running login wants the customer to open. * - * Only the displayed-code login ever sits here to be read: the browser-code - * login advances the step from its own success, so nothing gets the chance - * to render this. It exists because OpenAI's login finishes in another tab, - * with nothing to type back — so the step has to wait, and the footer button - * is where the waiting shows. + * Reported up by the panel, because the step's own button is what opens it — + * the card shows the same link inline for anyone finishing in another + * browser. Its arrival is also what moves the step off its waiting beat. */ - const [loginConnected, setLoginConnected] = useState(false); + const [connectAuthUrl, setConnectAuthUrl] = useState(null); /** * The key itself, held only for as long as the wizard is open. It is written * into the adapter config at hire time and never into the draft — a draft is @@ -1186,9 +1206,169 @@ function OnboardingWizardInner({ const loginSubmitsBrowserCode = adapterCaps.login?.panelMode === "submitted_browser_code"; - // Connect is pressed, the login is running, and it has not succeeded yet. + /** + * The one thing that can be wrong here before anything is pressed: there is + * no sandbox to sign in against, so Connect cannot get anywhere. Worth saying + * on arrival rather than after a press that goes nowhere. + * + * Its two neighbours in the old canvas are not worth the same. "Checking this + * source's credentials…" narrated a request nothing was waiting on, and "this + * source is already signed in" answered a question the customer had not asked + * yet — both were written for a canvas that opened on selection, and the + * press is what opens it now. + */ + const connectStepHasNoSandbox = + credentialMode !== "api" && !canShowAdapterLogin && !authSignalUndecided; + + /* + The sequence's derived state. Space and visibility are separate throughout: + the credential link fades on the first beat but keeps its space until the + second, and the card takes its space on the second but only appears on the + third — so the column slides once, when there is a reason for it to. + */ + const connectCollapsed = + connectPhase !== "idle" && connectPhase !== "unwindRow" && sourceSelected; + const connectHasCard = credentialMode === "api" || connectStepNeedsLogin || connectStepHasNoSandbox; + const connectCardLive = + connectHasCard && + (connectPhase === "loading" || + connectPhase === "ready" || + connectPhase === "waiting" || + connectPhase === "connecting"); + const connectCardSpace = connectCardLive || (connectHasCard && connectPhase === "unwindCard"); + /** + * Whether the card's contents are rendered at all. + * + * A beat longer than its space, and that beat matters. The height animates + * away over `unwindRoom`, and an element with nothing in it has no height to + * animate *from* — unmounting the contents when the space starts closing + * collapsed the column in a single frame instead, a 54px jump measured right + * after the fade. They stay until the room has finished closing. + * + * It cannot simply be "always", either: the panel starts a server session on + * mount, so rendering it at idle would open an OAuth session merely because + * the step was visited. + */ + const connectCardMounted = connectCardSpace || connectPhase === "unwindRoom"; + const connectLinkSpace = + connectPhase === "idle" || + connectPhase === "collapsing" || + connectPhase === "unwindRoom" || + connectPhase === "unwindRow"; + const connectLinkVisible = connectPhase === "idle" || connectPhase === "unwindRow"; + + /** A sign-in is running and has not succeeded. */ const connectStepLoggingIn = - connectStepNeedsLogin && loginStarted && !loginConnected; + connectStepNeedsLogin && connectPhase !== "idle" && connectPhase !== "connecting"; + + /** + * The beats, each waiting for the animation before it. + * + * `loading` is the exception: it ends when the server produces a prompt, not + * on a timer, so the card waits exactly as long as the login actually takes. + */ + useEffect(() => { + if (step !== 4) return; + if (connectPhase === "collapsing") { + const t = setTimeout( + // A key has nothing to fetch — the field exists the moment the source + // is chosen — and a source already signed in has nothing to fetch + // either. Only a live sign-in spends a beat waiting for its prompt; + // sending the others through it would stall them on a card that never + // opens. + () => + setConnectPhase( + credentialMode === "api" || !connectStepNeedsLogin ? "ready" : "loading", + ), + beatDelay(SOURCE_COLLAPSE_MS), + ); + return () => clearTimeout(t); + } + if (connectPhase === "connecting") { + // No success state: the step advances. The hold is so "Connecting" is + // legible as a state rather than a flicker on the way out — a step that + // left the instant a paste landed would read as the paste having gone + // wrong. + // + // A beat rather than a bare timer because Back stays live through it. A + // dropped handle hired two seconds after the customer had backed out, + // landing them on Review having asked for the opposite; `handleGiveHeartbeat` + // has no notion of the phase and could not refuse it. Leaving the phase — + // Back, the step changing, unmount — now cancels the hire with it. + const t = setTimeout(() => void handleGiveHeartbeat(), CONNECTED_HOLD_MS); + return () => clearTimeout(t); + } + if (connectPhase === "unwindCard") { + const t = setTimeout(() => setConnectPhase("unwindRoom"), beatDelay(CARD_EXIT_MS)); + return () => clearTimeout(t); + } + if (connectPhase === "unwindRoom") { + const t = setTimeout(() => setConnectPhase("unwindRow"), beatDelay(MAKE_ROOM_MS)); + return () => clearTimeout(t); + } + if (connectPhase === "unwindRow") { + // Let the selection go as the row starts back, not once it has arrived. + // Held to the end, the tile changed colour with nothing else moving — + // a cut rather than a release. Released here it fades across the travel + // and settles into its default instead of snapping to it. The tiles take + // the slower duration while `settling`, so the fade lasts the journey. + setSourcePicked(false); + const t = setTimeout(() => setConnectPhase("idle"), beatDelay(SOURCE_COLLAPSE_MS)); + return () => clearTimeout(t); + } + return; + }, [step, connectPhase, credentialMode, connectStepNeedsLogin]); + + /** + * The button's four faces, and which of them can be pressed. + * + * It is only live where there is something for it to do: a sign-in to open, a + * key to submit, or a hire to run. Through the waits it is the step reporting + * rather than offering — see `FooterNav`, where the label cross-fades over an + * easing width so those changes read as one control rather than four. + */ + const connectSourceLabel = CONNECT_SOURCE_NAMES[adapterType] ?? adapterType; + const connectCta: { label: string; icon: FooterPrimaryIcon; disabled: boolean } = + connectPhase === "waiting" + ? { label: "Waiting for code", icon: "spinner", disabled: true } + : connectPhase === "connecting" + ? { label: "Connecting", icon: "spinner", disabled: true } + : connectPhase === "ready" + ? connectStepNeedsLogin + ? { + label: `Sign in to ${connectSourceLabel}`, + icon: "none", + disabled: !connectAuthUrl, + } + : { + label: "Connect", + icon: "arrow", + disabled: + !connectStepReady || (credentialMode === "api" && !apiKey.trim()), + } + : // Nothing is chosen on arrival, and the row is what chooses. Until + // it has been answered the button has nothing to do. + { label: "Next", icon: "arrow", disabled: true }; + + /** + * Back, on the connect step, unwinds the sign-in before it leaves the step. + * + * With no Cancel on the card this is the only way out, and what it undoes + * depends on how far in you are. Unmounting the panel is what releases the + * server session — see the release-on-unmount effect in `AdapterLoginPanel` + * — so the card leaving is the cancel, not a separate call. + */ + function unwindConnectStep() { + setConnectAuthUrl(null); + // Where the reverse starts depends on how far the sequence got. Backing out + // during the collapse has no card to close and no room to give back, and + // entering `unwindCard` regardless mounted the panel — which starts a + // server login on mount — purely so the unmount could cancel it. Should + // that cancel fail, the reservation is held to the server deadline and an + // immediate retry cannot start. With no card open, the row is the whole of + // the unwind. + setConnectPhase(connectCardLive ? "unwindCard" : "unwindRow"); + } /** * What the step's primary action does, for both the button and Cmd+Enter. @@ -1200,10 +1380,13 @@ function OnboardingWizardInner({ * it is meant to start, against a source with no credential. */ function handleConnectStepPrimary() { - if (connectStepNeedsLogin && !loginStarted) { - setLoginStarted(true); + // Mid-sequence the button belongs to the sign-in, not to the step. + if (connectPhase === "ready" && connectStepNeedsLogin) { + if (connectAuthUrl) window.open(connectAuthUrl, "_blank", "noreferrer,noopener"); + setConnectPhase("waiting"); return; } + if (connectStepLoggingIn) return; void handleGiveHeartbeat(); } @@ -1223,19 +1406,6 @@ function OnboardingWizardInner({ * unanswered, and the visible tiles are the thing to press. `showAdapterLoginPanel` * still decides what goes *inside* the canvas — only not whether it exists. */ - /** - * The one thing that can be wrong here before anything is pressed: there is - * no sandbox to sign in against, so Connect cannot get anywhere. Worth saying - * on arrival rather than after a press that goes nowhere. - * - * Its two neighbours in the old canvas are not worth the same. "Checking this - * source's credentials…" narrated a request nothing was waiting on, and "this - * source is already signed in" answered a question the customer had not asked - * yet — both were written for a canvas that opened on selection, and the - * press is what opens it now. - */ - const connectStepHasNoSandbox = - credentialMode !== "api" && !canShowAdapterLogin && !authSignalUndecided; /** * Open once there is something in it: a key field, a sign-in that has been @@ -1248,7 +1418,7 @@ function OnboardingWizardInner({ */ const canvasOpen = sourceSelected && - (credentialMode === "api" || loginStarted || connectStepHasNoSandbox); + (credentialMode === "api" || connectCardSpace || connectStepHasNoSandbox); // The default (or a saved) adapterType can name an adapter the server has // since disabled — e.g. a cloud sandbox registry without claude_local. The @@ -1320,15 +1490,26 @@ function OnboardingWizardInner({ setAdapterEnvError(null); }, [step, adapterType, model, command, args, url, credentialMode, apiKey]); - // A login belongs to one source in one credential mode. Switching either - // means the card on screen is answering a question nobody asked any more, so - // the step goes back to offering Connect. The panel is keyed on the adapter - // as well, so it unmounts on the same change and releases its server session - // on the way out. + /** + * Leaving the step puts the row back to a question. + * + * Deliberately keyed on the step and nothing else. It used to reset on + * `adapterType` too, which made picking a source take two clicks: the first + * set the phase *and* the adapter, this effect saw the adapter change and put + * the phase straight back to idle, and only a second click — which changed no + * adapter, so woke no effect — was allowed to stand. + * + * Nothing else needs to reset it. A source can only change by being picked, + * and picking sets the phase itself; the credential mode can only change + * before the sequence starts, because its control is inert once the row has + * collapsed. + */ useEffect(() => { - setLoginStarted(false); - setLoginConnected(false); - }, [adapterType, credentialMode]); + if (step === 4) return; + setConnectPhase("idle"); + setConnectAuthUrl(null); + setSourcePicked(false); + }, [step]); const selectedModel = (adapterModels ?? []).find((m) => m.id === model); const hasAnthropicApiKeyOverrideCheck = @@ -2333,7 +2514,11 @@ function OnboardingWizardInner({ // tiles stretch and the name field sits under a question far // narrower than itself. isAgentArcStep || step === 1 - ? "w-(--sz-560px) max-w-full px-8 py-10 sm:px-(--sz-64px) sm:py-11" + ? // 40px inset, not 64: the connect sequence is drawn against + // a 480px column and the arc's other steps share the shell, + // so they widen with it rather than sitting narrower than + // the step between them. + "w-(--sz-560px) max-w-full px-8 py-10 sm:px-10 sm:py-11" : "w-full max-w-md px-8 py-12", )} > @@ -2814,37 +2999,20 @@ function OnboardingWizardInner({ {/* Step 4: Connect a model — adapter + model + env check (capsule above) */} {step === 4 && (
- {/* The two cards are self-describing; an "Adapter type" - eyebrow above them named the mechanism rather than the - choice. */}
- {/* The row is `ModelSourceTiles`, the same component the - connect-step prototype is drawn with, so the shipped step - and the design under review cannot drift apart. + {/* Sources come from `recommendedAdapters`, not a list + written here — that filter is `recommended` in the + display registry, so a third tile appears the day + someone marks one rather than the day someone + remembers to edit this file. - Sources come from `recommendedAdapters`, not a list - written here. That filter is `recommended` in the display - registry, which today means Claude Code and Codex and - nothing else — so the row stays two tiles because the - registry says so, and a third would appear here the day - someone marks one rather than the day someone remembers - to edit this file. */} + Picking one starts the sign-in now. The row is the + question, and answering it is what opens the card. */} ({ id: opt.type, - // The vendor name where this step has one, the registry's - // tool name where it does not. `MODEL_SOURCE_NAMES` was - // added with the reasoning above it and then never read, - // so the row went on showing "Claude Code" and "Codex" - // — the tool names — under a heading asking which - // provider you are signing in to. - // - // The fallback is what keeps the row rendering if the - // registry ever marks a third adapter `recommended`: - // an unnamed source gets its tool name rather than - // nothing. - label: MODEL_SOURCE_NAMES[opt.type] ?? opt.label, + label: CONNECT_SOURCE_NAMES[opt.type] ?? opt.label, icon: , }))} mode={credentialMode} @@ -2854,69 +3022,112 @@ function OnboardingWizardInner({ ? adapterType : null } + collapsed={connectCollapsed} + settling={connectPhase === "unwindRow"} onSelect={(id) => { + if (connectPhase !== "idle") return; setSourcePicked(true); setAdapterType(id); - if (id === "codex_local") return; - if (id === "opencode_local") { - setModel(DEFAULT_OPENCODE_LOCAL_MODEL); - return; - } - setModel(""); + if (id === "opencode_local") setModel(DEFAULT_OPENCODE_LOCAL_MODEL); + else if (id !== "codex_local") setModel(""); + setConnectPhase("collapsing"); }} /> - {/* The credential switch stands where the adapter - disclosure used to. That disclosure existed to reach the - adapters this step does not offer, and with the row down - to the two that are supported it was a control whose - whole contents were out of scope. The question actually - left on this step is how the two are authenticated, so - that is what the line asks. - - It names the destination rather than the state, which is - what a sentence has to do where a checkbox does not — - and it is only readable because the tiles' own tags, - directly above, say where you are. */} -
- -
- + {/* Fades on the first beat but keeps its space until the + second, so pressing a tile moves nothing vertically. + Once a sign-in is running there is no switching to keys + without abandoning it, so it goes rather than sitting + there inviting a press that cannot be honoured. */} + +
+ +
+
- {/* One canvas under the tiles, holding whatever the current - choice needs: a browser-code login for Claude, a - displayed-code login for Codex, or a key field for either - when the mode is keys. Four inputs, one place — so the - Connect button below does not move every time the answer - changes. + {/* + Room first, card second. `height` opens the space — which the + link's collapse shares, so the column slides once — and the + opacity only starts once that has finished. Reversed on the + way out: fade, then give the room back. - Closed until a source is picked. `contentKey` is the - source and the mode together, because either one changing - means a different input, and that is what the canvas - swaps on. */} - - {credentialMode === "api" ? ( - - ) : showAdapterLoginPanel && + {/* + The wrapper is always rendered — that is what lets its + height animate rather than jump — but its contents are + not, and they outlive the space by a beat. See + `connectCardMounted`. + */} + {!connectCardMounted ? null : credentialMode === "api" ? ( + + handleConnectStepPrimary()} + /> + + ) : connectStepNeedsLogin && createdCompanyId && resolvedLoginEnvironmentId ? ( /* The same panel the agent configuration form shows after a test — see AdapterLoginPanel in AgentConfigForm.tsx — - in the connect step's chrome and driven by the step's - own footer button. `autoStart` is that button: mounting - only happens once Connect is pressed, so the press has - already been taken by the time the panel exists. + in the connect step's chrome. It owns the session; the + step owns the sequence around it. + + Unmounting it is the cancel: the panel releases its + server session on unmount, so Back closing the card is + what frees the owner's reservation. No "Use saved login" control: the hire step already applies a stored login on its own. */ @@ -2927,19 +3138,16 @@ function OnboardingWizardInner({ environmentId={resolvedLoginEnvironmentId} chrome="onboarding" autoStart - onCancel={() => setLoginStarted(false)} + onPromptReady={(url) => { + setConnectAuthUrl(url); + // The prompt arriving is what ends the waiting beat. + if (url) setConnectPhase((p) => (p === "loading" ? "ready" : p)); + }} onConnected={() => { - setLoginConnected(true); - // The browser-code login ends here, on this screen, - // so the step moves on by itself — there is no - // success state to sit on, and one would be a screen - // whose only content is that you may continue. - // - // The displayed-code login does not: it is still - // running in another tab when this fires, and the - // customer's attention is there. It waits for the - // press, which is what the enabled Next is for. - if (loginSubmitsBrowserCode) void handleGiveHeartbeat(); + // The hold before the step advances is the phase's own + // beat, above, so that backing out during it cancels + // the hire. + setConnectPhase("connecting"); }} onStored={() => { queryClient.invalidateQueries({ @@ -2951,22 +3159,15 @@ function OnboardingWizardInner({ }); }} /> - ) : ( - /* The canvas only opens without a panel for one reason - now — `connectStepHasNoSandbox` — and it is the reason - worth saying out loud, because it is the one that makes - Connect a dead press. - - Its two former neighbours are gone with the canvas that - opened on selection: "already signed in" reassured - against a question nobody had asked, and "checking…" - narrated a request the customer was not waiting on. - Neither survives a canvas that opens on a press. */ + ) : connectStepHasNoSandbox ? ( + /* The one thing that can be wrong here before anything is + pressed, and the one worth saying out loud: without a + sandbox there is nothing to sign in against. */

No managed sandbox is available to sign in against yet.

- )} - + ) : null} + {/* Conditional adapter fields */} {/* No model picker. Every adapter this step offers resolves @@ -3143,7 +3344,11 @@ function OnboardingWizardInner({ {(isAgentArcStep || step === 1) && ( { setOnboardingPath(null); setStep(0); @@ -3162,19 +3367,10 @@ function OnboardingWizardInner({ : step === 5 ? "Get started" : step === 4 - ? // "Connect" is the step's own verb, and it is what - // starts the sign-in rather than what follows it. - // - // It becomes "Next" for the displayed-code login - // once that is running: at that point the sign-in - // is happening somewhere else, the button is not - // the thing doing it, and offering to "Connect" a - // second time would read as a retry. - connectStepLoggingIn && !loginSubmitsBrowserCode - ? "Next" - : "Connect" + ? connectCta.label : "Next" } + primaryIcon={step === 4 ? connectCta.icon : undefined} loadingLabel={ step === 1 ? "Creating..." @@ -3187,28 +3383,16 @@ function OnboardingWizardInner({ // displayed-code login is not — see `loginSubmitsBrowserCode` // — so it stays a still, disabled Next instead of spinning // against work happening in another tab. - loading={ - step === 3 - ? false - : loading || (connectStepLoggingIn && loginSubmitsBrowserCode) - } + // Step 4 says what it is doing through `connectCta` instead: + // it has four faces and only two of them are the step working. + loading={step === 3 || step === 4 ? false : loading} primaryDisabled={ step === 1 ? !companyName.trim() || loading : step === 3 ? !agentName.trim() : step === 4 - ? // Nothing is chosen on arrival, so the step cannot - // advance until something is. Without this a customer - // could pass the model step having touched none of - // it, and be hired against whatever the draft - // happened to carry. See `connectStepReady`, which - // Cmd+Enter asks as well. - !connectStepReady || - loading || - // A sign-in is running and has not landed. Nothing - // to press until it does. - connectStepLoggingIn + ? connectCta.disabled || loading : loading || launchStateIncomplete } onPrimary={() => { diff --git a/ui/src/components/onboarding/ConnectInputCanvas.tsx b/ui/src/components/onboarding/ConnectInputCanvas.tsx deleted file mode 100644 index ea0db7f6af..0000000000 --- a/ui/src/components/onboarding/ConnectInputCanvas.tsx +++ /dev/null @@ -1,165 +0,0 @@ -import { useLayoutEffect, useRef, type ReactNode } from "react"; -import { AnimatePresence, motion } from "motion/react"; - -import { - OnboardingLoginCard, - onboardingCardInputClass, -} from "../AdapterLoginChrome"; -import { - CANVAS_CONTENT_ENTER, - CANVAS_ENTER_TRAVEL, - CANVAS_CONTENT_EXIT, - CANVAS_CONTENT_TRAVEL, -} from "./onboarding-motion"; - -/** - * The connect step's input surface: one card that holds whatever the current - * choice needs, rather than a different control appearing in a different place - * for each combination. - * - * There are four things it can hold — a browser-code login for Claude, a - * displayed-code login for Codex, and an API key field for either — and they are - * not the same shape or the same height. Giving each its own slot would move the - * Connect button every time the choice changed. One canvas that resizes keeps - * the step's furniture still and makes the card read as the answer to the tile - * above it. - * - * It is closed until a source is picked. An empty card under an untouched row of - * tiles is a box asking to be filled with nothing. - */ - -/** Three lines of body text, so a short prompt and a long one open the same card. */ -const MIN_CONTENT_HEIGHT = 66; - -export function ConnectInputCanvas({ - open, - contentKey, - children, -}: { - open: boolean; - /** - * Identity of what is inside, and what the swap animates between. The source - * and the credential mode together, because either one changing means a - * different input is needed. - */ - contentKey: string; - children: ReactNode; -}) { - if (!open) return null; - - /* - No edge and no fill of its own. Everything this holds already draws its own - surface — the login panel is a bordered, filled card, the key field a - bordered input — so a frame here was the same treatment twice, one nested a - few pixels inside the other. The canvas is a place for the input to be, not - a thing to look at. - - Which leaves the padding to the contents as well: theirs is already sized - for what they hold, and a second inset would push it off the step's measure. - - The wrapper animates its arrival and nothing else. Picking a source is what - brings this into being, so it descends into place rather than appearing - already there — the movement is what ties it to the tile just pressed. - - Opacity and transform only. An earlier version animated *height* here with - `overflow: hidden`, and stalled three separate times — once leaving the login - card rendered inside a two-pixel box, once at four percent opacity while - `open` was true throughout. The casualty each time was the OAuth URL a - customer has to click. A height that is measured once cannot hold a panel - that grows when a login starts; these two properties can, because neither - clips and neither is measured. - */ - return ( - - {/* - `popLayout`, so the leaving input is taken out of flow while it animates - and the arriving one decides the card's height on its own. The default - mode would stack them and jump the card to the sum of both mid-swap. - - Not `mode="wait"`: it will not mount the next child until the previous - reports its exit finished, that report never came here, and the swap - stalled into an instant change with no transition at all. - */} - - - {children} - - - - ); -} - -/** - * The API key field, for when the credential mode is keys rather than a - * subscription. - * - * Built to the sign-in card's shape on purpose, and that reasoning is - * unchanged from when it was written — only its target moved. These two are - * alternatives to each other: one canvas shows one or the other and the - * credential switch above trades between them, so they have to read as two - * answers to one question rather than as two different kinds of thing. It was - * matched to the old bordered panel; the connect step's sign-in became a - * borderless card with 44px rows, and this stayed behind, so flipping the - * toggle changed the shape of the step rather than its content — the exact - * failure the original note was written to prevent. - * - * It now composes the same primitives rather than restating their measurements, - * which is what keeps that from happening again. - * - * The variable name is the label rather than a sentence about it. Someone - * pasting a key knows which one they are holding; what they cannot know is - * where this step will put it, and the name answers that in the place it is - * asked. It takes the instruction slot the sign-in cards use for their - * sentence, in mono, because it is a name and not prose. - */ -export function ApiKeyField({ - envKey, - value, - onChange, -}: { - envKey: string; - value: string; - onChange: (next: string) => void; -}) { - const inputRef = useRef(null); - - // Focus on mount, because the canvas only opens when this is the thing that - // was asked for. Layout effect so it happens before paint rather than as a - // visible jump after it. - useLayoutEffect(() => { - inputRef.current?.focus(); - }, []); - - return ( - {envKey}} - > - onChange(event.target.value)} - placeholder="Paste your key" - className={onboardingCardInputClass} - /> - - ); -} diff --git a/ui/src/components/onboarding/FooterNav.tsx b/ui/src/components/onboarding/FooterNav.tsx index 5159d6a838..13ce21d497 100644 --- a/ui/src/components/onboarding/FooterNav.tsx +++ b/ui/src/components/onboarding/FooterNav.tsx @@ -1,5 +1,11 @@ +import { motion } from "motion/react"; import { ArrowLeft, ArrowRight, Loader2 } from "lucide-react"; + import { Button } from "../ui/button"; +import { CTA_LABEL_IN, CTA_WIDTH } from "./onboarding-motion"; + +/** What sits after the primary label. */ +export type FooterPrimaryIcon = "arrow" | "spinner" | "none"; /** * Shared footer for the arc's step cards: a ghost pill "Back" and a primary @@ -8,6 +14,12 @@ import { Button } from "../ui/button"; * `onBack` is optional — a run that entered on this step has nowhere behind it * to return to, and an inert Back button reads as a dead control rather than a * boundary. + * + * The primary button animates between labels rather than swapping them. The + * connect step walks it through four ("Next" → "Sign in to Claude" → "Waiting + * for code" → "Connecting"), which are very different widths, and a control + * that changes size instantly reads as a different control appearing. See + * `CTA_WIDTH` and `CTA_LABEL_*` for why the two halves are timed apart. */ export function FooterNav({ onBack, @@ -15,6 +27,7 @@ export function FooterNav({ primaryDisabled, loading, loadingLabel, + primaryIcon, onPrimary, }: { onBack?: () => void; @@ -22,8 +35,19 @@ export function FooterNav({ primaryDisabled?: boolean; loading?: boolean; loadingLabel?: string; + /** + * Defaults to the loading state's own reading — spinner while loading, arrow + * otherwise — so callers that predate this prop are unchanged. The connect + * step sets it directly, because "Sign in to Claude" carries no icon while + * "Waiting for code" carries a spinner without the step being `loading`: it + * is waiting on another tab, not working. + */ + primaryIcon?: FooterPrimaryIcon; onPrimary: () => void; }) { + const label = loading && loadingLabel ? loadingLabel : primaryLabel; + const icon: FooterPrimaryIcon = primaryIcon ?? (loading ? "spinner" : "arrow"); + return (
{onBack ? ( @@ -47,16 +71,50 @@ export function FooterNav({ ) : ( )} - + {/* + `layout` on the button and `popLayout` on its contents are what make the + width ease rather than jump: the outgoing label leaves the flow at once, + so the button's target width becomes the incoming label's, and the + layout animation carries it there while the words cross-fade in place. + + Without `popLayout` the two labels would briefly sit side by side and + the button would widen to hold both before shrinking back. + */} + + +
); } diff --git a/ui/src/components/onboarding/ModelSourceTiles.tsx b/ui/src/components/onboarding/ModelSourceTiles.tsx index 92137b53ee..3f33dce849 100644 --- a/ui/src/components/onboarding/ModelSourceTiles.tsx +++ b/ui/src/components/onboarding/ModelSourceTiles.tsx @@ -2,7 +2,13 @@ import { useRef, type ReactNode } from "react"; import { AnimatePresence, motion } from "motion/react"; import { cn } from "../../lib/utils"; -import { TAG_SWAP_ENTER, TAG_SWAP_EXIT, TAG_SWAP_TRAVEL } from "./onboarding-motion"; +import { + SOURCE_EXIT_FADE, + SOURCE_COLLAPSE_MOVE, + TAG_SWAP_ENTER, + TAG_SWAP_EXIT, + TAG_SWAP_TRAVEL, +} from "./onboarding-motion"; /** * The connect step's row of model sources, and the tag under each one saying @@ -61,12 +67,14 @@ function ModelSourceTile({ selected, onSelect, buttonRef, + settling, }: { source: ModelSource; mode: CredentialMode; selected: boolean; onSelect: () => void; buttonRef: (node: HTMLButtonElement | null) => void; + settling: boolean; }) { return (