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 10 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 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}
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. |
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. |
![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".
ihabadham
force-pushed
the
ihabadham/feature/rsc-route-ssr-false
branch
from
5b74724 to
e77ed62
Compare
ihabadham
force-pushed
the
ihabadham/feature/rsc-route-ssr-false
branch
from
e77ed62 to
cf606f1
Compare
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
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
f07fcba to
9da8bc5
Compare
|
Address-review follow-up after
Resolved the review threads that were addressed, outdated after the split, or explicitly declined with the rationale above. |
Code Review: Add
|
Code Review — Add
|
Address-review summaryScan scope: full PR history; no prior summary marker. Mattered
Optional
Skipped
Follow-up issue: None. Next default scan starts after this comment. Say |
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