diff --git a/tests/e2e/conference-room-typing-intro.spec.ts b/tests/e2e/conference-room-typing-intro.spec.ts index 4a9b717dc3..294aa4b0d1 100644 --- a/tests/e2e/conference-room-typing-intro.spec.ts +++ b/tests/e2e/conference-room-typing-intro.spec.ts @@ -92,10 +92,14 @@ async function runOnboardingWizard(page: Page, companyName: string) { await source.waitFor({ timeout: 30_000 }); await source.click(); - // The forward button reads "Next" here too, so wait for it to enable rather - // than for it to appear — it is already on screen, disabled, and clicking a - // disabled button raises nothing and does nothing. - const connectNext = page.getByRole("button", { name: /^Next$/ }); + // "Connect", not "Next": this step's button starts the sign-in where there + // is one to start, so it is named for what it does. Here there is none — + // this instance has no sandbox environment, so the step has no login to + // offer and Connect goes straight to the hire. + // + // Waited on for enabled rather than for visible: it is already on screen, + // disabled, and clicking a disabled button raises nothing and does nothing. + const connectNext = page.getByRole("button", { name: /^Connect$/ }); await expect(connectNext).toBeEnabled({ timeout: 30_000 }); await connectNext.click(); diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index f6db6b766d..bcc985de3f 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("adapter step shows the login panel from the cheap auth signal, and blocks the hire on a failed test", async ({ + test("connect step starts the sign-in on Connect, rather than hiring, when the signal reports no credential", async ({ page, }) => { const pageErrors: string[] = []; @@ -191,6 +191,39 @@ test.describe("Onboarding wizard", () => { }), ); + // The sign-in Connect now starts. Stubbed so the card is deterministic: + // the session start answers, and the guarded prompt read hands back an + // authorization URL for the card's link row. + await page.route("**/setup-token-login-sessions", (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + sessionId: "e2e-setup-token-session", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }), + }), + ); + await page.route("**/setup-token-login-sessions/*/prompt", (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + authorizationUrl: "https://claude.ai/oauth/authorize?code=true&client=e2e", + transportAdvisory: null, + }), + }), + ); + await page.route("**/setup-token-login-sessions/e2e-setup-token-session", (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + sessionId: "e2e-setup-token-session", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + }), + }), + ); + // Fail the adapter test the "Connect" button runs, so the hire gate // blocks the create and this test can prove no agent is hired. await page.route("**/test-environment", (route) => @@ -240,35 +273,107 @@ 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. The step arrives with nothing - // selected — the tile row is a question, not a confirmation — and the login - // panel is what the answer opens, so there is nothing to assert until one - // 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): 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. const source = page.getByRole("radio").first(); await source.waitFor({ timeout: 30_000 }); await source.click(); - // The signal above reports no ready credential, so the login panel must now - // show, with no button to reuse a saved login. - // - // The panel names the provider rather than the plumbing it runs on, so this - // title is per-adapter. "Sign in to the environment" is now only the fallback - // for an adapter with no known provider name, which claude_local is not. - await expect(page.getByText("Sign in to Anthropic")).toBeVisible({ - timeout: 15_000, - }); + 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( + page.getByRole("link", { name: /claude\.ai\/oauth\/authorize/ }), + ).toBeVisible({ 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); - // The CTA reads "Next" on this step as on the one before it. Waited on for - // *enabled* rather than for visible: it is already on screen and disabled - // until the environment probe settles, and clicking a disabled button - // raises nothing and does nothing. - const connectNext = page.getByRole("button", { name: "Next", exact: true }); - await expect(connectNext).toBeEnabled({ timeout: 30_000 }); - await connectNext.click(); + expect(pageErrors, pageErrors.join("\n")).toHaveLength(0); + }); + test("connect step blocks the hire when the environment probe fails and no sign-in is needed", async ({ + page, + }) => { + // The other half of what the test above used to cover. The two claims are + // different situations now: there, an absent credential makes Connect start + // a sign-in; here there is no sandbox to sign in against — this throwaway + // instance only auto-creates the local environment, and nothing below adds + // one — so Connect goes straight to the probe, and the probe is the gate. + const pageErrors: string[] = []; + page.on("pageerror", (err) => pageErrors.push(err.message)); - // The failed test blocks the hire and shows its own checks. + const flagRes = await page.request.patch("/api/instance/settings/experimental", { + data: { enableConferenceRoomChat: true }, + }); + expect(flagRes.ok()).toBe(true); + + await page.route("**/test-environment", (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + adapterType: "claude_local", + status: "fail", + checks: [ + { + code: "claude_cli_not_found", + level: "fail", + message: "The claude CLI was not found on this host.", + }, + ], + testedAt: new Date().toISOString(), + }), + }), + ); + + let hireCalled = false; + await page.route("**/agent-hires", (route) => { + hireCalled = true; + return route.continue(); + }); + + await page.goto("/onboarding"); + + const startBtn = page.getByRole("button", { + name: /Start Onboarding|New Organization|Add Agent/, + }); + if (await startBtn.count()) { + await startBtn.first().click(); + } + const createCard = page.getByRole("button", { name: /Build a new organization/ }); + if (await createCard.count()) { + await createCard.first().click(); + } + + await expect( + page.getByRole("heading", { name: "What is the name of your organization?" }), + ).toBeVisible({ timeout: 15_000 }); + await page.getByPlaceholder("e.g. Northwind Labs").fill(`${COMPANY_NAME}-probe-gate`); + await page.getByRole("button", { name: /^Continue/ }).click(); + + await page.waitForSelector("#onboarding-agent-name", { timeout: 30_000 }); + await page.locator("#onboarding-agent-name").fill("Ada"); + await page.getByRole("button", { name: "Next" }).click(); + + const source = page.getByRole("radio").first(); + await source.waitFor({ timeout: 30_000 }); + await source.click(); + + const connect = page.getByRole("button", { name: "Connect", exact: true }); + await expect(connect).toBeEnabled({ timeout: 30_000 }); + await connect.click(); + + // The failed probe blocks the hire and shows its own checks. await expect(page.getByText("The claude CLI was not found on this host.")).toBeVisible({ timeout: 15_000, }); diff --git a/tests/e2e/planning-mode-visual-verification.spec.ts b/tests/e2e/planning-mode-visual-verification.spec.ts index cb362f03ce..765f370f87 100644 --- a/tests/e2e/planning-mode-visual-verification.spec.ts +++ b/tests/e2e/planning-mode-visual-verification.spec.ts @@ -69,14 +69,17 @@ test("captures planning mode UI for desktop and mobile", async ({ page }) => { await page.getByRole("button", { name: /^Next$/ }).click(); // The connect step arrives with no source selected — the tile row is a - // question, not a confirmation — so its CTA, which reads "Next" here too, - // stays disabled until one is pressed. Waited on for enabled rather than - // visible: it is already on screen, and clicking a disabled button raises - // nothing and does nothing. + // question, not a confirmation — so its CTA stays disabled until one is + // pressed. It reads "Connect", not "Next": the button starts the sign-in + // where there is one to start. This instance has no sandbox environment, so + // there is none, and Connect goes straight to the hire. + // + // Waited on for enabled rather than visible: it is already on screen, and + // clicking a disabled button raises nothing and does nothing. const source = page.getByRole("radio").first(); await source.waitFor({ timeout: 30_000 }); await source.click(); - const connectNext = page.getByRole("button", { name: /^Next$/ }); + const connectNext = page.getByRole("button", { name: /^Connect$/ }); await expect(connectNext).toBeEnabled({ timeout: 30_000 }); await connectNext.click(); diff --git a/ui/connect-flow-preview.html b/ui/connect-flow-preview.html new file mode 100644 index 0000000000..aabb6d2ed0 --- /dev/null +++ b/ui/connect-flow-preview.html @@ -0,0 +1,28 @@ + + + + + + + Connect flow — Paperclip onboarding + + + + +
+ + + diff --git a/ui/src/components/AdapterLoginChrome.tsx b/ui/src/components/AdapterLoginChrome.tsx new file mode 100644 index 0000000000..23a700ab22 --- /dev/null +++ b/ui/src/components/AdapterLoginChrome.tsx @@ -0,0 +1,251 @@ +import { useEffect, useRef, useState, type ReactNode } from "react"; +import { Copy, Check } from "lucide-react"; + +import { Button } from "./ui/button"; +import { copyTextToClipboard } from "../lib/clipboard"; + +/** + * Which shell a login panel draws itself in. + * + * `panel` is the settings-side chrome the login panels have always had: a + * titled card with its own "Sign in" button, sitting under an environment test + * in the agent configuration form. It stays the default, because two surfaces + * (the agent form and the new-agent page) render the panel that way and neither + * is being redesigned here. + * + * `onboarding` is the connect step's card. The difference is not skin-deep: the + * step's own footer button starts the login, so the panel has no "Sign in" + * control of its own, and success is not something the card reports — the step + * moves on. What is left is the part the customer acts on, which is the + * instruction, the link, and the code. + */ +export type AdapterLoginChrome = "panel" | "onboarding"; + +/** + * The connect step's login card: an instruction with a Cancel beside it, then + * the rows the customer works through. + * + * The rows are the caller's, because the two login modes genuinely differ in + * the last one — Claude takes a code back, OpenAI hands one out — while + * everything above it is the same card. Passing children rather than a variant + * flag keeps that difference where it actually lives. + */ +export function OnboardingLoginCard({ + instruction, + onCancel, + children, +}: { + instruction: string; + onCancel?: () => void; + children: ReactNode; +}) { + 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 + Cancel to stay on one line, which is how the design draws it — a gap + here wrapped it onto a second. + + It is still allowed to wrap rather than being pinned to one line: a + translation longer than the English will not fit however the row is + divided, and two lines is a better failure than an overflow. */} + {/* The instruction is a step down from Cancel, which is the hierarchy the + design draws — the label describes, the button acts. + + It is also what keeps the longest of these strings on one line. The + frame's own label measures 281px, and this string at 12px Inter + measures 280px, where at 14px it needs 327px in a row that has 327px + 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} +
+ ); +} + +/** + * One row inside the card: a 44px surface a shade lighter than the card itself. + * + * The lift is what makes the rows read as things to act on rather than lines of + * the paragraph above them, and it is the same step the step's own name field + * uses, so the two screens agree about what an input looks like. + */ +function LoginCardRow({ children }: { children: ReactNode }) { + return ( +
+ {children} +
+ ); +} + +/** + * The copy control at the right edge of a row. + * + * It swaps to a check for a moment after a copy, which is the only feedback a + * clipboard write can honestly give — the write either happened or it did not, + * and there is nothing to show for it on screen otherwise. `onCopied` lets a + * row that wants a word as well as a mark hear about it. + */ +function LoginCardCopyButton({ + value, + label, + onCopied, +}: { + value: string; + label: string; + onCopied?: () => void; +}) { + const [copied, setCopied] = useState(false); + const timeoutRef = useRef | null>(null); + + useEffect(() => () => { + if (timeoutRef.current) clearTimeout(timeoutRef.current); + }, []); + + return ( + + ); +} + +/** + * 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. + */ +export function OnboardingLoginCodeRow({ code }: { code: string }) { + const [copied, setCopied] = useState(false); + const timeoutRef = useRef | null>(null); + + useEffect(() => () => { + if (timeoutRef.current) clearTimeout(timeoutRef.current); + }, []); + + return ( + + + {code} + + {copied && copied!} + { + setCopied(true); + if (timeoutRef.current) clearTimeout(timeoutRef.current); + timeoutRef.current = setTimeout(() => setCopied(false), 1500); + }} + /> + + ); +} + +/** + * The field the browser code is pasted back into. + * + * 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. + * + * `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. + */ +export function OnboardingLoginCodeInput({ + value, + onChange, + onSubmit, + onPaste, + disabled, +}: { + value: string; + onChange: (value: string) => void; + onSubmit: () => void; + onPaste?: () => void; + disabled?: boolean; +}) { + return ( + onChange(event.target.value)} + onPaste={() => onPaste?.()} + onKeyDown={(event) => { + if (event.key === "Enter") { + event.preventDefault(); + onSubmit(); + } + }} + className="h-(--sz-44px) w-full rounded-lg bg-muted px-5 font-mono text-xs text-foreground placeholder:font-sans placeholder:text-sm placeholder:text-muted-foreground outline-none focus-visible:ring-ring/50 focus-visible:ring-(length:--rad-3)" + /> + ); +} diff --git a/ui/src/components/AgentConfigForm.render.test.tsx b/ui/src/components/AgentConfigForm.render.test.tsx index 83452cd390..f9d0aaba71 100644 --- a/ui/src/components/AgentConfigForm.render.test.tsx +++ b/ui/src/components/AgentConfigForm.render.test.tsx @@ -1441,6 +1441,19 @@ describe("AgentConfigForm environment selector", () => { expect(mockClipboard.copyTextToClipboard).toHaveBeenCalledWith("WXYZ-1234"); expect(mockClipboard.copyTextToClipboard).toHaveBeenCalledWith("https://auth.example.test/device"); + + // Code above URL, and the numbering agrees. Opening the page is what leaves + // this screen for a form that wants the code from it, so the code is read + // while it is still in front of you. The panel used to run the other way. + const labels = [...result.container.querySelectorAll("div")] + .map((el) => el.textContent?.trim()) + .filter((t) => t === "1. Code" || t === "2. Authentication URL"); + expect(labels).toEqual(["1. Code", "2. Authentication URL"]); + + const codeIndex = result.container.textContent!.indexOf("WXYZ-1234"); + const urlIndex = result.container.textContent!.indexOf("https://auth.example.test/device"); + expect(codeIndex).toBeGreaterThan(-1); + expect(urlIndex).toBeGreaterThan(codeIndex); }); it("keeps the code and URL visible after a later poll returns no prompt", async () => { @@ -1511,6 +1524,36 @@ describe("AgentConfigForm environment selector", () => { expect(result.container.textContent).not.toContain("WXYZ-1234"); }); + it("releases an active login session when the panel unmounts", async () => { + // The server holds a one-per-owner reservation until the session reaches a + // terminal state, so a panel that disappears mid-login would leave the owner + // unable to start another until it expires. + // + // Reachable in the settings form only by navigating away, which is why this + // went unnoticed. The connect step unmounts the panel routinely — Cancel + // closes the canvas, switching source remounts it under a new key, closing + // the wizard drops it — so the cleanup is what keeps an immediate retry + // possible. Deliberately not pushed to `roots`: this test does the unmount + // itself, and that unmount is the thing under test. + mockAgentsApi.testEnvironment.mockResolvedValue(AUTH_MISSING_RESULT); + const result = await renderCodexSandbox(); + + await runTest(result.container); + await startLogin(result.container); + expect(findButton(result.container, "Cancel"), "the login should be active").toBeTruthy(); + + mockAgentsApi.cancelAdapterAuthLogin.mockClear(); + await act(async () => { + result.root.unmount(); + }); + + expect(mockAgentsApi.cancelAdapterAuthLogin).toHaveBeenCalledWith( + "company-1", + "codex_local", + "session-1", + ); + }); + it("announces the login state through a polite live region", async () => { mockAgentsApi.testEnvironment.mockResolvedValue(AUTH_MISSING_RESULT); const result = await renderCodexSandbox(); diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 06b7449c5e..6af6a1e358 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -35,6 +35,13 @@ import { Button } from "@/components/ui/button"; import { FolderOpen, Heart, ChevronDown, X, Copy, Check, ExternalLink, Loader2, TriangleAlert, Bug } from "lucide-react"; import { asBoolean, asFiniteNumber, asObject, cn } from "../lib/utils"; import { copyTextToClipboard } from "../lib/clipboard"; +import { + OnboardingLoginCard, + OnboardingLoginCodeInput, + OnboardingLoginCodeRow, + OnboardingLoginUrlRow, + type AdapterLoginChrome, +} from "./AdapterLoginChrome"; import { resolveAdapterTestEnvironmentId, resolveLocalDefaultEnvironmentId, @@ -1999,9 +2006,32 @@ export type AdapterLoginDescriptor = { // `onApplyStored` binds the fixed reference to an existing stored login with no // new login round trip. The panel shows the apply-existing affordance only when // the status route reports a stored value. +// `autoStart`, `onCancel`, `onConnected` and `chrome` are what the onboarding +// connect step needs, and each is off or absent by default so the two settings +// surfaces that render this panel keep the behaviour they have. +// +// They are props on the existing panels rather than a second implementation +// because the part onboarding needs unchanged is the whole of it: the session +// start, the two polls, the server deadline, the one-shot completion read, the +// unmount release. A copy drawn to the new design would have had to reproduce +// all of that correctly, and the first thing to rot would have been the +// timeout and cleanup paths, which are the ones nobody exercises by hand. export type AdapterLoginPanelProps = AdapterLoginDescriptor & { onStored?: (storedSessionId: string) => void; onApplyStored?: () => void; + // Start the login on mount instead of waiting for a press. The connect step's + // footer button is the press — by the time the panel is rendered there, the + // customer has already asked for this. + autoStart?: boolean; + // The customer abandoned the login from inside the card. The panel has + // already cancelled the server session by the time this fires; the caller + // uses it to put its own control back to the state it started in. + onCancel?: () => void; + // The login reached its success state. Onboarding advances on this, which is + // why the `onboarding` chrome draws no success state of its own — the screen + // it would appear on is already gone. + onConnected?: () => void; + chrome?: AdapterLoginChrome; }; // The login panel dispatcher. It picks the panel from the projected panel mode, @@ -2040,6 +2070,10 @@ function DisplayedCodeLoginPanel({ companyId, adapterType, environmentId, + autoStart, + onCancel, + onConnected, + chrome = "panel", }: AdapterLoginPanelProps) { const [sessionId, setSessionId] = useState(null); const [startError, setStartError] = useState(null); @@ -2100,6 +2134,128 @@ function DisplayedCodeLoginPanel({ const isActive = Boolean(sessionId) && !isTerminal; const startDisabled = startLogin.isPending || isActive; + // Release the server session at once, without a change to the panel state, the + // way the submitted-browser-code panel does. The server holds a per-owner + // reservation until the session reaches a terminal state, so an abandoned + // session locks the owner out until the server deadline. Fire-and-forget: a + // 404 means the server already removed a terminal session, and a cleanup path + // cannot surface any other error either, so it drops them all. The manual + // Cancel button keeps using the `cancelLogin` mutation, because that path also + // returns the panel to its idle start state. + const releaseServerSession = useCallback( + (id: string) => { + void agentsApi.cancelAdapterAuthLogin(companyId, adapterType, id).catch(() => { + // Drop the error, as above. + }); + }, + [companyId, adapterType], + ); + + // Hold the active session id for the unmount cleanup. Onboarding removes this + // panel as soon as Cancel is pressed — `handleCancel` fires the request and + // calls `onCancel` without waiting for it — so the panel can be gone before + // the cancel resolves. Without this, a failed cancel, or any other unmount + // (navigating away, the step advancing), would leave the reservation held + // until the server deadline and an immediate retry unable to start. The ref is + // null once the session leaves the active state, so the cleanup never cancels + // a session the server already removed. + const activeSessionRef = useRef(null); + activeSessionRef.current = isActive ? sessionId : null; + + useEffect(() => { + return () => { + const id = activeSessionRef.current; + if (id) releaseServerSession(id); + }; + }, [releaseServerSession]); + + // Start once, on mount, when the caller has already taken the press. The ref + // is the guard rather than the mutation's own pending flag: `startLogin` + // settles, and without a latch a re-render after it settles would read "not + // pending, no session yet" during the gap before the session id lands and + // start a second login the server would count against the per-owner cap. + const autoStartedRef = useRef(false); + const startLoginRef = useRef(startLogin.mutate); + startLoginRef.current = startLogin.mutate; + useEffect(() => { + if (!autoStart || autoStartedRef.current) return; + autoStartedRef.current = true; + startLoginRef.current(); + }, [autoStart]); + + // Report success upward once. `authenticated` is this panel's terminal + // success: unlike the Claude login there is no completion read after it, so + // the status is the whole of the news. + const connectedRef = useRef(false); + const onConnectedRef = useRef(onConnected); + onConnectedRef.current = onConnected; + useEffect(() => { + if (status !== "authenticated" || connectedRef.current) return; + connectedRef.current = true; + onConnectedRef.current?.(); + }, [status]); + + const handleCancel = () => { + cancelLogin.mutate(); + onCancel?.(); + }; + + if (chrome === "onboarding") { + return ( + + {/* 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} +

+ )} + {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" && ( +

+ {status === "timed_out" + ? "The login timed out. Start it again." + : status === "cancelled" + ? "The login was cancelled." + : "The login did not finish. Start it again."} +

+ )} + + ); + } + return (
{/* `gap`, not `space-y`: the live region below collapses to @@ -2154,16 +2310,36 @@ function DisplayedCodeLoginPanel({ {isActive && prompt && (
- Open the authentication page and enter the code. + Copy the code, then open the authentication page.
- {/* URL first, then the code. The instruction above says to open the - page and *then* enter the code, and the numbering now says the - same — so the order the two rows appear in has to agree with both, - rather than handing over the code before the page it belongs to. */} + {/* Code first, then the URL, and the sentence and the numbering both + say so. + + This used to run the other way, on the reasoning that handing over + a code before the page it belongs to was getting ahead of the + customer. What that missed is where the two rows are used: opening + the page is what leaves this screen, and the form waiting on the + other side wants the code that was on this one. Reaching back for + it is the step worth removing, so the code is read and copied + while it is still in front of you. + + The onboarding card is ordered the same way and for the same + reason. The Claude panel below is not, and should not be — its + second row is a field to type *into*, so there the page genuinely + does come first. */}
- 1. Authentication URL + 1. Code +
+ {prompt.code} +
+ +
+
+
+
+ 2. Authentication URL
{prompt.url}
@@ -2184,15 +2360,6 @@ function DisplayedCodeLoginPanel({
-
-
-
- 2. Code -
- {prompt.code} -
- -
)} @@ -2245,6 +2412,10 @@ function SubmittedBrowserCodeLoginPanel({ environmentId, onStored, onApplyStored, + autoStart, + onCancel, + onConnected, + chrome = "panel", }: AdapterLoginPanelProps) { const [sessionId, setSessionId] = useState(null); const [startError, setStartError] = useState(null); @@ -2558,6 +2729,128 @@ function SubmittedBrowserCodeLoginPanel({ setBrowserCode(""); }; + // Start once, on mount, when the caller has already taken the press. Latched + // for the same reason as the displayed-code panel: a second start would burn + // an owner reservation, and here it would also rotate the stored token twice. + const autoStartedRef = useRef(false); + const startLoginRef = useRef(startLogin.mutate); + startLoginRef.current = startLogin.mutate; + useEffect(() => { + if (!autoStart || autoStartedRef.current) return; + autoStartedRef.current = true; + startLoginRef.current(); + }, [autoStart]); + + /** + * Submit the pasted code without a press. + * + * Only in the onboarding chrome, and only here: this is the login where the + * code comes *back* off the clipboard, so the paste is the answer and a + * Submit button after it adds a step that can be missed. The displayed-code + * login has no field to watch. + * + * Driven by the paste rather than by the value, which is the part that is + * easy to get wrong. `isValidBrowserCode` looks like a completeness check and + * is not one: it accepts any run of printable ASCII from a single character + * up, deliberately, because the provider's exact format has never been + * pinned down. Keying the submit off the value therefore fires on the first + * keystroke of anyone who types the code instead of pasting it — submitting + * one character, failing, and clearing the field they were typing into. + * + * So the paste arms it and the shape check still gates it, which leaves + * typing to Enter. A paste that is not usable simply sits in the field. + * + * `submitCode.isPending` is inside `canSubmit` and `handleSubmit` clears the + * field, so one paste can only submit once. + */ + const handleSubmitRef = useRef(handleSubmit); + handleSubmitRef.current = handleSubmit; + const autoSubmit = chrome === "onboarding"; + const pastedRef = useRef(false); + useEffect(() => { + if (!autoSubmit || !pastedRef.current) return; + pastedRef.current = false; + if (!canSubmit) return; + handleSubmitRef.current(); + }, [autoSubmit, canSubmit, browserCode]); + + // Report success upward once. The `stored` state is the only success state, + // which is why this watches `isStored` and not the `authenticated` status the + // completion read still has to follow. + const connectedRef = useRef(false); + const onConnectedRef = useRef(onConnected); + onConnectedRef.current = onConnected; + useEffect(() => { + if (!isStored || connectedRef.current) return; + connectedRef.current = true; + onConnectedRef.current?.(); + }, [isStored]); + + const handleCancel = () => { + cancelLogin.mutate(); + onCancel?.(); + }; + + if (chrome === "onboarding") { + return ( + + {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. */} + {transportInsecure && ( +

+ + This connection is not encrypted. The login code travels in clear text on this + 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) && ( +

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

+ )} + + ); + } + return (
{/* `gap`, not `space-y`: the live region below collapses to diff --git a/ui/src/components/OnboardingWizard.step.test.tsx b/ui/src/components/OnboardingWizard.step.test.tsx index cb2cd47c15..c35207b0a5 100644 --- a/ui/src/components/OnboardingWizard.step.test.tsx +++ b/ui/src/components/OnboardingWizard.step.test.tsx @@ -898,14 +898,19 @@ describe("OnboardingWizard — which step it lands on", () => { } /** - * The step's own CTA. By exact text, because "Back" sits beside it and both - * steps of the arc label their forward button the same way. + * The step's own CTA. By exact text, because "Back" sits beside it. + * + * Two labels rather than one: the connect step calls its forward button + * "Connect", since there the press starts a sign-in rather than simply + * advancing. The rest of the arc still says "Next". These tests are about + * where a press lands, so either will do. */ function stepCta(): HTMLButtonElement { - const cta = [...document.body.querySelectorAll("button")].find( - (b) => b.textContent?.trim() === "Next", - ); - expect(cta, "the step should render its Next button").toBeTruthy(); + const cta = [...document.body.querySelectorAll("button")].find((b) => { + const text = b.textContent?.trim(); + return text === "Next" || text === "Connect"; + }); + expect(cta, "the step should render its forward button").toBeTruthy(); return cta as HTMLButtonElement; } diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index ed1a69ba95..4c2445ccb4 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -67,6 +67,37 @@ const mockAgentsApi = vi.hoisted(() => ({ status: "present", }), ), + // The sign-in routes. The connect step's Connect button starts a login rather + // than hiring when the signal says the source has no credential, so the two + // login shapes need enough of a server to reach the state the step is about: + // a session that is running, and a prompt to show for it. + startClaudeSetupTokenLogin: vi.fn(async () => ({ + sessionId: "claude-session-1", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + })), + getClaudeSetupTokenLoginStatus: vi.fn(async () => ({ + sessionId: "claude-session-1", + status: "pending", + expiresAt: new Date(Date.now() + 600_000).toISOString(), + })), + getClaudeSetupTokenLoginPrompt: vi.fn(async () => ({ + authorizationUrl: "https://claude.ai/oauth/authorize?code=true", + transportAdvisory: null, + })), + cancelClaudeSetupTokenLogin: vi.fn(async () => ({})), + submitClaudeSetupTokenBrowserCode: vi.fn(async () => ({})), + completeClaudeSetupTokenLogin: vi.fn(async () => ({ storedSessionId: "stored-1" })), + startAdapterAuthLogin: vi.fn(async () => ({ + sessionId: "codex-session-1", + status: "pending", + })), + getAdapterAuthLoginStatus: vi.fn(async () => ({ + sessionId: "codex-session-1", + status: "pending", + prompt: { url: "https://auth.openai.com/codex/device", code: "Q2RJ-E1YIF" }, + })), + cancelAdapterAuthLogin: vi.fn(async () => ({})), })); // The adapter registry mock below always returns this function, so a test // can shape the built adapter config (e.g. a configured ANTHROPIC_API_KEY) @@ -162,19 +193,29 @@ vi.mock("../adapters/use-disabled-adapters", () => ({ // makes it undefined and the call throws. useAdapterRegistryLoaded: () => true, })); -// Adapters with a declared login capability, mirroring the real registry -// closely enough for the login-panel gate: `claude_local` and `codex_local` -// both support a sandbox login. Every other type has none, matching the -// real `useAdapterCapabilities` fallback for an unlisted type. -const ADAPTERS_WITH_LOGIN = new Set(["claude_local", "codex_local"]); +// Adapters with a declared login capability, mirroring the real registry: +// `claude_local` and `codex_local` both support a sandbox login, and every +// other type has none, matching the real `useAdapterCapabilities` fallback for +// an unlisted type. +// +// The panel modes are the real ones rather than one mode for both. They used to +// be, back when nothing outside the panel dispatcher read them. The connect +// step reads them now — the two logins end in different places, so its button +// waits differently for each — and a mock that called Claude's login +// `displayed_code` would have the step testing the wrong half of that. +// Reconcile with KNOWN_DEFAULTS in `adapters/use-adapter-capabilities.ts`. +const ADAPTER_LOGIN_MODES: Record = { + claude_local: "submitted_browser_code", + codex_local: "displayed_code", +}; vi.mock("../adapters/use-adapter-capabilities", () => ({ useAdapterCapabilities: () => (type: string) => ({ supportsInstructionsBundle: false, supportsSkills: false, supportsLocalAgentJwt: false, requiresMaterializedRuntimeSkills: false, - login: ADAPTERS_WITH_LOGIN.has(type) - ? { panelMode: "displayed_code" as const, timeoutPolicy: "fixed" as const } + login: ADAPTER_LOGIN_MODES[type] + ? { panelMode: ADAPTER_LOGIN_MODES[type]!, timeoutPolicy: "fixed" as const } : undefined, }), })); @@ -245,6 +286,20 @@ async function pickFirstSource( await click((text) => text === label); } +/** + * The arc footer's primary button, whatever this step calls it. + * + * Step 4 calls it "Connect", because there it starts a sign-in rather than + * simply advancing; every other arc step calls it "Next". These tests are about + * what the press does, not what it reads, so they match either — restating the + * label at twenty call sites would make a copy change look like a behaviour + * regression. The label itself is pinned once, in the step test that is about + * the label. + */ +function isArcPrimary(text: string): boolean { + return text.startsWith("Next") || text.startsWith("Connect"); +} + describe("OnboardingWizard restore-gate (stale localStorage across accounts)", () => { beforeEach(() => { mockAuthApi.getSession.mockResolvedValue({ @@ -388,7 +443,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(agentField, "Ada"); }); await flushReact(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(document.body.textContent).toContain("Connect a model"); await pickFirstSource(clickByText); @@ -398,7 +453,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // Through Connect to Review, so the Mission-row assertion runs against // the checklist that actually renders it — stopping at the model step // would let a Mission regression pass unseen. - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); // The review step is the heading and the woken agent, nothing else: the // checklist that restated the walk in three rows is gone, and with it // the Mission row that could only render unchecked. @@ -460,9 +515,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(agentField, "Ada"); }); await flushReact(); - await clickText((t) => t.startsWith("Next")); + await clickText((t) => isArcPrimary(t)); await pickFirstSource(clickText); - await clickText((t) => t.startsWith("Next")); + await clickText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); // The mock is declared with no parameters, so index the call rather than @@ -495,12 +550,12 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(agentField, "Ada"); }); await flushReact(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(document.body.textContent).toContain("Connect a model"); await pickFirstSource(clickByText); const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.trim().startsWith("Next"), + isArcPrimary(b.textContent?.trim() ?? ""), )!; await act(async () => { connect.dispatchEvent(new MouseEvent("click", { bubbles: true })); @@ -639,7 +694,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(agentField, "Ada"); }); await flushReact(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(document.body.textContent).toContain("Connect a model"); // Pick a source. The step arrives with nothing chosen — `adapterType` @@ -667,7 +722,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); expect(document.body.textContent).toContain( @@ -692,7 +747,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); @@ -738,7 +793,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(field, KEY); }); await flushReact(); - await handles.clickByText((t) => t.startsWith("Next")); + await handles.clickByText((t) => isArcPrimary(t)); return handles; } @@ -821,7 +876,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockAgentsApi.hire.mockRejectedValueOnce(new Error("network went away")); const { root, clickByText } = await connectWithApiKey(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockSecretsApi.createMyUserSecret).toHaveBeenCalledTimes(1); @@ -841,12 +896,12 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(1); // Switch to API keys, which changes the configuration the hire will send. await clickByText((t) => t.startsWith("Use API key")); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(2); @@ -868,13 +923,13 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(1); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); // A second Connect must not treat the first (cached) blocking result as // reusable — it re-probes, and the create path stays closed. - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(2); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); @@ -889,7 +944,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -914,7 +969,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // scenario this test is named for. const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -939,7 +994,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); // The status route must not even be asked — the conflict is decided @@ -964,7 +1019,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -989,7 +1044,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalled(); const testArgs = mockAgentsApi.testEnvironment.mock.calls.at(-1) as unknown[]; @@ -1008,7 +1063,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // The default `beforeEach` mock already rejects with a 404 `ApiError`. const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.testEnvironment).toHaveBeenCalled(); const testArgs = mockAgentsApi.testEnvironment.mock.calls.at(-1) as unknown[]; @@ -1062,7 +1117,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).toHaveBeenCalled(); @@ -1085,7 +1140,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); expect(document.body.textContent).toContain( @@ -1106,10 +1161,10 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockAgentsApi.hire.mockRejectedValue(new Error("hire failed")); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.getClaudeOAuthTokenStatus).toHaveBeenCalledTimes(1); - await clickByText((t) => t.startsWith("Next")); + await clickByText((t) => isArcPrimary(t)); expect(mockAgentsApi.getClaudeOAuthTokenStatus).toHaveBeenCalledTimes(2); await act(async () => root.unmount()); @@ -1895,12 +1950,12 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ).toBe(false); const cta = [...document.body.querySelectorAll("button")].find( - (b) => b.textContent?.trim() === "Next", + (b) => isArcPrimary(b.textContent?.trim() ?? ""), ); - expect(cta, "the step should render its Next button").toBeTruthy(); + expect(cta, "the step should render its forward button").toBeTruthy(); expect( cta!.hasAttribute("disabled"), - "Next must not advance a question the step has not visibly asked", + "Connect must not advance a question the step has not visibly asked", ).toBe(true); // And it opens again the moment the customer answers it themselves. @@ -1909,7 +1964,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); for (let i = 0; i < 5; i++) await flushReact(); const ctaAfter = [...document.body.querySelectorAll("button")].find( - (b) => b.textContent?.trim() === "Next", + (b) => isArcPrimary(b.textContent?.trim() ?? ""), ); expect(ctaAfter!.hasAttribute("disabled")).toBe(false); @@ -1936,11 +1991,11 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ).toBe(false); const cta = [...document.body.querySelectorAll("button")].find( - (b) => b.textContent?.trim() === "Next", + (b) => isArcPrimary(b.textContent?.trim() ?? ""), ); expect( cta!.hasAttribute("disabled"), - "Next must not hire an adapter the row never offered", + "Connect must not hire an adapter the row never offered", ).toBe(true); await act(async () => root.unmount()); @@ -2006,24 +2061,122 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await act(async () => root.unmount()); }); - it("shows the login panel for claude_local when the signal reports no ready credential", async () => { + /** Press the step's forward button and let the sign-in queries settle. */ + async function pressArcPrimary() { + const cta = [...document.body.querySelectorAll("button")].find((b) => + isArcPrimary(b.textContent?.trim() ?? ""), + ); + expect(cta, "the step should render its forward button").toBeTruthy(); + await act(async () => { + cta!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 8; i++) await flushReact(); + } + + it("starts the claude_local sign-in on Connect when the signal reports no ready credential", async () => { mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "absent" }); const { root } = await openStep4({ adapterType: "claude_local" }); - expect(document.body.textContent).toContain("Sign in to Anthropic"); + + // Nothing before the press. The card *is* the sign-in now, so it does not + // exist until the step has been asked to start one — which is the whole + // difference between this step and the one it replaced. + expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); + + await pressArcPrimary(); + + 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", + ); + await act(async () => root.unmount()); }); - it("shows the login panel for codex_local when the signal cannot decide", async () => { + 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(); + + const field = document.body.querySelector( + 'input[aria-label="Authorization code"]', + ) as HTMLInputElement | null; + expect(field, "the Claude card should offer a code field").toBeTruthy(); + + // The trap this pins. `isValidBrowserCode` reads like a completeness + // check and is not one — it accepts any printable ASCII from a single + // character up — so an auto-submit keyed off the value fires here, on the + // first character of anyone typing the code rather than pasting it, and + // clears the field they are typing into. + await act(async () => { + setControlledValue(field!, "Q"); + }); + for (let i = 0; i < 4; i++) await flushReact(); + expect(mockAgentsApi.submitClaudeSetupTokenBrowserCode).not.toHaveBeenCalled(); + + // The paste is the answer, so it goes without a press. Order matches a + // real paste: the event lands before the value changes. + await act(async () => { + field!.dispatchEvent(new Event("paste", { bubbles: true })); + setControlledValue(field!, "Q2RJ-E1YIF-authorization-code"); + }); + for (let i = 0; i < 4; i++) await flushReact(); + expect(mockAgentsApi.submitClaudeSetupTokenBrowserCode).toHaveBeenCalledWith( + "company-new", + "claude-session-1", + "Q2RJ-E1YIF-authorization-code", + ); + + await act(async () => root.unmount()); + }); + + 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(document.body.textContent).toContain("Sign in to OpenAI"); + + expect(mockAgentsApi.startAdapterAuthLogin).not.toHaveBeenCalled(); + + await pressArcPrimary(); + + 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("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. + 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", + ).toBeTruthy(); + await act(async () => root.unmount()); }); - it("hides the login panel when the signal reports a ready credential", async () => { + it("hires on Connect, with no sign-in, when the signal reports a ready credential", async () => { mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "present" }); const { root } = await openStep4({ adapterType: "claude_local" }); - expect(document.body.textContent).not.toContain("Sign in to Anthropic"); + + await pressArcPrimary(); + + // The positive half is what makes this a test: a source that is already + // signed in goes straight to the hire, so Connect keeps its old meaning + // wherever there is no sign-in to do. Asserting only the absence of a + // card would pass just as well if the button had stopped working. + expect(mockAgentsApi.hire).toHaveBeenCalled(); + expect(mockAgentsApi.startClaudeSetupTokenLogin).not.toHaveBeenCalled(); + await act(async () => root.unmount()); }); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index c6b704fe12..f9f092d792 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -670,6 +670,27 @@ function OnboardingWizardInner({ const [credentialMode, setCredentialMode] = useState( (saved?.credentialMode as CredentialMode) ?? "subscription", ); + /** + * Whether Connect has been pressed for the current source. + * + * 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. + */ + const [loginStarted, setLoginStarted] = useState(false); + /** + * Whether that sign-in reached its success state. + * + * 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. + */ + const [loginConnected, setLoginConnected] = useState(false); /** * 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 @@ -1136,6 +1157,56 @@ function OnboardingWizardInner({ const connectStepReady = sourceSelected && !adapterEnvLoading && !missionUnresolvedForHire; + /** + * Whether this step has a sign-in to do before it can hire. + * + * The same four conditions the card itself renders on, named once so the + * footer button and the card cannot disagree about whether a login is + * happening. When it is false — an API key, a source already signed in on the + * sandbox, no sandbox to sign in against — Connect goes straight to the hire, + * exactly as it did before. + */ + const connectStepNeedsLogin = Boolean( + credentialMode !== "api" && + showAdapterLoginPanel && + createdCompanyId && + resolvedLoginEnvironmentId, + ); + + /** + * Whether the source's login ends by taking a code back from the customer. + * + * This is what splits the two waits, and it is a real difference rather than + * a cosmetic one. The browser-code login finishes here, in the field on the + * card, so the button is busy and says so. The displayed-code login finishes + * somewhere else entirely — another tab, possibly another device — so the + * button is not busy, it is waiting, and a spinner would be claiming work + * this screen is not doing. + */ + const loginSubmitsBrowserCode = + adapterCaps.login?.panelMode === "submitted_browser_code"; + + // Connect is pressed, the login is running, and it has not succeeded yet. + const connectStepLoggingIn = + connectStepNeedsLogin && loginStarted && !loginConnected; + + /** + * What the step's primary action does, for both the button and Cmd+Enter. + * + * One function rather than the condition written twice. The keyboard path + * has drifted from the button here before — the comment on its `step === 4` + * branch is about exactly that — and the gap it left was a hire that skipped + * a check. This one would be worse: Cmd+Enter would hire before the sign-in + * it is meant to start, against a source with no credential. + */ + function handleConnectStepPrimary() { + if (connectStepNeedsLogin && !loginStarted) { + setLoginStarted(true); + return; + } + void handleGiveHeartbeat(); + } + /** * When the input canvas is open: exactly when a source has been chosen. * @@ -1152,7 +1223,32 @@ 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. */ - const canvasOpen = sourceSelected; + /** + * 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 + * started, or the news that no sign-in is possible. + * + * It no longer opens on selection alone. The card is the sign-in itself now, + * and a sign-in starts when Connect is pressed — so between picking a tile + * and pressing the button there is nothing to put here, and the step is the + * question and the button, which is what the design draws. + */ + const canvasOpen = + sourceSelected && + (credentialMode === "api" || loginStarted || 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 @@ -1224,6 +1320,16 @@ 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. + useEffect(() => { + setLoginStarted(false); + setLoginConnected(false); + }, [adapterType, credentialMode]); + const selectedModel = (adapterModels ?? []).find((m) => m.id === model); const hasAnthropicApiKeyOverrideCheck = adapterEnvResult?.checks.some( @@ -2101,8 +2207,16 @@ function OnboardingWizardInner({ // the condition out here again is what let this path hire against a // source the tile row had never shown, after the button was gated and // this was not. - else if (step === 4 && agentName.trim() && connectStepReady) - handleGiveHeartbeat(); + // Also gated on `connectStepLoggingIn`, the way the button is: a sign-in + // that is already running has nothing for this to do, and re-entering it + // would start a second server session. + else if ( + step === 4 && + agentName.trim() && + connectStepReady && + !connectStepLoggingIn + ) + handleConnectStepPrimary(); else if (step === 5) handleLaunchToDashboard(); } } @@ -2797,17 +2911,36 @@ function OnboardingWizardInner({ ) : showAdapterLoginPanel && createdCompanyId && resolvedLoginEnvironmentId ? ( - /* Shows as soon as the cheap auth signal reports no ready - credential, well before any adapter environment test - runs. Reuses the same panel the agent configuration - form shows after a test — see AdapterLoginPanel in - AgentConfigForm.tsx. No "Use saved login" control: the - hire step already applies a stored login on its own. */ + /* 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. + + No "Use saved login" control: the hire step already + applies a stored login on its own. */ setLoginStarted(false)} + 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(); + }} onStored={() => { queryClient.invalidateQueries({ queryKey: queryKeys.agents.authSignal( @@ -2819,20 +2952,18 @@ function OnboardingWizardInner({ }} /> ) : ( - /* No panel to show, and the two reasons for that are not - the same news. Saying either is better than an empty - card — the canvas is open because a source is selected, - and a blank one reads as something that failed to load — - but they must not be conflated: telling someone with no - sandbox that they are "already signed in" on it is - false, and it hides the one thing actually blocking - them. */ + /* 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. */

- {authSignalUndecided - ? "Checking this source's credentials…" - : canShowAdapterLogin - ? "This source is already signed in on the managed sandbox." - : "No managed sandbox is available to sign in against yet."} + No managed sandbox is available to sign in against yet.

)} @@ -3030,16 +3161,37 @@ function OnboardingWizardInner({ ? "Continue" : step === 5 ? "Get started" - : "Next" + : 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" + : "Next" } loadingLabel={ step === 1 ? "Creating..." : step === 4 - ? "Connecting..." + ? "Connecting" : "Launching..." } - loading={step === 3 ? false : loading} + // The browser-code login is finished on this screen, so the + // button is genuinely busy for its duration and shows it. The + // 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) + } primaryDisabled={ step === 1 ? !companyName.trim() || loading @@ -3052,7 +3204,11 @@ function OnboardingWizardInner({ // it, and be hired against whatever the draft // happened to carry. See `connectStepReady`, which // Cmd+Enter asks as well. - !connectStepReady || loading + !connectStepReady || + loading || + // A sign-in is running and has not landed. Nothing + // to press until it does. + connectStepLoggingIn : loading || launchStateIncomplete } onPrimary={() => { @@ -3060,7 +3216,10 @@ function OnboardingWizardInner({ if (skipsMissionStep) void handleCreateCompany(); else setStep(2); } else if (step === 3) setStep(4); - else if (step === 4) handleGiveHeartbeat(); + // One button, two jobs — start the sign-in, or hire — and + // Cmd+Enter has to do the same thing. See + // `handleConnectStepPrimary`. + else if (step === 4) handleConnectStepPrimary(); else handleLaunchToDashboard(); }} /> diff --git a/ui/src/connect-flow-preview-main.tsx b/ui/src/connect-flow-preview-main.tsx new file mode 100644 index 0000000000..c1675256ff --- /dev/null +++ b/ui/src/connect-flow-preview-main.tsx @@ -0,0 +1,295 @@ +import { StrictMode, useEffect, useRef, useState } from "react"; +import { createRoot } from "react-dom/client"; +import { MotionConfig } from "motion/react"; +import { isValidBrowserCode } from "@paperclipai/shared"; + +import { + OnboardingLoginCard, + OnboardingLoginCodeInput, + OnboardingLoginCodeRow, + OnboardingLoginUrlRow, +} from "./components/AdapterLoginChrome"; +import { AgentPreview } from "./components/onboarding/AgentPreview"; +import { ConnectInputCanvas } from "./components/onboarding/ConnectInputCanvas"; +import { CredentialModeLink } from "./components/onboarding/CredentialModeLink"; +import { FooterNav } from "./components/onboarding/FooterNav"; +import { + ModelSourceTiles, + type CredentialMode, + type ModelSource, +} from "./components/onboarding/ModelSourceTiles"; +import { OnboardingHeading } from "./components/onboarding/OnboardingPrimitives"; +import { PillGuy } from "./components/onboarding/PillGuy"; +import { SleepingZs } from "./components/onboarding/SleepingZs"; +import { Stepper } from "./components/onboarding/Stepper"; +import "./index.css"; + +/** + * Backend-free walkthrough of the connect step's sign-in, deployed so the flow + * can be reviewed from a link rather than a checkout. + * + * The sibling of `connect-model-preview-main.tsx`, and the difference between + * them is the point of this one. That page renders `ConnectModelPreview`, a + * mock built to ask a question about the tile row. This one imports the + * *shipped* login card, rows and field from `components/AdapterLoginChrome` — + * the same components the wizard renders — so what is on screen is the + * implementation rather than a drawing of it. The step's furniture around them + * (stepper, avatar, heading, tiles, credential link, footer) is the real + * presentational set too. + * + * What is faked is only the server. The three delays below stand in for a + * session start, a prompt round trip, and the poll that lands while the + * customer is finishing a login somewhere else. The wizard itself is not here: + * it needs a query client, a router and a company. + */ + +const PROMPT_DELAY_MS = 1200; +const SUBMIT_DELAY_MS = 900; +const POLL_DELAY_MS = 3200; + +/** + * OpenAI's blossom, inlined — the shipped step inlines it for the same reason. + * The supplied asset is a white fill that disappears on a light tile, and only + * an inline path can take `currentColor`. Keep in step with the copy in + * `OnboardingWizard.tsx`. + */ +function OpenAiBlossom({ className }: { className?: string }) { + return ( + + + + ); +} + +const MODEL_SOURCES: ModelSource[] = [ + { + id: "claude_local", + label: "Claude", + icon: , + }, + { id: "codex_local", label: "OpenAI", icon: }, +]; + +/** Where the flow is. `auth` splits by source, exactly as the step does. */ +type Phase = "idle" | "connecting" | "auth" | "submitting" | "done"; + +function ConnectFlowPreview({ + initialSourceId, + initialPhase, +}: { + initialSourceId: string | null; + initialPhase: Phase; +}) { + const [selectedId, setSelectedId] = useState(initialSourceId); + const [useApiKeys, setUseApiKeys] = useState(false); + const [phase, setPhase] = useState(initialPhase); + const [code, setCode] = useState(""); + const [polled, setPolled] = useState(false); + const timers = useRef>>([]); + + const submitsBrowserCode = selectedId === "claude_local"; + const mode: CredentialMode = useApiKeys ? "api" : "subscription"; + + const after = (ms: number, fn: () => void) => { + timers.current.push(setTimeout(fn, ms)); + }; + useEffect(() => () => timers.current.forEach(clearTimeout), []); + + // The prompt round trip. + useEffect(() => { + if (phase !== "connecting") return; + after(PROMPT_DELAY_MS, () => setPhase("auth")); + }, [phase]); + + // The poll that lands while the customer is away in another tab, for the + // displayed-code login only. Armed on *reaching* `auth` rather than on + // leaving `connecting`, so a `?state=openai` deep link arms it too — hung off + // the transition, that link opened on a Next that never enabled. + useEffect(() => { + if (phase !== "auth" || submitsBrowserCode || polled) return; + after(POLL_DELAY_MS, () => setPolled(true)); + }, [phase, submitsBrowserCode, polled]); + + const finishSubmit = () => { + setPhase("submitting"); + after(SUBMIT_DELAY_MS, () => setPhase("done")); + }; + + // The same two-part rule the shipped panel uses: the paste arms the submit, + // and the value gates it. Both halves matter here. Running it from the paste + // handler alone would submit whatever was in the field *before* the paste — + // the handler fires first — and skipping the check would advance the flow on + // an empty or malformed paste, which is a worse lie than not previewing it, + // since demonstrating this interaction is what the page is for. + const pastedRef = useRef(false); + useEffect(() => { + if (!pastedRef.current) return; + pastedRef.current = false; + if (!isValidBrowserCode(code.trim())) return; + finishSubmit(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [code]); + + const reset = () => { + timers.current.forEach(clearTimeout); + timers.current = []; + setPhase("idle"); + setCode(""); + setPolled(false); + }; + + const loggingIn = phase === "connecting" || phase === "auth" || phase === "submitting"; + const done = phase === "done"; + + return ( + +
+ + +
+
+ + {!done && } +
+ +
+ +
+ +
+ + {!done && ( + <> +
+ { + if (loggingIn) return; + setSelectedId(id); + }} + /> + { + setUseApiKeys(next === "api"); + reset(); + }} + /> +
+ + {/* Opens on the press, not on the selection — the card is the + sign-in, so there is nothing to hold before one starts. */} + + {submitsBrowserCode ? ( + + + { + if (code.trim()) finishSubmit(); + }} + onPaste={() => { + pastedRef.current = true; + }} + /> + + ) : ( + + {/* Code above link — see the same order in the shipped panel. + Pressing the link leaves for another tab that wants this + code, so it is read before the link is there to press. */} + + + + )} + + + )} + + {} : undefined} + primaryLabel={ + // "Start over" is preview-only: the real step has left for Review + // by now, and this page has nowhere to send you but back. + done ? "Start over" : loggingIn && !submitsBrowserCode ? "Next" : "Connect" + } + loadingLabel="Connecting" + // Busy only where the work is happening on this screen. The + // displayed-code login finishes in another tab, so its button is + // waiting rather than working, and stays still. + loading={loggingIn && submitsBrowserCode} + primaryDisabled={ + done ? false : selectedId === null || (loggingIn && !(polled && !submitsBrowserCode)) + } + onPrimary={() => { + if (done) reset(); + else if (phase === "idle") setPhase("connecting"); + else if (polled && !submitsBrowserCode) setPhase("done"); + }} + /> + + {/* Preview-only. A real paste carries the code; here anything will do. */} + {phase === "auth" && submitsBrowserCode && ( +

+ Preview: paste any text into the field to see the auto-submit. +

+ )} + {phase === "auth" && !submitsBrowserCode && !polled && ( +

+ Preview: Next enables when the poll lands, a few seconds from now. +

+ )} +
+ + ); +} + +/** `?state=` opens on a frame; everything stays clickable afterwards. */ +const STATES: Record = { + default: { initialSourceId: null, initialPhase: "idle" }, + selected: { initialSourceId: "claude_local", initialPhase: "idle" }, + connecting: { initialSourceId: "claude_local", initialPhase: "connecting" }, + claude: { initialSourceId: "claude_local", initialPhase: "auth" }, + openai: { initialSourceId: "codex_local", initialPhase: "auth" }, +}; + +const requested = new URLSearchParams(window.location.search).get("state") ?? "default"; +const initial = STATES[requested] ?? STATES.default!; + +createRoot(document.getElementById("root")!).render( + + {/* Centred against the viewport with `min-h-dvh` and `my-auto`, the way the + sibling preview is: align-items would put the top of a too-tall step out + of scroll reach, and auto margins collapse instead. */} +
+
+ +
+
+ , +); diff --git a/ui/vite.flow-preview.config.mjs b/ui/vite.flow-preview.config.mjs new file mode 100644 index 0000000000..50979135c6 --- /dev/null +++ b/ui/vite.flow-preview.config.mjs @@ -0,0 +1,71 @@ +import fs from "node:fs"; +import path from "path"; +import { fileURLToPath } from "url"; +import { defineConfig } from "vite"; +import react from "@vitejs/plugin-react"; +import tailwindcss from "@tailwindcss/vite"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +/** + * Builds the standalone connect-flow preview (`connect-flow-preview.html`) on its + * own, for deploying as a static page that a reviewer can open from a link. + * + * A separate config rather than a second rollup input on the app's build, and + * the reason is what ends up on the host rather than tidiness: a shared build + * emits the app's ~6MB `main` chunk into the same `dist/assets`, and every file + * in the deployed directory is publicly fetchable whether or not anything links + * to it. Building alone means the deployed bundle is this screen and the pieces + * it composes, full stop. It also leaves the app's own build untouched. + * + * cd ui && npx vite build --config vite.flow-preview.config.mjs + * + * Invoked directly rather than through a `build:flow-preview` script, and the + * reason is governance rather than taste: CODEOWNERS owns `package.json` by + * bare name, which matches at any depth, so adding one line to `ui/package.json` + * puts a preview-only convenience behind a code-owner review. The command is + * the same either way. + */ + +const OUT_DIR = "dist-flow-preview"; + +/** + * Land the entry as `index.html` so the mock is the site root and the output + * directory deploys as-is — no rename step to forget on a redeploy. + * + * Renamed on disk in `closeBundle` rather than rekeyed in `generateBundle`: + * Vite's own HTML plugin emits the document after user plugins have had their + * `generateBundle` turn, so a bundle-level rename finds nothing to rename. The + * document's asset links are absolute (`/assets/...`), so moving the file + * itself breaks nothing. + */ +const previewAsIndex = { + name: "flow-preview-html-as-index", + closeBundle() { + const built = path.resolve(__dirname, OUT_DIR, "connect-flow-preview.html"); + if (!fs.existsSync(built)) return; + fs.renameSync(built, path.resolve(__dirname, OUT_DIR, "index.html")); + }, +}; + +export default defineConfig({ + plugins: [react(), tailwindcss(), previewAsIndex], + resolve: { + alias: { + "@": path.resolve(__dirname, "./src"), + lexical: path.resolve(__dirname, "./node_modules/lexical/dist/Lexical.mjs"), + }, + }, + build: { + outDir: OUT_DIR, + emptyOutDir: true, + minify: "esbuild", + rollupOptions: { + input: path.resolve(__dirname, "connect-flow-preview.html"), + }, + }, + esbuild: { + drop: ["console", "debugger"], + legalComments: "none", + }, +});