mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 13:24:55 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Which companies a person belongs to is an authorization fact the server owns, and the UI caches the answer for speed > - It cached that answer under one `["companies"]` key with no account attached, while `main.tsx` sets `staleTime: 30_000` for every query > - So for thirty seconds after an account change, one person's list answered questions asked about another, arriving with no loading state and no error > - Three separate consumers each grew their own defense against this, and each was a place to forget one > - This pull request keys the entry by account, so a list belonging to someone else is not distrusted but unreachable > - The benefit is that the protection stops depending on every future consumer remembering to defend itself ## Linked Issues or Issue Description No public issue exists. Refs #11380, #11382, #11417, #11430. The problem follows. **What happened?** The company list lived in a single cache entry, `["companies"]`, carrying no record of which account it was fetched for. Combined with the app-wide 30s `staleTime`, any read within that window after an account change returned the previous account's list — from cache, with no request, no loading state and no error. Every consumer that treats the list as an authorization fact had to know this and defend itself: - `InviteLanding` reads it to decide whether you already belong to the inviting company (#11417). - `OnboardingWizard` reads it to decide whether a saved draft belongs to you (#11382, merged). - `CompanyProvider` reads it to pick and persist your active company (#11430). All three defenses are correct. The problem is structural: the fourth consumer has to invent a fourth one. **Expected behavior** A cached company list can only answer questions about the account it was fetched for. **Steps to reproduce** 1. On a self-hosted instance in `authenticated` mode, sign in as account A, which belongs to company X. 2. Within thirty seconds, have account B become the session in that tab — a second tab signing in, or A's session lapsing server-side. 3. Any consumer reading the company list receives A's list, and nothing in the query result indicates it is not B's. **Paperclip version or commit** `master` at `ac91b7f3b`, which includes #11430. ## What Changed - `ui/src/lib/queryKeys.ts` — `companies.list(userId)` replaces `companies.all` as the list's entry. `companies.all` remains the prefix, so it still matches for invalidation. - `ui/src/api/companies-query.ts` — `companyListQueryOptions(userId)` builds the keyed options; `useCompanyListQuery()` is the only observer entry point and holds until the session settles, because the key cannot be built before then; `fetchCompanyListForCurrentAccount(queryClient)` covers imperative paths; `useAccountIdentity()` exposes the session identity the key is built from. - `ui/src/api/companies-query.ts` — the `/companies` detach moved into the query function. - `ui/src/context/CompanyContext.tsx` — drops the session-watching refetch machinery the key now makes unnecessary (`removeQueries`, the explicit replacement fetch, the awaiting gate). It still clears the live selection on an account change, because that is component state and does not change key with the query. - `ui/src/pages/InviteLanding.tsx`, `ui/src/components/OnboardingWizard.tsx` — read through the account-aware API. - Tests — the account-keyed guarantee, prefix invalidation still reaching the list, the detach inside the query function, and updates where suites seeded the old shared key. ### The existing defenses are deliberately left in place The per-consumer gates in #11382, #11417 and #11430 are now belt and braces. They are also what will catch this refactor if it is wrong somewhere, so removing them in the same change that moves the foundation would be the wrong order. Simplifying them is a follow-up, once this has proven itself. ### Why `retry: 1` appears in CompanyProvider An earlier measurement on #11430 found a retry on the replacement fetch changed no outcome, because `removeQueries` made the observer rebind and issue a second request for free. Keying by account removes that mechanism and the free attempt with it. The retry now carries the property the incidental refetch used to — a single blip during an account change should not leave the customer with no companies until they find "Try again". #11430's test for that property is unchanged and still passes, which is how the gap was caught. ### A regression this went through, kept for the record Gating the query on the session settling meant that while the account was unknown the query was *disabled*, and a disabled query reports `isLoading: false` with no data — which the provider defaults to an empty list and reads as "asked, and owns nothing". That is the destructive branch #11477 had just fixed, reached through a different door: it would have cleared the customer's stored company on every cold boot. #11477's test caught it during the rebase. `useCompanyListQuery` now reports the wait for the account as part of the wait for the list. ### What this does not do It does not scope the rest of the per-account cache. `["companies", id]`, stats, and every other account-scoped entry still survive an account change; that is the cache-lifetime work in #11380. ## Verification - `pnpm vitest run` in `ui`: **4018 passed, 1 failed**. - `pnpm tsc -b` in `ui`: clean. - `companies-query.test.ts`: 6 passed. `CompanyContext.test.tsx`: 17 passed. `OnboardingWizard.test.tsx`: 13 passed. `InviteLanding.test.tsx`: 13 passed. The failure is the pre-existing timezone-dependent `IssueProperties.test.tsx`, fixed by #11478. Two behaviours are asserted rather than assumed, because the refactor is only safe if they hold: that invalidating the `companies` prefix still marks the account-keyed list stale (19 call sites depend on it), and that the query function detaches the in-flight `/companies` request before fetching. **Not done:** no manual two-account run in a browser. The path needs two accounts on an `authenticated` instance, which a local dev instance cannot exercise. ## Risks Moderate, and worth reading before approving. **It touches `InviteLanding.tsx`, which #11417 also modifies**, so one of the two will need a rebase — the conflict is mechanical (both change how the same query is read). This was #11481, which GitHub closed automatically when its base branch (#11430's) was deleted on merge; reopening a pull request whose base branch is gone is not permitted, so it continues here against `master` with the same head and the same review already recorded on #11481. **The list now waits for the session query.** The key cannot be built before the account is known. In the app the session is already fetched at boot by many components, so this is a dependency rather than an extra request, but it does serialize: on a cold boot the list waits for the session to land. Every test that renders a company-list consumer now needs a session in the cache, which is why several suites gained a seed. **A missing mock surfaces as a passing gate rather than an error.** The detach inside the query function meant suites whose `companiesApi` mock lacked `detachInflightList` had their query function throw, which read as "decided" in the onboarding gate and mounted the wizard early. Fixed in the affected suites; worth knowing as a failure mode. ## 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>
140 lines
6.1 KiB
TypeScript
140 lines
6.1 KiB
TypeScript
import { useQuery, type QueryClient } from "@tanstack/react-query";
|
|
import type { Company } from "@paperclipai/shared";
|
|
import { authApi } from "./auth";
|
|
import { companiesApi } from "./companies";
|
|
import { ApiError } from "./client";
|
|
import { queryKeys } from "../lib/queryKeys";
|
|
|
|
export type CompanyListResult = { companies: Company[]; unauthorized: boolean };
|
|
|
|
// Single source of truth for the company-list query. Every consumer reads the
|
|
// same cache entry, so they must agree on the shape — returning a bare
|
|
// `Company[]` from one and this wrapped object from another silently corrupts
|
|
// the entry and crashes whichever reads the other's shape.
|
|
//
|
|
// The entry is keyed by account. Callers therefore cannot ask for "the company
|
|
// list" without saying whose, which is the point: the previous design let a
|
|
// list fetched for one account answer a question about another, and every
|
|
// consumer had to defend against that individually.
|
|
export function companyListQueryOptions(userId: string | null) {
|
|
return {
|
|
queryKey: queryKeys.companies.list(userId),
|
|
queryFn: async (): Promise<CompanyListResult> => {
|
|
// Request coalescing keys on the path alone, so it would happily answer a
|
|
// fetch for this account with a `/companies` request issued under the
|
|
// previous one — putting the wrong account's list in an account-keyed
|
|
// entry. Coalescing has nothing to offer here anyway: React Query already
|
|
// dedupes concurrent fetches within a key, and *across* keys the accounts
|
|
// differ by construction, which is exactly when sharing is wrong.
|
|
companiesApi.detachInflightList();
|
|
try {
|
|
return { companies: await companiesApi.list(), unauthorized: false };
|
|
} catch (err) {
|
|
if (err instanceof ApiError && (err.status === 401 || err.status === 403)) {
|
|
return { companies: [], unauthorized: true };
|
|
}
|
|
throw err;
|
|
}
|
|
},
|
|
retry: false,
|
|
} as const;
|
|
}
|
|
|
|
const sessionQueryOptions = {
|
|
queryKey: queryKeys.auth.session,
|
|
queryFn: () => authApi.getSession(),
|
|
retry: false,
|
|
} as const;
|
|
|
|
/**
|
|
* The signed-in account, as the cache currently understands it.
|
|
*
|
|
* `settled` means the session query *answered*, not merely that it stopped
|
|
* being pending. The distinction is the whole point of this helper. `null` is a
|
|
* real answer — `authApi.getSession` returns it for a 401, meaning "signed out"
|
|
* — but a session request that *failed* answers nothing, and it throws rather
|
|
* than returning null. Treating that failure as `null` would key an account's
|
|
* list to `anonymous` while the request still carried their cookie, writing a
|
|
* credentialed response into the entry every signed-out reader trusts. Which is
|
|
* the original bug, rebuilt for anonymous.
|
|
*
|
|
* So: settled on success only. A failed session lookup leaves the list unfetched
|
|
* until the query recovers, which it does on the next refetch.
|
|
*/
|
|
export function useAccountIdentity(): { userId: string | null; settled: boolean } {
|
|
const { data: session, isSuccess } = useQuery(sessionQueryOptions);
|
|
return { userId: session?.user.id ?? null, settled: isSuccess };
|
|
}
|
|
|
|
/**
|
|
* Observe the company list for the account signed in now.
|
|
*
|
|
* Pass `enabled: false` to hold for a caller's own reasons; the session gate is
|
|
* applied on top of it either way.
|
|
*/
|
|
export function useCompanyListQuery(
|
|
options: { enabled?: boolean; staleTime?: number; retry?: number | boolean } = {},
|
|
) {
|
|
const { userId, settled } = useAccountIdentity();
|
|
const query = useQuery<CompanyListResult>({
|
|
...companyListQueryOptions(userId),
|
|
...options,
|
|
enabled: settled && (options.enabled ?? true),
|
|
});
|
|
|
|
// While the account is unknown this query is disabled, and a disabled query
|
|
// reports `isLoading: false` with `data: undefined` — which consumers default
|
|
// to an empty list and read as "asked, and owns nothing". That is the
|
|
// destructive reading: `shouldClearStoredCompanySelection` would throw away
|
|
// the customer's stored company on every cold boot, before the session had
|
|
// even landed.
|
|
//
|
|
// Waiting for the account is part of getting the list, so it is reported as
|
|
// part of getting the list. Consumers gate on these two, and both must mean
|
|
// "no answer yet" for the whole window in which there is no answer.
|
|
const waitingForAccount = !settled && (options.enabled ?? true);
|
|
return {
|
|
...query,
|
|
isLoading: query.isLoading || waitingForAccount,
|
|
isFetching: query.isFetching || waitingForAccount,
|
|
};
|
|
}
|
|
|
|
/**
|
|
* Resolve the account identity for an imperative path, fetching the session if
|
|
* the cache has no answer yet.
|
|
*
|
|
* Not `getQueryData` with a `?? null` fallback: an empty or errored session
|
|
* entry would read as "signed out" and key an authenticated response to
|
|
* `anonymous`. There is no safe default here — the identity is either known or
|
|
* must be obtained, and if it cannot be obtained the caller must fail rather
|
|
* than guess.
|
|
*/
|
|
export async function resolveAccountUserId(queryClient: QueryClient): Promise<string | null> {
|
|
const state = queryClient.getQueryState<Awaited<ReturnType<typeof authApi.getSession>>>(
|
|
queryKeys.auth.session,
|
|
);
|
|
// `isInvalidated` matters as much as `status` here. Invalidation is how the
|
|
// app says "this is the previous account's answer" — InviteLanding invalidates
|
|
// the session immediately after a sign-in and resolves an identity in the next
|
|
// breath. The entry is still `success`, still holding the *old* user, until a
|
|
// refetch lands; trusting it would key the new account's list to the old
|
|
// account's id, which is the misattribution this whole file exists to prevent.
|
|
if (state?.status === "success" && !state.isInvalidated) {
|
|
return state.data?.user.id ?? null;
|
|
}
|
|
const session = await queryClient.fetchQuery({ ...sessionQueryOptions, staleTime: 0 });
|
|
return session?.user.id ?? null;
|
|
}
|
|
|
|
/** Fetch the company list for the account signed in now, ignoring what is cached. */
|
|
export async function fetchCompanyListForCurrentAccount(
|
|
queryClient: QueryClient,
|
|
): Promise<CompanyListResult> {
|
|
const userId = await resolveAccountUserId(queryClient);
|
|
return queryClient.fetchQuery({
|
|
...companyListQueryOptions(userId),
|
|
staleTime: 0,
|
|
});
|
|
}
|