From 3ff636bc4852dfc07fcab8ccfdb0b4fd22c1548c Mon Sep 17 00:00:00 2001 From: Tonio Date: Sat, 22 Aug 2026 02:28:45 -0700 Subject: [PATCH] Drop the mission step from the wizard arc (#11935) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - New customers arrive through an onboarding arc that spans Paperclip Cloud and the tenant app > - Cloud's naming screen stopped asking for the company mission, but the tenant wizard still decided its first step by asking whether the company had one > - Every Cloud-created company therefore looked mission-less on arrival, so every walk detoured through a "Define your mission" screen the design had already removed > - This pull request removes that step from the arc, and makes step 1 create the company itself > - The benefit is a shorter arc that matches the design, and a three-step progress strip that now counts the steps that exist ## Linked Issues or Issue Description No public issue exists. The problem was found by walking staging end to end. **What happened:** A new customer signs in, names their organization, and waits for it to build. The tenant wizard then asks "Define your mission" before it asks for the first agent. Cloud no longer collects a mission, so this screen appears for every new customer. **Expected behavior:** The wizard asks for the first agent, the model, and a review. The progress strip counts three steps. **Actual behavior:** The wizard asks for the mission first. The progress strip counts five segments, because the run does not enter on the agent arc. **Additional context:** Three merged pull requests built the mission-based step choice this change removes: #11352, #11416 and #11429. The mission is now collected later, inside the tenant app, so onboarding does not ask for it at all. ## What Changed - `onboardingStepForCompany` always returns the agent step. The `companyHasMission` parameter is removed, because it cannot change the answer. - `resolveRouteOnboardingOptions` no longer accepts `companyHasMission`. - The dashboard no longer waits for the goal lookup before it opens the wizard. That wait only chose a step, and the step is now fixed. - Step 1 creates the company in a new `handleCreateCompany`. Company creation used to sit at the end of `handleConfirmMission`. - No company goal is written during onboarding. - The three-step strip now shows on the agent, model and review steps, because every Cloud-first run enters on the arc. - The full-length bar drops its second segment. No run can fill it. - The grow path keeps its step 2 questionnaire. Only the create path skips ahead. - Back from the agent step goes to the screen the run came from. - Four end-to-end specs no longer drive the wizard through the mission step. ## Verification Run the tenant test suite: ``` cd ui && npx vitest run ``` - 4356 tests pass. 471 files pass. - `npx tsc --noEmit` reports no errors. - Fault injection: forcing `skipsMissionStep` to `true` fails the grow questionnaire test. Removing the Back rule fails the Back test. Both tests fail on the exact defect they guard. - The three-step strip is asserted by an existing test. It checks `Step 1 of 3` and `aria-label="Create your first agent"`. Manual check on staging after the paired Cloud change: 1. Open a new incognito window. 2. Sign in with a new account. 3. Name the organization. 4. Confirm the wizard shows "Create your first agent" and "Step 1 of 3". ## Risks - **Behavioral change.** Onboarding no longer writes a company goal. An agent hired during onboarding starts without a seeded mission. This is intended. The mission moves to the tenant app. - **Dead code.** `ONBOARDING_MISSION_STEP` and the mission screen stay in the codebase, but nothing in the app opens them. They wait for the surface that collects the mission later. - **Grow path.** The grow path is unchanged, but it shares step 2 with the removed screen. New tests cover it. - **Superseded work.** #11352, #11416 and #11429 tuned the mission-based step choice. This change removes the branch they tuned. ## Model Used Claude Opus 5 (`claude-opus-5`), extended thinking, with tool use and code execution through Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal Paperclip ticket id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 5 --- .../e2e/conference-room-typing-intro.spec.ts | 5 +- tests/e2e/nux-phase4-screenshots.spec.ts | 14 +- tests/e2e/onboarding.spec.ts | 33 ++--- .../planning-mode-visual-verification.spec.ts | 6 +- ui/src/App.onboarding-launcher.test.tsx | 51 +++---- ui/src/App.tsx | 8 +- .../components/OnboardingWizard.step.test.tsx | 118 +++++++++------ ui/src/components/OnboardingWizard.test.tsx | 140 ++++++++++++++++++ ui/src/components/OnboardingWizard.tsx | 126 +++++++++++++--- ui/src/lib/onboarding-route.test.ts | 76 +++++----- ui/src/lib/onboarding-route.ts | 32 ++-- ui/src/pages/Dashboard.tsx | 15 +- 12 files changed, 412 insertions(+), 212 deletions(-) diff --git a/tests/e2e/conference-room-typing-intro.spec.ts b/tests/e2e/conference-room-typing-intro.spec.ts index 4bffe5f5eb..81db129601 100644 --- a/tests/e2e/conference-room-typing-intro.spec.ts +++ b/tests/e2e/conference-room-typing-intro.spec.ts @@ -18,7 +18,6 @@ import { * exactly that failing condition. */ -const MISSION = "Verify the first-task launch survives the wizard handoff."; const FIRST_TASK_TITLE = "Paperclip onboarding"; /** @@ -77,9 +76,7 @@ async function runOnboardingWizard(page: Page, companyName: string) { await page.getByPlaceholder("Acme Corp").fill(companyName); await page.getByRole("button", { name: /^Next/ }).click(); - // Step 2: mission (direct path default). - await page.getByPlaceholder("What is your team trying to achieve?").fill(MISSION); - await page.getByRole("button", { name: /Confirm mission/ }).click(); + // Step 1's "Next" creates the company; the mission step no longer runs. // Step 3: the lead's role, then its name. The role gates "Next", and // choosing one fills the name — so the walk only types here to override it. diff --git a/tests/e2e/nux-phase4-screenshots.spec.ts b/tests/e2e/nux-phase4-screenshots.spec.ts index d5b56929e4..c1bd87b419 100644 --- a/tests/e2e/nux-phase4-screenshots.spec.ts +++ b/tests/e2e/nux-phase4-screenshots.spec.ts @@ -72,17 +72,8 @@ test.describe("NUX Phase 4 visual QA", () => { await page.screenshot({ path: shot("02-create-name.png") }); await page.getByRole("button", { name: /^Next/ }).click(); - await expect( - page.getByRole("heading", { name: "Define your mission" }), - ).toBeVisible({ timeout: 10_000 }); - await page - .getByPlaceholder("What is your team trying to achieve?") - .fill("Build affordable home robots that handle household chores."); - await page.screenshot({ path: shot("03-create-mission.png") }); - - // Step 2 advances via "Confirm mission" (creates the company + goal); - // step 3 is the team-lead naming step of the capsule wizard. - await page.getByRole("button", { name: /Confirm mission/ }).click(); + // Step 1's "Next" creates the company and goes straight to the team lead. + // The mission screenshot that sat here is gone with the step it captured. await page.waitForSelector("#onboarding-agent-role", { timeout: 30_000, }); @@ -155,7 +146,6 @@ test.describe("NUX Phase 4 visual QA", () => { for (const f of [ "01-front-door.png", "02-create-name.png", - "03-create-mission.png", "04-hire-team-lead.png", "05-growth-intake.png", "06-board-chat.png", diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index ecd077d332..87acc8ca6b 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -6,14 +6,13 @@ import { test, expect } from "@playwright/test"; * The wizard now opens on a front door (path picker) and the "Create a new * company" path runs: * Step 0 — Front door (Create a new company / Level up existing) - * Step 1a — Name your organization - * Step 1b — Define your mission (direct or guided) + * Step 1a — Name your organization (creates the company) * Step 2 — Hire your team lead (adapter picker) * Step 3+ — Launch celebration → CEO chat → hiring plan → orientation * * This test covers the deterministic, LLM-free core: it drives the front door - * through company naming + mission definition (which creates the company and a - * company-level goal) and verifies the wizard advances to the team-lead step. + * through company naming (which creates the company) and verifies the wizard + * advances to the team-lead step without asking for a mission. * * The tail (CEO chat at step 4, hiring-plan generation at step 5, final * landing) depends on a live LLM and is verified separately during manual / @@ -22,10 +21,9 @@ import { test, expect } from "@playwright/test"; */ const COMPANY_NAME = `E2E-Test-${Date.now()}`; -const MISSION = "Build affordable home robots that handle household chores."; test.describe("Onboarding wizard", () => { - test("create-company path: name + mission creates company and goal", async ({ + test("create-company path: naming creates the company, and no goal is invented", async ({ page, }) => { const pageErrors: string[] = []; @@ -60,17 +58,9 @@ test.describe("Onboarding wizard", () => { await page.getByPlaceholder("Acme Corp").fill(COMPANY_NAME); await page.getByRole("button", { name: /^Next/ }).click(); - // Step 2 — Define your mission (direct entry is the default path). - await expect( - page.getByRole("heading", { name: "Define your mission" }), - ).toBeVisible({ timeout: 10_000 }); - await page - .getByPlaceholder("What is your team trying to achieve?") - .fill(MISSION); - - // "Confirm mission" creates the company + a company-level goal, then - // advances to the team-lead naming step of the capsule wizard. - await page.getByRole("button", { name: /Confirm mission/ }).click(); + // Step 1's "Next" now creates the company and goes straight to the agent. + // The mission step used to sit between them and do the creating; onboarding + // no longer asks for the mission, which is collected later in the app. await page.waitForSelector("#onboarding-agent-role", { timeout: 30_000, }); @@ -85,6 +75,10 @@ test.describe("Onboarding wizard", () => { ); expect(company, `company ${COMPANY_NAME} should exist`).toBeTruthy(); + // And no company-level goal, which is the point rather than an omission. + // Onboarding no longer asks for a mission, so writing one here would mean + // inventing a goal the customer never chose. The mission is collected later + // in the app, and the absence is what leaves room for it. const goalsRes = await page.request.get( `${baseUrl}/api/companies/${company.id}/goals`, ); @@ -93,7 +87,10 @@ test.describe("Onboarding wizard", () => { const companyGoal = (Array.isArray(goals) ? goals : []).find( (g: { level?: string }) => g.level === "company", ); - expect(companyGoal, "a company-level goal should be created").toBeTruthy(); + expect( + companyGoal, + "onboarding must not invent a mission the customer never gave", + ).toBeFalsy(); // The expanded wizard must not crash the app (Rules-of-Hooks regression). expect(pageErrors, pageErrors.join("\n")).toHaveLength(0); diff --git a/tests/e2e/planning-mode-visual-verification.spec.ts b/tests/e2e/planning-mode-visual-verification.spec.ts index fb362d40e9..2bcefc9139 100644 --- a/tests/e2e/planning-mode-visual-verification.spec.ts +++ b/tests/e2e/planning-mode-visual-verification.spec.ts @@ -59,11 +59,7 @@ test("captures planning mode UI for desktop and mobile", async ({ page }) => { await page.locator('input[placeholder="Acme Corp"]').fill(companyName); await page.getByRole("button", { name: /^Next/ }).click(); - await expect(page.getByRole("heading", { name: "Define your mission" })).toBeVisible({ timeout: 30_000 }); - await page - .getByPlaceholder("What is your team trying to achieve?") - .fill("Capture planning mode visual evidence for the graduated task UI."); - await page.getByRole("button", { name: /Confirm mission/ }).click(); + // Naming the company creates it and goes straight to the agent step. // The lead is no longer pre-named. Choosing a role fills the name from the // role's label, which is also what gates "Next". diff --git a/ui/src/App.onboarding-launcher.test.tsx b/ui/src/App.onboarding-launcher.test.tsx index 84e14e530c..7e9153f6d9 100644 --- a/ui/src/App.onboarding-launcher.test.tsx +++ b/ui/src/App.onboarding-launcher.test.tsx @@ -6,7 +6,6 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ONBOARDING_AGENT_STEP, - ONBOARDING_MISSION_STEP, } from "./lib/onboarding-route"; /** @@ -167,37 +166,29 @@ describe("the onboarding launcher's Add Agent button", () => { }); }); - it("still asks for the mission when the company has none", async () => { - mockGoalsApi.list.mockResolvedValue([]); - await render(); - await settle(); + it("goes to the agent step whatever the goals lookup says", async () => { + // Two tests lived here — company with no mission, and a lookup that failed — + // and both sent "Add Agent" to the mission step first. Onboarding no longer + // asks for the mission, so neither goal state can divert a button whose + // whole purpose is adding an agent. + for (const goals of [ + () => mockGoalsApi.list.mockResolvedValue([]), + () => mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")), + ]) { + dialogState.openOnboarding.mockClear(); + goals(); + await render(); + await settle(); - await act(async () => { - addAgentButton().dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); + await act(async () => { + addAgentButton().dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); - expect(dialogState.openOnboarding).toHaveBeenCalledWith({ - initialStep: ONBOARDING_MISSION_STEP, - companyId: "company-1", - }); - }); - - it("asks for the mission when the lookup fails, rather than skipping it", async () => { - // Same fail-open rule the other two entry points follow: an unknown - // mission costs the step, which the customer can answer. Confirming it now - // updates the company's existing goal rather than adding a second one. - mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); - await render(); - await settle(); - - await act(async () => { - addAgentButton().dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); - - expect(dialogState.openOnboarding).toHaveBeenCalledWith({ - initialStep: ONBOARDING_MISSION_STEP, - companyId: "company-1", - }); + expect(dialogState.openOnboarding).toHaveBeenCalledWith({ + initialStep: ONBOARDING_AGENT_STEP, + companyId: "company-1", + }); + } }); it("opens with no company when the prefix matches none", async () => { diff --git a/ui/src/App.tsx b/ui/src/App.tsx index ab983b0d69..161c2b915f 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -97,7 +97,6 @@ import { onboardingStepForCompany, shouldRedirectCompanylessRouteToOnboarding, } from "./lib/onboarding-route"; -import { useCompanyMission } from "./hooks/useCompanyMission"; import { filterHiddenInstanceSettingsPath, normalizeRememberedInstanceSettingsPath } from "./lib/instance-settings"; const CompanyExport = lazy(() => @@ -433,11 +432,6 @@ export function OnboardingRoutePage() { const matchedCompany = companyPrefix ? companies.find((company) => company.issuePrefix.toUpperCase() === companyPrefix.toUpperCase()) ?? null : null; - // Which step this company belongs on, by the same rule the route resolver - // and the dashboard already use. Resolved above the early return below, - // because a hook cannot be called after it. - const { hasMission } = useCompanyMission(matchedCompany?.id ?? null); - // The OnboardingWizard auto-opens on this route (and can also be opened // explicitly). While it is showing it covers the whole screen, so the // launcher card below must not stay interactive behind it — otherwise users @@ -474,7 +468,7 @@ export function OnboardingRoutePage() { // costs the step, which the customer can pass - and the // mission step now updates the existing goal rather than // adding a second one. - initialStep: onboardingStepForCompany(hasMission), + initialStep: onboardingStepForCompany(), companyId: matchedCompany.id, }) : openOnboarding() diff --git a/ui/src/components/OnboardingWizard.step.test.tsx b/ui/src/components/OnboardingWizard.step.test.tsx index 78ab9b166f..f344f1171b 100644 --- a/ui/src/components/OnboardingWizard.step.test.tsx +++ b/ui/src/components/OnboardingWizard.step.test.tsx @@ -233,45 +233,51 @@ describe("OnboardingWizard — which step it lands on", () => { expect(currentStep()).toBe("agent"); }); - it("stays closed until the mission lookup settles", async () => { - // The step is applied once. Opening before the answer is in would land the - // customer on the mission step and leave them there. + // Four tests lived here, and all four were about one thing: the landing step + // was derived from the company's goals, so every state of that lookup — + // pending, failed, resolved, resolved-again-with-a-different-answer — could + // move the customer. Onboarding no longer asks for the mission, so the step + // no longer reads the goals at all and those four states collapse into one + // assertion. Kept as three cases rather than one because the property worth + // defending is that *none* of them reaches the wizard, which a single happy + // path would not show. + it("opens on the agent step without waiting for the goals lookup", async () => { + // This used to stay closed until the lookup settled, because the step it + // would have picked depended on the answer. Waiting now only delays the open. routerState.pathname = "/PC1/onboarding"; mockGoalsApi.list.mockReturnValue(new Promise(() => {})); await render(); + await settle(); - expect(currentStep()).toBe("closed"); + expect(currentStep()).toBe("agent"); }); - it("opens on the mission step when the lookup fails, rather than not at all", async () => { - // Fail-open. A goals request that exhausts its retries must cost the step, - // not the whole flow. + it("opens on the agent step when the goals lookup fails outright", async () => { routerState.pathname = "/PC1/onboarding"; mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); await render(); await settle(); - expect(currentStep()).toBe("mission"); + expect(currentStep()).toBe("agent"); }); it("does not move an open wizard when a later refetch finds a mission", async () => { - // The defect this file exists for. The lookup fails, the wizard opens on - // the mission step, the customer starts typing — and a refetch then - // succeeds. The derived step flips from 2 to 3. Before the fix, the sync - // effect took that as a dependency and moved the customer to the agent - // step mid-sentence. + // The defect this file exists for, in its current form. A refetch landing + // mid-flow used to flip the derived step from 2 to 3 and move the customer + // mid-sentence. Nothing derives the step from goals any more, so the answer + // changing is not an event the wizard can see — which is what this asserts. routerState.pathname = "/PC1/onboarding"; mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); await render(); await settle(); - expect(currentStep()).toBe("mission"); + expect(currentStep()).toBe("agent"); await act(async () => { queryClient.setQueryData(queryKeys.goals.list("company-1"), [COMPANY_GOAL]); }); await settle(); - expect(currentStep()).toBe("mission"); + expect(currentStep()).toBe("agent"); }); it("does not move an open wizard when the dialog is re-opened with a new step", async () => { @@ -297,17 +303,13 @@ describe("OnboardingWizard — which step it lands on", () => { expect(currentStep()).toBe("mission"); }); - it("re-decides the step when the route names a different company", async () => { - // The guard must hold the step against a *stale value settling*, not - // against a genuinely new request. Navigating to another company's - // onboarding is a new request, and its answer is a different one. + it("re-decides the company when the route names a different one", async () => { + // The step is the same either way now; the company is not, and a route that + // names a new one is still a new request. routerState.pathname = "/PC1/onboarding"; - mockGoalsApi.list.mockImplementation((companyId: string) => - companyId === "company-2" ? Promise.resolve([COMPANY_GOAL]) : Promise.resolve([]), - ); await render(); await settle(); - expect(currentStep()).toBe("mission"); + expect(currentStep()).toBe("agent"); routerState.pathname = "/PC2/onboarding"; await rerender(); @@ -332,6 +334,22 @@ describe("OnboardingWizard — which step it lands on", () => { expect(currentStep()).toBe("mission"); } + // The route no longer lands on the mission step — onboarding stopped + // asking — so a test that needs that step opens it the way the tenant app + // will when it collects the mission later: explicitly, naming the company. + // What these tests defend is unchanged: state written for one company must + // not survive into the next. + async function openMissionStepFor(companyId: string) { + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId, + initialStep: ONBOARDING_MISSION_STEP, + }; + await render(); + await settle(); + expect(currentStep()).toBe("mission"); + } + async function click(el: Element) { await act(async () => { el.dispatchEvent(new MouseEvent("click", { bubbles: true })); @@ -473,11 +491,7 @@ describe("OnboardingWizard — which step it lands on", () => { // the next company skip saving its own mission, and the launch path then // links that company's project to the previous company's goal. mockGoalsApi.create.mockResolvedValue({ id: "goal-company-1" }); - routerState.pathname = "/PC1/onboarding"; - dialogState.onboardingOpen = false; - await render(); - await settle(); - expect(currentStep()).toBe("mission"); + await openMissionStepFor("company-1"); const direct = [...document.body.querySelectorAll("button")].find((b) => b.textContent?.includes("I know my mission"), @@ -489,7 +503,10 @@ describe("OnboardingWizard — which step it lands on", () => { await settle(); expect(currentStep()).toBe("agent"); - routerState.pathname = "/PC2/onboarding"; + dialogState.onboardingOptions = { + companyId: "company-2", + initialStep: ONBOARDING_MISSION_STEP, + }; await rerender(); await settle(); expect(currentStep()).toBe("mission"); @@ -522,10 +539,7 @@ describe("OnboardingWizard — which step it lands on", () => { resolveCreate = resolve; }), ); - routerState.pathname = "/PC1/onboarding"; - await render(); - await settle(); - expect(currentStep()).toBe("mission"); + await openMissionStepFor("company-1"); const direct = [...document.body.querySelectorAll("button")].find((b) => b.textContent?.includes("I know my mission"), @@ -536,7 +550,10 @@ describe("OnboardingWizard — which step it lands on", () => { await click(confirmMissionButton()!); // Switch companies before the write lands, then let it land. - routerState.pathname = "/PC2/onboarding"; + dialogState.onboardingOptions = { + companyId: "company-2", + initialStep: ONBOARDING_MISSION_STEP, + }; await rerender(); await settle(); await act(async () => resolveCreate({ id: "goal-company-1" })); @@ -566,7 +583,15 @@ describe("OnboardingWizard — which step it lands on", () => { // a goal id behind, and the company created next would read it as // "mission already written" and never be asked for one. mockGoalsApi.create.mockResolvedValue({ id: "goal-company-1" }); + // Reached explicitly: the route no longer lands here. The withdrawal this + // defends against is still route-driven, so the route is set too — it takes + // over the moment the explicit open is released. routerState.pathname = "/PC1/onboarding"; + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId: "company-1", + initialStep: ONBOARDING_MISSION_STEP, + }; await render(); await settle(); @@ -613,9 +638,6 @@ describe("OnboardingWizard — which step it lands on", () => { )!, ); await settle(); - setControlledValue(missionTextarea()!, "Acme's mission"); - await settle(); - await click(confirmMissionButton()!); await settle(); expect(mockCompaniesApi.create).toHaveBeenCalled(); expect(currentStep()).toBe("agent"); @@ -676,7 +698,8 @@ describe("OnboardingWizard — which step it lands on", () => { await render(); await settle(); - // Step 1: name a new company, then confirm the mission to create it. + // Step 1 creates the company on its own now — the mission step used to do + // it, and no longer runs. const nameInput = document.body.querySelector("input")! as HTMLInputElement; setControlledValue(nameInput, "Initech"); await settle(); @@ -686,12 +709,6 @@ describe("OnboardingWizard — which step it lands on", () => { await act(async () => { next.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); - await settle(); - setControlledValue(missionTextarea()!, "Initech's mission"); - await settle(); - await act(async () => { - confirmMissionButton()!.dispatchEvent(new MouseEvent("click", { bubbles: true })); - }); // A route supplies an existing company before the create lands. routerState.pathname = "/PC1/onboarding"; @@ -702,12 +719,15 @@ describe("OnboardingWizard — which step it lands on", () => { await settle(); // Adopting the created company would select it globally and take the - // customer off the one they navigated to. Asserted on the selection call - // rather than on the rendered name: the name reads "Acme" either way, - // because the switch reset clears it and the backfill refills it from the - // company list, which has no entry for the company just created. + // customer off the one they navigated to. The selection call is the + // assertion; it always was, and the author of this test said so. + // + // The rendered name used to back it up, but the wizard lands on the agent + // step now and that step names no company. Anchored on the step instead, so + // a selection call that never happened because nothing rendered would fail + // here rather than read as a pass. + expect(currentStep()).toBe("agent"); expect(companyState.setSelectedCompanyId).not.toHaveBeenCalled(); - expect(document.body.textContent).toContain("Acme"); }); it("applies the step again when the wizard is re-opened", async () => { diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index cb7e184721..1159bf65f6 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -195,6 +195,146 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( vi.clearAllMocks(); }); + describe("step 2, which is two screens wearing one number", () => { + // The create path's step 2 was the mission question and is skipped now. The + // grow path's step 2 is "tell us about your team", whose answers seed the + // lead agent — a different screen that happens to share the number, and one + // nothing covered until skipping the first nearly took it along. + + async function openStepOne(path: "create" | "grow") { + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ step: 1, onboardingPath: path, companyName: "Initech" }), + ); + mockDialog.onboardingOptions = {}; + mockCompany.companies = []; + mockCompany.loading = false; + mockCompaniesApi.list.mockResolvedValue([]); + + const { root, queryClient } = render(); + const renderTree = () => + act(async () => { + root.render( + + + , + ); + }); + await renderTree(); + await flushReact(); + return { root, renderTree }; + } + + async function clickByText(match: (text: string) => boolean) { + const el = [...document.body.querySelectorAll("button")].find((b) => + match(b.textContent?.trim() ?? ""), + )!; + await act(async () => { + el.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await flushReact(); + } + + it("keeps the grow path's questionnaire", async () => { + const { root } = await openStepOne("grow"); + await clickByText((t) => t.startsWith("Next")); + + expect(document.body.textContent).toContain("Tell us about your team"); + expect(mockCompaniesApi.create).not.toHaveBeenCalled(); + + await act(async () => root.unmount()); + }); + + it("skips it on the create path, creating the company on the way", async () => { + mockCompaniesApi.create.mockResolvedValue({ id: "company-new", issuePrefix: "INI" }); + const { root } = await openStepOne("create"); + await clickByText((t) => t.startsWith("Next")); + + expect(mockCompaniesApi.create).toHaveBeenCalledWith({ name: "Initech" }); + expect(document.body.textContent).toContain("Create your first agent"); + expect(document.body.textContent).not.toContain("Define your mission"); + + await act(async () => root.unmount()); + }); + + it("creates one company for one keystroke, modifier or not", async () => { + // The name field handles Enter itself and does not check for a modifier, + // so Cmd+Enter in that field reaches the field's handler *and* the + // wizard's step-level one. Both would start creating. The step-level + // `loading` guard cannot stop it — `setLoading(true)` has not landed + // while the same event is still bubbling — so the second caller reads a + // value the first has not written. Two companies, one keystroke. + mockCompaniesApi.create.mockResolvedValue({ id: "company-new", issuePrefix: "INI" }); + const { root } = await openStepOne("create"); + + const nameInput = document.body.querySelector( + 'input[placeholder="Acme Corp"]', + ) as HTMLInputElement; + await act(async () => { + nameInput.dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", metaKey: true, bubbles: true }), + ); + }); + await flushReact(); + + expect(mockCompaniesApi.create).toHaveBeenCalledTimes(1); + expect(document.body.textContent).toContain("Create your first agent"); + + await act(async () => root.unmount()); + }); + + it("creates one company however many times Enter repeats", async () => { + // Holding Enter down fires keydown repeatedly. Each one is a separate + // event, so `defaultPrevented` says nothing about the others, and neither + // `loading` nor `createdCompanyId` has been written by the time the next + // arrives — the first is state, the second is not set until the request + // it guards resolves. Only a ref written before the request goes out is + // visible to the caller behind it. + let resolveCreate: (c: { id: string; issuePrefix: string }) => void = () => {}; + mockCompaniesApi.create.mockReturnValue( + new Promise<{ id: string; issuePrefix: string }>((resolve) => { + resolveCreate = resolve; + }), + ); + const { root } = await openStepOne("create"); + + const nameInput = document.body.querySelector( + 'input[placeholder="Acme Corp"]', + ) as HTMLInputElement; + await act(async () => { + for (let i = 0; i < 4; i++) { + nameInput.dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", bubbles: true }), + ); + } + }); + + expect(mockCompaniesApi.create).toHaveBeenCalledTimes(1); + + await act(async () => resolveCreate({ id: "company-new", issuePrefix: "INI" })); + await flushReact(); + expect(document.body.textContent).toContain("Create your first agent"); + + await act(async () => root.unmount()); + }); + + it("sends Back to the screen the run actually came from", async () => { + // A create run reached the agent step from step 1, so Back owes it step 1 — + // not the mission screen it never saw. + mockCompaniesApi.create.mockResolvedValue({ id: "company-new", issuePrefix: "INI" }); + const { root } = await openStepOne("create"); + await clickByText((t) => t.startsWith("Next")); + expect(document.body.textContent).toContain("Create your first agent"); + + await clickByText((t) => t.includes("Back")); + + expect(document.body.textContent).toContain("Name your organization"); + expect(document.body.textContent).not.toContain("Define your mission"); + + await act(async () => root.unmount()); + }); + }); + it("re-syncs a restored draft once companies resolve asynchronously (companies start empty/loading)", async () => { // Regression for the initializer-only restore bug: the inner wizard's // ~20 useState(saved?.x ?? default) initializers only read `saved` on diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 25c4af41cd..146fcf5cb7 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -350,20 +350,18 @@ function OnboardingWizardInner({ (company) => company.issuePrefix.toUpperCase() === companyPrefix.toUpperCase(), )?.id ?? null : null; - const { hasMission: routeCompanyHasMission, settled: routeMissionSettled } = - useCompanyMission(routeMatchedCompanyId); - - // Hold the options back until the mission lookup settles, exactly as they - // are already held back while companies load. The step below is applied once - // and not revised, so the wizard must not open before the answer is in. + // The mission lookup used to gate this: the step was applied once and not + // revised, so opening before the answer arrived left the customer on the + // wrong step. The step no longer depends on the answer, so the wait bought + // nothing but a slower open. Companies still gate it — the resolver needs + // them to match the prefix at all. const routeOnboardingOptions = - (companyPrefix && companiesLoading) || !routeMissionSettled + companyPrefix && companiesLoading ? null : resolveRouteOnboardingOptions({ pathname: location.pathname, companyPrefix, companies, - companyHasMission: routeCompanyHasMission, }); const effectiveOnboardingOpen = onboardingOpen || (routeOnboardingOptions !== null && !routeDismissed); @@ -457,6 +455,12 @@ function OnboardingWizardInner({ // every company change, and the effect also calls setStep - it would drag // the user back to the route's initial step mid-flow. const createdCompanyIdRef = useRef(null); + // In flight, synchronously. `loading` cannot answer this: it is state, so a + // second caller in the same tick — key repeat holding Enter down — reads the + // value the first has not written yet. `createdCompanyId` cannot answer it + // either, because it is not set until the request it guards has resolved. A + // ref is written before the request goes out, so the second caller sees it. + const creatingCompanyRef = useRef(false); createdCompanyIdRef.current = createdCompanyId; // The mission of the company actually in hand, which is not always the one @@ -1190,6 +1194,54 @@ function OnboardingWizardInner({ } } + // Step 1 → 3 ("Name your company"): create the company, then go straight to + // the first agent. + // + // This work used to live at the end of `handleConfirmMission`, because step 1 + // led to the mission step and the company was created when that step was + // confirmed. Onboarding no longer asks for the mission, so step 1 has to do + // its own creating — routing 1 → 3 without this left the wizard on the agent + // step with no company to hire into, and nothing said so. + // + // No goal is written here. That is the difference from the path this was + // taken from, and it is deliberate: the mission is collected later, in the + // tenant app, so writing an empty one now would only give the company a goal + // it did not choose. + async function handleCreateCompany() { + if (createdCompanyId) { + setStep(3); + return; + } + if (creatingCompanyRef.current) return; + creatingCompanyRef.current = true; + setLoading(true); + setError(null); + try { + const company = await companiesApi.create({ name: companyName.trim() }); + queryClient.invalidateQueries({ queryKey: queryKeys.companies.all }); + // Nothing was in hand when this started, so "unchanged" means still + // nothing. A route that supplied a company while the request was open has + // taken over the wizard, and adopting the company just created would + // fight it — and would leave the customer on a company they never + // navigated to. + if (!stillTheSameCompany(null)) return; + setCreatedCompanyId(company.id); + // Keep the mirror current rather than waiting for the next render, for + // the same reason the mission path does: anything downstream that asks + // `stillTheSameCompany` in this tick would otherwise be told no. + createdCompanyIdRef.current = company.id; + setCreatedCompanyPrefix(company.issuePrefix); + setSelectedCompanyId(company.id); + setStep(3); + } catch (err) { + setError(err instanceof Error ? err.message : "Failed to create company"); + } finally { + creatingCompanyRef.current = false; + setLoading(false); + } + } + + // Step 4 → 5 ("Give it a heartbeat"): hire the lead agent + seed its // instructions, then advance to Review. Guarded so revisiting step 4 // doesn't hire a second agent. @@ -1368,6 +1420,14 @@ function OnboardingWizardInner({ } function handleKeyDown(e: React.KeyboardEvent) { + // Something nearer the key already dealt with it. The company-name field + // handles Enter itself and does not check for a modifier, so Cmd+Enter in + // that field reaches both handlers — and both would start creating a + // company. The `loading` guard below cannot catch that: `setLoading(true)` + // has not landed while the same event is still bubbling, so the second + // caller reads the value the first one has not written yet. Two companies, + // one keystroke. + if (e.defaultPrevented) return; if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) { e.preventDefault(); // Every button below is disabled while a request is in flight. The @@ -1376,7 +1436,10 @@ function OnboardingWizardInner({ // yet — two goals for one mission, two agents for one hire. if (loading) return; if (step === 0) return; // front door requires click - if (step === 1 && companyName.trim()) setStep(2); + if (step === 1 && companyName.trim()) { + if (skipsMissionStep) void handleCreateCompany(); + else setStep(2); + } else if (step === 2 && companyName.trim() && companyGoal.trim()) handleConfirmMission(); else if (step === 3 && agentName.trim()) setStep(4); else if (step === 4 && agentName.trim() && !missionUnresolvedForHire) @@ -1390,6 +1453,24 @@ function OnboardingWizardInner({ // The arc strip stands in for the full-length bar only when the run began on // the arc — the Cloud-first path, where the company already exists and steps // 1-2 never happen. A run that started at step 1 keeps one continuous count. + // Step 2 is two different screens wearing one number: the grow path's "tell us + // about your team" questionnaire, and the create path's mission step. + // Onboarding stopped asking for the mission, but the questionnaire is still + // how a grow run describes the team it is levelling up — its answers seed the + // lead agent — so only the create path skips ahead. + const skipsMissionStep = onboardingPath !== "grow"; + + // Back lands on whatever came before this step *for this run*, which is not + // always `step - 1`. A create run went 1 → 3, so stepping blindly would walk + // it into the mission screen it never saw. Two runs still belong on step 2 + // going back: a grow run, whose step 2 is the questionnaire rather than the + // mission, and a run that *entered* on the mission step because something + // opened it there — it has seen that screen, so Back owes it the way back. + function backStepFrom(current: Step): Step { + if (current === 3 && skipsMissionStep && entryStep !== 2) return 1; + return (current - 1) as Step; + } + const isAgentArcStep = agentArcStepFor(step) !== null; const showsAgentArcStepper = isAgentArcStep && entryStep >= 3; @@ -1450,15 +1531,19 @@ function OnboardingWizardInner({ : "w-full max-w-md px-8 py-12", )} > - {/* 5-segment progress bar (brand .wsteps/.wstep) — segment N + {/* Full-length progress bar (brand .wsteps/.wstep) — segment N filled once step ≥ N. Completed segments jump back. Hidden for a run that entered on the agent arc: the arc strip below counts that run's three steps, and showing both put two progress bars on the same screen. A run that started at step 1 - keeps this one throughout, so its count never restarts. */} + keeps this one throughout, so its count never restarts. + + Step 2 is absent: onboarding no longer asks for the mission, so + a segment for it would be one the run can never fill, and the + count would visibly skip from 1 to 3. */} {!showsAgentArcStepper && (
- {([1, 2, 3, 4, 5] as const).map((s) => { + {([1, 3, 4, 5] as const).map((s) => { const filled = step >= s; const canJump = canJumpToOnboardingStep({ targetStep: s, @@ -1683,8 +1768,8 @@ function OnboardingWizardInner({ onKeyDown={(e) => { if (e.key === "Enter" && companyName.trim()) { e.preventDefault(); - if (onboardingPath !== "grow" && !missionPath) setMissionPath("direct"); - setStep(2); + if (skipsMissionStep) void handleCreateCompany(); + else setStep(2); } }} autoFocus @@ -2349,7 +2434,7 @@ function OnboardingWizardInner({ setStep((step - 1) as Step) + ? () => setStep(backStepFrom(step)) : undefined } // The prototype's cloud flow hires on this step and calls the @@ -2382,7 +2467,7 @@ function OnboardingWizardInner({ diff --git a/ui/src/lib/onboarding-route.test.ts b/ui/src/lib/onboarding-route.test.ts index e382084908..985a53f47e 100644 --- a/ui/src/lib/onboarding-route.test.ts +++ b/ui/src/lib/onboarding-route.test.ts @@ -42,7 +42,7 @@ describe("resolveRouteOnboardingOptions", () => { companyPrefix: "pap", companies: [{ id: "company-1", issuePrefix: "PAP" }], }), - ).toEqual({ initialStep: 2, companyId: "company-1" }); + ).toEqual({ initialStep: ONBOARDING_AGENT_STEP, companyId: "company-1" }); }); it("falls back to company creation when the prefixed company is missing", () => { @@ -152,7 +152,7 @@ describe("companyPrefixFromOnboardingPath", () => { companyPrefix: companyPrefixFromOnboardingPath(pathname), companies, }), - ).toEqual({ initialStep: 2, companyId: "c1" }); + ).toEqual({ initialStep: ONBOARDING_AGENT_STEP, companyId: "c1" }); }); }); @@ -170,7 +170,7 @@ describe("navigating away from a company's onboarding route", () => { companyPrefix: companyPrefixFromOnboardingPath("/PC1/onboarding"), companies, }); - expect(onCompanyRoute).toEqual({ initialStep: 2, companyId: "c1" }); + expect(onCompanyRoute).toEqual({ initialStep: ONBOARDING_AGENT_STEP, companyId: "c1" }); const afterNavigating = resolveRouteOnboardingOptions({ pathname: "/onboarding", @@ -244,41 +244,32 @@ describe("shouldRouteAgentlessCompanyToOnboarding", () => { describe("resolveRouteOnboardingOptions — the agent step", () => { const companies = [{ id: "c1", issuePrefix: "PC1" }]; - it("opens a company that already has its mission on the agent step", () => { - // Cloud collected the mission at signup and the seed wrote it as a - // company-level goal. Re-asking it is the seam the seeded arc removes. + it("opens an existing company on the agent step", () => { expect( resolveRouteOnboardingOptions({ pathname: "/PC1/onboarding", companyPrefix: "PC1", companies, - companyHasMission: true, }), ).toEqual({ initialStep: ONBOARDING_AGENT_STEP, companyId: "c1" }); }); - it("still asks for the mission when the company has none", () => { - expect( - resolveRouteOnboardingOptions({ - pathname: "/PC1/onboarding", - companyPrefix: "PC1", - companies, - companyHasMission: false, - }), - ).toEqual({ initialStep: 2, companyId: "c1" }); - }); - - it("treats an unknown mission as absent while the goal query is in flight", () => { - // Costing the mission step is recoverable; skipping a question that was - // never answered leaves the company without one. - expect( - resolveRouteOnboardingOptions({ - pathname: "/PC1/onboarding", - companyPrefix: "PC1", - companies, - companyHasMission: undefined, - }), - ).toEqual({ initialStep: 2, companyId: "c1" }); + it("takes no mission input, so no goal state can route around the agent step", () => { + // Three cases used to live here — mission present, absent, and in flight — + // and two of them routed to the mission step. The resolver no longer + // accepts the input at all, which is what collapses them into one: there is + // no value a caller could pass that reaches a different step. + // + // Asserted on the accepted keys rather than by example, because the + // property being defended is that nothing can be passed. A test that tried + // would not fail, it would not compile. + const resolved = resolveRouteOnboardingOptions({ + pathname: "/PC1/onboarding", + companyPrefix: "PC1", + companies, + }); + expect(resolved).toEqual({ initialStep: ONBOARDING_AGENT_STEP, companyId: "c1" }); + expect(resolved!.initialStep).not.toBe(ONBOARDING_MISSION_STEP); }); it("keeps sending an unmatched prefix to company creation", () => { @@ -287,26 +278,27 @@ describe("resolveRouteOnboardingOptions — the agent step", () => { pathname: "/NOPE/onboarding", companyPrefix: "NOPE", companies, - companyHasMission: true, }), ).toEqual({ initialStep: 1 }); }); }); describe("onboardingStepForCompany", () => { - it("skips the mission question for a company that has one", () => { - expect(onboardingStepForCompany(true)).toBe(ONBOARDING_AGENT_STEP); + it("opens an existing company on the agent step", () => { + expect(onboardingStepForCompany()).toBe(ONBOARDING_AGENT_STEP); }); - it("asks for the mission when the company has none", () => { - expect(onboardingStepForCompany(false)).toBe(ONBOARDING_MISSION_STEP); - }); - - it("asks for the mission when the lookup has not answered", () => { - // Both an in-flight and a failed lookup arrive here as `undefined`. - // Costing the mission step is recoverable — the customer answers it. The - // opposite error skips a question nobody answered and leaves the company - // without a mission. - expect(onboardingStepForCompany(undefined)).toBe(ONBOARDING_MISSION_STEP); + it("never opens on the mission step, whatever the company's goals say", () => { + // This used to branch on whether the company had a mission, and a company + // without one was sent to ask for it first. Onboarding stopped asking — the + // mission is collected later, in the tenant app — but the branch outlived + // the question, and because Cloud's naming screen had dropped its own + // mission field, *every* Cloud-created company arrived looking mission-less + // and took the detour. + // + // There is no argument left to vary, which is the point: the assertion is + // that the mission step is unreachable from here rather than merely + // unlikely. + expect(onboardingStepForCompany()).not.toBe(ONBOARDING_MISSION_STEP); }); }); diff --git a/ui/src/lib/onboarding-route.ts b/ui/src/lib/onboarding-route.ts index b27f3b702f..fe3f5f65e9 100644 --- a/ui/src/lib/onboarding-route.ts +++ b/ui/src/lib/onboarding-route.ts @@ -57,30 +57,30 @@ export type ExistingCompanyOnboardingStep = | typeof ONBOARDING_AGENT_STEP; /** - * The step a company that already exists belongs on. + * The step a company that already exists belongs on: always the agent. * - * `undefined` means the mission is not known: the goal lookup has not answered - * yet, or it failed. Both read as "no mission". That direction of error costs - * the customer the mission step, which they can answer; the opposite would - * skip a question nobody answered and leave the company without a mission. + * This used to branch on whether the company had a mission, sending a company + * without one to `ONBOARDING_MISSION_STEP` first. Onboarding no longer asks — + * the mission is collected later, in the tenant app — so the branch had become + * a way to reach a question the arc had stopped asking. It survived because + * Cloud's naming screen dropped its own mission field, which left every + * Cloud-created company looking mission-less on arrival, and so every walk + * detoured through a step the design had removed. + * + * The parameter is gone rather than ignored: callers were running a goal lookup + * to compute it, and an argument that cannot change the answer is an invitation + * to keep computing it. */ -export function onboardingStepForCompany( - companyHasMission: boolean | undefined, -): ExistingCompanyOnboardingStep { - return companyHasMission === true ? ONBOARDING_AGENT_STEP : ONBOARDING_MISSION_STEP; +export function onboardingStepForCompany(): ExistingCompanyOnboardingStep { + return ONBOARDING_AGENT_STEP; } export function resolveRouteOnboardingOptions(params: { pathname: string; companyPrefix?: string; companies: OnboardingRouteCompany[]; - /** - * Whether the matched company already has its mission (a company-level - * goal). See {@link onboardingStepForCompany} for what `undefined` means. - */ - companyHasMission?: boolean; }): { initialStep: 1 | ExistingCompanyOnboardingStep; companyId?: string } | null { - const { pathname, companyPrefix, companies, companyHasMission } = params; + const { pathname, companyPrefix, companies } = params; if (!isOnboardingPath(pathname)) return null; @@ -99,7 +99,7 @@ export function resolveRouteOnboardingOptions(params: { } return { - initialStep: onboardingStepForCompany(companyHasMission), + initialStep: onboardingStepForCompany(), companyId: matchedCompany.id, }; } diff --git a/ui/src/pages/Dashboard.tsx b/ui/src/pages/Dashboard.tsx index 9fc5e9859f..d207a4dbd2 100644 --- a/ui/src/pages/Dashboard.tsx +++ b/ui/src/pages/Dashboard.tsx @@ -4,7 +4,6 @@ import { onboardingStepForCompany, shouldRouteAgentlessCompanyToOnboarding, } from "../lib/onboarding-route"; -import { useCompanyMission } from "../hooks/useCompanyMission"; import { claimOnboardingOffer } from "../lib/onboarding-auto-open"; import { Link } from "@/lib/router"; import { useQuery } from "@tanstack/react-query"; @@ -79,8 +78,6 @@ export function Dashboard() { // start the wizard at the front door with no company, and the new-company // path there would create a *second* company instead of giving this one an // agent. - const { hasMission: companyHasMission, settled: missionSettled } = - useCompanyMission(selectedCompanyId); const shouldOpenOnboarding = shouldRouteAgentlessCompanyToOnboarding({ pathname: location.pathname, agentsLoaded: agents !== undefined, @@ -93,16 +90,14 @@ export function Dashboard() { // companies already offered; see it for why that outlives this component. useEffect(() => { if (!shouldOpenOnboarding || !selectedCompanyId) return; - // Wait for the mission lookup to settle before opening: the wizard applies - // the step it is given once, so a step chosen before the answer is in is - // the step the customer is left on. - if (!missionSettled) return; if (!claimOnboardingOffer(selectedCompanyId)) return; openOnboarding({ companyId: selectedCompanyId, - initialStep: onboardingStepForCompany(companyHasMission), + initialStep: onboardingStepForCompany(), }); - }, [shouldOpenOnboarding, selectedCompanyId, missionSettled, companyHasMission, openOnboarding]); + // No mission lookup to wait on any more: the step this opens is the same + // whatever the goals say, so waiting only delayed the open. + }, [shouldOpenOnboarding, selectedCompanyId, openOnboarding]); useEffect(() => { setBreadcrumbs([{ label: "Dashboard" }]); @@ -277,7 +272,7 @@ export function Dashboard() {