Don't include 'react-dom/server' in the RSC bundle #1888
Conversation
WalkthroughThis PR refactors error handling in React Server Components (RSC) rendering across packages by introducing RSC-specific error handlers, splitting error message generation from HTML rendering, and updating the streaming pipeline to pipe error streams through a transform rather than directly writing chunks. It also adds RSC-specific base modules and separates test execution between RSC and non-RSC tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RSCRoute as RSCRoute<br/>(PromiseWrapper)
participant StreamPipeline as streamServerRendered<br/>ReactComponent
participant ErrorHandler as handleError /<br/>handleErrorRSC
participant Output as Output Stream
Client->>RSCRoute: Render RSC
RSCRoute->>StreamPipeline: streamServerRenderedComponent<br/>(options, renderStrategy, handleError)
alt Success Path
StreamPipeline->>Output: Pipe resolved stream
else Error Path
StreamPipeline->>ErrorHandler: Call handleError(options)
ErrorHandler->>Output: Return error stream<br/>(HTML/RSC formatted)
Output-->>StreamPipeline: Pipe via pipeToTransform
end
Output->>Client: Serialized response
alt Error in Payload
Client->>RSCRoute: createFromNodeStream<br/>(decodes payload)
RSCRoute->>RSCRoute: Check if Error instance
RSCRoute->>RSCRoute: Rethrow if Error
else Success
RSCRoute->>Client: Return resolved node
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
PR Review: Remove
|
![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
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)
64-76: Notify post‑SSR hooks (and optionally flag error) in the string fallback path.Currently, when a render function returns a string, we end the stream without calling post‑SSR hooks; hooks relying on notifySSREnd never fire. Optionally, flag this as an error to surface misuse.
Apply:
- writeChunk(reactRenderedElement); - endStream(); + // Mark misuse for diagnostics and still stream the string + reportError( + new Error( + `stream_react_component received a string for "${componentName}". Expected a React element.` + ), + ); + writeChunk(reactRenderedElement); + // Ensure post-SSR hooks run even in this fallback + streamingTrackers.postSSRHookTracker.notifySSREnd(); + endStream(); return;
🧹 Nitpick comments (6)
packages/react-on-rails/src/base/full.rsc.ts (1)
6-38: Consider extracting common logic to reduce duplication.The
createBaseFullObjectfunction is nearly identical to the one infull.ts(lines 34-66), differing only in the stub implementations. While this duplication may be intentional to avoid importing SSR modules in the RSC bundle, consider whether the common scaffolding logic could be extracted to reduce maintenance burden.However, if the duplication is necessary for proper tree-shaking and bundle separation, this is acceptable.
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
78-93: Abort React stream on shell error to avoid orphaned work.If the shell fails, we never pipe the React stream; abort it to free resources and prevent background work.
- onShellError(e) { - sendErrorHtml(convertToError(e)); - }, + onShellError(e) { + try { + // Stop any ongoing React work + renderingStream.abort(); + } catch {} + sendErrorHtml(convertToError(e)); + },packages/react-on-rails-pro/src/streamingUtils.ts (4)
146-151: Fix outdated comment (we’re not using Readable.from here).Comment mentions Readable.from, but code wraps via bufferStream. Update for accuracy.
- // 3. By wrapping in Readable.from(), we can explicitly emit errors into the readableStream without affecting the transformStream - // Note: Readable.from can merge multiple chunks into a single chunk, so we need to ensure that we can separate them later + // 3. By wrapping the PassThrough in bufferStream(), we can explicitly emit errors into the returned readable + // without affecting the underlying transformStream. bufferStream also preserves event order before reading starts.
156-158: Harden abort guard to avoid calling a non‑function.Use a function check rather than property existence.
- if (pipedStream && 'abort' in pipedStream) { - pipedStream.abort(); - } + if (pipedStream && typeof (pipedStream as any).abort === 'function') { + (pipedStream as any).abort(); + }
217-219: Clarify error message for server render hash in streaming path.Improve wording for faster diagnosis.
- throw new Error('Server rendering of streams is not supported for server render hashes.'); + throw new Error( + 'Streaming is not supported when the render function returns a server render hash. ' + + 'Register the component (not a renderer) or return a React element.' + );
236-250: RSC error‑handling alignment (context check).Centralized error streaming via
handleErroris good. Ensure the RSC path passes an RSC‑specifichandleErrorRSCso we don’t over‑eagerly render HTML for RSC payload errors; RSC fetchers should surface errors to client error boundaries. Based on learnings.Can you confirm
streamServerRenderedComponentis invoked withhandleErrorRSCfor RSC entry points and withhandleErrorfor SSR?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(4 hunks)packages/react-on-rails-pro/src/handleError.ts(1 hunks)packages/react-on-rails-pro/src/handleErrorRSC.ts(1 hunks)packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts(3 hunks)packages/react-on-rails-pro/src/streamingUtils.ts(1 hunks)packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsx(1 hunks)packages/react-on-rails/package.json(1 hunks)packages/react-on-rails/src/base/full.rsc.ts(1 hunks)packages/react-on-rails/src/base/full.ts(1 hunks)packages/react-on-rails/src/generateRenderingErrorMessage.ts(1 hunks)packages/react-on-rails/src/handleError.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsxpackages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails/src/base/full.rsc.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails/package.jsonpackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsxpackages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails/src/base/full.rsc.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails/package.json
🧬 Code graph analysis (9)
packages/react-on-rails-pro/jest.config.js (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
process(5-5)
packages/react-on-rails-pro/src/handleError.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToPipeableStream(4-4)
packages/react-on-rails/src/base/full.rsc.ts (2)
packages/react-on-rails/src/base/full.ts (3)
createBaseFullObject(34-66)BaseFullObjectType(29-32)ReactOnRailsFullSpecificFunctions(19-22)packages/react-on-rails/src/base/client.ts (2)
createBaseClientObject(58-275)BaseClientObjectType(43-52)
packages/react-on-rails/src/generateRenderingErrorMessage.ts (1)
packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)
packages/react-on-rails/src/handleError.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
packages/react-on-rails-pro/src/streamingUtils.ts (1)
streamServerRenderedComponent(179-251)
packages/react-on-rails-pro/src/handleErrorRSC.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToPipeableStream(4-4)
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (2)
packages/react-on-rails-pro/src/streamingUtils.ts (1)
streamServerRenderedComponent(179-251)packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)
packages/react-on-rails-pro/src/streamingUtils.ts (4)
packages/react-on-rails/src/types/index.ts (5)
RenderParams(220-224)StreamRenderState(473-476)ErrorOptions(236-243)assertRailsContextWithServerComponentMetadata(80-94)RailsContextWithServerStreamingCapabilities(65-68)packages/react-on-rails/src/serverRenderUtils.ts (3)
createResultObject(9-24)validateComponent(30-36)convertToError(26-28)packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsx
[error] 6-9: The 'react' package in this environment is not configured correctly. The 'react-server' condition must be enabled for React Server Components. This caused the test import to fail.
🪛 GitHub Actions: Lint JS and Ruby
packages/react-on-rails/src/base/full.ts
[warning] 19-19: Unused exported type: ReactOnRailsFullSpecificFunctions.
packages/react-on-rails/src/base/full.rsc.ts
[warning] 1-1: Unused files detected by knip: full.rsc.ts.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (16)
packages/react-on-rails/src/base/full.ts (1)
19-22: LGTM! Export is necessary for RSC implementation.The export of
ReactOnRailsFullSpecificFunctionsis used infull.rsc.ts(line 2), ensuring type safety across both the standard and RSC-specific implementations. The pipeline warning appears to be a false positive.packages/react-on-rails/src/base/full.rsc.ts (2)
1-4: LGTM! Type re-exports maintain API compatibility.The imports and type re-exports ensure the RSC bundle exposes the same type interface as the standard bundle, maintaining consistency across implementations.
1-38: I need to verify the build configuration to ensurefull.rsc.tsis actually being compiled to the output location specified in package.json.Now I need to check the actual build configuration to verify the TypeScript file is being compiled:
Based on my verification of the package.json exports and build configuration patterns, I can now provide a final assessment:
RSC bundle configuration is properly set up; knip warning does not indicate a build system problem.
The package.json correctly exports
./@internal/base/fullwith a"react-server"condition mapping to./lib/base/full.rsc.js. This is the correct and standard pattern for React Server Components bundles. Files are reported as unused if they are in the set of project files, but not in the set of files resolved from the entry files, which explains the knip warning—but this doesn't reflect an actual build system issue.Entry point source files (like
full.rsc.ts) are typically not listed in knip's entry file configuration. The build system should compile this file to the output location specified in the package.json exports. The warning is likely a false positive; if the build process hasn't been run yet on this PR branch, knip won't resolve the compiled output file.No action is required for the code or configuration shown. Verify the build pipeline runs successfully to confirm
full.rsc.tscompiles tolib/base/full.rsc.js, and the knip warning should resolve once the build artifacts are present.packages/react-on-rails/src/handleError.ts (1)
4-9: LGTM! Clean separation of concerns.The refactoring delegates error message generation to a dedicated module and renders it consistently. The use of
renderToStringis appropriate for synchronous string-based error rendering in the non-RSC bundle.packages/react-on-rails-pro/src/handleError.ts (1)
1-12: LGTM! Correct streaming implementation for Pro package.The implementation correctly uses
renderToPipeableStreamfor async streaming error rendering, while maintaining the same error message generation logic through the sharedgenerateRenderingErrorMessagemodule.packages/react-on-rails/package.json (2)
50-50: LGTM! New export enables shared error message generation.The new export for
generateRenderingErrorMessageallows both the base and Pro packages to share error formatting logic.
56-59: LGTM! Conditional exports correctly separate RSC and default bundles.The conditional export structure ensures that:
- The
react-servercondition loads the RSC-specific implementation (full.rsc.js) which avoids importing react-dom/server- The default condition loads the standard implementation (full.js) with full server rendering capabilities
This change directly addresses the PR objective of removing react-dom-server from the RSC bundle.
packages/react-on-rails-pro/src/handleErrorRSC.ts (1)
1-15: LGTM! RSC-specific error handler correctly avoids react-dom/server.The implementation correctly uses
react-on-rails-rsc/server.nodefor rendering, which aligns with the PR objective. The additional RSC-specific options (filePathToModuleMetadataandmoduleLoading) are provided with sensible defaults for error rendering scenarios.packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (4)
26-26: LGTM! Correctly imports RSC-specific error handler.The import of
handleErrorfrom./handleErrorRSC.tsensures the RSC bundle uses the RSC-specific rendering implementation that doesn't depend on react-dom/server.
54-55: LGTM! Refactored to use centralized streaming utilities.The use of
pipeToTransformfrom the new streaming utilities provides consistent error handling and stream transformation across the codebase.
86-91: LGTM! Error handling correctly uses RSC-specific renderer.The error path now uses
handleError(from handleErrorRSC.ts) to generate error HTML and pipes it through the transform stream, maintaining consistency with the new streaming architecture.
99-105: LGTM! Passes RSC error handler to streaming component.The
handleErrorparameter enablesstreamServerRenderedComponentto use the RSC-specific error rendering path, completing the separation of RSC and standard server rendering.packages/react-on-rails/src/generateRenderingErrorMessage.ts (2)
3-33: LGTM! Clear error detection for render function issues.The helper function correctly identifies and logs common render function misconfiguration patterns, providing helpful error messages to developers.
35-64: Verify the client-side return value.The function returns the string literal
'undefined'for client-side rendering (line 63). This seems unusual - typically you'd returnundefined(the value) or an empty string.Check if callers expect the string 'undefined' or if this should be:
return undefined; // or return '';Looking at the callers (
handleError.tsandhandleError.tsin Pro), they both wrap the result in a React element and render it, which suggests they're always called server-side. If that's the case, the client-side branch may be unreachable, but clarification would be helpful.packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsx (1)
1-13: Fix the pipeline failure by adding the react-server export condition.The test correctly validates that ReactOnRailsRSC doesn't import react-dom/server. However, the pipeline failure indicates the test environment needs the 'react-server' export condition enabled.
Set the NODE_CONDITIONS environment variable in your test script to include 'react-server':
#!/bin/bash # Add this to your package.json test script or CI configuration NODE_CONDITIONS=react-server yarn testAlternatively, update the jest.config.js to include 'react-server' by default:
testEnvironmentOptions: { - customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? [], + customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? ['react-server'], },⛔ Skipped due to learnings
Learnt from: alexeyr-ci PR: shakacode/react_on_rails#1644 File: node_package/src/ReactOnRailsRSC.ts:87-87 Timestamp: 2024-12-12T13:07:09.929Z Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
17-17: The code structure properly separates ReactDOMServer from RSC builds. Review comment is incorrect.The separation is guaranteed by:
Source isolation:
streamServerRenderedReactComponent.ts(which importsReactDOMServer) is imported exclusively inReactOnRails.node.ts, never in RSC entry points.Entry point isolation:
ReactOnRailsRSC.ts(RSC entry) does not importstreamServerRenderedReactComponentorReactOnRails.node.ts—only RSC-specific files likehandleErrorRSC.tsand the externalreact-on-rails-rscpackage.Conditional exports: The package exports define separate conditions (
react-server→ReactOnRailsRSC.js,node→ReactOnRails.node.js), andReactDOMServerhas noreact-servercondition, so tree-shaking works correctly.No cross-contamination: RSC-specific files (
.rsc.ts,.rsc.tsx) do not import from node or SSR modules.No additional verification needed—the current design already prevents ReactDOMServer from being bundled into RSC builds.
Likely an incorrect or invalid review comment.
| testEnvironmentOptions: { | ||
| customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? [], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider defaulting to 'react-server' export condition.
The current implementation defaults to an empty array, which causes the pipeline test failure. Since the RSC tests require the 'react-server' condition, consider including it by default.
Apply this diff:
testEnvironmentOptions: {
- customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? [],
+ customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? ['react-server'],
},This ensures RSC tests work correctly without requiring manual environment variable configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testEnvironmentOptions: { | |
| customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? [], | |
| }, | |
| testEnvironmentOptions: { | |
| customExportConditions: process.env.NODE_CONDITIONS?.split(',') ?? ['react-server'], | |
| }, |
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/jest.config.js around lines 31 to 33, the
testEnvironmentOptions.customExportConditions currently defaults to an empty
array which breaks RSC tests; change the logic so that when NODE_CONDITIONS is
unset or empty it defaults to ['react-server'], and when NODE_CONDITIONS is set
include its values but ensure 'react-server' is present (and deduplicated) in
the resulting array before assigning it to customExportConditions.
| const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = { | ||
| handleError() { | ||
| throw new Error('"handleError" function is not supported in RSC bundle'); | ||
| }, | ||
|
|
||
| serverRenderReactComponent() { | ||
| throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle'); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Fix type signature mismatch in stub functions.
The stub implementations don't match the expected signatures from ReactOnRailsFullSpecificFunctions. While the functions throw errors immediately, they must still satisfy the type contract to maintain type safety.
Apply this diff to correct the function signatures:
const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = {
- handleError() {
+ handleError(options) {
+ void options; // Mark as used
throw new Error('"handleError" function is not supported in RSC bundle');
},
- serverRenderReactComponent() {
+ serverRenderReactComponent(options) {
+ void options; // Mark as used
throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle');
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = { | |
| handleError() { | |
| throw new Error('"handleError" function is not supported in RSC bundle'); | |
| }, | |
| serverRenderReactComponent() { | |
| throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle'); | |
| }, | |
| }; | |
| const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = { | |
| handleError(options) { | |
| void options; // Mark as used | |
| throw new Error('"handleError" function is not supported in RSC bundle'); | |
| }, | |
| serverRenderReactComponent(options) { | |
| void options; // Mark as used | |
| throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle'); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/base/full.rsc.ts around lines 15 to 23, the stub
functions throw errors but do not match the parameter and return type signatures
defined by ReactOnRailsFullSpecificFunctions; update each stub to use the exact
parameter list and return type declared in the ReactOnRailsFullSpecificFunctions
interface (e.g. add the same parameters and return type annotations) and keep
the immediate throw inside the body so the implementation still throws an Error
indicating the function isn't supported in the RSC bundle.
Code Review - PR #1888: Don't include react dom server at the rsc bundleSummaryThis PR addresses issue #1886 by separating ReactDOMServer dependencies from the RSC bundle, improving error handling for React Server Components. The changes refactor error handling to use appropriate rendering methods for RSC vs traditional SSR. ✅ Strengths1. Excellent Architecture & Separation of Concerns
2. Smart Error Propagation in RSC
3. Robust Streaming Pipeline
4. Good Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-on-rails-pro/src/RSCRoute.tsx (1)
78-89: Consider refining the promise type to include Error.Since the code explicitly checks for
Erroras a resolved value (line 84), the promise parameter type could be refined toPromise<React.ReactNode | Error>for better type accuracy. However, this may be intentionally omitted to keep the external API clean, so this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/src/handleErrorRSC.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/src/handleErrorRSC.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/RSCRoute.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/RSCRoute.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (1)
packages/react-on-rails-pro/src/RSCRoute.tsx (1)
84-86: The review comment is incorrect based on how React Server Components handle error serialization.The concern assumes plain JSON serialization loses the Error prototype. However, React RSC uses a custom "Flight" serialization protocol (not JSON) that explicitly supports Error objects. The code comment itself documents this: "RSC bundle will return an error object serialized inside the RSC payload."
Key evidence:
- React's Flight protocol has built-in support for error serialization (per React's official documentation and GitHub discussions)
- The
instanceof Errorcheck is the standard pattern for RSC error handlingReact.use()properly deserializes errors from RSC payloads, preserving their type- The codebase's
convertToError()utility already handles both Error and non-Error thrown values appropriatelyThe existing code is correct and requires no changes.
Likely an incorrect or invalid review comment.
PR Review: Remove react-dom/server from RSC BundleSummaryThis PR addresses issue #1886 by removing ✅ Strengths1. Excellent Architecture & Separation of Concerns
2. Improved Error Handling
3. Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/handleError.ts(1 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/jest.config.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/package.json
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/handleError.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro/src/handleError.ts
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/handleError.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
script/convert
🧬 Code graph analysis (1)
packages/react-on-rails-pro/src/handleError.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)
🪛 GitHub Actions: Lint JS and Ruby
script/convert
[error] 51-51: Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call. Offense detected at line 51:26 (autocorrectable). Rubocop exited with code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (3)
packages/react-on-rails-pro/package.json (1)
10-11: LGTM! Clean test separation for RSC.The test script changes correctly isolate RSC tests from the main test suite. Setting
NODE_CONDITIONS=react-serverin the dedicatedtest-rscscript ensures the RSC bundle is tested under the proper conditional export environment, which aligns with the PR objective of removing react-dom-server from the RSC bundle.packages/react-on-rails-pro/src/handleError.ts (2)
1-5: LGTM! Imports are appropriate for SSR error handling.The imports are clean and correctly scoped. Using
renderToStringfromreact-on-rails/ReactDOMServeris appropriate here since this is the pro package for SSR, not the RSC bundle mentioned in the PR objectives.
14-14: LGTM! Clean default export.The default export is appropriate for this single-purpose module.
| const handleError = (options: ErrorOptions) => { | ||
| const msg = generateRenderingErrorMessage(options); | ||
| const reactElement = React.createElement('pre', null, msg); | ||
| const htmlString = renderToString(reactElement); | ||
| return Readable.from([htmlString]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
The function should declare its return type explicitly for better type safety and developer experience.
Apply this diff:
-const handleError = (options: ErrorOptions) => {
+const handleError = (options: ErrorOptions): Readable => {
const msg = generateRenderingErrorMessage(options);
const reactElement = React.createElement('pre', null, msg);
const htmlString = renderToString(reactElement);
return Readable.from([htmlString]);
};🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/handleError.ts around lines 7 to 12, the
handleError function lacks an explicit return type; add an explicit return type
of Readable (from Node's stream module) to the function signature (e.g. annotate
the function as returning Readable) and ensure { Readable } is imported from
'stream' if not already, keeping the implementation unchanged.
| import generateRenderingErrorMessage from 'react-on-rails/generateRenderingErrorMessage'; | ||
|
|
||
| const handleError = (options: ErrorOptions) => { | ||
| const msg = generateRenderingErrorMessage(options); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error stack is included in development and test environments.
Based on learnings, error messages should include the stack trace in development and test environments to aid debugging. Please ensure generateRenderingErrorMessage includes the error stack (accessible via options.e.stack) when process.env.NODE_ENV is 'development' or 'test'.
Based on learnings.
Run the following script to examine the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Verify generateRenderingErrorMessage includes error stack in dev/test
# Find and display the generateRenderingErrorMessage implementation
ast-grep --pattern 'function generateRenderingErrorMessage($$$) {
$$$
}'
# Also check for arrow function syntax
ast-grep --pattern 'const generateRenderingErrorMessage = ($$$) => {
$$$
}'Length of output: 0
I need to search more broadly for the generateRenderingErrorMessage function. Let me locate it first and then examine its implementation.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find generateRenderingErrorMessage definition
rg -n "generateRenderingErrorMessage" --type ts --type js
# Also look at the handleError.ts file for context
echo "=== handleError.ts content ==="
fd -n "handleError.ts" --type f --exec cat {}
Length of output: 1224
Now let me examine the actual implementation of the function:
---
🏁 Script executed:
```shell
#!/bin/bash
# Display the generateRenderingErrorMessage implementation
cat packages/react-on-rails/src/generateRenderingErrorMessage.ts
Length of output: 1948
Include error stack in development and test environments.
The current implementation of generateRenderingErrorMessage (packages/react-on-rails/src/generateRenderingErrorMessage.ts, lines 35-66) does not check process.env.NODE_ENV. The error stack is only included in the returned message when serverSide is true, but it should also be included when process.env.NODE_ENV is 'development' or 'test' to aid debugging.
Modify the function to conditionally include e.stack in the message based on the environment, not just the serverSide flag.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/handleError.ts around line 8 and in
packages/react-on-rails/src/generateRenderingErrorMessage.ts around lines 35-66,
the error message currently includes e.stack only when serverSide is true;
update generateRenderingErrorMessage so it also includes e.stack when
process.env.NODE_ENV === 'development' or 'test' (in addition to serverSide).
Change the conditional that builds the message to check (serverSide ||
process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') and
append e.stack when present, leaving other logic and return shape unchanged.
Code Review for PR #1888: Don't include react-dom-server at the RSC bundleSummaryThis PR addresses issue #1886 by refactoring error handling and streaming utilities to prevent RSC bundles from importing ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-on-rails-pro/jest.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/jest.config.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/jest.config.js
🧬 Code graph analysis (1)
packages/react-on-rails-pro/jest.config.js (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
process(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
| testEnvironmentOptions: process.env.NODE_CONDITIONS | ||
| ? { | ||
| customExportConditions: process.env.NODE_CONDITIONS.split(','), | ||
| } | ||
| : {}, |
There was a problem hiding this comment.
Address the unresolved past review comment and handle edge cases.
The past review comment identified that the current default breaks RSC tests and causes pipeline failures. Additionally, the current implementation doesn't handle edge cases:
- Empty string:
"".split(',')returns[""]instead of an empty array - Whitespace: values are not trimmed after splitting
Apply this diff to address both the default and edge cases:
- testEnvironmentOptions: process.env.NODE_CONDITIONS
- ? {
- customExportConditions: process.env.NODE_CONDITIONS.split(','),
- }
- : {},
+ testEnvironmentOptions: {
+ customExportConditions: process.env.NODE_CONDITIONS
+ ? process.env.NODE_CONDITIONS.split(',').map(s => s.trim()).filter(Boolean)
+ : ['react-server'],
+ },This ensures:
- RSC tests work correctly by defaulting to
['react-server'] - Empty strings are filtered out
- Whitespace is trimmed from condition names
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/jest.config.js around lines 31 to 35, the current
testEnvironmentOptions logic breaks RSC tests and doesn't handle edge cases;
change it so when process.env.NODE_CONDITIONS is unset you default to
['react-server'], and when it is set split on ',' then map each entry to trimmed
string and filter out empty strings (so "" and whitespace-only entries are
removed) before assigning to customExportConditions. Ensure the final value is
an array of non-empty trimmed strings.
Pull Request ReviewThank you for tackling issue #1886! This is a well-structured PR that successfully removes react-dom/server dependencies from the RSC bundle. StrengthsArchitecture & Design
Testing
Error Handling
Issues & ConcernsCritical Issues1. Incomplete error handling in handleErrorRSC.ts:
2. Type safety in streamingUtils.ts:
3. Error instanceof check (RSCRoute.tsx:84):
Documentation4. PR checklist incomplete:
5. script/convert changes:
RecommendationsHigh Priority
Medium Priority
Security & Performance
SummarySolid refactoring addressing the core issue. Main concerns:
Recommendation: Address critical issues and complete documentation before merging. Great work on test coverage and architecture! |
Code Review: PR #1888 - Don't include react dom server at the rsc bundleSummaryThis PR addresses issue #1886 by removing dependencies on ✅ Strengths
|
alexeyr-ci2
changed the title
Pull Request Review: Don't include 'react-dom/server' in the RSC bundleOverall AssessmentThis is a well-architected PR that addresses a critical issue with React Server Components (RSC) bundles inadvertently including Recommendation: APPROVE with minor suggestions Code Quality & Architecture ✅Strengths:
Test Coverage ✅Good:
Suggestions for Enhancement:
Potential Issues & Concerns1. Error Object Serialization in RSC Payload (Medium Priority)
|
AbanoubGhadban
force-pushed
the
abanoubghadban/remove-react-dom-server-from-rsc-bundle
branch
from
ed1e13d to
ebf8032
Compare
Pull Request Review: Don't include 'react-dom/server' in the RSC bundleSummaryThis PR addresses issue #1886 by improving error handling for RSC (React Server Components) payload generation and removing Code Quality & Best Practices ✅Strengths:
Areas for Improvement:
Potential Bugs & Issues
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
78-83: Ensure shell errors update render stateWhen the shell fails, we stream the fallback HTML but leave
renderState.hasErrorsfalse andrenderState.errorunset, so downstream consumers never learn an error occurred. Please flag the error viareportErrorbefore piping the fallback output.onShellError(e) { - sendErrorHtml(convertToError(e)); + const error = convertToError(e); + reportError(error); + sendErrorHtml(error); },
♻️ Duplicate comments (2)
packages/react-on-rails-pro/src/handleError.ts (2)
8-8: Error stack should include NODE_ENV check.A past review comment noted that
generateRenderingErrorMessageshould include the error stack not only whenserverSideis true, but also whenprocess.env.NODE_ENVis 'development' or 'test' to aid debugging. This applies to thegenerateRenderingErrorMessagefunction inpackages/react-on-rails/src/generateRenderingErrorMessage.ts.Based on learnings and past review comments.
7-12: Add explicit return type annotation.The
handleErrorfunction should explicitly declare its return type as: Readablefor better type safety and developer experience.Apply this diff:
-const handleError = (options: ErrorOptions) => { +const handleError = (options: ErrorOptions): Readable => { const msg = generateRenderingErrorMessage(options); const reactElement = React.createElement('pre', null, msg); const htmlString = renderToString(reactElement); return Readable.from([htmlString]); };
🧹 Nitpick comments (2)
packages/react-on-rails/src/base/full.ts (1)
19-22: LGTM! Type export enables RSC integration.The export change makes this type available to RSC streaming modules as intended. The type definition itself is well-structured and safe.
Optional: Consider adding
@publicJSDoc tag for API clarity.For consistency with
BaseFullObjectType(line 27) and to clearly signal this is now part of the public API surface, consider adding a@publicJSDoc tag to the comment above./** * SSR-specific functions that extend the base client object to create a full object. * Typed explicitly to ensure type safety when mutating the base object. + * @public */ export type ReactOnRailsFullSpecificFunctions = Pick<packages/react-on-rails/src/generateRenderingErrorMessage.ts (1)
18-18: Consolidate console.error calls for better readability.There are multiple separate
console.errorcalls throughout this function (lines 18, 28, 38, 43, 47, 50, 51). Consider collecting all error information and logging it in a singleconsole.errorcall to improve readability and performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.md(1 hunks)knip.ts(1 hunks)packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(4 hunks)packages/react-on-rails-pro/src/handleError.ts(1 hunks)packages/react-on-rails-pro/src/handleErrorRSC.ts(1 hunks)packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts(3 hunks)packages/react-on-rails-pro/src/streamingUtils.ts(1 hunks)packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsx(1 hunks)packages/react-on-rails/package.json(1 hunks)packages/react-on-rails/src/base/full.rsc.ts(1 hunks)packages/react-on-rails/src/base/full.ts(1 hunks)packages/react-on-rails/src/generateRenderingErrorMessage.ts(1 hunks)packages/react-on-rails/src/handleError.ts(1 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/react-on-rails/package.json
- packages/react-on-rails-pro/src/handleErrorRSC.ts
- packages/react-on-rails-pro/jest.config.js
- packages/react-on-rails/src/base/full.rsc.ts
- script/convert
- packages/react-on-rails-pro/tests/ReactOnRailsRSC.test.tsx
- packages/react-on-rails-pro/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/handleError.tsCHANGELOG.mdpackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails-pro/src/handleError.tsCHANGELOG.mdpackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
packages/react-on-rails-pro/src/handleError.tsCHANGELOG.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CHANGELOG.mdpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
CHANGELOG.mdpackages/react-on-rails-pro/src/RSCRoute.tsxknip.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
CHANGELOG.mdknip.tspackages/react-on-rails/src/base/full.ts
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
packages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 Learning: 2025-06-11T12:34:58.182Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCProvider.tsx:0-0
Timestamp: 2025-06-11T12:34:58.182Z
Learning: `RSCProvider` intentionally keeps failed `getServerComponent` promises cached (even if rejected) to avoid repeated fetch attempts; callers must use `refetchComponent` to retry.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
packages/react-on-rails/src/handleError.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
📚 Learning: 2025-04-09T13:02:16.009Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
Applied to files:
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/streamingUtils.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.tsknip.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-09-29T12:23:13.615Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T12:23:13.615Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Applied to files:
knip.ts
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
knip.ts
🧬 Code graph analysis (6)
packages/react-on-rails-pro/src/handleError.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)
packages/react-on-rails/src/generateRenderingErrorMessage.ts (1)
packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)
packages/react-on-rails/src/handleError.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (4)
packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/base/client.ts (1)
handleError(262-267)packages/react-on-rails-pro/src/streamingUtils.ts (1)
streamServerRenderedComponent(179-251)
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (4)
packages/react-on-rails-pro/src/streamingUtils.ts (1)
streamServerRenderedComponent(179-251)packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/base/client.ts (1)
handleError(262-267)
packages/react-on-rails-pro/src/streamingUtils.ts (4)
packages/react-on-rails/src/types/index.ts (5)
StreamRenderState(473-476)RenderParams(220-224)ErrorOptions(236-243)assertRailsContextWithServerComponentMetadata(80-94)RailsContextWithServerStreamingCapabilities(65-68)packages/react-on-rails/src/serverRenderUtils.ts (3)
createResultObject(9-24)validateComponent(30-36)convertToError(26-28)packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: markdown-link-check
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (7)
CHANGELOG.md (1)
32-33: LGTM! Changelog entry accurately reflects the PR changes.The entry correctly documents the improved RSC payload error handling feature, with proper formatting and attribution.
knip.ts (1)
45-45: LGTM! RSC base entry point correctly added.The addition of
'src/base/full.rsc.ts!'properly registers the new RSC-specific base module as a production entry point, consistent with the PR's goal of improving RSC support.packages/react-on-rails/src/handleError.ts (1)
6-10: LGTM! Clean refactoring improves maintainability.The simplified implementation properly delegates error message generation to the centralized
generateRenderingErrorMessagefunction, while maintaining the same external API. The use ofrenderToStringto wrap the message in a<pre>element is appropriate for error display.packages/react-on-rails-pro/src/RSCRoute.tsx (1)
80-88: Verify test coverage for Promise resolving to Error.The new error handling logic checks if a Promise resolves to an Error instance (rather than rejecting) and throws it. A past reviewer noted this behavior isn't documented in React's
usedocumentation and requested verification that tests cover this case.Please confirm that test coverage exists for the scenario where
React.use(promise)receives a Promise that resolves to an Error object (as opposed to a rejected Promise). This is critical for ensuring the RSC error serialization path works correctly.You can verify by checking if tests exist that:
- Create an RSC payload that resolves to an Error
- Verify the Error is properly thrown and caught by RSCRouteErrorBoundary
Based on past review comments.
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (3)
26-33: LGTM! Import updates align with refactored module structure.The updated import paths for
handleErrorand streaming utilities correctly reference the new dedicated modules, improving code organization.
54-90: LGTM! Error handling properly integrated with streaming pipeline.The refactored error handling flow correctly:
- Removes direct stream writes (
writeChunk,endStream)- Routes errors through
pipeToTransformfor consistent stream handling- Uses the centralized
handleErrorfunction for error HTML generationThis improves maintainability and ensures consistent error handling across the RSC rendering pipeline.
101-101: LGTM! handleError properly injected into streaming component.The addition of
handleErroras the third argument tostreamServerRenderedComponentcorrectly integrates the error handling callback into the streaming flow.
| return msg; | ||
| } | ||
|
|
||
| return 'undefined'; |
There was a problem hiding this comment.
Return undefined instead of the string 'undefined'.
Line 63 returns the string literal 'undefined', which is incorrect. For client-side rendering, this should return undefined (the value, not a string).
Apply this diff:
- return 'undefined';
+ return undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return 'undefined'; | |
| return undefined; |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/generateRenderingErrorMessage.ts around line 63,
the function currently returns the string literal 'undefined' but should return
the JavaScript undefined value; replace the string return with a bare undefined
return (i.e., return undefined) so client-side rendering receives the correct
undefined value.
ebf8032 to
7b00477
Compare
AbanoubGhadban
changed the base branch from
master
to
prepare-for-removing-react-server-dom-from-rsc-bundle
AbanoubGhadban
force-pushed
the
abanoubghadban/remove-react-dom-server-from-rsc-bundle
branch
from
dc06b64 to
cf466b7
Compare
AbanoubGhadban
deleted the
abanoubghadban/remove-react-dom-server-from-rsc-bundle
branch
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/react-on-rails-pro/src/handleError.ts (1)
5-8: Add explicit return type annotation.The function should declare its return type explicitly for better type safety and developer experience.
Apply this diff:
-const handleError = (options: ErrorOptions) => { +const handleError = (options: ErrorOptions): Readable => { const htmlString = handleErrorAsString(options); return Readable.from([htmlString]); };packages/react-on-rails/src/generateRenderingErrorMessage.ts (1)
63-63: Return undefined instead of the string 'undefined'.Line 63 returns the string literal
'undefined', which is incorrect. For client-side rendering, this should returnundefined(the value, not a string).Apply this diff:
- return 'undefined'; + return undefined;packages/react-on-rails/src/base/full.rsc.ts (1)
15-23: Fix type signature mismatch in stub functions.The stub implementations don't match the expected signatures from
ReactOnRailsFullSpecificFunctions. While the functions throw errors immediately, they must still satisfy the type contract to maintain type safety.Apply this diff to correct the function signatures:
const reactOnRailsFullSpecificFunctions: ReactOnRailsFullSpecificFunctions = { - handleError() { + handleError(options) { + void options; // Mark as used throw new Error('"handleError" function is not supported in RSC bundle'); }, - serverRenderReactComponent() { + serverRenderReactComponent(options) { + void options; // Mark as used throw new Error('"serverRenderReactComponent" function is not supported in RSC bundle'); }, };
🧹 Nitpick comments (2)
packages/react-on-rails-pro/src/handleError.ts (1)
5-8: Consider consolidating with core handleError if duplication becomes problematic.Both
packages/react-on-rails/src/handleError.tsand this file follow similar patterns (callhandleErrorAsString/generateRenderingErrorMessage, wrap result). While they serve different return types (string vs stream), if the duplication becomes difficult to maintain, consider extracting a shared helper or creating a factory pattern.packages/react-on-rails-pro/src/handleErrorRSC.ts (1)
5-11: Add explicit return type annotation.For consistency with other handlers and better type safety, add an explicit return type to the
handleErrorfunction.Apply this diff:
+import type { PipeableStream } from 'react-on-rails-rsc/server.node'; + -const handleError = (options: ErrorOptions) => { +const handleError = (options: ErrorOptions): PipeableStream => { const msg = generateRenderingErrorMessage(options); return renderToPipeableStream(new Error(msg), { filePathToModuleMetadata: {}, moduleLoading: { prefix: '', crossOrigin: null }, }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.md(1 hunks)knip.ts(1 hunks)packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(4 hunks)packages/react-on-rails-pro/src/handleError.ts(1 hunks)packages/react-on-rails-pro/src/handleErrorRSC.ts(1 hunks)packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts(3 hunks)packages/react-on-rails-pro/src/streamingUtils.ts(4 hunks)packages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsx(1 hunks)packages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx(1 hunks)packages/react-on-rails/package.json(1 hunks)packages/react-on-rails/src/base/full.rsc.ts(1 hunks)packages/react-on-rails/src/base/full.ts(2 hunks)packages/react-on-rails/src/generateRenderingErrorMessage.ts(2 hunks)packages/react-on-rails/src/handleError.ts(1 hunks)script/convert(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/react-on-rails-pro/src/RSCRoute.tsx
- packages/react-on-rails/package.json
- packages/react-on-rails-pro/jest.config.js
- packages/react-on-rails-pro/package.json
- packages/react-on-rails/src/base/full.ts
- packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
- knip.ts
- script/convert
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
packages/react-on-rails/src/handleError.tspackages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
packages/react-on-rails-pro/src/handleErrorRSC.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails/src/base/full.rsc.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
packages/react-on-rails-pro/src/handleError.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx
📚 Learning: 2025-04-09T13:02:16.009Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
Applied to files:
packages/react-on-rails-pro/src/streamingUtils.tspackages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsxpackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails/src/generateRenderingErrorMessage.tspackages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsxpackages/react-on-rails/src/base/full.rsc.ts
🧬 Code graph analysis (7)
packages/react-on-rails/src/handleError.ts (3)
packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)
packages/react-on-rails-pro/src/handleErrorRSC.ts (4)
packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToPipeableStream(4-4)
packages/react-on-rails-pro/src/handleError.ts (2)
packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)
packages/react-on-rails-pro/src/streamingUtils.ts (3)
packages/react-on-rails/src/base/full.ts (1)
handleError(44-46)packages/react-on-rails/src/base/full.rsc.ts (1)
handleError(16-18)packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
packages/react-on-rails-pro/src/streamingUtils.ts (1)
streamServerRenderedComponent(179-251)
packages/react-on-rails/src/generateRenderingErrorMessage.ts (1)
packages/react-on-rails/src/types/index.ts (1)
ErrorOptions(236-243)
packages/react-on-rails/src/base/full.rsc.ts (2)
packages/react-on-rails/src/base/full.ts (3)
createBaseFullObject(34-66)BaseFullObjectType(29-32)ReactOnRailsFullSpecificFunctions(19-22)packages/react-on-rails/src/base/client.ts (2)
createBaseClientObject(58-275)BaseClientObjectType(43-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: markdown-link-check
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (10)
packages/react-on-rails/src/handleError.ts (1)
1-12: LGTM!Clean separation of concerns—delegates message generation to
generateRenderingErrorMessageand wraps the result in HTML. The refactoring successfully isolates the React/renderToString dependency from the core error message generation logic.packages/react-on-rails-pro/tests/ReactOnRailsRSC.rsc.test.tsx (1)
1-13: Excellent test design for validating the PR objective.The test directly validates that
ReactOnRailsRSCdoesn't importreact-dom/serverby mocking it to throw. This ensures the RSC bundle remains free of server rendering dependencies.packages/react-on-rails-pro/src/handleErrorRSC.ts (1)
7-10: Verify empty manifest is appropriate for error rendering.The error handler uses an empty
filePathToModuleMetadataand defaultmoduleLoadingconfig. Ensure this is the intended behavior for error scenarios, or if the manifest should be obtained from the rendering context.packages/react-on-rails-pro/tests/RSCSerialization.rsc.test.tsx (1)
17-44: LGTM! Comprehensive RSC serialization tests.The tests validate both success and error paths for RSC streaming, ensuring proper round-trip encoding/decoding of plain objects and Error instances. Well-structured with clear test cases.
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (3)
26-31: LGTM! Proper modularization of error handling.The updated imports correctly reference the local
handleErrorimplementation and the refactoredstreamServerRenderedComponentutility, improving code organization.
58-59: Improved error stream handling.Using
pipeToTransforminstead of manualwriteChunk/endStreamcalls provides better stream composition and aligns with the updated streaming architecture throughout the codebase.
104-104: LGTM! Updated to three-argument signature.Correctly passes
handleErroras the third argument tostreamServerRenderedComponent, enabling proper error handling in the streaming pipeline.packages/react-on-rails-pro/src/streamingUtils.ts (3)
29-29: LGTM! Clean type additions for improved API design.The
ErrorOptionsimport and new exported types (StreamingTrackers,StreamRenderer) enhance type safety and provide clear contracts for the streaming pipeline.Also applies to: 152-161
182-182: LGTM! Dependency injection improves flexibility.Passing
handleErroras a parameter enables different error handling strategies per rendering mode and improves testability.
237-249: LGTM! Excellent refactoring to pipe-based error streaming.The shift from
writeChunk/endStreamtopipeToTransformcreates a more consistent streaming architecture. The error handling flow correctly:
- Emits JS errors when
throwJsErrorsis enabled- Converts exceptions to standard Error objects
- Pipes error HTML streams through the transform pipeline
The
handleErrorparameter contract assumes implementations won't throw synchronously; any such error would propagate uncaught from this catch block. Given this is a dependency injection pattern with compile-time type enforcement, this is acceptable.
Summary
Solves #1886
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores