From d95340b0b84ad0bc5a53bdd784fd090f2fba7ecc Mon Sep 17 00:00:00 2001 From: Tonio Date: Fri, 14 Aug 2026 08:18:07 -0700 Subject: [PATCH] feat(ui): send a company with no agent into onboarding, at the right step (#11352) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A company with no agent cannot do anything: no runs, no tasks, nothing to show. The dashboard says so in a banner with a link, which asks the customer to notice a problem the product can fix for them. It is worse for a company created by Paperclip Cloud: Cloud creates the company before the tenant boots, so the companyless redirect never runs, and the customer arrives on an empty dashboard straight out of a signup flow that already asked for a mission. The dashboard now opens onboarding when the agent list has loaded and is empty, and onboarding opens on the agent step when the company already has its mission — read from the company-level goal the seed writes, under the query key the launch path already uses, so it shares a cache entry rather than adding a fetch. The step is decided once. `initialStep` is derived from the company list and the goal list, so it changes on any retry, refetch or cache invalidation. An effect that took it as a dependency called `setStep` on every one of those and moved a customer who was already mid-flow. Gating the input only narrowed that window; it could not close it. The step now belongs to the request that opened the wizard: the effect reads it through a ref and is keyed on the wizard opening or the company changing. `createdCompanyIdRef` beside it already used this pattern for the same reason. That exposed a path nothing had ever taken. A company reached the mission step only by creating itself on step 1, so opening an existing company there found code that had never run: `companyName` is only typed on step 1, and both ways forward require it, so the step could not be completed at all; and confirming advanced without writing anything, so the mission the customer typed was discarded. Both fixed, and the write now reconciles against the goal list rather than adding a second company goal, since the mission lookup fails open and can send a company that has one back to that step. Company-scoped state now stays with its company. `clearCompanyScopedState` runs when the route replaces a company and when it withdraws one — the same event, and clearing half of it left a goal id that made the next company skip a mission it had never given. `stillTheSameCompany` guards all five async writes, after the server work rather than before it, so a company switch mid-flight cannot hand the new company the old one's goal, project, issue or agent, and cannot leave a hired agent without its instructions file. The keyboard path honours `loading` like every button already did. `claimOnboardingOffer` makes onboarding an offer that stays declined for the visit. Route ownership is now recorded whenever the route names a company, including one the wizard already holds. This changes a documented rule deliberately: without it a self-created company was never withdrawn, so `/onboarding` would show "create a company" while still holding the previous one and write the customer's new mission into it. Tested at the seam, because every defect on this branch lived between a value and its consumer and the predicate tests passed at every stage. `OnboardingWizard.step.test.tsx` renders the real wizard against the real resolver and the real mission hook across 18 cases, and each was fault-injected against the code it replaces rather than trusted on a green run. That caught a case that passed against the broken code, and a race in one of the guards. ui typecheck clean; full ui suite 3923 pass, with one timezone-dependent IssueProperties failure present on this branch's base in a file this change does not touch. All CI gates green; Greptile 5/5. Co-Authored-By: Claude Opus 5 --- .../components/OnboardingWizard.step.test.tsx | 705 ++++++++++++++++++ ui/src/components/OnboardingWizard.tsx | 239 +++++- ui/src/hooks/useCompanyMission.test.tsx | 126 ++++ ui/src/hooks/useCompanyMission.ts | 50 ++ ui/src/lib/onboarding-auto-open.test.ts | 27 + ui/src/lib/onboarding-auto-open.ts | 30 + ui/src/lib/onboarding-route.test.ts | 123 +++ ui/src/lib/onboarding-route.ts | 77 +- ui/src/pages/Dashboard.tsx | 51 ++ 9 files changed, 1406 insertions(+), 22 deletions(-) create mode 100644 ui/src/components/OnboardingWizard.step.test.tsx create mode 100644 ui/src/hooks/useCompanyMission.test.tsx create mode 100644 ui/src/hooks/useCompanyMission.ts create mode 100644 ui/src/lib/onboarding-auto-open.test.ts create mode 100644 ui/src/lib/onboarding-auto-open.ts diff --git a/ui/src/components/OnboardingWizard.step.test.tsx b/ui/src/components/OnboardingWizard.step.test.tsx new file mode 100644 index 0000000000..e4ac714472 --- /dev/null +++ b/ui/src/components/OnboardingWizard.step.test.tsx @@ -0,0 +1,705 @@ +// @vitest-environment jsdom + +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { queryKeys } from "../lib/queryKeys"; +import { + ONBOARDING_AGENT_STEP, + ONBOARDING_MISSION_STEP, +} from "../lib/onboarding-route"; + +/** + * Which step the onboarding wizard *lands on*, and what is allowed to move it + * afterwards. + * + * These are seam tests on purpose. `initialStep` is derived from two queries + * and consumed by an effect that calls `setStep`, and every defect this file + * guards lived in that seam rather than in either side of it — the pure + * helpers in `onboarding-route.test.ts` passed while the wizard was moving a + * customer off the step they were typing on. So the real component is rendered + * here, with the real route resolver and the real mission hook, and only the + * network and the surrounding contexts are stubbed. + */ + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +const mockGoalsApi = vi.hoisted(() => ({ + list: vi.fn(), + create: vi.fn(), + update: vi.fn(), +})); +const mockAdaptersApi = vi.hoisted(() => ({ list: vi.fn() })); +const mockCompaniesApi = vi.hoisted(() => ({ create: vi.fn() })); + +const routerState = vi.hoisted(() => ({ pathname: "/" })); +const dialogState = vi.hoisted(() => ({ + onboardingOpen: false, + onboardingOptions: {} as { initialStep?: number; companyId?: string }, + onboardingRouteDismissed: false, + closeOnboarding: vi.fn(), + setOnboardingRouteDismissed: vi.fn(), +})); +const companyState = vi.hoisted(() => ({ + companies: [ + { id: "company-1", name: "Acme", issuePrefix: "PC1" }, + { id: "company-2", name: "Globex", issuePrefix: "PC2" }, + ], + loading: false, + setSelectedCompanyId: vi.fn(), +})); + +vi.mock("../api/goals", () => ({ goalsApi: mockGoalsApi })); +vi.mock("@/api/adapters", () => ({ adaptersApi: mockAdaptersApi })); +vi.mock("../api/companies", () => ({ companiesApi: mockCompaniesApi })); +vi.mock("../api/agents", () => ({ + agentsApi: { create: vi.fn(), adapterModels: vi.fn().mockResolvedValue([]) }, +})); +vi.mock("../api/approvals", () => ({ approvalsApi: { create: vi.fn() } })); +vi.mock("../api/issues", () => ({ issuesApi: { create: vi.fn() } })); +vi.mock("../api/projects", () => ({ projectsApi: { list: vi.fn(), create: vi.fn() } })); + +vi.mock("@/lib/router", () => ({ + useLocation: () => ({ pathname: routerState.pathname }), + useNavigate: () => vi.fn(), + useParams: () => ({}), +})); + +vi.mock("../context/DialogContext", () => ({ + useDialog: () => dialogState, +})); + +vi.mock("../context/CompanyContext", () => ({ + useCompany: () => companyState, +})); + +// Canvas/animation leaves — nothing to do with the step machinery. +vi.mock("./AsciiArtAnimation", () => ({ AsciiArtAnimation: () => null })); +vi.mock("./AgentCapsule", () => ({ AgentCapsule: () => null })); +vi.mock("./FrontDoor", () => ({ FrontDoor: () => null })); + +const { OnboardingWizard } = await import("./OnboardingWizard"); + +/** The mission step renders this heading; the agent step renders this input. */ +function currentStep(): "mission" | "agent" | "closed" | "other" { + const body = document.body; + if (!body.querySelector("[role='dialog'], .fixed.inset-0")) return "closed"; + const headings = [...body.querySelectorAll("h3")].map((h) => h.textContent); + if (headings.includes("Define your mission")) return "mission"; + if (body.querySelector("input[placeholder='Chief of staff']")) return "agent"; + return "other"; +} + +function confirmMissionButton(): HTMLButtonElement | null { + return ( + [...document.body.querySelectorAll("button")].find((button) => + button.textContent?.includes("Confirm mission"), + ) ?? null + ); +} + +function missionTextarea(): HTMLTextAreaElement | null { + return document.body.querySelector("textarea"); +} + +/** Type into a controlled React input without a full user-event dependency. */ +function setControlledValue(el: HTMLTextAreaElement | HTMLInputElement, value: string) { + const prototype = + el instanceof HTMLTextAreaElement ? HTMLTextAreaElement.prototype : HTMLInputElement.prototype; + Object.getOwnPropertyDescriptor(prototype, "value")!.set!.call(el, value); + el.dispatchEvent(new Event("input", { bubbles: true })); +} + +const COMPANY_GOAL = { + id: "goal-1", + companyId: "company-1", + title: "Ship the thing", + description: null, + level: "company", + status: "active", + parentId: null, + ownerAgentId: null, + createdAt: new Date("2026-03-02T00:00:00Z"), + updatedAt: new Date("2026-03-02T00:00:00Z"), +}; + +describe("OnboardingWizard — which step it lands on", () => { + let container: HTMLDivElement; + let queryClient: QueryClient; + let root: Root | null = null; + + async function render() { + root = createRoot(container); + await act(async () => { + root!.render( + + + , + ); + }); + } + + /** Re-render after mutating the stubbed contexts or the location. */ + async function rerender() { + await act(async () => { + root!.render( + + + , + ); + }); + } + + // React Query resolves through microtasks and React schedules the re-render + // after them, so a single tick is not reliably enough under load. Several + // ticks cost microseconds and remove the ordering sensitivity. + async function settle(ticks = 12) { + for (let i = 0; i < ticks; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + } + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + // The wizard restores its step from localStorage, so a step left behind by + // an earlier case would decide the next one. + localStorage.clear(); + routerState.pathname = "/"; + dialogState.onboardingOpen = false; + dialogState.onboardingOptions = {}; + dialogState.onboardingRouteDismissed = false; + mockAdaptersApi.list.mockResolvedValue([]); + mockGoalsApi.list.mockResolvedValue([]); + }); + + afterEach(async () => { + await act(async () => root?.unmount()); + root = null; + queryClient.clear(); + container.remove(); + document.body.innerHTML = ""; + vi.clearAllMocks(); + }); + + it("opens a company that already has its mission on the agent step", async () => { + // The point of the change: Cloud collected the mission at signup and the + // seed wrote it as a company-level goal, so asking for it again asks a + // question the customer answered minutes earlier on another origin. + routerState.pathname = "/PC1/onboarding"; + mockGoalsApi.list.mockResolvedValue([COMPANY_GOAL]); + await render(); + await settle(); + + 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. + routerState.pathname = "/PC1/onboarding"; + mockGoalsApi.list.mockReturnValue(new Promise(() => {})); + await render(); + + expect(currentStep()).toBe("closed"); + }); + + 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. + routerState.pathname = "/PC1/onboarding"; + mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); + await render(); + await settle(); + + expect(currentStep()).toBe("mission"); + }); + + 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. + routerState.pathname = "/PC1/onboarding"; + mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); + await render(); + await settle(); + expect(currentStep()).toBe("mission"); + + await act(async () => { + queryClient.setQueryData(queryKeys.goals.list("company-1"), [COMPANY_GOAL]); + }); + await settle(); + + expect(currentStep()).toBe("mission"); + }); + + it("does not move an open wizard when the dialog is re-opened with a new step", async () => { + // The dashboard's auto-open sits behind queries too, so a refetch can call + // `openOnboarding` again with a different step for the same company. The + // wizard belongs to the customer by then. + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId: "company-1", + initialStep: ONBOARDING_MISSION_STEP, + }; + await render(); + await settle(); + expect(currentStep()).toBe("mission"); + + dialogState.onboardingOptions = { + companyId: "company-1", + initialStep: ONBOARDING_AGENT_STEP, + }; + await rerender(); + await settle(); + + 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. + 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"); + + routerState.pathname = "/PC2/onboarding"; + await rerender(); + await settle(); + + expect(currentStep()).toBe("agent"); + }); + + describe("the mission step, reached with a company that already exists", () => { + // Nothing sent an existing company here until the dashboard started + // opening agentless ones on this step. Both defects below were reachable + // the moment it did. + + async function openOnMissionStepForExistingCompany() { + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId: "company-1", + 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 })); + }); + } + + it("names the company it is asking about, so the step can be completed", async () => { + // `companyName` is only ever typed on step 1. Without a backfill it is + // empty here, the step's own copy has a blank where the name goes, and + // "Confirm mission" stays disabled — a customer sent to this step could + // not leave it. + await openOnMissionStepForExistingCompany(); + + expect(document.body.textContent).toContain("Acme"); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Ship the thing"); + await settle(); + + expect(confirmMissionButton()?.disabled).toBe(false); + }); + + it("saves the mission it asked for", async () => { + // Confirming used to advance to the agent step and write nothing, so the + // company kept no mission — the exact state this change exists to remove. + mockGoalsApi.create.mockResolvedValue({ id: "goal-new" }); + await openOnMissionStepForExistingCompany(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Ship the thing"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ title: "Ship the thing", level: "company", status: "active" }), + ); + expect(currentStep()).toBe("agent"); + }); + + it("does not write a second mission when Enter is pressed twice", async () => { + // The buttons are all disabled while a request is in flight; the + // keyboard has to be too. A second Enter re-enters the handler before + // the first has set the goal id its own guard reads, so both requests + // see "no mission yet" and the company ends up with two. + let resolveCreate: (goal: { id: string }) => void = () => {}; + mockGoalsApi.create.mockReturnValue( + new Promise<{ id: string }>((resolve) => { + resolveCreate = resolve; + }), + ); + await openOnMissionStepForExistingCompany(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Ship the thing"); + await settle(); + + const surface = document.body.querySelector(".fixed.inset-0.z-50.flex")!; + const submit = () => + act(async () => { + surface.dispatchEvent( + new KeyboardEvent("keydown", { key: "Enter", metaKey: true, bubbles: true }), + ); + }); + await submit(); + await submit(); + await act(async () => resolveCreate({ id: "goal-new" })); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenCalledTimes(1); + }); + + it("updates the mission it could not see, rather than adding a second", async () => { + // The cost of failing open. The lookup could not answer, so the customer + // was asked for a mission the company already had. Adding a goal would + // leave two active company-level goals, and the earlier one would keep + // winning `selectDefaultCompanyGoalId` outside this wizard — so the + // mission the customer just typed would lose. Their answer wins instead. + // The dashboard's lookup failed, which is why this company is on the + // mission step at all. By the time the customer confirms, the goal list + // reads — and it has a mission. + mockGoalsApi.list.mockResolvedValue([COMPANY_GOAL]); + mockGoalsApi.update.mockResolvedValue({ id: COMPANY_GOAL.id }); + await openOnMissionStepForExistingCompany(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "The mission they just typed"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).not.toHaveBeenCalled(); + expect(mockGoalsApi.update).toHaveBeenCalledWith( + COMPANY_GOAL.id, + expect.objectContaining({ title: "The mission they just typed" }), + ); + expect(currentStep()).toBe("agent"); + }); + + it("still writes the mission when the pre-write read also fails", async () => { + // Fail-open all the way down. If it cannot tell whether a mission + // exists, an unwritten mission is the worse error. + mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); + mockGoalsApi.create.mockResolvedValue({ id: "goal-new" }); + await openOnMissionStepForExistingCompany(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Ship the thing"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenCalledWith( + "company-1", + expect.objectContaining({ title: "Ship the thing" }), + ); + expect(currentStep()).toBe("agent"); + }); + + it("does not carry a mission across a switch to another company", async () => { + // Confirming for one company sets the goal id that `handleConfirmMission` + // reads as "already written". Carried across a company switch it makes + // 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"); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Acme's mission"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + expect(currentStep()).toBe("agent"); + + routerState.pathname = "/PC2/onboarding"; + await rerender(); + await settle(); + expect(currentStep()).toBe("mission"); + + const direct2 = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct2); + setControlledValue(missionTextarea()!, "Globex's mission"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenCalledTimes(2); + expect(mockGoalsApi.create).toHaveBeenLastCalledWith( + "company-2", + expect.objectContaining({ title: "Globex's mission" }), + ); + }); + + it("does not hand a new company the mission written for the old one", async () => { + // A route change can switch companies while the write is in flight, and + // the switch clears exactly the state the write is about to set. The + // goal is written and correct either way — but attributing it to the + // company now in hand would undo the clearing and let that company skip + // its own mission. + let resolveCreate: (goal: { id: string }) => void = () => {}; + mockGoalsApi.create.mockReturnValue( + new Promise<{ id: string }>((resolve) => { + resolveCreate = resolve; + }), + ); + routerState.pathname = "/PC1/onboarding"; + await render(); + await settle(); + expect(currentStep()).toBe("mission"); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Acme's mission"); + await settle(); + await click(confirmMissionButton()!); + + // Switch companies before the write lands, then let it land. + routerState.pathname = "/PC2/onboarding"; + await rerender(); + await settle(); + await act(async () => resolveCreate({ id: "goal-company-1" })); + await settle(); + + // Globex must still be asked, and must write its own mission. + expect(currentStep()).toBe("mission"); + mockGoalsApi.create.mockResolvedValue({ id: "goal-company-2" }); + const direct2 = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct2); + setControlledValue(missionTextarea()!, "Globex's mission"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenLastCalledWith( + "company-2", + expect.objectContaining({ title: "Globex's mission" }), + ); + }); + + it("does not carry a mission through a route that withdraws the company", async () => { + // Withdrawing a company and replacing one are the same event: this + // company is no longer the wizard's. Clearing only on replacement leaves + // 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" }); + routerState.pathname = "/PC1/onboarding"; + await render(); + await settle(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Acme's mission"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + expect(currentStep()).toBe("agent"); + + // Navigate to the unprefixed route, which names no company. + routerState.pathname = "/onboarding"; + await rerender(); + await settle(); + + // The wizard is back at company creation with nothing carried over. + const nameInput = document.body.querySelector("input") as HTMLInputElement | null; + expect(nameInput?.value).toBe(""); + expect(document.body.textContent).not.toContain("Acme's mission"); + }); + + it("withdraws a company the wizard created once the route stops naming it", async () => { + // The route only introduces a company when it names one the wizard is + // not already holding, so a company the wizard *created* was never + // recorded as route-owned and was never withdrawn. Visiting its own + // onboarding path and then `/onboarding` left the wizard showing + // "create a company" while still holding it — and the next confirmation + // wrote that customer's new mission into the old company. + mockCompaniesApi.create.mockResolvedValue({ id: "company-1", issuePrefix: "PC1" }); + mockGoalsApi.create.mockResolvedValue({ id: "goal-company-1" }); + routerState.pathname = "/onboarding"; + await render(); + await settle(); + + const nameInput = document.body.querySelector("input")! as HTMLInputElement; + setControlledValue(nameInput, "Acme"); + await settle(); + await click( + [...document.body.querySelectorAll("button")].find( + (b) => b.textContent?.trim() === "Next", + )!, + ); + await settle(); + setControlledValue(missionTextarea()!, "Acme's mission"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + expect(mockCompaniesApi.create).toHaveBeenCalled(); + expect(currentStep()).toBe("agent"); + + // Its own onboarding path, then back to the unprefixed one. + routerState.pathname = "/PC1/onboarding"; + await rerender(); + await settle(); + routerState.pathname = "/onboarding"; + await rerender(); + await settle(); + + const nameAfter = document.body.querySelector("input") as HTMLInputElement | null; + expect(nameAfter?.value).toBe(""); + expect(document.body.textContent).not.toContain("Acme's mission"); + }); + + it("does not write a second mission when the step is confirmed twice", async () => { + mockGoalsApi.create.mockResolvedValue({ id: "goal-new" }); + await openOnMissionStepForExistingCompany(); + + const direct = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("I know my mission"), + )!; + await click(direct); + setControlledValue(missionTextarea()!, "Ship the thing"); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + // Back to the mission step, then forward again. + const back = [...document.body.querySelectorAll("button")].find((b) => + b.textContent?.includes("Back"), + )!; + await click(back); + await settle(); + await click(confirmMissionButton()!); + await settle(); + + expect(mockGoalsApi.create).toHaveBeenCalledTimes(1); + expect(currentStep()).toBe("agent"); + }); + }); + + it("does not adopt a company it created once a route has supplied one", async () => { + // The same guard from the other end. Nothing was in hand when the create + // 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 new company would fight it — and would leave the customer + // on a company they never navigated to. + let resolveCreate: (company: { id: string; issuePrefix: string }) => void = () => {}; + mockCompaniesApi.create.mockReturnValue( + new Promise<{ id: string; issuePrefix: string }>((resolve) => { + resolveCreate = resolve; + }), + ); + routerState.pathname = "/onboarding"; + await render(); + await settle(); + + // Step 1: name a new company, then confirm the mission to create it. + const nameInput = document.body.querySelector("input")! as HTMLInputElement; + setControlledValue(nameInput, "Initech"); + await settle(); + const next = [...document.body.querySelectorAll("button")].find( + (b) => b.textContent?.trim() === "Next", + )!; + 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"; + await rerender(); + await settle(); + expect(mockCompaniesApi.create).toHaveBeenCalledWith({ name: "Initech" }); + await act(async () => resolveCreate({ id: "company-created", issuePrefix: "INI" })); + 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. + expect(companyState.setSelectedCompanyId).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain("Acme"); + }); + + it("applies the step again when the wizard is re-opened", async () => { + // Same guard, from the other side: closing and re-opening is a new + // request, so a freeze that outlived the open would be its own defect. + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId: "company-1", + initialStep: ONBOARDING_MISSION_STEP, + }; + await render(); + await settle(); + expect(currentStep()).toBe("mission"); + + dialogState.onboardingOpen = false; + await rerender(); + expect(currentStep()).toBe("closed"); + + dialogState.onboardingOpen = true; + dialogState.onboardingOptions = { + companyId: "company-1", + initialStep: ONBOARDING_AGENT_STEP, + }; + await rerender(); + await settle(); + + expect(currentStep()).toBe("agent"); + }); +}); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 1ea23016cf..bdf0bd3c70 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -47,6 +47,7 @@ import { companyPrefixFromOnboardingPath, resolveRouteOnboardingOptions, } from "../lib/onboarding-route"; +import { useCompanyMission } from "../hooks/useCompanyMission"; import { AsciiArtAnimation } from "./AsciiArtAnimation"; import { FrontDoor } from "./FrontDoor"; import { AgentCapsule } from "./AgentCapsule"; @@ -147,13 +148,28 @@ export function OnboardingWizard() { // Support opening the wizard from a route (e.g. /onboarding or an existing // company's "add agent" entry point) in addition to the dialog context. + // The company the path names, resolved before the mission lookup below so it + // has something to ask about. Same match the resolver makes. + const routeMatchedCompanyId = + companyPrefix && !companiesLoading + ? companies.find( + (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. const routeOnboardingOptions = - companyPrefix && companiesLoading + (companyPrefix && companiesLoading) || !routeMissionSettled ? null : resolveRouteOnboardingOptions({ pathname: location.pathname, companyPrefix, companies, + companyHasMission: routeCompanyHasMission, }); const effectiveOnboardingOpen = onboardingOpen || (routeOnboardingOptions !== null && !routeDismissed); @@ -239,19 +255,61 @@ export function OnboardingWizard() { // the user back to the route's initial step mid-flow. const createdCompanyIdRef = useRef(null); createdCompanyIdRef.current = createdCompanyId; + // The step the request wants, mirrored for the same reason. `initialStep` is + // *derived* - from the company list, and now from the goal list behind + // `useCompanyMission` - so its value changes whenever one of those queries + // does: a retry, a background refetch, a cache invalidation. An effect that + // depended on it would re-run on every such change and call setStep, moving + // a customer who is already mid-flow. Reading it through a ref breaks that + // dependency, so the effect runs when the wizard *opens* or when the company + // changes, and takes whatever the step is at that moment. + const initialStepRef = useRef(undefined); + initialStepRef.current = effectiveOnboardingOptions.initialStep; // Reset the route-dismissed flag when navigating to a different path. useEffect(() => { setRouteDismissed(false); }, [location.pathname]); + /** + * Forget everything that describes one particular company. + * + * Called when the wizard stops holding a company - the route replaced it, or + * withdrew it. Both are the same event, and clearing only part of it is what + * lets the next company skip work it has not done: a kept goal id reads as + * "this company's mission is already written", and the launch path would + * link the next company's project to the previous company's goal. + * + * The name and the prefix are cleared here too and backfilled again from the + * company list by the effects below, so they always describe the company in + * hand rather than the one before it. + */ + function clearCompanyScopedState() { + setCreatedCompanyPrefix(null); + setCompanyName(""); + setCompanyGoal(""); + setMissionPath(null); + setMissionConfirmed(false); + setCreatedCompanyGoalId(null); + setCreatedProjectId(null); + setCreatedIssueRef(null); + setCreatedAgentId(null); + } + // Sync step and company when onboarding opens with explicit options. // Only override saved state when explicit options provide values. + // + // The step belongs to the request that opened the wizard, not to the latest + // value of the expression that produced it - see `initialStepRef` above for + // why those differ. This effect is therefore keyed on the two things that + // make a *new* request: the wizard opening, and the company changing. + // Navigating from one company's onboarding path to another re-decides the + // step; the same request re-deriving a fresher value does not. useEffect(() => { if (!effectiveOnboardingOpen) return; // If explicit options are provided, they take precedence over saved state - if (effectiveOnboardingOptions.initialStep) { - setStep(effectiveOnboardingOptions.initialStep); + if (initialStepRef.current) { + setStep(initialStepRef.current); } const routeCompanyId = effectiveOnboardingOptions.companyId ?? null; if (routeCompanyId) { @@ -262,9 +320,26 @@ export function OnboardingWizard() { // navigating on to `/onboarding` would clear work the wizard did. if (routeCompanyId !== createdCompanyIdRef.current) { setCreatedCompanyId(routeCompanyId); - setCreatedCompanyPrefix(null); - routeCompanyIdRef.current = routeCompanyId; + clearCompanyScopedState(); } + // Ownership is recorded either way, including when the route merely + // names the company already in hand. Only the clearing above is + // conditional. + // + // This is a deliberate change to the rule the comment above described. + // Not recording ownership there protected wizard-created work from a + // later `/onboarding`, but it also meant that company was never + // withdrawn: create a company on step 1, visit its own onboarding path, + // then go to `/onboarding`, and the wizard shows "create a company" + // while still holding the previous one. The next confirmation then + // writes that customer's new mission into the old company - which is + // exactly the failure the withdrawal branch below was written to + // prevent, reached by a path it could not see. + // + // Losing the step-1 progress on `/onboarding` is the better error: + // `/onboarding` is a request to start a company, so honouring it beats + // silently writing into a different one. + routeCompanyIdRef.current = routeCompanyId; return; } if (routeCompanyIdRef.current) { @@ -277,15 +352,16 @@ export function OnboardingWizard() { // Only a company this route supplied is cleared. One the wizard created // itself, or restored from saved state, is left alone: the ref is null // in those cases, and clearing them would discard real progress. + // + // Withdrawing a company clears the same state that replacing one does. + // The two are the same event - this company is no longer the wizard's - + // and clearing only half of it leaves ids that make the *next* company + // skip work it has not done. setCreatedCompanyId(null); - setCreatedCompanyPrefix(null); routeCompanyIdRef.current = null; + clearCompanyScopedState(); } - }, [ - effectiveOnboardingOpen, - effectiveOnboardingOptions.companyId, - effectiveOnboardingOptions.initialStep - ]); + }, [effectiveOnboardingOpen, effectiveOnboardingOptions.companyId]); // Backfill issue prefix for an existing company once companies are loaded. useEffect(() => { @@ -294,6 +370,21 @@ export function OnboardingWizard() { if (company) setCreatedCompanyPrefix(company.issuePrefix); }, [effectiveOnboardingOpen, createdCompanyId, createdCompanyPrefix, companies]); + // Backfill the name too, for the same company and the same reason. + // + // `companyName` is otherwise only ever typed on step 1, so a company that + // enters the wizard further along has none. That is a dead end rather than a + // cosmetic gap: the mission step prints the name in its own copy, and both + // ways forward from that step - the button and the Enter key - require + // `companyName.trim()`. An existing company opened on the mission step could + // not leave it. Nothing reached that state until the dashboard started + // opening agentless companies there. + useEffect(() => { + if (!effectiveOnboardingOpen || !createdCompanyId || companyName) return; + const company = companies.find((c) => c.id === createdCompanyId); + if (company) setCompanyName(company.name); + }, [effectiveOnboardingOpen, createdCompanyId, companyName, companies]); + // Persist wizard state to localStorage on every change useEffect(() => { if (!effectiveOnboardingOpen) return; @@ -471,6 +562,23 @@ export function OnboardingWizard() { setRouteDismissed(true); } + /** + * Whether the company an async handler started for is still the one in hand. + * + * A route change can switch companies while a request is in flight, and the + * switch clears the created resource ids so the new company starts clean. A + * write that lands afterwards would put them back, and hand that company the + * previous one's goal, project, issue or agent — which is exactly what the + * clearing exists to prevent. + * + * Every async write below asks this before it attributes anything. It never + * cancels the server work, which is done and correct either way; it declines + * only to record it against a company it does not belong to. + */ + function stillTheSameCompany(companyIdAtStart: string | null) { + return createdCompanyIdRef.current === companyIdAtStart; + } + async function handleLaunchToDashboard() { if (!createdCompanyId || !createdAgentId) { setError(INCOMPLETE_ONBOARDING_STATE_MESSAGE); @@ -483,7 +591,7 @@ export function OnboardingWizard() { if (!goalId) { const goals = await goalsApi.list(createdCompanyId); goalId = selectDefaultCompanyGoalId(goals); - setCreatedCompanyGoalId(goalId); + if (stillTheSameCompany(createdCompanyId)) setCreatedCompanyGoalId(goalId); } let projectId = createdProjectId; @@ -502,7 +610,7 @@ export function OnboardingWizard() { queryKey: queryKeys.projects.list(createdCompanyId) }); } - setCreatedProjectId(projectId); + if (stillTheSameCompany(createdCompanyId)) setCreatedProjectId(projectId); } let issueRef = createdIssueRef; @@ -518,12 +626,20 @@ export function OnboardingWizard() { }) ); issueRef = issue.identifier ?? issue.id; - setCreatedIssueRef(issueRef); + if (stillTheSameCompany(createdCompanyId)) setCreatedIssueRef(issueRef); queryClient.invalidateQueries({ queryKey: queryKeys.issues.list(createdCompanyId) }); } + // Everything above is server work and stands on its own: the company has + // its goal, its onboarding project and its first task. What follows is + // this wizard finishing — selecting a company, discarding its own state + // and navigating. None of that is right for a customer who has moved to + // another company in the meantime: it would take them back, and `reset()` + // would discard the progress they had started there. + if (!stillTheSameCompany(createdCompanyId)) return; + const prefix = createdCompanyPrefix; // Select the new company as a route sync, not a manual switch: the // explicit navigate below is the intended destination, so page-memory's @@ -614,17 +730,91 @@ export function OnboardingWizard() { // mission step (e.g. via Back) doesn't create a duplicate company. async function handleConfirmMission() { if (createdCompanyId) { - setStep(3); + // An existing company needs its mission written, not just skipped past. + // This branch used to advance without saving anything, which was + // harmless while nothing sent an existing company to the mission step - + // a company reached step 2 only by creating itself on step 1, one line + // below. The dashboard now opens an agentless company here, so the + // customer types a mission and presses "Confirm mission". Advancing + // without writing it would leave the company with no mission at all, + // which is the state this whole change exists to remove. + // + // Only when the wizard has not already written one. Returning to step 2 + // and confirming again must not add a second goal. + if (createdCompanyGoalId) { + setStep(3); + return; + } + setLoading(true); + setError(null); + try { + const parsedGoal = parseOnboardingGoalInput(companyGoal); + const payload = { + title: parsedGoal.title, + ...(parsedGoal.description ? { description: parsedGoal.description } : {}) + }; + + // The company may already have a mission this step could not see. + // `useCompanyMission` fails open, so a goal lookup that exhausted its + // retries sends a company that has one here anyway. Adding a second + // company-level goal would leave two, and the earlier one would keep + // winning `selectDefaultCompanyGoalId` everywhere outside this wizard. + // + // So read once more before writing, and update rather than add. The + // customer just answered the question on a step that asked it, so + // their answer is the mission. A read that fails still writes: an + // unwritten mission is the failure this whole change exists to remove. + let existingGoalId: string | null = null; + try { + const goals = await queryClient.fetchQuery({ + queryKey: queryKeys.goals.list(createdCompanyId), + queryFn: () => goalsApi.list(createdCompanyId) + }); + existingGoalId = selectDefaultCompanyGoalId(goals); + } catch { + // Still cannot tell. Fall through and write. + } + + const goal = existingGoalId + ? await goalsApi.update(existingGoalId, payload) + : await goalsApi.create(createdCompanyId, { + ...payload, + level: "company", + status: "active" + }); + queryClient.invalidateQueries({ + queryKey: queryKeys.goals.list(createdCompanyId) + }); + if (!stillTheSameCompany(createdCompanyId)) return; + setCreatedCompanyGoalId(goal.id); + setStep(3); + } catch (err) { + setError(err instanceof Error ? err.message : "Failed to save the mission"); + } finally { + setLoading(false); + } return; } setLoading(true); setError(null); try { const company = await companiesApi.create({ name: companyName.trim() }); + queryClient.invalidateQueries({ queryKey: queryKeys.companies.all }); + // Same guard as the others, from the other end: 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 here rather than waiting for the next render. + // The goal write below asks `stillTheSameCompany(company.id)`, and a ref + // that still held the pre-create value would answer "no" to the handler + // that just did the creating - so the goal would never be attributed and + // the wizard would sit on the mission step it had just completed. + createdCompanyIdRef.current = company.id; setCreatedCompanyPrefix(company.issuePrefix); setSelectedCompanyId(company.id); - queryClient.invalidateQueries({ queryKey: queryKeys.companies.all }); const parsedGoal = parseOnboardingGoalInput(companyGoal); const goal = await goalsApi.create(company.id, { @@ -635,10 +825,11 @@ export function OnboardingWizard() { level: "company", status: "active" }); - setCreatedCompanyGoalId(goal.id); queryClient.invalidateQueries({ queryKey: queryKeys.goals.list(company.id) }); + if (!stillTheSameCompany(company.id)) return; + setCreatedCompanyGoalId(goal.id); setStep(3); // → Create your team lead } catch (err) { @@ -715,14 +906,17 @@ export function OnboardingWizard() { }); } const agent = hire.agent; - setCreatedAgentId(agent.id); queryClient.invalidateQueries({ queryKey: queryKeys.agents.list(createdCompanyId) }); - // Seed the CEO's agent instructions file so the agent always has // company context + a hiring-plan output format rule. Non-fatal on // failure — the agent can still function with adapter defaults. + // + // Before the ownership check below on purpose. This agent exists now, + // and it needs its instructions whatever this wizard goes on to show. + // Guarding server work rather than attribution would leave a hired agent + // with adapter defaults because the customer changed pages. try { const bundle = await agentsApi.instructionsBundle(agent.id, createdCompanyId); await agentsApi.saveInstructionsFile( @@ -745,6 +939,8 @@ export function OnboardingWizard() { console.warn("Failed to seed CEO instructions:", err); } + if (!stillTheSameCompany(createdCompanyId)) return; + setCreatedAgentId(agent.id); // Advance to the Review step — the lead is now online. The user drives // strategy + hiring from the planning chat after "Get started". setStep(5); @@ -807,6 +1003,11 @@ export function OnboardingWizard() { function handleKeyDown(e: React.KeyboardEvent) { if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) { e.preventDefault(); + // Every button below is disabled while a request is in flight. The + // keyboard has to honour the same rule, or a second Enter re-enters a + // handler whose guard is a piece of state the first one has not set + // 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); else if (step === 2 && companyName.trim() && companyGoal.trim()) handleConfirmMission(); diff --git a/ui/src/hooks/useCompanyMission.test.tsx b/ui/src/hooks/useCompanyMission.test.tsx new file mode 100644 index 0000000000..13d594f1e0 --- /dev/null +++ b/ui/src/hooks/useCompanyMission.test.tsx @@ -0,0 +1,126 @@ +// @vitest-environment jsdom + +import { flushSync } from "react-dom"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { Goal } from "@paperclipai/shared"; +import { useCompanyMission } from "./useCompanyMission"; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +const mockGoalsApi = vi.hoisted(() => ({ list: vi.fn() })); + +vi.mock("../api/goals", () => ({ goalsApi: mockGoalsApi })); + +function companyGoal(id: string): Goal { + return { + id, + companyId: "company-1", + title: "Ship the thing", + description: null, + level: "company", + status: "active", + parentId: null, + ownerAgentId: null, + createdAt: new Date("2026-03-02T00:00:00Z"), + updatedAt: new Date("2026-03-02T00:00:00Z"), + } as Goal; +} + +let captured: ReturnType | null = null; + +function Harness({ companyId }: { companyId: string | null }) { + captured = useCompanyMission(companyId); + return null; +} + +describe("useCompanyMission", () => { + let container: HTMLDivElement; + let queryClient: QueryClient; + let root: Root | null = null; + + function render(companyId: string | null) { + root = createRoot(container); + flushSync(() => { + root!.render( + + + , + ); + }); + } + + // React Query resolves through microtasks and React schedules the re-render + // after them, so a single tick is not reliably enough under load. Drain + // until the hook reports an answer rather than guessing at a tick count. + async function settle() { + for (let i = 0; i < 50 && !captured?.settled; i++) { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + } + + beforeEach(() => { + captured = null; + container = document.createElement("div"); + document.body.appendChild(container); + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + }); + + afterEach(() => { + flushSync(() => root?.unmount()); + root = null; + queryClient.clear(); + container.remove(); + vi.clearAllMocks(); + }); + + it("withholds an answer while the lookup is in flight", () => { + mockGoalsApi.list.mockReturnValue(new Promise(() => {})); + render("company-1"); + + expect(captured).toEqual({ hasMission: undefined, settled: false }); + }); + + it("reports a mission when the company has a company-level goal", async () => { + mockGoalsApi.list.mockResolvedValue([companyGoal("goal-1")]); + render("company-1"); + await settle(); + + expect(captured).toEqual({ hasMission: true, settled: true }); + }); + + it("reports no mission when the company has no company-level goal", async () => { + mockGoalsApi.list.mockResolvedValue([]); + render("company-1"); + await settle(); + + expect(captured).toEqual({ hasMission: false, settled: true }); + }); + + it("settles with an unknown mission when the lookup fails", async () => { + // The fail-open rule. Waiting for the data itself would leave `settled` + // false forever after a request exhausts its retries, and every caller + // gates opening onboarding on it — an agentless company would then get no + // onboarding at all, which is worse than being asked for its mission + // twice. + mockGoalsApi.list.mockRejectedValue(new Error("goals unavailable")); + render("company-1"); + await settle(); + + expect(captured).toEqual({ hasMission: undefined, settled: true }); + }); + + it("settles immediately when there is no company to ask about", () => { + // A disabled query stays pending forever. Reading that as "still loading" + // is the same failure as above, reached without a request. + render(null); + + expect(captured).toEqual({ hasMission: undefined, settled: true }); + expect(mockGoalsApi.list).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/src/hooks/useCompanyMission.ts b/ui/src/hooks/useCompanyMission.ts new file mode 100644 index 0000000000..ea887bc1b4 --- /dev/null +++ b/ui/src/hooks/useCompanyMission.ts @@ -0,0 +1,50 @@ +import { useQuery } from "@tanstack/react-query"; +import { goalsApi } from "../api/goals"; +import { queryKeys } from "../lib/queryKeys"; +import { selectDefaultCompanyGoalId } from "../lib/onboarding-launch"; + +/** + * Whether a company already has its mission, for deciding which onboarding + * step it belongs on. + * + * A company created by Paperclip Cloud does have one: Cloud collects the + * mission at signup and the tenant writes it as a company-level goal. Opening + * such a company on the mission step asks the customer something they answered + * minutes earlier on another origin. + * + * `settled` says whether the answer can be acted on. Callers wait for it + * before opening the wizard, because the wizard applies a step once, when it + * opens, and does not revise it afterwards — see the sync effect in + * `OnboardingWizard`. A step decided before the lookup finishes would be the + * step the customer is left on. + * + * Settled, not answered, on purpose. A gate that waits for the data itself + * fails closed: a goals request that exhausts its retries leaves the value + * undefined forever, and onboarding would then never open at all. `hasMission` + * stays `undefined` after a failure, which {@link onboardingStepForCompany} + * reads as "no mission" — the customer is asked for it again, and the flow + * continues. Asking a question twice is recoverable; never opening onboarding + * is not. This is the same fail-open rule the wake and provisioning readiness + * gates follow: a check that guards a convenience must never be able to block + * the thing it guards. + * + * The goal list is read under the query key the launch path already uses, so + * this shares that cache entry rather than adding a request. + */ +export function useCompanyMission(companyId: string | null | undefined): { + hasMission: boolean | undefined; + settled: boolean; +} { + const { data: goals, isPending } = useQuery({ + queryKey: queryKeys.goals.list(companyId ?? ""), + queryFn: () => goalsApi.list(companyId!), + enabled: Boolean(companyId), + }); + + return { + hasMission: goals ? selectDefaultCompanyGoalId(goals) !== null : undefined, + // A disabled query stays pending forever, so no company means nothing to + // wait for rather than an answer that never comes. + settled: !companyId || !isPending, + }; +} diff --git a/ui/src/lib/onboarding-auto-open.test.ts b/ui/src/lib/onboarding-auto-open.test.ts new file mode 100644 index 0000000000..c5845efbd9 --- /dev/null +++ b/ui/src/lib/onboarding-auto-open.test.ts @@ -0,0 +1,27 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { claimOnboardingOffer, resetOnboardingOffers } from "./onboarding-auto-open"; + +describe("claimOnboardingOffer", () => { + beforeEach(resetOnboardingOffers); + + it("lets the first offer through", () => { + expect(claimOnboardingOffer("company-1")).toBe(true); + }); + + it("refuses a second offer for the same company", () => { + // The dashboard effect re-runs whenever a query behind it refetches, and + // again on every return to the page. Neither may reopen a wizard the + // customer closed. + claimOnboardingOffer("company-1"); + expect(claimOnboardingOffer("company-1")).toBe(false); + }); + + it("offers each company once, in any order", () => { + // A single stored company id fails here: declining for A, visiting B, then + // returning to A would offer A again. + expect(claimOnboardingOffer("company-a")).toBe(true); + expect(claimOnboardingOffer("company-b")).toBe(true); + expect(claimOnboardingOffer("company-a")).toBe(false); + expect(claimOnboardingOffer("company-b")).toBe(false); + }); +}); diff --git a/ui/src/lib/onboarding-auto-open.ts b/ui/src/lib/onboarding-auto-open.ts new file mode 100644 index 0000000000..5be578f896 --- /dev/null +++ b/ui/src/lib/onboarding-auto-open.ts @@ -0,0 +1,30 @@ +/** + * Which companies have already been offered onboarding automatically. + * + * The dashboard opens onboarding for a company that has no agent. That is an + * offer, and an offer that reappears after it is declined is not one, so each + * company gets at most one. + * + * Module scope rather than state inside the dashboard, on purpose. A ref dies + * with the component, so navigating away and back would offer again, and it + * holds one company, so visiting a second agentless company and returning to + * the first would too. A page reload does offer again, which is the right + * scope: that is a new visit rather than the same one continuing. + */ +const offeredCompanyIds = new Set(); + +/** + * Records an offer, and reports whether it is the first one for this company. + * Returns false when onboarding has already been offered, in which case the + * caller must not open it again. + */ +export function claimOnboardingOffer(companyId: string): boolean { + if (offeredCompanyIds.has(companyId)) return false; + offeredCompanyIds.add(companyId); + return true; +} + +/** Forget every offer. For tests, which must not inherit each other's state. */ +export function resetOnboardingOffers(): void { + offeredCompanyIds.clear(); +} diff --git a/ui/src/lib/onboarding-route.test.ts b/ui/src/lib/onboarding-route.test.ts index ff3156f7a8..e382084908 100644 --- a/ui/src/lib/onboarding-route.test.ts +++ b/ui/src/lib/onboarding-route.test.ts @@ -3,8 +3,12 @@ import { companyPrefixFromOnboardingPath, isOnboardingPath, isOnboardingWizardActive, + onboardingStepForCompany, resolveRouteOnboardingOptions, shouldRedirectCompanylessRouteToOnboarding, + shouldRouteAgentlessCompanyToOnboarding, + ONBOARDING_AGENT_STEP, + ONBOARDING_MISSION_STEP, } from "./onboarding-route"; describe("isOnboardingPath", () => { @@ -187,3 +191,122 @@ describe("navigating away from a company's onboarding route", () => { ).toEqual({ initialStep: 1 }); }); }); + +describe("shouldRouteAgentlessCompanyToOnboarding", () => { + it("sends a company with no agents to onboarding", () => { + expect( + shouldRouteAgentlessCompanyToOnboarding({ + pathname: "/PC1/dashboard", + agentsLoaded: true, + agentCount: 0, + }), + ).toBe(true); + }); + + it("leaves a company that has agents alone", () => { + expect( + shouldRouteAgentlessCompanyToOnboarding({ + pathname: "/PC1/dashboard", + agentsLoaded: true, + agentCount: 1, + }), + ).toBe(false); + }); + + it("waits for the agent list before deciding", () => { + // In flight, the list is undefined and reads exactly like an empty one. + // Deciding here would bounce every user through onboarding on each cold + // load — the count is zero only because nothing has arrived yet. + expect( + shouldRouteAgentlessCompanyToOnboarding({ + pathname: "/PC1/dashboard", + agentsLoaded: false, + agentCount: 0, + }), + ).toBe(false); + }); + + it("does not redirect onto onboarding from onboarding", () => { + // The loop: finish the wizard without creating an agent, and a redirect + // that ignored the current path would send you straight back in. + for (const pathname of ["/onboarding", "/PC1/onboarding"]) { + expect( + shouldRouteAgentlessCompanyToOnboarding({ + pathname, + agentsLoaded: true, + agentCount: 0, + }), + ).toBe(false); + } + }); +}); + +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. + 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("keeps sending an unmatched prefix to company creation", () => { + expect( + resolveRouteOnboardingOptions({ + 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("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); + }); +}); diff --git a/ui/src/lib/onboarding-route.ts b/ui/src/lib/onboarding-route.ts index c616a6286e..db34e57939 100644 --- a/ui/src/lib/onboarding-route.ts +++ b/ui/src/lib/onboarding-route.ts @@ -38,12 +38,49 @@ export function companyPrefixFromOnboardingPath(pathname: string): string | unde return segments[0]; } +/** The wizard step that asks for the company's mission. */ +export const ONBOARDING_MISSION_STEP = 2; + +/** + * The wizard step that asks for the first agent. + * + * A company that already has its mission has answered steps 1 and 2 — Cloud + * collects both at signup and the seed writes the mission as a company-level + * goal. Opening such a company on "what is the mission?" asks the customer + * something they answered minutes earlier on another origin, which is the + * seam the seeded arc exists to remove. + */ +export const ONBOARDING_AGENT_STEP = 3; + +export type ExistingCompanyOnboardingStep = + | typeof ONBOARDING_MISSION_STEP + | typeof ONBOARDING_AGENT_STEP; + +/** + * The step a company that already exists belongs on. + * + * `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. + */ +export function onboardingStepForCompany( + companyHasMission: boolean | undefined, +): ExistingCompanyOnboardingStep { + return companyHasMission === true ? ONBOARDING_AGENT_STEP : ONBOARDING_MISSION_STEP; +} + export function resolveRouteOnboardingOptions(params: { pathname: string; companyPrefix?: string; companies: OnboardingRouteCompany[]; -}): { initialStep: 1 | 2; companyId?: string } | null { - const { pathname, companyPrefix, companies } = params; + /** + * 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; if (!isOnboardingPath(pathname)) return null; @@ -61,7 +98,41 @@ export function resolveRouteOnboardingOptions(params: { return { initialStep: 1 }; } - return { initialStep: 2, companyId: matchedCompany.id }; + return { + initialStep: onboardingStepForCompany(companyHasMission), + companyId: matchedCompany.id, + }; +} + +/** + * Whether a company with no agents should be sent to onboarding rather than + * left on the dashboard. + * + * A company with no agent cannot do anything: no runs, no tasks, nothing to + * show. The dashboard already says so in a banner with a "Create one here" + * link, which asks the customer to notice a problem the product could simply + * fix for them. + * + * It also closes the gap a Cloud-provisioned stack falls into. Cloud creates + * the company before the tenant boots, so `shouldRedirectCompanylessRouteToOnboarding` + * never fires — a seeded customer arrives at an empty dashboard having just + * answered a signup flow, and nothing routes them onward. + * + * `agentsLoaded` is required rather than optional on purpose. Callers hold + * agents as `Agent[] | undefined` while the query is in flight, and an absent + * list reads exactly like an empty one — redirecting on that would bounce + * every user through onboarding on each cold load. + */ +export function shouldRouteAgentlessCompanyToOnboarding(params: { + pathname: string; + agentsLoaded: boolean; + agentCount: number; +}): boolean { + if (!params.agentsLoaded) return false; + if (params.agentCount > 0) return false; + // Already there. Redirecting onto the path we are on is the loop that + // "finished the wizard but created no agent" would otherwise spin in. + return !isOnboardingPath(params.pathname); } export function shouldRedirectCompanylessRouteToOnboarding(params: { diff --git a/ui/src/pages/Dashboard.tsx b/ui/src/pages/Dashboard.tsx index f3ff724def..9fc5e9859f 100644 --- a/ui/src/pages/Dashboard.tsx +++ b/ui/src/pages/Dashboard.tsx @@ -1,4 +1,11 @@ import { useEffect, useMemo, useRef, useState } from "react"; +import { useLocation } from "@/lib/router"; +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"; import { dashboardApi } from "../api/dashboard"; @@ -41,6 +48,7 @@ function getRecentIssues(issues: Issue[]): Issue[] { export function Dashboard() { const { selectedCompanyId, companies } = useCompany(); const { openOnboarding } = useDialogActions(); + const location = useLocation(); const { setBreadcrumbs } = useBreadcrumbs(); const [animatedActivityIds, setAnimatedActivityIds] = useState>(new Set()); const seenActivityIdsRef = useRef>(new Set()); @@ -53,6 +61,49 @@ export function Dashboard() { enabled: !!selectedCompanyId, }); + // A company with no agent cannot do anything — no runs, no tasks, nothing + // to show. The banner below already says so and offers a link; this takes + // the customer there instead of asking them to notice. + // + // It also closes the gap a Cloud-provisioned stack falls into. Cloud creates + // the company before the tenant boots, so the companyless redirect never + // fires and a seeded customer lands here, on an empty dashboard, straight + // out of signup. + // + // Opened as the dialog rather than navigated to: the wizard is already + // mounted globally, so there is no route to race and no redirect to loop. + // Placed with the other hooks — the early returns below mean anything + // further down would be called conditionally. + // + // The company and the step are both passed. Opening with empty options would + // 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, + agentCount: agents?.length ?? 0, + }); + // Auto-open once per company. Every input to the effect sits behind a query, + // so a refetch re-runs it, and the customer can also navigate away and come + // back — both would otherwise call `openOnboarding` again and reopen a + // wizard that was deliberately closed. `claimOnboardingOffer` holds the + // 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), + }); + }, [shouldOpenOnboarding, selectedCompanyId, missionSettled, companyHasMission, openOnboarding]); + useEffect(() => { setBreadcrumbs([{ label: "Dashboard" }]); }, [setBreadcrumbs]);