Add RSCRoute ssr={false} to skip initial RSC payload generation#3318
Add RSCRoute ssr={false} to skip initial RSC payload generation#3318ihabadham wants to merge 5 commits into
RSCRoute ssr={false} to skip initial RSC payload generation#3318Conversation
|
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:
WalkthroughAdds an optional ChangesRSCRoute ssr prop implementation
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant Hydrate as hydrateRoot
participant Server as React Server
participant Stream as Streaming Handler
participant Provider as RSC Provider
rect rgba(200, 100, 100, 0.5)
Note over Client,Provider: SSR Phase - ssr={false}
Server->>Server: RSCRoute render
Server->>Server: Check ssr=false & window undefined
Server->>Stream: throw RSCRouteSSRFalseBailoutError
Stream->>Stream: Detect bailout by digest
Stream->>Client: Emit Suspense fallback HTML + digest
end
rect rgba(100, 150, 200, 0.5)
Note over Client,Provider: Hydration Phase
Client->>Hydrate: hydrateRoot with onRecoverableError
Hydrate->>Hydrate: Bailout appears in recoverable errors
Hydrate->>Hydrate: isRSCRouteSSRFalseBailoutError check
Hydrate->>Hydrate: Skip error reporting (filtered)
end
rect rgba(100, 200, 100, 0.5)
Note over Client,Provider: Client Retry Phase - After Mount/Suspense
Client->>Provider: Suspense triggers retry
Provider->>Provider: getServerComponent(componentName)
Provider->>Client: HTTP /rsc_payload/componentName
Client->>Client: Render received payload and remove fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
ihabadham
changed the title
ihabadham
changed the title
RSCRoute ssr={false} to skip initial RSC payload generation
size-limit report 📦
|
Code ReviewOverviewThis PR implements Phase 2 of the RSCRoute ssr=false feature, upgrading from the Phase 1 mount-gated approach to a proper React Suspense retry path via a classified server-side bailout error. |
Code ReviewOverviewThis PR implements Phase 2 of the RSCRoute ssr=false feature, upgrading from Phase 1 (mount-gated approach) to a proper React Suspense retry path via a classified server-side bailout error. RSCRouteSSRFalseBailoutError is thrown before any provider-dependent work, React Suspense streams the nearest fallback, and the client retries through the existing RSCProvider path unchanged. What works well
IssuesDocumentation: the Suspense for loading states example now implies ssr=false is required for loading states The section was originally about showing a loading indicator during client-side navigation using a default ssr=true route. The updated example adds ssr=false to SlowServerComponent, conflating two concerns: (1) client-side navigation loading state - wrap any RSCRoute in Suspense, no ssr=false needed; and (2) deferred SSR - ssr=false specifically to skip initial payload generation. A developer reading this section to add a loading spinner during navigation will copy the example and accidentally opt out of SSR. See inline comment. Minor: redundant cause branch in onRecoverableError In wrapServerComponentRenderer/client.tsx, when error is not an Error instance, cause is assigned error itself, so the second isRSCRouteSSRFalseBailoutError(cause) check is a no-op duplicate of the first. Harmless since React always passes an Error to onRecoverableError. See inline comment. SummaryThe core implementation is solid and the test suite is comprehensive. Two items: (1) a documentation issue that could lead users to accidentally apply ssr=false when they only want a client-navigation loading indicator; (2) a minor redundant code path. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return ( | ||
| <Suspense fallback={<div>Loading…</div>}> | ||
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} /> | ||
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} ssr={false} /> |
There was a problem hiding this comment.
This example now ties ssr={false} to the general "Suspense for loading states" use case, but the two are independent:
- Loading state during client navigation works with any
RSCRoute, regardless ofssr. A Suspense boundary around<RSCRoute componentName="SlowServerComponent" ... />(nossr={false}) already shows the fallback during client navigation. ssr={false}is specifically for deferring the initial SSR payload. Using it here causesSlowServerComponentto skip SSR entirely, which may not be what a reader wants.
Suggestion: restore the example without ssr={false} so the client-navigation loading case is shown in isolation, or add a comment in the snippet that makes it explicit this is a deferred-SSR example rather than a general loading-state pattern:
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} ssr={false} /> | |
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} /> |
| const cause = | ||
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : error; | ||
|
|
||
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
There was a problem hiding this comment.
When error instanceof Error is false, cause is set to error itself, so isRSCRouteSSRFalseBailoutError(cause) on line 98 is identical to isRSCRouteSSRFalseBailoutError(error) — a no-op duplicate.
In practice this branch is unreachable because React always passes an Error to onRecoverableError, but the else-path is still dead weight. If you want to guard against a future change, the cleaner form is:
| const cause = | |
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : error; | |
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { | |
| const cause = error instanceof Error && error.cause !== undefined ? error.cause : undefined; | |
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
This makes it clear that cause is only checked when it is genuinely distinct from error, and avoids the redundant second check when cause would just alias error.
Greptile SummaryThis PR implements Phase 2 of the
Confidence Score: 3/5The core implementation is solid, but the new E2E test will fail at runtime due to a non-existent Playwright API call. The streaming SSR bailout, digest propagation, notifySSREndOnce guard, and onRecoverableError suppression are all correctly wired. The single defect is in the E2E test: page.requests() is not a method on Playwright's Page object, so that assertion block will throw a TypeError and the test will never pass. react_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts — the page.requests() call on lines 1137-1145 needs to be replaced with a page.on('request', ...) listener registered before navigation. Important Files Changed
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/planning/3101-rsc-route-ssr-false.md`:
- Around line 264-266: Phase 2's design that depends on React's
onRecoverableError.errorInfo.digest must be updated because React 19 removed
digest; change the plan to identify the intentional bailout via an alternate
channel: either (A) emit a distinct, well-known bailout error type/message from
the server (e.g., Error subclass or unique message prefix) and have the client
match that pattern in onRecoverableError, or (B) include the bailout identifier
in server response metadata/headers and surface that to the client so it can
suppress only that bailout; update the Phase 2 text around onRecoverableError,
the "client suppresses only recoverable errors carrying that digest" wording,
and any downstream sections referencing digest to describe the chosen
alternative mechanism (refer to Phase 2, onRecoverableError, and the
"intentional bailout" handling).
🪄 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: bbac9882-3b75-4697-b6ca-62432b22628e
📒 Files selected for processing (22)
docs/pro/react-server-components/inside-client-components.mdinternal/planning/3101-rsc-route-ssr-false.mdpackages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/src/ClientSideRenderer.tspackages/react-on-rails-pro/src/defaultRSCProviderRegistry.tspackages/react-on-rails-pro/src/handleRecoverableError.client.tspackages/react-on-rails-pro/src/registerDefaultRSCProvider.client.tsxpackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/tests/ClientSideRenderer.test.tspackages/react-on-rails-pro/tests/deferredRouteSsr.test.tsxpackages/react-on-rails/src/reactApis.ctspackages/react-on-rails/src/reactHydrateOrRender.tsreact_on_rails/lib/react_on_rails/packs_generator.rbreact_on_rails/spec/dummy/spec/packs_generator_spec.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/app/views/pages/unwrapped_rsc_route_client_render.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/unwrapped_rsc_route_stream_render.html.erbreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyUnwrappedRSCRouteChild.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/UnwrappedRSCRouteDemo.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/UnwrappedStreamRSCRouteDemo.tsxreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts
✅ Files skipped from review due to trivial changes (5)
- react_on_rails_pro/spec/dummy/app/views/pages/unwrapped_rsc_route_stream_render.html.erb
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/UnwrappedStreamRSCRouteDemo.tsx
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/UnwrappedRSCRouteDemo.tsx
- react_on_rails_pro/spec/dummy/config/routes.rb
- react_on_rails_pro/spec/dummy/app/views/pages/unwrapped_rsc_route_client_render.html.erb
|
Code review for RSCRoute ssr=false. See inline comments for specific issues. Overall: the implementation is correct. The classified-error approach for intentional server bailouts is the right pattern for React streaming SSR. Lazy-import of the RSC fetch runtime is a good performance choice. Error boundary retry behavior is preserved. Key items before merge: (1) consider a DEV-mode warning when ssr=false fires without a nearby user-placed Suspense boundary to prevent silent empty-content issues; (2) simplify getRecoverableErrorCause in handleRecoverableError.client.ts by dropping the redundant 'cause' in error check since Error.prototype.cause is typed in ES2022; (3) consider a test for the no-Suspense edge case to document the failure mode explicitly. |
| const handleRecoverableError = (error: unknown) => { | ||
| const cause = getRecoverableErrorCause(error); | ||
|
|
||
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
There was a problem hiding this comment.
The 'cause' in error check and subsequent cast are unnecessary — Error.prototype.cause is standard ES2022 and already typed on Error in modern TypeScript targets.
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { | |
| const getRecoverableErrorCause = (error: unknown): unknown => | |
| error instanceof Error ? error.cause : undefined; |
|
|
||
| import type { ReactElement } from 'react'; | ||
| import type { RailsContext } from 'react-on-rails/types'; | ||
|
|
There was a problem hiding this comment.
This module-level singleton has no 'use client' guard and no server-side runtime check. The import chain currently keeps it client-only (via ClientSideRenderer.ts), but if this file is accidentally imported server-side in a future change, the singleton state would be shared across all SSR requests in the process.
Consider adding a comment at the top of the file documenting the client-only invariant, or a runtime guard:
// This module is client-only. Its mutable singleton state must not be shared across SSR requests.
// It is imported exclusively from client-side modules (ClientSideRenderer.ts).
| if (!ssr && typeof window === 'undefined') { | ||
| throw new RSCRouteSSRFalseBailoutError(componentName); |
There was a problem hiding this comment.
Two notes on this bailout guard:
-
Edge-runtime portability:
typeof window === 'undefined'is the established pattern in this codebase and correct for Node/Rails streaming. Worth a comment that some edge runtimes (Deno, Cloudflare Workers) definewindow, so this would behave differently there — good to document before RSC support expands to edge targets. -
Silent failure without a scoped Suspense boundary: When this throw is caught by the root
Suspense fallback={null}(because the user forgot to add a scoped boundary), the section renders empty with no feedback. A DEV-onlyconsole.warnbefore throwing would give developers an actionable message rather than a silent blank area.
| this.name = 'RSCRouteSSRFalseBailoutError'; | ||
| } | ||
| } | ||
|
|
||
| export function isRSCRouteSSRFalseBailoutError(error: unknown): error is RSCRouteSSRFalseBailoutError { | ||
| return ( |
There was a problem hiding this comment.
The digest-based duck-typing path here is the correct React pattern: when React serializes an error across the server/client streaming boundary, the instanceof check will fail (the class prototype is lost), so matching on the digest string is the right fallback. Worth a short comment explaining why both checks exist:
| this.name = 'RSCRouteSSRFalseBailoutError'; | |
| } | |
| } | |
| export function isRSCRouteSSRFalseBailoutError(error: unknown): error is RSCRouteSSRFalseBailoutError { | |
| return ( | |
| export function isRSCRouteSSRFalseBailoutError(error: unknown): error is RSCRouteSSRFalseBailoutError { | |
| // instanceof works on the server and in the browser before React serializes the error. | |
| // The digest check handles the hydration boundary case where React wraps the original error | |
| // and the prototype chain is no longer intact. | |
| return ( | |
| error instanceof RSCRouteSSRFalseBailoutError || | |
| (typeof error === 'object' && | |
| error !== null && | |
| 'digest' in error && | |
| (error as { digest?: unknown }).digest === RSC_ROUTE_SSR_FALSE_BAILOUT_DIGEST) | |
| ); | |
| } |
| if (isRSCRouteSSRFalseBailoutError(error)) { | ||
| return error.digest; | ||
| } | ||
|
|
There was a problem hiding this comment.
Good pattern. Returning error.digest tells React's streaming renderer to embed the digest string in the data-dgst attribute of the Suspense boundary, which the client hydration handler then uses to identify the intentional bailout and suppress it from onRecoverableError. The flow is: server throws → onError returns digest → React encodes digest in HTML → client hydrateRoot calls onRecoverableError with the wrapped error → handleRecoverableError suppresses it.
One small note: for the real-error path, reportError is called but undefined is returned, which is correct — React uses the absence of a return value to mean "no digest, treat this as a real error."
| getServerComponent: async (args) => { | ||
| const { default: getReactServerComponent } = await import('./getReactServerComponent.client.ts'); | ||
| return getReactServerComponent(domNodeId, railsContext)(args); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The if (typeof window !== 'undefined') guard is redundant here since the 'use client' directive at the top already marks this as a client-only module and the file is only imported from client-side code paths. The guard is harmless, but the comment above it is more valuable than the guard itself — explaining why this registration happens at module load time (before any lazy chunk containing RSCRoute is evaluated) would help future maintainers understand the timing constraint.
Also: each call to the factory creates a new createRSCProvider class instance. Since the factory is called once per root mount this is acceptable, but worth noting in case the factory pattern is later reused in a high-frequency context.
Code ReviewOverviewThis PR implements Phases 2 and 3 of the
The design is well-thought-out, the PR description and planning doc are thorough, and the test coverage is solid. Positives
Concerns1. In 2.
3. This is the conventional React check and works correctly here. One edge case worth documenting: if 4. E2E test fixture: opaque cache-key strings In 5. The new optional Minor Nits
SummaryThe implementation is correct and the design decisions are well-justified. The classified bailout + digest approach is the right way to integrate with React's streaming SSR error handling without broadly suppressing recoverable errors. The lazy-import strategy for the RSC fetch runtime is a solid performance optimization. The concerns above are mostly about defensive coding and test maintainability rather than correctness. Approve with minor suggestions. |
| export const setDefaultRSCProviderFactory = (factory: DefaultRSCProviderFactory) => { | ||
| defaultRSCProviderFactory = factory; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Silent overwrite: if this is called a second time (e.g. a generated pack imported twice via barrel re-exports or a chunking edge case), the previous factory is lost with no warning. Since the side-effect import pattern makes double-registration hard to spot, consider guarding in dev:
| export const setDefaultRSCProviderFactory = (factory: DefaultRSCProviderFactory) => { | |
| if (process.env.NODE_ENV !== 'production' && defaultRSCProviderFactory) { | |
| console.warn( | |
| '[react-on-rails-pro] setDefaultRSCProviderFactory called more than once. ' + | |
| 'The previous factory will be replaced. Check for duplicate imports of registerDefaultRSCProvider/client.', | |
| ); | |
| } | |
| defaultRSCProviderFactory = factory; | |
| }; |
| import { isRSCRouteSSRFalseBailoutError } from './RSCRouteSSRFalseBailoutError.ts'; | ||
|
|
||
| const getRecoverableErrorCause = (error: unknown): unknown => | ||
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : undefined; |
There was a problem hiding this comment.
'cause' in error is always true for any Error instance in ES2022+ (where cause is a standard property), so the second condition never filters anything out. The cast to Error & { cause?: unknown } is also redundant since Error.cause is already typed as unknown in the latest TS lib. This can be simplified:
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : undefined; | |
| const getRecoverableErrorCause = (error: unknown): unknown => | |
| error instanceof Error ? error.cause : undefined; |
| onAllReady() { | ||
| streamingTrackers.postSSRHookTracker.notifySSREnd(); | ||
| notifySSREndOnce(); | ||
| }, |
There was a problem hiding this comment.
Returning a non-undefined string from onError is the hook by which React embeds the value into the data-dgst attribute of the <template data-msg> element in the streamed HTML. The client's onRecoverableError receives an error with that same digest, which is how handleRecoverableError can match and suppress it. This is intentional and correct React streaming SSR behavior — worth a brief inline comment since it's non-obvious:
| }, | |
| onError(e) { | |
| const error = convertToError(e); | |
| if (isRSCRouteSSRFalseBailoutError(error)) { | |
| // Returning the digest embeds it in the streamed HTML data-dgst attribute so | |
| // the client onRecoverableError handler can identify and suppress this expected retry. | |
| return error.digest; | |
| } | |
| reportError(error); | |
| return undefined; | |
| }, |
| getServerComponent: async (args) => { | ||
| const { default: getReactServerComponent } = await import('./getReactServerComponent.client.ts'); | ||
| return getReactServerComponent(domNodeId, railsContext)(args); | ||
| }, |
There was a problem hiding this comment.
The dynamic import() here is the intentional tree-shaking boundary — the RSC fetch runtime is only loaded when a deferred route actually initiates a payload fetch, not at root mount. The module system caches the import after the first resolution, so repeated calls to getServerComponent don't re-evaluate the module. This is the right approach, but a short comment would make the intent clear to future readers:
| }, | |
| getServerComponent: async (args) => { | |
| // Lazy import keeps the RSC fetch runtime out of the initial bundle; the | |
| // module cache ensures this import resolves instantly after the first fetch. | |
| const { default: getReactServerComponent } = await import('./getReactServerComponent.client.ts'); | |
| return getReactServerComponent(domNodeId, railsContext)(args); | |
| }, |
| expect(html).not.toContain('Post 1'); | ||
|
|
||
| const consoleMessages: string[] = []; | ||
| page.on('console', (message) => { |
There was a problem hiding this comment.
This duplicates the constant from RSCRouteSSRFalseBailoutError.ts as a string literal. If the digest value is ever changed, this spec will silently keep passing (the not.toContain assertions succeed vacuously and the toContain assertion on line 2136 would fail with an unhelpful message). Importing from source makes renames fail fast:
| page.on('console', (message) => { | |
| import { RSC_ROUTE_SSR_FALSE_BAILOUT_DIGEST } from '../../../packages/react-on-rails-pro/src/RSCRouteSSRFalseBailoutError.ts'; |
(Adjust the relative path as needed for the E2E test's tsconfig or use a package alias if one exists.)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b74724db1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| reactElement, | ||
| shouldHydrate, | ||
| shouldHydrate && wrappedByDefaultRSCProvider | ||
| ? { identifierPrefix: domNodeId, onRecoverableError: handleRecoverableError } |
There was a problem hiding this comment.
Passing identifierPrefix: domNodeId for every hydrated root that gets auto-wrapped by the default RSC provider can create server/client ID divergence on non-stream SSR pages. In this commit, generated client packs now register the default provider broadly, but the non-stream server path (serverRenderReactComponent using renderToString) does not set a matching identifier prefix, so components using useId can hit hydration mismatches and fall back to client rendering when rsc_support_enabled is on and prerendered roots hydrate through this branch.
Useful? React with 👍 / 👎.
ihabadham
force-pushed
the
ihabadham/feature/rsc-route-ssr-false
branch
from
5b74724 to
e77ed62
Compare
Code Review: Add
|
| const rscRequests = (await page.requests()).filter((payloadRequest) => | ||
| payloadRequest.url().includes('/rsc_payload/'), | ||
| ); | ||
| expect( | ||
| rscRequests.some((payloadRequest) => payloadRequest.url().includes('/rsc_payload/SimpleComponent')), | ||
| ).toBe(true); |
There was a problem hiding this comment.
page.requests() is not a Playwright API — this will throw a TypeError at runtime, causing the assertions below to never execute. Collect requests via page.on('request', ...) before navigating instead:
| const rscRequests = (await page.requests()).filter((payloadRequest) => | |
| payloadRequest.url().includes('/rsc_payload/'), | |
| ); | |
| expect( | |
| rscRequests.some((payloadRequest) => payloadRequest.url().includes('/rsc_payload/SimpleComponent')), | |
| ).toBe(true); | |
| const rscRequests: string[] = []; | |
| page.on('request', (req) => { | |
| if (req.url().includes('/rsc_payload/')) rscRequests.push(req.url()); | |
| }); | |
| expect( | |
| rscRequests.some((url) => url.includes('/rsc_payload/SimpleComponent')), | |
| ).toBe(true); | |
| expect( | |
| rscRequests.some((url) => url.includes('/rsc_payload/MyServerComponent')), | |
| ).toBe(false); |
Note: page.on('request', ...) must be set up before page.goto(...) (which it already is in this test since the listener registration just needs to move before goto). Also remove the await — event listeners are synchronous.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/planning/3101-rsc-route-ssr-false.md`:
- Around line 264-265: Replace the unhyphenated phrase "client recoverable
errors" with the hyphenated compound adjective "client-recoverable errors" in
the document text (referencing the sentence that mentions the client
onRecoverableError retry path and the Phase 2 suppression wording) so the phrase
reads clearly as "client-recoverable errors."
🪄 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: 6fc2775c-bdeb-4f23-b986-21fe9c82fe3a
📒 Files selected for processing (12)
docs/pro/react-server-components/inside-client-components.mdinternal/planning/3101-rsc-route-ssr-false.mdpackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/src/RSCRouteSSRFalseBailoutError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/tests/deferredRouteSsr.test.tsxpackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts
💤 Files with no reviewable changes (2)
- react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx
- react_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails-pro/tests/jest.setup.js
| React still reports this documented retry path through client `onRecoverableError`. Phase 2 should not broadly | ||
| suppress client recoverable errors; the intentional-bailout classification belongs on the server stream path. |
There was a problem hiding this comment.
Use hyphenated compound adjective for clarity.
At Line 265, “client recoverable errors” reads better as “client-recoverable errors.”
🧰 Tools
🪛 LanguageTool
[grammar] ~265-~265: Use a hyphen to join words.
Context: ...ase 2 should not broadly suppress client recoverable errors; the intentional-bail...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/planning/3101-rsc-route-ssr-false.md` around lines 264 - 265,
Replace the unhyphenated phrase "client recoverable errors" with the hyphenated
compound adjective "client-recoverable errors" in the document text (referencing
the sentence that mentions the client onRecoverableError retry path and the
Phase 2 suppression wording) so the phrase reads clearly as "client-recoverable
errors."
| * path as a real server-rendering failure. | ||
| */ | ||
| export class RSCRouteSSRFalseBailoutError extends Error { | ||
| digest = RSC_ROUTE_SSR_FALSE_BAILOUT_DIGEST; |
There was a problem hiding this comment.
The digest field is used as an identity token for classification and should not be mutable. Mark it readonly:
| digest = RSC_ROUTE_SSR_FALSE_BAILOUT_DIGEST; | |
| readonly digest = RSC_ROUTE_SSR_FALSE_BAILOUT_DIGEST; |
| const cause = | ||
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : error; | ||
|
|
||
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
There was a problem hiding this comment.
When error is not an Error instance, cause falls back to error itself, making isRSCRouteSSRFalseBailoutError(cause) a duplicate of the isRSCRouteSSRFalseBailoutError(error) check already on the same line. The fallback should be undefined:
| const cause = | |
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : error; | |
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { | |
| const cause = | |
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : undefined; | |
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
| const notifySSREndOnce = () => { | ||
| if (didNotifySSREnd) return; | ||
|
|
||
| didNotifySSREnd = true; | ||
| streamingTrackers.postSSRHookTracker.notifySSREnd(); |
There was a problem hiding this comment.
The once-guard is correct and the test proves it's needed, but the reason onAllReady can fire more than once on the bailout path is non-obvious. A short comment would help future maintainers:
| const notifySSREndOnce = () => { | |
| if (didNotifySSREnd) return; | |
| didNotifySSREnd = true; | |
| streamingTrackers.postSSRHookTracker.notifySSREnd(); | |
| // Guard against onAllReady firing multiple times. React's streaming renderer can | |
| // invoke it an additional time after a Suspense boundary resolves a classified | |
| // bailout (e.g. RSCRoute ssr={false}), resulting in duplicate postSSRHook calls. | |
| const notifySSREndOnce = () => { | |
| if (didNotifySSREnd) return; | |
| didNotifySSREnd = true; | |
| streamingTrackers.postSSRHookTracker.notifySSREnd(); | |
| }; |
Use a classified server bailout so scoped Suspense boundaries can stream fallback HTML without generating an RSC payload for ssr=false routes. Filter that bailout from stream error reporting and suppress only the matching recoverable hydration error by digest, while preserving real render errors. Add package and dummy-app coverage for fallback rendering, payload skipping, mixed route behavior, and client retry through the existing provider path.
ihabadham
force-pushed
the
ihabadham/feature/rsc-route-ssr-false
branch
from
e77ed62 to
cf606f1
Compare
| pipeToTransform(errorHtmlStream); | ||
| }; | ||
|
|
||
| const notifySSREndOnce = () => { |
There was a problem hiding this comment.
The notifySSREndOnce guard is a useful safety measure, but the reason onAllReady could fire more than once isn't documented. React's renderToPipeableStream guarantees onAllReady is called exactly once per render, so this guard is defensive against something in the surrounding machinery (e.g., a Pro-package extension point calling notifySSREnd elsewhere). A brief comment explaining the specific scenario that prompted this guard would help future maintainers understand whether it can ever be removed.
| const notifySSREndOnce = () => { | |
| // Guard against double-notification: with the Suspense bailout path, the Pro-package | |
| // streaming lifecycle may produce conditions where this callback fires more than once. | |
| const notifySSREndOnce = () => { | |
| if (didNotifySSREnd) return; | |
| didNotifySSREnd = true; | |
| streamingTrackers.postSSRHookTracker.notifySSREnd(); | |
| }; |
| const cause = | ||
| error instanceof Error && 'cause' in error ? (error as Error & { cause?: unknown }).cause : error; | ||
|
|
||
| if (isRSCRouteSSRFalseBailoutError(error) || isRSCRouteSSRFalseBailoutError(cause)) { |
There was a problem hiding this comment.
The cause unwrapping is correct for React's current error wrapping behavior (one level of cause), but it may silently fail to suppress the recoverable error if React changes how it wraps errors in a future version. Consider adding a comment noting this assumption, and/or checking whether isRSCRouteSSRFalseBailoutError matching on digest (the duck-typing branch in RSCRouteSSRFalseBailoutError.ts) covers the real-world React wrapping cases seen in testing.
Also worth noting: when neither branch matches, the fallback uses globalThis.reportError before console.error. This is correct modern behaviour, but it's a silent change from the previous default (which was React's own console.error call). Any Pro-level global error monitoring that hooks reportError will now receive these recoverable errors, which may or may not be desired. Worth a mention in the PR changelog/docs.
| }; | ||
|
|
||
| const RSCRoute = ({ componentName, componentProps, ssr = true }: RSCRouteProps): ReactNode => { | ||
| if (!ssr && typeof window === 'undefined') { |
There was a problem hiding this comment.
typeof window === 'undefined' is the right check for the React on Rails Node renderer context. One edge case worth documenting: if this component ever runs in an environment where window is polyfilled/defined on the server (some SSG toolchains or specific test harnesses), the bailout will silently not fire and the component will fall through to RSCRouteContent/useRSC() instead. It won't crash (it matches the ssr={true} path), but the ssr={false} intent is lost without any warning. Given React on Rails Pro's target environment this is unlikely, but a brief comment pinning the assumption would help future readers.
| return ( | ||
| <Suspense fallback={<div>Loading…</div>}> | ||
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} /> | ||
| <RSCRoute componentName="SlowServerComponent" componentProps={{ user }} ssr={false} /> |
There was a problem hiding this comment.
This code example was changed from the original implicit ssr={true} to ssr={false}. The section is titled "Suspense for loading states", and the original prose explained that Suspense is useful during client-side navigation — i.e., even with the default ssr={true} a route can suspend while the client fetches a payload on navigation.
By changing the example to ssr={false} only, the section no longer documents the more common case. Readers of this section may incorrectly conclude that Suspense is only needed with ssr={false}. Consider keeping both: a short ssr={true} (or no prop) example for client-navigation loading, then the ssr={false} example for the deferred initial render case.
Code ReviewOverviewThis PR implements Phase 2 of What works well
Issues to address1. React's 2. The one-level Also: adding the 3. Functionally correct for the React on Rails Node renderer environment. A short comment would guard against confusion if this code is ever tested in an environment with a server-side 4. Documentation: Suspense example now only shows The "Suspense for loading states" section previously documented using Minor notes
SummaryThe core implementation is solid. The four items above are the only things worth addressing before merge: the two missing comments, the changelog note about the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
59-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNotify
postSSRHookTrackeron error HTML exits too.
notifySSREndOnce()is only reached fromonAllReady(). IfreactRenderingResultrejects, oronShellErrorfalls back tohandleError, the stream still completes without the matching SSR-end notification.Suggested fix
const sendErrorHtml = (error: Error) => { const errorHtmlStream = handleError({ e: error, name: componentName, serverSide: true }); pipeToTransform(errorHtmlStream); + notifySSREndOnce(); };Also applies to: 117-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts` around lines 59 - 69, The SSR-end notification is missed when error HTML is sent; update sendErrorHtml (and any places that call handleError fallback such as onShellError and the reactRenderingResult rejection path) to ensure notifySSREndOnce() is invoked when the error stream finishes or errors. Concretely, after creating errorHtmlStream in sendErrorHtml, attach handlers (e.g., 'end'/'close' and 'error') or await the stream completion so that notifySSREndOnce() is called once the error output completes; keep notifySSREndOnce idempotent (it already is) so calling it from onShellError and the rejection catch is safe. Ensure the change references sendErrorHtml, notifySSREndOnce, onShellError and the reactRenderingResult rejection path.
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts (1)
26-28: ⚡ Quick winConsider validating the deferred payload request completes successfully.
The test waits for the request to occur but doesn't verify it completed with a successful status. This could mask fetch failures.
📡 Proposed enhancement to validate request success
- const deferredPayloadRequest = page.waitForRequest(/\/rsc_payload\/SimpleComponent/); + const deferredPayloadRequestPromise = page.waitForRequest(/\/rsc_payload\/SimpleComponent/); await page.goto(MIXED_ROUTE_PATH); - await deferredPayloadRequest; + const deferredPayloadRequest = await deferredPayloadRequestPromise; + const response = await deferredPayloadRequest.response(); + expect(response?.status()).toBe(200);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@react_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts` around lines 26 - 28, The test currently awaits the request captured in deferredPayloadRequest (from page.waitForRequest(/\/rsc_payload\/SimpleComponent/)) but doesn't verify it succeeded; update the test to capture the Request object (deferredPayloadRequest), await its Response (via request.response() or switch to page.waitForResponse), and assert the response is non-null and has a successful status (e.g., status === 200) after navigating to MIXED_ROUTE_PATH so fetch failures are surfaced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts`:
- Around line 59-69: The SSR-end notification is missed when error HTML is sent;
update sendErrorHtml (and any places that call handleError fallback such as
onShellError and the reactRenderingResult rejection path) to ensure
notifySSREndOnce() is invoked when the error stream finishes or errors.
Concretely, after creating errorHtmlStream in sendErrorHtml, attach handlers
(e.g., 'end'/'close' and 'error') or await the stream completion so that
notifySSREndOnce() is called once the error output completes; keep
notifySSREndOnce idempotent (it already is) so calling it from onShellError and
the rejection catch is safe. Ensure the change references sendErrorHtml,
notifySSREndOnce, onShellError and the reactRenderingResult rejection path.
---
Nitpick comments:
In `@react_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts`:
- Around line 26-28: The test currently awaits the request captured in
deferredPayloadRequest (from
page.waitForRequest(/\/rsc_payload\/SimpleComponent/)) but doesn't verify it
succeeded; update the test to capture the Request object
(deferredPayloadRequest), await its Response (via request.response() or switch
to page.waitForResponse), and assert the response is non-null and has a
successful status (e.g., status === 200) after navigating to MIXED_ROUTE_PATH so
fetch failures are surfaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 935170a4-0949-40b6-8ce9-ef3d82084d91
📒 Files selected for processing (12)
docs/pro/react-server-components/inside-client-components.mdinternal/planning/3101-rsc-route-ssr-false.mdpackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/src/RSCRouteSSRFalseBailoutError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/tests/deferredRouteSsr.test.tsxpackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/e2e-tests/rsc_route_ssr_false.spec.ts
1 participant
Summary
Adds
ssr={false}support toRSCRoute.By default,
RSCRoutekeeps the existingssr={true}behavior: the server renders the route, generates its RSC payload, embeds that payload in the HTML, and the client hydrates from the embedded payload.When
ssr={false}, that route instance skips initial server-side RSC payload generation. In streaming SSR, the route throws a classified server-only bailout before provider work. If it is inside a scopedSuspenseboundary, React streams that fallback HTML and retries the route on the client.The client retry uses the existing RSC provider path, including cache lookup,
/rsc_payload/:componentNamefetching,ServerComponentFetchError, anduseRSC().refetchComponent(...)retry behavior.Part of #3101. Follow-up #3394 adds default-provider support for deferred roots that do not manually use
wrapServerComponentRenderer.Behavior
API
ssrdefaults totrue.ssr={false}does not call the server-side RSC payload path during initial SSR.window.REACT_ON_RAILS_RSC_PAYLOADS[...]entries.ssr={true}routes can SSR whilessr={false}routes defer.RSCProviderpath.ssror passingssr={true}preserves the existing server-rendered path.Implementation notes
Rendering a user-provided
Suspensefallback on the server requires React's server-error bailout path. Returningnullwould skip RSC payload work, but it would not activate the surroundingSuspensefallback. To get both behaviors,RSCRoute ssr={false}throws a classified server-only bailout beforeuseRSC, provider cache-key generation, or RSC payload generation.React treats that throw as recoverable when it is caught by
Suspense: the server streams the nearest fallback and the client retries the route. React still reports that recovery path through the server streamonErrorcallback and the clienthydrateRootrecoverable-error callback, so React on Rails Pro distinguishes this intentional bailout from real render failures.This PR:
ssr?: booleantoRSCRouteProps.RSCRouteSSRFalseBailoutErrorto mark the intentional server-only bailout.Tests
Covered with package tests, streaming tests, docs, and dummy-app E2E for:
ssr={true}behavior;ssr={true}/ssr={false}routes in wrapped streaming roots;ServerComponentFetchErroranduseRSC().refetchComponent(...)retry;Test plan
pnpm --filter react-on-rails-pro run type-checkpnpm --filter react-on-rails-pro run testpnpm --filter react-on-rails run type-checkpnpm --filter react-on-rails run testpnpm --filter react-on-rails run buildpnpm --filter react-on-rails-pro run buildENABLE_JEST_CONSOLE=true pnpm --filter react-on-rails-pro exec jest tests/deferredRouteSsr.test.tsx --runInBandENABLE_JEST_CONSOLE=true pnpm --filter react-on-rails-pro exec jest tests/streamServerRenderedReactComponent.test.jsx --runInBand -t 'RSCRoute ssr=false|post-SSR'cd react_on_rails_pro/spec/dummy && pnpm exec playwright test e2e-tests/rsc_route_ssr_false.spec.ts --project=chromium --config=playwright.config.tspnpm exec prettier --check <changed files>pnpm exec eslint --no-ignore <changed source and focused test files>git diff --check