fix(react-query): retry fetch on remount when error boundary has not been reset#10962
fix(react-query): retry fetch on remount when error boundary has not been reset#10962EduardF1 wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesretryOnMount guard and test alignment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
nahrinoda
left a comment
There was a problem hiding this comment.
The fix direction is correct. ensurePreventErrorBoundaryRetry is called in useBaseQuery (line 87) BEFORE the observer is created (line 95-101), so query?.getObserversCount() reflects only other active observers — on a fresh remount with no siblings, it's 0, allowing retry. The suspense carve-out makes sense: suspense queries must go through the reset flow because React's Suspense/ErrorBoundary lifecycle manages retries differently.
One question on the second test change (line 483): the fireEvent.click was moved UP before advanceTimersByTimeAsync, and the intermediate retry step (old lines 494-499) was removed entirely. This collapses two user interactions into one. Can you confirm this test still validates the "after a previous reset" scenario — i.e., that a second error→retry cycle (where the boundary is NOT reset) still recovers? As written, it looks like the test now only covers one error→reset→retry cycle followed by a single non-reset retry, which is less coverage than the original three-step flow.
|
@nahrinoda yes — the reworked test still covers the second error → retry cycle without the error boundary being reset. The sequence remains: initial error, boundary reset enables the first remount retry, that retry still returns an error, then the next remount happens with no additional boundary reset and verifies we still retry again and can reach data. I left the code unchanged because the coverage is preserved. |
Summary
useQuerywiththrowOnErrorwould refuse to retry on remount even after the error boundary had unmounted and remounted the failed observer — effectively leaving the component stuck in an error state with no recovery path unless the user explicitly reset the boundary.ensurePreventErrorBoundaryRetryinerrorBoundaryUtils.ts: retry is only suppressed when the boundary has not been reset and the query is either usingsuspenseor still has live observers. On a fresh mount (zero observers), the retry suppression is lifted so the remounted component can attempt a new fetch.staleTime: Infinity.Files changed
packages/react-query/src/errorBoundaryUtils.ts— guard loosened to allow remount retrypackages/react-query/src/__tests__/QueryResetErrorBoundary.test.tsx— two new/updated test casesTest plan
pnpm run test:lib -- --run QueryResetErrorBoundaryinpackages/react-query— all 14 tests passpnpm test) — to be run by CI🤖 Generated with Claude Code
Summary by CodeRabbit