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() {