diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index b5b8d6c525..8c23e267e8 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -194,32 +194,19 @@ export function OnboardingWizard() { }, []); // Whether this account owns the company the draft names is an authorization - // question, and the shared company cache cannot answer it. + // question, and the answer has to be about the account asking now. // - // `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 shared company cache could not answer it at all: one entry for every + // account, served for thirty seconds after a switch with no loading state and + // no error, so a check that trusted "not loading, no error" handed one + // account's draft to the next. The entry is keyed by account now, and that + // trap is gone with it. // - // Sign-out is no longer the hole it was: `useSignOut` now resets every - // account-scoped cache entry rather than invalidating two of them, so the - // ordinary A-signs-out-then-B-signs-in path does not leave A's companies in - // hand. - // - // That covers the button, not the question. An account can change without - // it - a session lapsing server-side, a second account signing in on a warm - // tab, a caller supplying the company context from somewhere else - so this - // gate stays independent of that fix rather than deferring to 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. + // What survives is smaller and still worth a request. A cached list is the + // right account's but can be thirty seconds old, so a company created moments + // ago in another tab is missing from it — and missing reads as "you do not own + // this", which deletes the draft rather than withholding it. So this still + // asks for a list fetched for this mount. const companiesQuery = useCompanyListQuery({ staleTime: 0, // Only a *parseable* saved draft poses the question. Without one there is @@ -229,27 +216,28 @@ export function OnboardingWizard() { 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. + // Decidable only with a list that succeeded, actually arrived, and that the + // server was willing to give us. // - // `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. + // Whose list it is stopped being a question here: the entry is keyed by + // account, so the previous account's list is unreachable rather than merely + // rejected. What the checks still answer is whether there is an answer at all, + // and the reason that matters is the *destructive* branch below — an + // undecidable draft is withheld and recoverable, but a draft judged + // not-yours is deleted. // - // `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. + // `isSuccess`: React Query keeps the last good `data` through a failed + // refetch, and a retained list is not evidence about now. // - // 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. + // `staleTime: 0` on the query, still: a cached list is the right account's but + // can be thirty seconds old, and a company created moments ago in another tab + // would be missing from it — which reads as "this draft belongs to a company + // you do not own" and deletes it. + // + // `unauthorized`: the query 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 && diff --git a/ui/src/hooks/useSignOut.ts b/ui/src/hooks/useSignOut.ts index a7d5c3ef78..7d54525c40 100644 --- a/ui/src/hooks/useSignOut.ts +++ b/ui/src/hooks/useSignOut.ts @@ -62,12 +62,16 @@ export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) { // Drop every account-scoped cache entry, rather than invalidating a // couple of them. `invalidateQueries` only marks an entry stale and goes - // on serving the old value until a refetch succeeds — and the companies - // query sets `retry: false`, so a single failed request is enough to - // leave the previous account's company list readable for the whole of - // the *next* account's session. Consumers that read a non-empty list as - // authoritative (company auto-selection, the invite-landing "already a - // member" check, the board-access gate) would then act on it. + // on serving the old value until a refetch succeeds, and several of these + // queries set `retry: false`, so a single failed request is enough to + // leave the previous account's data readable for the whole of the *next* + // account's session. + // + // The company list is no longer the example: it is keyed by account, so + // the next account reads a different entry regardless of what happens + // here. Everything else still is — the board-access gate, per-company + // details and stats, and every account-scoped key added since — which is + // why this sweeps by predicate rather than naming the keys it knows. // // `resetQueries` rather than `removeQueries`: removal empties the cache // but does not notify the observers already subscribed to those entries, diff --git a/ui/src/pages/InviteLanding.tsx b/ui/src/pages/InviteLanding.tsx index 99825026a9..0482ed5b6e 100644 --- a/ui/src/pages/InviteLanding.tsx +++ b/ui/src/pages/InviteLanding.tsx @@ -242,19 +242,29 @@ export function InviteLandingPage() { retry: false, }); - // The company list is keyed by account now (#11488), so a list belonging to - // somebody else cannot be read here at all. The mount-scoped gate below is kept - // as a second line rather than removed with the first: this page turns the list - // into an authorization verdict, and it should not be the place that discovers - // a hole in the keying. `local_trusted` instances have no accounts, so there the - // shared list is the only identity there is and the gate stays open. + // Whose list this is, is no longer this page's problem: the entry is keyed by + // account, so another account's list is unreachable rather than merely + // distrusted. What is left for the gate below is narrower and still real — do + // we have an answer *yet*. Without it, a pending query reads as an empty list, + // which reads as "not a member", which auto-accepts an invite the customer may + // already hold. + // + // Hence `staleTime: 0` and a mount-scoped flag rather than `isSuccess`: a + // cached list is the right account's now, but it can be thirty seconds old, and + // acting on "not a member" is the direction that costs something. + // + // `local_trusted` has no accounts at all, so there is no identity to key on and + // nothing to check the list against; the gate stays open. const companiesQuery = useCompanyListQuery({ enabled: !!sessionQuery.data && !!inviteQuery.data?.companyId, staleTime: 0, }); const membershipIsAccountScoped = healthQuery.data?.deploymentMode !== "local_trusted"; + // No `sessionQuery.data` term: the query only fetches while a session exists, so + // the flag cannot be true without one, and if the session lapses the observer + // re-keys to the anonymous entry and holds no data to leak. const membershipListIsCurrent = membershipIsAccountScoped - ? Boolean(sessionQuery.data) && companiesQuery.isFetchedAfterMount + ? companiesQuery.isFetchedAfterMount : true; const companyList = membershipListIsCurrent ? companiesQuery.data?.companies ?? [] : [];