react-router - Add preloadQuery.waitForStaticResult() API for awaiting partial query data in loaders#545
Conversation
👷 Deploy request for apollo-client-nextjs-docmodel pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 3134704 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
|
@mateuszhajdziony is attempting to deploy a commit to the Apollo Client - Next package - integration tests Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new preloadQuery.waitForStaticResult API (with predicate overloads) to the streaming client, integrates it into React Router loaders, updates types/exports, provides a loader example and Playwright test, and includes documentation and a changeset for a minor release. Changes
Sequence DiagramsequenceDiagram
participant Loader as React Router<br/>Loader
participant Preloader as PreloadQuery<br/>Handler
participant Observable as Apollo<br/>Observable
participant Promise as Promise<br/>Resolution
Loader->>Preloader: waitForStaticResult(queryRef, predicate?)
Preloader->>Observable: subscribe(observableByRef.get(queryRef))
Observable->>Preloader: emit({ data: {...}, loading: false? / incremental payloads })
Preloader->>Preloader: evaluate predicate(result)?
alt Predicate matches OR no predicate & non-loading
Preloader->>Promise: resolve(StaticResult)
Promise->>Loader: result returned (for meta())
Preloader->>Observable: unsubscribe / cleanup
else keep waiting until match or completion/error
Observable->>Preloader: emit(nextResult) or complete/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 2
🧹 Nitpick comments (1)
integration-test/react-router/app/routes/preloadQuery.awaitable.tsx (1)
46-49: Consider using nullish coalescing for rating display.The current
||operator would show "loading..." for a rating value of0. While unlikely in practice, using??is more precise for null/undefined checks.♻️ Suggested change
Rating:{" "} <span data-testid={`rating-${id}`}> - {rating?.value || "loading..."} + {rating?.value != null ? `${rating.value}/5` : "loading..."} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-test/react-router/app/routes/preloadQuery.awaitable.tsx` around lines 46 - 49, The JSX uses the logical OR operator for rendering the rating ({rating?.value || "loading..."}) which treats 0 as falsy; change it to the nullish coalescing operator so only null/undefined fall back to "loading..." — update the expression referencing rating?.value inside the span with data-testid `rating-${id}` to use ?? instead of ||.
🤖 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-router/README.md`:
- Around line 195-206: The Products example references an undefined product
variable; use the data returned from useReadQuery(queryRef) instead. Update the
Products component to read the product from the useReadQuery result (e.g.,
replace product.title and product.rating with the corresponding properties on
data, such as data.product or data?.product depending on the shape) so that all
usages reference the defined symbol returned by useReadQuery(queryRef) rather
than the nonexistent product variable.
- Around line 208-209: The note block marker `> [!NOTE]` is missing a separating
newline so the content isn't rendered as a block note; update the markdown
around the `> [!NOTE]` marker so there is a blank line (or use a new line
containing just `>`) after the marker and ensure the following line(s) starting
with the content about `resolveWhen` are prefixed as blockquote lines (e.g.,
start with `>`), so the note renders correctly.
---
Nitpick comments:
In `@integration-test/react-router/app/routes/preloadQuery.awaitable.tsx`:
- Around line 46-49: The JSX uses the logical OR operator for rendering the
rating ({rating?.value || "loading..."}) which treats 0 as falsy; change it to
the nullish coalescing operator so only null/undefined fall back to "loading..."
— update the expression referencing rating?.value inside the span with
data-testid `rating-${id}` to use ?? instead of ||.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cac3322f-2f22-4762-9f2e-c5cfcd751611
📒 Files selected for processing (7)
.changeset/add-preload-query-awaitable.mdintegration-test/playwright/src/preloadQuery.test.tsintegration-test/react-router/app/routes/preloadQuery.awaitable.tsxpackages/client-react-streaming/src/index.shared.tspackages/client-react-streaming/src/transportedQueryRef.tspackages/react-router/README.mdpackages/react-router/src/preloader.tsx
📜 Review details
🔇 Additional comments (9)
packages/client-react-streaming/src/index.shared.ts (1)
8-13: LGTM!The new
AwaitablePreloadResulttype is correctly re-exported alongside the related preloader types, making it available for consumers of the public API.integration-test/playwright/src/preloadQuery.test.ts (1)
180-205: LGTM!The test correctly validates the
preloadQuery.awaitable()behavior:
- Document title is set immediately from data resolved via
resolveWhen(before deferred content arrives)- Rating element shows "loading..." initially, then updates to "5/5" after the
@deferfragment streams inThe 5s timeout appropriately accounts for the 1000ms
delayDeferredplus network variability..changeset/add-preload-query-awaitable.md (1)
1-8: LGTM!The changeset correctly documents the new API with a minor version bump for both affected packages. The description clearly explains the use case and behavior.
packages/react-router/src/preloader.tsx (2)
27-42: LGTM!The
AwaitablePreloadQueryResulttype is well-documented and correctly mirrors the streaming library'sAwaitablePreloadResultstructure while using React Router'sunstable_SerializesTowrapper for the queryRef.
88-101: LGTM!The implementation correctly wraps
preloader.awaitable()to convert the stream to a promiscade for turbo-stream compatibility while passingresolveWhenthrough unchanged. This maintains consistency with the existingpreloadQuerybehavior.integration-test/react-router/app/routes/preloadQuery.awaitable.tsx (1)
8-23: LGTM!The loader correctly demonstrates the
preloadQuery.awaitable()pattern:
- Destructures
queryRefandresolveWhenfrom the awaitable call- Awaits only the critical data needed for
meta()via the predicate- Returns both the streaming
queryRefand the extracted titleThis allows the document title to be set immediately while deferred ratings stream in later.
packages/client-react-streaming/src/transportedQueryRef.ts (3)
89-109: LGTM!The type definitions for
awaitableandAwaitablePreloadResultare well-documented with clear JSDoc explaining the behavior, including the rejection conditions. TheprepareForReuselimitation is correctly documented.Also applies to: 116-131
166-206: LGTM!Good refactoring to extract
prepareQueryas a shared helper. This reduces duplication betweenpreloadandpreload.awaitablewhile keeping the logic for creating the transported query ref and watch query options in one place.
237-293: LGTM!The
awaitableimplementation is well-structured:
- Guard clause (lines 243-247): Correctly throws for unsupported
prepareForReusemode- Observable creation (line 254): Creates once, allowing multiple
resolveWhencalls with different predicates- Subscription lifecycle (lines 260-285): Properly unsubscribes on resolve, reject, or error
- Predicate logic (lines 263-276): Correctly handles:
- Match found → resolve with data
- Query completes without match (
!result.loading) → reject- Still loading → wait for next incremental result
- Predicate throws → reject
The implementation works naturally with
@defersince each incremental result triggers the predicate check.
|
omg we absolutely NEED this feature merged! |
| const { queryRef, resolveWhen } = preloadQuery.awaitable(MY_QUERY); | ||
|
|
||
| // Await only the product title — `rating` will stream in later via @defer | ||
| const data = await resolveWhen( | ||
| (data) => data?.product?.title !== null | ||
| ); |
There was a problem hiding this comment.
Hi, just looking at this now :)
We already have preloadQuery.toPromise(queryRef) - could we do this in a similar shape?
| const { queryRef, resolveWhen } = preloadQuery.awaitable(MY_QUERY); | |
| // Await only the product title — `rating` will stream in later via @defer | |
| const data = await resolveWhen( | |
| (data) => data?.product?.title !== null | |
| ); | |
| const queryRef = preloadQuery(MY_QUERY); | |
| // Await only the product title — `rating` will stream in later via @defer | |
| const result = await preloadQuery.waitForStaticResult( | |
| queryRef, | |
| (result) => result.data?.product?.title !== null | |
| ); |
signatures would be something like
// returns a promise that resolves to the final result
waitForStaticResult<TData>(queryRef): Promise<Result<TData>>
// returns a promise that could resolve sooner if the predicate is satisfied
// throws if the predicate is not satisfied when the query is fully resolved
waitForStaticResult<TData, TResult extends Result<TData>>(queryRef: PreloadedQuery<T>, predicate: (result: Result<TData>) => result is TResult): Promise<TResult>;There was a problem hiding this comment.
Ah got it, so you'd like this to be a static method callable separately with the queryRef. Can do probably, but I'd have to investigate if this method, being separate, would still have access to the original stream. I'll try to prepare another proposal today evening or tomorrow if its possible. But otherwise do you agree with this API and is it "mergeable"?
There was a problem hiding this comment.
If we can get it to the flow I laid out, I think we could get it in, yes. The static in the name will hopefully make it clear that it shouldn't be used for data anyone would consider "changing" during a page render.
There was a problem hiding this comment.
@phryneas please review, the API is now reworked according to your proposal
…query data in loaders
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
LordPikaChu
force-pushed
the
react-router-awaitable
branch
from
8d4cef4 to
a267962
Compare
LordPikaChu
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/client-react-streaming/src/transportedQueryRef.ts`:
- Around line 228-241: The prepareForReuse branch returns a wrapped ref without
registering an observable, but later code (e.g., waitForStaticResult) expects
observableByRef to contain an entry for transportedQueryRef and throws; fix by
ensuring the prepareForReuse path populates observableByRef the same way the
non-reuse path does: after obtaining internalQueryRef via getInternalQueryRef
and wrapQueryRef, create or obtain the underlying observable (e.g., from the
internalQueryRef or by calling client.watchQuery with the same
watchQueryOptions) and set observableByRef.set(transportedQueryRef, observable)
before returning the wrapped ref; alternatively, change waitForStaticResult to
defensively handle missing observableByRef entries, but prefer populating
observableByRef in the prepareForReuse branch (symbols: prepareForReuse,
transportedQueryRef, observableByRef, getInternalQueryRef, wrapQueryRef,
client.watchQuery, waitForStaticResult).
- Around line 270-275: The predicate check is incorrectly gated by `result.data
!= null` so nullable payloads never get evaluated; update the block in
transportedQueryRef where `if (predicate) { if (result.data != null &&
predicate(staticResult)) { ... } else if (!result.loading) { ... } }` to call
`predicate(staticResult)` regardless of `result.data` (i.e., remove the
`result.data != null` guard) so the predicate can pass for null payloads,
keeping the unsubscribe/resolve on success and the unsubscribe/reject when
`!result.loading` unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f9ff39c-0825-4492-a1b9-5ef3f2a806ac
📒 Files selected for processing (7)
.changeset/add-preload-query-wait-for-static-result.mdintegration-test/playwright/src/preloadQuery.test.tsintegration-test/react-router/app/routes/preloadQuery.awaitable.tsxpackages/client-react-streaming/src/index.shared.tspackages/client-react-streaming/src/transportedQueryRef.tspackages/react-router/README.mdpackages/react-router/src/preloader.tsx
✅ Files skipped from review due to trivial changes (3)
- .changeset/add-preload-query-wait-for-static-result.md
- packages/react-router/README.md
- integration-test/playwright/src/preloadQuery.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/client-react-streaming/src/index.shared.ts
- integration-test/react-router/app/routes/preloadQuery.awaitable.tsx
📜 Review details
🔇 Additional comments (2)
packages/react-router/src/preloader.tsx (2)
61-64: Good API extension withclientin loader args.Adding
clienton Line 63 makes loader-side integrations more flexible without changing existingpreloadQuerybehavior.
78-85: Delegation keeps await logic centralized.Line 80–83 forwards directly to the underlying preloader implementation, which is the right way to prevent behavior drift across packages.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/client-react-streaming/src/transportedQueryRef.ts (1)
228-237:⚠️ Potential issue | 🟠 Major
prepareForReuserefs still can't usewaitForStaticResult.The reuse path returns on Line 234 without registering the observable, so every reused ref will hit the
"no observable found"throw on Line 257. Either populateobservableByReffor reused refs too, or fail fast with an explicit unsupported-mode error instead of exposing a broken API path.Also applies to: 255-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/client-react-streaming/src/transportedQueryRef.ts` around lines 228 - 237, The reuse path for prepareForReuse returns a wrapped ref without registering its observable, causing waitForStaticResult to throw; update the prepareForReuse branch in transportedQueryRef creation (the code using getInternalQueryRef, transportedQueryRef, and wrapQueryRef) to register the underlying observable into observableByRef for the returned ref (or alternatively throw a clear unsupported-mode error), and apply the same fix to the analogous reuse logic near the other branch (the code around the other returned/wrapped ref) so reused refs either have observableByRef populated or fail fast with an explicit unsupported-mode message instead of leaving a broken API path.
🤖 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/client-react-streaming/src/transportedQueryRef.ts`:
- Around line 239-245: The subscribe callbacks currently close over the
subscription variable before it's assigned, risking a ReferenceError; fix by
declaring the subscription binding before calling subscribe and ensuring
callbacks reference that pre-declared variable (e.g., let subscription:
ReturnType<typeof observable.subscribe> | null = null; then assign subscription
= observable.subscribe(...); and use subscription?.unsubscribe() in the
next/error handlers). Apply the same change in the waitForStaticResult code path
so both uses of client.watchQuery / observableByRef and transportedQueryRef
avoid closing over an uninitialized subscription.
---
Duplicate comments:
In `@packages/client-react-streaming/src/transportedQueryRef.ts`:
- Around line 228-237: The reuse path for prepareForReuse returns a wrapped ref
without registering its observable, causing waitForStaticResult to throw; update
the prepareForReuse branch in transportedQueryRef creation (the code using
getInternalQueryRef, transportedQueryRef, and wrapQueryRef) to register the
underlying observable into observableByRef for the returned ref (or
alternatively throw a clear unsupported-mode error), and apply the same fix to
the analogous reuse logic near the other branch (the code around the other
returned/wrapped ref) so reused refs either have observableByRef populated or
fail fast with an explicit unsupported-mode message instead of leaving a broken
API path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c38b96e-72b9-4f26-acfc-40a04785342c
📒 Files selected for processing (1)
packages/client-react-streaming/src/transportedQueryRef.ts
📜 Review details
🔇 Additional comments (1)
packages/client-react-streaming/src/transportedQueryRef.ts (1)
165-216: NiceprepareQueryextraction.Centralizing the option sanitization and
watchQueryOptionsconstruction makes the preload flow much easier to audit.
4 participants
Add
preloadQuery.awaitable()API for selectively awaiting partial query data in loaders.This new method returns
{ queryRef, resolveWhen }— thequeryRefworks identically to the standardpreloadQuery()for streaming, whileresolveWhen(predicate)returns a promise that resolves withdataas soon as the predicate returnstrueagainst an incoming result. This enables use cases like populating React Router'smeta()function with query data without blocking the full streaming response, and works naturally with@deferfor incremental delivery.This solves the issue described in #332, while preserving both the original intent of library authors and streaming capabilities
Summary by CodeRabbit
New Features
Documentation
Tests