Use RSC payload to render server components on server #1696
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces comprehensive infrastructure for React Server Components (RSC) support in React on Rails, spanning configuration, server/client registration, payload streaming, context propagation, and SSR hydration. It adds new utilities, components, and APIs for managing RSC manifests, payloads, and hooks, while refactoring and expanding tests and documentation to reflect the new capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Rails as Rails (SSR)
participant ReactOnRails as ReactOnRails (Node)
participant RSCPayloadGen as RSCPayloadGenerator
participant RailsHTMLStream as HTML Stream with RSC Injection
participant Client as Browser
participant RSCProvider as RSCProvider (Client)
Rails->>ReactOnRails: Render request with RailsContext (SSR)
ReactOnRails->>RSCPayloadGen: getRSCPayloadStream(component, props, context)
RSCPayloadGen->>ReactOnRails: Stream RSC payload (tee'd for SSR + HTML)
ReactOnRails->>RailsHTMLStream: injectRSCPayload(pipeableHtmlStream, railsContext)
RailsHTMLStream->>Rails: Stream HTML + embedded RSC payload scripts
Rails-->>Client: Complete HTML with embedded RSC payloads
Client->>RSCProvider: useRSC(component, props)
RSCProvider->>window: Check for preloaded RSC payload
alt Preloaded
RSCProvider->>Client: Hydrate using preloaded payload
else Not preloaded
RSCProvider->>Rails: Fetch RSC payload via HTTP
Rails-->>RSCProvider: Stream RSC payload
RSCProvider->>Client: Hydrate using streamed payload
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@AbanoubGhadban can you update the PR description? |
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
9e49cb6 to
b0173f1
Compare
@justin808 Sure, I updated it |
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
316958d to
5c99bc4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
node_package/src/loadJsonFile.ts (1)
1-22: Well-implemented utility for loading JSON filesThis utility function is a good addition that provides caching for loaded JSON files, preventing redundant disk reads. The implementation includes proper error handling, logging, and follows async/await patterns correctly.
There is one optimization worth considering:
async function loadJsonFile(fileName: string) { // Asset JSON files are uploaded to node renderer. // Renderer copies assets to the same place as the server-bundle.js and rsc-bundle.js. // Thus, the __dirname of this code is where we can find the manifest file. const filePath = path.resolve(__dirname, fileName); if (!loadedJsonFiles.has(filePath)) { try { const file = JSON.parse(await fs.readFile(filePath, 'utf8')); loadedJsonFiles.set(filePath, file); } catch (error) { - console.error(`Failed to load JSON file: ${filePath}`, error); + const errorMessage = `Failed to load JSON file: ${filePath}`; + console.error(errorMessage, error); - throw error; + throw new Error(errorMessage, { cause: error }); } } return loadedJsonFiles.get(filePath)!; }This change would provide a more descriptive error message while preserving the original error as the cause, making debugging easier.
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts (1)
3-35: Improve error handling and resource management in stream transformationWhile the core transformation logic is solid, there are a few concerns with the implementation:
- The error handling stringifies the entire stream (
JSON.stringify(stream)on line 33), which could be problematic for large streams or potentially expose sensitive data.- There's no cleanup for the decoder or transform stream if an error occurs.
- The function assumes a specific format (JSON with an
htmlproperty) without validation.Consider these improvements:
export default function transformRSCStream(stream: NodeJS.ReadableStream): NodeJS.ReadableStream { const decoder = new TextDecoder(); let lastIncompleteChunk = ''; const htmlExtractor = new Transform({ transform(oneOrMoreChunks, _, callback) { try { const decodedChunk = lastIncompleteChunk + decoder.decode(oneOrMoreChunks); const separateChunks = decodedChunk.split('\n').filter(chunk => chunk.trim() !== ''); if (!decodedChunk.endsWith('\n')) { lastIncompleteChunk = separateChunks.pop() ?? ''; } else { lastIncompleteChunk = ''; } for (const chunk of separateChunks) { const parsedData = JSON.parse(chunk) as { html: string }; + if (!parsedData || typeof parsedData.html !== 'string') { + throw new Error(`Invalid RSC chunk format: ${chunk.substring(0, 100)}${chunk.length > 100 ? '...' : ''}`); + } this.push(parsedData.html); } callback(); } catch (error) { callback(error as Error); } } }); try { return stream.pipe(htmlExtractor); } catch (error) { - throw new Error(`Error transforming RSC stream (${stream.constructor.name}), (stream: ${stream}), stringified stream: ${JSON.stringify(stream)}, error: ${error}`); + throw new Error(`Error transforming RSC stream (${stream.constructor.name}), error: ${error}`); } }package.json (1)
57-57: Consider using versioned dependencies instead of branch referencesWhile using a specific branch works for development, it's generally better to use versioned dependencies for stability. Consider publishing a versioned release of the RSC package once the feature is stable.
Also applies to: 65-65
node_package/src/createReactOutput.ts (1)
8-26: Good refactoring of React element creation logicExtracting the element creation logic into a separate function improves code organization and reusability. The function correctly handles both direct React elements and components that need to be wrapped.
However, the warning message mentions "ReactOnRails v13 will throw error on this", but according to package.json, the current version is 15.0.0-alpha.2. Consider updating this message.
-incorrectly returned a React Element (JSX). Instead, return a React Function Component by -wrapping your JSX in a function. ReactOnRails v13 will throw error on this, as React Hooks do not +incorrectly returned a React Element (JSX). Instead, return a React Function Component by +wrapping your JSX in a function. This may cause errors with React Hooks, as they do notnode_package/src/ReactOnRailsRSC.ts (1)
27-31: Enhance error message clarity
When the Rails context is missing critical fields, it might help to specifically mention which fields are missing rather than displaying a generic "Rails context is not available."- throw new Error('Rails context is not available'); + throw new Error('Rails context is missing either `serverSide` or `reactClientManifestFileName`.');node_package/src/RSCServerRoot.ts (3)
7-13: Evaluate the globalgenerateRSCPayloaddeclaration
Declaring a global could conflict in certain bundling scenarios or mock contexts. Consider scoping it under a namespace or module export if collisions are a concern.
31-57: Consider making theprefixconfigurable
Currently,/webpack/development/is hardcoded. If you plan to run in multiple environments (e.g., production), a configurable prefix would be more flexible.
60-65: Inconsistent naming in error message
The code checksrailsContext.reactClientManifestFileNameandrailsContext.reactServerClientManifestFileNamebut the message mentionsserverClientManifestFileName and reactServerClientManifestFileName. Clarify or rename for accuracy.- `${'serverClientManifestFileName and reactServerClientManifestFileName are required. ' + + `${'reactClientManifestFileName and reactServerClientManifestFileName are required. ' +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
lib/react_on_rails/configuration.rb(6 hunks)lib/react_on_rails/test_helper/webpack_assets_status_checker.rb(1 hunks)lib/react_on_rails/utils.rb(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(3 hunks)node_package/src/createReactOutput.ts(2 hunks)node_package/src/handleError.ts(1 hunks)node_package/src/loadJsonFile.ts(1 hunks)node_package/src/loadReactClientManifest.ts(0 hunks)node_package/src/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/registerServerComponent/server.ts(2 hunks)node_package/src/serverRenderReactComponent.ts(2 hunks)node_package/src/streamServerRenderedReactComponent.ts(4 hunks)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/types/index.ts(4 hunks)node_package/tests/serverRenderReactComponent.test.js(2 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(2 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/loadReactClientManifest.ts
🧰 Additional context used
🧬 Code Definitions (3)
node_package/src/registerServerComponent/server.rsc.ts (1)
node_package/src/types/index.ts (3) (3)
ReactComponent(99-99)RenderFunction(101-101)ReactOnRails(179-214)
node_package/src/registerServerComponent/server.ts (2)
node_package/src/ComponentRegistry.ts (1) (1)
components(55-57)node_package/src/types/index.ts (3) (3)
ReactComponent(99-99)RenderFunction(101-101)RailsContext(16-47)
node_package/src/ReactOnRailsRSC.ts (3)
node_package/src/types/index.ts (2) (2)
RSCRenderParams(143-145)StreamRenderState(222-225)node_package/src/streamServerRenderedReactComponent.ts (1) (1)
transformRenderStreamChunksToResultObject(86-121)node_package/src/loadJsonFile.ts (1) (1)
loadJsonFile(6-22)
🔇 Additional comments (39)
node_package/src/ReactOnRails.client.ts (1)
334-335: Simple addition to support RSC bundlesThis change adds a new
isRSCBundleproperty to the ReactOnRails object, initialized to false. This flag helps identify whether the current bundle is being used as a React Server Components (RSC) bundle, which aligns with addressing the server components rendering twice issue mentioned in the PR objectives.node_package/src/handleError.ts (1)
65-68: Good defensive programming to handle missing renderToStringThis change adds a check to verify that
ReactDOMServer.renderToStringis a function before invoking it, preventing potential runtime errors in environments where the function might not be available. This is especially important for RSC environments where different rendering methods might be used.lib/react_on_rails/test_helper/webpack_assets_status_checker.rb (1)
55-56: Properly extending asset checking to include server client manifestThis addition extends the
all_compiled_assetsmethod to handle the React server client manifest file, following the same pattern as the existing client manifest file handling. This change is consistent with the PR's goal of properly handling RSC payloads.lib/react_on_rails/utils.rb (1)
125-132: LGTM: React server client manifest path implementationThe implementation follows the established pattern of other manifest file path methods. The comment clearly explains that the server manifest is generated by the server bundle and won't be served from the dev server, which justifies why it doesn't use the
PackerUtils.using_packer?check that's present in the client manifest method.node_package/src/registerServerComponent/server.rsc.ts (1)
1-26: Well-documented RSC component registration implementationThe function is simple but effective, delegating to
ReactOnRails.registerwithout additional wrapping. The documentation clearly explains the difference between this registration approach and server bundle registration, which helps developers understand when to use this function.lib/react_on_rails/configuration.rb (6)
13-13: LGTM: Good constant naming conventionThe constant name follows the same convention as the existing client manifest constant.
25-25: LGTM: Default configuration setupThe default configuration is set correctly using the defined constant.
70-70: LGTM: Attribute accessor additionThe attribute accessor is correctly added to the list, maintaining alphabetical ordering.
86-87: LGTM: Initialize method parameter additionThe parameter is correctly added to the initialize method signature, maintaining the pattern of other parameters.
117-117: LGTM: Configuration property assignmentThe configuration property is correctly set in the initialize method.
269-275: LGTM: Webpack generated files setupThe modification to include the new manifest file in the webpack_generated_files array follows the same pattern as the other files.
node_package/tests/serverRenderReactComponent.test.js (2)
71-88: Clear test naming improvementThe test case renaming from "serverRenderReactComponent renders promises" to "serverRenderReactComponent renders promises that return strings" improves clarity by explicitly stating what type of promise result is being tested.
90-110: Good addition of test coverage for React component promisesThis new test appropriately verifies that
serverRenderReactComponentcan handle promises that resolve to React components, which is an important capability for the RSC implementation. The test structure is clean and effectively validates both the rendering result and error handling.node_package/src/registerServerComponent/server.ts (2)
2-14: Improved documentation and implementation for RSC supportThe updated documentation clearly explains the new approach of wrapping components with
RSCServerRootto handle server-side rendering using pre-generated RSC payloads. This aligns well with the PR objective of reducing duplicate rendering of server components.
26-34: Well-structured implementation for RSC component wrappingThe implementation effectively wraps each component with
RSCServerRoot, enabling the use of pre-generated RSC payloads. The updated function signature that accepts bothReactComponentandRenderFunctionprovides good flexibility.package.json (1)
14-17: Good conditional exports configuration for server componentsThe updated exports configuration properly separates the server-side RSC implementation from the default implementation, which is essential for the React Server Components architecture.
node_package/src/createReactOutput.ts (2)
76-79: Enhanced promise handling for React componentsThe updated promise handling now correctly processes promises that resolve to either strings or React components, which is essential for supporting the RSC implementation. This change properly integrates with the new
createReactElementFromRenderFunctionfunction.
82-82: Simplified component wrapping with the new helper functionUsing the extracted helper function here makes the code more consistent and easier to maintain.
node_package/src/ReactOnRailsRSC.ts (3)
18-18: Switching toloadJsonFileis a clean and scalable approach
This replacement for the previous manifest loader is straightforward, and the caching logic inloadJsonFilehelps reduce repeated file reads.
41-41: Good use ofPromise.all
Executing the manifest load and the render result in parallel is efficient. No concerns here.
77-78: Be mindful of globally settingReactOnRails.isRSCBundle
If multiple bundles or configurations run simultaneously, globally toggling this flag could cause unintended side effects. Confirm it aligns with your usage across different environments.node_package/tests/streamServerRenderedReactComponent.test.jsx (6)
57-91: Comprehensive registration logic for diverse component types
You’ve covered multiple scenarios using thecomponentTypeswitch, ensuring tests can handle sync, async, and error-throwing variations. This adds valuable flexibility to the test suite.
190-206: Validating streamed components from promises
These tests confirm that both anasyncRenderFunctionand a simplerenderFunctionproduce valid stream outputs. The checks look thorough.
208-220: Good coverage of sync errors
Ensuring that synchronous errors within the render functions are handled correctly is crucial. These tests confirm correct error reporting.
222-238: Robust handling of async errors
The coverage here clearly verifies fallback logic when async errors occur, strengthening confidence in error-handling behavior.
240-258: Error simulation in erroneous render functions
This block cleanly demonstrates how errors originating within the render function (sync or async) are captured. Nicely done.
283-306: Excellent test for promise-resolved strings
This ensures the library can also stream raw HTML strings. Great addition to broaden the supported return types.node_package/src/RSCServerRoot.ts (2)
20-22: Proactive React version check
This error clarifies the requirement for React’s new hooks. If the user accidentally uses an older version, they’ll get a direct message. Looks good overall.
59-88: Structured approach for RSC usage
Gathering manifests and constructing a React element from the generated payload stream is a clean design. No major concerns on concurrency or security.node_package/src/types/index.ts (3)
16-47: Well-structured discriminated union for RailsContextThe refactoring of RailsContext into a discriminated union based on the
serverSideproperty is well-designed. This pattern ensures type safety by requiring different properties depending on whether rendering happens on the client or server side. The extensive comments provide good context about the purpose of the RSC payload parameters.
60-62: Enhanced return type flexibility for rendering functionsThe modified return types now correctly accommodate promises that resolve to either strings or React elements, which is essential for supporting asynchronous server component rendering. This change aligns well with the PR's goal of handling RSC payloads.
213-213: New flag to identify RSC bundlesThe addition of
isRSCBundleto the ReactOnRails interface provides a clear way to identify whether the current bundle is an RSC bundle. This flag is important for conditional behavior based on the rendering context.node_package/src/serverRenderReactComponent.ts (2)
47-60: Enhanced promise handling for React elementsThis is a great improvement to the
processPromisefunction. It now properly handles promises that resolve to either strings or React elements, callingprocessReactElementwhen necessary. This change enables support for rendering functions that return promises resolving to React elements, which is essential for RSC rendering.
84-86: Updated promise result type for consistencyThe function signature update correctly aligns with the enhanced promise types, ensuring type safety throughout the rendering pipeline. This change is necessary to support the new functionality while maintaining type correctness.
node_package/src/streamServerRenderedReactComponent.ts (5)
123-123: Enhanced function signature for streaming React componentsThe function now accepts both React elements and promises that resolve to React elements or strings, which is essential for supporting RSC. This change aligns with the overall goal of the PR to use RSC payload for server component rendering.
153-185: Well-implemented promise handling for streamingThe promise-based approach elegantly handles both synchronous and asynchronous rendering paths:
Promise.resolvenormalizes the input to always be a promise- The type check for strings provides helpful error messages
- The renderToPipeableStream is only called when appropriate
The error handling through onShellError and the various event handlers ensures a robust streaming implementation.
139-151: Improved error handling with extracted onShellErrorExtracting the onShellError logic into a separate variable improves code readability and maintainability. The error handling is comprehensive, covering both synchronous and asynchronous errors.
190-193: Updated StreamRenderer type for consistencyThe StreamRenderer type has been correctly updated to match the new function signature, ensuring type safety throughout the streaming pipeline. This change is necessary to support the enhanced rendering capabilities.
213-215:❓ Verification inconclusive
Verify promise handling without isPromise check
The condition now only checks for
isServerRenderHashand omits the previous check forisPromise. Ensure this doesn't cause issues with promise handling, as the function signature now explicitly accepts promises.
🏁 Script executed:
#!/bin/bash # Check how isPromise is used elsewhere in the codebase grep -r "isPromise" --include="*.ts" --include="*.js" node_package/Length of output: 686
Action: Confirm Promise Handling Strategy in Streamed Server Render
It appears that in
node_package/src/streamServerRenderedReactComponent.ts(lines 213–215), the check for promises usingisPromisehas been removed—likely because the function signature now explicitly accepts promise values. Our repository search shows thatisPromiseis still actively used in other contexts (e.g., increateReactOutput.tsandserverRenderReactComponent.ts), which indicates that handling promises is still a relevant concern in some parts of the codebase.
- Verify: Confirm that omitting the promise check here doesn’t cause unhandled promise values to slip through or lead to unexpected behavior.
- Clarify: Ensure that the error message—currently stating “Server rendering of streams is not supported for server render hashes or promises”—accurately reflects the intended support of promises. If promises are allowed, consider updating the error message accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (2)
15-20: Consider adding error handling for DOM operations.While the script injection logic is well-structured, there's no error handling for the DOM operations. Consider wrapping the DOM manipulation in a try-catch block to gracefully handle potential errors if this code runs in environments where DOM operations might not be available.
if (replayConsoleCode?.trim() !== '') { + try { const scriptElement = document.createElement('script'); scriptElement.textContent = replayConsoleCode; document.body.appendChild(scriptElement); + } catch (error) { + console.error('Error injecting console replay script:', error); + } }
24-38: Consider more descriptive variable naming for clarity.While the code is well-structured, some variable names could be more descriptive to better convey their purpose. For example,
decodedValuemight be better named asdecodedChunkordecodedTextto clarify what it represents.node_package/src/RSCClientRoot.ts (2)
3-4: ESLint directive usage.
Disablingno-underscore-dangleis likely necessary due to__FLIGHT_DATA. If possible, limit its scope to just the lines where_properties are used.
46-77: Constructing a readable stream fromwindow.__FLIGHT_DATA.
Overridingpushto enqueue data is creative. Verify that later usage does not inadvertently rely on standard array push behavior.node_package/src/RSCPayloadContainer.ts (1)
53-85: Async chunk handling logic.
Your promise chain effectively handles data, error, and end events. Just ensure error scenarios have adequate test coverage, especially for partially streamed data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/react_on_rails/helper.rb(2 hunks)node_package/src/RSCClientRoot.ts(4 hunks)node_package/src/RSCPayloadContainer.ts(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(5 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/types/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node_package/src/RSCServerRoot.ts
🧰 Additional context used
🧬 Code Definitions (1)
lib/react_on_rails/helper.rb (3)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(194-198)spec/dummy/spec/packs_generator_spec.rb (1)
configuration(26-28)lib/react_on_rails/configuration.rb (1)
configuration(16-56)
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (19)
node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (3)
11-21: Good refactoring: Extraction of JSON chunk handling logic.The introduction of the
handleJsonChunkfunction improves code organization by encapsulating the common logic for processing JSON chunks. This makes the code more maintainable and easier to understand.
23-42: Robust handling of ArrayBuffer data in stream processing.The conditional processing of ArrayBuffer views is a good approach. The code correctly:
- Combines incomplete chunks from previous iterations
- Splits by newlines and preserves incomplete chunks for the next iteration
- Filters out empty lines before parsing
- Includes proper error handling for JSON parsing
This makes the stream processing more robust and efficient.
43-45: Appropriate fallback for non-ArrayBuffer values.Adding a direct call to
handleJsonChunkfor non-ArrayBuffer values ensures the function can handle both types of input correctly, improving the function's flexibility and compatibility.lib/react_on_rails/helper.rb (2)
365-367: Validate thatrsc_payload_generation_url_pathhandles nil or empty values correctly.
Currently,rsc_urlis assigned only when Pro mode is detected. Confirm that a nil URL path is handled gracefully in the downstream code.Would you like a script to search the codebase for references to
rscPayloadGenerationUrland check if it handles nil or empty values safely?
378-379: New data keys added torails_context.
StoringrorProandrscPayloadGenerationUrlis straightforward, but ensure all relevant tests and usage cases consider these new keys for completeness.node_package/src/buildConsoleReplay.ts (1)
49-53: Efficient conditional wrapping of console replay.
Good practice to avoid rendering an empty script when there’s nothing to replay.node_package/src/RSCClientRoot.ts (3)
18-23: Global__FLIGHT_DATAfor streaming.
Defining an array onwindowis a workable approach for bridging client/server data, but confirm that no security or memory leak concerns arise from widely accessible global data.
79-83: Extracting a transformable RSC stream.
The function nicely composes the newly created stream with a console-logging transform. This keeps your RSC flow organized.
110-114: Conditional RSC hydration vs. fetch flow.
Switching tohydrateRootwhen the DOM already contains content (e.g., SSR scenario) is a solid approach. The fallback tocreateRootfor empty DOM nodes is sensible.node_package/src/RSCPayloadContainer.ts (1)
87-93: Boundary checks on chunk indices.
Throwing an error for out-of-range indices is correct. Consider logging or capturing these errors for debugging streaming issues.node_package/src/types/index.ts (4)
16-47: Good restructuring of RailsContext type to improve type safetyThe transformation of
RailsContextfrom an interface to a discriminated union type withserverSideas the discriminator is a significant improvement for type safety. This approach enforces proper property requirements based on whether we're in server or client context.The additional type documentation for
serverSideRSCPayloadParametersexplains the purpose clearly and follows good practices by usingunknowntype to avoid direct dependencies.
60-60: Type enhancement for Promise resolutionThe update to allow
CreateReactOutputResultto resolve to either a string or a ReactElement provides better flexibility for handling asynchronous server component rendering.
62-62: Type enhancement for RenderFunctionResultSimilar to the previous change, expanding
RenderFunctionResultto allow promises to resolve to either a string or a ReactComponent improves flexibility in handling render function outputs.
213-213: New isRSCBundle flag addedAdding the
isRSCBundleboolean property to theReactOnRailsinterface properly indicates whether the current bundle is a React Server Components bundle, supporting the PR's objective of using RSC payload for server component rendering.node_package/src/streamServerRenderedReactComponent.ts (5)
90-124: Good refactoring of console replay functionalityThe extraction of console replay logic into a separate function improves code organization and readability. The addition of timeout handling with proper cleanup prevents potential memory leaks.
148-148: Enhanced function signature to support asynchronous component renderingUpdating both the
streamRenderReactComponentfunction andStreamRenderertype to accept promises allows for more flexible component rendering patterns, which aligns with the PR's goal of improving server component rendering.Also applies to: 216-216
164-176: Improved error handling with dedicated functionExtracting the error handling logic into a separate
onShellErrorfunction improves readability and maintainability. The function properly updates render state, handles error throwing based on configuration, and manages stream lifecycle.
178-210: Robust Promise handling for rendering resultsThe new Promise-based approach properly handles both synchronous and asynchronous rendering results. The code correctly identifies string results and provides helpful error messages when string rendering is encountered instead of component rendering, while ensuring proper stream handling in all cases.
The implementation of shell ready and error callbacks in the rendering stream provides better control over the rendering lifecycle and error propagation.
244-259: Improved error handling in catch blockThe refactored error handling logic now properly manages the stream lifecycle by emitting errors directly (rather than throwing them), writing error content to the stream, and ensuring the stream is correctly ended. This prevents potential memory leaks and ensures proper error reporting.
| const scriptElement = React.createElement('script', { | ||
| dangerouslySetInnerHTML: { | ||
| __html: escapeScript(`(self.__FLIGHT_DATA||=[]).push(${chunk.chunk})`), | ||
| }, | ||
| key: `script-${chunkIndex}`, | ||
| }); |
There was a problem hiding this comment.
Potential XSS risk with dangerouslySetInnerHTML.
Though you’re escaping possible script tags, confirm that the data injected into __FLIGHT_DATA is fully sanitized to prevent malicious scripts.
Would you like suggestions on a safer approach, such as storing data in attributes instead of script content?
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
There was a problem hiding this comment.
@AbanoubGhadban Here are safer approaches to handling the RSC payload data:
1. Use script tag with type="application/json"
const RSCPayloadContainer = (
{ chunkIndex, getChunkPromise }: RSCPayloadContainerInnerProps,
): React.ReactNode => {
const chunkPromise = getChunkPromise(chunkIndex);
const chunk = React.use(chunkPromise);
// Script with type="application/json" won't be executed
const scriptElement = React.createElement('script', {
type: 'application/json',
id: `flight-data-${chunkIndex}`,
key: `script-${chunkIndex}`,
text: chunk.chunk, // Using text prop instead of dangerouslySetInnerHTML
});
// Add initialization code in a separate script or component
React.useEffect(() => {
const scriptEl = document.getElementById(`flight-data-${chunkIndex}`);
if (scriptEl && scriptEl.textContent) {
(window.__FLIGHT_DATA || (window.__FLIGHT_DATA = [])).push(
JSON.parse(scriptEl.textContent)
);
}
}, [chunkIndex]);
return (
<>
{scriptElement}
{!chunk.isLastChunk && (
<React.Suspense fallback={null}>
<RSCPayloadContainer
chunkIndex={chunkIndex + 1}
getChunkPromise={getChunkPromise}
/>
</React.Suspense>
)}
</>
);
};2. Use a hidden div with textContent
const RSCPayloadContainer = (
{ chunkIndex, getChunkPromise }: RSCPayloadContainerInnerProps,
): React.ReactNode => {
const chunkPromise = getChunkPromise(chunkIndex);
const chunk = React.use(chunkPromise);
// Create a hidden div with the data
const dataElement = React.createElement('div', {
style: { display: 'none' },
id: `flight-data-${chunkIndex}`,
key: `data-${chunkIndex}`,
'data-flight': true, // Marker attribute for identifying the elements
}, chunk.chunk); // Text content is safely set
// Process the data
React.useEffect(() => {
const dataEl = document.getElementById(`flight-data-${chunkIndex}`);
if (dataEl && dataEl.textContent) {
(window.__FLIGHT_DATA || (window.__FLIGHT_DATA = [])).push(
JSON.parse(dataEl.textContent)
);
}
}, [chunkIndex]);
return (
<>
{dataElement}
{!chunk.isLastChunk && (
<React.Suspense fallback={null}>
<RSCPayloadContainer
chunkIndex={chunkIndex + 1}
getChunkPromise={getChunkPromise}
/>
</React.Suspense>
)}
</>
);
};3. Use a template element (modern approach)
const RSCPayloadContainer = (
{ chunkIndex, getChunkPromise }: RSCPayloadContainerInnerProps,
): React.ReactNode => {
const chunkPromise = getChunkPromise(chunkIndex);
const chunk = React.use(chunkPromise);
// Template elements aren't rendered but preserve their content
const templateElement = React.createElement('template', {
id: `flight-data-${chunkIndex}`,
key: `template-${chunkIndex}`,
}, chunk.chunk);
// Process the data
React.useEffect(() => {
const templateEl = document.getElementById(`flight-data-${chunkIndex}`) as HTMLTemplateElement;
if (templateEl && templateEl.innerHTML) {
(window.__FLIGHT_DATA || (window.__FLIGHT_DATA = [])).push(
JSON.parse(templateEl.innerHTML)
);
}
}, [chunkIndex]);
return (
<>
{templateElement}
{!chunk.isLastChunk && (
<React.Suspense fallback={null}>
<RSCPayloadContainer
chunkIndex={chunkIndex + 1}
getChunkPromise={getChunkPromise}
/>
</React.Suspense>
)}
</>
);
};These approaches eliminate the XSS risk while still allowing you to deliver the RSC flight data to the client for hydration. The first option is likely the most standard approach for embedding non-executable data in HTML.
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
11c97ce to
3b95637
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
node_package/src/ClientSideRenderer.ts (1)
17-23:⚠️ Potential issueFix return type mismatch in delegateToRenderer function
The function signature has been changed from returning
Promise<boolean>toboolean, but the implementation still contains asynchronous calls. This is causing multiple pipeline failures across different builds.-function delegateToRenderer( +async function delegateToRenderer( componentObj: RegisteredComponent, props: Record<string, unknown>, railsContext: RailsContext, domNodeId: string, trace: boolean, -): boolean { +): Promise<boolean> {🧰 Tools
🪛 GitHub Check: build
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.🪛 GitHub Check: examples (oldest)
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.🪛 GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.🪛 GitHub Check: examples (newest)
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.🪛 GitHub Actions: Lint JS and Ruby
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: Generator tests
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 23-23: TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
[error] 23-23: TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: Main test
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
♻️ Duplicate comments (1)
node_package/src/streamServerRenderedReactComponent.ts (1)
234-236:⚠️ Potential issueError message needs updating to reflect promise support.
The error message still mentions that "stream rendering is not supported for server render hashes or promises" even though promises are now supported. This should be updated to only mention server render hashes.
- throw new Error('Server rendering of streams is not supported for server render hashes or promises.'); + throw new Error('Server rendering of streams is not supported for server render hashes.');
🧹 Nitpick comments (9)
node_package/src/registerServerComponent/server.rsc.ts (1)
22-26: LGTM with minor formatting issuesThe function implementation correctly delegates to ReactOnRails.register. There are some minor formatting issues that ESLint has flagged:
- Remove trailing whitespace on lines 6 and 11
- Adjust function parameter formatting
Consider fixing these formatting issues to keep the codebase consistent:
-const registerServerComponent = ( - components: { [id: string]: ReactComponent | RenderFunction }, -) => ReactOnRails.register(components); +const registerServerComponent = (components: { [id: string]: ReactComponent | RenderFunction }) => + ReactOnRails.register(components);🧰 Tools
🪛 ESLint
[error] 22-24: Replace
⏎··components:·{·[id:·string]:·ReactComponent·|·RenderFunction·},⏎)·=>withcomponents:·{·[id:·string]:·ReactComponent·|·RenderFunction·})·=>⏎·(prettier/prettier)
node_package/src/buildConsoleReplay.ts (1)
54-54: Formatting inconsistency in type declarationThe type declaration changed from
(typeof console)['history']totypeof console['history']. This change is causing ESLint warnings.- customConsoleHistory: typeof console['history'] | undefined = undefined, + customConsoleHistory: (typeof console)['history'] | undefined = undefined,🧰 Tools
🪛 ESLint
[error] 54-54: Replace
typeof·consolewith(typeof·console)(prettier/prettier)
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts (1)
33-33: Consider formatting the error message for better readabilityThe error message line is quite long. Consider breaking it into multiple lines for improved readability.
- throw new Error(`Error transforming RSC stream (${stream.constructor.name}), (stream: ${stream}), stringified stream: ${JSON.stringify(stream)}, error: ${error}`); + throw new Error( + `Error transforming RSC stream (${stream.constructor.name}), ` + + `(stream: ${stream}), ` + + `stringified stream: ${JSON.stringify(stream)}, ` + + `error: ${error}` + );🧰 Tools
🪛 ESLint
[error] 33-33: Replace
Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error}with⏎······Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error},⏎····(prettier/prettier)
node_package/src/ReactOnRails.client.ts (2)
121-121: Fix the options interface formattingThe parameter type formatting should use semicolons to separate properties to be consistent with TypeScript interfaces.
- setOptions(newOptions: {traceTurbolinks?: boolean, turbo?: boolean }): void { + setOptions(newOptions: {traceTurbolinks?: boolean; turbo?: boolean }): void {🧰 Tools
🪛 ESLint
[error] 121-121: Replace
traceTurbolinks?:·boolean,with·traceTurbolinks?:·boolean;(prettier/prettier)
342-342: Consider using object spread syntax instead of Object.assignModern JavaScript typically uses the object spread syntax for better readability.
- this.options = Object.assign({}, DEFAULT_OPTIONS); + this.options = { ...DEFAULT_OPTIONS };🧰 Tools
🪛 ESLint
[error] 342-342: Use an object spread instead of
Object.assigneg:{ ...foo }.(prefer-object-spread)
node_package/src/RSCServerRoot.ts (4)
1-7: Fix import order.The
streamimport should occur before the type import according to standard import ordering rules.import * as React from 'react'; import { createFromNodeStream } from 'react-on-rails-rsc/client.node'; +import { PassThrough } from 'stream'; import type { RenderFunction, RailsContext } from './types'; import transformRSCStream from './transformRSCNodeStreamAndReplayConsoleLogs'; import loadJsonFile from './loadJsonFile'; import RSCPayloadContainer from './RSCPayloadContainer'; -import { PassThrough } from 'stream';🧰 Tools
🪛 ESLint
[error] 7-7:
streamimport should occur before type import of./types(import/order)
22-24: Good React feature check, but fix error message formatting.The code correctly checks for React.use support, which is necessary for server components, but the error message formatting should be improved.
if (!('use' in React && typeof React.use === 'function')) { - throw new Error('React.use is not defined. Please ensure you are using React 18 with experimental features enabled or React 19+ to use server components.'); + throw new Error( + 'React.use is not defined. Please ensure you are using React 18 with experimental features enabled or React 19+ to use server components.' + ); }🧰 Tools
🪛 ESLint
[error] 23-23: Replace
'React.use·is·not·defined.·Please·ensure·you·are·using·React·18·with·experimental·features·enabled·or·React·19+·to·use·server·components.'with⏎····'React.use·is·not·defined.·Please·ensure·you·are·using·React·18·with·experimental·features·enabled·or·React·19+·to·use·server·components.',⏎··(prettier/prettier)
88-93: Ensure stream piping doesn't leak resources.The code creates two PassThrough streams and pipes the RSC payload stream to both, but there's no explicit error handling for potential pipe failures.
// Tee the stream to pass it to the server component and the payload container const rscPayloadStream1 = new PassThrough(); -rscPayloadStream.pipe(rscPayloadStream1); +rscPayloadStream.pipe(rscPayloadStream1).on('error', (err) => { + console.error('Error in RSC payload stream piping:', err); +}); const rscPayloadStream2 = new PassThrough(); -rscPayloadStream.pipe(rscPayloadStream2); +rscPayloadStream.pipe(rscPayloadStream2).on('error', (err) => { + console.error('Error in RSC payload stream piping:', err); +});
94-108: Use React.Fragment shorthand for better readability.The code correctly returns a function that renders both the server component and payload container, but could be simplified using the React.Fragment shorthand syntax.
return () => React.createElement( - React.Fragment, - null, [ + <>, + [ React.createElement( React.Fragment, { key: 'serverComponentElement' }, use(serverComponentElement) ), React.createElement( RSCPayloadContainer, { RSCPayloadStream: rscPayloadStream2, key: 'rscPayloadContainer' }, ), - ] + ] + </> );Note: If you prefer to avoid JSX syntax and want to keep using
React.createElement, you could still clean up the formatting for readability.🧰 Tools
🪛 ESLint
[error] 95-97: Replace
·React.createElement(⏎····React.Fragment,⏎···with⏎····React.createElement(React.Fragment,(prettier/prettier)
[error] 98-102: Replace
⏎········React.Fragment,⏎········{·key:·'serverComponentElement'·},⏎········use(serverComponentElement)⏎······withReact.Fragment,·{·key:·'serverComponentElement'·},·use(serverComponentElement)(prettier/prettier)
[error] 103-104: Replace
⏎········RSCPayloadContainer,withRSCPayloadContainer,·{(prettier/prettier)
[error] 105-105: Replace
·{·RSCPayloadStream:·rscPayloadStream2,·key:·'rscPayloadContainer'·}with·RSCPayloadStream:·rscPayloadStream2,⏎········key:·'rscPayloadContainer'(prettier/prettier)
[error] 106-106: Insert
}(prettier/prettier)
[error] 107-108: Delete
⏎··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
-
lib/react_on_rails/configuration.rb(6 hunks) -
lib/react_on_rails/helper.rb(2 hunks) -
lib/react_on_rails/test_helper/webpack_assets_status_checker.rb(1 hunks) -
lib/react_on_rails/utils.rb(1 hunks) -
node_package/src/ClientSideRenderer.ts(1 hunks) -
node_package/src/ComponentRegistry.ts(1 hunks) -
node_package/src/RSCClientRoot.ts(4 hunks) -
node_package/src/RSCPayloadContainer.ts(1 hunks) -
node_package/src/RSCServerRoot.ts(1 hunks) -
node_package/src/ReactOnRails.client.ts(3 hunks) -
node_package/src/ReactOnRailsRSC.ts(3 hunks) -
node_package/src/buildConsoleReplay.ts(1 hunks) -
node_package/src/createReactOutput.ts(2 hunks) -
node_package/src/handleError.ts(1 hunks) -
node_package/src/loadJsonFile.ts(1 hunks) -
node_package/src/loadReactClientManifest.ts(0 hunks) -
node_package/src/registerServerComponent/server.rsc.ts(1 hunks) -
node_package/src/registerServerComponent/server.ts(2 hunks) -
node_package/src/serverRenderReactComponent.ts(2 hunks) -
node_package/src/streamServerRenderedReactComponent.ts(5 hunks) -
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks) -
node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(2 hunks) -
node_package/src/types/index.ts(4 hunks) -
node_package/tests/serverRenderReactComponent.test.js(2 hunks) -
node_package/tests/streamServerRenderedReactComponent.test.jsx(2 hunks) -
package.json(2 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/loadReactClientManifest.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/react_on_rails/helper.rb
- node_package/src/handleError.ts
- node_package/tests/serverRenderReactComponent.test.js
- node_package/src/loadJsonFile.ts
- lib/react_on_rails/utils.rb
- lib/react_on_rails/test_helper/webpack_assets_status_checker.rb
🧰 Additional context used
🧬 Code Definitions (4)
node_package/tests/streamServerRenderedReactComponent.test.jsx (4)
lib/react_on_rails/react_component/render_options.rb (1)
props(26-28)spec/dummy/client/app/store/composeInitialState.js (1)
props(3-3)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx (1)
props(16-50)node_package/src/ReactOnRails.client.ts (1)
streamServerRenderedReactComponent(287-291)
node_package/src/streamServerRenderedReactComponent.ts (3)
node_package/src/buildConsoleReplay.ts (1)
buildConsoleReplay(53-62)node_package/src/serverRenderUtils.ts (2)
createResultObject(3-18)convertToError(20-22)node_package/src/types/index.ts (1)
RenderParams(142-146)
node_package/src/ReactOnRailsRSC.ts (3)
node_package/src/types/index.ts (2)
RSCRenderParams(148-150)StreamRenderState(226-229)node_package/src/streamServerRenderedReactComponent.ts (1)
transformRenderStreamChunksToResultObject(86-147)node_package/src/loadJsonFile.ts (1)
loadJsonFile(7-25)
node_package/src/registerServerComponent/server.rsc.ts (1)
node_package/src/types/index.ts (3)
ReactComponent(104-104)RenderFunction(106-106)ReactOnRails(185-218)
🪛 GitHub Check: build
node_package/src/ComponentRegistry.ts
[failure] 23-23:
Cannot find name 'RenderFunction'.
node_package/src/ClientSideRenderer.ts
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Check: examples (oldest)
node_package/src/ComponentRegistry.ts
[failure] 23-23:
Cannot find name 'RenderFunction'.
node_package/src/ClientSideRenderer.ts
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
node_package/src/ComponentRegistry.ts
[failure] 23-23:
Cannot find name 'RenderFunction'.
node_package/src/ClientSideRenderer.ts
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Check: examples (newest)
node_package/src/ComponentRegistry.ts
[failure] 23-23:
Cannot find name 'RenderFunction'.
node_package/src/ClientSideRenderer.ts
[failure] 23-23:
Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: JS unit tests for Renderer package
node_package/src/ComponentRegistry.ts
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
node_package/src/ClientSideRenderer.ts
[error] 23-23: TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
[error] 23-23: TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 ESLint
node_package/src/createReactOutput.ts
[error] 19-19: Insert ······
(prettier/prettier)
[error] 22-22: Insert ,⏎····
(prettier/prettier)
[error] 87-87: Insert ;
(prettier/prettier)
node_package/tests/streamServerRenderedReactComponent.test.jsx
[error] 210-210: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 228-228: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 242-242: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 260-260: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 282-282: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 316-316: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
node_package/src/registerServerComponent/server.ts
[error] 7-7: Delete ·
(prettier/prettier)
[error] 10-10: Delete ·
(prettier/prettier)
node_package/src/ClientSideRenderer.ts
[error] 24-24: 'name' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
node_package/src/RSCPayloadContainer.ts
[error] 6-6: Insert ;
(prettier/prettier)
[error] 10-10: Insert ;
(prettier/prettier)
[error] 15-15: Insert ;
(prettier/prettier)
[error] 18-20: Replace ⏎····.replace(/<!--/g,·'<\\!--')⏎···· with .replace(/<!--/g,·'<\\!--')
(prettier/prettier)
[error] 23-23: Insert {
(prettier/prettier)
[error] 24-25: Replace ·{·chunkIndex,·getChunkPromise·}:·RSCPayloadContainerInnerProps,⏎ with ·chunkIndex,⏎··getChunkPromise,⏎}:·RSCPayloadContainerInnerProps
(prettier/prettier)
[error] 45-48: Replace ⏎········RSCPayloadContainer,⏎········{·chunkIndex:·chunkIndex·+·1,·getChunkPromise·},⏎······ with RSCPayloadContainer,·{·chunkIndex:·chunkIndex·+·1,·getChunkPromise·}
(prettier/prettier)
[error] 51-51: Insert ;
(prettier/prettier)
[error] 87-87: Insert ⏎····
(prettier/prettier)
[error] 88-88: Replace ···· with ······
(prettier/prettier)
[error] 89-89: Insert ··
(prettier/prettier)
[error] 90-90: Insert ··
(prettier/prettier)
[error] 92-92: Replace ···· with ······
(prettier/prettier)
[error] 93-93: Replace },·[chunkPromises] with ··},⏎····[chunkPromises],⏎··
(prettier/prettier)
node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
[error] 3-3: Replace stream:·ReadableStream<Uint8Array·|·RenderResult> with ⏎··stream:·ReadableStream<Uint8Array·|·RenderResult>,⏎
(prettier/prettier)
[error] 12-12: Delete ······
(prettier/prettier)
[error] 17-17: Replace .trim().replace(/^<script.*>/,·'') with ⏎··········.trim()⏎··········.replace(/^<script.*>/,·'')⏎··········
(prettier/prettier)
[error] 24-24: Delete ······
(prettier/prettier)
[error] 32-32: Replace line with (line)
(prettier/prettier)
node_package/src/streamServerRenderedReactComponent.ts
[error] 99-99: Insert ;
(prettier/prettier)
[error] 149-149: Replace reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,·options:·RenderParams with ⏎··reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,⏎··options:·RenderParams,⏎
(prettier/prettier)
[error] 172-172: Insert ;
(prettier/prettier)
[error] 174-174: Replace .then(reactRenderedElement with ⏎····.then((reactRenderedElement)
(prettier/prettier)
[error] 175-175: Insert ··
(prettier/prettier)
[error] 176-176: Insert ··
(prettier/prettier)
[error] 177-177: Insert ··
(prettier/prettier)
[error] 178-179: Replace 'To·benefit·from·React·on·Rails·Pro·streaming·feature,·your·render·function·should·return·a·React·component.\n'·+·⏎········'Do·not·call·ReactDOMServer.renderToString()·inside·the·render·function·as·this·defeats·the·purpose·of·streaming.\n' with ····'To·benefit·from·React·on·Rails·Pro·streaming·feature,·your·render·function·should·return·a·React·component.\n'·+⏎············'Do·not·call·ReactDOMServer.renderToString()·inside·the·render·function·as·this·defeats·the·purpose·of·streaming.\n',
(prettier/prettier)
[error] 180-180: Insert ··
(prettier/prettier)
[error] 182-182: Replace ······ with ········
(prettier/prettier)
[error] 183-183: Insert ··
(prettier/prettier)
[error] 184-184: Insert ··
(prettier/prettier)
[error] 185-185: Insert ··
(prettier/prettier)
[error] 187-187: Insert ··
(prettier/prettier)
[error] 188-188: Insert ··
(prettier/prettier)
[error] 189-189: Insert ··
(prettier/prettier)
[error] 190-190: Insert ··
(prettier/prettier)
[error] 191-191: Insert ··
(prettier/prettier)
[error] 192-192: Insert ··
(prettier/prettier)
[error] 193-193: Insert ··
(prettier/prettier)
[error] 194-194: Insert ··
(prettier/prettier)
[error] 195-195: Insert ··
(prettier/prettier)
[error] 196-196: Replace ········ with ··········
(prettier/prettier)
[error] 197-197: Insert ··
(prettier/prettier)
[error] 198-198: Replace ········ with ··········
(prettier/prettier)
[error] 199-199: Insert ··
(prettier/prettier)
[error] 200-200: Replace ········ with ··········
(prettier/prettier)
[error] 201-201: Insert ··
(prettier/prettier)
[error] 202-202: Insert ··
(prettier/prettier)
[error] 203-203: Insert ··
(prettier/prettier)
[error] 204-204: Insert ··
(prettier/prettier)
[error] 205-205: Insert ··
(prettier/prettier)
[error] 206-206: Replace ··}) with ····})⏎····
(prettier/prettier)
[error] 240-245: Replace ⏎······readableStream,⏎······writeChunk,⏎······emitError,⏎······endStream⏎····}·=·transformRenderStreamChunksToResultObject({·hasErrors:·true,·isShellReady:·false,·result:·null with ·readableStream,·writeChunk,·emitError,·endStream·}·=·transformRenderStreamChunksToResultObject({⏎······hasErrors:·true,⏎······isShellReady:·false,⏎······result:·null,⏎···
(prettier/prettier)
node_package/src/ReactOnRailsRSC.ts
[error] 24-24: Replace reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,·options:·RSCRenderParams with ⏎··reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,⏎··options:·RSCRenderParams,⏎
(prettier/prettier)
[error] 37-37: Insert ⏎···
(prettier/prettier)
node_package/src/RSCClientRoot.ts
[error] 65-65: Insert ,
(prettier/prettier)
[error] 77-77: Insert ;
(prettier/prettier)
[error] 83-83: Insert ;
(prettier/prettier)
[error] 114-114: Insert ;
(prettier/prettier)
node_package/src/RSCServerRoot.ts
[error] 7-7: stream import should occur before type import of ./types
(import/order)
[error] 20-20: Insert ;
(prettier/prettier)
[error] 23-23: Replace 'React.use·is·not·defined.·Please·ensure·you·are·using·React·18·with·experimental·features·enabled·or·React·19+·to·use·server·components.' with ⏎····'React.use·is·not·defined.·Please·ensure·you·are·using·React·18·with·experimental·features·enabled·or·React·19+·to·use·server·components.',⏎··
(prettier/prettier)
[error] 28-28: Replace stream:·NodeJS.ReadableStream,·ssrManifest:·Record<string,·unknown> with ⏎··stream:·NodeJS.ReadableStream,⏎··ssrManifest:·Record<string,·unknown>,⏎
(prettier/prettier)
[error] 31-31: Insert ;
(prettier/prettier)
[error] 33-33: Replace reactServerManifestFileName:·string,·reactClientManifestFileName:·string with ⏎··reactServerManifestFileName:·string,⏎··reactClientManifestFileName:·string,⏎
(prettier/prettier)
[error] 41-41: Replace "/webpack/development/" with '/webpack/development/'
(prettier/prettier)
[error] 54-54: Insert ,
(prettier/prettier)
[error] 59-59: Insert ;
(prettier/prettier)
[error] 61-61: Replace {·componentName,·componentProps·}:·RSCServerRootProps,·railsContext?:·RailsContext with ⏎··{·componentName,·componentProps·}:·RSCServerRootProps,⏎··railsContext?:·RailsContext,⏎
(prettier/prettier)
[error] 62-62: Replace !railsContext?.serverSide·||·!railsContext?.reactClientManifestFileName·||·!railsContext?.reactServerClientManifestFileName with ⏎····!railsContext?.serverSide·||⏎····!railsContext?.reactClientManifestFileName·||⏎····!railsContext?.reactServerClientManifestFileName⏎··
(prettier/prettier)
[error] 64-64: Insert ⏎········
(prettier/prettier)
[error] 65-65: Replace ······ with ········
(prettier/prettier)
[error] 66-66: Replace 'as·stated·in·the·React·Server·Component·tutorial.·The·received·rails·context·is:·'}${··JSON.stringify(railsContext)}`` with ··'as·stated·in·the·React·Server·Component·tutorial.·The·received·rails·context·is:·'⏎······}${JSON.stringify(railsContext)},
(prettier/prettier)
[error] 73-73: Replace ······ with ········
(prettier/prettier)
[error] 74-74: Replace 'is·set·to·true.' with ··'is·set·to·true.',
(prettier/prettier)
[error] 80-80: Insert ,
(prettier/prettier)
[error] 85-85: Insert ,
(prettier/prettier)
[error] 95-97: Replace ·React.createElement(⏎····React.Fragment,⏎··· with ⏎····React.createElement(React.Fragment,
(prettier/prettier)
[error] 98-102: Replace ⏎········React.Fragment,⏎········{·key:·'serverComponentElement'·},⏎········use(serverComponentElement)⏎······ with React.Fragment,·{·key:·'serverComponentElement'·},·use(serverComponentElement)
(prettier/prettier)
[error] 103-104: Replace ⏎········RSCPayloadContainer, with RSCPayloadContainer,·{
(prettier/prettier)
[error] 105-105: Replace ·{·RSCPayloadStream:·rscPayloadStream2,·key:·'rscPayloadContainer'·} with ·RSCPayloadStream:·rscPayloadStream2,⏎········key:·'rscPayloadContainer'
(prettier/prettier)
[error] 106-106: Insert }
(prettier/prettier)
[error] 107-108: Delete ⏎··
(prettier/prettier)
node_package/src/ReactOnRails.client.ts
[error] 121-121: Replace traceTurbolinks?:·boolean, with ·traceTurbolinks?:·boolean;
(prettier/prettier)
[error] 137-139: Replace ⏎········Invalid·options·passed·to·ReactOnRails.options:·${JSON.stringify(newOptions)},⏎······ with Invalid·options·passed·to·ReactOnRails.options:·${JSON.stringify(newOptions)}
(prettier/prettier)
[error] 342-342: Use an object spread instead of Object.assign eg: { ...foo }.
(prefer-object-spread)
node_package/src/buildConsoleReplay.ts
[error] 54-54: Replace typeof·console with (typeof·console)
(prettier/prettier)
node_package/src/registerServerComponent/server.rsc.ts
[error] 6-6: Delete ·
(prettier/prettier)
[error] 11-11: Delete ·
(prettier/prettier)
[error] 22-24: Replace ⏎··components:·{·[id:·string]:·ReactComponent·|·RenderFunction·},⏎)·=> with components:·{·[id:·string]:·ReactComponent·|·RenderFunction·})·=>⏎·
(prettier/prettier)
node_package/src/serverRenderReactComponent.ts
[error] 61-61: Replace 'Your·render·function·returned·a·Promise,·which·is·only·supported·by·a·node·renderer,·not·ExecJS.' with ⏎······'Your·render·function·returned·a·Promise,·which·is·only·supported·by·a·node·renderer,·not·ExecJS.',⏎····
(prettier/prettier)
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts
[error] 33-33: Replace Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error} with ⏎······Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error},⏎····
(prettier/prettier)
node_package/src/types/index.ts
[error] 35-35: Insert ⏎··|·
(prettier/prettier)
[error] 36-36: Insert ····
(prettier/prettier)
[error] 37-37: Replace } with ····}⏎·
(prettier/prettier)
[error] 38-38: Insert ····
(prettier/prettier)
[error] 39-39: Insert ····
(prettier/prettier)
[error] 40-40: Insert ····
(prettier/prettier)
[error] 41-41: Insert ····
(prettier/prettier)
[error] 42-42: Insert ····
(prettier/prettier)
[error] 43-43: Insert ····
(prettier/prettier)
[error] 44-44: Insert ····
(prettier/prettier)
[error] 45-45: Insert ····
(prettier/prettier)
[error] 46-46: Insert ····
(prettier/prettier)
[error] 47-47: Insert ····
(prettier/prettier)
[error] 48-48: Replace } with ····}⏎
(prettier/prettier)
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/ClientSideRenderer.ts
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: Generator tests
node_package/src/ClientSideRenderer.ts
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 GitHub Actions: Main test
node_package/src/ClientSideRenderer.ts
[error] 23-23: error TS1055: Type 'boolean' is not a valid async function return type in ES5 because it does not refer to a Promise-compatible constructor value.
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (56)
lib/react_on_rails/configuration.rb (6)
13-13: LGTM: Added constant for the React Server Client Manifest fileThis adds a new constant for the default React Server Client Manifest file name, consistent with existing naming conventions.
25-25: LGTM: Added default configuration for React Server Client ManifestThe default configuration value is properly set using the constant defined earlier.
70-70: LGTM: Added attribute accessor for React Server Client ManifestThe new configuration property has been properly added to the attribute accessors list.
86-87: LGTM: Updated method signature to include new parameterThe initialize method has been properly extended to support the new configuration parameter.
117-117: LGTM: Added initialization for React Server Client ManifestThe instance variable is properly set from the constructor parameter, consistent with other configuration properties.
269-275: LGTM: Enhanced webpack generated files handlingThe method now correctly includes the React Server Client Manifest file in the webpack generated files list when it's present, following the same pattern as other manifest files.
node_package/src/registerServerComponent/server.rsc.ts (2)
1-3: LGTM: Proper imports for the new moduleThe imports correctly bring in the ReactOnRails client and necessary type definitions.
4-21: LGTM: Well-documented function with clear JSDocThe JSDoc comment clearly explains the purpose of this function, how it differs from the server bundle registration, and includes a helpful usage example.
🧰 Tools
🪛 ESLint
[error] 6-6: Delete
·(prettier/prettier)
[error] 11-11: Delete
·(prettier/prettier)
node_package/src/buildConsoleReplay.ts (1)
57-62: LGTM: Improved handling of empty console replayGood optimization that avoids adding empty script tags when there's no console output to replay.
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts (1)
1-35: Implementation for transforming RSC streams looks solidThis new utility function transforms a Node.js ReadableStream containing JSON chunks into an HTML stream by extracting the
htmlproperty from each JSON object. The implementation correctly handles:
- Accumulating incomplete chunks
- Processing multiple newline-separated JSON objects
- Error propagation and detailed error messages
The stream transformation approach is efficient for handling React Server Component payloads.
🧰 Tools
🪛 ESLint
[error] 33-33: Replace
Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error}with⏎······Error·transforming·RSC·stream·(${stream.constructor.name}),·(stream:·${stream}),·stringified·stream:·${JSON.stringify(stream)},·error:·${error},⏎····(prettier/prettier)
package.json (1)
15-18: Good addition of conditional exports for RSC supportThe change to define a conditional export for
./registerServerComponent/serveradds proper support for React Server Components by exposing a different entry point for the "react-server" condition. This is consistent with modern React Server Components architecture.node_package/src/registerServerComponent/server.ts (3)
1-4: Appropriate import of RSCServerRoot for server component renderingThe addition of the RSCServerRoot import and type imports are necessary for the new implementation.
5-25: Well-documented component wrapper functionalityThe updated documentation clearly explains how RSCServerRoot handles server-side rendering of React Server Components using pre-generated RSC payloads, which helps developers understand the changes in behavior.
🧰 Tools
🪛 ESLint
[error] 7-7: Delete
·(prettier/prettier)
[error] 10-10: Delete
·(prettier/prettier)
26-35: Good implementation of component wrapping for RSC supportThe implementation now wraps each registered component with RSCServerRoot, enabling proper server-side rendering of React Server Components. This approach effectively separates the component definition from its server rendering logic.
However, there are a few minor whitespace issues in the JSDoc comments that the linter has flagged.
node_package/src/ReactOnRails.client.ts (2)
137-140: Improved error message formattingThe error message is now more readable with the multi-line formatting.
🧰 Tools
🪛 ESLint
[error] 137-139: Replace
⏎········Invalid·options·passed·to·ReactOnRails.options:·${JSON.stringify(newOptions)},⏎······withInvalid·options·passed·to·ReactOnRails.options:·${JSON.stringify(newOptions)}(prettier/prettier)
344-345: Good addition of isRSCBundle flagAdding the
isRSCBundleflag set tofalseby default establishes the baseline for the client bundle. This will help differentiate between client and RSC bundles in the codebase.node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (3)
3-3: Updated function signature to support RSC payload renderingThe function now accepts a
ReadableStreamof eitherUint8ArrayorRenderResult. This change enhances the component's ability to handle both binary data and structured rendering results, which is crucial for the PR's goal of using RSC payload for server component rendering.🧰 Tools
🪛 ESLint
[error] 3-3: Replace
stream:·ReadableStream<Uint8Array·|·RenderResult>with⏎··stream:·ReadableStream<Uint8Array·|·RenderResult>,⏎(prettier/prettier)
13-23: Good extraction of JSON chunk handling into a separate functionExtracting this logic improves code organization and readability. The function handles both HTML content and console replay scripts appropriately.
🧰 Tools
🪛 ESLint
[error] 17-17: Replace
.trim().replace(/^<script.*>/,·'')with⏎··········.trim()⏎··········.replace(/^<script.*>/,·'')⏎··········(prettier/prettier)
25-47: Enhanced stream handling logic to support both Uint8Array and RenderResultThe updated code now differentiates between ArrayBuffer views (binary data) and direct RenderResult objects, enabling the dual functionality needed for RSC support. This implementation properly handles both cases while maintaining backward compatibility.
🧰 Tools
🪛 ESLint
[error] 32-32: Replace
linewith(line)(prettier/prettier)
node_package/src/ClientSideRenderer.ts (1)
29-29: Improved logging consistencyUsing
componentObj.nameinstead ofnameimproves logging consistency. This is a good change.node_package/src/createReactOutput.ts (3)
11-29: Good extraction of React element creation logicCreating a separate function for handling React elements is a good refactoring that improves code maintainability. The function properly handles both direct React elements and component functions.
Consider fixing these minor formatting issues:
export function createReactElementFromRenderFunction( renderFunctionResult: ReactComponent, name: string, props: Record<string, unknown> | undefined, ): React.ReactElement { if (React.isValidElement(renderFunctionResult)) { // If already a ReactElement, then just return it. console.error( -`Warning: ReactOnRails: Your registered render-function (ReactOnRails.register) for ${name} + `Warning: ReactOnRails: Your registered render-function (ReactOnRails.register) for ${name} incorrectly returned a React Element (JSX). Instead, return a React Function Component by wrapping your JSX in a function. ReactOnRails v13 will throw error on this, as React Hooks do not -work if you return JSX. Update by wrapping the result JSX of ${name} in a fat arrow function.`); +work if you return JSX. Update by wrapping the result JSX of ${name} in a fat arrow function.`, + ); return renderFunctionResult; } // If a component, then wrap in an element const reactComponent = renderFunctionResult as ReactComponent; return React.createElement(reactComponent, props); }🧰 Tools
🪛 ESLint
[error] 19-19: Insert
······(prettier/prettier)
[error] 22-22: Insert
,⏎····(prettier/prettier)
85-89: Enhanced promise handling for render function resultsThe code now properly handles promises that resolve to either strings or React components, providing better support for asynchronous rendering. This is a necessary improvement for RSC support.
Fix the missing semicolon:
return (renderFunctionResult as Promise<string | ReactComponent>).then((result) => { if (typeof result === 'string') return result; - return createReactElementFromRenderFunction(result, name, props) + return createReactElementFromRenderFunction(result, name, props); });🧰 Tools
🪛 ESLint
[error] 87-87: Insert
;(prettier/prettier)
91-91: Improved consistency in React element creationUsing the new utility function here ensures consistent handling of React elements across the codebase.
node_package/tests/streamServerRenderedReactComponent.test.jsx (5)
63-97: Comprehensive test setup for different component typesThe
setupStreamTestfunction has been enhanced to support different component registration strategies, making it more versatile for testing various component types. This is a good improvement for test coverage.
206-222: New tests for render functions that resolve to React componentsThese tests validate that both synchronous and asynchronous render functions that return React components work correctly with the streaming functionality. They appropriately check for HTML content, console scripts, and error states.
🧰 Tools
🪛 ESLint
[error] 210-210: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
224-254: Added tests for handling errors in render functionsThese tests verify that both synchronous and asynchronous errors in render functions are properly handled. They check that error messages are included in the output and that the error state is correctly set.
🧰 Tools
🪛 ESLint
[error] 228-228: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 242-242: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
256-297: Remove.onlyto avoid skipping other tests
.onlywill cause Jest to skip all other tests. Remember to remove.onlybefore merging, or your CI might miss regressions.- it.only.each(['erroneousRenderFunction', 'erroneousAsyncRenderFunction'])( + it.each(['erroneousRenderFunction', 'erroneousAsyncRenderFunction'])(🧰 Tools
🪛 ESLint
[error] 260-260: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 282-282: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
299-322: New test for string results from promisesThis test ensures that promises resolving to strings are properly handled in the streaming component. This is important for supporting different types of rendering results.
🧰 Tools
🪛 ESLint
[error] 316-316: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
node_package/src/RSCClientRoot.ts (6)
3-4: Good use of disabling underscore-dangle for internal usage.Since
__FLIGHT_DATAis user-defined, disabling the rule is justified.
18-23: Global__FLIGHT_DATAproperty looks correct.Declaring this property on the window object ensures it's accessible for client-side streaming logic.
79-83: Stream creation and transformation approach is solid.The function neatly composes the stream, transforms it, and returns a React-friendly stream consumer for RSC data.
🧰 Tools
🪛 ESLint
[error] 83-83: Insert
;(prettier/prettier)
111-111: Hydration path for non-empty DOM content appears correct.Using
hydrateRootensures React reuses existing server-rendered markup.
114-114: Fallback path for empty DOM content is properly handled.Falling back to
fetchRSCfor initial rendering is a sensible approach.🧰 Tools
🪛 ESLint
[error] 114-114: Insert
;(prettier/prettier)
46-77:❓ Verification inconclusive
Confirm streaming closure timing to avoid missed chunks.
When the DOM is ready (or if it’s already loaded), the stream is closed. If new chunks arrive after the stream closes, they could be lost. Please confirm if this aligns with the intended design.
🏁 Script executed:
#!/bin/bash # Description: Verify usage of `createRSCStreamFromPage` to ensure no leftover chunks arrive post-closure. rg -A 15 "createRSCStreamFromPage"Length of output: 2377
Confirm Stream Closure Timing in
createRSCStreamFromPageThe implementation currently closes the stream when the DOM is ready—either immediately if the document has already loaded or upon the
DOMContentLoadedevent if it is still loading. This means that any chunks pushed towindow.__FLIGHT_DATAafter the stream is closed would be lost. Please confirm that this behavior is intentional and aligns with the design goals, or if there should be additional logic to handle any late-arriving chunks.🧰 Tools
🪛 ESLint
[error] 65-65: Insert
,(prettier/prettier)
[error] 77-77: Insert
;(prettier/prettier)
node_package/src/RSCPayloadContainer.ts (6)
3-6: Chunk type definition is straightforward.Describing each chunk’s content and last-chunk status clarifies the streaming model.
🧰 Tools
🪛 ESLint
[error] 6-6: Insert
;(prettier/prettier)
8-10: Props interface defined clearly for RSC payload streams.No issues noted; it cleanly represents external input.
🧰 Tools
🪛 ESLint
[error] 10-10: Insert
;(prettier/prettier)
12-15: Internal props interface is well-defined.Structuring the chunk index and promise retrieval function is clear.
🧰 Tools
🪛 ESLint
[error] 15-15: Insert
;(prettier/prettier)
17-21: Partial script escaping looks good, but confirm comprehensive sanitization.
escapeScriptmitigates simple script tags, but it might not handle all potential injection vectors. Consider a robust sanitizer if untrusted data can flow here.🧰 Tools
🪛 ESLint
[error] 18-20: Replace
⏎····.replace(/<!--/g,·'<\\!--')⏎····with.replace(/<!--/g,·'<\\!--')(prettier/prettier)
29-34: Potential XSS risk withdangerouslySetInnerHTML.This repeats a previously noted concern to confirm data is sanitized before injection.
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
53-100: Recursive Suspense structure effectively handles streaming.Promises array and event handlers seem correct. Ensure large payloads won’t cause memory issues with an ever-growing array.
🧰 Tools
🪛 ESLint
[error] 87-87: Insert
⏎····(prettier/prettier)
[error] 88-88: Replace
····with······(prettier/prettier)
[error] 89-89: Insert
··(prettier/prettier)
[error] 90-90: Insert
··(prettier/prettier)
[error] 92-92: Replace
····with······(prettier/prettier)
[error] 93-93: Replace
},·[chunkPromises]with··},⏎····[chunkPromises],⏎··(prettier/prettier)
node_package/src/ReactOnRailsRSC.ts (3)
24-28: Ensure the Rails context is set properly.Throwing an error if
railsContextorreactClientManifestFileNameis absent is helpful, but confirm that upstream code always sets these values.🧰 Tools
🪛 ESLint
[error] 24-24: Replace
reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,·options:·RSCRenderParamswith⏎··reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,⏎··options:·RSCRenderParams,⏎(prettier/prettier)
38-39: Concurrently loading manifest and rendering result is efficient.Combining promises is a good performance optimization. No issues here.
72-72:isRSCBundleflag is introduced successfully.Double-check that any subsequent logic depending on
isRSCBundleis updated accordingly.node_package/src/RSCServerRoot.ts (1)
61-76: Good error handling for missing context properties.The component correctly validates required context properties early and provides detailed error messages to help troubleshoot configuration issues.
🧰 Tools
🪛 ESLint
[error] 61-61: Replace
{·componentName,·componentProps·}:·RSCServerRootProps,·railsContext?:·RailsContextwith⏎··{·componentName,·componentProps·}:·RSCServerRootProps,⏎··railsContext?:·RailsContext,⏎(prettier/prettier)
[error] 62-62: Replace
!railsContext?.serverSide·||·!railsContext?.reactClientManifestFileName·||·!railsContext?.reactServerClientManifestFileNamewith⏎····!railsContext?.serverSide·||⏎····!railsContext?.reactClientManifestFileName·||⏎····!railsContext?.reactServerClientManifestFileName⏎··(prettier/prettier)
[error] 64-64: Insert
⏎········(prettier/prettier)
[error] 65-65: Replace
······with········(prettier/prettier)
[error] 66-66: Replace
'as·stated·in·the·React·Server·Component·tutorial.·The·received·rails·context·is:·'}${··JSON.stringify(railsContext)}`` with··'as·stated·in·the·React·Server·Component·tutorial.·The·received·rails·context·is:·'⏎······}${JSON.stringify(railsContext)},(prettier/prettier)
[error] 73-73: Replace
······with········(prettier/prettier)
[error] 74-74: Replace
'is·set·to·true.'with··'is·set·to·true.',(prettier/prettier)
node_package/src/serverRenderReactComponent.ts (2)
56-72: Good enhancement to handle React elements in promises.The
processPromisefunction has been updated to handle promises that resolve to either strings or React elements. This enhancement increases flexibility in component rendering and allows for more advanced use cases.The implementation correctly differentiates between string and React element results and processes them accordingly. Good error message for ExecJS renderer compatibility.
🧰 Tools
🪛 ESLint
[error] 61-61: Replace
'Your·render·function·returned·a·Promise,·which·is·only·supported·by·a·node·renderer,·not·ExecJS.'with⏎······'Your·render·function·returned·a·Promise,·which·is·only·supported·by·a·node·renderer,·not·ExecJS.',⏎····(prettier/prettier)
96-100: Type signature updated correctly for promise handling.The
createPromiseResultfunction signature has been updated to reflect the new promise type that can resolve to either a string or a React element, maintaining consistency with the changes toprocessPromise.node_package/src/types/index.ts (3)
17-48: Good use of discriminated union type for server/client context separation.The refactoring of
RailsContextfrom an interface to a discriminated union type is well-designed, providing clear separation between server-side and client-side contexts with their specific requirements. The detailed comments for the server-side properties are particularly helpful.This makes the type safer by ensuring that server-side specific properties are only available when
serverSideis true, preventing potential runtime errors.🧰 Tools
🪛 ESLint
[error] 35-35: Insert
⏎··|·(prettier/prettier)
[error] 36-36: Insert
····(prettier/prettier)
[error] 37-37: Replace
}with····}⏎·(prettier/prettier)
[error] 38-38: Insert
····(prettier/prettier)
[error] 39-39: Insert
····(prettier/prettier)
[error] 40-40: Insert
····(prettier/prettier)
[error] 41-41: Insert
····(prettier/prettier)
[error] 42-42: Insert
····(prettier/prettier)
[error] 43-43: Insert
····(prettier/prettier)
[error] 44-44: Insert
····(prettier/prettier)
[error] 45-45: Insert
····(prettier/prettier)
[error] 46-46: Insert
····(prettier/prettier)
[error] 47-47: Insert
····(prettier/prettier)
[error] 48-48: Replace
}with····}⏎(prettier/prettier)
65-67: Type enhancements for better promise support.The types
CreateReactOutputResultandRenderFunctionResulthave been updated to support promises that resolve to React elements, which aligns with the implementation changes inserverRenderReactComponent.ts. This ensures type safety when using these enhanced promise capabilities.
217-217: Added isRSCBundle property for RSC context identification.The new
isRSCBundleproperty in theReactOnRailsinterface will help identify when code is executing in an RSC bundle context, which is crucial for conditional logic that needs to behave differently in those environments.node_package/src/streamServerRenderedReactComponent.ts (7)
91-99: Good implementation of console replay chunk building.The extracted
buildConsoleReplayChunkfunction improves code organization and reusability. It efficiently handles console output capture and prevents sending empty replay scripts. The function correctly tracks previously replayed messages to avoid duplicates.🧰 Tools
🪛 ESLint
[error] 99-99: Insert
;(prettier/prettier)
108-115: Improved console replay with timeout and cleanup.Using a timeout to batch console replay messages is more efficient than sending them immediately with each chunk. The code correctly clears the previous timeout before setting a new one to prevent memory leaks.
119-126: Added flush method ensures all console logs are captured.The new
flushmethod ensures that any remaining console replay messages are sent before the stream ends, preventing loss of debugging information. This is an important addition for stream transformation.
149-149: Enhanced function signature for better promise support.The
streamRenderReactComponentfunction now accepts both React elements and promises that resolve to either React elements or strings, providing more flexibility in component rendering.🧰 Tools
🪛 ESLint
[error] 149-149: Replace
reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,·options:·RenderParamswith⏎··reactRenderingResult:·ReactElement·|·Promise<ReactElement·|·string>,⏎··options:·RenderParams,⏎(prettier/prettier)
160-172: Improved error handling with extracted function.Error handling logic has been extracted into a separate
onShellErrorfunction, which improves code readability and maintainability. The function correctly handles errors, updates the render state, and writes error HTML to the stream.🧰 Tools
🪛 ESLint
[error] 172-172: Insert
;(prettier/prettier)
174-206: Good promise handling with informative error messages.The code now uses
Promise.resolveto uniformly handle both React elements and promises. When a string is received instead of a React element, it provides a clear error message explaining why this is suboptimal for streaming and suggests not callingrenderToStringinside render functions.🧰 Tools
🪛 ESLint
[error] 174-174: Replace
.then(reactRenderedElementwith⏎····.then((reactRenderedElement)(prettier/prettier)
[error] 175-175: Insert
··(prettier/prettier)
[error] 176-176: Insert
··(prettier/prettier)
[error] 177-177: Insert
··(prettier/prettier)
[error] 178-179: Replace
'To·benefit·from·React·on·Rails·Pro·streaming·feature,·your·render·function·should·return·a·React·component.\n'·+·⏎········'Do·not·call·ReactDOMServer.renderToString()·inside·the·render·function·as·this·defeats·the·purpose·of·streaming.\n'with····'To·benefit·from·React·on·Rails·Pro·streaming·feature,·your·render·function·should·return·a·React·component.\n'·+⏎············'Do·not·call·ReactDOMServer.renderToString()·inside·the·render·function·as·this·defeats·the·purpose·of·streaming.\n',(prettier/prettier)
[error] 180-180: Insert
··(prettier/prettier)
[error] 182-182: Replace
······with········(prettier/prettier)
[error] 183-183: Insert
··(prettier/prettier)
[error] 184-184: Insert
··(prettier/prettier)
[error] 185-185: Insert
··(prettier/prettier)
[error] 187-187: Insert
··(prettier/prettier)
[error] 188-188: Insert
··(prettier/prettier)
[error] 189-189: Insert
··(prettier/prettier)
[error] 190-190: Insert
··(prettier/prettier)
[error] 191-191: Insert
··(prettier/prettier)
[error] 192-192: Insert
··(prettier/prettier)
[error] 193-193: Insert
··(prettier/prettier)
[error] 194-194: Insert
··(prettier/prettier)
[error] 195-195: Insert
··(prettier/prettier)
[error] 196-196: Replace
········with··········(prettier/prettier)
[error] 197-197: Insert
··(prettier/prettier)
[error] 198-198: Replace
········with··········(prettier/prettier)
[error] 199-199: Insert
··(prettier/prettier)
[error] 200-200: Replace
········with··········(prettier/prettier)
[error] 201-201: Insert
··(prettier/prettier)
[error] 202-202: Insert
··(prettier/prettier)
[error] 203-203: Insert
··(prettier/prettier)
[error] 204-204: Insert
··(prettier/prettier)
[error] 205-205: Insert
··(prettier/prettier)
[error] 206-206: Replace
··})with····})⏎····(prettier/prettier)
240-254: Improved error handling with stream utilities.The error handling in the catch block has been updated to use the stream transformation utilities, which provides consistent error reporting. The code correctly writes the error HTML to the stream and ends it properly.
🧰 Tools
🪛 ESLint
[error] 240-245: Replace
⏎······readableStream,⏎······writeChunk,⏎······emitError,⏎······endStream⏎····}·=·transformRenderStreamChunksToResultObject({·hasErrors:·true,·isShellReady:·false,·result:·nullwith·readableStream,·writeChunk,·emitError,·endStream·}·=·transformRenderStreamChunksToResultObject({⏎······hasErrors:·true,⏎······isShellReady:·false,⏎······result:·null,⏎···(prettier/prettier)
node_package/src/RSCServerRoot.ts
Outdated
| const rscPayloadStream = await generateRSCPayload( | ||
| componentName, | ||
| componentProps, | ||
| railsContext.serverSideRSCPayloadParameters | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for generateRSCPayload function.
The code correctly calls generateRSCPayload with the required parameters, but there's no error handling if the function throws an error.
Let's check how errors from this function are handled:
🏁 Script executed:
#!/bin/bash
# Search for similar async function calls in the codebase to see how they're handling errors
rg -A 5 -B 5 "await .*Payload" --type tsLength of output: 1546
Attention: Missing Error Handling for generateRSCPayload
It appears that the call to generateRSCPayload in node_package/src/RSCServerRoot.ts isn’t wrapped in any error handling. A similar pattern is observed in RSCClientRoot.ts where asynchronous payload-fetching functions are called without explicit error management. To make the code more robust, please consider introducing a try-catch block around the call. For example:
try {
const rscPayloadStream = await generateRSCPayload(
componentName,
componentProps,
railsContext.serverSideRSCPayloadParameters
);
} catch (error) {
// handle the error appropriately—log it, send a server error response, etc.
}This change will ensure that any errors thrown by generateRSCPayload are properly caught and managed.
🧰 Tools
🪛 ESLint
[error] 85-85: Insert ,
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
node_package/src/RSCPayloadContainer.ts (1)
28-33:⚠️ Potential issuePotential XSS vulnerability with dangerouslySetInnerHTML.
Using
dangerouslySetInnerHTMLto inject JavaScript poses a security risk, even with basic escaping.Consider alternatives:
- Store data in a safer format like a JSON attribute on a non-executable element
- Use a dedicated script loading mechanism with strict content validation
- const scriptElement = React.createElement('script', { - dangerouslySetInnerHTML: { - __html: escapeScript(`(self.__FLIGHT_DATA||=[]).push(${chunk.chunk})`), - }, - key: `script-${chunkIndex}`, - }); + // Option 1: Store data as a data attribute and process it safely + const dataElement = React.createElement('div', { + 'data-flight-chunk': JSON.stringify(chunk.chunk), + 'data-flight-index': chunkIndex, + key: `data-${chunkIndex}`, + id: `flight-data-${chunkIndex}`, + }); + + // Then use a separate initialization script that safely processes these elements🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
node_package/src/RSCServerRoot.ts (2)
38-67:⚠️ Potential issueSSR manifest creation needs error handling and hardcoded paths should be configurable.
This function has two important issues that were previously flagged but not addressed:
- There's no error handling if manifest files can't be loaded
- The webpack path "/webpack/development/" is hardcoded, which won't work in production environments
const createSSRManifest = async ( reactServerManifestFileName: string, reactClientManifestFileName: string, ) => { - const [reactServerManifest, reactClientManifest] = await Promise.all([ - loadJsonFile(reactServerManifestFileName), - loadJsonFile(reactClientManifestFileName), - ]); + try { + const [reactServerManifest, reactClientManifest] = await Promise.all([ + loadJsonFile(reactServerManifestFileName), + loadJsonFile(reactClientManifestFileName), + ]); - const ssrManifest = { - moduleLoading: { - prefix: '/webpack/development/', - crossOrigin: null, - }, - moduleMap: {} as Record<string, unknown>, - }; + // Get prefix from environment or fallback to default + const webpackPrefix = process.env.WEBPACK_PREFIX || '/webpack/development/'; + + const ssrManifest = { + moduleLoading: { + prefix: webpackPrefix, + crossOrigin: null, + }, + moduleMap: {} as Record<string, unknown>, + }; Object.entries(reactClientManifest).forEach(([aboluteFileUrl, clientFileBundlingInfo]) => { const serverFileBundlingInfo = reactServerManifest[aboluteFileUrl]; ssrManifest.moduleMap[(clientFileBundlingInfo as { id: string }).id] = { '*': { id: (serverFileBundlingInfo as { id: string }).id, chunks: (serverFileBundlingInfo as { chunks: string[] }).chunks, name: '*', }, }; }); return ssrManifest; + } catch (error) { + throw new Error(`Failed to load or process manifest files: ${error instanceof Error ? error.message : String(error)}`); + } };
99-103:⚠️ Potential issueMissing Error Handling for
generateRSCPayloadThe call to
generateRSCPayloadisn't wrapped in any error handling, which was previously flagged. If the function fails, the error would be unhandled and could crash the application.- const rscPayloadStream = await generateRSCPayload( - componentName, - componentProps, - railsContext.serverSideRSCPayloadParameters, - ); + try { + const rscPayloadStream = await generateRSCPayload( + componentName, + componentProps, + railsContext.serverSideRSCPayloadParameters, + );And modify the rest of the function to be within this try block with appropriate error handling.
🧹 Nitpick comments (5)
node_package/src/ComponentRegistry.ts (1)
23-23: Remove Unnecessary Extra Whitespace
The extra space betweencomponentand.lengthon line 23 is flagged by the static analysis tools (prettier/prettier) and should be removed. Keeping the accessor tight improves readability and avoids linting issues.- const isRenderer = renderFunction && component .length === 3; + const isRenderer = renderFunction && component.length === 3;🧰 Tools
🪛 ESLint
[error] 23-23: Delete
·(prettier/prettier)
node_package/src/ReactOnRails.client.ts (1)
340-340: Consider using object spread operator instead of Object.assign.The ESLint rule suggests using the modern object spread syntax for better readability.
- this.options = Object.assign({}, DEFAULT_OPTIONS); + this.options = { ...DEFAULT_OPTIONS };🧰 Tools
🪛 ESLint
[error] 340-340: Use an object spread instead of
Object.assigneg:{ ...foo }.(prefer-object-spread)
node_package/src/RSCPayloadContainer.ts (1)
17-19: Implement more comprehensive script content sanitization.The current
escapeScriptfunction handles some basic escaping but may not cover all potential security issues.Consider using a more comprehensive sanitization approach:
- function escapeScript(script: string) { - return script.replace(/<!--/g, '<\\!--').replace(/<\/(script)/gi, '</\\$1'); - } + function escapeScript(script: string) { + // Escape HTML comments, script tags, and other potentially dangerous content + return script + .replace(/<!--/g, '<\\!--') + .replace(/<\/(script)/gi, '</\\$1') + .replace(/</g, '\\u003C') + .replace(/>/g, '\\u003E') + .replace(/\//g, '\\u002F'); + }node_package/src/RSCServerRoot.ts (2)
1-8: Fix import order to comply with ESLint rules.The
streamimport should occur before the type import according to the ESLint ruleimport/order. Consider reordering imports:import * as React from 'react'; import { createFromNodeStream } from 'react-on-rails-rsc/client.node'; +import { PassThrough } from 'stream'; import type { RenderFunction, RailsContext } from './types'; import transformRSCStream from './transformRSCNodeStreamAndReplayConsoleLogs'; import loadJsonFile from './loadJsonFile'; import RSCPayloadContainer from './RSCPayloadContainer'; -import { PassThrough } from 'stream';🧰 Tools
🪛 ESLint
[error] 7-7:
streamimport should occur before type import of./types(import/order)
112-119: Improve React element creation syntaxThe current implementation creates React elements with
React.createElementcalls, which is verbose and harder to read. Consider using JSX for better readability if your project is configured for it.If JSX is available, this could be simplified to:
return () => ( <> <React.Fragment key="serverComponentElement"> {use(serverComponentElement)} </React.Fragment> <RSCPayloadContainer RSCPayloadStream={rscPayloadStream2} key="rscPayloadContainer" /> </> );If JSX is not an option, at least consider using the array spread operator for cleaner code:
return () => React.createElement(React.Fragment, null, [ - React.createElement(React.Fragment, { key: 'serverComponentElement' }, use(serverComponentElement)), - React.createElement(RSCPayloadContainer, { - RSCPayloadStream: rscPayloadStream2, - key: 'rscPayloadContainer', - }), + React.createElement( + React.Fragment, + { key: 'serverComponentElement' }, + use(serverComponentElement) + ), + React.createElement( + RSCPayloadContainer, + { + RSCPayloadStream: rscPayloadStream2, + key: 'rscPayloadContainer', + } + ), ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
node_package/src/ComponentRegistry.ts(1 hunks)node_package/src/RSCClientRoot.ts(4 hunks)node_package/src/RSCPayloadContainer.ts(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(4 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/createReactOutput.ts(2 hunks)node_package/src/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/registerServerComponent/server.ts(2 hunks)node_package/src/serverRenderReactComponent.ts(3 hunks)node_package/src/streamServerRenderedReactComponent.ts(5 hunks)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(2 hunks)node_package/src/types/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- node_package/src/registerServerComponent/server.ts
- node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts
- node_package/src/buildConsoleReplay.ts
- node_package/src/registerServerComponent/server.rsc.ts
- node_package/src/serverRenderReactComponent.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- node_package/src/RSCClientRoot.ts
- node_package/src/types/index.ts
- node_package/src/streamServerRenderedReactComponent.ts
🧰 Additional context used
🧬 Code Definitions (2)
node_package/src/ReactOnRailsRSC.ts (2)
node_package/src/types/index.ts (2)
RSCRenderParams(151-153)ReactOnRails(188-221)node_package/src/loadJsonFile.ts (1)
loadJsonFile(7-25)
node_package/src/RSCServerRoot.ts (3)
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts (1)
transformRSCStream(3-37)node_package/src/loadJsonFile.ts (1)
loadJsonFile(7-25)node_package/src/types/index.ts (2)
RenderFunction(109-109)RailsContext(17-51)
🪛 GitHub Actions: Lint JS and Ruby
node_package/src/createReactOutput.ts
[warning] 11-11: Unused export 'createReactElementFromRenderFunction' detected.
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 ESLint
node_package/src/ComponentRegistry.ts
[error] 23-23: Delete ·
(prettier/prettier)
node_package/src/RSCServerRoot.ts
[error] 7-7: stream import should occur before type import of ./types
(import/order)
node_package/src/ReactOnRails.client.ts
[error] 340-340: Use an object spread instead of Object.assign eg: { ...foo }.
(prefer-object-spread)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
🔇 Additional comments (12)
node_package/src/ComponentRegistry.ts (1)
1-3: Address Missing Type Import for RenderFunction
A previous review comment pointed out thatRenderFunctionwas not imported, leading to build errors (e.g., TS2304). If this type is used or expected (possibly via related type definitions in the registry), please update the import statement accordingly. For example:-import { type RegisteredComponent, type ReactComponentOrRenderFunction } from './types'; +import { type RegisteredComponent, type ReactComponentOrRenderFunction, type RenderFunction } from './types';node_package/src/ReactOnRails.client.ts (1)
343-343: Good addition of theisRSCBundleflag to distinguish client-side bundle.This flag correctly identifies that this is not an RSC bundle, which will help with conditional logic throughout the application related to server components.
node_package/src/createReactOutput.ts (3)
83-90: Improved promise handling for render function results.The enhancement to properly handle promises that resolve to either strings or React components makes the code more robust. By checking the type of the resolved value and processing it accordingly, this change ensures consistent React element generation.
92-92: Code simplification using the new helper function.Good refactoring to use the newly created helper function, which improves consistency and maintainability.
11-30:✅ Verification successful
Good extraction of element creation logic into a separate function.
The new function
createReactElementFromRenderFunctionimproves code reusability and consolidates the logic for handling render function results. The warning message for JSX return values is informative and helps with future React compatibility.However, the pipeline shows a warning about this function being unused, despite it being used within the file. This could be a linting misconfiguration.
🏁 Script executed:
#!/bin/bash # Check where createReactElementFromRenderFunction is actually used grep -n "createReactElementFromRenderFunction" node_package/src/createReactOutput.tsLength of output: 323
Confirmed: Function Usage is Correct & Lint Warning Likely a Misconfiguration
Good extraction of the element creation logic into a separate function. The grep results confirm that
createReactElementFromRenderFunctionis indeed defined (line 11) and used at lines 88 and 92 in the file. The pipeline warning about the function being unused appears to be a lint misconfiguration rather than an issue with the code. Please review your lint settings if the warning persists.🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 11-11: Unused export 'createReactElementFromRenderFunction' detected.
node_package/src/RSCPayloadContainer.ts (1)
49-99: Well-structured stream handling with proper React suspense integration.The
RSCPayloadContainerWrappereffectively manages streaming data with appropriate error handling and React suspense integration. TheuseStateinitialization with a function ensures the setup happens only once, and the callback forgetChunkPromiseis properly memoized.node_package/src/ReactOnRailsRSC.ts (3)
24-31: Good enhancement to function signature and validation.The updated function signature now properly handles both direct React elements and promises, making it more flexible. The validation of required context properties helps prevent runtime errors.
42-43: Efficient concurrent loading using Promise.all.Using
Promise.allto load the client manifest and resolve the rendering result concurrently is a good optimization that can improve performance.
76-76: Correct setting of isRSCBundle flag for server components.Setting
isRSCBundletotruehere correctly identifies this as an RSC bundle, complementing the client-side setting. This flag will be essential for conditional logic throughout the application.node_package/src/RSCServerRoot.ts (3)
22-26: Good React version checkThe check for React.use availability ensures compatibility with the required React version, which is a good practice.
73-85: Good validation of required context propertiesThe validation ensures that all necessary context properties are present before proceeding, with helpful error messages that include the received context.
87-93: Good validation of generateRSCPayload availabilityThe check for the availability of the generateRSCPayload function provides a clear error message that guides the user on how to enable RSC support.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
node_package/src/createReactOutput.ts (2)
11-30: Good refactoring to centralize React element creation logicThis new helper function improves code organization by centralizing the logic for handling render function results, whether they're React elements or components.
However, consider using
console.warninstead ofconsole.errorsince this is a warning rather than a critical error. Also, the error message is quite verbose and could be more concise while maintaining clarity.- console.error( - `Warning: ReactOnRails: Your registered render-function (ReactOnRails.register) for ${name} - incorrectly returned a React Element (JSX). Instead, return a React Function Component by - wrapping your JSX in a function. ReactOnRails v13 will throw error on this, as React Hooks do not - work if you return JSX. Update by wrapping the result JSX of ${name} in a fat arrow function.`, - ); + console.warn( + `Warning: ReactOnRails: Render-function for '${name}' returned JSX directly instead of a Function Component. ` + + `This prevents React Hooks from working properly and will throw an error in ReactOnRails v13. ` + + `Wrap your JSX in a function instead.` + );
86-89: Improved promise handling for server componentsThe enhanced promise handling now correctly processes both string results (HTML) and React components, which addresses the issue of server components rendering twice as mentioned in the PR objectives.
For completeness, consider adding more robust error handling for unexpected result types:
return (renderFunctionResult as Promise<string | ReactComponent>).then((result) => { if (typeof result === 'string') return result; + if (result === null || result === undefined) { + console.error(`ReactOnRails: Render function for '${name}' resolved to ${result}`); + return null; + } return createReactElementFromRenderFunction(result, name, props); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
knip.ts(1 hunks)node_package/src/createReactOutput.ts(2 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
node_package/src/createReactOutput.ts (7)
node_package/src/types/index.ts (1)
ReactComponent(107-107)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js (1)
name(4-11)spec/dummy/client/app/startup/HelloWorldProps.jsx (1)
name(8-8)spec/dummy/client/app/startup/HelloWorldHooks.jsx (1)
name(8-8)spec/dummy/client/app/store/composeInitialState.js (1)
props(3-3)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx (1)
props(16-50)spec/dummy/client/app/startup/HelloWorldApp.jsx (1)
props(14-14)
🪛 ESLint
node_package/tests/streamServerRenderedReactComponent.test.jsx
[error] 210-210: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 228-228: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 242-242: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 260-260: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 282-282: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 316-316: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (7)
knip.ts (1)
12-12: Properly registered new RSC server component entry point.This change correctly adds the new RSC-specific server component implementation file to the Knip configuration, ensuring it's properly tracked as a production dependency. This addition aligns with the PR objective of improving server component rendering.
node_package/tests/streamServerRenderedReactComponent.test.jsx (5)
63-97: Great addition of component type handling!This refactoring supports testing various component registration strategies, making the tests more comprehensive. The switch statement cleanly handles different component types and provides proper error handling for unknown types.
173-173: Good adjustment for test resilienceChanging from exact length to
toBeGreaterThanOrEqualmakes the test more robust against implementation changes that might add extra chunks without breaking functionality.Also applies to: 194-194
206-254: Comprehensive tests for rendering functionsThese tests thoroughly validate that both synchronous and asynchronous render functions work correctly, including proper error handling. They reuse test logic efficiently with
it.each.🧰 Tools
🪛 ESLint
[error] 210-210: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 228-228: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 242-242: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
256-297: Thorough error handling testsThese tests ensure errors in render functions (both sync and async) are properly caught and reported, with correct validation of error messages and error emission behavior.
🧰 Tools
🪛 ESLint
[error] 260-260: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 282-282: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
299-322: Good test for Promise resolving to stringThis test validates an important edge case where a component returns a Promise that resolves to a string rather than a React element.
🧰 Tools
🪛 ESLint
[error] 316-316: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
node_package/src/createReactOutput.ts (1)
92-92: Consistent use of the helper functionGood refactoring to consistently use the new helper function for non-promise results as well.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
node_package/src/RSCPayloadContainer.ts (1)
28-33:⚠️ Potential issueSecurity concerns with
dangerouslySetInnerHTMLremain unaddressedThe current implementation uses
dangerouslySetInnerHTMLto inject streamed content into a script tag, which poses XSS risks despite theescapeScriptfunction. The escaping only handles comment tags and script closing tags but doesn't protect against all injection vectors.Consider alternatives like:
- Using JSON.stringify for the chunk data before injection
- Encoding the data as base64 and storing in a data attribute
- Implementing more comprehensive sanitization
const scriptElement = React.createElement('script', { dangerouslySetInnerHTML: { - __html: escapeScript(`(self.__FLIGHT_DATA||=[]).push(${chunk.chunk})`), + __html: `(self.__FLIGHT_DATA||=[]).push(${JSON.stringify(chunk.chunk)})`, }, key: `script-${chunkIndex}`, });🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🧹 Nitpick comments (3)
node_package/src/RSCPayloadContainer.ts (3)
50-81: Potential memory leak with unbounded promise arrayThe
chunkPromisesarray grows indefinitely as new chunks arrive, which could lead to memory issues for large or continuous streams.Consider adding a cleanup mechanism for resolved promises, especially if this component might be used with large datasets or long-lived connections. One approach would be to replace resolved promises with
nullafter they're consumed, allowing them to be garbage collected.
65-78: Improve stream error handlingThe current implementation creates a new promise after handling an error, which might lead to unexpected behavior if the stream continues to emit data after errors.
Consider adding more robust error handling:
- Add error logging
- Implement a timeout mechanism for when streams stop without ending
- Consider adding a maximum chunk count to prevent DoS attacks
RSCPayloadStream.on('error', (error) => { + console.error('React on Rails: Error in RSC payload stream', error); rejectCurrentPromise(error); createNewPromise(); });
71-74: Consider graceful recovery from stream errorsCurrently, after a stream error, the code creates a new promise but doesn't indicate to the user that an error occurred or attempt to reconnect.
Implement a more user-friendly approach to stream errors, such as:
- A visible error state in the UI
- A retry mechanism for transient errors
- A fallback rendering option when streams fail
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
node_package/src/RSCClientRoot.ts(3 hunks)node_package/src/RSCPayloadContainer.ts(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/createReactOutput.ts(2 hunks)node_package/src/loadJsonFile.ts(1 hunks)node_package/src/registerServerComponent/server.ts(2 hunks)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- node_package/src/loadJsonFile.ts
- node_package/src/ReactOnRails.client.ts
- node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts
- node_package/src/createReactOutput.ts
- node_package/src/RSCClientRoot.ts
- node_package/src/RSCServerRoot.ts
- node_package/src/registerServerComponent/server.ts
🧰 Additional context used
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (11)
node_package/tests/streamServerRenderedReactComponent.test.jsx (9)
63-97: Excellent implementation of componentType parameter for flexible component testingThis enhancement allows for comprehensive testing of various component registration strategies, which is crucial for RSC support. The switch statement is well-structured with clear cases for each component type and proper error handling for unknown types.
173-173: Improved test flexibility withtoBeGreaterThanOrEqual(2)This change makes the tests more robust by acknowledging that the exact number of chunks may vary, while still ensuring the minimum required chunks are present. This prevents tests from becoming brittle when the implementation details change.
Also applies to: 194-194, 250-250
179-179: Updated error message expectations for accuracyThe modified expectations better match the actual error messages produced by the component, ensuring the tests correctly validate error handling behavior.
Also applies to: 200-200, 255-255
206-224: Good test coverage for render function componentsThis test ensures that both synchronous and asynchronous render functions correctly produce the expected output in the stream. This is important for ensuring compatibility with various component declaration styles.
226-240: Comprehensive testing of synchronous errorsThis test validates that error handling works correctly when components throw synchronous errors, ensuring the error information is properly included in the stream output.
242-260: Thorough handling of asynchronous errorsThis test verifies that errors occurring in asynchronous code paths are properly captured and included in the stream. This is crucial for debugging React suspense-related issues.
262-282: Error handling for erroneous render functionsThis test confirms that errors thrown directly by render functions (rather than by the components they return) are properly handled. The dynamic error message selection based on component type is a nice touch.
284-307: Verification of error emission when throwJsErrors is trueThis test ensures that when configured to throw JS errors, the stream emits the error event correctly. This is important for integration with error handling middleware.
309-334: Support for string Promise componentsThis test validates an important use case where components return a Promise resolving to a string instead of a React element. This is essential for supporting various RSC payload formats.
node_package/src/RSCPayloadContainer.ts (2)
3-15: Types are well-definedThe type definitions clearly represent the structure and relationships between components. Good job defining specific types for the different props and data structures.
39-46: Efficient recursive rendering approachThe recursive rendering pattern with Suspense is a clean approach to handling the streaming nature of the data. This implementation will progressively render chunks as they arrive.
|
|
||
| createNewPromise(); | ||
| RSCPayloadStream.on('data', (streamChunk) => { | ||
| resolveCurrentPromise({ chunk: decoder.decode(streamChunk as Uint8Array), isLastChunk: false }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate chunk data before processing
The current implementation directly uses the decoded stream chunk without validation, which could lead to issues if malformed data is received.
Add validation for the chunk data to ensure it's valid JavaScript that can be safely evaluated:
- resolveCurrentPromise({ chunk: decoder.decode(streamChunk as Uint8Array), isLastChunk: false });
+ const decodedChunk = decoder.decode(streamChunk as Uint8Array);
+ try {
+ // Simple validation - attempt to parse as JSON to ensure it's valid
+ JSON.parse(decodedChunk);
+ resolveCurrentPromise({ chunk: decodedChunk, isLastChunk: false });
+ } catch (e) {
+ rejectCurrentPromise(new Error('Invalid chunk data received'));
+ }Note: This assumes the chunks are valid JSON. If they're in a different format, adjust the validation accordingly.
📝 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.
| resolveCurrentPromise({ chunk: decoder.decode(streamChunk as Uint8Array), isLastChunk: false }); | |
| const decodedChunk = decoder.decode(streamChunk as Uint8Array); | |
| try { | |
| // Simple validation - attempt to parse as JSON to ensure it's valid | |
| JSON.parse(decodedChunk); | |
| resolveCurrentPromise({ chunk: decodedChunk, isLastChunk: false }); | |
| } catch (e) { | |
| rejectCurrentPromise(new Error('Invalid chunk data received')); | |
| } |
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
9acaf78 to
20a71db
Compare
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/ror1719/add-support-for-async-render-function-returns-component
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
node_package/tests/streamServerRenderedReactComponent.test.jsx (1)
118-118: 🛠️ Refactor suggestionBug fix: Using
once('end', resolve)instead ofon('end', resolve)Using
once()instead ofon()prevents potential memory leaks by ensuring the callback is removed after it's executed. This is the correct approach for one-time events like 'end'.Also applies to: 137-137, 153-153, 169-169, 190-190, 211-211, 231-231, 247-247, 267-267, 291-291, 327-327
node_package/src/RSCPayloadContainer.ts (3)
28-33:⚠️ Potential issuePotential XSS risk with
dangerouslySetInnerHTML.Though you're escaping script tags, confirm that the data injected into
__FLIGHT_DATAis fully sanitized to prevent malicious scripts.I recommend using a safer approach to inject the data. Consider:
- const scriptElement = React.createElement('script', { - dangerouslySetInnerHTML: { - __html: escapeScript(`(self.__FLIGHT_DATA||=[]).push(${chunk.chunk})`), - }, - key: `script-${chunkIndex}`, - }); + // Create a data element that doesn't execute code + const scriptElement = React.createElement('script', { + type: 'application/json', + id: `flight-data-${chunkIndex}`, + key: `script-${chunkIndex}`, + dangerouslySetInnerHTML: { + __html: escapeScript(chunk.chunk), + }, + }); + + // Then create a separate script to process it + const processScript = React.createElement('script', { + key: `process-${chunkIndex}`, + dangerouslySetInnerHTML: { + __html: escapeScript(` + (function() { + const data = document.getElementById('flight-data-${chunkIndex}').textContent; + (self.__FLIGHT_DATA||=[]).push(JSON.parse(data)); + })(); + `), + }, + });If you need to maintain the current approach, at least add validation before injecting:
+ // Validate that chunk.chunk is valid JSON before injecting + try { + JSON.parse(chunk.chunk); + } catch (e) { + console.error('Invalid JSON in chunk:', e); + return null; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
67-67: 🛠️ Refactor suggestionValidate chunk data before processing.
The current implementation directly uses the decoded stream chunk without validation, which could lead to issues if malformed data is received.
Add validation for the chunk data to ensure it's valid JavaScript that can be safely evaluated:
- resolveCurrentPromise({ chunk: decoder.decode(streamChunk as Uint8Array), isLastChunk: false }); + const decodedChunk = decoder.decode(streamChunk as Uint8Array); + try { + // Simple validation - attempt to parse as JSON to ensure it's valid + JSON.parse(decodedChunk); + resolveCurrentPromise({ chunk: decodedChunk, isLastChunk: false }); + } catch (e) { + rejectCurrentPromise(new Error('Invalid chunk data received')); + }
85-87:⚠️ Potential issueFix array bounds check logic.
The current bounds check has an off-by-one error. JavaScript arrays are zero-indexed, so the valid indices are 0 to length-1.
- if (chunkIndex > chunkPromises.length) { + if (chunkIndex >= chunkPromises.length) { throw new Error('React on Rails Error: RSC Chunk index out of bounds'); }
🧹 Nitpick comments (8)
docs/javascript/render-functions.md (1)
1-342: Excellent comprehensive documentationThis is a great addition that thoroughly documents render functions, their types, and how to use them with Ruby helper methods. The examples cover all the use cases and return types clearly.
Minor formatting improvements:
- Fix trailing punctuation in headings (lines 144, 150, 178, 185, 190)
- Consider adding spacing after numbered list markers in lines 9-10
-1. `props`: The props passed from the Ruby helper methods (via the `props:` parameter), which become available in your JavaScript. -2. `railsContext`: Rails contextual information like current pathname, locale, etc. See the [Render-Functions and the Rails Context](https://www.shakacode.com/react-on-rails/docs/guides/render-functions-and-railscontext/) documentation for more details. +1. `props`: The props passed from the Ruby helper methods (via the `props:` parameter), which become available in your JavaScript. + +2. `railsContext`: Rails contextual information like current pathname, locale, etc. See the [Render-Functions and the Rails Context](https://www.shakacode.com/react-on-rails/docs/guides/render-functions-and-railscontext/) documentation for more details.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Loose punctuation mark.
Context: ...nctions take two parameters: 1.props: The props passed from the Ruby helper m...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~10-~10: Loose punctuation mark.
Context: ...le in your JavaScript. 2.railsContext: Rails contextual information like curre...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~126-~126: Loose punctuation mark.
Context: ...s an error. Always wrap HTML strings in{ renderedHtml: '...' }. 2. **Objects Require Specific Proper...(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~192-~192: Did you mean “and”?
Context: ...nts: - The render function MUST return an object - The object MUST include a `com...(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
144-144: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
150-150: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
185-185: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
190-190: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
node_package/src/RSCClientRoot.ts (5)
3-4: Document the reason for disabling the ESLint rule.Adding a comment explaining why the no-underscore-dangle rule needs to be disabled would improve code maintainability.
-/* eslint-disable no-underscore-dangle */ +/* eslint-disable no-underscore-dangle */ +// Disabled because we need to access and modify the __FLIGHT_DATA property on window
18-22: Document the purpose of the global Window interface extension.Adding documentation about what
__FLIGHT_DATAis used for would help other developers understand its purpose and expected usage.declare global { interface Window { + /** + * Global array that stores flight data chunks received from the server + * This is used to hydrate React Server Components on the client + */ __FLIGHT_DATA: unknown[]; } }
46-77: Consider adding cleanup logic and improve type safety.The current implementation might lead to memory leaks if the stream controller isn't properly cleaned up. Additionally, the type of chunk data should be more specific than
unknown.const createRSCStreamFromPage = () => { - let streamController: ReadableStreamController<RSCPayloadChunk> | undefined; + let streamController: ReadableStreamDefaultController<RSCPayloadChunk> | undefined; const stream = new ReadableStream<RSCPayloadChunk>({ start(controller) { if (typeof window === 'undefined') { return; } - const handleChunk = (chunk: unknown) => { + const handleChunk = (chunk: unknown) => { + // Validate chunk before enqueueing + if (chunk && typeof chunk === 'object') { controller.enqueue(chunk as RSCPayloadChunk); + } else { + console.error('Invalid RSC payload chunk received:', chunk); + } }; if (!window.__FLIGHT_DATA) { window.__FLIGHT_DATA = []; } window.__FLIGHT_DATA.forEach(handleChunk); - window.__FLIGHT_DATA.push = (...chunks: unknown[]) => { + const originalPush = Array.prototype.push; + window.__FLIGHT_DATA.push = function(...chunks: unknown[]) { chunks.forEach(handleChunk); - return chunks.length; + return originalPush.apply(this, chunks); }; streamController = controller; }, + cancel() { + // Restore original push method on cleanup + if (typeof window !== 'undefined' && window.__FLIGHT_DATA) { + Object.setPrototypeOf(window.__FLIGHT_DATA, Array.prototype); + } + } });
79-83: Add error handling for stream creation.The current implementation doesn't handle potential errors that could occur during stream transformation.
const createFromRSCStream = () => { - const stream = createRSCStreamFromPage(); - const transformedStream = transformRSCStreamAndReplayConsoleLogs(stream); - return createFromReadableStream<React.ReactNode>(transformedStream); + try { + const stream = createRSCStreamFromPage(); + const transformedStream = transformRSCStreamAndReplayConsoleLogs(stream); + return createFromReadableStream<React.ReactNode>(transformedStream); + } catch (error) { + console.error('Failed to create RSC stream:', error); + // Return a promise that resolves to an error component + return Promise.resolve(React.createElement('div', { + style: { color: 'red' } + }, 'Error loading server components')); + } };
111-112: Add error handling for the hydration process.When switching to server component hydration, the code should handle potential errors that might occur during the process.
- const root = await createFromRSCStream(); - ReactDOMClient.hydrateRoot(domNode, root); + try { + const root = await createFromRSCStream(); + ReactDOMClient.hydrateRoot(domNode, root); + } catch (error) { + console.error('Failed to hydrate RSC from server:', error); + const root = await fetchRSC({ componentName, rscPayloadGenerationUrlPath, componentProps }); + ReactDOMClient.createRoot(domNode).render(root); + }node_package/src/isServerRenderResult.ts (1)
19-21: Consider adding JSDoc to document the purpose of the updated function.Adding documentation would help clarify the purpose of the updated function signature.
+/** + * Type guard function that checks if a value is a Promise. + * Supports various input types that might be returned from render functions. + * + * @param testValue The value to check + * @returns A type predicate indicating if the value is a Promise + */ export function isPromise<T>( testValue: CreateReactOutputResult | RenderFunctionResult | Promise<T> | RenderStateHtml | string | null, ): testValue is Promise<T> {node_package/tests/serverRenderReactComponent.test.ts (1)
183-207: Consider additional test for error case in async rendering.While the test coverage is good, consider adding a specific test for error handling in async render functions.
it('serverRenderReactComponent handles errors in async render functions', async () => { const errorMessage = 'Async render error'; const X9: RenderFunction = () => Promise.reject(new Error(errorMessage)); X9.renderFunction = true; ComponentRegistry.register({ X9 }); const renderResult = serverRenderReactComponent({ name: 'X9', domNodeId: 'myDomId', trace: false, throwJsErrors: false, renderingReturnsPromises: true, }); assertIsPromise(renderResult); const result = await renderResult; expect(result.hasErrors).toBeTruthy(); expect(result.html).toContain(errorMessage); expect(result.html).toContain('Exception in rendering!'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
docs/javascript/render-functions.md(1 hunks)eslint.config.ts(1 hunks)knip.ts(1 hunks)lib/react_on_rails/configuration.rb(6 hunks)lib/react_on_rails/helper.rb(2 hunks)lib/react_on_rails/test_helper/webpack_assets_status_checker.rb(1 hunks)lib/react_on_rails/utils.rb(1 hunks)node_package/src/RSCClientRoot.ts(3 hunks)node_package/src/RSCPayloadContainer.ts(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(5 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/createReactOutput.ts(2 hunks)node_package/src/handleError.ts(1 hunks)node_package/src/isServerRenderResult.ts(2 hunks)node_package/src/loadJsonFile.ts(1 hunks)node_package/src/loadReactClientManifest.ts(0 hunks)node_package/src/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/registerServerComponent/server.ts(2 hunks)node_package/src/serverRenderReactComponent.ts(6 hunks)node_package/src/serverRenderUtils.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(5 hunks)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(2 hunks)node_package/src/types/index.ts(6 hunks)node_package/tests/serverRenderReactComponent.test.js(0 hunks)node_package/tests/serverRenderReactComponent.test.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(6 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (2)
- node_package/tests/serverRenderReactComponent.test.js
- node_package/src/loadReactClientManifest.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- knip.ts
- lib/react_on_rails/test_helper/webpack_assets_status_checker.rb
- node_package/src/ReactOnRails.client.ts
- node_package/src/handleError.ts
- lib/react_on_rails/utils.rb
- lib/react_on_rails/helper.rb
- node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts
- node_package/src/registerServerComponent/server.rsc.ts
- node_package/src/registerServerComponent/server.ts
- node_package/src/loadJsonFile.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- package.json
- node_package/src/buildConsoleReplay.ts
- node_package/src/ReactOnRailsRSC.ts
- node_package/src/RSCServerRoot.ts
- node_package/src/createReactOutput.ts
- node_package/src/types/index.ts
🧰 Additional context used
🧬 Code Definitions (2)
node_package/src/serverRenderUtils.ts (1)
node_package/src/types/index.ts (1)
FinalHtmlResult(195-195)
node_package/src/RSCClientRoot.ts (2)
node_package/src/types/index.ts (1)
RSCPayloadChunk(205-207)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
transformRSCStreamAndReplayConsoleLogs(3-60)
🪛 LanguageTool
docs/javascript/render-functions.md
[uncategorized] ~9-~9: Loose punctuation mark.
Context: ...nctions take two parameters: 1. props: The props passed from the Ruby helper m...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~10-~10: Loose punctuation mark.
Context: ...le in your JavaScript. 2. railsContext: Rails contextual information like curre...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~126-~126: Loose punctuation mark.
Context: ...s an error. Always wrap HTML strings in { renderedHtml: '...' }. 2. **Objects Require Specific Proper...
(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~192-~192: Did you mean “and”?
Context: ...nts: - The render function MUST return an object - The object MUST include a `com...
(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
docs/javascript/render-functions.md
144-144: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
150-150: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
178-178: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
185-185: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
190-190: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (42)
node_package/src/serverRenderUtils.ts (2)
6-7: Added FinalHtmlResult type to importsThis new type import supports the parameter type update in
createResultObject, allowing for more structured rendering results beyond simple strings.
10-10:✅ Verification successful
Updated parameter type for enhanced flexibility
The change from
string | nulltoFinalHtmlResult | nullexpands the function's capability to handle both simple HTML strings and the more structuredServerRenderHashRenderedHtmlobjects, which appears to support the PR's goal of improving server component rendering.Since this is a type update, please ensure that all callsites have been updated to handle the possibility of receiving a
ServerRenderHashRenderedHtmlobject instead of just a string.
🏁 Script executed:
#!/bin/bash # Find all callsites of createResultObject to verify they can handle the new parameter type rg "createResultObject\(" --type ts -A 3 -B 3Length of output: 4031
Action Required: Confirmed Type Update Compatibility
The update changing the
htmlparameter type increateResultObjectfromstring | nulltoFinalHtmlResult | nullhas been verified. Our grep search shows that all call sites in:
node_package/src/ReactOnRailsRSC.tsnode_package/src/serverRenderReactComponent.tsnode_package/src/streamServerRenderedReactComponent.tsare passing values (mostly strings) that are valid for the new union type. These call sites continue to work correctly (e.g., via JSON.stringify), so they can safely handle either a simple string or a structured
ServerRenderHashRenderedHtmlobject.Please double-check that none of these call sites perform string-specific operations on the
htmlfield that might assume a strict string format. Otherwise, the change is acceptable.lib/react_on_rails/configuration.rb (6)
13-13: Good addition of the constant for server client manifestThis addition aligns well with the existing pattern used for client manifests, providing a clear default value.
25-25: LGTM: Configuration default properly setThe new server client manifest file is correctly added to the configuration hash with the default value.
71-71: LGTM: Attribute accessor added appropriatelyThe new manifest file attribute is properly added to the accessor list.
87-88: LGTM: Initialize method parameter addedThe parameter is correctly added to the method signature with a default value of nil.
118-118: LGTM: Property initialization addedThe new property is properly initialized in the method.
308-315: LGTM: Proper file list constructionThe implementation correctly adds the new manifest file to the webpack_generated_files array only if it's present.
eslint.config.ts (1)
148-148: LGTM: Proper configuration for test filesAdding the test pattern to
allowDefaultProjectis appropriate to ensure proper ESLint support for the TypeScript test files.node_package/tests/streamServerRenderedReactComponent.test.jsx (5)
63-64: Well-structured component type handlingGreat improvement to the test setup, allowing testing of different component registration strategies through a clean switch statement.
Also applies to: 65-97
173-173: Improved test resilienceReplacing exact chunk count expectations with minimum expectations makes the tests more resilient to implementation changes that might add additional chunks.
Also applies to: 194-194, 250-250
206-224: Comprehensive testing for render functionsGood approach using
it.eachto test both synchronous and asynchronous render functions with the same expectations, reducing code duplication and improving test coverage.
262-307: Good error handling testsThese tests thoroughly verify how errors are handled in both synchronous and asynchronous render functions, including proper error propagation when
throwJsErrorsis enabled.
309-334: LGTM: Added support for Promise resolving to stringThis test case ensures that components can return Promises that resolve to strings, which is an important use case for server rendering.
node_package/src/isServerRenderResult.ts (1)
1-10: LGTM! Good type definition updates.The expanded type signatures for the functions improve type safety while maintaining backward compatibility.
node_package/tests/serverRenderReactComponent.test.ts (3)
12-24: Well-structured type assertion helpers.Good implementation of custom type assertions that provide clear error messages. This approach enhances test readability and maintainability.
26-51: Comprehensive test setup for basic component rendering.The test setup effectively clears the component registry before each test and validates both the rendering process and expected output.
125-158: Excellent documentation of edge case behavior.The comments explaining the unintended side effects of returning strings from render functions provide valuable context for the tests and future developers.
node_package/src/streamServerRenderedReactComponent.ts (14)
1-1: No issues with the new import statement.
The change to import React as a namespace is a valid approach and aligns well with TypeScript convention.
7-7: Import ofisServerRenderHashandisPromiselooks good.
This import captures the new helpers needed for distinguishing server render hashes and promises.
11-11: Addition ofStreamableComponentResulttype.
This change provides stronger type coverage for streaming-related returns.
90-99: Console replay logic looks proper.
Building the console replay chunk dynamically and returningnullwhen empty is a clean solution. Validating length before stringifying is efficient. No issues found.
108-117: Use of timeout to schedule console replay injection.
Clearing the previous timeout before scheduling the new one prevents potential concurrency issues. The zero-delay approach is acceptable for pushing ancillary data asynchronously.
119-124: Flush method usage is consistent.
InvokingclearTimeoutand then pushing any remaining console replay chunk upon flush ensures that all console logs are included before finalizing the stream.
149-152: Refined function signature forstreamRenderReactComponent.
Changing fromReactElementtoStreamableComponentResultcorrectly accommodates both synchronous elements and async/promise-based elements.
163-170: Centralizing error tracking withreportError.
MarkinghasErrorson the render state and optionally emitting the error into the stream is a clean, modular approach.
172-176: DedicatedsendErrorHtmlfor finalizing an error response.
This function enhances readability by separating error handling from other logic.
178-210: Handling promise-based rendering with React 18 streaming.
Properly checking for string outputs versus valid React elements helps maintain streaming consistency, whilerenderToPipeableStreamusage aligns with React 18.
215-215: Stream renderer type annotation is appropriate.
Introducing this generic signature clarifies which types are supported by the streaming renderer.
235-237: Error thrown for server render hashes in streaming.
The updated message cleanly indicates that streaming is not compatible with server render hashes, reflecting the new code logic.
239-250: Promise check and valid React element enforcement.
Converting or throwing an error if the promise result is not a valid React element helps prevent improper usage. The messaging is clear.
255-268: Robust fallback for uncaught errors in streaming pipeline.
By settinghasErrorsand sending error HTML, the code ensures that clients receive appropriate feedback without breaking the stream.node_package/src/serverRenderReactComponent.ts (10)
1-1: Namespace import for React.
This style aligns well with TypeScript usage, and no issues are apparent.
18-19: Newly introduced typesCreateReactOutputAsyncResultandRenderStateHtml.
Adding these clarifies async results and HTML return formats for server rendering.
30-30: Implicit declaration ofhtmlResult.
Refactoring to a single variable is straightforward and helps maintain readability.
46-46: Using nullish coalescing withhtmlResult.
This ensures the returned result is never undefined, matching the expected contract.
48-57:processReactElementgracefully handles render-to-string failures.
EncapsulatingrenderToStringin a try/catch block centralizes error handling and clarifies potential misconfigurations.
60-62: UpdatedprocessPromisesignature.
Switching toCreateReactOutputAsyncResultaligns with the new async flow, making the code more type-safe.
71-76: Conditional React element check in promise resolution.
Appropriately rendering React elements or returning the final value ensures correct behavior for mixed promise results.
85-86: Refined server render result logic.
Distinguishing between server render hash, promises, and React elements in a single flow clarifies the code path for each outcome.
122-122: Split out final result creation.
createFinalResultconsolidates promise handling and synchronous results into a single function, improving testability.
129-129: Delegation tocreatePromiseResult.
Spreading current render state and returning a structured promise ensures that console logs and rendered HTML are properly collated.
AbanoubGhadban
force-pushed
the
abanoubghadban/ror1719/add-support-for-async-render-function-returns-component
branch
from
0306315 to
e467df4
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
0faea62 to
d92f586
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/ror1719/add-support-for-async-render-function-returns-component
branch
from
fe75977 to
86f8bfb
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
f53eb02 to
8698a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
node_package/src/RSCPayloadContainer.ts (3)
28-33: Security risk with dangerouslySetInnerHTML.While you're escaping some script tags, using
dangerouslySetInnerHTMLstill presents XSS risks. The__FLIGHT_DATAis being directly populated with potentially untrusted data.🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
17-19: 🛠️ Refactor suggestionEnhance script escaping function for better security.
The current escaping function only handles a limited set of special HTML sequences. For better XSS protection, consider a more comprehensive implementation.
function escapeScript(script: string) { - return script.replace(/<!--/g, '<\\!--').replace(/<\/(script)/gi, '</\\$1'); + return script + .replace(/<!--/g, '<\\!--') + .replace(/<\/(script)/gi, '</\\$1') + .replace(/</g, '\\u003C') + .replace(/>/g, '\\u003E') + .replace(/\//g, '\\u002F') + .replace(/\u2028/g, '\\u2028') + .replace(/\u2029/g, '\\u2029'); }
84-87:⚠️ Potential issueFix array bounds check logic.
There's an off-by-one error in the bounds check. JavaScript arrays are zero-indexed, so valid indices are 0 to length-1.
- if (chunkIndex > chunkPromises.length) { + if (chunkIndex >= chunkPromises.length) { throw new Error('React on Rails Error: RSC Chunk index out of bounds'); }node_package/src/RSCServerRoot.ts (2)
38-67: Prefix path is still hardcoded.This repeats an earlier suggestion regarding making the
"/webpack/development/"path configurable. If a user is not in a development environment, they may need a custom prefix. Consider falling back to an environment variable or a provided option to allow greater flexibility.
100-105: Stream error handling is missing.As previously suggested, consider attaching
.on('error', ...)handlers to thesePassThroughstreams. This ensures errors are caught and handled rather than causing unhandled exceptions in production environments.
🧹 Nitpick comments (3)
node_package/src/RSCPayloadContainer.ts (1)
66-69: Add validation for stream chunks before processing.The current implementation directly uses decoded stream chunks without validation, which could lead to issues if malformed data is received.
- resolveCurrentPromise({ chunk: decoder.decode(streamChunk as Uint8Array), isLastChunk: false }); + const decodedChunk = decoder.decode(streamChunk as Uint8Array); + try { + // Simple validation to ensure it's valid - the exact validation depends on the expected format + if (decodedChunk.trim()) { + resolveCurrentPromise({ chunk: decodedChunk, isLastChunk: false }); + } else { + console.warn('Empty chunk received, skipping'); + return; // Don't create a new promise for empty chunks + } + } catch (e) { + rejectCurrentPromise(new Error(`Invalid chunk data received: ${e.message}`)); + } createNewPromise();node_package/src/types/index.ts (1)
37-37: Consider clarifying the usage ofrscPayloadGenerationUrl.The new optional property
rscPayloadGenerationUrlmay need additional documentation or usage examples to help maintainers understand its purpose and how it integrates with the RSC payload rendering workflow.node_package/src/RSCServerRoot.ts (1)
1-16: Avoid relying on a global function definition.Declaring
function generateRSCPayloadin theglobalscope can lead to hard-to-track dependencies. Consider importing or injecting a function reference instead, to promote clearer module boundaries and testability.- declare global { - function generateRSCPayload( - componentName: string, - props: Record<string, unknown>, - railsContext: RailsContext, - ): Promise<NodeJS.ReadableStream>; - } + import { generateRSCPayload } from './rscPayloadGenerator'; // Example import
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
CHANGELOG.md(1 hunks)eslint.config.ts(1 hunks)knip.ts(1 hunks)lib/react_on_rails/configuration.rb(6 hunks)lib/react_on_rails/helper.rb(3 hunks)lib/react_on_rails/test_helper/webpack_assets_status_checker.rb(1 hunks)lib/react_on_rails/utils.rb(1 hunks)node_package/src/RSCClientRoot.ts(3 hunks)node_package/src/RSCPayloadContainer.ts(1 hunks)node_package/src/RSCServerRoot.ts(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(4 hunks)node_package/src/buildConsoleReplay.ts(1 hunks)node_package/src/loadJsonFile.ts(1 hunks)node_package/src/loadReactClientManifest.ts(0 hunks)node_package/src/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/registerServerComponent/server.ts(2 hunks)node_package/src/serverRenderReactComponent.ts(1 hunks)node_package/src/streamServerRenderedReactComponent.ts(2 hunks)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts(2 hunks)node_package/src/types/index.ts(3 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/loadReactClientManifest.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- eslint.config.ts
- knip.ts
- lib/react_on_rails/test_helper/webpack_assets_status_checker.rb
- node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts
- lib/react_on_rails/utils.rb
- lib/react_on_rails/helper.rb
- node_package/src/registerServerComponent/server.ts
- lib/react_on_rails/configuration.rb
- node_package/src/registerServerComponent/server.rsc.ts
- node_package/src/buildConsoleReplay.ts
- node_package/src/streamServerRenderedReactComponent.ts
- node_package/src/loadJsonFile.ts
- package.json
- node_package/src/ReactOnRails.client.ts
- node_package/src/transformRSCStreamAndReplayConsoleLogs.ts
- node_package/src/ReactOnRailsRSC.ts
🧰 Additional context used
🧬 Code Definitions (2)
node_package/src/serverRenderReactComponent.ts (1)
node_package/src/serverRenderUtils.ts (1)
createResultObject(9-24)
node_package/src/RSCServerRoot.ts (4)
node_package/src/types/index.ts (1)
RailsContext(20-54)node_package/tests/testUtils.js (1)
stream(14-21)node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts (1)
transformRSCStream(3-31)node_package/src/loadJsonFile.ts (1)
loadJsonFile(7-25)
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.ts
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (11)
node_package/src/serverRenderReactComponent.ts (1)
118-118: Bug fix for error handling in result object creation.Fixed a bug where the result object creation was using the original render state instead of the error state when an error occurred during promise resolution. This ensures that error information is properly propagated.
CHANGELOG.md (1)
26-32: Well-documented architectural improvements in the CHANGELOG.The new changelog entries clearly document the key improvements made to the RSC rendering flow, including eliminating double rendering and reducing HTTP requests. The mention of new components provides good context for users wanting to understand what changed.
node_package/src/RSCClientRoot.ts (4)
3-22: Added global type definition for Flight data.Good addition of the global Window interface extension for the
__FLIGHT_DATAarray that will store React Server Components data. This provides proper TypeScript support for the global variable.
46-77: Stream creation from embedded RSC payload data.This function creates a ReadableStream from the server-embedded Flight data by:
- Setting up a stream controller
- Processing existing chunks in
__FLIGHT_DATA- Overriding the array's push method to process new chunks immediately
- Properly closing the stream when DOM is fully loaded
This is an elegant approach to avoid unnecessary HTTP requests by reusing server-provided data.
79-83: Helper function to create RSC stream from page data.Creates a complete pipeline to transform the raw RSC stream and prepare it for React's consumption. This encapsulates the multi-step process into a single reusable function.
110-115: Improved SSR hydration logic based on DOM content.The component now intelligently chooses between:
- Hydrating from existing server-rendered content using embedded Flight data
- Fetching fresh data when the DOM node is empty
This optimization eliminates redundant network requests when server-rendered content is available.
node_package/src/types/index.ts (2)
20-20: Use of type alias is appropriate.Converting
RailsContextfrom aninterfaceto atypealias is suitable here, especially since you’re leveraging a union type. This change cleanly accommodates conditional properties based onserverSide.
38-54: Good approach for conditionally requiring server-side properties.Using a union to differentiate between
serverSide: falseandserverSide: truehelps ensure type safety for RSC. This structure nicely enforces thatserverSideRSCPayloadParametersand manifest file names are only present whenserverSideis true.node_package/src/RSCServerRoot.ts (3)
22-26: React version check is a reasonable safeguard.This explicit check provides clear feedback if the user’s React version does not support server components. It’s a practical guard to avoid silent failures.
69-92: Validation checks look good.You’re verifying the presence of required fields:
serverSide,reactClientManifestFileName, andreactServerClientManifestFileName, plus confirminggenerateRSCPayloadis defined. These checks will help fail fast if the setup is incomplete.
107-115: Creative use of React fragments and theusehook.Wrapping the server component’s result and the payload container side by side in fragments cleanly separates the streamed RSC content from the payload stream. This approach is succinct and leverages React 18 features well.
There was a problem hiding this comment.
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
node_package/src/RSCServerRoot.ts:73
- [nitpick] The error message thrown when required railsContext properties are missing could be clearer. Consider specifying exactly which properties are required (e.g., 'reactClientManifestFileName' and 'reactServerClientManifestFileName') to aid debugging.
if (!railsContext?.serverSide || !railsContext?.reactClientManifestFileName || !railsContext?.reactServerClientManifestFileName) {
node_package/src/transformRSCNodeStreamAndReplayConsoleLogs.ts:13
- The current check for line breaks assumes '\n' as the newline delimiter. Consider handling other newline formats (such as '\r\n') to ensure complete chunk detection across environments.
if (!decodedChunk.endsWith('\n')) {
node_package/src/RSCClientRoot.ts:111
- [nitpick] The logic diverges on whether DOM content exists, choosing between createFromRSCStream and fetch-based hydration. Consider adding a comment to explain why the presence of innerHTML triggers a different hydration strategy to improve code clarity.
if (domNode.innerHTML) {
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
node_package/src/RSCServerRoot.tsx (1)
38-67: Minor spelling error: "aboluteFileUrl".Consider correcting to "absoluteFileUrl" for clarity. For example:
- Object.entries(reactClientManifest).forEach(([aboluteFileUrl, clientFileBundlingInfo]) => { + Object.entries(reactClientManifest).forEach(([absoluteFileUrl, clientFileBundlingInfo]) => {node_package/src/RSCPayloadContainer.tsx (1)
17-19: escapeScript method.
Implementation is minimal. Consider more robust sanitization if user content is allowed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eslint.config.ts(4 hunks)node_package/src/RSCClientRoot.ts(3 hunks)node_package/src/RSCPayloadContainer.tsx(1 hunks)node_package/src/RSCServerRoot.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
node_package/src/RSCClientRoot.ts (2)
node_package/src/types/index.ts (1)
RSCPayloadChunk(205-207)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
transformRSCStreamAndReplayConsoleLogs(3-60)
🪛 Biome (1.9.4)
node_package/src/RSCPayloadContainer.tsx
[error] 50-50: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (18)
node_package/src/RSCClientRoot.ts (5)
8-8: Imports look good.
16-20: No issues with global interface extension.
44-90: Consider verifying late loading scenario.
Current approach closes the stream on DOMContentLoaded, which might cause data loss if the RSC payload script loads afterward.Please manually test or confirm that all data is guaranteed to be enqueued before DOMContentLoaded triggers.
92-96: Helper function logic is clear.
124-124: Efficient fallback logic.
This approach to detect existing content vs. fetching anew is neat.node_package/src/RSCServerRoot.tsx (7)
1-8: Import statements are correct.
9-16: Global function declaration.
No issues found, provided that it's properly defined at runtime.
17-21: Type definitions.
Looks good for basic usage.
22-26: React.use version check.
Ensures correct React version and experimental features.
28-36: createFromReactOnRailsNodeStream.
Straightforward usage of transformRSCStream.
69-113: RSCServerRoot.
Implementation is well-structured.
115-115: Export statement.
No concerns.node_package/src/RSCPayloadContainer.tsx (6)
1-2: Initial imports.
No issues found.
3-7: StreamChunk type.
Accurately defines the chunk structure.
8-10: RSCPayloadContainerProps.
Matches the expected NodeJS.ReadableStream usage.
12-15: RSCPayloadContainerInnerProps.
Property definitions are clear and typed.
21-39: Component documentation.
Clear explanation of this payload container’s purpose and flow.
71-121: Chunk-based approach.
Implementation logic for streaming chunks is sound and well-structured.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
node_package/src/RSCServerRoot.tsx (1)
1-116: 🛠️ Refactor suggestionAdd error handling for stream processing failures
The code doesn't handle errors that might occur during stream processing, such as network errors or timeouts.
Consider adding error handling for the stream processing:
+ import { promisify } from 'util'; // In the RSCServerRoot component: const rscPayloadStream = await generateRSCPayload(componentName, componentProps, railsContext); // Tee the stream to pass it to the server component and the payload container const rscPayloadStream1 = new PassThrough(); + rscPayloadStream1.on('error', (error) => { + console.error('Error in RSC payload stream 1:', error); + }); rscPayloadStream.pipe(rscPayloadStream1); const rscPayloadStream2 = new PassThrough(); + rscPayloadStream2.on('error', (error) => { + console.error('Error in RSC payload stream 2:', error); + }); rscPayloadStream.pipe(rscPayloadStream2); + rscPayloadStream.on('error', (error) => { + console.error('Error in original RSC payload stream:', error); + // Propagate the error to the derived streams + rscPayloadStream1.emit('error', error); + rscPayloadStream2.emit('error', error); + }); const serverComponentElement = createFromReactOnRailsNodeStream(rscPayloadStream1, ssrManifest); + + // Add a timeout for stream processing + const streamTimeout = setTimeout(() => { + const timeoutError = new Error('RSC payload stream timed out'); + rscPayloadStream.emit('error', timeoutError); + }, 30000); // 30 seconds timeout + + // Clean up the timeout when the component unmounts + React.useEffect(() => { + return () => clearTimeout(streamTimeout); + }, []);
🧹 Nitpick comments (5)
node_package/src/transformRSCNodeStream.ts (1)
1-31: Add JSDoc comments to improve code documentationThis function serves an important purpose in the RSC rendering flow, but lacks documentation explaining what it does and how it should be used.
Consider adding JSDoc comments:
import { Transform } from 'stream'; + /** + * Transforms an RSC stream containing JSON objects with 'html' property + * into a stream of HTML strings. + * + * Each chunk in the input stream is expected to be a valid JSON object + * containing an 'html' property. The function extracts and returns + * the HTML content from each chunk. + * + * @param stream - A readable stream containing JSON objects with 'html' property + * @returns A transformed stream containing only the HTML content + */ export default function transformRSCStream(stream: NodeJS.ReadableStream): NodeJS.ReadableStream { // implementation }node_package/src/RSCServerRoot.tsx (4)
100-105: Improve stream handling by using built-in duplex featuresThe current approach of creating two PassThrough streams and manually piping them might be inefficient. Node.js has built-in methods for duplicating streams.
Consider using Node.js's
stream.Readable.prototype.tee()if available (Node.js 16.7.0+) or a more efficient stream duplication:- // Tee the stream to pass it to the server component and the payload container - const rscPayloadStream1 = new PassThrough(); - rscPayloadStream.pipe(rscPayloadStream1); - const rscPayloadStream2 = new PassThrough(); - rscPayloadStream.pipe(rscPayloadStream2); - const serverComponentElement = createFromReactOnRailsNodeStream(rscPayloadStream1, ssrManifest); + // Use more efficient stream duplication if available + let rscPayloadStream1, rscPayloadStream2; + if (typeof rscPayloadStream.tee === 'function') { + // For Node.js 16.7.0+ + [rscPayloadStream1, rscPayloadStream2] = rscPayloadStream.tee(); + } else { + // Fallback for older Node.js versions + rscPayloadStream1 = new PassThrough(); + rscPayloadStream2 = new PassThrough(); + rscPayloadStream.pipe(rscPayloadStream1); + rscPayloadStream.pipe(rscPayloadStream2); + } + const serverComponentElement = createFromReactOnRailsNodeStream(rscPayloadStream1, ssrManifest);Note: Verify Node.js version compatibility in your environment before implementing this change.
38-67: Extract the manifest creation into a separate utility fileThe
createSSRManifestfunction handles complex logic that might be reused elsewhere and makes the component file larger than necessary.Consider moving this function to a separate utility file (e.g.,
createSSRManifest.ts):// New file: createSSRManifest.ts + import loadJsonFile from './loadJsonFile'; + + export default async function createSSRManifest( + reactServerManifestFileName: string, + reactClientManifestFileName: string, + webpackPrefix: string = '/webpack/development/' + ) { + const [reactServerManifest, reactClientManifest] = await Promise.all([ + loadJsonFile(reactServerManifestFileName), + loadJsonFile(reactClientManifestFileName), + ]); + + const ssrManifest = { + moduleLoading: { + prefix: webpackPrefix, + crossOrigin: null, + }, + moduleMap: {} as Record<string, unknown>, + }; + + Object.entries(reactClientManifest).forEach(([aboluteFileUrl, clientFileBundlingInfo]) => { + const serverFileBundlingInfo = reactServerManifest[aboluteFileUrl]; + if (!serverFileBundlingInfo) return; + + ssrManifest.moduleMap[(clientFileBundlingInfo as { id: string }).id] = { + '*': { + id: (serverFileBundlingInfo as { id: string }).id, + chunks: (serverFileBundlingInfo as { chunks: string[] }).chunks, + name: '*', + }, + }; + }); + + return ssrManifest; + } // In RSCServerRoot.tsx + import createSSRManifest from './createSSRManifest'; // Remove the createSSRManifest function from RSCServerRoot.tsx and update the call: - const ssrManifest = await createSSRManifest( + const webpackPrefix = railsContext.webpackDevServerBaseUrl || + `/webpack/${process.env.NODE_ENV || 'development'}/`; + const ssrManifest = await createSSRManifest( railsContext.reactServerClientManifestFileName, railsContext.reactClientManifestFileName, + webpackPrefix );
73-84: Improve error message clarity with specific checksThe current error message combines multiple checks into one conditional, making it hard to determine which specific property is missing.
Consider separate checks with specific error messages:
- if ( - !railsContext?.serverSide || - !railsContext?.reactClientManifestFileName || - !railsContext?.reactServerClientManifestFileName - ) { - throw new Error( - 'serverClientManifestFileName and reactServerClientManifestFileName are required. ' + - 'Please ensure that React Server Component webpack configurations are properly set ' + - 'as stated in the React Server Component tutorial. ' + - 'Make sure to use "stream_react_component" instead of "react_component" to SSR a server component.', - ); - } + if (!railsContext?.serverSide) { + throw new Error( + 'Server components can only be rendered server-side. ' + + 'Make sure to use "stream_react_component" instead of "react_component" to SSR a server component.' + ); + } + + if (!railsContext?.reactClientManifestFileName) { + throw new Error( + 'reactClientManifestFileName is required. ' + + 'Please ensure that React Server Component webpack configurations are properly set ' + + 'as stated in the React Server Component tutorial.' + ); + } + + if (!railsContext?.reactServerClientManifestFileName) { + throw new Error( + 'reactServerClientManifestFileName is required. ' + + 'Please ensure that React Server Component webpack configurations are properly set ' + + 'as stated in the React Server Component tutorial.' + ); + }
85-92: Add version check for generateRSCPayloadYou're checking for the existence of the function but not its compatibility with your expected signature.
Consider adding a simple version check if the function supports a version property:
if (typeof generateRSCPayload !== 'function') { throw new Error( 'generateRSCPayload is not defined. Please ensure that you are using at least version 4.0.0 of ' + 'React on Rails Pro and the Node renderer, and that ReactOnRailsPro.configuration.enable_rsc_support ' + 'is set to true.', ); } + // Optional version check if the function has a version property + if ( + 'version' in generateRSCPayload && + typeof generateRSCPayload.version === 'string' && + generateRSCPayload.version < '4.0.0' + ) { + throw new Error( + `Incompatible generateRSCPayload version: ${generateRSCPayload.version}. ` + + 'Please ensure that you are using at least version 4.0.0 of React on Rails Pro.' + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node_package/src/RSCServerRoot.tsx(1 hunks)node_package/src/transformRSCNodeStream.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
node_package/src/RSCProvider.tsx (2)
3-3: Use a type-only import for the client module.The import pulls in client-only code at runtime but is only used for typing. Change to a type-only import to prevent client code from being bundled on the server.
56-62: Add error handling for failed server component fetches.The promise chain doesn't handle errors, causing failed fetches to remain cached indefinitely. Add a catch block to remove failed promises from the cache.
🧹 Nitpick comments (1)
node_package/src/getReactServerComponent.client.ts (1)
63-68: Document or reconsider the array mutation pattern.Mutating the
pushmethod of the payloads array could cause unexpected side effects if the array is used elsewhere. Consider using a dedicated stream controller pattern or clearly document this behavior.Alternative approaches:
- Use a dedicated event emitter for new chunks
- Create a wrapper object with explicit streaming methods
- Document this mutation in the function's JSDoc to warn consumers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
node_package/src/RSCProvider.tsx(1 hunks)node_package/src/RSCRoute.tsx(1 hunks)node_package/src/RSCRouteError.ts(1 hunks)node_package/src/ServerComponentFetchError.ts(1 hunks)node_package/src/getReactServerComponent.client.ts(1 hunks)node_package/src/getReactServerComponent.server.ts(1 hunks)package.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- node_package/src/ServerComponentFetchError.ts
- node_package/src/RSCRouteError.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 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.599Z
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.
node_package/src/RSCRoute.tsx (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.599Z
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.
node_package/src/RSCProvider.tsx (2)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.599Z
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#1696
File: node_package/src/RSCProvider.tsx:40-41
Timestamp: 2025-06-09T08:01:35.951Z
Learning: In React components, cache objects (like Map, Record, etc.) that are created within the component's scope or factory function closure are automatically garbage collected when the component unmounts. This pattern is acceptable and doesn't cause memory leaks because the cache is tied to the component lifecycle, unlike global or module-level caches that persist indefinitely.
🧬 Code Graph Analysis (2)
node_package/src/RSCProvider.tsx (3)
node_package/src/types/index.ts (1)
RailsContextWithComponentSpecificMetadata(66-70)node_package/src/utils.ts (1)
createRSCPayloadKey(15-21)node_package/src/ReactOnRails.client.ts (1)
getComponent(142-144)
node_package/src/getReactServerComponent.client.ts (3)
node_package/src/types/index.ts (2)
RailsContext(21-64)assertRailsContextWithComponentSpecificMetadata(79-89)node_package/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
transformRSCStreamAndReplayConsoleLogs(17-75)node_package/src/utils.ts (2)
wrapInNewPromise(32-37)createRSCPayloadKey(15-21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (8)
node_package/src/RSCRoute.tsx (2)
9-31: Well-designed error boundary pattern!The error boundary correctly enriches errors with component context, enabling parent components to implement retry logic. This aligns with React's error handling best practices for RSC implementations.
64-77: Efficient implementation with proper caching!The component correctly checks for cached components before initiating fetches, reducing unnecessary network requests. The error boundary wrapping ensures proper error context propagation.
node_package/src/RSCProvider.tsx (1)
64-75: 🛠️ Refactor suggestionClean up the promise cache when refetching.
The
refetchComponentmethod should also delete the old promise fromfetchRSCPromisesto prevent potential race conditions.const refetchComponent = (componentName: string, componentProps: unknown) => { const key = createRSCPayloadKey(componentName, componentProps, railsContext); - cachedComponents[key] = undefined; + delete cachedComponents[key]; + delete fetchRSCPromises[key]; const promise = getServerComponent({ componentName, componentProps, railsContext, enforceRefetch: true, }); fetchRSCPromises[key] = promise; return promise; };Likely an incorrect or invalid review comment.
node_package/src/getReactServerComponent.server.ts (5)
1-5: LGTM! Clean and well-organized imports.The imports are well-structured and include all necessary dependencies for RSC server-side rendering functionality.
64-68: Good defensive programming with clear error messaging.The validation check for
ReactOnRails.getRSCPayloadStreamprovides excellent developer experience with a descriptive error message pointing to the likely cause.
76-80: LGTM! Clean integration with the RSC infrastructure.The function properly passes the required manifest file names from the Rails context and integrates well with the stream transformation pipeline.
33-56: Excellent documentation with clear usage guidance.The JSDoc provides comprehensive information about the function's purpose, behavior, and important usage warnings for internal vs. public APIs.
70-74:✅ Verification successful
Verify error propagation from RSC payload stream generation.
Based on the retrieved learnings, errors from RSC payload generation should propagate to client components. However, ensure that any critical server-side errors are properly handled to prevent server crashes.
Let me verify how errors from
getRSCPayloadStreamare handled:
🏁 Script executed:
#!/bin/bash # Description: Check error handling in getRSCPayloadStream implementation # Expected: Find error handling patterns in RSCPayloadGenerator # Search for error handling in RSC payload generation ast-grep --pattern $'try { $$$ } catch ($$$) { $$$ }' # Search for error propagation patterns rg -A 5 -B 5 "getRSCPayloadStream"Length of output: 35615
No additional error handling needed for RSC payload stream
The
getRSCPayloadStreamimplementation inRSCPayloadGenerator.tsdoesn’t catch errors, so any exception thrown by the underlyinggenerateRSCPayloadwill bubble up naturally. IngetReactServerComponent.server.ts, you correctly check thatgetRSCPayloadStreamexists but do not wrap it in atry/catch—meaning any stream-generation errors will propagate to your Rails controller, causing the request to fail fast. This aligns with our RSC error-propagation design: React Server Components errors surface to the client viaErrorBoundaryand RSCRoute, and we do not want silent failures on the server. No changes required here.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
node_package/src/injectRSCPayload.ts (1)
68-76:⚠️ Potential issueCall
onRSCPayloadGenerateddirectly – optional chaining hides bugs
The earlier review pointed this out already; the optional chaining is still present.
If the function is unexpectedly undefined we already log an error above – silently skipping execution defeats the whole injection logic.- ReactOnRails.onRSCPayloadGenerated?.(railsContext, (streamInfo) => { + ReactOnRails.onRSCPayloadGenerated(railsContext, (streamInfo) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
node_package/src/RSCPayloadGenerator.ts(1 hunks)node_package/src/ReactOnRailsRSC.ts(2 hunks)node_package/src/getReactServerComponent.client.ts(1 hunks)node_package/src/getReactServerComponent.server.ts(1 hunks)node_package/src/injectRSCPayload.ts(1 hunks)node_package/src/types/index.ts(8 hunks)node_package/tests/SuspenseHydration.test.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- node_package/src/getReactServerComponent.client.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- node_package/src/ReactOnRailsRSC.ts
- node_package/tests/SuspenseHydration.test.tsx
- node_package/src/RSCPayloadGenerator.ts
🧰 Additional context used
🧠 Learnings (1)
node_package/src/injectRSCPayload.ts (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.599Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
…s to use URL-encoded props format.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
node_package/tests/registerServerComponent.client.test.jsx (5)
3-6: Restore globals patched for the webpack-runtime after the test suite
window.__webpack_require__andwindow.__webpack_chunk_load__are permanently overridden for the whole Jest JVM.
If any other test file relies on the real implementations (or stubs them differently) this will bleed across suites.Consider snapshotting the originals in a
beforeAlland putting them back in anafterAll:-beforeAll(() => { - window.__webpack_require__ = jest.fn(); - window.__webpack_chunk_load__ = jest.fn(); -}); +let originalWebpackRequire; +let originalWebpackChunkLoad; + +beforeAll(() => { + originalWebpackRequire = window.__webpack_require__; + originalWebpackChunkLoad = window.__webpack_chunk_load__; + window.__webpack_require__ = jest.fn(); + window.__webpack_chunk_load__ = jest.fn(); +}); + +afterAll(() => { + window.__webpack_require__ = originalWebpackRequire; + window.__webpack_chunk_load__ = originalWebpackChunkLoad; +});
20-20: Suite title is outdatedThe tests exercise
registerServerComponent, yet thedescribelabel is still"RSCClientRoot", a leftover from the removed suite.
Renaming improves greppability and prevents confusion in test reports.
51-60: Reduce filesystem churn insidemockRSCRequestReading the same two fixture files for every invocation (and therefore many times per test run) adds ~20 ms per call on SSDs and far more on CI.
Cache the parsed chunks at module scope:-const mockRSCRequest = async (...) => { - const chunksDirectory = path.join(__dirname, ...); - const chunk1 = JSON.parse(fs.readFileSync(...)); - const chunk2 = JSON.parse(fs.readFileSync(...)); +const chunksDirectory = path.join(__dirname, 'fixtures', 'rsc-payloads', 'simple-shell-with-async-component'); +const CHUNK1 = JSON.parse(fs.readFileSync(path.join(chunksDirectory, 'chunk1.json'), 'utf8')); +const CHUNK2 = JSON.parse(fs.readFileSync(path.join(chunksDirectory, 'chunk2.json'), 'utf8')); + +const mockRSCRequest = async (...) => {
111-140: Console spy is never restored
jest.spyOn(console, 'log')persists after the test and can hide real console output from later suites.
AddconsoleSpy.mockRestore()in afinallyblock or inafterEach.
183-189: Prefer reassignment overdeletefor perf-critical globalsStatic analysis flagged the
deleteoperator.
Since this is a predictable property onwindow, set it toundefinedor an empty object instead:-delete window.REACT_ON_RAILS_RSC_PAYLOADS; +window.REACT_ON_RAILS_RSC_PAYLOADS = undefined;It avoids the hidden-class de-opt in V8 while achieving the same effect.
🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 188-188: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node_package/tests/registerServerComponent.client.test.jsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
node_package/tests/registerServerComponent.client.test.jsx
[error] 183-183: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 188-188: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (2)
node_package/tests/registerServerComponent.client.test.jsx (2)
24-33:jest.resetModules()alone will not clear dynamic mocks declared inside testsBecause
jest.mock()is hoisted, the manual mock inside the"React.use is not defined"test (lines 40-43) will persist for the remainder of the file despite thisbeforeEach.
Wrap that test injest.isolateModules()or explicitly calljest.unmock('react')in anafterEachto avoid module-cache contamination.
213-215:document.readyStatemay be non-configurable in JSDOMOverwriting built-in DOM props can throw in some JSDOM versions.
Wrap theObject.definePropertymutations intry/catch, or usejest.spyOn(document, 'readyState', 'get')with a mock getter to stay future-proof.
| await act(async () => { | ||
| pushFirstChunk(); | ||
| render(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nested act() blocks may trigger React-dom warnings
render() itself wraps its work in act(), yet you call it inside another act().
React warns against nested act() calls because the inner one is a no-op in strict mode.
Restructure to:
await pushFirstChunk();
await render(); // render already handles act()🤖 Prompt for AI Agents
In node_package/tests/registerServerComponent.client.test.jsx around lines 93 to
96, the test code wraps a call to render() inside an act() block, but render()
already includes its own act() wrapper, causing nested act() calls and React
warnings. To fix this, remove the outer act() and instead await pushFirstChunk()
directly, then await render() separately, ensuring no nested act() calls occur.
…processing by adding catch blocks for promise rejections and error events in streams.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
node_package/src/injectRSCPayload.ts (1)
68-76: Still relying on optional chaining – previous feedback not fully addressed.You log an error when
onRSCPayloadGeneratedis missing, but the call at line 76 still uses optional chaining, so the callback is silently skipped in that scenario, defeating the purpose of the check.
This is the same concern raised in an earlier review.node_package/src/types/index.ts (3)
20-64:serverSideRSCPayloadParametersshould be consistently required or optional.It remains optional here while other union members treat it as required, reproducing the previously-reported type-safety hole.
72-78: Inconsistent optionality propagates into the derived type.
RailsContextWithServerComponentCapabilitiesstill marksserverSideRSCPayloadParametersoptional, contradicting the run-time assumptions downstream.
91-110: Assertion helper still omitsserverSideRSCPayloadParameters.The runtime guard will pass even when the payload parameters are missing, leading to late crashes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
node_package/src/ReactOnRailsRSC.ts(2 hunks)node_package/src/getReactServerComponent.server.ts(1 hunks)node_package/src/injectRSCPayload.ts(1 hunks)node_package/src/types/index.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- node_package/src/ReactOnRailsRSC.ts
- node_package/src/getReactServerComponent.server.ts
🧰 Additional context used
🧠 Learnings (1)
node_package/src/injectRSCPayload.ts (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.599Z
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.
🧬 Code Graph Analysis (1)
node_package/src/injectRSCPayload.ts (2)
node_package/src/types/index.ts (2)
RailsContextWithServerComponentCapabilities(72-77)ReactOnRails(281-350)node_package/src/utils.ts (1)
createRSCPayloadKey(15-21)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (3)
node_package/src/injectRSCPayload.ts (2)
14-17:escapeScriptimplementation looks correct.The replacement logic correctly prevents premature
<!--and</script>terminations without over-escaping.
125-127: Good catch propagatinghtmlStreamerrors.Forwarding the error prevents silent failures higher up the pipeline.
node_package/src/types/index.ts (1)
345-349:addPostSSRHooksignature looks sound.Clear API and correctly typed – no issues spotted.
This reverts commit 9206e01.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
node_package/src/injectRSCPayload.ts (1)
68-77: Still silently skips RSC payload injection when the callback is undefinedEarlier feedback asked to remove the optional chaining and fail loudly.
Here you added a console error but kept?., so the function short-circuits and nothing happens, leaving the app in a partially rendered state with no hard failure or stack trace.-ReactOnRails.onRSCPayloadGenerated?.(railsContext, (streamInfo) => { +if (!ReactOnRails.onRSCPayloadGenerated) { + throw new Error( + 'ReactOnRails.onRSCPayloadGenerated is not defined. RSC payloads cannot be injected.', + ); +} + +ReactOnRails.onRSCPayloadGenerated(railsContext, (streamInfo) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node_package/src/injectRSCPayload.ts(1 hunks)node_package/tests/injectRSCPayload.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- node_package/tests/injectRSCPayload.test.ts
🧰 Additional context used
🧠 Learnings (1)
node_package/src/injectRSCPayload.ts (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (1)
node_package/src/injectRSCPayload.ts (1)
116-123: Ordering fix looks good – guarantees array initialisation precedes HTML flushThe
startRSC()invocation now happens before flushing buffered HTML, ensuring the client sees the payload array before parsing component markup. This removes the hydration race condition flagged in the last review. 👍
| import { createRSCPayloadKey } from './utils.ts'; | ||
| import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts'; |
There was a problem hiding this comment.
Remove .ts extensions from import specifiers to avoid runtime resolution failures
The source is compiled to JavaScript (.js) during the build. Keeping the .ts extension in the module specifier makes Node / Webpack look for a utils.ts file at runtime, which will not exist after compilation and will throw ERR_MODULE_NOT_FOUND.
-import { createRSCPayloadKey } from './utils.ts';
-import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts';
+import { createRSCPayloadKey } from './utils';
+import {
+ RailsContextWithServerComponentCapabilities,
+ PipeableOrReadableStream,
+} from './types';📝 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.
| import { createRSCPayloadKey } from './utils.ts'; | |
| import { RailsContextWithServerComponentCapabilities, PipeableOrReadableStream } from './types/index.ts'; | |
| import { createRSCPayloadKey } from './utils'; | |
| import { | |
| RailsContextWithServerComponentCapabilities, | |
| PipeableOrReadableStream, | |
| } from './types'; |
🤖 Prompt for AI Agents
In node_package/src/injectRSCPayload.ts at lines 3 to 4, remove the '.ts'
extensions from the import statements for './utils.ts' and './types/index.ts'.
Change them to './utils' and './types/index' respectively to ensure that after
compilation to JavaScript, the runtime module resolution does not fail due to
looking for non-existent '.ts' files.
Using RSC Payload to Render Server Components on Server
Overview
This PR implements a significant architectural improvement to the React Server Components (RSC) rendering flow in both
react_on_railsandreact_on_rails_pro. Previously, server components were rendered twice - once for HTML generation and again for RSC payload. This PR unifies the process by generating the RSC payload first and using it for server-side rendering, eliminating double rendering and reducing HTTP requests.Previous Rendering Flow
Before this improvement, the React Server Components rendering process worked as follows:
Initial HTML Generation:
stream_react_componenthelper is called in the viewRSC Payload Generation:
rsc_payload_react_componenton the serverClient Hydration:
This approach had two main inefficiencies:
Double Rendering: Server components are rendered twice:
Multiple Requests: Requires two separate HTTP requests:
sequenceDiagram participant Browser participant View participant NodeRenderer participant ServerBundle participant RSCBundle Note over Browser,RSCBundle: 1. Initial HTML Generation Browser->>View: Request page View->>NodeRenderer: stream_react_component NodeRenderer->>ServerBundle: Generate HTML ServerBundle-->>NodeRenderer: HTML for all components NodeRenderer-->>Browser: Stream HTML Note over Browser,RSCBundle: 2. RSC Payload Generation Browser->>NodeRenderer: Fetch RSC payload NodeRenderer->>RSCBundle: rsc_payload_react_component RSCBundle-->>NodeRenderer: RSC payload with:<br/>- Server components<br/>- Client component refs NodeRenderer-->>Browser: Stream RSC payload Note over Browser: 3. Client Hydration Browser->>Browser: Process RSC payload loop For each client component Browser->>Browser: Fetch component chunk Browser->>Browser: Hydrate component endNote
For simplicity, this diagram shows the RSC payload being fetched after the HTML is fully streamed to the client. In reality, the browser begins fetching the RSC payload and starts hydration immediately as soon as it receives the necessary HTML, without waiting for the complete page to be streamed. This parallel processing enables faster page interactivity and better performance.
Key Improvements
Technical Implementation
Inter-Bundle Communication
The core of this implementation is a mechanism for communication between bundles in the node renderer:
runOnOtherBundlefunction in the VM context that allows executing code in a different bundlegenerateRSCPayloadfunction that usesrunOnOtherBundleto run rendering requests in the RSC bundleKey Components for RSC Rendering
Two new components are introduced in React on Rails to facilitate server-side RSC rendering:
RSCServerRoot
RSCServerRootis the core component responsible for:generateRSCPayloadfunctionThis component operates as follows:
RSCPayloadContainer
RSCPayloadContainerhandles embedding the RSC payload within the HTML response by:self.REACT_ON_RAILS_RSC_PAYLOADThis creates a seamless experience where:
New Rendering Flow
When
stream_react_componentis called:generateRSCPayloadwith the component name and propsThe server bundle then:
On the client:
Configuration and Detection
ReactOnRails.isRSCBundleflag to identify their special purposeVisual Representation
sequenceDiagram participant Browser participant View participant NodeRenderer participant RSCBundle participant ServerBundle Note over Browser,ServerBundle: 1. Initial Request Browser->>View: Request page View->>NodeRenderer: stream_react_component NodeRenderer->>ServerBundle: Execute rendering request ServerBundle->>RSCBundle: generateRSCPayload(component, props) RSCBundle-->>ServerBundle: RSC payload with:<br/>- Server components<br/>- Client component refs ServerBundle-->>NodeRenderer: Generate HTML using RSC payload Note over Browser,ServerBundle: 2. Single Response NodeRenderer-->>Browser: Stream HTML with embedded RSC payload Note over Browser: 3. Client Hydration Browser->>Browser: Process embedded RSC payload loop For each client component Browser->>Browser: Fetch component chunk Browser->>Browser: Hydrate component endTesting
serverRenderRSCReactComponentfunctionPerformance Improvements
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
RSCRoute,RSCProvider,ServerComponentFetchError, and error boundaries.Improvements
Bug Fixes
Tests
Chores