diff --git a/ui/src/components/OnboardingWizard.adapters.test.tsx b/ui/src/components/OnboardingWizard.adapters.test.tsx index 5796793f25..942cb67341 100644 --- a/ui/src/components/OnboardingWizard.adapters.test.tsx +++ b/ui/src/components/OnboardingWizard.adapters.test.tsx @@ -43,6 +43,14 @@ vi.mock("../context/DialogContext", () => ({ vi.mock("../context/CompanyContext", () => ({ useCompany: () => mockCompany, })); +// The restore gate fetches the company list itself to verify draft ownership, +// rather than trusting the shared cache, so this module now needs stubbing +// here. An empty list is fine: the drafts in this file carry no +// `createdCompanyId`, so there is no ownership question — but the fetch has to +// succeed for the gate to treat the draft as decidable at all. +vi.mock("../api/companies", () => ({ + companiesApi: { create: vi.fn(), list: vi.fn().mockResolvedValue([]) }, +})); vi.mock("../adapters", () => ({ listUIAdapters: () => mockAdapterRegistry.list, getUIAdapter: () => ({ buildAdapterConfig: () => ({}) }), diff --git a/ui/src/components/OnboardingWizard.test.tsx b/ui/src/components/OnboardingWizard.test.tsx index dc985e9983..f837407c66 100644 --- a/ui/src/components/OnboardingWizard.test.tsx +++ b/ui/src/components/OnboardingWizard.test.tsx @@ -25,6 +25,10 @@ const mockCompany = vi.hoisted(() => ({ const mockCompaniesApi = vi.hoisted(() => ({ create: vi.fn(), update: vi.fn(), + // The gate fetches the list itself now, rather than reading the shared + // cache, so ownership cases are driven from here. `mockCompany.companies` + // below still feeds the *inner* wizard, which is a different question. + list: vi.fn(), })); const mockGoalsApi = vi.hoisted(() => ({ create: vi.fn(), @@ -113,6 +117,8 @@ vi.mock("./AsciiArtAnimation", () => ({ AsciiArtAnimation: () => null })); vi.mock("./FrontDoor", () => ({ FrontDoor: () => null })); vi.mock("./AgentCapsule", () => ({ AgentCapsule: () => null })); +import { ApiError } from "../api/client"; +import { queryKeys } from "../lib/queryKeys"; import { ONBOARDING_STORAGE_KEY, OnboardingWizard } from "./OnboardingWizard"; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -144,6 +150,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.companies = []; mockCompany.loading = false; mockCompany.error = null; + mockCompaniesApi.list.mockResolvedValue([]); mockAdapterRegistry.list = []; mockAdapterRegistry.disabled = new Set(); mockCompaniesApi.create.mockResolvedValue({ @@ -186,6 +193,15 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockDialog.onboardingOptions = {}; mockCompany.companies = []; mockCompany.loading = true; + // Deferred rather than swapped later: the gate's fetch fires on the first + // render, so replacing the mock afterwards would never reach it. + let resolveList: (companies: Array<{ id: string; name: string; issuePrefix: string }>) => void = + () => {}; + mockCompaniesApi.list.mockReturnValue( + new Promise>((resolve) => { + resolveList = resolve; + }), + ); const { container, root, queryClient } = render(); const renderTree = () => @@ -208,6 +224,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( // Companies resolve asynchronously, owning the saved company. mockCompany.companies = [{ id: "c1", name: "Saved Co", issuePrefix: "SC" }]; mockCompany.loading = false; + await act(async () => { + resolveList([{ id: "c1", name: "Saved Co", issuePrefix: "SC" }]); + }); await renderTree(); await flushReact(); @@ -242,6 +261,9 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ); mockCompany.companies = [{ id: "company-new", name: "My Co", issuePrefix: "MC" }]; mockCompany.loading = false; + mockCompaniesApi.list.mockResolvedValue([ + { id: "company-new", name: "My Co", issuePrefix: "MC" }, + ]); const { root, queryClient } = render(); await act(async () => { @@ -281,6 +303,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.companies = []; mockCompany.loading = false; mockCompany.error = new Error("company list unavailable"); + mockCompaniesApi.list.mockRejectedValue(new Error("company list unavailable")); const { root, queryClient } = render(); await act(async () => { @@ -371,6 +394,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.companies = [{ id: "c1", name: "Saved Co", issuePrefix: "SC" }]; mockCompany.loading = false; mockCompany.error = new Error("refetch failed"); + mockCompaniesApi.list.mockRejectedValue(new Error("refetch failed")); const { root, queryClient } = render(); await act(async () => { @@ -452,6 +476,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.companies = []; mockCompany.loading = false; mockCompany.error = new Error("company list unavailable"); + mockCompaniesApi.list.mockRejectedValue(new Error("company list unavailable")); const { root, queryClient } = render(); await act(async () => { @@ -491,6 +516,7 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( mockCompany.companies = [{ id: "company-a", name: "Account A Co", issuePrefix: "AAC" }]; mockCompany.loading = false; mockCompany.error = new Error("refetch failed for the new account"); + mockCompaniesApi.list.mockRejectedValue(new Error("refetch failed for the new account")); const { root, queryClient } = render(); await act(async () => { @@ -509,6 +535,196 @@ describe("OnboardingWizard restore-gate (stale localStorage across accounts)", ( ) as HTMLInputElement | null; expect(nameInput?.value ?? "").not.toBe("A's Lead"); + await act(async () => { + root.unmount(); + }); + }); + it("does not judge ownership against a warm cache that never refetched", async () => { + // The door this whole change exists to shut, and the one the previous + // version missed. `main.tsx` sets `staleTime: 30_000`, so for thirty + // seconds after a sign-in the company list is served straight from cache + // with no request — and `Auth.tsx` invalidates on sign-in but invalidation + // keeps serving the old data while refetching. Either way account A's + // companies arrive with *no loading state and no error*, so a gate keyed + // on "not loading, no error" reads them as authoritative. + // + // Here the context reports exactly that healthy-looking stale state, and + // the fetch for this session has not answered. A's draft must not be + // restored on the strength of A's cached list. + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ + step: 3, + companyName: "Account A Co", + agentName: "A's Lead", + createdCompanyId: "company-a", + }), + ); + // The shared cache still holds A's list, and looks entirely healthy. + mockCompany.companies = [{ id: "company-a", name: "Account A Co", issuePrefix: "AAC" }]; + mockCompany.loading = false; + mockCompany.error = null; + // The list fetched for *this* session answers with B's companies, which + // do not include A's. Note the fetch must actually complete: leaving it + // pending would keep the wizard unmounted and the assertion below would + // hold for the wrong reason. + mockCompaniesApi.list.mockResolvedValue([ + { id: "company-b", name: "Account B Co", issuePrefix: "BBC" }, + ]); + + const { root, queryClient } = render(); + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + + // Mounted — so this is a real observation, not an unmounted false pass. + expect(document.body.textContent).not.toBe(""); + expect(document.body.textContent).not.toContain("A's Lead"); + const nameInput = document.body.querySelector( + 'input[placeholder="Chief of staff"]', + ) as HTMLInputElement | null; + expect(nameInput?.value ?? "").not.toBe("A's Lead"); + + await act(async () => { + root.unmount(); + }); + }); + + it("does not delete a draft when the company list comes back unauthorized", async () => { + // `companiesListQueryOptions` folds 401/403 into + // `{ companies: [], unauthorized: true }` rather than throwing, so an auth + // blip arrives as a successful fetch of an empty list — which reads as + // "this account owns nothing" and would delete the draft. + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ step: 3, companyName: "Saved Co", createdCompanyId: "c1" }), + ); + mockCompaniesApi.list.mockRejectedValue( + new ApiError("forbidden", 403, null), + ); + + const { root, queryClient } = render(); + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + + expect(window.localStorage.getItem(ONBOARDING_STORAGE_KEY)).not.toBeNull(); + + await act(async () => { + root.unmount(); + }); + }); + it("does not restore against data retained from a failed post-mount refetch", async () => { + // The gate's weak point if it only asks "was there a fetch after mount, + // and is there data". React Query keeps the last successful `data` when a + // refetch fails — so after an account switch the retained value is the + // *previous* account's list, and accepting it restores their draft. + window.localStorage.setItem( + ONBOARDING_STORAGE_KEY, + JSON.stringify({ + step: 3, + companyName: "Account A Co", + agentName: "A's Lead", + createdCompanyId: "company-a", + }), + ); + const { root, queryClient } = render(); + // A's list, already in the cache from their session. + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "company-a", name: "Account A Co", issuePrefix: "AAC" }], + unauthorized: false, + }); + // B's session: the refetch fails, so A's data is retained. + mockCompaniesApi.list.mockRejectedValue(new Error("refetch failed")); + + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + + // Mounted, so this observes the real thing rather than an empty document. + expect(document.body.textContent).not.toBe(""); + expect(document.body.textContent).not.toContain("A's Lead"); + const nameInput = document.body.querySelector( + 'input[placeholder="Chief of staff"]', + ) as HTMLInputElement | null; + expect(nameInput?.value ?? "").not.toBe("A's Lead"); + + await act(async () => { + root.unmount(); + }); + }); + it("does not mount undecided over a warm cache, which would overwrite the draft", async () => { + // The customer's own draft, their own companies already cached, and a + // refetch in flight. `isLoading` is false whenever retained data exists, + // so a gate keyed on it would mount the wizard while ownership was still + // undecidable — and with the wizard open, the persist effect writes the + // wizard's state back on every change, overwriting their draft with + // defaults before the answer arrives. + const draft = JSON.stringify({ + step: 3, + companyName: "Saved Co", + agentName: "Ops Lead", + createdCompanyId: "c1", + }); + window.localStorage.setItem(ONBOARDING_STORAGE_KEY, draft); + const { root, queryClient } = render(); + queryClient.setQueryData(queryKeys.companies.all, { + companies: [{ id: "c1", name: "Saved Co", issuePrefix: "SC" }], + unauthorized: false, + }); + mockCompaniesApi.list.mockReturnValue(new Promise(() => {})); + + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + + // Nothing mounted, so nothing could have written over the draft. + expect(document.body.textContent).toBe(""); + expect(window.localStorage.getItem(ONBOARDING_STORAGE_KEY)).toBe(draft); + + await act(async () => { + root.unmount(); + }); + }); + it("clears an unreadable draft without waiting on the company endpoint", async () => { + // Junk is junk whoever owns what, so judging it must not queue behind a + // request that cannot change the answer — nor issue one at all. + window.localStorage.setItem(ONBOARDING_STORAGE_KEY, "{not json"); + const { root, queryClient } = render(); + mockCompaniesApi.list.mockReturnValue(new Promise(() => {})); + + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + + expect(window.localStorage.getItem(ONBOARDING_STORAGE_KEY)).toBeNull(); + expect(mockCompaniesApi.list).not.toHaveBeenCalled(); + await act(async () => { root.unmount(); }); diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 76f7421169..3dbe712eca 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -5,6 +5,7 @@ import { useLocation, useNavigate, useParams } from "@/lib/router"; import { useDialog } from "../context/DialogContext"; import { useCompany } from "../context/CompanyContext"; import { companiesApi } from "../api/companies"; +import { companiesListQueryOptions } from "../api/companies-query"; import { goalsApi } from "../api/goals"; import { agentsApi } from "../api/agents"; import { approvalsApi } from "../api/approvals"; @@ -171,7 +172,8 @@ const INCOMPLETE_ONBOARDING_STATE_MESSAGE = * clear before computing `saved` and mounting the inner component at all. */ export function OnboardingWizard() { - const { companies, loading: companiesLoading, error: companiesError } = useCompany(); + // Deliberately does not call `useCompany()`. The list it exposes is the + // shared cache, which is what this gate must not trust - see below. // Parsed once (not re-parsed by the cleanup effect below) so the restored // value and the "should we wipe the blob" decision always agree. @@ -185,48 +187,77 @@ export function OnboardingWizard() { } }, []); - // A failed company query is not an answer *when it left us with nothing*. - // React Query reports that as `isLoading === false` with `data` defaulted to - // an empty list, which reads exactly like "settled, and this account owns - // nothing" - and that verdict deletes the draft below. Discarding a - // customer's onboarding because their company request timed out is the one - // outcome here that cannot be undone. + // Whether this account owns the company the draft names is an authorization + // question, and the shared company cache cannot answer it. // - // An error with companies still in hand is a different thing: a background - // refetch failed over a cache that is still good. Blocking on that would - // fail closed, and onboarding would simply never appear for a customer whose - // list is fine. Only an error that leaves the list empty is undecidable. - // Named for what it governs: whether the *draft* can be judged. It is not - // the same question as whether to mount - see the gate below. - // Any error at all, not only one that left the list empty. + // `main.tsx` sets `staleTime: 30_000` for every query, so for thirty seconds + // after a sign-in a mounted observer of the company list serves whatever is + // cached with no request at all. `Auth.tsx` invalidates the list on sign-in, + // but invalidation keeps serving the old data while it refetches. Neither + // shows up as loading and neither shows up as an error, so the previous + // account's companies arrive looking perfectly healthy - and a check that + // trusts "not loading, no error" finds the old company id in them and hands + // one account's onboarding draft to the next. // - // The companies cache is not scoped to an account and survives sign-out - - // `useSignOut` invalidates only the session and health queries, and - // invalidation keeps serving the old data anyway. So after A signs out and B - // signs in, a *failed* refetch leaves A's companies in hand. Trusting a - // non-empty list there would find A's company id in it, call the draft - // owned, and hand A's onboarding to B - which is the exact leak this whole - // change exists to close, reached through a different door. + // Sign-out does not help: `useSignOut` never clears the company cache, and + // on the self-hosted path there is no document reload to clear it either. + // Even once that is fixed, an account change can skip the button entirely - + // a session lapsing server-side, a second account signing in on a warm tab. // - // The cost is small and recoverable: during a transient refetch failure the - // draft is not restored. It is not deleted either, and the next successful - // load restores it. + // So this asks for a list fetched *for this mount*, rather than reading the + // one in hand. `isFetchedAfterMount` is the part that matters; `staleTime: 0` + // is what makes that reachable while the shared entry is still fresh. It + // shares the query key, so the result populates the same cache entry the + // rest of the app reads. + const companiesQuery = useQuery({ + ...companiesListQueryOptions, + staleTime: 0, + // Only a *parseable* saved draft poses the question. Without one there is + // nothing to authorize, and this must not add a request to every wizard + // mount - nor make the cleanup of unreadable junk wait on an endpoint that + // has no bearing on whether it is junk. + enabled: rawBlob !== undefined && rawBlob !== null, + }); + + // Decidable only with a list that succeeded, was fetched since this + // component mounted, actually arrived, and that the server was willing to + // give us. // - // A truthy check rather than `!== null`. The context types this - // `Error | null`, but `undefined` reaches here from any consumer that - // provides the company context without an `error` key, and `undefined !== - // null` would read a healthy load as a failure. - const ownershipUndecidable = companiesLoading || Boolean(companiesError); + // `isSuccess` is what ties the answer to this session. React Query keeps the + // last good `data` when a refetch fails, so after an account switch the + // retained value is the *previous* account's list - but a failed refetch + // flips status to error, so `isSuccess` rejects it. Combined with + // `staleTime: 0`, which makes every mount refetch, and the mount gate below + // holding while that is in flight, a success here is always this session's. + // + // `isFetchedAfterMount` looks like it belongs here too and does not: it is + // true after a *failed* refetch as well, so it never rejects anything + // `isSuccess` has not already rejected. Left out rather than kept as + // decoration - no test could distinguish it, which is how a guard rots. + // + // The `unauthorized` check is the last one: `companiesListQueryOptions` + // folds 401 and 403 into `{ companies: [], unauthorized: true }` rather than + // throwing, so an auth blip arrives as a *successful* fetch of an empty list + // and would otherwise read as "this account owns nothing" and delete the + // draft. + const ownershipDecidable = + companiesQuery.isSuccess && + companiesQuery.data !== undefined && + !companiesQuery.data.unauthorized; const { saved, staleStateDetected } = useMemo(() => { if (rawBlob === undefined) return { saved: null, staleStateDetected: false }; - // Companies not settled yet: restoreOnboardingState must not be called - // (see its CONTRACT). Not stale, just not decidable yet. - if (ownershipUndecidable) return { saved: null, staleStateDetected: false }; + // Unreadable, so junk regardless of who owns what. Judged before the + // ownership check rather than after it, so clearing it does not wait on a + // company request that cannot change the answer. if (rawBlob === null) return { saved: null, staleStateDetected: true }; - const restored = restoreOnboardingState(rawBlob, companies); + // Not decidable yet, or not decidable at all. Either way: restore nothing, + // delete nothing. A draft withheld is recoverable on the next load; a + // draft deleted, or one handed to the wrong account, is not. + if (!ownershipDecidable) return { saved: null, staleStateDetected: false }; + const restored = restoreOnboardingState(rawBlob, companiesQuery.data!.companies); return { saved: restored, staleStateDetected: restored === null }; - }, [rawBlob, ownershipUndecidable, companies]); + }, [rawBlob, ownershipDecidable, companiesQuery.data]); // A discarded/malformed state should not sit in storage waiting to confuse // the next onboarding attempt (e.g. a different signed-in user). @@ -235,24 +266,27 @@ export function OnboardingWizard() { onboardingDraftStorage.clear(); }, [staleStateDetected]); - // A saved blob exists and the company list is still in flight: wait rather - // than mount the inner wizard with a premature, and unrecoverable, guess at - // the draft. + // A saved blob exists and the verification fetch is still in flight: wait, + // rather than mount the inner wizard with a premature and unrecoverable + // guess at the draft. Its ~20 `useState(saved?.x ?? default)` initializers + // only read `saved` once. // - // Only while *loading*, not on error. An error with an empty list is still - // undecidable - nothing is restored and nothing is cleared, per the memo - // above - but withholding the wizard on top of that is a dead end, because - // the companies query sets `retry: false`. With no companies the dashboard - // offers a "Get Started" button that opens onboarding, and a gate that - // returned null here would make that button do nothing at all until a - // refetch happened to succeed. + // `isFetching`, not `isLoading`. `isLoading` is false whenever the cache + // holds retained data, so a refetch over a warm cache would mount the wizard + // while ownership was still undecidable - and with the wizard open, the + // persist effect would overwrite the customer's own draft with defaults + // before the answer arrived. `isFetching` covers the refetch too. + // + // While in flight, not on failure. The companies query sets `retry: false`, + // so a failed fetch stays failed; and with no companies the dashboard offers + // a "Get Started" button wired to onboarding, which a gate that returned null + // here would make do nothing at all. // // Mounting does not cost the draft. The persist effect that would overwrite // it is itself gated on `effectiveOnboardingOpen`, so a mounted-but-closed - // wizard writes nothing, and the blob survives for a later load that can - // decide. If the wizard *is* open the customer is onboarding right now, - // which supersedes the draft anyway. - if (rawBlob !== undefined && companiesLoading) { + // wizard writes nothing. If the wizard is open the customer is onboarding + // right now, which supersedes the draft anyway. + if (rawBlob !== undefined && companiesQuery.isFetching) { return null; }