ref: switch from throwing promises to React.use#10366
Open
Conversation
|
View your CI Pipeline Execution ↗ for commit a5fec0f
☁️ Nx Cloud last updated this comment at |
4 tasks
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
size-limit report 📦
|
Contributor
📝 WalkthroughWalkthroughAdds suspense orchestration: exports a pending thenable, introduces a React-compatible Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useBaseQuery
participant SuspenseModule as use/getSuspensePromise
participant QueryObserver
participant WeakMapCache as WeakMapCache
Component->>useBaseQuery: calls hook (shouldSuspend=true)
useBaseQuery->>SuspenseModule: request thenable via getSuspensePromise(options, observer, errorResetBoundary)
SuspenseModule->>WeakMapCache: lookup by observer & queryHash
alt cached
WeakMapCache-->>SuspenseModule: return cached thenable
else not cached
SuspenseModule->>QueryObserver: call fetchOptimistic(...)
QueryObserver-->>SuspenseModule: returns promise
SuspenseModule->>WeakMapCache: store {queryHash, promise}
end
SuspenseModule-->>useBaseQuery: returns thenable to React.use / fallback
Note right of Component: React Suspense boundary awaits thenable
QueryObserver->>QueryObserver: observer.updateResult() (in finally)
QueryObserver-->>Component: data available -> render resumes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-query/src/__tests__/useSuspenseQueries.test.tsx (1)
696-710: Consider simplifying the microtask flush pattern.The
await Promise.resolve()insideactis used to flush microtasks after advancing timers. While this works, it's an unusual pattern that may confuse future maintainers.This may be necessary due to the interaction between fake timers, suspense, and
placeholderData, so keeping it is acceptable if tests fail without it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/__tests__/useSuspenseQueries.test.tsx` around lines 696 - 710, The test uses an inline microtask flush via awaiting Promise.resolve() inside act after vi.advanceTimersByTimeAsync, which is nonstandard and confusing; replace those inline awaits with a clear utility (e.g., a shared flushPromises/flushMicrotasks helper) or use the test runner's explicit microtask helper so intent is clear. Update the two occurrences inside the act blocks that follow vi.advanceTimersByTimeAsync (around the act/vi.advanceTimersByTimeAsync/await Promise.resolve sequence) so they call the chosen helper instead, keeping the rest of the sequence (act, vi.advanceTimersByTimeAsync, then flush helper) identical and ensuring fireEvent.click and subsequent assertions still run inside act as before.
🤖 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/suspense.ts`:
- Around line 135-153: The cache suspenseObserverPromiseCache stores promises
keyed by observer + defaultedOptions.queryHash but never clears them, causing
reuse of already-settled promises; update the logic around where you create and
set promise (the fetchOptimistic call and suspenseObserverPromiseCache.set) to
attach finally handlers that remove the cache entry for that observer when the
promise settles (both resolve and reject) so future suspensions for the same
observer+queryHash will re-fetch; apply the same change to the other similar
block that uses suspenseObserverPromiseCache (the second occurrence handling
observers).
In `@packages/react-query/src/useQueries.ts`:
- Around line 349-385: The cached combined thenable in
suspenseQueriesPromiseCache can remain fulfilled and should be invalidated once
the Promise.all(...) settles; update getCombinedSuspensePromise so that after
creating the pendingThenable() and wiring
Promise.all(suspensePromises).then(promise.resolve, promise.reject) you also
attach a cleanup step (e.g., a .then or .finally) that removes the cache entry
for this QueriesObserver (suspenseQueriesPromiseCache.delete(observer)) if the
stored promise is the same one you created, ensuring future suspension cycles
recreate a fresh thenable; keep early return for single suspense promise
unchanged and use the existing identifiers suspenseQueriesPromiseCache,
getCombinedSuspensePromise, pendingThenable, and QueriesObserver to locate the
change.
---
Nitpick comments:
In `@packages/react-query/src/__tests__/useSuspenseQueries.test.tsx`:
- Around line 696-710: The test uses an inline microtask flush via awaiting
Promise.resolve() inside act after vi.advanceTimersByTimeAsync, which is
nonstandard and confusing; replace those inline awaits with a clear utility
(e.g., a shared flushPromises/flushMicrotasks helper) or use the test runner's
explicit microtask helper so intent is clear. Update the two occurrences inside
the act blocks that follow vi.advanceTimersByTimeAsync (around the
act/vi.advanceTimersByTimeAsync/await Promise.resolve sequence) so they call the
chosen helper instead, keeping the rest of the sequence (act,
vi.advanceTimersByTimeAsync, then flush helper) identical and ensuring
fireEvent.click and subsequent assertions still run inside act as before.
🪄 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: be132759-0ec4-4901-a690-8e2a45b28e29
📒 Files selected for processing (12)
packages/query-core/src/index.tspackages/react-query/src/__tests__/QueryResetErrorBoundary.test.tsxpackages/react-query/src/__tests__/suspense.test.tsxpackages/react-query/src/__tests__/usePrefetchInfiniteQuery.test.tsxpackages/react-query/src/__tests__/usePrefetchQuery.test.tsxpackages/react-query/src/__tests__/useSuspenseInfiniteQuery.test.tsxpackages/react-query/src/__tests__/useSuspenseQueries.test.tsxpackages/react-query/src/__tests__/useSuspenseQuery.test.tsxpackages/react-query/src/__tests__/utils.tsxpackages/react-query/src/suspense.tspackages/react-query/src/useBaseQuery.tspackages/react-query/src/useQueries.ts
Comment on lines
+135
to
+153
| const queryHash = defaultedOptions.queryHash | ||
| const cached = suspenseObserverPromiseCache.get(observer) | ||
|
|
||
| if (cached?.queryHash === queryHash) { | ||
| return cached.promise as Promise<QueryObserverResult<TData, TError>> | ||
| } | ||
|
|
||
| const promise = fetchOptimistic( | ||
| defaultedOptions, | ||
| observer, | ||
| errorResetBoundary, | ||
| ) | ||
|
|
||
| suspenseObserverPromiseCache.set(observer, { | ||
| queryHash, | ||
| promise, | ||
| }) | ||
|
|
||
| return promise |
Contributor
There was a problem hiding this comment.
Expire cached suspense promises after they settle.
This cache only keys on queryHash and never clears. If the same observer suspends again for the same key—e.g. after resetQueries, an error-boundary retry, or another same-key cycle that goes back to isPending—use() will reuse a fulfilled promise and stop suspending, so the hook can leak a pending result instead of the fallback.
🛠️ Minimal fix
const promise = fetchOptimistic(
defaultedOptions,
observer,
errorResetBoundary,
)
suspenseObserverPromiseCache.set(observer, {
queryHash,
promise,
})
+
+ void promise.finally(() => {
+ const cached = suspenseObserverPromiseCache.get(observer)
+ if (cached?.promise === promise) {
+ suspenseObserverPromiseCache.delete(observer)
+ }
+ })
return promise
}Also applies to: 156-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/suspense.ts` around lines 135 - 153, The cache
suspenseObserverPromiseCache stores promises keyed by observer +
defaultedOptions.queryHash but never clears them, causing reuse of
already-settled promises; update the logic around where you create and set
promise (the fetchOptimistic call and suspenseObserverPromiseCache.set) to
attach finally handlers that remove the cache entry for that observer when the
promise settles (both resolve and reject) so future suspensions for the same
observer+queryHash will re-fetch; apply the same change to the other similar
block that uses suspenseObserverPromiseCache (the second occurrence handling
observers).
Comment on lines
+349
to
+385
| const suspenseQueriesPromiseCache = new WeakMap< | ||
| QueriesObserver<any>, | ||
| { | ||
| queryHashes: Array<string> | ||
| promise: Promise<Array<unknown>> | ||
| } | ||
| >() | ||
|
|
||
| function getCombinedSuspensePromise( | ||
| observer: QueriesObserver<any>, | ||
| queryHashes: Array<string>, | ||
| suspensePromises: Array<Promise<unknown>>, | ||
| ) { | ||
| if (suspensePromises.length === 1) { | ||
| return suspensePromises[0]! | ||
| } | ||
|
|
||
| const cached = suspenseQueriesPromiseCache.get(observer) | ||
|
|
||
| if ( | ||
| cached && | ||
| cached.queryHashes.length === queryHashes.length && | ||
| cached.queryHashes.every((hash, index) => hash === queryHashes[index]) | ||
| ) { | ||
| return cached.promise | ||
| } | ||
|
|
||
| const promise = pendingThenable<Array<unknown>>() | ||
|
|
||
| Promise.all(suspensePromises).then(promise.resolve, promise.reject) | ||
|
|
||
| suspenseQueriesPromiseCache.set(observer, { | ||
| queryHashes, | ||
| promise, | ||
| }) | ||
|
|
||
| return promise |
Contributor
There was a problem hiding this comment.
Invalidate the combined thenable once a suspense cycle finishes.
This cache is sticky across cycles. After the first Promise.all(...) settles, a later same-hash suspension on the same QueriesObserver will reuse a fulfilled thenable, so use() won’t suspend anymore and useQueries can leak pending results on reset/retry flows.
🛠️ Minimal fix
const promise = pendingThenable<Array<unknown>>()
- Promise.all(suspensePromises).then(promise.resolve, promise.reject)
+ Promise.all(suspensePromises).then(
+ (value) => {
+ const cached = suspenseQueriesPromiseCache.get(observer)
+ if (cached?.promise === promise) {
+ suspenseQueriesPromiseCache.delete(observer)
+ }
+ promise.resolve(value)
+ },
+ (error) => {
+ const cached = suspenseQueriesPromiseCache.get(observer)
+ if (cached?.promise === promise) {
+ suspenseQueriesPromiseCache.delete(observer)
+ }
+ promise.reject(error)
+ },
+ )
suspenseQueriesPromiseCache.set(observer, {
queryHashes,
promise,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-query/src/useQueries.ts` around lines 349 - 385, The cached
combined thenable in suspenseQueriesPromiseCache can remain fulfilled and should
be invalidated once the Promise.all(...) settles; update
getCombinedSuspensePromise so that after creating the pendingThenable() and
wiring Promise.all(suspensePromises).then(promise.resolve, promise.reject) you
also attach a cleanup step (e.g., a .then or .finally) that removes the cache
entry for this QueriesObserver (suspenseQueriesPromiseCache.delete(observer)) if
the stored promise is the same one you created, ensuring future suspension
cycles recreate a fresh thenable; keep early return for single suspense promise
unchanged and use the existing identifiers suspenseQueriesPromiseCache,
getCombinedSuspensePromise, pendingThenable, and QueriesObserver to locate the
change.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-query/src/suspense.ts (1)
144-159:⚠️ Potential issue | 🔴 CriticalExpire cached suspense promises after settlement.
Line 144 returns cached promises solely by
observer+queryHash, but Line 154 only sets entries and never clears them. A settled promise can be reused in a later pending cycle, skipping intended suspension/fetch behavior.Minimal fix
const promise = fetchOptimistic( defaultedOptions, observer, errorResetBoundary, ) suspenseObserverPromiseCache.set(observer, { queryHash, promise, }) + + void promise.finally(() => { + const cached = suspenseObserverPromiseCache.get(observer) + if (cached?.promise === promise) { + suspenseObserverPromiseCache.delete(observer) + } + }) return promise }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/suspense.ts` around lines 144 - 159, The cached suspense promise stored in suspenseObserverPromiseCache for an observer/queryHash pair is never cleared, so settled promises can be reused incorrectly; update the logic where you set the cache (in the code that creates promise via fetchOptimistic and calls suspenseObserverPromiseCache.set(observer, { queryHash, promise })) to attach a finally handler to the promise that removes the cache entry (only if the stored entry still matches the same queryHash) so the cache only holds in-flight promises and is cleared after resolution or rejection (reference: suspenseObserverPromiseCache, observer, queryHash, and the promise returned from fetchOptimistic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react-query/src/suspense.ts`:
- Around line 144-159: The cached suspense promise stored in
suspenseObserverPromiseCache for an observer/queryHash pair is never cleared, so
settled promises can be reused incorrectly; update the logic where you set the
cache (in the code that creates promise via fetchOptimistic and calls
suspenseObserverPromiseCache.set(observer, { queryHash, promise })) to attach a
finally handler to the promise that removes the cache entry (only if the stored
entry still matches the same queryHash) so the cache only holds in-flight
promises and is cleared after resolution or rejection (reference:
suspenseObserverPromiseCache, observer, queryHash, and the promise returned from
fetchOptimistic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f7bb92c-d9bc-407c-b885-8448f4c43feb
📒 Files selected for processing (1)
packages/react-query/src/suspense.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-query/src/__tests__/ssr.test.tsx (1)
242-266: Strengthen the assertion to verify the “no undefined data” contractThe title says undefined data must not leak, but the test currently only checks for
"loading". Add a negative assertion for"undefined"so the intent is explicitly enforced.Suggested patch
expect(markup).toContain('loading') + expect(markup).not.toContain('undefined') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/__tests__/ssr.test.tsx` around lines 242 - 266, The test titled "useSuspenseQuery should suspend on the server instead of exposing undefined data" currently only asserts that the fallback "loading" is present; add a negative assertion to ensure undefined data doesn't leak by asserting that the rendered markup does not contain the string "undefined" (e.g., expect(markup).not.toContain('undefined')) after renderToString in the same test that uses useSuspenseQuery, Page, queryFn and markup so the intent is explicitly enforced.packages/react-query/src/__tests__/ssr-hydration.test.tsx (1)
311-329: Make hydration assertions explicit for both fallback and undefined leakage
toContain('1')is a good signal, but this test’s contract is stronger. Add explicit negative checks for"loading"and"undefined"on both server markup and hydrated DOM.Suggested patch
expect(markup).toContain('1') + expect(markup).not.toContain('loading') + expect(markup).not.toContain('undefined') @@ expect(consoleMock).toHaveBeenCalledTimes(0) expect(el.innerHTML).toContain('1') + expect(el.innerHTML).not.toContain('loading') + expect(el.innerHTML).not.toContain('undefined')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/__tests__/ssr-hydration.test.tsx` around lines 311 - 329, The test currently only asserts that the server markup and hydrated DOM contain '1' but doesn't assert that the fallback ("loading") or stringified "undefined" leaked; update the assertions around markup and el.innerHTML (after creating markup/stringifiedState and after hydrate + ReactHydrate with QueryClient and ProfilesComponent) to explicitly assert that neither 'loading' nor 'undefined' appear (e.g., expect(markup).not.toContain('loading') and .not.toContain('undefined'), and expect(el.innerHTML).not.toContain('loading') and .not.toContain('undefined')), keeping the existing consoleMock and '1' assertions intact so the test verifies no fallback/undefined leakage during server render or hydration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-query/src/__tests__/ssr-hydration.test.tsx`:
- Around line 311-329: The test currently only asserts that the server markup
and hydrated DOM contain '1' but doesn't assert that the fallback ("loading") or
stringified "undefined" leaked; update the assertions around markup and
el.innerHTML (after creating markup/stringifiedState and after hydrate +
ReactHydrate with QueryClient and ProfilesComponent) to explicitly assert that
neither 'loading' nor 'undefined' appear (e.g.,
expect(markup).not.toContain('loading') and .not.toContain('undefined'), and
expect(el.innerHTML).not.toContain('loading') and .not.toContain('undefined')),
keeping the existing consoleMock and '1' assertions intact so the test verifies
no fallback/undefined leakage during server render or hydration.
In `@packages/react-query/src/__tests__/ssr.test.tsx`:
- Around line 242-266: The test titled "useSuspenseQuery should suspend on the
server instead of exposing undefined data" currently only asserts that the
fallback "loading" is present; add a negative assertion to ensure undefined data
doesn't leak by asserting that the rendered markup does not contain the string
"undefined" (e.g., expect(markup).not.toContain('undefined')) after
renderToString in the same test that uses useSuspenseQuery, Page, queryFn and
markup so the intent is explicitly enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8205bbb8-f670-4446-b0ae-1368d44600a6
📒 Files selected for processing (2)
packages/react-query/src/__tests__/ssr-hydration.test.tsxpackages/react-query/src/__tests__/ssr.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integrations/react-start/src/components/default-catch-boundary.tsx (1)
38-49: Minor: "Go Back" link has mismatchedtoprop and actual behavior.The link has
to="/"but theonClickhandler prevents default navigation and callswindow.history.back(). This could cause confusion if inspecting the component and may not work correctly with JS disabled. Consider using a<button>element instead since this doesn't perform standard link navigation.♻️ Suggested refactor to use button for history.back()
) : ( - <Link - to="/" - className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} - onClick={(e) => { - e.preventDefault() - window.history.back() - }} - > + <button + onClick={() => window.history.back()} + className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} + > Go Back - </Link> + </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integrations/react-start/src/components/default-catch-boundary.tsx` around lines 38 - 49, The "Go Back" control uses a Link with a conflicting to="/" prop while its onClick prevents navigation and calls window.history.back(), which is misleading and problematic; replace the Link component with a semantic <button> (or the framework's Button component) in the default-catch-boundary so it no longer supplies a to prop, remove the preventDefault logic, keep the existing className for styling, ensure the onClick calls window.history.back() (or history.back()) and add an accessible label/aria-label if needed; update any references to Link in this block and confirm the handler is defined inline on the button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@integrations/react-start/src/components/default-catch-boundary.tsx`:
- Around line 38-49: The "Go Back" control uses a Link with a conflicting to="/"
prop while its onClick prevents navigation and calls window.history.back(),
which is misleading and problematic; replace the Link component with a semantic
<button> (or the framework's Button component) in the default-catch-boundary so
it no longer supplies a to prop, remove the preventDefault logic, keep the
existing className for styling, ensure the onClick calls window.history.back()
(or history.back()) and add an accessible label/aria-label if needed; update any
references to Link in this block and confirm the handler is defined inline on
the button.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9e9158a-3f6e-4c1a-9c94-bda392ddb982
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
integrations/react-start/.gitignoreintegrations/react-start/package.jsonintegrations/react-start/src/components/default-catch-boundary.tsxintegrations/react-start/src/components/not-found.tsxintegrations/react-start/src/router.tsxintegrations/react-start/src/routes/__root.tsxintegrations/react-start/src/routes/index.tsxintegrations/react-start/tsconfig.jsonintegrations/react-start/vite.config.ts
✅ Files skipped from review due to trivial changes (3)
- integrations/react-start/.gitignore
- integrations/react-start/tsconfig.json
- integrations/react-start/package.json
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Tests