fix(solid-query): don't trigger Suspense when data is already cached#10592
Open
ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
Open
fix(solid-query): don't trigger Suspense when data is already cached#10592ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR prevents unnecessary Solid Suspense activation when query data is already cached (including ChangesSolid Query Suspense Fix
Sequence Diagram(s)sequenceDiagram
participant Component
participant useBaseQuery
participant QueryResource
participant QueryClient
Component->>useBaseQuery: read data
useBaseQuery->>useBaseQuery: check state.data
useBaseQuery->>useBaseQuery: untrack(state.isFetching)
alt not fetching and state.data present
useBaseQuery-->>Component: return cached state.data
else fetching or no cached data
useBaseQuery->>QueryResource: read resource (may suspend)
QueryResource-->>useBaseQuery: resource.data
useBaseQuery-->>Component: return resource.data (or suspend)
end
Note right of QueryClient: ensureQueryData preloads cache used above
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
View your CI Pipeline Execution ↗ for commit 4e59da3
☁️ Nx Cloud last updated this comment at |
ousamabenyounes
force-pushed
the
fix/issue-9955
branch
2 times, most recently
from
c947290 to
7a99cf8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/solid-query/src/__tests__/suspense.test.tsx (1)
911-950: ⚡ Quick winConsider spying on the
Pagequery function to assert it was never invoked.The test correctly guards against the fallback mounting, but it does not verify that the background
queryFn(returning'fresh') is never called. Avi.fn()spy would make the test more explicit about the "no refetch" contract implied bystaleTime: Infinity, and would catch regressions where the fix avoids Suspense but still triggers an unintended network call.✅ Suggested improvement
+ const freshFn = vi.fn(() => sleep(10).then(() => 'fresh')) + function Page() { const state = useQuery(() => ({ queryKey: key, - queryFn: () => sleep(10).then(() => 'fresh'), + queryFn: freshFn, staleTime: Infinity, })) return <div>data: {state.data}</div> }Then add at the end:
expect(rendered.getByText('data: preloaded')).toBeInTheDocument() expect(fallbackMounted).toBe(false) + expect(freshFn).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-query/src/__tests__/suspense.test.tsx` around lines 911 - 950, Add a vi.fn() spy for the queryFn used inside the Page component and assert it was never called after render: replace the inline queryFn in Page's useQuery with a spied function (e.g., const pageQueryFn = vi.fn(() => sleep(10).then(() => 'fresh')) and pass that to useQuery), then after rendering assert pageQueryFn was not called (expect(pageQueryFn).not.toHaveBeenCalled()) to ensure the background refetch never ran; keep the ensureQueryData preload untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/solid-query/src/__tests__/suspense.test.tsx`:
- Around line 911-950: Add a vi.fn() spy for the queryFn used inside the Page
component and assert it was never called after render: replace the inline
queryFn in Page's useQuery with a spied function (e.g., const pageQueryFn =
vi.fn(() => sleep(10).then(() => 'fresh')) and pass that to useQuery), then
after rendering assert pageQueryFn was not called
(expect(pageQueryFn).not.toHaveBeenCalled()) to ensure the background refetch
never ran; keep the ensureQueryData preload untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39a9d48e-01a7-4062-9fd2-663ae1782d1f
📒 Files selected for processing (3)
.changeset/solid-query-suspense-ensure-data.mdpackages/solid-query/src/__tests__/suspense.test.tsxpackages/solid-query/src/useBaseQuery.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/solid-query-suspense-ensure-data.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-query/src/useBaseQuery.ts
ousamabenyounes
added a commit
to ousamabenyounes/query
that referenced
this pull request
May 8, 2026
Address CodeRabbit nitpick on PR TanStack#10592: spy on Page's queryFn and assert it was never called, strengthening the guarantee that the background refetch did not run when data was preloaded via ensureQueryData. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit nitpick in e815276: spied on |
ousamabenyounes
added a commit
to ousamabenyounes/query
that referenced
this pull request
May 9, 2026
Address CodeRabbit nitpick on PR TanStack#10592: spy on Page's queryFn and assert it was never called, strengthening the guarantee that the background refetch did not run when data was preloaded via ensureQueryData. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ousamabenyounes
force-pushed
the
fix/issue-9955
branch
from
e815276 to
77e3cd8
Compare
When data was preloaded via `ensureQueryData` (e.g. from a router loader), the proxy `data` getter read `queryResource.latest`, which falls back to a suspending read while the resource is in its initial pending state, even though the fetcher had already synchronously resolved with cached data. If the store already has data and no fetch is in-flight, return `state.data` directly. `state.isFetching` is read via `untrack` so it doesn't widen the data subscriber's reactive deps. Fixes TanStack#9955
Address CodeRabbit nitpick on PR TanStack#10592: spy on Page's queryFn and assert it was never called, strengthening the guarantee that the background refetch did not run when data was preloaded via ensureQueryData. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ousamabenyounes
force-pushed
the
fix/issue-9955
branch
from
77e3cd8 to
4e59da3
Compare
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
Fixes #9955.
When data was preloaded via
ensureQueryData(e.g. from a TanStack Router loader), accessingstate.datain a Suspense boundary would re-trigger the Suspense fallback even though the data was already in cache andisSuccesswastrue.Root cause
In
useBaseQuery.ts, the proxydatagetter readsqueryResource.latest?.datawheneverstate.data !== undefined. Per Solid's docs,.latestfalls back to aread()-equivalent (and therefore triggers Suspense) when the resource has no prior resolved value.In the preloaded-cache case, the resource's fetcher synchronously calls
resolve(...)because!observerResult.isLoading, but the resource doesn't transition out of the initialpendingstate until the next microtask. Reading.latestinside that microtask gap suspends the component even thoughstate.dataalready holds the cached value.Fix
If the store already has
dataand no fetch is in-flight (!state.isFetching), returnstate.datadirectly without going throughqueryResource.state.isFetchingis read viauntrackso it doesn't widen the data subscriber's reactive deps (preserves existing reconcile / memo-churn tests).When a fetch is in-flight, the existing
queryResource.latest?.datapath is preserved so background-refresh and re-mount semantics are unchanged.Verification
packages/solid-query/src/__tests__/suspense.test.tsxthat pre-populates the cache viaqueryClient.ensureQueryData(...), renders inside<Suspense>, and asserts the fallback never mounts. Test fails onmain(Suspense fires), passes with the fix.pnpm exec nx run @tanstack/solid-query:test:lib— all 310 tests pass.pnpm exec nx run @tanstack/solid-query:test:types— all TS versions pass.pnpm exec nx run @tanstack/solid-query:test:eslint— clean.patchfor@tanstack/solid-query).Test plan
🤖 Generated with Claude Code
Generated by Ora Studio
Vibe coded by ousamabenyounes
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Generated by Ora Studio (with Claude Code)
Vibe coded by Ben Younes Ousama