build(react-query): update queryerrorresetboundary to use a more robust reset mechanism with reset id#10375
Conversation
…obust reset mechanism with reset id Replace the simple boolean isResetRef flag (which is consumed by the first query to call getIsReset) with a numeric reset ID. This prevents successful queries from consuming the reset signal and ensures all errored queries under the boundary can observe the same reset event. The getResetId method returns the current ID without side effects, while reset() increments it. Affected files: QueryErrorResetBoundary.tsx, useBaseQuery.ts Signed-off-by: ChinhLee <76194645+chinhkrb113@users.noreply.github.com>
…obust reset mechanism with reset id Replace the simple boolean isResetRef flag (which is consumed by the first query to call getIsReset) with a numeric reset ID. This prevents successful queries from consuming the reset signal and ensures all errored queries under the boundary can observe the same reset event. The getResetId method returns the current ID without side effects, while reset() increments it. Affected files: QueryErrorResetBoundary.tsx, useBaseQuery.ts Signed-off-by: ChinhLee <76194645+chinhkrb113@users.noreply.github.com>
📝 WalkthroughWalkthroughQuery error reset handling transitions from a boolean state accessor to a numeric identifier system. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-query/src/useBaseQuery.ts`:
- Around line 103-105: The current useBaseQuery effect always calls
errorResetBoundary.clearReset() on mount/update which can prematurely reset the
error reset ID; modify useBaseQuery so clearReset is only invoked when
appropriate (e.g., after a successful query render or when the query has
transitioned to a non-error state) rather than unconditionally on every
mount/update. Locate the React.useEffect in useBaseQuery that references
errorResetBoundary and change the condition to check the query's status/previous
status (or a success flag) before calling errorResetBoundary.clearReset(),
ensuring errorResetBoundary (from useQueryErrorResetBoundary) remains stable and
that clearReset is only called when queries are ready to observe the reset
signal.
- Around line 107-109: The useEffect in useBaseQuery currently calls
observer.subscribe(notifyManager.batchCalls(() => {})) which supplies an empty
callback so state changes never trigger re-renders; replace the empty function
with a real updater (e.g., a state setter or forceUpdate) wrapped by
notifyManager.batchCalls so the component re-renders when the observer notifies,
and add a cleanup that unsubscribes the returned subscription when the effect
tears down; locate useBaseQuery and update the observer.subscribe call and
effect cleanup accordingly.
- Around line 111-112: The call to shouldSuspend currently passes an extra
argument and returns a Promise from fetchOptimistic which breaks the declared
return type of useBaseQuery (QueryObserverResult) and prevents React Suspense
from working; fix it by first checking isRestoring separately (don’t pass it
into shouldSuspend(defaultedOptions, result)), and when suspension is required
call fetchOptimistic(defaultedOptions, observer, errorResetBoundary) and throw
the resulting Promise (not return it) so the function maintains its synchronous
return type while enabling React Suspense; update the code paths around
shouldSuspend, isRestoring and fetchOptimistic in useBaseQuery accordingly.
- Line 99: The call observer.setOptions(defaultedOptions, { isNewCacheEntry })
passes a second arg that's ignored because setOptions in queryObserver.ts only
accepts one parameter; either extend setOptions in QueryObserver
(queryObserver.ts) to accept a second parameter (e.g., setOptions(options, {
isNewCacheEntry }) ) and have QueryObserver/observer logic consume that flag, or
remove the second parameter here in useBaseQuery.ts and ensure the
isNewCacheEntry handling is moved to where setOptions actually reads options
(e.g., attach isNewCacheEntry into defaultedOptions before calling setOptions or
perform the related logic immediately after computing isNewCacheEntry). Use the
symbols observer.setOptions, defaultedOptions, isNewCacheEntry and
QueryObserver/setOptions in your change to keep references clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab6705dc-05d8-4db2-b524-2dcaf3a17a22
📒 Files selected for processing (2)
packages/react-query/src/QueryErrorResetBoundary.tsxpackages/react-query/src/useBaseQuery.ts
| export interface QueryErrorResetBoundaryValue { | ||
| clearReset: QueryErrorClearResetFunction | ||
| isReset: QueryErrorIsResetFunction | ||
| getResetId: QueryErrorGetResetIdFunction | ||
| reset: QueryErrorResetFunction | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of isReset() in the codebase to confirm incomplete migration
echo "=== Searching for isReset() calls ==="
rg -n 'isReset\(\)' --type ts
echo ""
echo "=== Searching for .isReset references ==="
rg -n '\.isReset' --type tsRepository: TanStack/query
Length of output: 854
🏁 Script executed:
# Read the QueryErrorResetBoundary.tsx file to see the interface and implementation
cat -n packages/react-query/src/QueryErrorResetBoundary.tsx | head -40Repository: TanStack/query
Length of output: 1296
🏁 Script executed:
# Read errorBoundaryUtils.ts to see how isReset() is being used
cat -n packages/react-query/src/errorBoundaryUtils.ts | head -100Repository: TanStack/query
Length of output: 2682
Critical: Interface change breaks consumers in errorBoundaryUtils.ts.
The interface no longer exposes isReset() but only getResetId(). However, errorBoundaryUtils.ts still calls errorResetBoundary.isReset() at lines 41 and 76, and the same issue exists in preact-query/src/errorBoundaryUtils.ts at lines 35 and 70. This will cause a runtime error:
TypeError: errorResetBoundary.isReset is not a function
Update the consumer code to use the new getResetId() API with appropriate comparison logic, or keep isReset() in the interface alongside getResetId() for backward compatibility.
|
|
||
| // note: this must be called before useSyncExternalStore | ||
| const result = observer.getOptimisticResult(defaultedOptions) | ||
| observer.setOptions(defaultedOptions, { isNewCacheEntry }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify setOptions signature in query-core
echo "=== setOptions method signature ==="
ast-grep --pattern 'setOptions($PARAM) {
$$$
}'Repository: TanStack/query
Length of output: 93
🏁 Script executed:
# Find the files mentioned
fd -type f -name "useBaseQuery.ts"
fd -type f -name "queryObserver.ts"Repository: TanStack/query
Length of output: 399
🏁 Script executed:
# Find the files mentioned
fd "useBaseQuery.ts"
fd "queryObserver.ts"Repository: TanStack/query
Length of output: 310
🏁 Script executed:
# Read useBaseQuery.ts from react-query around line 99
wc -l packages/react-query/src/useBaseQuery.ts
head -110 packages/react-query/src/useBaseQuery.ts | tail -20Repository: TanStack/query
Length of output: 597
🏁 Script executed:
# Read queryObserver.ts around lines 138-176 to see setOptions signature
wc -l packages/query-core/src/queryObserver.ts
sed -n '130,180p' packages/query-core/src/queryObserver.tsRepository: TanStack/query
Length of output: 1345
🏁 Script executed:
# Check lines 87-89 in useBaseQuery.ts to see the "dead code" mentioned
sed -n '85,92p' packages/react-query/src/useBaseQuery.tsRepository: TanStack/query
Length of output: 295
🏁 Script executed:
# Search for all setOptions definitions to check for overloads or multiple implementations
rg "setOptions\s*\(" packages/query-core/src/queryObserver.tsRepository: TanStack/query
Length of output: 148
🏁 Script executed:
# Check if isNewCacheEntry is used anywhere else in the file
rg "isNewCacheEntry" packages/react-query/src/useBaseQuery.ts -A 2 -B 2Repository: TanStack/query
Length of output: 381
🏁 Script executed:
# Check if Observer class extends from QueryObserver or if there are other setOptions definitions
rg "class.*Observer" packages/query-core/src/queryObserver.ts -A 3Repository: TanStack/query
Length of output: 160
🏁 Script executed:
# Check what Observer is imported as in useBaseQuery.ts
head -30 packages/react-query/src/useBaseQuery.ts | grep -E "^import|^export|Observer"Repository: TanStack/query
Length of output: 501
🏁 Script executed:
# Verify the exact line numbers for context around the setOptions call
sed -n '85,105p' packages/react-query/src/useBaseQuery.ts | cat -nRepository: TanStack/query
Length of output: 786
Remove second parameter from setOptions call or update the method signature to accept it.
observer.setOptions() in queryObserver.ts accepts only a single options parameter, but line 99 passes a second argument { isNewCacheEntry } which is silently ignored. The isNewCacheEntry value computed at lines 87-89 is never actually used.
Either:
- Update the
setOptionssignature inquery-coreto accept and handleisNewCacheEntry, or - Remove the second parameter and handle
isNewCacheEntrylogic differently
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/useBaseQuery.ts` at line 99, The call
observer.setOptions(defaultedOptions, { isNewCacheEntry }) passes a second arg
that's ignored because setOptions in queryObserver.ts only accepts one
parameter; either extend setOptions in QueryObserver (queryObserver.ts) to
accept a second parameter (e.g., setOptions(options, { isNewCacheEntry }) ) and
have QueryObserver/observer logic consume that flag, or remove the second
parameter here in useBaseQuery.ts and ensure the isNewCacheEntry handling is
moved to where setOptions actually reads options (e.g., attach isNewCacheEntry
into defaultedOptions before calling setOptions or perform the related logic
immediately after computing isNewCacheEntry). Use the symbols
observer.setOptions, defaultedOptions, isNewCacheEntry and
QueryObserver/setOptions in your change to keep references clear.
| React.useEffect(() => { | ||
| observer.setOptions(defaultedOptions) | ||
| }, [defaultedOptions, observer]) | ||
| errorResetBoundary.clearReset() | ||
| }, [errorResetBoundary]) |
There was a problem hiding this comment.
Effect dependency may cause excessive clearReset calls.
The effect runs whenever errorResetBoundary changes. Since errorResetBoundary comes from useQueryErrorResetBoundary() (context), it should be stable. However, this unconditionally calls clearReset() on every mount, which resets the ID counter to 0.
This conflicts with the stated PR objective: if clearReset() is called on mount before queries can observe the reset ID, the reset signal may be lost before errored queries can act on it.
Consider whether this effect should only run under specific conditions (e.g., after a successful query render).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/useBaseQuery.ts` around lines 103 - 105, The current
useBaseQuery effect always calls errorResetBoundary.clearReset() on mount/update
which can prematurely reset the error reset ID; modify useBaseQuery so
clearReset is only invoked when appropriate (e.g., after a successful query
render or when the query has transitioned to a non-error state) rather than
unconditionally on every mount/update. Locate the React.useEffect in
useBaseQuery that references errorResetBoundary and change the condition to
check the query's status/previous status (or a success flag) before calling
errorResetBoundary.clearReset(), ensuring errorResetBoundary (from
useQueryErrorResetBoundary) remains stable and that clearReset is only called
when queries are ready to observe the reset signal.
| React.useEffect(() => { | ||
| observer.subscribe(notifyManager.batchCalls(() => {})) | ||
| }, [observer]) |
There was a problem hiding this comment.
Critical: Empty subscription callback prevents component re-renders.
The subscription callback is empty (() => {}), which means the component will never re-render when query state changes. This completely breaks the hook's reactivity.
The previous implementation likely used useSyncExternalStore or passed a callback that triggers re-renders (e.g., via forceUpdate or state setter). Without this, query data changes, loading states, and errors won't cause the component to update.
🐛 Suggested fix pattern
+ const [, forceUpdate] = React.useReducer((x) => x + 1, 0)
+
React.useEffect(() => {
- observer.subscribe(notifyManager.batchCalls(() => {}))
+ const unsubscribe = observer.subscribe(
+ notifyManager.batchCalls(forceUpdate)
+ )
+ return unsubscribe
}, [observer])Also note: the effect is missing a cleanup function to unsubscribe when the component unmounts or observer changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/useBaseQuery.ts` around lines 107 - 109, The
useEffect in useBaseQuery currently calls
observer.subscribe(notifyManager.batchCalls(() => {})) which supplies an empty
callback so state changes never trigger re-renders; replace the empty function
with a real updater (e.g., a state setter or forceUpdate) wrapped by
notifyManager.batchCalls so the component re-renders when the observer notifies,
and add a cleanup that unsubscribes the returned subscription when the effect
tears down; locate useBaseQuery and update the observer.subscribe call and
effect cleanup accordingly.
| if (shouldSuspend(defaultedOptions, result, isRestoring)) { | ||
| return fetchOptimistic(defaultedOptions, observer, errorResetBoundary) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify shouldSuspend signature
echo "=== shouldSuspend function signature ==="
rg -n -A 5 'export const shouldSuspend' packages/react-query/src/suspense.tsRepository: TanStack/query
Length of output: 338
🏁 Script executed:
# Find useBaseQuery.ts and read the relevant lines
cd packages/react-query/src && cat -n useBaseQuery.ts | sed -n '100,125p'Repository: TanStack/query
Length of output: 804
🏁 Script executed:
# Find fetchOptimistic function definition
rg -n -B 2 -A 10 'export.*fetchOptimistic|const fetchOptimistic' packages/react-query/src/Repository: TanStack/query
Length of output: 837
🏁 Script executed:
# Check the return type of useBaseQuery hook
rg -n 'function useBaseQuery|const useBaseQuery' -A 3 packages/react-query/src/useBaseQuery.tsRepository: TanStack/query
Length of output: 134
🏁 Script executed:
# Get complete fetchOptimistic function to see return type
rg -n -A 25 'export const fetchOptimistic = ' packages/react-query/src/suspense.tsRepository: TanStack/query
Length of output: 595
🏁 Script executed:
# Get useBaseQuery full signature with return type
rg -n -A 15 'export function useBaseQuery' packages/react-query/src/useBaseQuery.tsRepository: TanStack/query
Length of output: 388
🏁 Script executed:
# Get useBaseQuery return type (should follow the parameter list)
cat -n packages/react-query/src/useBaseQuery.ts | sed -n '27,50p'Repository: TanStack/query
Length of output: 997
Arity and return type mismatches prevent this code from compiling.
Line 111-112 contains two critical issues:
-
Arity mismatch:
shouldSuspendaccepts exactly 2 parameters (defaultedOptions,result), but 3 are passed includingisRestoring. TypeScript will error on the extra argument. -
Return type mismatch:
useBaseQueryis declared to returnQueryObserverResult<TData, TError>(line 43), but line 112 returns the result offetchOptimistic, which returns aPromise. This violates the return type contract.
Additionally, for React Suspense to work correctly, the Promise must be thrown, not returned.
Fix
- if (shouldSuspend(defaultedOptions, result, isRestoring)) {
- return fetchOptimistic(defaultedOptions, observer, errorResetBoundary)
+ if (shouldSuspend(defaultedOptions, result) && !isRestoring) {
+ throw fetchOptimistic(defaultedOptions, observer, errorResetBoundary)
}The isRestoring check must be separate since it's not part of shouldSuspend's signature. The Promise should be thrown to trigger React Suspense behavior.
📝 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.
| if (shouldSuspend(defaultedOptions, result, isRestoring)) { | |
| return fetchOptimistic(defaultedOptions, observer, errorResetBoundary) | |
| if (shouldSuspend(defaultedOptions, result) && !isRestoring) { | |
| throw fetchOptimistic(defaultedOptions, observer, errorResetBoundary) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/useBaseQuery.ts` around lines 111 - 112, The call to
shouldSuspend currently passes an extra argument and returns a Promise from
fetchOptimistic which breaks the declared return type of useBaseQuery
(QueryObserverResult) and prevents React Suspense from working; fix it by first
checking isRestoring separately (don’t pass it into
shouldSuspend(defaultedOptions, result)), and when suspension is required call
fetchOptimistic(defaultedOptions, observer, errorResetBoundary) and throw the
resulting Promise (not return it) so the function maintains its synchronous
return type while enabling React Suspense; update the code paths around
shouldSuspend, isRestoring and fetchOptimistic in useBaseQuery accordingly.
1 participant
Code Quality
Problem
Replace the simple boolean isResetRef flag (which is consumed by the first query to call getIsReset) with a numeric reset ID. This prevents successful queries from consuming the reset signal and ensures all errored queries under the boundary can observe the same reset event. The getResetId method returns the current ID without side effects, while reset() increments it.
Severity:
highFile:
packages/react-query/src/QueryErrorResetBoundary.tsxSolution
Use a ref to store the current resetId (avoids unnecessary re-renders). Provide both
resetandgetResetIdin context. Update the context type to reflect the new API. Keep the existing publicresetfunction signature for backwards compatibility.Changes
packages/react-query/src/QueryErrorResetBoundary.tsx(modified)packages/react-query/src/useBaseQuery.ts(modified)🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Contributed by Lê Thành Chỉnh
Code is a tool. Mindset is the real value.
Closes #2712
Summary by CodeRabbit