fix(react-query): allow retryOnMount when throwOnError is function (#9336) #9338
Conversation
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
TkDodo
reviewed
Jun 30, 2025
| options.retryOnMount = false | ||
| } | ||
| } else if (options.throwOnError) { | ||
| if (!errorResetBoundary.isReset() && typeof options.throwOnError !== 'function') { |
There was a problem hiding this comment.
The function can return true or false so I think we'd need to evaluate it to get the result
There was a problem hiding this comment.
Updated to evaluate the throwOnError function result instead of just checking if it's a function. Thanks for the feedback!
Han5991
force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
from
f7b90c2 to
9c8516d
Compare
|
@TkDodo Hi! Simple fix for retryOnMount when throwOnError is function. |
Han5991
force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
2 times, most recently
from
334eca9 to
bcc8460
Compare
…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
Han5991
force-pushed
the
fix/usequery-retry-on-mount-with-throw-on-error-issue(#9336)
branch
from
bcc8460 to
e8e0caa
Compare
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 2 import { 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) | ||
| }) |
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 () => { | |
| 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) | |
| }) | |
| 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> | |
| ) | |
| } | |
| // First mount | |
| 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) | |
| rendered1.unmount() | |
| const initialFetchCount = fetchCount | |
| // Second mount | |
| const rendered2 = renderWithClient(queryClient, <Component />) | |
| await vi.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) | ||
| }) |
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) | ||
| }) |
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 () => { | |
| 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) | |
| }) | |
| 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> | |
| ) | |
| } | |
| // First render | |
| 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) | |
| rendered1.unmount() | |
| const initialFetchCount = fetchCount | |
| // Remount in a fresh container | |
| const rendered2 = renderWithClient(queryClient, <Component />) | |
| await vi.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) | |
| }) |
Comment on lines
+56
to
+63
| const query = client | ||
| .getQueryCache() | ||
| .get< | ||
| TQueryFnData, | ||
| TError, | ||
| TQueryData, | ||
| TQueryKey | ||
| >(defaultedOptions.queryHash) |
There was a problem hiding this comment.
Compute the Query after _experimental_beforeQuery to avoid stale queryHash mismatches.
You fetch the cached query before calling _experimental_beforeQuery, which can mutate options (incl. queryKey/queryHash). This risks passing the wrong query into ensurePreventErrorBoundaryRetry. Inline the cache lookup at the call-site after the hook and drop this precomputed variable.
Apply this diff:
- const query = client
- .getQueryCache()
- .get<
- TQueryFnData,
- TError,
- TQueryData,
- TQueryKey
- >(defaultedOptions.queryHash)Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
82
to
84
| ensureSuspenseTimers(defaultedOptions) | ||
| ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary) | ||
|
|
||
| ensurePreventErrorBoundaryRetry(defaultedOptions, errorResetBoundary, query) | ||
| useClearResetErrorBoundary(errorResetBoundary) |
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.
|
lachlancollins
force-pushed
the
main
branch
from
5d7869e to
18a12ca
Compare
Sign up for free
to 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
Bug Fixes
Tests