mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-15 20:53:23 +02:00
fix(onboarding): verify draft ownership against a list fetched this session (#11382)
#11370 stopped onboarding restoring a saved draft when the company list had *errored*. It still trusted the list when the list looked healthy — and the wider door was exactly that. `main.tsx` sets `staleTime: 30_000` app-wide and `Auth.tsx` invalidates rather than resets on sign-in, so `invalidateQueries` keeps serving the previous account's companies with `isLoading` false and no error at all. On a self-hosted instance, where sign-out does not reload the page, signing in as a second account in the same warm tab could restore the first account's draft. No request had to fail. The wizard now judges ownership against a list it fetched for the current session: its own `useQuery` on the shared key with `staleTime: 0`, gated so it runs only when a parseable draft exists and adds no request otherwise. Every clause of that gate earns its place, and each was verified by removing it and watching a specific case fail: - `isSuccess` ties the answer to this session. React Query retains the last good `data` when a refetch fails, so after an account switch the retained value is the previous account's list; a failed refetch flips status to error and this rejects it. - The `unauthorized` check catches the opposite error. `companiesListQueryOptions` folds 401 and 403 into `{ companies: [], unauthorized: true }` rather than throwing, so an auth blip arrives as a *successful* empty list and would otherwise read as "this account owns nothing" and delete the draft. - The mount gate keys on `isFetching`, not `isLoading`. `isLoading` is false whenever retained data exists, so a refetch over a warm cache mounted the wizard undecided — and with the wizard open, the persist effect overwrote the customer's own draft with defaults before the answer arrived. It still releases on failure, so the "Get Started" dead end stays fixed. - An unreadable draft is judged, and cleared, before any of the above, and does not enable the query at all. `isFetchedAfterMount` was in an earlier revision and is deliberately not here: it is true after a failed refetch too, so it rejects nothing `isSuccess` has not, and no test could distinguish it. Worth recording how the first defect survived a check. I fault-injected it, saw a test fail, and concluded the guard worked. It was failing for an unrelated reason — the inner wizard mounted during the fetch and locked its state initializers to defaults, so the draft could not appear whatever the gate decided. Fixing the mount gate exposed the real behaviour. An injection is only evidence if the failure it produces is the one being claimed. This narrows onboarding only. The general fault is that a sign-out leaves account-scoped caches in place, and account changes that skip the button — a session lapsing server-side, a second account in a warm tab — reach the same stale list. Tracked separately; this defence should not be removed as redundant when that lands. ui typecheck clean; full ui suite 3963 pass, with only the timezone-dependent IssueProperties failure already present on master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
bc9f70f54c
commit
484b1f626c
3 files changed
+306
-48
No files matched your search
@@ -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: () => ({}) }),
|
||||
|
||||
@@ -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<string>();
|
||||
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<Array<{ id: string; name: string; issuePrefix: string }>>((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(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<OnboardingWizard />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<OnboardingWizard />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<OnboardingWizard />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<OnboardingWizard />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<OnboardingWizard />
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
});
|
||||
await flushReact();
|
||||
|
||||
expect(window.localStorage.getItem(ONBOARDING_STORAGE_KEY)).toBeNull();
|
||||
expect(mockCompaniesApi.list).not.toHaveBeenCalled();
|
||||
|
||||
await act(async () => {
|
||||
root.unmount();
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user