mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-07 07:23:08 +02:00
TonioandClaude Opus 5
327f59cac2
## 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>
@paperclipai/ui
Published static assets for the Paperclip board UI.
What gets published
The npm package contains the production build under dist/. It does not ship the UI source tree or workspace-only dependencies.
Storybook
Storybook config, stories, and fixtures live under ui/storybook/.
pnpm --filter @paperclipai/ui storybook
pnpm --filter @paperclipai/ui build-storybook
Typical use
Install the package, then serve or copy the built files from node_modules/@paperclipai/ui/dist.