Uh oh!
There was an error while loading. Please reload this page.
fix(react-query): allow retryOnMount when throwOnError is function (#9336) - #9338
Merged
TkDodo merged 19 commits intoDec 30, 2025
Conversation
TkDodo
reviewed
Jun 30, 2025
Uh oh!
There was an error while loading. Please reload this page.
Han5991force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
from
July 6, 2025 01:31
f7b90c2 to
9c8516dCompareHan5991
commented
Jul 7, 2025
ContributorAuthor
@TkDodo Hi! Simple fix for retryOnMount when throwOnError is function. |
Han5991force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
2 times, most recently
from
August 13, 2025 14:20
334eca9 to
bcc8460Compare…cases When throwOnError is a function, allow retryOnMount to proceed even when error boundary hasn't reset, while maintaining the prevention behavior for boolean throwOnError values.
Refine behavior to handle function-type throwOnError, allowing retries when appropriate. Ensure boolean throwOnError values still prevent retries when the error boundary isn't reset.
This commit introduces tests to verify the behavior of the `throwOnError` callback in scenarios where `retryOnMount` is enabled. It ensures proper handling of retries based on the error state and the `throwOnError` function's return value. These tests improve the reliability and coverage of error handling logic in `useQuery`.
- Pass query object to ensurePreventErrorBoundaryRetry for accurate state checking - Preserve query deduplication behavior while fixing throwOnError function handling - Fixes issue where throwOnError function couldn't access query error state
Han5991force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
from
September 20, 2025 04:14
bcc8460 to
e8e0caaCompareContributor
📝 WalkthroughWalkthroughThe PR changes retry-on-mount behavior by evaluating Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Hook as useBaseQuery / useQueries
participant Cache as QueryCache
participant Query as Query (query.state)
participant EBUtils as ensurePreventErrorBoundaryRetry
participant ErrorReset as ErrorResetBoundary
rect rgba(200,230,255,0.4)
Hook->>Cache: get(queryHash)
Cache-->>Hook: Query?
Hook->>EBUtils: ensurePreventErrorBoundaryRetry(options, ErrorReset, Query)
end
rect rgba(230,255,220,0.4)
EBUtils->>EBUtils: if options.suspense || experimental_prefetchInRender -> return
EBUtils->>Query: read error state (if provided)
alt throwOnError is function
EBUtils->>EBUtils: shouldThrowError(query.state.error, throwOnError)
EBUtils-->>Hook: maybe set retryOnMount = false
else throwOnError truthy
EBUtils-->>Hook: set retryOnMount = false
end
end
Note over Hook,ErrorReset: Hook proceeds with query lifecycle honoring modified retryOnMount
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/react-query/src/__tests__/useQuery.test.tsx (2)
6785-6962: Optional: remove console.log noise in tests.The added tests log on each fetch attempt. Consider removing to keep CI output clean.
- console.log(`Fetching... (attempt ${fetchCount})`)
1-10: If you prefer waitFor, import from @testing-library/react (optional alternative).If you want to keep waitFor semantics instead of timer flushes, add the import and replace vi.advanceTimersByTimeAsync calls accordingly.
Additional change outside the selected ranges:
// at line 2import{act,fireEvent,render,waitFor}from'@testing-library/react'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-query/src/__tests__/useQuery.test.tsx(7 hunks)packages/react-query/src/errorBoundaryUtils.ts(1 hunks)packages/react-query/src/useBaseQuery.ts(2 hunks)packages/react-query/src/useQueries.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/react-query/src/errorBoundaryUtils.ts (2)
packages/query-core/src/index.ts (2)
Query(53-53)shouldThrowError(37-37)packages/query-core/src/utils.ts (1)
shouldThrowError(446-456)
packages/react-query/src/useBaseQuery.ts (2)
packages/query-core/src/queryObserver.ts (1)
query(702-719)packages/react-query/src/errorBoundaryUtils.ts (1)
ensurePreventErrorBoundaryRetry(13-47)
packages/react-query/src/__tests__/useQuery.test.tsx (1)
packages/react-query/src/__tests__/utils.tsx (1)
renderWithClient(9-23)
packages/react-query/src/useQueries.ts (2)
packages/react-query/src/suspense.ts (1)
ensureSuspenseTimers(21-47)packages/react-query/src/errorBoundaryUtils.ts (1)
ensurePreventErrorBoundaryRetry(13-47)
🔇 Additional comments (2)
packages/react-query/src/errorBoundaryUtils.ts (1)
28-46: Correctly evaluate function-valued throwOnError and gate retryOnMount.This implements the intended behavior: only disable retryOnMount when the error boundary isn't reset AND throwOnError is:
- a function that returns true for the current error, or
- a boolean true.
Looks good.
packages/react-query/src/useQueries.ts (1)
245-249: Pass the cached Query into ensurePreventErrorBoundaryRetry (good).Aligns multi-query flow with the updated errorBoundary handling. No issues.
Comment on lines
+6785
to
+6835
| it('should retry on mount when throwOnError returns false', async () => { | ||
| const key = queryKey() | ||
| let fetchCount = 0 | ||
| const queryFn = vi.fn().mockImplementation(() => { | ||
| fetchCount++ | ||
| console.log(`Fetching... (attempt ${fetchCount})`) | ||
| return Promise.reject(new Error('Simulated 500 error')) | ||
| }) | ||
| function Component() { | ||
| const { status, error } = useQuery({ | ||
| queryKey: key, | ||
| queryFn, | ||
| throwOnError: () => false, | ||
| retryOnMount: true, | ||
| staleTime: Infinity, | ||
| retry: false, | ||
| }) | ||
| return ( | ||
| <div> | ||
| <div data-testid="status">{status}</div> | ||
| {error && <div data-testid="error">{error.message}</div>} | ||
| </div> | ||
| ) | ||
| } | ||
| const { unmount, getByTestId } = renderWithClient( | ||
| queryClient, | ||
| <Component />, | ||
| ) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| expect(getByTestId('error')).toHaveTextContent('Simulated 500 error') | ||
| expect(fetchCount).toBe(1) | ||
| unmount() | ||
| const initialFetchCount = fetchCount | ||
| renderWithClient(queryClient, <Component />) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| expect(fetchCount).toBe(initialFetchCount + 1) | ||
| expect(queryFn).toHaveBeenCalledTimes(2) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix test flakiness: use fake-timer flush and don’t reuse getByTestId after unmount.
- vi.waitFor is not a vitest API; flush with vi.advanceTimersByTimeAsync(0) instead (fake timers are enabled).
- Don’t reuse queries from an unmounted render; capture a new render result.
Apply this diff:
- const { unmount, getByTestId } = renderWithClient(- queryClient,- <Component />,- )-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )- expect(getByTestId('error')).toHaveTextContent('Simulated 500 error')+ const rendered1 = renderWithClient(queryClient, <Component />)+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered1.getByTestId('status')).toHaveTextContent('error')+ expect(rendered1.getByTestId('error')).toHaveTextContent(+ 'Simulated 500 error',+ )- expect(fetchCount).toBe(1)-- unmount()+ expect(fetchCount).toBe(1)+ rendered1.unmount()
@@
- renderWithClient(queryClient, <Component />)-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )+ const rendered2 = renderWithClient(queryClient, <Component />)+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered2.getByTestId('status')).toHaveTextContent('error')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should retry on mount when throwOnError returns false',async()=>{ | |
| constkey=queryKey() | |
| letfetchCount=0 | |
| constqueryFn=vi.fn().mockImplementation(()=>{ | |
| fetchCount++ | |
| console.log(`Fetching... (attempt ${fetchCount})`) | |
| returnPromise.reject(newError('Simulated 500 error')) | |
| }) | |
| functionComponent(){ | |
| const{ status, error }=useQuery({ | |
| queryKey: key, | |
| queryFn, | |
| throwOnError: ()=>false, | |
| retryOnMount: true, | |
| staleTime: Infinity, | |
| retry: false, | |
| }) | |
| return( | |
| <div> | |
| <divdata-testid="status">{status}</div> | |
| {error&&<divdata-testid="error">{error.message}</div>} | |
| </div> | |
| ) | |
| } | |
| const{ unmount, getByTestId }=renderWithClient( | |
| queryClient, | |
| <Component/>, | |
| ) | |
| awaitvi.waitFor(()=> | |
| expect(getByTestId('status')).toHaveTextContent('error'), | |
| ) | |
| expect(getByTestId('error')).toHaveTextContent('Simulated 500 error') | |
| expect(fetchCount).toBe(1) | |
| unmount() | |
| constinitialFetchCount=fetchCount | |
| renderWithClient(queryClient,<Component/>) | |
| awaitvi.waitFor(()=> | |
| expect(getByTestId('status')).toHaveTextContent('error'), | |
| ) | |
| expect(fetchCount).toBe(initialFetchCount+1) | |
| expect(queryFn).toHaveBeenCalledTimes(2) | |
| }) | |
| it('should retry on mount when throwOnError returns false',async()=>{ | |
| constkey=queryKey() | |
| letfetchCount=0 | |
| constqueryFn=vi.fn().mockImplementation(()=>{ | |
| fetchCount++ | |
| console.log(`Fetching... (attempt ${fetchCount})`) | |
| returnPromise.reject(newError('Simulated 500 error')) | |
| }) | |
| functionComponent(){ | |
| const{ status, error }=useQuery({ | |
| queryKey: key, | |
| queryFn, | |
| throwOnError: ()=>false, | |
| retryOnMount: true, | |
| staleTime: Infinity, | |
| retry: false, | |
| }) | |
| return( | |
| <div> | |
| <divdata-testid="status">{status}</div> | |
| {error&&<divdata-testid="error">{error.message}</div>} | |
| </div> | |
| ) | |
| } | |
| // First mount | |
| constrendered1=renderWithClient(queryClient,<Component/>) | |
| awaitvi.advanceTimersByTimeAsync(0) | |
| expect(rendered1.getByTestId('status')).toHaveTextContent('error') | |
| expect( | |
| rendered1.getByTestId('error') | |
| ).toHaveTextContent('Simulated 500 error') | |
| expect(fetchCount).toBe(1) | |
| rendered1.unmount() | |
| constinitialFetchCount=fetchCount | |
| // Second mount | |
| constrendered2=renderWithClient(queryClient,<Component/>) | |
| awaitvi.advanceTimersByTimeAsync(0) | |
| expect(rendered2.getByTestId('status')).toHaveTextContent('error') | |
| expect(fetchCount).toBe(initialFetchCount+1) | |
| expect(queryFn).toHaveBeenCalledTimes(2) | |
| }) |
🤖 Prompt for AI Agents
packages/react-query/src/__tests__/useQuery.test.tsx around lines 6785 to 6835:
the test is flaky because it uses vi.waitFor (not a vitest API with fake timers)
and reuses getByTestId from a component after unmount; replace the vi.waitFor
flush with vi.advanceTimersByTimeAsync(0) to advance fake timers and ensure task
queue is flushed, and when re-rendering after unmount capture the new render
result (assign the return of renderWithClient to a new variable) and call
getByTestId on that new result instead of reusing the previous one so assertions
target a mounted DOM.
Comment on lines
+6837
to
+6909
| it('should not retry on mount when throwOnError function returns true', async () => { | ||
| const key = queryKey() | ||
| let fetchCount = 0 | ||
| const queryFn = vi.fn().mockImplementation(() => { | ||
| fetchCount++ | ||
| console.log(`Fetching... (attempt ${fetchCount})`) | ||
| return Promise.reject(new Error('Simulated 500 error')) | ||
| }) | ||
| function Component() { | ||
| const { status, error } = useQuery({ | ||
| queryKey: key, | ||
| queryFn, | ||
| throwOnError: () => true, | ||
| retryOnMount: true, | ||
| staleTime: Infinity, | ||
| retry: false, | ||
| }) | ||
| return ( | ||
| <div> | ||
| <div data-testid="status">{status}</div> | ||
| {error && <div data-testid="error">{error.message}</div>} | ||
| </div> | ||
| ) | ||
| } | ||
| const { unmount, getByTestId } = renderWithClient( | ||
| queryClient, | ||
| <ErrorBoundary | ||
| fallbackRender={({ error }) => ( | ||
| <div> | ||
| <div data-testid="status">error</div> | ||
| <div data-testid="error">{error?.message}</div> | ||
| </div> | ||
| )} | ||
| > | ||
| <Component /> | ||
| </ErrorBoundary>, | ||
| ) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| expect(getByTestId('error')).toHaveTextContent('Simulated 500 error') | ||
| expect(fetchCount).toBe(1) | ||
| unmount() | ||
| const initialFetchCount = fetchCount | ||
| renderWithClient( | ||
| queryClient, | ||
| <ErrorBoundary | ||
| fallbackRender={({ error }) => ( | ||
| <div> | ||
| <div data-testid="status">error</div> | ||
| <div data-testid="error">{error?.message}</div> | ||
| </div> | ||
| )} | ||
| > | ||
| <Component /> | ||
| </ErrorBoundary>, | ||
| ) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| // Should not retry because throwOnError returns true | ||
| expect(fetchCount).toBe(initialFetchCount) | ||
| expect(queryFn).toHaveBeenCalledTimes(1) | ||
| }) |
Contributor
There was a problem hiding this comment.
Same test issues: replace vi.waitFor and avoid stale container after unmount.
Mirror the fixes from the previous test; also keep assertions via the newly captured render result.
- const { unmount, getByTestId } = renderWithClient(- queryClient,- <ErrorBoundary- fallbackRender={({ error }) => (- <div>- <div data-testid="status">error</div>- <div data-testid="error">{error?.message}</div>- </div>- )}- >- <Component />- </ErrorBoundary>,- )-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )- expect(getByTestId('error')).toHaveTextContent('Simulated 500 error')+ const rendered1 = renderWithClient(+ queryClient,+ <ErrorBoundary+ fallbackRender={({ error }) => (+ <div>+ <div data-testid="status">error</div>+ <div data-testid="error">{error?.message}</div>+ </div>+ )}+ >+ <Component />+ </ErrorBoundary>,+ )+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered1.getByTestId('status')).toHaveTextContent('error')+ expect(rendered1.getByTestId('error')).toHaveTextContent('Simulated 500 error')
@@
- unmount()+ rendered1.unmount()
@@
- renderWithClient(- queryClient,- <ErrorBoundary- fallbackRender={({ error }) => (- <div>- <div data-testid="status">error</div>- <div data-testid="error">{error?.message}</div>- </div>- )}- >- <Component />- </ErrorBoundary>,- )-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )+ const rendered2 = renderWithClient(+ queryClient,+ <ErrorBoundary+ fallbackRender={({ error }) => (+ <div>+ <div data-testid="status">error</div>+ <div data-testid="error">{error?.message}</div>+ </div>+ )}+ >+ <Component />+ </ErrorBoundary>,+ )+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered2.getByTestId('status')).toHaveTextContent('error')🤖 Prompt for AI Agents
In packages/react-query/src/__tests__/useQuery.test.tsx around lines 6837 to
6909, the test still uses vi.waitFor and reads from a stale container after
unmount; replace vi.waitFor with the testing-library async waitFor helper and
capture the render result for the second render to avoid querying the old
container. Specifically, import/use waitFor (not vi.waitFor), store the result
of the second renderWithClient call in a variable (e.g., const { getByTestId } =
renderWithClient(...)) and use that getByTestId for the post-unmount assertions,
and update the async waits to await waitFor(() => expect(...)) so the test no
longer queries a stale container.
Comment on lines
+6911
to
+6962
| it('should handle throwOnError function based on actual error state', async () => { | ||
| const key = queryKey() | ||
| let fetchCount = 0 | ||
| const queryFn = vi.fn().mockImplementation(() => { | ||
| fetchCount++ | ||
| console.log(`Fetching... (attempt ${fetchCount})`) | ||
| return Promise.reject(new Error('Simulated 500 error')) | ||
| }) | ||
| function Component() { | ||
| const { status, error } = useQuery({ | ||
| queryKey: key, | ||
| queryFn, | ||
| throwOnError: (error) => error.message.includes('404'), | ||
| retryOnMount: true, | ||
| staleTime: Infinity, | ||
| retry: false, | ||
| }) | ||
| return ( | ||
| <div> | ||
| <div data-testid="status">{status}</div> | ||
| {error && <div data-testid="error">{error.message}</div>} | ||
| </div> | ||
| ) | ||
| } | ||
| const { unmount, getByTestId } = renderWithClient( | ||
| queryClient, | ||
| <Component />, | ||
| ) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| expect(getByTestId('error')).toHaveTextContent('Simulated 500 error') | ||
| expect(fetchCount).toBe(1) | ||
| unmount() | ||
| const initialFetchCount = fetchCount | ||
| renderWithClient(queryClient, <Component />) | ||
| await vi.waitFor(() => | ||
| expect(getByTestId('status')).toHaveTextContent('error'), | ||
| ) | ||
| // Should retry because throwOnError returns false (500 error doesn't include '404') | ||
| expect(fetchCount).toBe(initialFetchCount + 1) | ||
| expect(queryFn).toHaveBeenCalledTimes(2) | ||
| }) |
Contributor
There was a problem hiding this comment.
Same fixes for the 404/500 predicate case.
Use timer flush and distinct render containers after remount.
- const { unmount, getByTestId } = renderWithClient(- queryClient,- <Component />,- )-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )- expect(getByTestId('error')).toHaveTextContent('Simulated 500 error')+ const rendered1 = renderWithClient(queryClient, <Component />)+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered1.getByTestId('status')).toHaveTextContent('error')+ expect(rendered1.getByTestId('error')).toHaveTextContent('Simulated 500 error')
@@
- unmount()+ rendered1.unmount()
@@
- renderWithClient(queryClient, <Component />)-- await vi.waitFor(() =>- expect(getByTestId('status')).toHaveTextContent('error'),- )+ const rendered2 = renderWithClient(queryClient, <Component />)+ await vi.advanceTimersByTimeAsync(0)+ expect(rendered2.getByTestId('status')).toHaveTextContent('error')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should handle throwOnError function based on actual error state',async()=>{ | |
| constkey=queryKey() | |
| letfetchCount=0 | |
| constqueryFn=vi.fn().mockImplementation(()=>{ | |
| fetchCount++ | |
| console.log(`Fetching... (attempt ${fetchCount})`) | |
| returnPromise.reject(newError('Simulated 500 error')) | |
| }) | |
| functionComponent(){ | |
| const{ status, error }=useQuery({ | |
| queryKey: key, | |
| queryFn, | |
| throwOnError: (error)=>error.message.includes('404'), | |
| retryOnMount: true, | |
| staleTime: Infinity, | |
| retry: false, | |
| }) | |
| return( | |
| <div> | |
| <divdata-testid="status">{status}</div> | |
| {error&&<divdata-testid="error">{error.message}</div>} | |
| </div> | |
| ) | |
| } | |
| const{ unmount, getByTestId }=renderWithClient( | |
| queryClient, | |
| <Component/>, | |
| ) | |
| awaitvi.waitFor(()=> | |
| expect(getByTestId('status')).toHaveTextContent('error'), | |
| ) | |
| expect(getByTestId('error')).toHaveTextContent('Simulated 500 error') | |
| expect(fetchCount).toBe(1) | |
| unmount() | |
| constinitialFetchCount=fetchCount | |
| renderWithClient(queryClient,<Component/>) | |
| awaitvi.waitFor(()=> | |
| expect(getByTestId('status')).toHaveTextContent('error'), | |
| ) | |
| // Should retry because throwOnError returns false (500 error doesn't include '404') | |
| expect(fetchCount).toBe(initialFetchCount+1) | |
| expect(queryFn).toHaveBeenCalledTimes(2) | |
| }) | |
| it('should handle throwOnError function based on actual error state',async()=>{ | |
| constkey=queryKey() | |
| letfetchCount=0 | |
| constqueryFn=vi.fn().mockImplementation(()=>{ | |
| fetchCount++ | |
| console.log(`Fetching... (attempt ${fetchCount})`) | |
| returnPromise.reject(newError('Simulated 500 error')) | |
| }) | |
| functionComponent(){ | |
| const{ status, error }=useQuery({ | |
| queryKey: key, | |
| queryFn, | |
| throwOnError: (error)=>error.message.includes('404'), | |
| retryOnMount: true, | |
| staleTime: Infinity, | |
| retry: false, | |
| }) | |
| return( | |
| <div> | |
| <divdata-testid="status">{status}</div> | |
| {error&&<divdata-testid="error">{error.message}</div>} | |
| </div> | |
| ) | |
| } | |
| // First render | |
| constrendered1=renderWithClient(queryClient,<Component/>) | |
| awaitvi.advanceTimersByTimeAsync(0) | |
| expect(rendered1.getByTestId('status')).toHaveTextContent('error') | |
| expect(rendered1.getByTestId('error')).toHaveTextContent('Simulated 500 error') | |
| expect(fetchCount).toBe(1) | |
| rendered1.unmount() | |
| constinitialFetchCount=fetchCount | |
| // Remount in a fresh container | |
| constrendered2=renderWithClient(queryClient,<Component/>) | |
| awaitvi.advanceTimersByTimeAsync(0) | |
| expect(rendered2.getByTestId('status')).toHaveTextContent('error') | |
| // Should retry because throwOnError returns false (500 error doesn't include '404') | |
| expect(fetchCount).toBe(initialFetchCount+1) | |
| expect(queryFn).toHaveBeenCalledTimes(2) | |
| }) |
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
82
to
84
| ensureSuspenseTimers(defaultedOptions) | ||
| ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary) | ||
| ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary, query) | ||
| useClearResetErrorBoundary(errorResetBoundary) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a fresh cache lookup when calling ensurePreventErrorBoundaryRetry.
Inline the query read here so it reflects any changes done by _experimental_beforeQuery.
- ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary, query)+ ensurePreventErrorBoundaryRetry(+ defaultedOptions,+ errorResetBoundary,+ client+ .getQueryCache()+ .get<TQueryFnData, TError, TQueryData, TQueryKey>(+ defaultedOptions.queryHash,+ ),+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ensureSuspenseTimers(defaultedOptions) | |
| ensurePreventErrorBoundaryRetry(defaultedOptions,errorResetBoundary) | |
| ensurePreventErrorBoundaryRetry(defaultedOptions,errorResetBoundary,query) | |
| useClearResetErrorBoundary(errorResetBoundary) | |
| ensureSuspenseTimers(defaultedOptions) | |
| ensurePreventErrorBoundaryRetry( | |
| defaultedOptions, | |
| errorResetBoundary, | |
| client | |
| .getQueryCache() | |
| .get<TQueryFnData,TError,TQueryData,TQueryKey>( | |
| defaultedOptions.queryHash, | |
| ), | |
| ) | |
| useClearResetErrorBoundary(errorResetBoundary) |
🤖 Prompt for AI Agents
In packages/react-query/src/useBaseQuery.ts around lines 82 to 84, inline a
fresh query cache lookup when calling ensurePreventErrorBoundaryRetry instead of
passing the stale "query" variable so changes from _experimental_beforeQuery are
reflected; replace the current call with one that reads the up-to-date query
from the queryCache (e.g. perform a direct cache lookup/find here) and pass that
freshQuery into ensurePreventErrorBoundaryRetry, keeping the surrounding
ensureSuspenseTimers and useClearResetErrorBoundary calls unchanged.
🦋 Changeset detectedLatest commit: 9fd84a6 The changes in this PR will be included in the next version bump. 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 |
…#9338) - Replace vi.waitFor with vi.advanceTimersByTimeAsync in tests - Use separate render results to avoid stale DOM references - Inline fresh query lookup in ensurePreventErrorBoundaryRetry after _experimental_beforeQuery
TkDodo
reviewed
Dec 28, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
View your CI Pipeline Execution ↗ for commit 9fd84a6
☁️ Nx Cloud last updated this comment at |
- move query retrieval after `_experimental_beforeQuery` in `useBaseQuery` - refactor `ensurePreventErrorBoundaryRetry` for better clarity - make `query` parameter required in `ensurePreventErrorBoundaryRetry`
ContributorAuthor
Hi @TkDodo, thank you for the review! I've applied your suggestions:
|
TkDodo
reviewed
Dec 29, 2025
Uh oh!
There was an error while loading. Please reload this page.
…eventErrorBoundaryRetry
TkDodo
reviewed
Dec 30, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #9338 +/- ##
===========================================
+ Coverage 45.79% 84.12% +38.32%
===========================================
Files 200 26 -174 Lines 8520 378 -8142 Branches 1975 111 -1864 ===========================================
- Hits 3902 318 -3584 + Misses 4158 51 -4107 + Partials 460 9 -451
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Merged
Contributor
🎉 This PR has been released! Thank you for your contribution! |
4 tasks
TkDodo added a commit
that referenced
this pull request
Aug 20, 2026
…11166) Ports #9338 to preact-query: ensurePreventErrorBoundaryRetry now receives the cached query and evaluates a function-form throwOnError against the actual error before setting retryOnMount = false, instead of treating any function as truthy. Includes the regression tests from #9338, two of which fail on current main. Fixes#11165 Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Dominik Dorfmeister 🔮 <office@dorfmeister.cc>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixed#9336
Summary
Summary by CodeRabbit
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.