[WIP] Use RSC payload to render server components on server #1696
Conversation
WalkthroughThis pull request expands the configuration and runtime support for server-side rendering of React Server Components. New constants, methods, and components have been introduced and updated across configuration files, rendering functions, stream transformations, and registration methods. The changes update the manifest handling, add new streaming utilities and error checks, adjust type definitions, and revise tests and package exports accordingly—all to enhance the management and rendering of server components. Changes
Sequence Diagram(s)RSC Server-Side Rendering FlowsequenceDiagram
participant Client
participant RSCServerRoot
participant generateRSCPayload
participant StreamTransformer
participant Renderer
Client->>RSCServerRoot: Request render(componentName, props)
RSCServerRoot->>generateRSCPayload: Invoke generateRSCPayload(...)
generateRSCPayload-->>RSCServerRoot: Return payload stream
RSCServerRoot->>StreamTransformer: Transform payload stream
StreamTransformer-->>RSCServerRoot: Transformed stream
RSCServerRoot->>Renderer: Create React element from stream
Renderer-->>RSCServerRoot: Rendered component
RSCServerRoot->>Client: Deliver rendered output
Streaming Rendering & Error Handling FlowsequenceDiagram
participant Client
participant streamRenderReactComponent
participant PromiseResolver
participant Renderer
participant ErrorHandler
Client->>streamRenderReactComponent: Request streaming render
streamRenderReactComponent->>PromiseResolver: Resolve reactRenderingResult
alt Resolved value is a string
PromiseResolver-->>streamRenderReactComponent: String result
streamRenderReactComponent->>ErrorHandler: Log error and write error stream
else Resolved value is a React element
PromiseResolver-->>streamRenderReactComponent: React element result
streamRenderReactComponent->>Renderer: Create rendering stream
Renderer-->>streamRenderReactComponent: Streaming output
end
streamRenderReactComponent->>Client: Send rendered stream output
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
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)
| @@ -20,7 +20,7 @@ | |||
| } | |||
|
|
|||
| const renderFunction = isRenderFunction(component); | |||
| const isRenderer = renderFunction && component.length === 3; | |||
| const isRenderer = renderFunction && (component as RenderFunction).length === 3; | |||
There was a problem hiding this comment.
Fix missing type import causing build failures
The build is failing because RenderFunction is not imported. You need to import this type from the types module.
-import { type RegisteredComponent, type ReactComponentOrRenderFunction } from './types';
+import { type RegisteredComponent, type ReactComponentOrRenderFunction, type RenderFunction } 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.
| const isRenderer = renderFunction && (component as RenderFunction).length === 3; | |
| import { type RegisteredComponent, type ReactComponentOrRenderFunction, type RenderFunction } from './types'; | |
| ... | |
| const isRenderer = renderFunction && (component as RenderFunction).length === 3; |
🧰 Tools
🪛 GitHub Check: build
[failure] 23-23:
Cannot find name 'RenderFunction'.
🪛 GitHub Check: examples (oldest)
[failure] 23-23:
Cannot find name 'RenderFunction'.
🪛 GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
[failure] 23-23:
Cannot find name 'RenderFunction'.
🪛 GitHub Check: examples (newest)
[failure] 23-23:
Cannot find name 'RenderFunction'.
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
[error] 23-23: TS2304: Cannot find name 'RenderFunction'.
| @@ -55,15 +58,15 @@ | |||
| "publint": "^0.3.8", | |||
| "react": "^19.0.0", | |||
| "react-dom": "^19.0.0", | |||
| "react-on-rails-rsc": "19.0.0", | |||
| "react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc.git#add-support-for-generating-client-components-manifest-for-server-bundle", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Using a GitHub branch as dependency may be unstable for production
Both the dependencies and peerDependencies are pointing to a specific GitHub branch rather than a published version. This approach is suitable for development but might lead to instability in production environments.
Consider switching to a published version when the feature is stable or add a comment explaining the temporary nature of this dependency.
Also applies to: 69-69
🌐 Web query:
What are the pros and cons of using a GitHub branch as a dependency in package.json?
💡 Result:
When using a GitHub branch as a dependency in package.json, developers gain flexibility but introduce trade-offs in reliability and maintenance:
Pros
-
Access to unreleased features
Directly reference branches or commits to test experimental changes or patches not yet published to npm[1][7][12]. For example:"dependencies": { "chalk": "github:chalk/chalk#feature/new-color-api" }
-
Temporary fixes for forks
Quickly integrate fixes from a fork without waiting for upstream approval[12][14]. This is particularly useful for addressing urgent bugs in unmaintained projects. -
Private repository support
Reference private GitHub repositories as dependencies without publishing to npm[3][4][7], useful for internal tools or proprietary code. -
Semver flexibility
Pin dependencies to specific tags or ranges using GitHub’ssemver:syntax (e.g.,github:user/repo#semver:^2.4.1) to avoid unintended updates[1][7].
Cons
-
Dependency instability
Branches can be force-pushed or deleted, breaking builds if the referenced commit/branch becomes unavailable[4][7][10]. This is riskier than version-locked npm packages. -
Version management challenges
Tools likenpm outdatedor automated dependency updaters may not track GitHub-based dependencies effectively[6][10][12], complicating updates and audits. -
Increased merge conflicts
Frequent updates to active branches may require repeated adjustments in dependent projects, especially in team environments[8][11]. -
Longer installation times
Cloning repositories duringnpm installis slower than fetching packaged tarballs from npm[4][7][12], impacting CI/CD pipelines. -
Tooling incompatibilities
Some build systems (e.g., Nx) struggle with GitHub URLs inpackage.json, causing errors during tasks like generating lockfiles[10][14]. -
License/security risks
GitHub dependencies may bypass automated license compliance checks and vulnerability scans, increasing legal/security exposure[9][13].
When to Use
- For short-term testing of unreleased features.
- When maintaining a fork with critical fixes not yet merged upstream.
- For private packages not intended for public npm distribution.
Alternatives
- Publish forks to npm as scoped packages (e.g.,
@myorg/chalk) for stability[3]. - Use
npm packoryarn linkfor local development before official releases[7][14].
For long-term projects, prefer npm-published versions with strict package-lock.json/yarn.lock policies[6][14]. Reserve GitHub dependencies for scenarios requiring immediate access to unreleased code.
Citations:
- 1: https://dev.to/michalbryxi/github-urls-in-package-json-5412
- 2: https://gitprotect.io/blog/from-novice-to-pro-understanding-git-branching-strategies/
- 3: http://nodesource.com/blog/github-package-registry/
- 4: https://www.curiouslychase.com/posts/install-npm-dependency-from-github-url
- 5: https://www.gitkraken.com/learn/git/best-practices/git-branch-strategy
- 6: https://dev.to/adamklein/package-lock-json-in-git-or-not-50l5
- 7: https://www.warp.dev/terminus/npm-install-from-github
- 8: https://launchdarkly.com/blog/git-branching-strategies-vs-trunk-based-development/
- 9: https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/exploring-the-dependencies-of-a-repository
- 10: Using Github branch as dependency in package.json cause build (--generatedPackageJson) to fail nrwl/nx#13868
- 11: https://docs.aws.amazon.com/prescriptive-guidance/latest/choosing-git-branch-approach/advantages-and-disadvantages-of-the-git-hub-flow-strategy.html
- 12: https://martinwolf.org/2018/04/github-branch-as-dependency-package-json/
- 13: https://docs.github.com/code-security/supply-chain-security/understanding-your-software-supply-chain/about-dependency-review
- 14: Use specific commit or branch in package.json ionic-team/capacitor#3798
- 15: https://docs.npmjs.com/files/package.json/
- 16: How to point dependency to a github branch codesandbox/codesandbox-client#1466
Dependency Instability Risk – Update Required
- The dependency for
"react-on-rails-rsc"(as well as the corresponding entry inpeerDependencies) references a GitHub branch. Although this can facilitate rapid testing of experimental features, it poses stability risks in production since branches can be force-pushed, deleted, or modified. - Recommendation:
- Switch to a Published Version: Once the experimental feature stabilizes, update the dependency to a released npm version to ensure reliability in production.
- Or Document Its Temporary Nature: If you must continue using the GitHub branch for now, add a clear comment in
package.jsonexplaining that this dependency is temporary and why the branch is being used.
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.
| // 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); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for stream operations.
Stream operations can fail, but there's no error handling for the pipe operations. Consider adding error handlers to manage potential stream 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', (error) => {
+ console.error('Error in RSC payload stream 1:', error);
+ // Handle error appropriately (e.g., close streams, emit error event)
+});
const rscPayloadStream2 = new PassThrough();
-rscPayloadStream.pipe(rscPayloadStream2);
+rscPayloadStream.pipe(rscPayloadStream2).on('error', (error) => {
+ console.error('Error in RSC payload stream 2:', error);
+ // Handle error appropriately (e.g., close streams, emit error event)
+});📝 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.
| // 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); | |
| // Tee the stream to pass it to the server component and the payload container | |
| const rscPayloadStream1 = new PassThrough(); | |
| rscPayloadStream.pipe(rscPayloadStream1).on('error', (error) => { | |
| console.error('Error in RSC payload stream 1:', error); | |
| // Handle error appropriately (e.g., close streams, emit error event) | |
| }); | |
| const rscPayloadStream2 = new PassThrough(); | |
| rscPayloadStream.pipe(rscPayloadStream2).on('error', (error) => { | |
| console.error('Error in RSC payload stream 2:', error); | |
| // Handle error appropriately (e.g., close streams, emit error event) | |
| }); | |
| const serverComponentElement = createFromReactOnRailsNodeStream(rscPayloadStream1, ssrManifest); |
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.
| @@ -82,7 +115,7 @@ describe('streamServerRenderedReactComponent', () => { | |||
| it('streamServerRenderedReactComponent streams the rendered component', async () => { | |||
| const { renderResult, chunks } = setupStreamTest(); | |||
| await new Promise((resolve) => { | |||
| renderResult.on('end', resolve); | |||
| renderResult.once('end', resolve); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug fix: Using once('end', resolve) instead of on('end', resolve)
Using once() instead of on() 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
|
|
||
| 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')); | |
| } |
| if (chunkIndex > chunkPromises.length) { | ||
| throw new Error('React on Rails Error: RSC Chunk index out of bounds'); | ||
| } |
There was a problem hiding this comment.
Fix 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');
}📝 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.
| if (chunkIndex > chunkPromises.length) { | |
| throw new Error('React on Rails Error: RSC Chunk index out of bounds'); | |
| } | |
| if (chunkIndex >= chunkPromises.length) { | |
| throw new Error('React on Rails Error: RSC Chunk index out of bounds'); | |
| } |
Summary
This PR resolves the issue causing server components to render twice, as explained here:
🔗 https://github.com/shakacode/react_on_rails_pro/blob/2618750/docs/react-server-components-rendering-flow.md
It implements the solution proposed in the Future Improvements section of the document.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Tests