feat(editor): adopt TanStack Query, convert three read-only fetches - #7264
Merged
Conversation
…nly fetches The editor had no query client: 292 apiClient call sites, 64 hand-rolled loading flags, and three module-level caches reimplementing dedupe, retry and invalidation by hand. The Processor (src/portal) has run on TanStack Query for a while; this brings the same foundation to the editor. The foundation does not ship alone. A provider nothing consumes has no benefit and leaves a dead abstraction if the follow-up stalls, so it lands with the three read-only fetch sites that convert safely: - useFooterInfo — footer and admin legal section now share one request - useGroupEnabled (core + desktop) — cached per group, was refetching on every mount; desktop keeps its offline short-circuit - UserSelector — was fetching the whole user roster on each of four mount sites (one in the Processor), with an effect re-running on t/user identity change All three keep their existing return shape, so no consumer files change. Desktop needs more than the provider. operationRouter resolves the same relative path to the local bundled backend, a self-hosted server or the SaaS backend depending on connection mode, and Query caches by key rather than resolved URL — so a cached entry can outlive the backend that filled it. group-enabled routes through operationRouter, so this commit introduces that hazard and carries the fix: DesktopQueryCacheReset clears the cache on mode change, and CONFIG_STALE_TIME is a shadowed module (Infinity in core, 5 min on desktop) backstopping the case a mode change does not cover, such as a self-hosted server coming back online. Login.test.tsx now wraps in TestQueryProvider: AuthLayout renders Footer, which reads useFooterInfo. The real /login route is already inside AppProviders, so this is a test-isolation fix, not a runtime gap. Plan: notes/REACT_QUERY_ROLLOUT_PLAN.html
ConnorYoh
requested review from
EthanHealy01,
Frooodle,
jbrunton96 and
reecebrowne
as code owners
August 3, 2026 14:27
Two of these would have shipped broken.
queryClient.clear() evicts cache entries without notifying mounted
observers, so a panel keeps rendering the old backend's answer until
something unrelated re-renders it. The docblock justified that with "a
mode switch already remounts the SaaS provider tree", but AppProviders
deliberately skips the remount when switching TO self-hosted — the exact
transition the reset exists for. Now resetQueries(), covered by a test
that fails against clear().
networkMode defaulted to "online", which pauses fetches whenever the
WebView reports offline. That check describes internet reachability and
says nothing about a bundled backend on 127.0.0.1 or a self-hosted server
on the LAN, so losing Wi-Fi stranded every query pending against a backend
that was up. Set to "always" in the shared defaults — self-hosted web
users on localhost hit this too, so it isn't desktop-only.
Also from review:
- Fetchers move to core/api/{config,users}.ts. They were in three
different places (private in a hook, exported from a hook, inline in a
component), which is the first thing PR 2 would have had to guess at.
Also removes the desktop hook's @core/hooks import.
- desktop useGroupEnabled read the monitor snapshot during render, so an
unrelated re-render could flip a rendered panel mid-interaction — the
opposite of what its comment claimed. Now useSyncExternalStore over a
selected boolean, since the monitor reassigns its state object on every
poll.
- The cache reset also listens for the self-hosted server going up or
down, which reroutes endpoints with no mode event. Guarded to
offline<->reachable so idle/checking don't trigger it, and mounted in
the pre-auth branch too, where mode switches were being missed.
- desktop/query/staleTime.ts re-exports core so a new tier there doesn't
silently vanish on desktop.
- TestQueryProvider moves to core/tests/utils/. core/testing/ holds
runtime simulation code imported by production, and is layer-shadowed.
- fetchUsers guards a non-array body; footer-info logs before rethrowing,
since its toast is suppressed and the failure was otherwise silent.
- Dropped the staleTime claim that auth-change invalidation is wired. It
isn't, so the doc now says what the tier is safe for instead.
One source of truth for query behaviour: core/query/queryClient.ts exports
baseQueryOptions and the portal builds its client from the same object.
The two option sets were byte-identical, which is exactly how they drift —
and networkMode: "always" needs to apply to both, since the portal also
runs against localhost in self-hosted deployments.
Separate instances, not a shared one. The editor and portal mount as
sibling routes rather than nested, so they never coexist in one tree; a
single instance would only mean the cache survives navigation between the
two products, and today they share no keys. It also breaks the contract
three portal tests rely on, that createPortalQueryClient() returns a fresh
client per test. Worth doing in the planned collapse PR, where the portal's
key and test story gets sorted together.
Comments cut roughly in half. The main offender was repetition: "a query
key does not pin a backend" was written out in full five times across
keys.ts, both staleTime modules, DesktopQueryCacheReset and its call site.
It now lives in the component that acts on it, with short pointers
elsewhere. Also dropped changelog-voice asides ("matching the previous
catch handler"), narration of adjacent code, and a claim that
UserSelector's roster is shared with the policy wizard — it isn't, that
file only has a TODO naming it.
Contributor
📦 Tauri Desktop Builds Ready!The desktop applications have been built and are ready for testing. Download Artifacts:🍎 macOS Universal: Download Stirling-PDF-macos-universal (.dmg) - 336.5 MB Built from commit 8c6869b |
Every docblock in the new query layer said in a paragraph what fits on a line. Now: keys.ts 1 line, staleTime 1, queryClient 2, the converted hooks 1-3 each. DesktopQueryCacheReset keeps the most because the two things it encodes are both non-obvious — why a key can outlive its backend, and why resetQueries rather than clear(). No code change.
Aikido flagged the offline check as contradicting its own comment. It does, and the consequence is worse than the mismatch: the monitor has four states, and treating everything non-offline as reachable inverted the recovery path. start() sets "checking" and stop() sets "idle". So when the server is down and the monitor restarts, offline -> checking read as a flip and reset the cache while routing still pointed at the local fallback, then checking -> online read as no flip and skipped the reset that mattered. The local fallback's answers stayed cached after the server came back. Now only online and offline are considered. Two tests added, both verified to fail against the previous logic.
Frooodle
approved these changes
Aug 4, 2026
reecebrowne
approved these changes
Aug 4, 2026
This was referenced Aug 4, 2026
pull Bot
pushed a commit
to jnnycn007/Stirling-PDF
that referenced
this pull request
Aug 7, 2026
# Description of Changes > Stacked on Stirling-Tools#7264 — review that first. This diff is against its branch. ## The problem `AppConfigContext` hand-rolled a query client: a `fetchCountRef` dedupe guard, an exponential-backoff retry loop with its own `sleep()`, a `hasResolvedConfig` flag, and manual 401/5xx branching. All to fetch one endpoint that 80 files read. ## End state Same provider, same public contract, React Query underneath. **250 lines to 142**, and no consumer file changes. | | Before | After | |---|---|---| | Dedupe | `fetchCountRef` guard | query key | | Retry | `for` loop + `sleep()` + backoff maths | `retry` + `retryDelay` | | 401 | caught in the component, sets default config | `fetchAppConfig` returns the default — the retry predicate and error state only see real failures | | Auth pages | early return inside the fetch | `enabled` | | Resolved-yet tracking | `hasResolvedConfig` state | derived from the query | `fetchAppConfig` moves to `core/api/config.ts` with the simulation hook and request options, so the context no longer knows how config is fetched. **Behaviour change:** config survives a provider remount instead of refetching. That matters on desktop, where a connection-mode switch remounts the tree — and Stirling-Tools#7264's cache reset already clears it on exactly that transition. ## Testing The existing 12-case contract test passes unchanged apart from the query wrapper. It caught a real mistake: `failureCount` is 0-based in v5, so `<= maxRetries` gave one attempt too many. Four cases added — cached remount, `maxRetries` honoured, 4xx not retried, `autoFetch` off. `task frontend:check` green: 1672 tests across 191 files, typecheck on all five flavours, eslint, dpdm, prettier. ## Coming next | PR | Scope | |---|---| | 3 | `useEndpointConfig` — core (251 lines) plus a 482-line desktop override with its own dependency polling. Split out of this PR; different risk profile, and it deserves its own review. | | 4 | `useAdminSettings` (20 consumers) and the config sections | | 5 | Polling loops → `refetchInterval` | | 6 | Finish the Processor, collapse to one client | | 7 | Tool execution — mutation state only | --------- Co-authored-by: Reece Browne <74901996+reecebrowne@users.noreply.github.com>
pull Bot
pushed a commit
to 5474312/Stirling-PDF
that referenced
this pull request
Aug 10, 2026
…g-Tools#7285) # Description of Changes > Stacked on Stirling-Tools#7264, sibling of Stirling-Tools#7283. Independent of Stirling-Tools#7283 — the only overlap is two additive lines in `core/query/keys.ts` and `core/api/config.ts`. Either can merge first. ## The problem `useEndpointConfig` kept its own cache: a module-level `globalFetchDone` boolean, a mutable `globalEndpointCache` object, and a `resetGlobalCache()` called from the JWT listener. Which consumer mounted first decided who paid for the request, and nothing invalidated it except a page reload. ## End state One shared query for the whole availability map; each of the 12 consumers projects the endpoints it asked for. **251 lines to 101**, same return shape, no consumer changes. | | Before | After | |---|---|---| | Cross-consumer cache | `globalFetchDone` + mutable module object | query key | | Invalidation | `resetGlobalCache()` mutating that object | `invalidateQueries` | | Per-endpoint check | own `useState` triple | query keyed by endpoint | Behaviour kept deliberately: - **Unknown endpoints and any failure still read as enabled.** This fires before auth settles, and disabling every tool on a hiccup is worse than letting one call fail later. - **`retry` is off for the availability map.** The fallback *is* the answer, so retrying only doubles a request every logged-out visitor makes on load. ## Desktop is untouched `desktop/hooks/useEndpointConfig.ts` shadows this module entirely — no shared code, so core converting doesn't affect it and there's no half-migrated state. It's 482 lines of orchestration rather than fetching: dependency-ready gating, `tauriBackendService` and `selfHostedServerMonitor` subscriptions, a 2.5s timeout retry for backend startup, a legacy `?endpoints=` fallback for old servers, and SaaS-routing optimism that rewrites disabled endpoints to enabled. It also has no test coverage to convert against, and it decides whether tools appear at all in the desktop app. That's a different job from this one and wants its own review. Next PR. ## Testing 9 new tests: projection onto the requested subset, one request across consumers, unknown-endpoint fallback, failure fallback with no retry, empty-list no-fetch, JWT invalidation, and the three single-endpoint cases. `task frontend:check` green: 1677 tests across 192 files, typecheck on all five flavours, eslint, dpdm, prettier. Co-authored-by: Reece Browne <74901996+reecebrowne@users.noreply.github.com>
pull Bot
pushed a commit
to qtxtz/Stirling-PDF
that referenced
this pull request
Aug 29, 2026
…Tools#7726) # Description of Changes Step 5 of the TanStack Query rollout, covering the admin People, Teams and Team details screens. Follows Stirling-Tools#7264, Stirling-Tools#7283, Stirling-Tools#7285. ## The problem Two separate ones, in the same three files. **Reads.** Each section fetched and held its own copy of the same resources: People read the roster and the team list, Teams read the team list plus the roster again when its add-member modal opened, Team details read all three. Cost scaled with how many screens you visited rather than with how much data exists. **Writes.** Thirteen handlers each did the same five things by hand: set a processing flag, call the service, toast the outcome, dig a message out of an axios error, and reload their own slice. Refreshing was a convention, not a mechanism, and one handler had already forgotten it. ## The fix Three shared query keys (`adminUsers`, `teams`, `teamDetails`), and one `useAdminMutation` helper that every write is declared against: ```ts const createTeam = useAdminMutation({ write: (name: string) => teamService.createTeam(name), invalidates: ["teams"], success: t("workspace.teams.createTeam.success"), errorFallback: t("workspace.teams.createTeam.error"), onDone: () => { setNewTeamName(""); setCreateModalOpened(false); }, }); ``` Each write names the slices it disturbs, which is the part that only works when reads and writes are designed together: `createTeam` invalidates the team list, while a membership move invalidates the list, both teams' detail rows and the roster, because it genuinely changes all three. Invalidation refetches only mounted queries, so this costs nothing extra. The blanket "invalidate everything" helper survives in exactly one role: child components (invite, password change, seat update) that write through their own services, where the affected scopes are not visible from the call site. ## Why it is better, measured Request counts come from one harness driving `teams -> team details -> back -> people`, run against the branch point and against this branch. The assertion is committed, so it cannot silently regress. | | Before | After | |---|---|---| | Requests | 7 | **3** | | `getTeams` | 4 | **1** | | `getUsers` | 2 | **1** | | `getTeamDetails` | 1 | 1 | | Committed renders | 17 | **15** | Three is one per distinct resource, the floor for that sequence. The four `getTeams` were the Teams table, Team details fetching the same list for its "move to team" dropdown, the explicit refresh on the back button, and People. Renders barely move, which is expected: this changes where data lives, not how often React draws. It is reported because a caching change can quietly cost renders, and this one does not. On the code itself, across the three sections: | | | |---|---| | Net lines | **-216** | | `useState`/`useEffect` removed | **11**, none added | | Duplicated `isAxiosError` blocks | 13 to **1** | | `setProcessing` calls | 19 to **0** | `isAxiosError` is no longer imported by any of the three files. ## Bug fixed `disableMfaByAdmin` showed a success toast and never refreshed. The menu item renders only when `user.mfaEnabled` is true, so an admin disabled MFA, was told it worked, and watched the option stay on screen until a manual reload. It is covered by a test that fails if the invalidation is removed. ## Behaviour worth checking in review - A write no longer blocks its handler before closing the modal. The dialog closes when the write succeeds and the table updates when the refetch lands, rather than the button spinning through both. - Modal submit buttons now track their own mutation rather than one shared flag. Team details still derives a single busy flag, now from its five mutations rather than a `useState`, so its row actions disable together as before. - The per-handler `console.error` is kept, once, in the shared error path. ## Testing Five tests, each verified by breaking the implementation and confirming that one test, and only that one, fails: | Mutation | Caught by | |---|---| | Drop the shared stale window (`staleTime: 0`) | request-count test | | Make invalidation a no-op | write-visibility test | | Ignore the login-enabled gate | login-disabled test | | Stop invalidating after the MFA write | MFA-refresh test | | Fall back to the generic error message | server-message test | The write tests drive the real flows through their modals and menus rather than calling hooks directly. `task frontend:check` passes typecheck, lint and oxfmt, and 2383 of 2385 editor tests. The two failures, `workbenchSession.test.ts` and `notificationActions.test.tsx`, are untouched here and fail identically with this branch's changes reverted. ## Scope The three services keep their current shape; nothing outside these three sections and the new hook module changes. Child modals that write through their own services still refresh via the blanket helper, and converting those is separate work.
pull Bot
pushed a commit
to kokizzu/Stirling-PDF
that referenced
this pull request
Aug 29, 2026
…ls#7436) # Description of Changes Step 4 of the TanStack Query rollout, and the first of the polling hooks. Follows Stirling-Tools#7264, Stirling-Tools#7283, Stirling-Tools#7285. ## The problem `useSigningSessions` hand-rolled its own fetch, loading state and `setInterval`. Two consequences: - **A raw `setInterval` keeps polling a hidden tab.** Browsers throttle background timers, they do not stop them, so a backgrounded editor with Shared Sign open keeps hitting both endpoints for as long as it is open. - **No tests.** The hook had none, and its quietest behaviour (below) is the easiest thing to break without noticing. ## End state One query behind `qk.signingSessions()`, with the polling lifecycle handed to the library: - Polling stops while the tab is hidden, and refetches on return rather than leaving data up to a full interval stale. - Mounts render from cache while they revalidate, so moving between the tool picker and the signing tool no longer flashes an empty list. - 12 tests where there were none. Same return shape, so no consumer files change. ### What this is not This is not a deduplication win. The three consumers are never mounted at the same time: `ToolPanel` renders the tool picker or the active tool and never both, so the badge cannot be on screen with either of the others, and `SharedSigningLauncher` and `useSigningSessionController` sit inside two different tools. The shared key earns its keep on cache reuse across those transitions, not on concurrent fetches. ## The bit worth reviewing The hand-rolled `{ silent: true }` flag encoded three states, and no single Query flag reproduces them: | | Spinner | Toast on failure | |---|---|---| | First load | yes | yes | | Background poll | no | no | | Explicit refetch | **yes** | **yes** | `isLoading` is false during an explicit refetch when data is already on screen; `isFetching` is true during a background poll. Neither matches, so the user-initiated case is tracked with a small flag and the failure toast is gated on `isLoadingError` plus the explicit path. ## Testing Twelve tests. Rather than trust them, each claim was checked by breaking the implementation and confirming the relevant test fails: | Mutation | Caught by | |---|---| | `refetchIntervalInBackground: true` | hidden-tab test | | Drop `refetchOnWindowFocus` | returns-to-view test | | Drop the user-initiated spinner flag | manual-refresh test | | Toast on every error | background-failure-is-silent test | | Give each observer its own key | dedupe test | Three things worth knowing for the next conversion: - **`waitFor` flushes renders.** Recording an index *after* `waitFor(callCount === 2)` skips past the in-flight render, so a "did the spinner flip on" assertion passes vacuously. The marker has to go before the poll. - **Fake timers hide in-flight state.** The fetch settles inside the same `act()`, so the intermediate render never happens. That test uses real timers and a held-open promise. - **`visibilitychange` has to bubble.** query-core listens for it on `window`, and the real event bubbles from `document`. A test helper dispatching a non-bubbling event never reaches the focus manager, and the pause behaviour still appears to work because `refetchInterval` reads `document.visibilityState` directly at tick time rather than through the event. **One claim is deliberately unguarded.** `isLoading` vs `isFetching` for a background poll produces no re-render at all, so there is nothing observable for a test to assert and no user-visible difference to protect. ## Pre-existing failures `task frontend:check` passes typecheck, lint and oxfmt, and 2363 of 2365 editor tests. The two failures, `workbenchSession.test.ts` and `notificationActions.test.tsx`, fail identically with this branch's changes reverted and are untouched by it. ## Scope This is one of five pollers. The remaining four, `useLocalFolderPoller`, `WatchedFolderWorkbenchView`, `SessionDetailPanel` and cloud `TeamSection`, are separate files with their own consumers and follow separately, now that the silent-refresh pattern has a worked example. --------- Co-authored-by: Anthony Stirling <77850077+Frooodle@users.noreply.github.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
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.
Description of Changes
The problem
The editor has no query client. ~295
apiClientcall sites, each mount refetching what the last one just got, and three module-level caches reimplementing dedupe, retry and invalidation by hand — each shaped differently.The Processor (
frontend/editor/src/portal) has run on TanStack Query since #7135. The editor never got it.End state
The editor has a query client, and the three read-only fetch sites that convert safely now use it.
@tanstack/react-queryis already a dependency — no new package.Foundation
core/query/queryClient.tsbaseQueryOptions+ client factory. The portal now builds its client from the same options.networkMode: "always"—navigator.onLinedescribes internet reachability, which says nothing about a bundled backend on 127.0.0.1 or a self-hosted server on the LAN.core/query/keys.ts["editor", resource, ...params]core/query/staleTime.ts+desktop/query/staleTime.tsInfinityon web, 5 min on desktopcore/api/config.ts,core/api/users.tsportal/api/*core/tests/utils/TestQueryProvider.tsxdesktop/components/DesktopQueryCacheReset.tsxQueryClientProvidermounts at the top ofcore/components/AppProviders.tsx. That diff looks large but is one wrapper plus the reindent underneath it.Converted. All three keep their existing return shape, so no consumer changes.
useFooterInfouseGroupEnabledUserSelectortoruserchanged identityDesktop needs more than the provider.
operationRouterresolves the same relative path to the local bundled backend, a self-hosted server, or the SaaS backend. Query caches by key, not by resolved URL, so a cached entry can outlive the backend that filled it.group-enabledroutes this way, so this PR introduces the hazard and carries the fix:DesktopQueryCacheResetcallsresetQueries()when the connection mode changes or the self-hosted server goes up or down, andCONFIG_STALE_TIMEis finite on desktop as a backstop.Behaviour changes
loadingfor one extra attempt plus backoff.staleTime: Infinityon web means admin edits to legal links no longer appear on remount within a session. Saving those already prompts a restart, so this is accepted rather than incidental.useGroupEnabledshows the translated offline reason on first render. The old code showed raw English for one render.UserSelectordrops threeconsole.logs that were dumping user records to the console.Decisions
1. The foundation doesn't ship alone. A provider nothing consumes gives a reviewer nothing to react to and rots if the follow-up stalls, so it lands with the cheapest safe conversions.
2. Hooks keep their existing return shape. The alternative is switching to
{ data, isPending, error }and updating consumers now. Cost of my choice: we carry aloading-shaped façade indefinitely, and consumers don't getisFetching/refetchwithout a second pass. Taken because it's what keeps each later migration a one-file diff.3. Shared defaults, separate instances. The editor and the Processor mount as sibling routes, not nested — they never coexist in one tree. Both clients now come from the same
baseQueryOptions, so behaviour can't drift. A single shared instance would only buy cache surviving navigation between the two products, which is worth little while they share no keys, and it breaks the contract three portal tests rely on (createPortalQueryClient()returning a fresh client per test). That belongs in the collapse PR. Consequence meanwhile: the desktop reset covers the editor client only — harmless, since the portal isn't in desktop builds.4. The desktop reset is wholesale. A mode switch already remounts the SaaS provider tree, so there's nothing to preserve, and an allowlist of "mode-sensitive" keys would be a trap every new query has to remember to join.
Coming next
Ordered by consumers per line changed.
AppConfigContext+useEndpointConfig— ~80 consumers, deletes ~200 lines of hand-rolled cache, retry and dedupeuseAdminSettings(20 consumers) and the config sectionsrefetchIntervalNot in scope, deliberately:
usePdfLibLinks(its cache is a refcounted ArrayBuffer lifetime manager), thumbnail hooks, watched-folder IndexedDB reads, the desktop health monitors. UnifyingendpointAvailabilityService/saasAppConfigServicewith the query cache would mean handingoperationRoutera query client — its own PR if a second reason appears.Testing
task frontend:checkgreen: 1666 tests across 191 files, typecheck on all five flavours, eslint--max-warnings=0, dpdm, prettier.New tests cover request de-duplication, per-group key isolation, the desktop offline short-circuit, and the cache reset. The reset test was verified to fail against the
clear()implementation it replaced.UserSelectorhas no test beyond its existing stories.One existing test needed a wrapper:
Login.test.tsxrenders<Login />in isolation, andAuthLayout→Footer→useFooterInfonow needs a client. The real/loginroute is already insideAppProviders, so this is test isolation, not a runtime gap.Rollback is a clean revert — nothing persists outside the React tree.