fix(frontend): package cache writes address the host's QueryClient - #5915
Draft
ardaerzin wants to merge 82 commits into
Draft
fix(frontend): package cache writes address the host's QueryClient#5915ardaerzin wants to merge 82 commits into
ardaerzin wants to merge 82 commits into
Conversation
…token source for oss, ee and mobile
…d transform leave the app
…d file surfaces leave the app
…ation-ui — one shell for desktop and mobile
…ove into @agenta/playground-ui
… into @agenta/sessions-ui
…ent roster leave the app
This lane reintroduces `first_agent_intent` call sites that the same fix on `pkg/session-surfaces` couldn't see, so they still send the raw "describe your agent" text under a `message` property. `truncateForCapture` only capped length — a composer message can hold credentials, customer data or a pasted secret, and none of that belongs in an analytics property. `classifyAgentIntent` is the signal the module says it exists to capture: it buckets the message into support/research/ops/content/coding/data/other. The two free-text submit paths already sent that bucket alongside the raw text, so dropping the text loses nothing there; both coding-agent-copy paths (the home strip and the onboarding chat composer) gain the classification they never had. `truncateForCapture` and `MESSAGE_CAPTURE_LIMIT` go with their last reference.
…bounce behind AGENTA_MOBILE_REVERSE_GATE
The tabbed workspace decides live-vs-replay on entityId alone, which is null while the agent query is still pending — so an agent-backed session rendered the replay branch and its "no agent to message yet" notice until resolution landed. Hold the chat slot on ChatLoading until useAgentEntity settles.
…joins the components in @agenta/entity-ui Every component on the agent overview was already shared; only the two-column arrangement was still written twice, inline in the desktop page and again in the mobile screen. AgentOverviewLayout takes main/rail slots and owns the geometry, so /m stops rendering a single 768px column at desktop widths and the two surfaces cannot drift. `scroll` distinguishes the hosts: the desktop page is a full-height frame whose columns scroll independently, while mobile's ScreenScaffold already owns the scroller.
…st the frame The components on this page were already shared; the ARRANGEMENT was not, so /m assembled its own sections in its own order and was missing Files and Usage entirely. AgentOverviewBody is the whole body — composer slot, both list cards, and the rail in one order (what the agent IS, CARRIES, WILL DO, COST) — rendered by the desktop page and the mobile screen alike. SessionListCard moves to @agenta/sessions-ui with the three app verbs as props (mobile's useSessionRowMenu already returns exactly those), replacing mobile's own AgentOverviewSection. AgentFilesCard moves to @agenta/entity-ui/agent, de-antd'd so mobile can render it. The OSS wrappers those two replaced are gone.
/m already had a start-session composer on Home; the agent page had none. Rather than a second composer, HomeTaskComposer gains fixedAgentId — with it the picker drops, because an agent's own page already answers which agent and a picker there would let you send from it to a different one. AgentComposer is the mobile binding, on HomeComposer's mint-stash-route flow so attachments staged before the session exists keep their scope.
…s its margins ContentRail renders a plain div, so the body's `flex-1`/`h-full` columns had no definite height to resolve against and the left one scrolled inside a stunted box. Making the wrapper a flex column restores the chain. Padding scales with the viewport — tight on a phone, desktop-like margins from lg.
…sktop margins UsageCard already serves /m's home and takes an appId, so the agent rail scopes it to this agent — the same figures the desktop strip shows. Margins go from 24px to 64px at lg, measured against the desktop page rather than guessed.
The kebab lived in the OSS page: an antd Dropdown wired to app-management modals, with /m offering nothing. AgentActionsMenu is the shared verb set — rename, copy id, copy slug, delete — on Radix, and useAgentActions is its default implementation (updateWorkflow / archiveWorkflow, both already in entities), built the way useSessionActions already does shared verbs. Rename and delete take host overrides so the desktop keeps its app-management modals, which validate and refresh the apps cache; /m has none and falls through to the shared ones. Copying is shared outright. The swap left the OSS header's antd Dropdown, Button, MoreOutlined and copyToClipboard imports dead; they're gone.
The title carried flex-1, so it absorbed the row and pushed the menu to the opposite end. Sizing it to its text puts the two together as on the desktop; min-w-0 keeps a long name truncating.
… shared ones Both were desktop-only. SidebarToggleButton moves to @agenta/navigation-ui — it needed nothing OSS-specific once sidebarCollapsedAtom was in @agenta/navigation and EnhancedButton was antd-free — and takes collapsedAtom as a prop so it does not quietly bypass SidebarShell's own contract. The OSS path stays a re-export because settingsScope imports it too. The version rides buildHelpDocsNavItem's suffix, lazy-loaded from mobile's own package.json.
Help & Docs is a group inside a `vertical` bottom section, so NavMenu draws it through the flyout trigger, not RowLabel — where the suffix had been ported. The label was therefore missing on BOTH the desktop rail and /m. Drawn on both paths now; the collapsed icon rail still omits it, as 111 intended.
… antd items useSessionCardVerbs handed actions.menuItems() straight to SessionListCard's menuFor, which is typed (vm) => SessionMenuEntry[]. antd's MenuProps["items"] admits null entries, groups and submenus, so the shapes do not line up. toSessionMenuEntries is the existing converter the sessions page already used.
… spine leaves the app The tab model, the static-table helper and the page shell were already pure: no antd, no @/oss. They move verbatim (git mv, so 111's featureFlags->preferences rename keeps its history) and OSS keeps its three import paths as re-exports, so none of the 13 tab files move. Access is injected rather than read: useSettingsAccess needs edition detection, project permissions and org state, none of which exist in a package. The package owns the SettingsAccess shape and the visibility rules; each host computes the flags. A host with no provider gets CLOSED_SETTINGS_ACCESS, so a missing provider hides gated tabs instead of exposing them.
Both were presentation over host state, so both extract cleanly once de-antd'd: Typography -> tokens, Switch -> Radix, Radio -> RadioGroup, Divider -> a rule, danger Button -> variant=destructive, --ant-color-error* -> colorError* tokens. Theme is shared because it is a per-viewer choice both apps honour; the experiment flags stay host-supplied, since a flag only exists where it ships. AccountPage keeps the typed-email gate but delegates the dialog through renderConfirm, so no host can ship a one-click account delete.
First settings extraction with a data half rather than presentation only. The service was 46 lines over axios + the project id, both already in @agenta/shared/api, so it moves with an import swap. OSS re-exports it, keeping one implementation for its three consumers — one of which is in EE, so this is also the first settings piece to cross editions. The APIKeys page itself is untouched; splitting its fetch/permissions/dialogs into a headless hook plus a view is the next unit.
/m's settings was a placeholder saying to use the desktop. It now renders the SHARED page shell and Preferences tab, with the title and description read from the shared tab model so the copy cannot drift from the desktop's. useThemeMode extracts the half of the desktop's ThemeContextProvider both apps need — the stored preference, the theme it resolves to, and the .dark class — keyed off the same storage entry, so a viewer's choice follows them between surfaces. The antd ConfigProvider half stays in the app. Preferences only, deliberately: it is the one tab needing no profile, org or permission state. Access stays CLOSED so nothing edition-gated can leak in before /m can compute real flags.
…ccount tab The signed-in user had no package source, so /m could not render Account. fetchProfile was a three-line GET and User already lived in @agenta/shared/types, so both move into entities with a useProfile hook beside them; OSS re-exports the fetcher, keeping one implementation. The hook stays thinner than the desktop's profile atom, which also persists to disk, gates a fanout and redirects — app concerns, not entity ones. AccountPage's deletion becomes optional as a pair (action + dialog): deleting an account is an EE capability that tears down owned orgs, and /m has no EE surface, so it renders identity only instead of a button that cannot work.
…troller Two implementations of read-agenta-theme / resolve / toggle-.dark existed once /m gained its own. The state half now comes from @agenta/ui/theme; what stays is the part only this app has — antd's ConfigProvider, the darkAlgorithm token config and the agenta cssVar key class. Public API is unchanged (ThemeMode, ThemeContext, useAppTheme, getDeviceTheme), so no consumer moves. useLocalStorage, useLazyEffect, useState and the local getAppTheme fell out with the state and are gone. Behaviour note: appTheme was seeded synchronously from storage at first render; the hook reads after mount instead, because reading localStorage during render is a hydration mismatch. The document boot script has already applied the class, so there is no flash.
The tab interleaved fetching, permissions, two dialogs and the table in one component. useApiKeys owns the list and the verbs; the parts that are genuinely the host's arrive as callbacks — confirmDelete resolves from its own dialog, and onCreated receives the secret, because a key is returned once and the host has to reveal and offer to copy it there and then. ApiKeysPage is the view, off antd (Alert/Button/Tooltip onto @agenta/ui). The Loading enum in assets/constants.ts went with the old multi-flag loading state: the hook returns listing/creating/deleting.
…lling service useBillingSubscription is now the single reader, shared by the entitlement gates and by whoever names the plan, so the two cannot drift apart or race for the same cache entry. It carries the desktop's retry policy, which was missing here: never retry a 4xx, and never retry 502/503/504. A billing service that is down answers every request that way, and the default three attempts just queue slow failures behind render-critical traffic.
…genta/settings-ui The package could read usage and the subscription but not act on either, so a host with no billing layer could only report a plan, never change one. Adds the four writes behind the page — switch, cancel, Stripe checkout, Stripe portal — and the two catalog reads. useBillingCatalog derives the pair the page cannot guess: which slug is the free tier (the pricing map says, and slugs are env-overridable) and whether the current plan is contact-sales. Without them the chooser offers to "upgrade" to the free plan and sends a downgrade through checkout instead of cancellation.
Triggers is missing from EE settings because it is gated on NEXT_PUBLIC_AGENTA_TOOLS_ENABLED, which is declared nowhere in hosting or CI — so it is undefined everywhere and both Tools and Triggers stay hidden. Nothing regressed; the flag has simply never been set. (One flag drives both tabs, which is worth revisiting: Triggers has little to do with the tool catalog.) /m had these hardcoded true, so it showed both tabs while the desktop hid them — the opposite of aligning the two surfaces. It also tested `license === "ee"`, missing the `cloud*` tiers the desktop's isEE() accepts. The four env-only gates move to @agenta/shared/api, which both hosts already import, and oss/lib/helpers/isEE.ts keeps only isEmailAuthEnabled — that one reads resolved auth config, not a bare env var. 15 call sites repointed. Mobile's build-time env allowlist gains the license, tools and billing keys. Without them a built image reads "" no matter how the container is configured, because `process.env[key]` with a computed key is not inlined. Gates: lint 24/24, tsc 0 across shared, oss, ee and mobile.
The previous commit claimed this and did not do it. The settings feature had been refactored in the meantime — the access object moved to settingsTabs.ts — so the string replace matched nothing and no-op'd silently, and lint/tsc stayed green because nothing was broken, only unchanged. canShowTools/canShowTriggers now come from isToolsEnabled(), and isEE from the shared isEE() rather than a bare `=== "ee"` that misses the cloud tiers. settingsTabs.ts is the only place mobile builds SettingsAccess, so this is the whole surface.
readOnly was a guard against the create/edit drawers rendering antd forms with no ConfigProvider here. That reason is gone — SchemaForm and SubscriptionForm moved to @rc-component/form, so the drawers carry no antd theming. Destructive actions in those sections ask for confirmation through an imperative callback (the desktop passes antd's AlertPopup). useConfirmSheet adapts the existing ConfirmSheet, which is controlled, to that shape, and reports a failed action in the sheet rather than swallowing it — this app has no toasts. Subscriptions and schedules take no confirm: they delete through their own handlers, not through the shared prompt. Gates: lint 24/24, tsc 0 on mobile. Not yet exercised in a browser — the drawers and the confirm path are the things to try.
A section with a title but no search rendered the heading, then the buttons on their own row below — a band of empty space between them, and the buttons sitting closer to the table than to the section they act on. The three Triggers sections all read that way. The actions now ride the title's row when there is one, and fall back to the end of the filter row when there is not (Tools, which searches and has no title, is unchanged). A section with both gets the actions beside the title and the search on its own row, since the search box is too wide to share. Aligned to the top of the title block, which is two lines — name over description. Gates: lint 24/24, tsc 0 across ui, settings-ui, oss and mobile.
Four narrow-viewport fixes, all in the primitive so every settings tab gets them: The header row stacks below sm instead of wrapping. `ml-auto` meant the buttons landed hard-right once they wrapped, leaving a gap under the description — they now span the row. The empty state's call to action is the only one shown on a phone. The toolbar carries the same button, and at this width the two sit far enough apart to read as different controls. Checked every caller first: the CTA-less empty states are all search-no-match ones, where the search box is right there, so nothing is left without an action. Search goes full-width below sm; a fixed 260px overflowed. The empty region loses some height on narrow — three stacked sections were mostly whitespace before reaching content. Not fixed here: the table still compresses rather than scrolling, because it is `w-full` with no min-width. Five columns in 360px is the real problem and wants a card-per-row layout below sm — its own change. Gates: lint 24/24, tsc 0 across ui, settings-ui, oss and mobile.
A full row for one icon button is too much. Reload now stays beside the section title at every width; only the primary button — wide, labelled — wraps to its own full-width row below sm, and rejoins the line from sm up. With the empty state hiding the primary button on a phone, the header collapses to a single line: title, description, reload. Only this file is staged — the other changes in the tree are another agent's work in flight (mobile's tsc is failing on jotai-family resolution and some nav/chat edits that have nothing to do with this).
…entity-ui OSSdrillInUIProvider carried 588 lines of workflow-reference wiring that was not OSS-specific — /m needs the same behaviour to render a config pane, and could not reach it. It becomes useWorkflowReferenceBridge in the package, and the provider keeps only the host's own composition. The bridge imports nothing from @/oss, so it holds in a host that cannot see the app layer. Lane: this belongs with the entity-ui work, above pkg/session-surfaces — see docs/design/sessions-ux-stack/restack-onto-112.md.
Consumes useWorkflowReferenceBridge through a 20-line provider, so /m gets the reference behaviour the desktop has instead of a second implementation. Lane: mobile, and it depends on the bridge commit below it.
…eclare it Six packages imported `atomFamily` from `jotai/utils` while resolving it through a transitive copy — @agenta/mobile was failing tsc on `Cannot find module 'jotai-family'` because nothing in its graph declared the dep. Every call site now imports from `jotai-family`, and each package that uses it lists it. 46 files, one behavioural change: none. Lane: this is package-wide plumbing and belongs below the settings lanes — see docs/design/sessions-ux-stack/restack-onto-112.md.
The title rules — normalize, truncate on a character boundary, join the parts — lived in oss/components/PageTitle/utils and /m had its own `parts` array doing roughly the same thing. They now share one implementation, with its test. The surrogate-pair guard is the reason this is worth sharing rather than reimplementing: a server-persisted session title can arrive cut mid-pair, and the trailing high surrogate renders as a replacement character. Each host's PageTitle is now just the <title> element for its router. Lane: @agenta/shared, below everything else; the oss and mobile rewires ride with it since the old util is deleted in the same change.
The SDK guards those imports with try/catch, so webpack resolving them is noise. Production only — dev runs Turbopack.
Package code reached the TanStack cache through the `queryClient` singleton exported by @agenta/shared/api. That only works when the host passes the same object to <QueryClientProvider> and hydrates queryClientAtom with it. /m installed its own client, so the singleton's cache was empty there and every package-layer invalidateQueries / setQueryData / removeQueries was a silent no-op — mutations returned success, toasts fired, and nothing refreshed until a reload. Add getHostQueryClient(), which reads the client the host actually installed off queryClientAtom, and route all 17 package call sites through it, resolved per call so nothing captures a pre-hydration value. Mobile's queryClient.ts now re-exports the shared singleton and merges its defaults onto it instead of replacing it. A dev-only assertion in queryClient.ts flags a host that installed a rival client on first package-layer cache access.
no-restricted-imports bans the named `queryClient` import from @agenta/shared/api and from the @agenta/shared root barrel, which re-exports it. The paths list is exported as `restrictedImportPaths`; the 11 package configs that override the rule spread it back in so none of them silently drops the bans. no-restricted-imports does not see `await import(...)`, and the singleton was reachable that way. A no-restricted-syntax rule bans dynamic imports of both modules outright — a selector matching only the destructured name would still miss `const m = await import(...); m.queryClient`. Package code that needs the client lazily imports the narrow @agenta/shared/api/hostQueryClient subpath instead. Both bans were probed with a deliberate violation: passing before, erroring after. Unit tests cover the dev assertion: fires once on a mismatched host, stays quiet on a matching one, and is off in production and SSR.
web/AGENTS.md states the asymmetric rule: a host passes @agenta/shared/api's queryClient to <QueryClientProvider> and hydrates queryClientAtom with the same object; package code never imports the singleton and calls getHostQueryClient() per use. The agenta-package-practices skill points at it. docs/design/query-client-host-divergence/ carries the investigation — the symptom, the root cause, the dead ends, and the probe recipe for checking whether a host is wired correctly — plus the /m verification sweep: nine flows driven live, each checked by stamping window before the mutation and asserting the stamp survived alongside the row-set change, so a surviving stamp proves the page never reloaded. It also records what was NOT verified and why (the OAuth leg needs a real Google consent; runnable/deploy.ts has no reachable surface on mobile).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change establishes a shared QueryClient host contract. Mobile reuses the shared client, package cache operations resolve it per call, runtime assertions detect mismatches, and ESLint restrictions prevent direct or dynamic singleton imports. ChangesQueryClient host contract
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MobileApp
participant queryClientAtom
participant getHostQueryClient
participant PackageMutation
MobileApp->>queryClientAtom: hydrate shared QueryClient
PackageMutation->>getHostQueryClient: resolve client per call
getHostQueryClient->>queryClientAtom: read host client
getHostQueryClient-->>PackageMutation: return host QueryClient
PackageMutation->>MobileApp: invalidate or update host cache
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
docs/design/query-client-host-divergence/handoff.md (1)
141-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required Turbo checks.
The checklist runs
pnpm lint-fix, TypeScript checks, and one unit test, but it does not run Turbo build and lint for affected packages. Add those commands before the handoff calls the static gates complete.As per coding guidelines, before pushing package changes, verify the affected package with Turbo build and lint commands.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c9c5dda-3ea5-41b7-8955-9ca35438048f
📒 Files selected for processing (40)
.agents/skills/agenta-package-practices/SKILL.mddocs/design/query-client-host-divergence/handoff-2.mddocs/design/query-client-host-divergence/handoff.mddocs/design/query-client-host-divergence/plan.mdweb/AGENTS.mdweb/mobile/src/lib/queryClient.tsweb/packages/agenta-annotation/src/state/controllers/annotationFormController.tsweb/packages/agenta-annotation/src/state/controllers/annotationSessionController.tsweb/packages/agenta-auth-ui/eslint.config.mjsweb/packages/agenta-auth/eslint.config.mjsweb/packages/agenta-entities/src/gatewayTool/hooks/useToolConnectionActions.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerConnectionActions.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSchedule.tsweb/packages/agenta-entities/src/gatewayTrigger/hooks/useTriggerSubscription.tsweb/packages/agenta-entities/src/gatewayTrigger/state/optimistic.tsweb/packages/agenta-entities/src/runnable/deploy.tsweb/packages/agenta-entities/src/shared/invalidation/index.tsweb/packages/agenta-entities/src/webhook/atoms.tsweb/packages/agenta-entity-ui/eslint.config.mjsweb/packages/agenta-entity-ui/src/gatewayTool/drawers/ConnectDrawer.tsxweb/packages/agenta-entity-ui/src/gatewayTool/drawers/ConnectionManagerDrawer.tsxweb/packages/agenta-entity-ui/src/gatewayTool/hooks/useReconnectToolConnection.tsweb/packages/agenta-entity-ui/src/gatewayTrigger/drawers/TriggerConnectDrawer.tsxweb/packages/agenta-home-ui/eslint.config.mjsweb/packages/agenta-navigation-ui/eslint.config.mjsweb/packages/agenta-navigation/eslint.config.mjsweb/packages/agenta-observability/eslint.config.mjsweb/packages/agenta-playground/src/state/controllers/playgroundController.tsweb/packages/agenta-playground/src/state/controllers/traceRefResolution.tsweb/packages/agenta-sessions-ui/eslint.config.mjsweb/packages/agenta-sessions/eslint.config.mjsweb/packages/agenta-settings-ui/eslint.config.mjsweb/packages/agenta-settings-ui/src/tools/hooks/useToolsConnections.tsweb/packages/agenta-settings/eslint.config.mjsweb/packages/agenta-shared/package.jsonweb/packages/agenta-shared/src/api/hostQueryClient.tsweb/packages/agenta-shared/src/api/index.tsweb/packages/agenta-shared/src/api/queryClient.tsweb/packages/agenta-shared/tests/unit/hostQueryClientAssertion.test.tsweb/packages/eslint.config.mjs
Comment on lines
+21
to
+31
| Worktree `.claude/worktrees/sessions-ux`, currently on branch `docs/sessions-ux-stack` (top of a | ||
| 30-PR stack, PRs #5865–#5894). Before touching anything: | ||
|
|
||
| ```bash | ||
| cd .claude/worktrees/sessions-ux | ||
| git status --porcelain # expect: M web/mobile/src/lib/queryClient.ts, ?? docs/design/query-client-host-divergence/ | ||
| git log --oneline -1 | ||
| ``` | ||
|
|
||
| That modified mobile file is the already-verified fix — **do not revert it**, and do not fold your | ||
| work into it. Ask where the mobile fix should land before committing it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove transient branch state from the durable handoff.
Lines 21-31 require .claude/worktrees/sessions-ux, branch docs/sessions-ux-stack, and a specific dirty status. That state will be false after the stack advances or this PR lands. Record it as historical context or replace it with repository-relative instructions.
| `web/mobile/src/lib/queryClient.ts` now re-exports the shared singleton and merges mobile's | ||
| defaults onto it rather than replacing them (`setDefaultOptions` is a whole-object write, so | ||
| spreading the existing `queries` preserves `experimental_prefetchInRender` set by the package | ||
| layer). `AppProviders` needed no change. Verified live on `/m`: create, delete and connect all |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the verification summary.
Line 60 says that “connect” was verified live. Lines 186 and 194-195 say that the Tool OAuth leg was not completed. If “connect” refers to that flow, mark it partial or name the exact flow that passed. This follows the coverage details later in this document.
Comment on lines
+224
to
+226
| All three ids equal → wired correctly. An `undefined` id on `atom`/`provider`, or an **empty | ||
| `getAll()`** on the singleton, means the host installed a rival client and every package-layer | ||
| write is a no-op. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat an empty cache as supporting evidence only.
Line 225 calls an empty singleton cache proof of a rival client. A correctly wired client can have no query instances before a query mounts. Require matching client identities and a known query key before concluding that the host installed a rival. TanStack Query documents empty results when no queries exist. (tanstack.com)
Comment on lines
+3
to
+6
| // The SAME client the packages hold. `@agenta/entities` and friends reach the cache through the | ||
| // `@agenta/shared/api` singleton directly (16 files, ~70 calls), so a host that installs a client | ||
| // of its own gets working reads and silently dead writes — every mutation leaves the list stale | ||
| // until a reload. Desktop passes this singleton; `/m` must too. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the mobile client comment.
The migrated package call sites now resolve getHostQueryClient() at operation time. The current wording says that @agenta/entities and related packages still access the singleton directly, and the 16 files/~70 calls count is stale. Rewrite the comment to describe the host hydration contract.
Proposed wording
-// The SAME client the packages hold. `@agenta/entities` and friends reach the cache through the
-// `@agenta/shared/api` singleton directly (16 files, ~70 calls), so a host that installs a client
-// of its own gets working reads and silently dead writes — every mutation leaves the list stale
-// until a reload. Desktop passes this singleton; `/m` must too.
+// Package cache operations resolve the host-installed client at call time.
+// Mobile must hydrate the shared singleton into `queryClientAtom`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The SAME client the packages hold. `@agenta/entities` and friends reach the cache through the | |
| // `@agenta/shared/api` singleton directly (16 files, ~70 calls), so a host that installs a client | |
| // of its own gets working reads and silently dead writes — every mutation leaves the list stale | |
| // until a reload. Desktop passes this singleton; `/m` must too. | |
| // Package cache operations resolve the host-installed client at call time. | |
| // Mobile must hydrate the shared singleton into `queryClientAtom`. |
Comment on lines
7
to
+11
| // Tools and triggers are independent surfaces over the SAME shared | ||
| // `gateway_connections` rows, so a write here must also invalidate the triggers | ||
| // list — otherwise a connection removed from tools would read as stale there. | ||
| const invalidateConnections = () => { | ||
| const queryClient = getHostQueryClient() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cover the direct tool-drawer mutation path.
In web/packages/agenta-entity-ui/src/gatewayTool/drawers/ConnectDrawer.tsx, Lines 10-15, the local invalidateConnections helper invalidates only ["tools", "connections"] and ["tools", "catalog"]. It does not invalidate ["triggers", "connections"]. A tool connection write can therefore leave the trigger list stale. Add the trigger invalidation there or reuse this helper.
Proposed fix
queryClient.invalidateQueries({queryKey: ["tools", "catalog"]})
+queryClient.invalidateQueries({queryKey: ["triggers", "connections"]})
Comment on lines
+17
to
+19
| /** | ||
| * Dev-only: a host that installs its own QueryClient makes every package-layer write a silent no-op. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the diagnostic describe the post-migration failure mode.
getHostQueryClient() now returns the client stored in queryClientAtom. A rival host client is therefore the client used by migrated package writes, not automatically an orphan cache. The JSDoc and console message still describe the pre-migration singleton path and can send debugging to the wrong conclusion. Keep the shared-singleton contract check, but report a contract violation or identify remaining direct singleton consumers instead. This follows the supplied getHostQueryClient() implementation.
Also applies to: 32-36
Comment on lines
+28
to
+84
| it("stays silent when the host installed the shared singleton", async () => { | ||
| const {queryClient, assertHostQueryClient} = await loadQueryClientModule() | ||
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined) | ||
|
|
||
| assertHostQueryClient(() => queryClient) | ||
| vi.runAllTimers() | ||
|
|
||
| expect(error).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("reports a host that installed a rival client", async () => { | ||
| const {assertHostQueryClient} = await loadQueryClientModule() | ||
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined) | ||
| const rival = new QueryClient() | ||
|
|
||
| assertHostQueryClient(() => rival) | ||
| vi.runAllTimers() | ||
|
|
||
| expect(error).toHaveBeenCalledOnce() | ||
| expect(error.mock.calls[0][0]).toContain("Host QueryClient mismatch") | ||
| }) | ||
|
|
||
| it("does not flag a host that hydrates late", async () => { | ||
| const {queryClient, assertHostQueryClient} = await loadQueryClientModule() | ||
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined) | ||
| let hydrated = false | ||
|
|
||
| assertHostQueryClient(() => (hydrated ? queryClient : new QueryClient())) | ||
| vi.advanceTimersByTime(1) | ||
| hydrated = true | ||
| vi.runAllTimers() | ||
|
|
||
| expect(error).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("checks only once", async () => { | ||
| const {assertHostQueryClient} = await loadQueryClientModule() | ||
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined) | ||
| const rival = new QueryClient() | ||
|
|
||
| assertHostQueryClient(() => rival) | ||
| assertHostQueryClient(() => rival) | ||
| vi.runAllTimers() | ||
|
|
||
| expect(error).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it("does not run in production", async () => { | ||
| process.env.NODE_ENV = "production" | ||
| const {assertHostQueryClient} = await loadQueryClientModule() | ||
| const error = vi.spyOn(console, "error").mockImplementation(() => undefined) | ||
|
|
||
| assertHostQueryClient(() => new QueryClient()) | ||
| vi.runAllTimers() | ||
|
|
||
| expect(error).not.toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'getHostQueryClient|queryClientAtom' web/packages/agenta-shared/tests || trueRepository: Agenta-AI/agenta
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
fd -a 'hostQueryClient|api/index|\bapi\b' web/packages/agenta-shared/src web/packages/agenta-shared/tests 2>/dev/null | sed 's#^\./##' || true
echo "== test file outline =="
ast-grep outline web/packages/agenta-shared/tests/unit/hostQueryClientAssertion.test.ts --view compact || true
echo "== test relevant contents =="
cat -n web/packages/agenta-shared/tests/unit/hostQueryClientAssertion.test.ts | sed -n '1,130p'
echo "== source api files candidates =="
fd 'hostQueryClient\.ts$|index\.ts$' web/packages/agenta-shared/src/api 2>/dev/null | while read -r f; do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,180p'
doneRepository: Agenta-AI/agenta
Length of output: 6776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== queryClient source =="
cat -n web/packages/agenta-shared/src/api/queryClient.ts | sed -n '1,180p'
echo "== api index line 33 context =="
cat -n web/packages/agenta-shared/src/api/index.ts | sed -n '1,80p'
echo "== shared tests references to accessor/client boundaries =="
rg -n -C 4 'getHostQueryClient|queryClientAtom|assertHostQueryClient|queryClient' web/packages/agenta-shared/tests || true
echo "== package-wide tests references under web (focused) =="
rg -n -C 3 'getHostQueryClient|queryClientAtom|assertHostQueryClient' web --glob '*.{test,spec}.{ts,tsx,js,jsx}' || trueRepository: Agenta-AI/agenta
Length of output: 35555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== queryClient source =="
cat -n web/packages/agenta-shared/src/api/queryClient.ts | sed -n '1,180p'
echo "== api index line 33 context =="
cat -n web/packages/agenta-shared/src/api/index.ts | sed -n '1,80p'
echo "== shared tests references to accessor/client boundaries =="
rg -n -C 4 'getHostQueryClient|queryClientAtom|assertHostQueryClient|query.6Client' web/packages/agenta-shared/tests || true
echo "== package-wide tests focused references =="
rg -n -C 3 'getHostQueryClient|queryClientAtom|assertHostQueryClient' web --glob '*.{test,spec}.{ts,tsx,js,jsx}' || trueRepository: Agenta-AI/agenta
Length of output: 35234
Cover the getHostQueryClient() boundary.
These tests resolve assertHostQueryClient directly instead of hydrating queryClientAtom and calling getHostQueryClient(). The accessor and re-export now read from jotai storage, so add one test for that path.
Comment on lines
+47
to
+59
| { | ||
| name: "@agenta/shared/api", | ||
| importNames: ["queryClient"], | ||
| message: | ||
| "Use `getHostQueryClient()` from '@agenta/shared/api' — the `queryClient` singleton is only for hosts to pass to <QueryClientProvider>; package writes on it are dead when the host installed its own client.", | ||
| }, | ||
| // Same singleton, reachable through the root barrel's re-export. | ||
| { | ||
| name: "@agenta/shared", | ||
| importNames: ["queryClient"], | ||
| message: | ||
| "Use `getHostQueryClient()` from '@agenta/shared/api' — the `queryClient` singleton is only for hosts to pass to <QueryClientProvider>; package writes on it are dead when the host installed its own client.", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In ESLint 9.39.4, does the no-restricted-imports rule with importNames: ["queryClient"] reject import * as sharedApi from "@agenta/shared/api"? What configuration rejects namespace imports from a specific module?
💡 Result:
In ESLint 9.39.4, the no-restricted-imports rule with importNames: ["queryClient"] does not reject namespace imports like import * as sharedApi from "@agenta/shared/api". The importNames option specifically restricts only the named imports that match the strings provided in the array [1][2]. Since namespace imports (e.g., import * as ...) do not import specific named bindings, they remain unaffected by the importNames configuration [2]. To restrict imports from a specific module entirely, you must configure the rule to target the module name without specifying importNames. This will prevent all forms of imports, including namespace imports, from that path [1][3]. Example configuration to reject all imports (including namespace imports) from a specific module: { "rules": { "no-restricted-imports": ["error", { "paths": [{ "name": "@agenta/shared/api", "message": "Direct imports from @agenta/shared/api are restricted." }] }] } } If your goal is to specifically target namespace imports while allowing other types of imports, the core no-restricted-imports rule does not have a dedicated option to restrict them. The rule is designed to restrict specific imported names or the module path as a whole [1].
Citations:
- 1: https://eslint.org/docs/latest/rules/no-restricted-imports
- 2: linter:
no-restricted-importsincorrectly flags namespace imports whenimportNames: ["default"]is used oxc-project/oxc#19237 - 3: https://stackoverflow.com/questions/66330715/eslint-no-restricted-imports-only-restrict-importing-from-a-package-root
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== eslint config excerpt =="
sed -n '33,70p' web/packages/eslint.config.mjs | cat -n
echo
echo "== package metadata and no-restricted-imports usages =="
rg -n '"eslint"|"`@eslint/js`"|no-restricted-imports|restrictedImportPaths|restrictedSyntax|restrictedImports' web/packages web -g '!node_modules' -g '!dist' -g '!build' | head -200
echo
echo "== available eslint/cli tooling =="
for f in web/packages/eslint.config.mjs web/package.json pnpm-lock.yaml; do
[ -f "$f" ] && echo "--- $f ---" && sed -n '1,140p' "$f"
done
git ls-files 'web/packages/eslint.config.mjs' 'web/package.json' 'pnpm-lock.yaml' | xargs -r sed -n '1,160p'Repository: Agenta-AI/agenta
Length of output: 29452
Block namespace access to queryClient.
importNames: ["queryClient"] only blocks named imports. import * as sharedApi from "@agenta/shared/api" bypasses the rule, so package code can still use sharedApi.queryClient. Keep a targeted configuration: add a separate restricted path entry without importNames, or a no-restricted-syntax selector that blocks namespace imports to these entry points.
- mobile's queryClient comment described the pre-migration world (packages reaching the singleton directly, a stale 16-files/~70-calls count). It now states the contract: packages resolve the host client at call time, hosts hydrate the atom. - The probe recipe called an empty singleton cache proof of a rival client. It is not — a correctly wired client is also empty before any query mounts. Demoted to supporting evidence; the id mismatch is the conclusive tell. - The "verified live" line predated the sweep and claimed connect was covered; the WP3 table says its OAuth leg was not. Narrowed to what actually passed. - Both handoffs marked HISTORICAL — their branch/worktree/dirty-state instructions are spent and would mislead the next reader. - Recorded WP5: ConnectDrawer never invalidates ["triggers", "connections"]. Pre-existing (identical on the base) and deliberately left out of this lane, which stays a pure "which client is addressed" change.
Contributor
Author
|
Thanks — three of the doc/comment nits were right and are fixed in 079bc20. Two notes on the Majors:
No config change needed. (The hole that was real is dynamic
This lane only changes which client is addressed; adding an invalidation is a behaviour change, the same reason WP4 is excluded here. Recorded as WP5 in
|
Contributor
Author
Unplanned A/B: the bug reproduced on the unfixed base, minutes laterWhile cleaning up test data I was checked out on a different branch off the same base ( Renaming a trigger subscription there: the mutation persisted server-side but the table did not change. The new name only appeared after a full reload. That is the original symptom, exactly as reported. Same browser session, same project, same dev stack, a few minutes after the verification runs on this branch — where create, delete and the optimistic toggle all settled with no reload. The only variable is this change. That is the control the original sweep didn't have: it shows the passes on this branch are the fix working, not the bug happening to be dormant. It also names one more affected path — subscription edit/rename goes through the same dead invalidation, and was not in the WP3 table. |
ardaerzin
force-pushed
the
docs/sessions-ux-stack
branch
2 times, most recently
from
August 11, 2026 06:01
6fdcb88 to
15c246b
Compare
ardaerzin
added a commit
that referenced
this pull request
Aug 15, 2026
…ches Review findings from #5915. `ConnectDrawer` kept its own `invalidateConnections` that hit only ["tools","connections"] and ["tools","catalog"], while the shared hook in @agenta/entities also invalidates ["triggers","connections"] — with a comment saying why: gateway_connections rows back the triggers list. So connecting a tool through the drawer left triggers stale. The shared helper is exported now and the drawer calls it, rather than keeping a copy that drifts. `assertHostQueryClient` had tests; `getHostQueryClient()` — the accessor every package write actually goes through — had none. Two tests cover it reading `queryClientAtom` and following a late swap, which is the property that makes hydrating after mount work instead of pinning the first client. The JSDoc on the assertion still described the pre-migration failure mode (writes dying on the singleton). Package writes follow the atom now, so the real failure is a host that installs one client and hydrates another: two caches, reads from one, writes to the other. The mobile comment said the same thing slightly inverted and is now precise about install-without-hydrate. The handoff embedded a branch, a worktree and an expected dirty status as preconditions. Recorded as historical instead. Not changed: the review also claimed `importNames: ["queryClient"]` misses `import * as ns`. It does not — a probe file lints as "* import is invalid because 'queryClient' ... is restricted" under ESLint 9. Three further findings (the empty-cache evidence, the "verified live" claim, the mobile comment's stale call count) were already handled in 079bc20.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Package code reached the TanStack cache through the
queryClientsingleton exported by@agenta/shared/api. That only works if the host passes that same object to<QueryClientProvider>and hydratesqueryClientAtomwith it./minstalled its own client, so on mobile the singleton's cache was empty and every package-layerinvalidateQueries/setQueryData/removeQueriesaddressed an orphan — mutations returned success, toasts fired, nothing refreshed until a reload, and there was no error anywhere./mwas the only affected host. OSS and EE pass the singleton into their provider, so the two coincide there and the bug was invisible. It presented as a regression only because this stack shipped the first/msurfaces that perform package-layer mutations; the hazard predates them.What changed
getHostQueryClient()reads the client the host actually installed offqueryClientAtom, and all 17 package call sites go through it, resolved per call so nothing captures a pre-hydration value. Mobile'squeryClient.tsnow re-exports the shared singleton and merges its defaults onto it rather than replacing it (setDefaultOptionsis a whole-object write, so spreading the existingqueriespreserves package-set options likeexperimental_prefetchInRender).The invariant is now enforced, since "the host's client is the one I imported" was something nothing checked:
no-restricted-importsbans the namedqueryClientimport from@agenta/shared/apiand from the@agenta/sharedroot barrel, which re-exports it. The paths list is exported asrestrictedImportPaths; the 11 package configs that override the rule spread it back in, so none silently drops the bans.no-restricted-importscannot seeawait import(...), and the singleton was reachable that way — one call site used exactly that shape. Ano-restricted-syntaxrule bans dynamic imports of both modules outright. A selector matching only the destructured name would still missconst m = await import(...); m.queryClient, so package code that needs the client lazily imports the narrow@agenta/shared/api/hostQueryClientsubpath instead.Verification
Both lint bans were probed with a deliberate violation: passing before, erroring after.
A green
tscproves nothing here — the original bug type-checked perfectly and shipped a success toast — so nine flows were driven live on/m. Each was checked by stampingwindowbefore the mutation and asserting the stamp survived alongside the row-set change; a surviving stamp is positive proof the page never reloaded, which a screenshot cannot give. The section "Reload all …" button was never pressed.The optimistic toggle is the load-bearing check: a lost
setQueryDatais invisible, so "flips and stays flipped after the refetch" is what distinguishes a live cache write from a dead one.Not verified, and why: the full OAuth leg of tool connect/reconnect needs a real Google consent screen.
runnable/deploy.ts(10 invalidations) has no reachable surface on/mat all — converted and type-checked only; it can regress only on desktop, where the two clients coincide anyway.pnpm lint-fixgreen with the tree unchanged;@agenta/sharedunit tests 352 pass, including 5 for the assertion. Pre-existing on this base and untouched by this lane:@agenta/mobile'stokens:checkstaleness, and 21tscerrors that are all one signature (@agenta/sessions-uihas no exported member …).Out of scope
WP4 — the
refetchOnWindowFocus: false+staleTime: 30_000gap on the gateway queries, which is why the Gmail OAuth return showed a stale drawer — is a behaviour change and gets its own PR. This lane stays a pure "which client is addressed" change.Full write-up, including the dead ends and the probe recipe for checking whether a host is wired correctly:
docs/design/query-client-host-divergence/plan.md.