diff --git a/tests/e2e/conference-room-typing-intro.spec.ts b/tests/e2e/conference-room-typing-intro.spec.ts index 0935ebc0a2..4a9b717dc3 100644 --- a/tests/e2e/conference-room-typing-intro.spec.ts +++ b/tests/e2e/conference-room-typing-intro.spec.ts @@ -82,10 +82,22 @@ async function runOnboardingWizard(page: Page, companyName: string) { // name and hires under the neutral `general` role. await page.waitForSelector("#onboarding-agent-name", { timeout: 30_000 }); await page.locator("#onboarding-agent-name").fill("Ada"); - await page.getByRole("button", { name: /^Next/ }).click(); + await page.getByRole("button", { name: /^Next$/ }).click(); - // Step 4: adapter (claude_local default); heartbeat is intercepted. - await page.getByRole("button", { name: /^Connect$/ }).click(); + // Step 4: pick a model source, then advance. Nothing is selected on arrival + // — the row is a question, not a confirmation — so the CTA is disabled until + // a tile is pressed. By role rather than by label: which adapters the tiles + // offer depends on the registry this environment reports. + const source = page.getByRole("radio").first(); + 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$/ }); + await expect(connectNext).toBeEnabled({ timeout: 30_000 }); + await connectNext.click(); // Step 5: review → Get started creates the first task and opens its // detail page. diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index c53e5d99b2..f8094a4464 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -219,9 +219,17 @@ test.describe("Onboarding wizard", () => { await page.locator("#onboarding-agent-name").fill("Ada"); await page.getByRole("button", { name: "Next" }).click(); - // Step 4 (Connect a model): the default adapter is claude_local, and the - // signal above reports no ready credential, so the login panel must show - // with no button to reuse a saved login. + // 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. + 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 @@ -231,11 +239,13 @@ test.describe("Onboarding wizard", () => { }); await expect(page.getByRole("button", { name: "Use saved login" })).toHaveCount(0); - // Exact, because the progress strip's segments are buttons too and one of - // them is labelled "Connect a model" for assistive tech. An unanchored - // /^Connect/ matches both it and this CTA, which is a strict-mode violation - // rather than a wrong click — Playwright refuses instead of guessing. - await page.getByRole("button", { name: "Connect", exact: true }).click(); + // 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(); // The failed test blocks the hire and shows its own checks. await expect(page.getByText("The claude CLI was not found on this host.")).toBeVisible({ diff --git a/tests/e2e/planning-mode-visual-verification.spec.ts b/tests/e2e/planning-mode-visual-verification.spec.ts index d3a69bf1f8..cb362f03ce 100644 --- a/tests/e2e/planning-mode-visual-verification.spec.ts +++ b/tests/e2e/planning-mode-visual-verification.spec.ts @@ -66,8 +66,19 @@ test("captures planning mode UI for desktop and mobile", async ({ page }) => { await page.waitForSelector("#onboarding-agent-name", { timeout: 30_000 }); await page.locator("#onboarding-agent-name").fill(AGENT_NAME); - await page.getByRole("button", { name: /^Next/ }).click(); - await page.getByRole("button", { name: /^Connect$/ }).click(); + 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. + const source = page.getByRole("radio").first(); + await source.waitFor({ timeout: 30_000 }); + await source.click(); + const connectNext = page.getByRole("button", { name: /^Next$/ }); + await expect(connectNext).toBeEnabled({ timeout: 30_000 }); + await connectNext.click(); // The review step names the agent rather than the step. await expect( diff --git a/ui/src/components/AgentConfigForm.render.test.tsx b/ui/src/components/AgentConfigForm.render.test.tsx index 80e4c87c6c..83452cd390 100644 --- a/ui/src/components/AgentConfigForm.render.test.tsx +++ b/ui/src/components/AgentConfigForm.render.test.tsx @@ -1424,7 +1424,7 @@ describe("AgentConfigForm environment selector", () => { await runTest(result.container); await startLogin(result.container); - expect(result.container.textContent).toContain("Preparing the login"); + expect(result.container.textContent).toContain("Preparing..."); expect(result.container.textContent).not.toContain("https://"); }); diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 4867f276e8..e67da4cd3d 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -2119,7 +2119,7 @@ function DisplayedCodeLoginPanel({ {isActive && !prompt && (
- Preparing the login… + Preparing...
)} @@ -2128,19 +2128,14 @@ function DisplayedCodeLoginPanel({
Open the authentication page and enter the code.
+ {/* 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 -
- {prompt.code} -
- -
-
-
-
- Authentication URL + 1. Authentication URL
{prompt.url}
@@ -2161,6 +2156,15 @@ function DisplayedCodeLoginPanel({
+
+
+
+ 2. Code +
+ {prompt.code} +
+ +
)} @@ -2607,7 +2611,7 @@ function SubmittedBrowserCodeLoginPanel({ {isActive && !authorizationUrl && !isCompleting && (
- Preparing the login… + Preparing...
)} @@ -2630,7 +2634,7 @@ function SubmittedBrowserCodeLoginPanel({
- Authorization URL + 1. Authorization URL
{authorizationUrl}
@@ -2653,7 +2657,7 @@ function SubmittedBrowserCodeLoginPanel({
- Browser code + 2. Browser code
{ await settle(); } + async function press(button: Element) { + await act(async () => { + button.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await settle(); + } + + /** + * 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. + */ + 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(); + return cta as HTMLButtonElement; + } + + /** + * Pick a model source. The connect step arrives with nothing chosen, so its + * CTA stays disabled until a tile is pressed — found by `aria-checked` + * rather than by label, because this suite mocks the display registry and + * the tiles carry whatever it happens to return. + */ + async function pickModelSource() { + const tiles = [...document.body.querySelectorAll("button[aria-checked]")]; + expect(tiles.length, "the connect step should offer a source").toBeGreaterThan(0); + await press(tiles[0]!); + } + it("seeds the lead agent's instructions with the mission it was never asked for", async () => { // The regression this exists for. The agent step feeds // `composeCeoInstructions` from the mission field, and a company entered @@ -898,22 +929,11 @@ describe("OnboardingWizard — which step it lands on", () => { await openOnAgentStep(); await nameAgent(); - const next = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Next"), - )!; - await act(async () => { - next.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); + await press(stepCta()); - const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Connect"), - )!; - expect(connect.hasAttribute("disabled")).toBe(false); - await act(async () => { - connect.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); + await pickModelSource(); + expect(stepCta().hasAttribute("disabled")).toBe(false); + await press(stepCta()); expect(mockAgentsApi.saveInstructionsFile).toHaveBeenCalled(); const [, file] = mockAgentsApi.saveInstructionsFile.mock.calls[0]; @@ -930,18 +950,9 @@ describe("OnboardingWizard — which step it lands on", () => { await openOnAgentStep(); await nameAgent(); - const next = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Next"), - )!; - await act(async () => { - next.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); - expect( - [...document.body.querySelectorAll("button")] - .find((b) => b.textContent?.includes("Connect"))! - .hasAttribute("disabled"), - ).toBe(false); + await press(stepCta()); + await pickModelSource(); + expect(stepCta().hasAttribute("disabled")).toBe(false); mockGoalsApi.list.mockReturnValue(new Promise(() => {})); await act(async () => { @@ -951,62 +962,28 @@ describe("OnboardingWizard — which step it lands on", () => { }); await settle(2); - const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Connect"), - )!; - expect(connect.hasAttribute("disabled")).toBe(true); + expect(stepCta().hasAttribute("disabled")).toBe(true); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); }); - it("hydrates again when the same company comes back through onboarding", async () => { - // The hydration marker is a ref, so it outlives the state it describes. - // `reset()` clears the mission field; leaving the marker set would make - // the second run believe a mission it no longer holds was already - // fetched — and hire the agent without it, exactly as before this fix. - await openOnAgentStep(); - - const close = [...document.body.querySelectorAll("button")].find((b) => - b.querySelector(".sr-only")?.textContent?.includes("Close"), - ); - expect(close).toBeDefined(); - await act(async () => { - close!.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); - // `reset()` ran: the wizard is back at the front door with a cleared - // mission field, which is precisely the state the marker must not - // outlive. - expect(currentStep()).not.toBe("agent"); - - routerState.pathname = "/"; - await rerender(); - await settle(); - routerState.pathname = "/PC1/onboarding"; - dialogState.onboardingRouteDismissed = false; - await rerender(); - await settle(); - expect(currentStep()).toBe("agent"); - await nameAgent(); - - const next = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Next"), - )!; - await act(async () => { - next.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); - const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Connect"), - )!; - await act(async () => { - connect.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); - - expect(mockAgentsApi.saveInstructionsFile).toHaveBeenCalled(); - const [, file] = mockAgentsApi.saveInstructionsFile.mock.calls.at(-1)!; - expect(file.content).toContain("Scale the marketplace"); - }); + // Removed: "hydrates again when the same company comes back through + // onboarding". + // + // It closed the wizard with the X and re-opened it, which made `reset()` + // clear `hydratedMissionForRef` and the second pass hydrate again. The arc + // has no X any more — the connect step deliberately has no exit, because + // nothing downstream of it works until a model is connected — so `reset()` + // is now reachable only from a completed launch. + // + // Three substitutes were tried and all three were green against a wizard + // with the behaviour deleted, which is worse than no test: routing to "/" + // never withdraws the company; a swap to another company re-points the + // marker by itself, since it stores which company was hydrated rather than + // a bare flag; and either route dance remounts the inner wizard, so the ref + // does not survive to be tested. What the marker guards is still covered + // from the front by "seeds the lead agent's instructions with the mission it + // was never asked for". Restore a real version of this when the arc gains a + // way out. it("hires under the neutral role, with the name the customer typed", async () => { // The arc stopped asking for a role, so every onboarding hire is filed @@ -1016,20 +993,9 @@ describe("OnboardingWizard — which step it lands on", () => { await openOnAgentStep(); await nameAgent("Ada"); - const next = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Next"), - )!; - await act(async () => { - next.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); - const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Connect"), - )!; - await act(async () => { - connect.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await settle(); + await press(stepCta()); + await pickModelSource(); + await press(stepCta()); expect(mockAgentsApi.hire).toHaveBeenCalled(); const [, payload] = mockAgentsApi.hire.mock.calls.at(-1)!; diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index b7d225be5a..add0e145d8 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -227,6 +227,24 @@ function render() { return { container, root, queryClient }; } +/** + * Press the first model-source tile. + * + * By `aria-checked` rather than by label: the display registry is mocked in this + * suite, so a tile reads "claude_localSubscription" rather than "Claude Code", + * and a test that selected on the visible name would be asserting the mock. + * + * The connect step arrives with nothing chosen, so this is what opens the input + * surface and lets the step advance. + */ +async function pickFirstSource( + click: (match: (text: string) => boolean) => Promise, +): Promise { + const tile = [...document.body.querySelectorAll("button[aria-checked]")][0]; + const label = tile?.textContent?.trim() ?? ""; + await click((text) => text === label); +} + describe("OnboardingWizard restore-gate (stale localStorage across accounts)", () => { beforeEach(() => { mockAuthApi.getSession.mockResolvedValue({ @@ -301,6 +319,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.loading = false; mockCompaniesApi.list.mockResolvedValue([]); + // The model step needs tiles to pick from, and this suite's default + // registry is empty. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "codex_local" }]; const { root, queryClient } = render(); const renderTree = () => act(async () => { @@ -370,13 +391,14 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await clickByText((t) => t.startsWith("Next")); expect(document.body.textContent).toContain("Connect a model"); + await pickFirstSource(clickByText); expect(document.body.textContent).not.toContain("Adapter environment check"); expect(document.body.textContent).not.toContain("Test now"); // 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("Connect")); + await clickByText((t) => t.startsWith("Next")); // 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. @@ -405,6 +427,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompaniesApi.list.mockResolvedValue([]); mockCompaniesApi.create.mockResolvedValue({ id: "company-new", issuePrefix: "INI" }); + // The model step needs tiles to pick from, and this suite's default + // registry is empty. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "codex_local" }]; const { root, queryClient } = render(); const renderTree = () => act(async () => { @@ -436,7 +461,8 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); await flushReact(); await clickText((t) => t.startsWith("Next")); - await clickText((t) => t.startsWith("Connect")); + await pickFirstSource(clickText); + await clickText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); // The mock is declared with no parameters, so index the call rather than @@ -471,9 +497,10 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await flushReact(); await clickByText((t) => t.startsWith("Next")); expect(document.body.textContent).toContain("Connect a model"); + await pickFirstSource(clickByText); const connect = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.trim().startsWith("Connect"), + b.textContent?.trim().startsWith("Next"), )!; await act(async () => { connect.dispatchEvent(new MouseEvent("click", { bubbles: true })); @@ -569,6 +596,11 @@ 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() { + // 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 + // that can never advance. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "codex_local" }]; mockCompaniesApi.create.mockResolvedValue({ id: "company-new", issuePrefix: "INI" }); window.localStorage.setItem( ONBOARDING_STORAGE_KEY, @@ -610,6 +642,13 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await clickByText((t) => t.startsWith("Next")); expect(document.body.textContent).toContain("Connect a model"); + // 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 + // not advance until a tile is pressed. Every case below is about what + // happens *after* that choice, so the helper makes it. + await pickFirstSource(clickByText); + return { root, clickByText }; } @@ -628,7 +667,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); expect(document.body.textContent).toContain( @@ -653,7 +692,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); @@ -691,7 +730,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( async function connectWithApiKey() { const handles = await openConnectStep(); - await handles.clickByText((t) => t.startsWith("Use API keys")); + await handles.clickByText((t) => t.startsWith("Use API key")); const field = document.body.querySelector( 'input[type="password"]', ) as HTMLInputElement; @@ -699,7 +738,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( setControlledValue(field, KEY); }); await flushReact(); - await handles.clickByText((t) => t.startsWith("Connect")); + await handles.clickByText((t) => t.startsWith("Next")); return handles; } @@ -782,7 +821,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("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockSecretsApi.createMyUserSecret).toHaveBeenCalledTimes(1); @@ -802,12 +841,12 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(1); // Switch to API keys, which changes the configuration the hire will send. - await clickByText((t) => t.startsWith("Use API keys")); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Use API key")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(2); @@ -829,13 +868,13 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); 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("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.testEnvironment).toHaveBeenCalledTimes(2); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); @@ -850,7 +889,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -875,7 +914,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("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -900,7 +939,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); // The status route must not even be asked — the conflict is decided @@ -925,7 +964,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); const hireArgs = mockAgentsApi.hire.mock.calls.at(-1) as unknown[]; @@ -950,7 +989,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.testEnvironment).toHaveBeenCalled(); const testArgs = mockAgentsApi.testEnvironment.mock.calls.at(-1) as unknown[]; @@ -969,7 +1008,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("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.testEnvironment).toHaveBeenCalled(); const testArgs = mockAgentsApi.testEnvironment.mock.calls.at(-1) as unknown[]; @@ -1023,7 +1062,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).toHaveBeenCalled(); @@ -1046,7 +1085,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( }); const { root, clickByText } = await openConnectStep(); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.hire).not.toHaveBeenCalled(); expect(document.body.textContent).toContain( @@ -1067,10 +1106,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("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.getClaudeOAuthTokenStatus).toHaveBeenCalledTimes(1); - await clickByText((t) => t.startsWith("Connect")); + await clickByText((t) => t.startsWith("Next")); expect(mockAgentsApi.getClaudeOAuthTokenStatus).toHaveBeenCalledTimes(2); await act(async () => root.unmount()); @@ -1228,7 +1267,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // It mounts: the companies query sets `retry: false`, and with no // companies the dashboard's "Get Started" button opens onboarding — a gate // that rendered nothing here would make that button dead. - expect(document.body.textContent).not.toBe(""); + expect(document.querySelector('[data-testid="onboarding-wizard"]')).not.toBeNull(); // The draft is not restored, because ownership cannot be verified... const nameInput = document.body.querySelector( "#onboarding-agent-name", @@ -1273,7 +1312,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( expect(localStorage).toHaveBeenCalled(); // It mounted: the wizard is open with no draft, rather than the render // throwing on the way in. - expect(document.body.textContent).not.toBe(""); + expect(document.querySelector('[data-testid="onboarding-wizard"]')).not.toBeNull(); localStorage.mockRestore(); await act(async () => { @@ -1314,7 +1353,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await flushReact(); // Mounted rather than blank... - expect(document.body.textContent).not.toBe(""); + expect(document.querySelector('[data-testid="onboarding-wizard"]')).not.toBeNull(); // ...but the draft was not restored, because the list cannot be trusted. const nameInput = document.body.querySelector( "#onboarding-agent-name", @@ -1325,44 +1364,14 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( root.unmount(); }); }); - it("closes without throwing when the browser denies storage access", async () => { - // `reset()` clears the draft and `handleClose` calls it, so the close - // button is a fourth storage call site. Guarding the read, the cleanup and - // the persist effect one at a time is how this one stayed unguarded while - // the others looked fixed. - const deny = () => { - throw new DOMException("The operation is insecure.", "SecurityError"); - }; - const localStorage = vi.spyOn(window, "localStorage", "get").mockImplementation(deny); - mockCompany.companies = [{ id: "c1", name: "My Co", issuePrefix: "MC" }]; - mockCompany.loading = false; - - const { root, queryClient } = render(); - await act(async () => { - root.render( - - - , - ); - }); - await flushReact(); - - const close = [...document.body.querySelectorAll("button")].find((b) => - b.textContent?.includes("Close"), - ); - expect(close).toBeDefined(); - await act(async () => { - close!.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - await flushReact(); - - expect(mockDialog.closeOnboarding).toHaveBeenCalled(); - - localStorage.mockRestore(); - await act(async () => { - root.unmount(); - }); - }); + // The close-on-storage-denial case is gone with the control that reached it. + // Onboarding is a gate now, not a dialog: there is no X, Escape does not + // dismiss it, and nothing in the wizard calls `handleClose`. A test that + // clicked Close was asserting a path a customer no longer has. + // + // The open side of the same hazard is still covered by "renders instead of + // throwing when the browser denies storage access" directly above, which is + // the half that can still happen. it("leaves the draft untouched when the company query fails and onboarding is closed", async () => { // Mounting is safe for the draft precisely because the persist effect is // gated on the wizard being open. A closed wizard writes nothing, so the @@ -1490,7 +1499,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await flushReact(); // Mounted — so this is a real observation, not an unmounted false pass. - expect(document.body.textContent).not.toBe(""); + expect(document.querySelector('[data-testid="onboarding-wizard"]')).not.toBeNull(); expect(document.body.textContent).not.toContain("A's Lead"); const nameInput = document.body.querySelector( "#onboarding-agent-name", @@ -1564,7 +1573,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( await flushReact(); // Mounted, so this observes the real thing rather than an empty document. - expect(document.body.textContent).not.toBe(""); + expect(document.querySelector('[data-testid="onboarding-wizard"]')).not.toBeNull(); expect(document.body.textContent).not.toContain("A's Lead"); const nameInput = document.body.querySelector( "#onboarding-agent-name", @@ -1668,6 +1677,12 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockInstanceSettingsApi.getExperimental.mockResolvedValue({ enableManagedSandboxOnly: false }); mockAgentsApi.getAdapterAuthSignal.mockReset(); mockAgentsApi.getAdapterAuthSignal.mockResolvedValue({ status: "present" }); + // The row has to actually offer the adapter these drafts name. The login + // panel lives inside the input canvas, and the canvas opens for a chosen + // source — a saved adapter the row cannot show is not a chosen one, so + // without this the panel is absent for a reason that has nothing to do + // with the auth signal these tests are about. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "codex_local" }]; }); /** Drives the wizard to the Connect step with a company already created. */ @@ -1703,9 +1718,137 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // enough to reach the end of that chain. for (let i = 0; i < 5; i++) await flushReact(); expect(document.body.textContent).toContain("Connect a model"); + // No pick needed here: the draft this helper restores already names an + // adapter, which is what a run returning to this step actually carries. return { root, queryClient }; } + it("will not advance on a saved adapter the step no longer offers", async () => { + // A draft can name an adapter this registry does not carry — a cloud + // sandbox without claude_local, an adapter since disabled. The row hides + // it, so the step shows an unanswered question: no tile filled, no input + // canvas. The CTA has to agree with that. + // + // It did not. The gate asked `sourcePicked`, which only means "a draft + // named something", so Next stayed live on a step that had visibly asked + // nothing and would hire against the hidden name. Reported by Greptile on + // #12726. + mockAdapterRegistry.list = [{ type: "codex_local" }]; + const { root } = await openStep4({ adapterType: "some_retired_adapter" }); + + const tiles = [...document.body.querySelectorAll("button[aria-checked]")]; + expect(tiles.length, "the row should still offer what it has").toBeGreaterThan(0); + expect( + tiles.some((t) => t.getAttribute("aria-checked") === "true"), + "no tile should read as chosen", + ).toBe(false); + + const cta = [...document.body.querySelectorAll("button")].find( + (b) => b.textContent?.trim() === "Next", + ); + expect(cta, "the step should render its Next button").toBeTruthy(); + expect( + cta!.hasAttribute("disabled"), + "Next must not advance a question the step has not visibly asked", + ).toBe(true); + + // And it opens again the moment the customer answers it themselves. + await act(async () => { + tiles[0]!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 5; i++) await flushReact(); + const ctaAfter = [...document.body.querySelectorAll("button")].find( + (b) => b.textContent?.trim() === "Next", + ); + expect(ctaAfter!.hasAttribute("disabled")).toBe(false); + + await act(async () => root.unmount()); + }); + + it("will not advance on a saved adapter the row does not show", async () => { + // The other shape of the same defect, and the one the snap cannot cover. + // + // The tile row is `recommendedAdapters`; the snap's idea of "visible" is + // recommended *plus* the advanced list. An adapter in the second but not + // the first — a saved `opencode_local`, say — therefore satisfies the + // snap, which leaves it alone, while the row it is supposed to be chosen + // in never shows it. Nothing is highlighted, the canvas is shut, and with + // the gate on `sourcePicked` the CTA was live: one press hires against an + // adapter the customer has not seen on this screen. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "opencode_local" }]; + const { root } = await openStep4({ adapterType: "opencode_local" }); + + const tiles = [...document.body.querySelectorAll("button[aria-checked]")]; + expect( + tiles.some((t) => t.getAttribute("aria-checked") === "true"), + "the saved adapter is not in this row, so nothing should read as chosen", + ).toBe(false); + + const cta = [...document.body.querySelectorAll("button")].find( + (b) => b.textContent?.trim() === "Next", + ); + expect( + cta!.hasAttribute("disabled"), + "Next must not hire an adapter the row never offered", + ).toBe(true); + + await act(async () => root.unmount()); + }); + + it("will not hire from the keyboard on a source nobody selected", async () => { + // The step has two ways forward, and gating only the visible one leaves + // the defect intact behind a keystroke. Cmd+Enter called + // `handleGiveHeartbeat` directly with its own, older list of conditions, + // so with the button correctly disabled the same screen still hired on + // Cmd+Enter. Reported by Greptile on #12726 after the button was fixed. + // + // The saved adapter is one the registry no longer carries, so the snap + // replaces it with `claude_local` and clears the pick: the row shows two + // tiles, neither chosen. A keystroke that gets through hires claude_local + // — a real, offerable adapter that nobody on this screen selected, which + // is what makes the bypass worth a test rather than a comment. + mockAdapterRegistry.list = [{ type: "claude_local" }, { type: "codex_local" }]; + const { root } = await openStep4({ adapterType: "some_retired_adapter" }); + + const tiles = [...document.body.querySelectorAll("button[aria-checked]")]; + expect( + tiles.some((t) => t.getAttribute("aria-checked") === "true"), + "the snap must not leave a tile reading as chosen", + ).toBe(false); + + const wizard = document.querySelector('[data-testid="onboarding-wizard"]'); + expect(wizard, "the wizard should be mounted").not.toBeNull(); + for (const modifier of [{ metaKey: true }, { ctrlKey: true }]) { + await act(async () => { + wizard!.dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", bubbles: true, ...modifier }), + ); + }); + for (let i = 0; i < 6; i++) await flushReact(); + } + + expect( + mockAgentsApi.hire, + "no keystroke may hire a source nobody selected", + ).not.toHaveBeenCalled(); + + // And it works once the question is answered, so this is a gate rather + // than a dead shortcut. + await act(async () => { + tiles[0]!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + for (let i = 0; i < 6; i++) await flushReact(); + await act(async () => { + wizard!.dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", bubbles: true, metaKey: true }), + ); + }); + for (let i = 0; i < 6; i++) await flushReact(); + expect(mockAgentsApi.hire).toHaveBeenCalled(); + + await act(async () => root.unmount()); + }); + it("starts no call to the test-environment route on adapter selection", async () => { const { root } = await openStep4(); expect(mockAgentsApi.testEnvironment).not.toHaveBeenCalled(); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index e71b581ad7..26f6df8e68 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -112,7 +112,6 @@ import { Check, Loader2, ChevronDown, - X } from "lucide-react"; type Step = 0 | 1 | 2 | 3 | 4 | 5; @@ -549,6 +548,22 @@ function OnboardingWizardInner({ const [adapterType, setAdapterType] = useState(() => restoreOnboardingAdapterType(saved?.adapterType), ); + /** + * Whether a model source has been chosen, as opposed to which one + * `adapterType` happens to hold. + * + * The two are not the same, and reading the second as the first is what made + * this step arrive with a tile already lit and its input already open: the + * hire needs an adapter, so `adapterType` always carries one, restored or + * 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. + */ + const [sourcePicked, setSourcePicked] = useState( + () => typeof saved?.adapterType === "string" && saved.adapterType.length > 0, + ); const savedNativeRunnerDraft = saved?.adapterType === "paperclip_runner"; const [cwd, setCwd] = useState((saved?.cwd as string) ?? ""); // Native drafts may carry provider-specific configuration that is invalid @@ -1027,19 +1042,46 @@ function OnboardingWizardInner({ * `adapterType` alone, because a restored draft can name an adapter this step * no longer offers — a selection the customer cannot see. */ - const sourceSelected = recommendedAdapters.some((opt) => opt.type === adapterType); + const sourceSelected = + sourcePicked && recommendedAdapters.some((opt) => opt.type === adapterType); /** - * When the input canvas is open. + * Whether the connect step may advance. * - * A selected source is the ordinary reason — the card is the answer to the - * tile that was just pressed, so an untouched row leaves nothing under it. But - * it opens for a pending sign-in regardless of the row, because the adapter - * needing credentials does not depend on it having a tile: a restored draft - * naming an adapter this step no longer offers still cannot hire without one, - * and hiding the panel would leave that dead end with nothing to press. + * One predicate, because there are two ways to advance and they drifted. The + * button's condition and Cmd+Enter's were written out separately, so when the + * button gained `sourceSelected` and `adapterEnvLoading` the keyboard kept the + * older, shorter list — and hired against a source the row had never shown. + * The same defect the button was just fixed for, one path over. + * + * `loading` is deliberately not here. The keyboard handler returns on it + * before reaching any step, for a reason particular to keystrokes: a second + * Enter re-enters a handler whose guard is state the first has not written + * yet. That check belongs at the top of the handler, not per-step. + * + * Anything that gates this step belongs in here, so the next one is added + * once rather than twice. */ - const canvasOpen = sourceSelected || showAdapterLoginPanel; + const connectStepReady = + sourceSelected && !adapterEnvLoading && !missionUnresolvedForHire; + + /** + * When the input canvas is open: exactly when a source has been chosen. + * + * The card is the answer to the tile that was just pressed, so an untouched + * row leaves nothing under it — a sign-in bar offered before the question was + * answered says the step already knows which provider is meant, which it does + * not. + * + * This used to open for a pending sign-in as well, `|| showAdapterLoginPanel`, + * so that a restored draft naming an adapter this step no longer offers still + * had something to press. That reasoning came from when the row arrived with a + * selection already made and an invisible one was a dead end. It is not one + * now: the row is a question, an unofferable saved adapter simply leaves it + * 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 default (or a saved) adapterType can name an adapter the server has // since disabled — e.g. a cloud sandbox registry without claude_local. The @@ -1058,6 +1100,13 @@ function OnboardingWizardInner({ if (visible.some((a) => a.type === adapterType)) return; const next = visible[0].type as AdapterType; setAdapterType(next); + // The snap is not a choice. It replaces a name the customer can no longer + // see with the first one they can, which is the right thing to hold — but + // holding it *as chosen* would put a filled tile and an open sign-in panel + // on a step nobody has answered, and re-create by the back door exactly the + // preselection this step was changed to stop doing. The saved answer was + // unofferable, so the question is open again. + setSourcePicked(false); if (next === "codex_local") return; if (next === "opencode_local") { setModel(DEFAULT_OPENCODE_LOCAL_MODEL); @@ -1977,7 +2026,11 @@ function OnboardingWizardInner({ } else if (step === 2 && companyName.trim() && companyGoal.trim()) handleConfirmMission(); else if (step === 3 && agentName.trim()) setStep(4); - else if (step === 4 && agentName.trim() && !missionUnresolvedForHire) + // `connectStepReady`, the same predicate the step's button uses. Spelling + // 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(); else if (step === 5) handleLaunchToDashboard(); } @@ -2040,16 +2093,19 @@ function OnboardingWizardInner({ RemoveScroll which blocks wheel events on our custom (non-DialogContent) scroll container. A plain div preserves the background without scroll-locking. */}
-
- {/* Close button */} - + {/* A deliberate hook for "the wizard mounted". + The tests that assert it opens used to prove it by finding any text + in the document — which, with the front door mocked to null in that + suite, was only ever the close button's screen-reader label. Removing + the button took the proof with it, and those tests would have gone on + passing indefinitely if it had stayed. A named anchor says what they + mean rather than depending on whatever happens to render. */} +
{/* Step 0: Front Door — full-screen choice */} {step === 0 && (
@@ -2086,8 +2142,13 @@ function OnboardingWizardInner({ // narrower than the next screen's makes the whole frame jump on // Continue — which is the thing that read as "off" to begin // with, and is more obvious once the buttons match. + // 68px sides, so the column inside the 560px frame is 424px — + // the measure the design draws every arc step to. It was 40px + // (a 480px column), which is wide enough that the two model + // 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-10 sm:py-11" + ? "w-(--sz-560px) max-w-full px-8 py-10 sm:px-(--sz-68px) sm:py-11" : "w-full max-w-md px-8 py-12", )} > @@ -2195,7 +2256,7 @@ function OnboardingWizardInner({ // sentence restating it only pushes the fields down. lede={ step === 3 ? undefined : step === 4 ? ( - <>Paperclip works with your existing subscription or API keys. + <>Paperclip works with your subscription or API keys. ) : ( <>{agentName.trim() || "Your first agent"} is ready to work! ) @@ -2541,11 +2602,21 @@ function OnboardingWizardInner({ `general` role; a specific one can be set later, where there is context to choose it in. */} {step === 3 && ( -
+
- + + {/* + Filled, not outlined, and the column's full width — the + same field the naming step before the hand-off draws. + `bg-muted` is the design's field surface; the default + Input is a hairline border over `bg-input/30`, which on + this ground reads as an empty outline rather than a place + to type. The border is kept but made transparent so the + focus ring, which colours the border, still has one. + */} setAgentName(e.target.value)} @@ -2582,11 +2653,13 @@ function OnboardingWizardInner({ }))} mode={credentialMode} selectedId={ + sourcePicked && recommendedAdapters.some((opt) => opt.type === adapterType) ? adapterType : null } onSelect={(id) => { + setSourcePicked(true); setAdapterType(id); if (id === "codex_local") return; if (id === "opencode_local") { @@ -2873,11 +2946,9 @@ function OnboardingWizardInner({ primaryLabel={ step === 1 ? "Continue" - : step === 3 - ? "Next" - : step === 4 - ? "Connect" - : "Get started" + : step === 5 + ? "Get started" + : "Next" } loadingLabel={ step === 1 @@ -2893,7 +2964,13 @@ function OnboardingWizardInner({ : step === 3 ? !agentName.trim() : step === 4 - ? loading || adapterEnvLoading || missionUnresolvedForHire + ? // 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 : loading || launchStateIncomplete } onPrimary={() => { diff --git a/ui/src/components/onboarding/ConnectInputCanvas.tsx b/ui/src/components/onboarding/ConnectInputCanvas.tsx index 13280e1fe8..89d4a0492c 100644 --- a/ui/src/components/onboarding/ConnectInputCanvas.tsx +++ b/ui/src/components/onboarding/ConnectInputCanvas.tsx @@ -4,6 +4,7 @@ import { AnimatePresence, motion } from "motion/react"; import { cn } from "../../lib/utils"; import { CANVAS_CONTENT_ENTER, + CANVAS_ENTER_TRAVEL, CANVAS_CONTENT_EXIT, CANVAS_CONTENT_TRAVEL, } from "./onboarding-motion"; @@ -53,17 +54,24 @@ export function ConnectInputCanvas({ 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. - Nothing animates on this wrapper, deliberately. It carried an enter/exit - three times — height, then opacity — and stalled every time, once leaving the - login card rendered inside a two-pixel box and once at four percent opacity - while `open` was true the whole while. The casualty each time was the OAuth - URL a customer has to click. The swap inside still animates; the container - holding it does not need to, and cannot be trusted to. + 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 @@ -89,7 +97,7 @@ export function ConnectInputCanvas({ {children} -
+ ); } diff --git a/ui/src/components/onboarding/CredentialModeLink.tsx b/ui/src/components/onboarding/CredentialModeLink.tsx index cfd85d7d15..f8e1111c58 100644 --- a/ui/src/components/onboarding/CredentialModeLink.tsx +++ b/ui/src/components/onboarding/CredentialModeLink.tsx @@ -18,7 +18,7 @@ import { LINK_LABEL_FADE_IN, LINK_LABEL_FADE_OUT } from "./onboarding-motion"; */ const LINK_LABEL: Record = { - subscription: "Use API keys instead", + subscription: "Use API key instead", api: "Use subscription instead", }; diff --git a/ui/src/components/onboarding/ModelSourceTiles.tsx b/ui/src/components/onboarding/ModelSourceTiles.tsx index 1aa9fba179..b2cf82c238 100644 --- a/ui/src/components/onboarding/ModelSourceTiles.tsx +++ b/ui/src/components/onboarding/ModelSourceTiles.tsx @@ -82,12 +82,17 @@ function ModelSourceTile({ // and lending it to focus as well would mean tabbing across the row // looked like picking every tile in turn. "outline-none focus-visible:ring-ring/50 focus-visible:ring-(length:--rad-3)", - // Hover brings the surface up to the same half-strength ground the - // selected tile already sits on, and stops there. Pointing at a tile - // should say "this one is live", not "this one is chosen" — so the - // bright stroke stays reserved for the choice, and the only thing - // separating hover from selection is the border. - selected ? "border-foreground bg-accent/50" : "border-border hover:bg-accent/50", + // Selection is a lighter surface, not a brighter edge. Both states keep + // the same border — it draws the tile, not the choice — and the fill + // carries the state. A bright stroke on one tile made the row read as + // one outlined object beside one plain one, rather than two of a kind + // with one of them picked. + // + // Hover stops short of the selected fill, so pointing at a tile says + // "this one is live" rather than "this one is chosen". + selected + ? "border-border bg-accent" + : "border-border bg-card hover:bg-accent/40", )} > diff --git a/ui/src/components/onboarding/onboarding-motion.ts b/ui/src/components/onboarding/onboarding-motion.ts index 5bdebcf319..27c9441a80 100644 --- a/ui/src/components/onboarding/onboarding-motion.ts +++ b/ui/src/components/onboarding/onboarding-motion.ts @@ -147,6 +147,17 @@ export const CANVAS_EASE = TAG_SWAP_EASE; * legible was time added to a swap that had nothing to wait for. A spinner * standing in for no work is a slower screen that also says something untrue. */ + +/** + * How far the input canvas descends into place when a source is picked. + * + * Larger than the swap's travel, and in the opposite direction. A swap trades + * one input for another in a space that already exists, so it barely moves; this + * is a surface arriving where there was none, and it comes down from above so + * the movement reads as the tile above it opening out. + */ +export const CANVAS_ENTER_TRAVEL = 10; + export const CANVAS_CONTENT_ENTER = TAG_SWAP_ENTER; export const CANVAS_CONTENT_EXIT = TAG_SWAP_EXIT; export const CANVAS_CONTENT_TRAVEL = TAG_SWAP_TRAVEL; diff --git a/ui/src/index.css b/ui/src/index.css index 4c60ccc6a3..208e0e3905 100644 --- a/ui/src/index.css +++ b/ui/src/index.css @@ -2286,6 +2286,7 @@ span.paperclip-mention-chip[data-mention-kind="external-object"] { --sz-calc-13: calc(0.75rem + 0.5rem); /* Extracted from ui/src/components/IssueRow.tsx (ml-[calc(theme(spacing.3)+theme(spacing.2))]). */ --sz-140px: 140px; /* Extracted from ui/src/components/JsonSchemaForm.tsx (min-h-[140px]). */ --sz-52px: 52px; /* Extracted from ui/src/components/KanbanBoard.tsx (w-[52px]). */ + --sz-68px: 68px; /* The onboarding arc's side inset — 560px frame, 424px column. */ --sz-48px: 48px; /* Extracted from ui/src/components/KanbanBoard.tsx (min-w-[48px]). */ --sz-260px: 260px; /* Extracted from ui/src/components/KanbanBoard.tsx (min-w-[260px]). */ --sz-120px: 120px; /* Extracted from ui/src/components/KanbanBoard.tsx (min-h-[120px]). */ diff --git a/ui/storybook/fixtures/onboardingDraft.test.ts b/ui/storybook/fixtures/onboardingDraft.test.ts index bb8a322305..e76b2cb337 100644 --- a/ui/storybook/fixtures/onboardingDraft.test.ts +++ b/ui/storybook/fixtures/onboardingDraft.test.ts @@ -45,12 +45,15 @@ describe("storybook onboarding draft", () => { expect(readOnboardingDraft()?.createdAgentId).toBe(""); }); - // The label, not the type, was seeded here once. Nothing failed loudly: the - // connect step fell back to a real adapter, and the mismatch would only have - // surfaced as a hire posting a type the server does not know. - it("names the adapter by its type", () => { + // A run standing on step 3 has not reached the connect step, so it cannot + // have chosen a source there. Seeding one is not a harmless head start: the + // step reads a saved `adapterType` as "already picked", and every arc story + // opened with Claude Code selected and its sign-in panel already showing — + // the preselection the step was changed to stop doing, restored by the + // fixture. Stories that need a source click one, the way a customer does. + it("does not claim a model source was chosen before the connect step", () => { seedOnboardingDraft(); - expect(readOnboardingDraft()?.adapterType).toBe("claude_local"); + expect(readOnboardingDraft()).not.toHaveProperty("adapterType"); }); // `restoreOnboardingState` treats restoring as an authorization decision and diff --git a/ui/storybook/fixtures/onboardingDraft.ts b/ui/storybook/fixtures/onboardingDraft.ts index 88bdf2aa33..604105d89b 100644 --- a/ui/storybook/fixtures/onboardingDraft.ts +++ b/ui/storybook/fixtures/onboardingDraft.ts @@ -40,10 +40,15 @@ export function seedOnboardingDraft(): void { companyName: "Paperclip Storybook", agentName: "Darnold", agentRole: "general", - // The adapter's real type, not its label. This read `claude_code`, which - // is no adapter at all — the connect step recovered by falling back, and - // the hire would have posted a type the server does not know. - adapterType: "claude_local", + // No `adapterType`. This draft describes a run standing on step 3, and a + // run that has not reached the connect step cannot have chosen a source + // there — seeding one made every arc story arrive with Claude Code already + // picked, which is precisely the preselection the step was changed to stop + // doing. Stories that need a source pick one, the way a customer does. + // + // (It read `claude_code` before that, which is no adapter at all: the step + // recovered by falling back, and the hire would have posted a type the + // server does not know.) createdCompanyId: STORYBOOK_COMPANY_ID, createdCompanyPrefix: "PAP", createdAgentId: "", diff --git a/ui/storybook/stories/onboarding-agent-arc.stories.tsx b/ui/storybook/stories/onboarding-agent-arc.stories.tsx index 28b5ad7bec..81e039f31a 100644 --- a/ui/storybook/stories/onboarding-agent-arc.stories.tsx +++ b/ui/storybook/stories/onboarding-agent-arc.stories.tsx @@ -128,14 +128,38 @@ const STEP_TIMEOUT_MS = 15_000; * from the wait above. The wizard re-renders as those queries land, and a node * captured a moment earlier can be detached by the time it is clicked — a click * that raises no error and does nothing. + * + * The arrival is waited on by *heading*, not by the next button's label. The arc + * labels its forward button "Next" on every step it has one, so a story that + * waited for a button name would be satisfied by the button it just clicked and + * report arriving somewhere it never left. This is not hypothetical: these + * stories waited for a button named "Connect" until the label changed, at which + * point Review sat on the connect step looking like a story written to open + * there — the exact failure `STEP_TIMEOUT_MS` is commented against. */ -async function advance(from: string, to: string) { +async function advance(to: string) { await waitFor( - () => expect(screen.getByRole("button", { name: from })).toBeEnabled(), + () => expect(screen.getByRole("button", { name: PRIMARY })).toBeEnabled(), { timeout: STEP_TIMEOUT_MS }, ); - await userEvent.click(screen.getByRole("button", { name: from })); - await screen.findByRole("button", { name: to }, { timeout: STEP_TIMEOUT_MS }); + await userEvent.click(screen.getByRole("button", { name: PRIMARY })); + await screen.findByText(to, { selector: "h2, h1" }, { timeout: STEP_TIMEOUT_MS }); +} + +/** The arc's forward button, which reads the same on every step but the last. */ +const PRIMARY = "Next"; + +/** + * Pick a model source, which the connect step needs before it will go forward. + * + * Nothing is selected on arrival — deliberately, so the row reads as a question + * rather than a confirmation — and the CTA stays disabled until one is pressed. + * Found by role rather than by label so the choice does not depend on which + * adapters the fixture registry happens to offer. + */ +async function pickFirstSource() { + const tiles = await screen.findAllByRole("radio", {}, { timeout: STEP_TIMEOUT_MS }); + await userEvent.click(tiles[0]!); } /** @@ -191,7 +215,7 @@ export const ConnectAModel: StoryObj = { return resetOnboardingFixtureState; }, render: () => , - play: () => advance("Next", "Connect"), + play: () => advance("Connect a model"), }; /** @@ -208,7 +232,7 @@ export const ConnectAModelAlreadySignedIn: StoryObj = { return resetOnboardingFixtureState; }, render: () => , - play: () => advance("Next", "Connect"), + play: () => advance("Connect a model"), }; /** @@ -224,7 +248,7 @@ export const ConnectAModelNoSandbox: StoryObj = { return resetOnboardingFixtureState; }, render: () => , - play: () => advance("Next", "Connect"), + play: () => advance("Connect a model"), }; /** @@ -245,8 +269,9 @@ export const Review: StoryObj = { }, render: () => , play: async () => { - await advance("Next", "Connect"); - await advance("Connect", "Get started"); + await advance("Connect a model"); + await pickFirstSource(); + await advance("Let's get started..."); }, };