refactor(ui): retire the shared-cache reasoning the account key made obsolete (#11507)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Three places in the UI turn the company list into an authorization verdict: the invite landing page, the onboarding draft gate, and company auto-selection > - Each grew a defense when one `["companies"]` cache entry answered for every account, and each documented that hazard at length > - #11488 keyed the entry by account, so the hazard those comments describe can no longer happen > - The comments stayed, and a comment that describes a trap that no longer exists is how the next reader removes a mechanism that is still holding something up > - This pull request replaces that reasoning with what the mechanisms actually do now, and removes the one condition that genuinely went dead > - The benefit is that the next person to simplify these gates has accurate reasons to work from ## Linked Issues or Issue Description No public issue exists. Follow-up to #11488, #11430 and #11417. The problem follows. **What happened?** The three gates were written against a shared, account-less company cache. #11488 keyed that entry by account, which made the documented hazard impossible — but the documentation stayed. Each gate now carries a long explanation of a cross-account leak that the key prevents, while the mechanism it explains is in fact still required for a different and unrelated reason. That is a maintenance hazard in a specific direction: a reader who checks the comment against the code concludes the mechanism is obsolete, removes it, and reintroduces a failure the comment never mentioned. **Expected behavior** The reasoning next to each gate describes why the gate is there now. **Steps to reproduce** Read the comment above `ownershipDecidable` in `OnboardingWizard.tsx` against `master`. It justifies `isSuccess` on the grounds that "after an account switch the retained value is the previous account's list", which the account-keyed entry makes impossible. **Paperclip version or commit** `master` at `0817fbad9`. ## What Changed - `ui/src/pages/InviteLanding.tsx` — dropped the `Boolean(sessionQuery.data)` conjunct from `membershipListIsCurrent`; rewrote the comment. - `ui/src/components/OnboardingWizard.tsx` — replaced the shared-cache explanation above the ownership gate with the reason the gate still exists. - `ui/src/hooks/useSignOut.ts` — corrected the sweep's rationale, which cited the company list as its example of data the next account could read. ### The one dead condition `membershipListIsCurrent` tested `Boolean(sessionQuery.data) && companiesQuery.isFetchedAfterMount`. The first term cannot be false when the second is true: the query is `enabled` only while a session exists, so the flag cannot be set without one. The lapsed-session case it looked like it covered is covered by the keying instead — the observer re-keys to the anonymous entry and holds no data to leak. Tests pass with it removed, but that only shows no test distinguishes it, which is why the reasoning above is recorded in the code rather than left for the next reader to redo. ### What is deliberately kept Each gate turned out to be load-bearing for a reason that has nothing to do with accounts: - **InviteLanding** still waits for a list fetched this mount. A pending query reads as an empty list, which reads as "not a member", which auto-accepts an invite the customer may already hold. - **OnboardingWizard** still forces a fetch with `staleTime: 0`. 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. - **CompanyProvider** still clears the live selection on an account change. That is component state and does not change key with the query. Removing them as redundant is the mistake the stale comments invited; this change is what makes that argument harder to make by accident. ## Verification - `pnpm tsc -b` in `ui`: clean. - `InviteLanding.test.tsx`, `OnboardingWizard.test.tsx`, `CompanyContext.test.tsx`, `useSignOut.test.tsx`, `companies-query.test.ts`: **67 passed**, run twice. No behaviour change is claimed and none is intended: the only non-comment edit is the removal of a condition that cannot alter the expression's value. **Not done:** no browser run. Nothing here is observable at runtime. ## Risks Low. Comments, plus one condition shown to be unreachable-false. The risk that remains is a documentation risk in the other direction: if the keying is ever reverted or bypassed, these comments will understate what the gates protect against. They name #11488 so that connection is findable. **This does not close the class.** Account-scoped entries other than the list — `["companies", id]`, stats, and the rest — still survive an account change that skips the sign-out button. That is unclaimed work, and larger than this. ## Model Used Claude Opus 5 (`claude-opus-5`), through Claude Code. Extended thinking enabled. Tool use enabled: file read and edit, shell for typecheck and test runs. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
40e7add71c
commit
6d0adbfb5d
|
|
@ -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 &&
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 ?? [] : [];
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue