perf(web): migrate high-impact data-fetching hooks to TanStack Query - #1860
Open
simple-agent-manager[bot] wants to merge 14 commits into
Conversation
…log hooks Splits lib/query-options.ts into a domain directory with a barrel (rule 18) and adds identity-scoped key factories for credentials, agents, trial, tasks, chats, infrastructure and notifications. Migrates useActiveTasks, useRecentChats, useAllChatSessions, useAgentProfiles and useProviderCatalog off hand-rolled useState/useEffect loaders, and scopes the previously unscoped Nodes/Workspaces/SettingsNotifications query keys. Adds useQueryScope() as the single definition of the authenticated cache scope.
…shared queries useProjectChatState drops five mount-effect loaders (credentials, trial status, provider catalog, agent catalog, agent profiles) in favour of the shared queries, so the app's primary surface reuses the same cache entries as settings, onboarding and workspace creation instead of refetching all five on every chat open. Profile creation/update now invalidate the shared entry rather than splicing a component-local array, so an edit made in the composer reaches the profiles page. TaskForm, TaskSubmitForm, TriggerForm, ScalingSettings and useSessionState follow.
…grated queries Per-hook matrix mirroring tests/unit/hooks/useProjectData.test.tsx: concurrent-consumer dedup, cache reuse on remount, data still visible while isRefreshing, stale data retained when a background refetch fails, and two-scope isolation. Adds the two assertions that pin the behaviour this migration claims: - hidden-tab polls issue zero requests (verified discriminating — setting refetchIntervalInBackground:true makes it fail) - useAllChatSessions never re-enters loading once cached, which is what stops Chats.tsx blanking its list on refetch (verified discriminating — restoring loading=isFetching makes it fail) Also covers profile mutations propagating to sibling consumers via invalidation, the rule-48 toast/context refetch-loop shape, and a guard that no newly added query key becomes persistable.
…components The migration makes these components genuinely depend on the query cache and on the authenticated identity (useQueryScope -> useAuth), so their tests must supply both. Follows the pattern Wave 2 established in tests/unit/pages/projects.test.tsx. Also fixes a latent bug in QueryTestWrapper: it constructed a new QueryClient during every render, so any dedup / cache-reuse / stale-while-revalidate assertion routed through it was measuring a cold cache each time. Consolidates useAllChatSessions.test.ts into the new .tsx file (the old one targets the pre-migration signature); its two unique cases — first-load loading, and an empty success being an empty state rather than an error — are carried over.
Asserts the numbers the PR reports instead of describing them: nine concurrent consumers of the five shared endpoints issue five requests, a navigate-away-and-back issues none, and two different projects still fetch their own profile lists (dedup must not collapse genuinely distinct resources).
The nodes, workspaces and notification-preferences stale-while-revalidate tests
invalidated with the old unscoped prefixes ('nodes', 'workspaces',
'notification-preferences'), which now match nothing — so no refetch was triggered
and the assertions failed for the wrong reason.
Using the exported factories makes each test exercise the actual key contract, and
pins the scope to the id the mocked useAuth returns.
…validate Invalidate-only introduced a real regression. Callers act on the returned profile immediately — useProjectChatState selects the new id — while the cached list is still the pre-mutation one, and selectProfileId drops a selected id the list does not contain. So the brand new selection was discarded during the read-after-write window. The create/update/delete mutations now write the server-confirmed row into the shared entry before revalidating. That is strictly more than the pre-migration code did: it spliced into one component's local array, so sibling surfaces never saw it. Also corrects the project-chat wizard test, which mocked listAgentProfiles as permanently empty while createAgentProfile succeeded — a server that accepts a create then denies the row exists. Under that mock, resetting the selection is the correct behaviour, so the mock was hiding the contract rather than testing it. Regression test verified discriminating: removing the seed makes it fail.
… claims
Addresses the CRITICAL finding from the performance review, independently confirmed
by the architecture and task-completion reviewers.
AppShell mounts OnboardingProvider AND ChoosePathWizard on every authenticated page,
and each independently fetched listCredentials + listGitHubInstallations +
listAgentCredentials. That was six requests on every page load before the page itself
fetched anything — the largest single source of duplication in the app, and I had
missed it. Both now read the new useSetupStatus hook: three shared requests, reused by
the settings and project-chat surfaces too.
The same reviewers found that the doc comments and test variable names asserted a
consolidation that was not true ('replaces 7 listCredentials loaders' when 3 routed
through the hook). Those claims are now accurate, the task file records exactly which
five call sites remain and why, and the follow-up is tracked as SAM idea
01M0BZ28VT7Z63BG4WV8GCZH8P rather than living only in a gitignored state file.
Also: Settings.tsx now uses useQueryScope like every other migrated file; the two
genuinely dead invalidator exports are removed; useRecentChats/useAllChatSessions
propagate the real error message like their five siblings; and Workspaces.tsx keeps
the current list visible while a status-filter change loads.
A test file that fails to load contributes zero assertions, so a check that counts only assertion-level failures reports a collection error as success. On 2026-08-19 a full apps/web run reported 0 failed while 15 files were failing to import; it was caught only by reconciling the total against a baseline and asking where 70 tests had gone. Two earlier readings in the same session were also wrong — one because a piped tail returns its own exit code, one because a truncated run was used as a baseline. Generalizes the rule-53 'silence is not success' failure mode to test reporting, and to any other tool whose pass is expressed as an absence.
Contributor
The useSetupStatus migration made OnboardingProvider and ChoosePathWizard depend on the query cache and on useQueryScope, so their tests need the same providers every other migrated surface's tests got. Also corrects one of my own assertions: useAllChatSessions now propagates the real error message rather than a fixed string, so the first-load-failure test asserts the message the server actually returned.
… deps @tanstack/query-persist-client-core, idb-keyval and fake-indexeddb were added by the query-cache persistence PR (530f8e9) without entries in the evidence file. The gap is invisible to a pull_request-triggered run against the integration branch, but the manual workflow_dispatch run this workstream needs diffs against origin/main and therefore sees them — and it would block the integration PR at main regardless. Not this workstream's dependencies; fixed here because it is three JSON entries and it is what is turning this branch's only available CI signal red.
…tch the migration apps/api/tests/unit/project-default-provider.test.ts asserts on the TEXT of apps/web/src/components/ScalingSettings.tsx, so moving its credential read from a listCredentials() mount effect to the shared useCredentials query broke a string match. The assertion's intent is unchanged — the provider selector's options come from the user's own credentials — so it now matches how that is expressed. I found this from CI, not locally, because I ran only the apps/web suite. The monorepo suite is what CI runs; running the subset I happened to touch is not equivalent coverage.
|
Contributor
Author
Verification completeCI run 32212039387 — success, 12/12 non-skipped jobs, on
Local suites
The one monorepo failure was Two failures CI caught that local runs did not
Ready for the program coordinator. Not merged. |
simple-agent-manager Bot
pushed a commit
that referenced
this pull request
Aug 19, 2026
…1860) Squash-merge of workstream F (Wave 3). Migrates 15 hand-rolled useState/useEffect data-fetching hooks to TanStack Query with shared query-option factories, stale-while-revalidate rendering, identity-scoped cache keys, and hidden-tab poll gating. Key changes: - New query-options/ directory with domain-grouped factories (agents, chats, credentials, infrastructure, notifications, projects, tasks, trial) - Shared stale times (query-stale-times.ts) and poll config (poll-intervals.ts) - Dedup wins: listCredentials 7→2, listAgentProfiles 5→0, listAgents 5→3, getProviderCatalog 4→2, AppShell 6→3 requests/load - useSetupStatus shared hook eliminates OnboardingProvider + ChoosePathWizard double-fetch (CRITICAL performance reviewer finding) - All migrated hooks tested for SWR, dedup, identity scoping, and hidden-tab pause behavior - Process rule: "A Green Test Count Is Not A Green Suite" (.claude/rules/02) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
UI Performance Program — Workstream F (item #4 of SAM idea
01M09SKVNJGJNJY2WGCZ6D89XZ).Migrates the highest-volume hand-rolled
useState/useEffectdata-fetching surfaces inapps/webonto shared TanStack Query keys, so they gain request deduplication,stale-while-revalidate, and hidden-tab poll pausing.
Base is the program integration branch, not
main. Do not merge — the programcoordinator merges sub-PRs.
Direct API call sites removed
Counted as
git grep -c '<fn>('overapps/web/src, excludinglib/api/and the newlib/query-options/:listCredentialslistAgentProfileslistAgentsgetProviderCataloglistAgentCredentialsgetTrialStatusThe biggest win, which I initially missed
AppShellmountsOnboardingProviderandChoosePathWizardon every authenticatedpage, and each independently fetched
listCredentials+listGitHubInstallations+listAgentCredentials. That was six requests on every page load before the pageitself fetched anything, and it made my original "one request per
staleTimewindow"claim false. The performance reviewer caught it; I verified it, then routed both through
a new
useSetupStatushook — 6 → 3, shared with settings and project chat.What else changed
lib/query-options.ts→lib/query-options/directory + barrel (rule 18). Importersunchanged. Adds identity-scoped key factories for credentials, agents, trial, tasks,
chats, infrastructure and notifications.
useCredentials,useAgentCatalog,useAgentCredentials,useTrialStatus,useSetupStatus,useQueryScope,useActiveTasks,useRecentChats,useAllChatSessions,useAgentProfiles,useProviderCatalog.useProjectChatStateloses all five mount-effect loaders (file shrinks 1005 → 984).setQueryDataandthen invalidate, instead of chaining
await fetchProfiles().Nodes/Workspaces/SettingsNotificationsquery keys are now identity-scoped,and their hardcoded
10_000intervals move into env-overridableDEFAULT_*constants.Two fixed defects (not just refactoring)
Chats.tsxblanked its whole list on every refetch.useAllChatSessionsresetloadingon refresh and the page rendered under{!loading && …}.loadingis nowtrue only when nothing is cached.
cached list stale during the read-after-write window, and
selectProfileIddrops aselected id the list does not contain — so the brand new selection was discarded.
This one I introduced mid-PR and then fixed; see Post-Mortem.
Not a security fix — stated plainly
The previously unscoped keys (
['nodes','list']etc.) were not a live cross-accountleak:
AuthProvidercallsqueryClient.clear()on identity transition and unmountsconsumers while it happens, and unscoped keys are structurally unpersistable. Scoping
them is defence in depth.
PERSISTED_QUERY_OPERATIONSis unchanged ({'projects/list'}). Everything migratedhere is on Wave 2's never-persist-without-security-review list; a test pins that none of
the 13 new keys can reach disk.
Deliberately not migrated
Five components still read these endpoints directly —
AgentsSection,ProjectAgentsSection,ProjectOnboardingWizard,CreateWorkspace,and
OnboardingChecklist(the last is unreferenced in production). The reasons arerecorded per-file in the task file's "Deferred call sites" table, and the follow-up is
SAM idea
01M0BZ28VT7Z63BG4WV8GCZH8P. Doc comments and test variable names werecorrected so nothing claims a consolidation that did not happen.
Validation
pnpm lint— clean; 4 pre-existingreact-hooks/exhaustive-depswarnings in filesthis PR does not touch
pnpm typecheckpnpm test—apps/web: 3331 passed / 0 failed / 0 file-level collectionfailures across 280 files. Baseline on the integration head, same command and
reporter: 3246 passed / 0 failed across 271 files. Delta +85, which
reconciles: ~90 tests added across 10 new spec files, minus the 5 cases from
useAllChatSessions.test.tsthat were consolidated into its.tsxreplacement.The reconciliation is stated because a count that moves the wrong way is the only
cheap signal that a file silently stopped loading — see Post-Mortem.
pnpm check:fastcollection errors, not just assertion failures — an earlier run reported "0 failed"
while 15 test files were silently failing to import. See Post-Mortem.
Staging Verification (REQUIRED for all code changes — merge-blocking)
N/A per explicit program instruction. The workstream brief states: "Do NOT deploy to
or mutate staging — staging verification is consolidated at the single primary
integration PR later." This is also covered by the project policy "Respect explicit
no-staging instructions". Staging was not deployed to or mutated by this branch.
N/A: no infra changesStaging Verification Evidence
None, deliberately. Verification for this workstream happens at the program's primary
integration PR. Local evidence in place of it: full
apps/websuite green against ameasured baseline, typecheck, lint, production build, and the Playwright comparison below.
UI Compliance Checklist (Required for UI changes)
source behind it. Verified by diffing each page's render tree against the base.
(
role="status",role="alert",aria-busy) is byte-identical to the base branch.vite build+vite previewrather than new specs being written, since no JSXchanged.
recent-chats-dropdown-auditfails identically (13 failures) on thisbranch and on the base commit
f5f33559b, so it is pre-existing and unaffected;I built and served the base branch on a second port specifically to attribute it
rather than assume.
End-to-End Verification (Required for multi-component changes)
tests/unit/hooks/query-dedup-request-counts.test.tsxdrives nine concurrentconsumers of five endpoints through the real
QueryClientand asserts five requestsData Flow Trace
Project-chat open, the app's primary surface (rule 26):
Untested Gaps
useTrialStatusanduseProviderCataloghave dedup, remount-reuse and error-pathcoverage, but not the
isRefreshing-with-data or two-scope-isolation dimensions theother hooks have. Both use the byte-identical wrapper formula as five fully covered
hooks, but that is structural equivalence, not proof. Called out rather than claimed.
by an instrumented browser session. The five deferred components above still issue
their own requests.
Post-Mortem
This PR fixes two defects, one pre-existing and one I introduced during the work.
What broke
/chatspage blanked its entire session list on everybackground refresh.
just-created agent profile could lose its selection in the composer.
Root cause
useAllChatSessionssetloading = trueon refresh, andpages/Chats.tsxrenderedits list under
{!loading && …}. The hook and the page were individually reasonable;the combination was not.
useAgentProfiles.createProfileinvalidated the shared list, but callers act on thereturned profile immediately while the cache still holds the pre-mutation list.
selectProfileIdkeeps the current id only if the list contains it, so it fell backto the first entry. The pre-migration code masked this by splicing into a
component-local array.
Class of bug
class
.claude/rules/48exists for.hazard in
.claude/rules/06. Replacing a synchronous local write with anasynchronous cache invalidation opened a window where a reconciling effect could
observe stale state and revert the user's action.
Why it wasn't caught
held a refetch in flight and asserted the list was still mounted.
listAgentProfilesas permanently empty whilecreateAgentProfilesucceeded — a server that accepts a create then denies the rowexists. Under that mock, resetting the selection is correct, so the mock hid the
contract instead of testing it.
A third failure is worth recording even though it produced no shipped bug: a full
suite run reported
0 failedwhile 15 test files were failing to import (I hadwritten the wrong relative path for a shared helper). A file that never loads contributes
zero assertions, so an assertion-only failure count reads as green. I caught it only by
reconciling the total test count against the baseline — 3176 vs 3246 — and asking where
70 tests went. Two earlier runs in this session were also misread: one because
| tailreturns the pipe's exit code rather than vitest's, and one because a truncated run
produced a "baseline" of 1050 tests for a 3246-test suite.
Process fix included in this PR
.claude/rules/02-quality-gates.mdgains a subsection under "Evaluating Test Realism":"A green test count is not a green suite" — requiring that any reported test result
reconcile the total against a known expected count and check per-file collection status,
and banning
vitest … | tailas an exit-code source. This generalizes beyond this PR:it is the same "silence is not success" failure mode
.claude/rules/53already names forscheduled handlers, applied to test reporting.
Post-mortem file
tasks/archive/2026-08-19-tanstack-query-migration-high-impact-hooks.mdSpecialist Review Evidence (Required for agent-authored PRs)
OnboardingProvider+ChoosePathWizardmount on every authenticated page and each fetched the same three endpoints, falsifying the dedup claim. Verified independently, then migrated both viauseSetupStatus(6 requests → 3) in20a43c44d. Its LOWuseProviderCatalogwaterfall note is pre-existing (identical in base) and left as-is.Settings.tsxdid not use theuseQueryScopehook this PR introduced, and three exports were dead. Both fixed in20a43c44d. Confirmed the directory split, config-module separation anduseAuthcoupling are sound.data === undefinedguard means a single-token swap would not fail it. Added the missing scope-isolation and error-path tests; corrected misleading test variable names. Confirmed the key-factory test fixes closed a genuine vacuous-test hole..do-state.md. Task file now carries a per-file "Deferred call sites" table; checkboxes synced; follow-up tracked as SAM idea01M0BZ28VT7Z63BG4WV8GCZH8P.Exceptions (If any)
Scope: Staging verification not performed.
Rationale: Explicit program instruction — verification is consolidated at the
primary integration PR. Also covered by the "Respect explicit no-staging instructions"
project policy.
Expiration: At the program's integration PR, which must verify this workstream's
surfaces on staging before anything reaches
main.Scope: Task file not pushed to
main(the default/doPhase 1 behaviour).Rationale: A push to
mainin this canonical repo triggers CI and DeployProduction. Doing that for a docs-only commit would be an unauthorized production
mutation under the "merge only when explicitly authorized" policy. The task file rides
on this branch instead.
Expiration: N/A.
Agent Preflight (Required)
Classification
External References
useQueryreference (official documentation):https://tanstack.com/query/latest/docs/framework/react/reference/useQuery
persistQueryClientplugin (official documentation):https://tanstack.com/query/v5/docs/framework/react/plugins/persistQueryClient
not the docs:
node_modules/.pnpm/@tanstack+query-core@5.101.2/…/build/modern/queryObserver.js(
#updateRefetchInterval) and…/focusManager.js(isFocused). Read directly becausethe claim is load-bearing for rule 60 compliance.
Codebase Impact Analysis
All changes are confined to
apps/web:apps/web/src/lib/query-options/— new directory + barrel (wasapps/web/src/lib/query-options.ts)apps/web/src/lib/poll-intervals.ts,apps/web/src/lib/chat-query-config.ts,apps/web/src/lib/query-stale-times.ts— cadence/limit/freshness constantsapps/web/src/hooks/—useCredentials,useAgentCatalog,useTrialStatus,useSetupStatus,useQueryScope(new);useActiveTasks,useRecentChats,useAllChatSessions,useAgentProfiles,useProviderCatalog(rewritten)apps/web/src/pages/—Settings,SettingsContext,SettingsNotifications,Chats,Dashboard,Nodes,Workspaces,Node,ProjectSettings,ProjectProfiles,ProjectSkills,ProjectDeploymentEnvironmentDetail,project-chat/useProjectChatState,workspace/useSessionStateapps/web/src/components/—AppShellconsumersonboarding/OnboardingContext,onboarding/choose-path/ChoosePathWizard; plusRecentChatsDropdown,ScalingSettings,task/TaskSubmitForm,project/TaskForm,triggers/TriggerFormapps/web/tests/— 10 new spec files, ~20 updatedapps/api,packages/shared,packages/vm-agent,scripts/,infra/— untouchedNo API, Worker, database or infrastructure paths are modified.
Documentation & Specs
N/A: no user-facing behaviour changes.This is an internal data-fetching refactor plustwo UI defect fixes; nothing in
apps/www/src/content/docs/docs/describes the affectedinternals. The durable engineering record is
tasks/archive/2026-08-19-tanstack-query-migration-high-impact-hooks.mdand SAM idea01M0BZ28VT7Z63BG4WV8GCZH8P.Constitution & Risk Check
Principle XI (No Hardcoded Values) — the main one this touches. Every cadence,
limit and freshness window introduced or relocated is a
DEFAULT_*constant with aVITE_*override:NODE_LIST_POLL_MS,WORKSPACE_LIST_POLL_MS,ACTIVE_TASKS_POLL_MS(
lib/poll-intervals.ts);RECENT_CHATS_POLL_INTERVAL_MS,RECENT_CHATS_LIMIT,ALL_CHATS_LIMIT(lib/chat-query-config.ts);AGENT_CATALOG_STALE_TIME_MS,PROVIDER_CATALOG_STALE_TIME_MS,TRIAL_STATUS_STALE_TIME_MS(
lib/query-stale-times.ts). The two hardcoded10_000literals inNodes.tsxandWorkspaces.tsxare removed.VITE_RECENT_CHATS_POLL_MSandVITE_RECENT_CHATS_LIMITkeep their exact pre-existing names so deployments that set them are unaffected.
Rules checked: 48 (stale-while-revalidate — the whole point of the PR, plus two
fixed violations), 60 (request I/O and polling hygiene), 16 (invalidate, never reload),
24/59 (extend the Wave 1–2 factory pattern rather than fork it), 18 (query-options split
into a directory + barrel; every new file is under 100 lines and the three pre-existing
oversized files all shrank), 06 (React interaction-effect analysis — this is what caught
the profile-selection regression), 26 (project chat treated as the primary surface).
Key risks and tradeoffs:
the migration being behaviour-preserving by construction (the hook result shape is
identical to the pre-existing
useProjectDatacontract) and by a measured baselinecomparison rather than a bare "tests pass".
AuthProvider.useQueryScope()callsuseAuth(), whichthrows outside the provider — so every migrated component now requires it. Every route
already sits inside
AuthProviderinApp.tsx, and the architecture reviewerconfirmed this matches how
useToast/useProjectContext/useSettingsContextalreadybehave. The cost was real and was paid: ~19 test files needed the provider added.
documented per-file rather than papered over, and tracked as an idea.
section. This is the largest residual risk and is why it must be covered at the
integration PR.