mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-12 18:06:11 +02:00
## 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>
114 lines
5.2 KiB
TypeScript
114 lines
5.2 KiB
TypeScript
import { useMutation, useQueryClient } from "@tanstack/react-query";
|
|
import { authApi } from "@/api/auth";
|
|
import { navigateTopLevel } from "@/lib/browserNavigation";
|
|
import { queryKeys } from "@/lib/queryKeys";
|
|
import { useCloudInstance } from "./useCloudInstance";
|
|
|
|
const CLOUD_SIGN_OUT_PATH = "/cloud/logout";
|
|
|
|
/**
|
|
* Query-key roots that describe the *instance* rather than the account that was
|
|
* signed in, and so are allowed to outlive a sign-out.
|
|
*
|
|
* `health` is the only one. It carries deployment mode, bootstrap state and
|
|
* Cloud metadata — nothing account-scoped — and `useCloudInstance` observes it
|
|
* with `enabled: false`, deliberately leaving the fetch to CloudAccessGate.
|
|
* Dropping the entry would strand every such observer on `null` until the gate
|
|
* happened to refetch, flipping Cloud instances into their self-hosted
|
|
* rendering mid-sign-out. It is refreshed in place instead.
|
|
*
|
|
* Everything *not* listed here is treated as account-scoped and cleared. That
|
|
* direction is the point: a query key added later is account-scoped unless
|
|
* someone deliberately says otherwise, so forgetting this file fails closed.
|
|
*/
|
|
const INSTANCE_SCOPED_QUERY_ROOTS: readonly unknown[] = [queryKeys.health[0]];
|
|
|
|
export function isAccountScopedQueryKey(queryKey: readonly unknown[]): boolean {
|
|
return !INSTANCE_SCOPED_QUERY_ROOTS.includes(queryKey[0]);
|
|
}
|
|
|
|
interface UseSignOutOptions {
|
|
onSignedOut?: () => void;
|
|
}
|
|
|
|
/**
|
|
* Owns the app-wide sign-out decision.
|
|
*
|
|
* Cloud-managed tenants must enter the harness-owned logout sequence without
|
|
* first clearing the tenant session; that path is a top-level navigation, so
|
|
* the document reload it triggers builds a new QueryClient and there is nothing
|
|
* left here to clear. Authenticated self-hosted instances keep the local API
|
|
* flow and drop the account-scoped caches afterward.
|
|
*/
|
|
export function useSignOut({ onSignedOut }: UseSignOutOptions = {}) {
|
|
const cloud = useCloudInstance();
|
|
const queryClient = useQueryClient();
|
|
|
|
return useMutation({
|
|
mutationFn: async () => {
|
|
if (cloud) {
|
|
onSignedOut?.();
|
|
navigateTopLevel(CLOUD_SIGN_OUT_PATH);
|
|
return "cloud" as const;
|
|
}
|
|
|
|
await authApi.signOut();
|
|
return "self-hosted" as const;
|
|
},
|
|
onSuccess: async (target) => {
|
|
if (target === "cloud") return;
|
|
|
|
onSignedOut?.();
|
|
|
|
// 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 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,
|
|
// so a mounted `useQuery` keeps returning its last result until some
|
|
// unrelated re-render happens to rebuild the query. That is not a corner
|
|
// case here — CompanyProvider sits above the router and stays mounted
|
|
// across the whole sign-out/sign-in cycle. Reset notifies them, so the
|
|
// old data is gone from the cache *and* from everything reading it.
|
|
//
|
|
// Measured against query-core 5.101.4 rather than reasoned about:
|
|
// removal produced 0 notifications and left the observer holding the
|
|
// signed-out account's session; reset produced 3 and null. The
|
|
// consequence of the former is not just stale reads — the redirect in
|
|
// CloudAccessGate fires on the session going empty, and it never runs.
|
|
//
|
|
// The opposite advice holds for a *local* reset of a key an observer is
|
|
// still mounted against. Reset rewinds the update counters
|
|
// `isFetchedAfterMount` derives from while that observer keeps its
|
|
// bind-time baseline, so the flag can never read true again and anything
|
|
// gated on it withholds forever. `AppsConnect` is the consumer to check
|
|
// against: it gates its render on that flag for two queries.
|
|
//
|
|
// Sign-out is not that case. It resets the session too, so `AppsConnect`
|
|
// — which sits behind a route — unmounts on the redirect and remounts
|
|
// with a fresh baseline.
|
|
//
|
|
// Not awaited: the refetches this kicks off are expected to 401 now that
|
|
// the session is gone, and the sign-out button should not sit pending
|
|
// while they fail.
|
|
void queryClient.resetQueries({
|
|
predicate: (query) => isAccountScopedQueryKey(query.queryKey),
|
|
});
|
|
|
|
// Instance-scoped, so refreshed in place rather than dropped — see
|
|
// INSTANCE_SCOPED_QUERY_ROOTS.
|
|
await queryClient.invalidateQueries({ queryKey: queryKeys.health });
|
|
},
|
|
});
|
|
}
|