fix(query-core): ensure query refetches on mount/retry when status is error (#9728) #9927
Conversation
🦋 Changeset detectedLatest commit: 233d48e The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR patches Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
xiangnuans
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/open-keys-create.md (1)
1-5: Changeset scope and message look accurate; optionally mention focus/reconnect.Patch scope to
@tanstack/query-coreand the summary align with theshouldFetchOnchange. Note that the implementation also affects refetch on window focus and reconnect via the same helper, which is fine but not called out here; up to you if you want to spell that out in the note.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/open-keys-create.md(1 hunks)packages/query-core/src/queryObserver.ts(1 hunks)packages/react-query/src/__tests__/issue-9728.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/react-query/src/__tests__/issue-9728.test.tsx
📚 Learning: 2025-09-02T17:57:33.184Z
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
Applied to files:
.changeset/open-keys-create.md
🧬 Code graph analysis (2)
packages/query-core/src/queryObserver.ts (1)
packages/query-core/src/query.ts (1)
isStale(296-306)
packages/react-query/src/__tests__/issue-9728.test.tsx (1)
packages/query-core/src/queryObserver.ts (1)
refetch(298-304)
🔇 Additional comments (4)
packages/query-core/src/queryObserver.ts (1)
771-791: Error-awareshouldFetchOnlogic matches the intended fix.Adding
query.state.status === 'error'to the fetch predicate ensures that enabled queries with non‑static staleTime will refetch on mount/focus/reconnect when they are in an error state, even if their data isn’t time‑stale, which is exactly what issue 9728 describes. Guarding withvalue !== falsepreserves opt‑out viarefetchOn* = false, and the existingstaleTime === 'static'early return means static queries keep their previous behavior.packages/react-query/src/__tests__/issue-9728.test.tsx (3)
16-36: Deterministic queryFn and client config nicely model the regression.Using a
vi.fnwith acount‑based async implementation plusretry: falseandstaleTime: Infinityprovides a clear, deterministic setup for success→error→success, matching the reported bug scenario.
37-78: Error boundary + reset wiring correctly exercises “retry via remount”.
Pagethrowing viathrowOnError: trueunderErrorBoundaryandQueryErrorResetBoundary, with the Retry button callingresetErrorBoundary, ensures the query is remounted after an error in the presence of existing data—exactly the path the core fix targets.
88-107: Assertions cover initial success, error transition, and final successful refetch.The three phases—first render success (1 call), refetch leading to error UI (2 calls), and Retry leading to “Success 3” (3 calls)—give good coverage for the bug and will regress if
shouldFetchOnstops refetching on error with non‑stale data.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 3m 33s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-28 16:03:45 UTC
| return ( | ||
| value === 'always' || | ||
| (value !== false && | ||
| (isStale(query, options) || query.state.status === 'error')) | ||
| ) |
There was a problem hiding this comment.
I think we can make the argument that if a query errors and has data, that this data should be considered stale. However, this check here doesn’t convey that and likely misses a couple of cases, e.g. staleTime: 'static', which definitely shouldn't be refetched.
We do however have the option to mark a query as invalidated, and then the functions isStale and isStaleByTime on a query both check that in the correct order.
So maybe the right fix would be to just set isInvalidated in the reducer when we get an error:
query/packages/query-core/src/query.ts
Line 650 in 1638c02
I think setting it to:
// flag existing data as invalidated if we get a background error
isInvalidated: true,
should suffice because if we get an error and have no data yet, we’re always implicitly stale
There was a problem hiding this comment.
Good point — I agree that semantically, “error with existing data” could be modeled as invalidated, and that would keep all stale logic centralized in isStale / isStaleByTime.
For this PR, I tried to keep the change narrowly scoped to fixing the concrete regression in #9728 via shouldFetchOn, without altering global invalidation semantics (which could have broader effects, e.g. with staleTime: 'static').
That said, I do think invalidating on background errors could be a cleaner long-term model. Happy to explore that in a follow-up PR if that aligns with the direction you’d prefer.
There was a problem hiding this comment.
yeah I’d prefer this direction really because I don’t think I can merge this PR as it would have the regression of queries with staleTime: 'static' getting refetched if they have an error even though they shouldn't. The flow is:
staleTime: 'static'- fetch successfully
- re-fetch with an error
now this solution would then return true from refetchOn, but the solution that sets isInvalidated would not.
If you want, you can change the approach in this PR or file a new one.
There was a problem hiding this comment.
would also be great to have a test for this flow ⬆️
There was a problem hiding this comment.
Got it — thanks for the clarification, that makes sense 👍
I agree this would be a regression for staleTime: 'static', and the invalidation approach is the right abstraction.
I’ll update this PR to:
- remove the observer-level
status === 'error'check - instead mark queries as
isInvalidatedon background errors in the reducer - add a regression test for the
staleTime: 'static' → success → errorflow you described
Will push an update shortly.
There was a problem hiding this comment.
i think your fix is correct but we can simplify the code by just doing isInvalidated: true unconditionally. The reason is that if we have no data, the query is always considered stale:
query/packages/query-core/src/query.ts
Lines 308 to 312 in 1638c02
so setting isInvalidate: true if we have no data doesn’t matter. Maybe leave a comment though :)
There was a problem hiding this comment.
Makes sense! Since isStaleByTime already treats queries with no data as stale, we can simplify this unconditionally. Updated accordingly.
…king error status - Remove error status check in shouldFetchOn - Set isInvalidated: true in reducer when background error occurs (only if data exists) - Add tests for staleTime: 'static' and non-static queries with background errors This approach centralizes stale logic in isStale/isStaleByTime and prevents regression where staleTime: 'static' queries would incorrectly refetch on window focus after a background error.
…nuans/query into fix/9728-retry-after-error
Set isInvalidated: true unconditionally since queries with no data are always considered stale per isStaleByTime logic.
…nuans/query into fix/9728-retry-after-error
2 participants
error (#9728)
🎯 Changes
shouldFetchOninpackages/query-core/src/queryObserver.ts:query.state.status === 'error'.refetchOnWindowFocus,refetchOnMount, or manual retry via ErrorBoundary), it will trigger a fetch if the query is in an error state, even if the data is technically considered "fresh" (not stale) due tostaleTime: Infinity.packages/react-query/src/__tests__/issue-9728.test.tsxto reproduce the issue and verify the fix.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.