[WIP] Add support for RSC #1644
Conversation
WalkthroughThis pull request introduces comprehensive support for React Server Components (RSC) in the React on Rails library. The changes span multiple files, adding new methods, modifying existing implementations, and enhancing the library's capability to handle server-side rendering with streaming. Key modifications include introducing Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
spec/dummy/config/webpack/alias.js (1)
7-7: Add a comment explaining the alias mapping.Adding a comment to explain why the
streamalias is needed would improve maintainability.+ // Alias for stream module to ensure compatibility with browser environments stream: 'stream-browserify'spec/dummy/config/webpack/webpackConfig.js (1)
7-7: Add a comment explaining the fallback configuration.Adding a comment to explain why the
streamfallback is needed would improve maintainability.+ // Fallback configuration for stream module to ensure compatibility with browser environments clientConfig.resolve.fallback = { stream: require.resolve('stream-browserify') };node_package/src/ReactOnRailsRSC.ts (2)
13-20: Handle the absence ofAsyncLocalStoragemore robustly.Consider providing more details in the log message or handling the absence of
AsyncLocalStoragein a way that informs the user of potential limitations.- console.log('AsyncLocalStorage not found'); + console.warn('AsyncLocalStorage not found. Some features may not work as expected.');
29-76: Ensure comprehensive test coverage forserverRenderRSCReactComponent.Given the complexity of this function, ensure that it is comprehensively tested, especially for error handling and edge cases.
- No references to
serverRenderRSCReactComponentwere found in the existing test files.Analysis chain
Ensure comprehensive test coverage for
serverRenderRSCReactComponent.Given the complexity of this function, ensure that it is comprehensively tested, especially for error handling and edge cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if comprehensive tests are provided for `serverRenderRSCReactComponent`. # Test: Search for test cases related to `serverRenderRSCReactComponent`. Expect: Comprehensive test cases. rg --type javascript --type typescript 'serverRenderRSCReactComponent'Length of output: 105
Script:
#!/bin/bash # Description: Verify if comprehensive tests are provided for `serverRenderRSCReactComponent`. # Test: Search for test cases related to `serverRenderRSCReactComponent`. Expect: Comprehensive test cases. rg --type js --type ts 'serverRenderRSCReactComponent'Length of output: 488
Script:
#!/bin/bash # Description: Verify if comprehensive tests are provided for `serverRenderRSCReactComponent`. # Step 1: Find all test files in the repository. test_files=$(fd --type f --extension js --extension ts --glob '*test*') # Step 2: Search for references to `serverRenderRSCReactComponent` in the identified test files. if [ -n "$test_files" ]; then rg 'serverRenderRSCReactComponent' $test_files else echo "No test files found." fiLength of output: 1289
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (11)
- lib/react_on_rails/helper.rb (4 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1 hunks)
- lib/react_on_rails/utils.rb (2 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/ReactOnRailsRSC.ts (1 hunks)
- node_package/src/serverRenderReactComponent.ts (4 hunks)
- node_package/src/types/index.ts (2 hunks)
- package.json (3 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
- spec/dummy/config/webpack/webpackConfig.js (1 hunks)
Additional comments not posted (26)
node_package/src/ReactOnRailsRSC.ts (3)
22-27: LGTM!The
stringToStreamfunction is correctly implemented.
78-79: LGTM!The export statements are correctly implemented.
1-4: Ensure type definitions are added for the modules.The
@ts-expect-errorcomments indicate that type definitions are pending. Ensure these are added to avoid type errors.Verification successful
Ensure type definitions are added for the modules.
The
@ts-expect-errorcomments indicate that type definitions are pending for therenderToReadableStreamandPassThroughmodules. Ensure these are added to avoid type errors.
node_package/src/ReactOnRailsRSC.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if type definitions for the modules are added or tracked in the codebase. # Test: Search for type definition files or TODO comments. Expect: Type definitions or tracking comments. rg --type typescript -A 2 '@ts-expect-error'Length of output: 79
Script:
#!/bin/bash # Description: Verify if type definitions for the modules are added or tracked in the codebase. # Test: Search for type definition files or TODO comments. Expect: Type definitions or tracking comments. rg --type-add 'ts:*.ts' -A 2 '@ts-expect-error'Length of output: 883
package.json (5)
23-23: LGTM!Updating
@types/reactto^18.2.0ensures better type safety and compatibility with the latest React version.
24-24: LGTM!Updating
@types/react-domto^18.2.0ensures better type safety and compatibility with the latest React version.
49-49: LGTM! Verify the usage of the new dependency.Adding
react-server-dom-webpackenhances server-side rendering capabilities.However, ensure that the new dependency is correctly utilized in the codebase.
Verification successful
Verified Usage of
react-server-dom-webpackThe
react-server-dom-webpackdependency is correctly utilized in the codebase, specifically innode_package/src/ReactOnRailsRSC.ts.
node_package/src/ReactOnRailsRSC.ts: Import and usage ofrenderToReadableStreamfromreact-server-dom-webpack/server.edge.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new dependency in the codebase. # Test: Search for the usage of `react-server-dom-webpack`. Expect: Correct usage of the new dependency. rg --type js -A 5 $'react-server-dom-webpack'Length of output: 44
Script:
#!/bin/bash # Description: Verify the usage of the new dependency in the codebase. # Test: Search for the usage of `react-server-dom-webpack` in all file types. rg -A 5 'react-server-dom-webpack' # Test: Search for import statements that might alias `react-server-dom-webpack`. rg -A 5 'import .* from .*react-server-dom-webpack'Length of output: 1847
5-10: LGTM! Verify the usage of new entry points.The addition of the
exportsfield enhances module interoperability by specifying entry points for both server-side and default usage.However, ensure that the new entry points are correctly utilized in the codebase.
47-48: LGTM! Verify compatibility with the rest of the codebase.Updating
reactandreact-domto18.3.0-canary-670811593-20240322aligns with modern React practices but ensure compatibility with the rest of the codebase.Verification successful
Verify compatibility with the rest of the codebase.
Updating
reactandreact-domto18.3.0-canary-670811593-20240322aligns with modern React practices but ensure compatibility with the rest of the codebase by checking for deprecated or breaking changes.
Verify compatibility with the rest of the codebase.
Updating
reactandreact-domto18.3.0-canary-670811593-20240322aligns with modern React practices but ensure compatibility with the rest of the codebase by conducting a comprehensive check.
LGTM! Verify compatibility with the rest of the codebase.
Updating
reactandreact-domto18.3.0-canary-670811593-20240322aligns with modern React practices. The search did not reveal any deprecated lifecycle methods or breaking changes. Ensure compatibility with the rest of the codebase by conducting manual verification if necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify compatibility with the rest of the codebase. # Test: Search for the usage of `react` and `react-dom`. Expect: No compatibility issues. rg --type js -A 5 $'react' rg --type js -A 5 $'react-dom'Length of output: 44790
Script:
#!/bin/bash # Description: Verify compatibility with the rest of the codebase for deprecated or breaking changes. # Test: Search for deprecated React lifecycle methods that might be affected by the update. rg --type js -A 5 'componentWillMount|componentWillReceiveProps|componentWillUpdate' # Test: Search for usage of ReactDOM methods that might have breaking changes. rg --type js -A 5 'ReactDOM.render|ReactDOM.hydrate|ReactDOM.createPortal'Length of output: 163
Script:
#!/bin/bash # Description: Verify compatibility with the rest of the codebase for React components and ReactDOM usages. # Test: Search for all React component definitions. rg --type js -A 5 'React.Component|React.PureComponent|function\s+\w+\s*\(' # Test: Search for all ReactDOM usages. rg --type js -A 5 'ReactDOM.render|ReactDOM.hydrate|ReactDOM.createRoot|ReactDOM.hydrateRoot'Length of output: 10621
lib/react_on_rails/react_component/render_options.rb (2)
106-108: LGTM!The
stream?method provides a straightforward way to query the:stream?option.
110-112: LGTM!The
rsc?method provides a straightforward way to query the:rsc?option.node_package/src/types/index.ts (2)
142-142: LGTM! Verify the implementation of the new method.The
serverRenderRSCReactComponentmethod enhances performance and user experience by enabling progressive rendering.However, ensure that the new method is correctly implemented in the codebase.
141-141: LGTM! Verify the implementation of the new method.The
streamServerRenderedReactComponentmethod enhances performance and user experience by enabling progressive rendering.However, ensure that the new method is correctly implemented in the codebase.
Verification successful
The
streamServerRenderedReactComponentmethod is correctly implemented.The method is defined in
serverRenderReactComponent.tsand handles the streaming of server-rendered React components usingReactDOMServer.renderToPipeableStream. It also includes error handling to manage exceptions during rendering.
- Implementation found in
node_package/src/serverRenderReactComponent.tsfrom lines 209 to 249.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new method in the codebase. # Test: Search for the implementation of `streamServerRenderedReactComponent`. Expect: Correct implementation of the new method. rg --type ts -A 5 $'streamServerRenderedReactComponent'Length of output: 2064
Script:
#!/bin/bash # Description: Extract the full implementation of the `streamServerRenderedReactComponent` method. # Test: Retrieve the implementation details of `streamServerRenderedReactComponent`. rg --type ts -A 20 $'export const streamServerRenderedReactComponent = (options: RenderParams) => {'Length of output: 321
Script:
#!/bin/bash # Description: Extract the full implementation of the `streamServerRenderedReactComponent` method. # Test: Retrieve the implementation details of `streamServerRenderedReactComponent`. rg --type ts -A 50 $'export const streamServerRenderedReactComponent = \\(options: RenderParams\\) => {'Length of output: 327
Script:
#!/bin/bash # Description: Extract the full implementation of the `streamServerRenderedReactComponent` method using ast-grep. # Test: Retrieve the implementation details of `streamServerRenderedReactComponent`. ast-grep --lang typescript --pattern $'export const streamServerRenderedReactComponent = (options: RenderParams) => { $$$ }'Length of output: 3488
node_package/src/serverRenderReactComponent.ts (3)
1-2: Update import statements forReactandPipeableStream.The import of
ReactandPipeableStreamfromreact-dom/serveraligns with the new stream-based rendering approach.
74-101: Implement stream-based rendering usingrenderToPipeableStream.The new implementation uses
renderToPipeableStreamfor stream-based rendering. This approach enhances performance and allows for asynchronous processing of the rendered output. Ensure that the stream is correctly handled and errors are properly managed.
209-249: IntroducestreamServerRenderedReactComponentfunction.The new function encapsulates the logic for rendering a React component into a stream. It checks for conditions and handles errors appropriately. This addition enhances the server-side rendering capabilities by supporting stream-based rendering.
lib/react_on_rails/utils.rb (3)
Line range hint
69-92:
Redefinebundle_js_file_pathmethod.The method now includes a new parameter
bundle_name, allowing it to handle different bundle names dynamically. This change enhances the flexibility and robustness of the method.
96-110: Updateserver_bundle_js_file_pathmethod.The method now uses the new
bundle_js_file_pathmethod, aligning with the new structure for managing bundle paths. This update enhances maintainability and clarity.
112-117: Introducersc_bundle_js_file_pathmethod.The new method handles the RSC bundle path, following the same pattern as the
server_bundle_js_file_pathmethod. This addition improves the modularity and configurability of the code.lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
95-99: Introduceexec_server_render_streaming_jsmethod.The new method handles JavaScript code execution in a streaming manner. It accepts three parameters:
js_code,render_options, and an optionaljs_evaluator. This addition expands the capabilities of the module by allowing for streaming JavaScript execution.node_package/src/ReactOnRails.ts (2)
245-251: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
streamServerRenderedReactComponentare correctly implemented.
253-260: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that the function
serverRenderRSCReactComponentis not mistakenly invoked in the wrong context.Verification successful
The function
serverRenderRSCReactComponentis not mistakenly invoked in the wrong context.
- The function is defined in
node_package/src/ReactOnRails.tsand throws an error if invoked.- It is referenced in
node_package/src/ReactOnRailsRSC.tsandnode_package/src/types/index.ts, but no invocations were found outside these contexts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `serverRenderRSCReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ts -A 5 $'serverRenderRSCReactComponent'Length of output: 1445
Script:
#!/bin/bash # Description: Verify the context of the function usage in `node_package/src/ReactOnRailsRSC.ts` and `node_package/src/types/index.ts`. # Search for the function usage in `node_package/src/ReactOnRailsRSC.ts` with more context lines. rg --type ts -A 10 -B 10 $'serverRenderRSCReactComponent' node_package/src/ReactOnRailsRSC.ts # Search for the function usage in `node_package/src/types/index.ts` with more context lines. rg --type ts -A 10 -B 10 $'serverRenderRSCReactComponent' node_package/src/types/index.tsLength of output: 1701
Script:
#!/bin/bash # Description: Verify that `serverRenderRSCReactComponent` is not mistakenly invoked outside the intended context. # Search for invocations of `serverRenderRSCReactComponent` across the codebase. rg --type ts $'serverRenderRSCReactComponent('Length of output: 158
Script:
#!/bin/bash # Description: Verify that `serverRenderRSCReactComponent` is not mistakenly invoked outside the intended context. # Search for invocations of `serverRenderRSCReactComponent` across the codebase with corrected regex pattern. rg --type ts 'serverRenderRSCReactComponent\('Length of output: 252
lib/react_on_rails/helper.rb (5)
480-484: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
internal_rsc_react_componentare correctly implemented.
113-121: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
stream_react_component_asyncare correctly implemented.
103-111: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
stream_react_componentare correctly implemented.Verification successful
Verified: The function usage of
stream_react_componentis correctly implemented.The only usage of
stream_react_componentis within thestream_react_component_asyncmethod, and it is used appropriately in an asynchronous context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ruby -A 5 $'stream_react_component'Length of output: 1048
Script:
#!/bin/bash # Description: Verify all function calls to `stream_react_component` are correctly implemented across the entire codebase. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ruby 'stream_react_component'Length of output: 319
94-101: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
rsc_react_componentare correctly implemented.
393-417: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
build_react_component_result_for_server_streamed_contentare correctly implemented.Verification successful
Verify the function call context in the codebase.
The function
build_react_component_result_for_server_streamed_contentis called within the same file where it is defined. Ensure that the parametersresult[:result],result[:tag], andresult[:render_options]are correctly defined and passed in the calling context.
Verify the function call context in the codebase.
The previous script failed due to a regex parse error. Let's correct the search pattern and re-run the script to verify the context of the function call to
build_react_component_result_for_server_streamed_content.
Verify the return value of the
internal_react_componentmethod.The function call to
build_react_component_result_for_server_streamed_contentis found within thestream_react_componentmethod. We need to verify theinternal_react_componentmethod to ensure that it returns a hash with the keys:result,:tag, and:render_options.
Verify the complete implementation of the
internal_react_componentmethod.We need to see the full implementation of the
internal_react_componentmethod to confirm if it returns a hash with the keys:result,:tag, and:render_options.
Function usage verified successfully.
The function
build_react_component_result_for_server_streamed_contentis correctly implemented and called within the codebase. Theinternal_react_componentmethod returns a hash with the keys:render_options,:tag, and:result, ensuring that the parameters passed tobuild_react_component_result_for_server_streamed_contentare correctly defined.
stream_react_componentmethod callsinternal_react_component, which returns the necessary keys.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `build_react_component_result_for_server_streamed_content` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation of the function calls. rg --type ruby -A 5 $'build_react_component_result_for_server_streamed_content'Length of output: 994
Script:
#!/bin/bash # Description: Verify the context of the function call to `build_react_component_result_for_server_streamed_content` to ensure the parameters are correctly defined and passed. # Search for the context around the function call to examine the definition of `result`. rg --type ruby -A 10 -B 10 $'build_react_component_result_for_server_streamed_content('Length of output: 253
Script:
#!/bin/bash # Description: Verify the context of the function call to `build_react_component_result_for_server_streamed_content` to ensure the parameters are correctly defined and passed. # Search for the context around the function call to examine the definition of `result`. rg --type ruby -A 10 -B 10 $'build_react_component_result_for_server_streamed_content\('Length of output: 3034
Script:
#!/bin/bash # Description: Verify the return value of the `internal_react_component` method to ensure it returns a hash with the keys `:result`, `:tag`, and `:render_options`. # Search for the definition of the `internal_react_component` method. rg --type ruby -A 10 $'def internal_react_component'Length of output: 989
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the `internal_react_component` method. # Search for the complete implementation of the `internal_react_component` method. rg --type ruby -A 50 -B 10 $'def internal_react_component'Length of output: 4813
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
4e3ad5b to
440511d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
node_package/src/ReactOnRails.ts (1)
253-260: LGTM! Consider adding a more descriptive error message.The implementation looks good. Consider providing more context in the error message to help developers understand why this method is not supported.
- throw new Error('serverRenderRSCReactComponent is supported in RSC bundle only.'); + throw new Error('serverRenderRSCReactComponent is supported in RSC bundle only. Ensure you are using the correct bundle for React Server Components.');CHANGELOG.md (1)
8-8: Format bare URL as a proper link.Bare URLs should be formatted as proper links in markdown files.
- If you need help upgrading `react_on_rails`, `webpacker` to `shakapacker`, or JS packages, contact [email protected]. + If you need help upgrading `react_on_rails`, `webpacker` to `shakapacker`, or JS packages, contact [[email protected]](mailto:[email protected]).Tools
Markdownlint
8-8: null
Bare URL used(MD034, no-bare-urls)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (12)
- CHANGELOG.md (2 hunks)
- lib/react_on_rails/helper.rb (5 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1 hunks)
- lib/react_on_rails/utils.rb (2 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/ReactOnRailsRSC.ts (1 hunks)
- node_package/src/serverRenderReactComponent.ts (4 hunks)
- node_package/src/types/index.ts (2 hunks)
- package.json (3 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
- spec/dummy/config/webpack/webpackConfig.js (1 hunks)
Additional context used
Markdownlint
CHANGELOG.md
8-8: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (32)
spec/dummy/config/webpack/alias.js (1)
7-7: Alias addition forstreammodule looks good.The alias for
streammodule mapping tostream-browserifyenhances module resolution for browser environments.spec/dummy/config/webpack/webpackConfig.js (1)
7-7: Fallback configuration forstreammodule looks good.The fallback configuration for
streammodule usingstream-browserifyensures compatibility for environments where the nativestreammodule may not be available.node_package/src/ReactOnRailsRSC.ts (5)
1-12: Import statements look good.The import statements bring in necessary modules and types for the functionalities being introduced.
13-20: Async function for settingAsyncLocalStoragelooks good.The function attempts to set
AsyncLocalStoragein the global scope and handles its absence gracefully.
22-27:stringToStreamfunction looks good.The function correctly converts a string to a stream using
PassThrough.
29-37:getBundleConfigfunction looks good.The function correctly reads the JSON file and processes its keys to remove the
file://prefix.
88-89: Export statements look good.The export statements appropriately export types and the default
ReactOnRailsobject.package.json (2)
5-10: LGTM! Theexportsfield enhances module compatibility.The addition of the
exportsfield provides more granular control over how the module is imported in different environments. Ensure that the paths toReactOnRailsRSC.jsandReactOnRails.jsare correct.
23-24: Approve dependency updates but verify compatibility.The updates to
@types/react,@types/react-dom,react,react-dom, and the addition ofreact-server-dom-webpackare approved. Ensure that these updates are compatible with the rest of the codebase and do not introduce breaking changes.Also applies to: 47-49
lib/react_on_rails/react_component/render_options.rb (1)
106-113: LGTM! The new methodsstream?andrsc?are well integrated.The methods correctly retrieve the options from the
optionshash. Ensure that these options are set correctly in the codebase.node_package/src/types/index.ts (1)
2-2: LGTM! The new methodsstreamServerRenderedReactComponentandserverRenderRSCReactComponentare well defined.The methods are correctly integrated into the
ReactOnRailsinterface. Ensure that these methods are implemented correctly in the codebase.Also applies to: 141-142
node_package/src/serverRenderReactComponent.ts (4)
1-3: Imports look good.The new imports
ReactDOMServerandPassThroughare necessary for the new streaming rendering approach.
202-207: Function looks good.The
stringToStreamfunction correctly converts a string to a stream.
209-248: Function looks good.The
streamServerRenderedReactComponentfunction correctly manages the rendering process and handles errors using the new streaming rendering approach.However, ensure the new function is used correctly throughout the codebase.
Verification successful
Function looks good.
The
streamServerRenderedReactComponentfunction correctly manages the rendering process and handles errors using the new streaming rendering approach.However, ensure the new function is used correctly throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new function. # Test: Search for the function usage. Expect: Correct usage of `streamServerRenderedReactComponent`. rg --type ts -A 5 $'streamServerRenderedReactComponent'Length of output: 2064
74-101: New rendering approach looks good.The use of
renderToPipeableStreamimproves performance by streaming the rendered output. The function handles different rendering results and includes error handling.However, ensure the new function is used correctly throughout the codebase.
lib/react_on_rails/utils.rb (3)
Line range hint
69-95:
Method looks good.The
bundle_js_file_pathmethod has been renamed and modified to handle different bundle names dynamically. The error handling and default path construction are correct.
96-110: Method looks good.The
server_bundle_js_file_pathmethod has been modified to use the newbundle_js_file_pathmethod, improving maintainability.
112-117: Method looks good.The new
rsc_bundle_js_file_pathmethod improves configurability by managing the retrieval of a resource bundle path.lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
95-99: Method looks good.The new
exec_server_render_streaming_jsmethod enhances functionality by providing an alternative for JavaScript execution with streaming capabilities.node_package/src/ReactOnRails.ts (1)
245-251: LGTM! Verify the correct usage ofRenderParamsandstreamServerRenderedReactComponent.The implementation looks good. Ensure that
RenderParamsandstreamServerRenderedReactComponentare correctly defined and imported.Verification successful
Verified: The
RenderParamsandstreamServerRenderedReactComponentare correctly defined and imported.
RenderParamsis defined innode_package/src/types/index.ts.streamServerRenderedReactComponentis defined innode_package/src/serverRenderReactComponent.tsand imported innode_package/src/ReactOnRails.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `RenderParams` and `streamServerRenderedReactComponent`. # Test: Search for the definitions of `RenderParams` and `streamServerRenderedReactComponent`. Expect: Correct definitions and imports. rg --type typescript -A 5 $'RenderParams' rg --type typescript -A 5 $'streamServerRenderedReactComponent'Length of output: 173
Script:
#!/bin/bash # Description: Verify the correct usage of `RenderParams` and `streamServerRenderedReactComponent`. # Test: Search for the definitions of `RenderParams` and `streamServerRenderedReactComponent`. Expect: Correct definitions and imports. rg -A 5 'RenderParams' rg -A 5 'streamServerRenderedReactComponent'Length of output: 7052
lib/react_on_rails/helper.rb (5)
115-123: LGTM! Verify the correct usage ofstream_react_componentandFiber.The implementation looks good. Ensure that
stream_react_componentis correctly defined and that theFiberusage is appropriate.Verification successful
LGTM!
The implementation of
stream_react_component_asyncand the usage ofstream_react_componentandFiberare correct and consistent.
stream_react_componentis correctly defined and used withinstream_react_component_async.- The
Fiberusage is appropriate for yielding chunks of data asynchronously.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `stream_react_component` and `Fiber`. # Test: Search for the definition of `stream_react_component`. Expect: Correct definition and usage. rg --type ruby -A 5 $'def stream_react_component' # Test: Check for the usage of `Fiber`. Expect: Appropriate usage. rg --type ruby -A 5 $'Fiber'Length of output: 2275
94-103: LGTM! Verify the correct usage ofinternal_rsc_react_componentandFiber.The implementation looks good. Ensure that
internal_rsc_react_componentis correctly defined and that theFiberusage is appropriate.Verification successful
LGTM! The implementation of
rsc_react_componentis correct.
- The
internal_rsc_react_componentmethod is correctly defined and used.- The
Fiberusage is appropriate and consistent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `internal_rsc_react_component` and `Fiber`. # Test: Search for the definition of `internal_rsc_react_component`. Expect: Correct definition and usage. rg --type ruby -A 5 $'def internal_rsc_react_component' # Test: Check for the usage of `Fiber`. Expect: Appropriate usage. rg --type ruby -A 5 $'Fiber'Length of output: 1924
105-113: LGTM! Verify the correct usage ofinternal_react_componentand options modification.The implementation looks good. Ensure that
internal_react_componentis correctly defined and that the options are appropriately modified for streaming.
396-420: LGTM! Verify the correct transformation logic.The implementation looks good. Ensure that the transformation logic for the rendered HTML stream is correctly implemented.
Verification successful
Verified: The transformation logic for the rendered HTML stream in the method
build_react_component_result_for_server_streamed_contentis correctly implemented.
- The first chunk includes the necessary hydration tags.
- The subsequent chunks are correctly processed.
- The final transformation ensures the HTML is marked as safe.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct transformation logic for the rendered HTML stream. # Test: Search for the transformation logic in `build_react_component_result_for_server_streamed_content`. Expect: Correct implementation. rg --type ruby -A 10 $'def build_react_component_result_for_server_streamed_content'Length of output: 1107
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the method `build_react_component_result_for_server_streamed_content`. # Search for the complete implementation of the method `build_react_component_result_for_server_streamed_content` in the Ruby file. rg --type ruby -A 20 $'def build_react_component_result_for_server_streamed_content'Length of output: 1744
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the method `build_react_component_result_for_server_streamed_content`. # Search for the complete implementation of the method `build_react_component_result_for_server_streamed_content` in the Ruby file. rg --type ruby -A 30 $'def build_react_component_result_for_server_streamed_content'Length of output: 2428
483-487: LGTM! Verify the correct usage ofserver_rendered_react_componentand therscoption.The implementation looks good. Ensure that
server_rendered_react_componentis correctly defined and that therscoption is appropriately handled.Verification successful
LGTM! The
server_rendered_react_componentmethod is correctly defined, and therscoption is appropriately handled.
- The
server_rendered_react_componentmethod includes logic for server-side rendering and error management.- The
rscoption is set and checked within the rendering process, ensuring it is handled correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of `server_rendered_react_component` and the `rsc` option. # Test: Search for the definition of `server_rendered_react_component`. Expect: Correct definition and usage. rg --type ruby -A 5 $'def server_rendered_react_component' # Test: Check for the handling of the `rsc` option. Expect: Appropriate handling. rg --type ruby -A 5 $'options = options.merge(rsc?: true)'Length of output: 542
Script:
#!/bin/bash # Description: Gather full implementation of `server_rendered_react_component` and check for `rsc` option handling. # Get the full implementation of `server_rendered_react_component` rg --type ruby -A 20 $'def server_rendered_react_component' lib/react_on_rails/helper.rb # Search for any usage of `rsc` option in the codebase rg --type ruby 'rsc?: true'Length of output: 1348
Script:
#!/bin/bash # Description: Gather complete implementation of `server_rendered_react_component` and check for `rsc` option handling within `lib/react_on_rails/helper.rb`. # Get the complete implementation of `server_rendered_react_component` rg --type ruby -A 50 $'def server_rendered_react_component' lib/react_on_rails/helper.rb # Search for any references to `rsc` within `lib/react_on_rails/helper.rb` rg --type ruby 'rsc' lib/react_on_rails/helper.rbLength of output: 3052
CHANGELOG.md (7)
9-9: LGTM!The link is properly formatted.
10-10: LGTM!The link is properly formatted.
20-20: LGTM!The new version entry is properly formatted.
21-21: LGTM!The new section header is properly formatted.
22-22: LGTM!The new entry under the "Fixed" section is properly formatted.
1131-1131: LGTM!The link to the "Unreleased" comparison is properly formatted.
1132-1132: LGTM!The link to the
[14.0.1]comparison is properly formatted.
node_package/src/ReactOnRailsRSC.ts
Outdated
| ReactOnRails.serverRenderRSCReactComponent = (options: RenderParams) => { | ||
| const { name, domNodeId, trace, props, railsContext, throwJsErrors } = options; | ||
|
|
||
| let renderResult: null | PassThrough = null; | ||
|
|
||
| try { | ||
| const componentObj = ComponentRegistry.get(name); | ||
| if (componentObj.isRenderer) { | ||
| throw new Error(`\ | ||
| Detected a renderer while server rendering component '${name}'. \ | ||
| See https://github.com/shakacode/react_on_rails#renderer-functions`); | ||
| } | ||
|
|
||
| const reactRenderingResult = createReactOutput({ | ||
| componentObj, | ||
| domNodeId, | ||
| trace, | ||
| props, | ||
| railsContext, | ||
| }); | ||
|
|
||
| if (isServerRenderHash(reactRenderingResult) || isPromise(reactRenderingResult)) { | ||
| throw new Error('Server rendering of streams is not supported for server render hashes or promises.'); | ||
| } | ||
|
|
||
| renderResult = new PassThrough(); | ||
| const streamReader = renderToReadableStream(reactRenderingResult, getBundleConfig()).getReader(); | ||
| const processStream = async () => { | ||
| const { done, value } = await streamReader.read(); | ||
| if (done) { | ||
| renderResult?.push(null); | ||
| return; | ||
| } | ||
|
|
||
| renderResult?.push(value); | ||
| processStream(); | ||
| } | ||
| processStream(); | ||
| } catch (e: unknown) { | ||
| if (throwJsErrors) { | ||
| throw e; | ||
| } | ||
|
|
||
| renderResult = stringToStream(`Error: ${e}`); | ||
| } | ||
|
|
||
| return renderResult; | ||
| }; |
There was a problem hiding this comment.
serverRenderRSCReactComponent function looks good.
The function performs server-side rendering for React components and includes error handling and stream processing.
Reminder: Add unit tests for serverRenderRSCReactComponent.
Ensure that unit tests are added to cover the new server-side rendering functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
440511d to
1c026a7
Compare
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (16)
- .gitignore (1 hunks)
- jest.config.js (1 hunks)
- lib/react_on_rails/helper.rb (5 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1 hunks)
- lib/react_on_rails/utils.rb (3 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/ReactOnRailsRSC.ts (1 hunks)
- node_package/src/serverRenderReactComponent.ts (2 hunks)
- node_package/src/types/index.ts (2 hunks)
- node_package/tests/ReactOnRails.test.js (1 hunks)
- node_package/tests/jest.setup.js (1 hunks)
- package.json (3 hunks)
- spec/dummy/config/webpack/alias.js (1 hunks)
- spec/dummy/config/webpack/commonWebpackConfig.js (1 hunks)
- spec/dummy/config/webpack/webpackConfig.js (1 hunks)
Files skipped from review due to trivial changes (2)
- .gitignore
- spec/dummy/config/webpack/alias.js
Files skipped from review as they are similar to previous changes (7)
- lib/react_on_rails/helper.rb
- lib/react_on_rails/react_component/render_options.rb
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
- node_package/src/ReactOnRailsRSC.ts
- node_package/src/types/index.ts
- package.json
- spec/dummy/config/webpack/webpackConfig.js
Additional comments not posted (14)
jest.config.js (1)
4-4: Ensure the setup script exists and is correctly placed.The
setupFilesoption specifies a setup script located at<rootDir>/node_package/tests/jest.setup.js. Verify that this file exists and is correctly configured.Verification successful
The setup script
node_package/tests/jest.setup.jsexists and is correctly configured.
- The script contains necessary polyfills for
TextEncoderandTextDecoderin the jsdom environment, ensuring Jest tests run smoothly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the setup script exists at the specified location. # Test: Check if the file exists. Expect: The file should exist. if [ ! -f "node_package/tests/jest.setup.js" ]; then echo "Error: The setup script node_package/tests/jest.setup.js does not exist." exit 1 fi echo "The setup script node_package/tests/jest.setup.js exists."Length of output: 174
Script:
#!/bin/bash # Description: Output the contents of the jest.setup.js file to verify its configuration. # Display the contents of the setup script cat node_package/tests/jest.setup.jsLength of output: 672
node_package/tests/jest.setup.js (1)
1-13: Ensure polyfills do not conflict with existing definitions.The polyfills for
TextEncoderandTextDecoderare necessary for environments where they are not defined. Ensure that these polyfills do not conflict with any existing definitions.Verification successful
No conflicting definitions of
TextEncoderandTextDecoderfound.The verification process confirms that there are no existing definitions or imports of
TextEncoderandTextDecoderin the codebase. The polyfills innode_package/tests/jest.setup.jsdo not conflict with any existing definitions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the polyfills for TextEncoder and TextDecoder do not conflict with existing definitions. # Test: Check if TextEncoder and TextDecoder are already defined in the global scope. Expect: They should not be defined. node -e "if (typeof global.TextEncoder !== 'undefined' || typeof global.TextDecoder !== 'undefined') { console.error('Error: TextEncoder or TextDecoder is already defined.'); process.exit(1); } else { console.log('TextEncoder and TextDecoder are not defined.'); }"Length of output: 362
Script:
#!/bin/bash # Search for any definitions or imports of TextEncoder and TextDecoder in the codebase ast-grep --lang javascript --pattern 'const TextEncoder = $_;' ast-grep --lang javascript --pattern 'const TextDecoder = $_;' ast-grep --lang javascript --pattern 'import { TextEncoder } from $_;' ast-grep --lang javascript --pattern 'import { TextDecoder } from $_;'Length of output: 1183
Script:
#!/bin/bash # Corrected ast-grep patterns for searching definitions or imports of TextEncoder and TextDecoder in the codebase ast-grep --lang javascript --pattern 'const TextEncoder = $_;' ast-grep --lang javascript --pattern 'const TextDecoder = $_;' ast-grep --lang javascript --pattern 'import { TextEncoder } from "$_";' ast-grep --lang javascript --pattern 'import { TextDecoder } from "$_";' # Fallback to rg for searching definitions or imports of TextEncoder and TextDecoder in the codebase rg 'const TextEncoder' --context 5 rg 'const TextDecoder' --context 5 rg 'import { TextEncoder' --context 5 rg 'import { TextDecoder' --context 5Length of output: 657
Script:
#!/bin/bash # Corrected rg patterns for searching definitions or imports of TextEncoder and TextDecoder in the codebase rg 'const TextEncoder' --context 5 rg 'const TextDecoder' --context 5 rg 'import \{ TextEncoder' --context 5 rg 'import \{ TextDecoder' --context 5Length of output: 155
spec/dummy/config/webpack/commonWebpackConfig.js (1)
44-44: Ensure compatibility with the newprocessconfiguration.The addition of
process: 'process/browser'in theProvidePluginconfiguration is intended to improve compatibility with certain libraries. Verify that this change does not introduce any conflicts or issues.Verification successful
Verification successful: The new
processconfiguration does not introduce any conflicts or issues.The addition of
process: 'process/browser'in theProvidePluginconfiguration is intended to provide a polyfill for theprocessobject in the browser environment. The search results indicate that theprocessobject is used primarily for setting and checking environment variables, which should not be affected by this change.
spec/dummy/babel.config.jsspec/dummy/config/webpack/webpackConfig.jsspec/dummy/config/webpack/test.jsspec/dummy/config/webpack/production.jsspec/dummy/config/webpack/development.jsspec/dummy/config/webpack/commonWebpackConfig.jsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new process configuration does not introduce any conflicts or issues. # Test: Check if the process object is available globally in the browser environment. Expect: The process object should be defined. node -e "const webpack = require('webpack'); const config = require('./spec/dummy/config/webpack/commonWebpackConfig.js')(); webpack(config, (err, stats) => { if (err || stats.hasErrors()) { console.error('Error: Webpack compilation failed.'); process.exit(1); } else { console.log('Webpack compilation succeeded.'); } });"Length of output: 421
Script:
#!/bin/bash # Description: Search for the usage of the `process` object in the codebase to identify potential issues or conflicts. # Search for the usage of `process` in JavaScript files rg 'process' --glob '*.js' -A 3Length of output: 2474
node_package/tests/ReactOnRails.test.js (1)
22-30: LGTM! Improved test clarity by directly manipulating the DOM.The changes enhance the clarity of the test by directly manipulating the DOM and checking the visible output rather than relying on internal component details.
node_package/src/serverRenderReactComponent.ts (4)
2-2: LGTM! New import forPassThroughstream.The
PassThroughstream from thestreammodule is correctly imported for use in the new rendering approach.
170-175: LGTM! New functionstringToStreamcorrectly implemented.The function converts a string to a stream using
PassThrough, which is useful for converting error messages to streams.
177-219: LGTM! Enhanced server-side rendering withrenderToPipeableStream.The new function
streamServerRenderedReactComponentenhances server-side rendering by allowing for more efficient streaming of rendered output. Error handling and logging are appropriately implemented.
219-220: LGTM! Correct return statement forrenderResult.The return statement correctly returns the
renderResultstream.lib/react_on_rails/utils.rb (3)
Line range hint
69-97:
LGTM! Enhanced flexibility withbundle_js_file_path.The new method
bundle_js_file_pathenhances the flexibility of the bundle path retrieval process by allowing it to handle different bundle names dynamically.
98-112: LGTM! Streamlined logic withbundle_js_file_path.The method
server_bundle_js_file_pathhas been updated to use the newbundle_js_file_pathmethod, streamlining the logic for retrieving the server bundle path.
114-119: LGTM! New methodrsc_bundle_js_file_pathenhances configurability.The new method
rsc_bundle_js_file_pathenhances configurability by allowing the retrieval of a resource bundle path, with a default bundle name for the development environment.node_package/src/ReactOnRails.ts (3)
262-264: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
handleErrorare correctly implemented.Verification successful
All usages of
handleErrorare correctly implemented.The function
handleErroris consistently used across the codebase, and no issues were found.
serverRenderReactComponent.ts: Multiple valid usages.ReactOnRails.ts: Valid usage.handleError.ts: Proper definition and export.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handleError` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type python -A 5 $'handleError'Length of output: 66
Script:
#!/bin/bash # Description: Verify all function calls to `handleError` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type ts -A 5 $'handleError'Length of output: 3682
258-260: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
serverRenderRSCReactComponentare correctly implemented.Verification successful
Function usage verified.
The function
serverRenderRSCReactComponentis correctly implemented and used only in the appropriate context.
- Definition and error throwing:
node_package/src/ReactOnRails.ts- Implementation for RSC bundle:
node_package/src/ReactOnRailsRSC.ts- Type declaration:
node_package/src/types/index.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `serverRenderRSCReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type python -A 5 $'serverRenderRSCReactComponent'Length of output: 84
Script:
#!/bin/bash # Description: Verify all function calls to `serverRenderRSCReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type ts -A 5 $'serverRenderRSCReactComponent'Length of output: 1445
249-251: LGTM! Verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
streamServerRenderedReactComponentare correctly implemented.Verification successful
Verification Successful: Function usage is correct.
The
streamServerRenderedReactComponentfunction is correctly implemented and used within the codebase.
- Definition:
node_package/src/serverRenderReactComponent.ts- Usage:
node_package/src/ReactOnRails.ts- Type Declaration:
node_package/src/types/index.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `streamServerRenderedReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type python -A 5 $'streamServerRenderedReactComponent'Length of output: 89
Script:
#!/bin/bash # Description: Verify all function calls to `streamServerRenderedReactComponent` are correctly implemented. # Test: Search for the function usage. Expect: Only valid occurrences. rg --type ts -A 5 $'streamServerRenderedReactComponent'Length of output: 2038
Comments failed to post (1)
node_package/src/serverRenderReactComponent.ts
205-207: Reminder: Add console replay script to the stream.
The TODO comment indicates that the console replay script needs to be added to the stream to avoid console messages leaking between different components rendering.
Do you want me to generate the code for adding the console replay script to the stream or open a GitHub issue to track this task?
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
1c026a7 to
40c89e4
Compare
| rescue Object.const_get( | ||
| ReactOnRails::PackerUtils.packer_type.capitalize | ||
| )::Manifest::MissingEntryError |
There was a problem hiding this comment.
This looks like it could easily raise an error itself. Any way around it?
There was a problem hiding this comment.
I added enough specs in utils_spec.rb to ensure that it doesn't cause errors if the packer is webpacker or shaka packer'. For now, React on Rails only supports these packers. If we plan to add support for more packers, we can refactor the PackerUtils` module in a follow-up PR.
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
f38ed50 to
0c2313e
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
4 times, most recently
from
d5d5d68 to
8937c5f
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
40c89e4 to
d437954
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
lib/react_on_rails/react_component/render_options.rb (2)
118-120: Consider adding configuration fallback for consistency.For consistency with other options like
stream?, consider adding a configuration fallback using theretrieve_configuration_value_forpattern. This would allow setting a default RSC behavior in the global configuration.Here's a suggested implementation:
def rsc? - options[:rsc?] + retrieve_configuration_value_for(:rsc?) end
118-120: Add documentation for the RSC feature.Since this is part of adding React Server Components support, it would be helpful to add documentation explaining:
- The purpose of the RSC option
- How it interacts with other rendering options (especially
stream?)- Any requirements or constraints when using RSC
Add RDoc documentation above the method:
+# Returns whether React Server Components (RSC) rendering is enabled. +# @return [Boolean] true if RSC rendering is enabled, false otherwise def rsc? options[:rsc?] endnode_package/src/types/index.ts (1)
174-174: Add JSDoc documentation and consider RSC-specific typesWhile the method signature is correct, consider these improvements:
Add JSDoc documentation explaining:
- The RSC-specific behavior and constraints
- How it differs from
streamServerRenderedReactComponent- Error handling approach
Consider creating an RSC-specific params type:
interface RSCRenderParams extends RenderParams { // Add any RSC-specific options rscMode?: boolean; // ... other RSC-specific fields }lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (3)
Line range hint
59-63: Document RSC implementation detailsWhile the code structure for RSC support is clean, please add documentation explaining:
- The RSC implementation approach
- How
eval_streaming_jshandles RSC differently from regular streaming- Any specific requirements or limitations of RSC support
79-82: Improve readability of conditional returnThe current line is too long and complex. Consider refactoring for better readability:
- return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream? || render_options.rsc? + if render_options.stream? || render_options.rsc? + # Streamed component is returned as stream of strings. + # We need to parse each chunk and replay the console messages. + result.transform { |chunk| parse_result_and_replay_console_messages(chunk, render_options) } + else + parse_result_and_replay_console_messages(result, render_options) + end🧰 Tools
🪛 rubocop
[convention] 79-79: Modifier form of
unlessmakes the line too long.(Style/IfUnlessModifier)
[convention] 79-79: Line is too long. [134/120]
(Layout/LineLength)
Line range hint
59-82: Consider RSC architectural implicationsThe RSC implementation touches core rendering functionality. Please consider:
- Performance implications of RSC vs traditional SSR
- Memory usage with streaming and RSC
- Error boundaries and fallback strategies
- Client/server boundary management
- Caching strategies for RSC components
Would you like assistance in implementing any of these architectural considerations?
🧰 Tools
🪛 rubocop
[convention] 79-79: Modifier form of
unlessmakes the line too long.(Style/IfUnlessModifier)
[convention] 79-79: Line is too long. [134/120]
(Layout/LineLength)
node_package/src/ReactOnRailsRSC.ts (2)
1-1: Remove unused import ofReactElement.The
ReactElementtype imported from'react'is not used in this file. Unused imports can clutter the code and should be removed to improve readability.Apply this diff to remove the unused import:
-import type { ReactElement } from 'react';🧰 Tools
🪛 eslint
[error] 1-1: 'ReactElement' is defined but never used.
(@typescript-eslint/no-unused-vars)
82-88: Handle error types safely when constructing error messages.In the
catchblock,eis of typeunknown. Interpolating it directly into a string may result in"[object Object]"or other unintended outputs.Apply this diff to safely convert
eto a string:renderResult = stringToStream(`Error: ${e}`); +// Ensure the error is converted to a meaningful string +const errorMessage = e instanceof Error ? e.message : String(e); +renderResult = stringToStream(`Error: ${errorMessage}`);This ensures that the error message is meaningful and avoids potential issues when
eis not a string.lib/react_on_rails/utils.rb (1)
114-120: Makebundle_nameconfigurable inrsc_bundle_js_file_pathThe
rsc_bundle_js_file_pathmethod currently uses a hardcodedbundle_name("rsc-bundle.js"). Making this value configurable would enhance flexibility, allowing users to specify custom bundle names for React Server Components.Would you like assistance in implementing a configuration option for the
rsc_bundle_js_file_pathmethod?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
lib/react_on_rails/helper.rb(4 hunks)lib/react_on_rails/react_component/render_options.rb(1 hunks)lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb(3 hunks)lib/react_on_rails/utils.rb(2 hunks)node_package/src/ReactOnRails.ts(2 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)node_package/src/types/index.ts(2 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 rubocop
lib/react_on_rails/helper.rb
[convention] 153-159: Redundant assignment before returning detected.
(Style/RedundantAssignment)
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
[convention] 79-79: Modifier form of unless makes the line too long.
(Style/IfUnlessModifier)
[convention] 79-79: Line is too long. [134/120]
(Layout/LineLength)
[warning] 233-233: Remove debugger entry point binding.pry.
(Lint/Debugger)
🪛 eslint
node_package/src/ReactOnRailsRSC.ts
[error] 1-1: 'ReactElement' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (8)
package.json (2)
49-49:
Review the use of canary versions in dependencies
The addition of react-server-dom-webpack and use of canary versions for React packages raises several concerns:
- Using canary versions in production could lead to instability
- All React-related packages should be pinned to the same version
- The peerDependencies need updating to reflect RSC requirements
Let's verify the React version requirements for RSC support:
Suggested updates:
- Document why canary versions are needed
- Update peerDependencies to reflect minimum React version needed for RSC
- Add a warning in the README about the experimental nature of RSC support
Would you like me to help draft the documentation updates?
5-10: Verify the exports configuration and file paths
The exports configuration needs attention in several areas:
- The nested structure under "." might cause compatibility issues with some bundlers
- The export paths need verification
Let's verify the export paths and suggest improvements:
Consider restructuring the exports to be more explicit:
"exports": {
- ".": {
- "rsc-server": "./node_package/lib/ReactOnRailsRSC.js",
- "default": "./node_package/lib/ReactOnRails.js"
- }
+ "./rsc-server": "./node_package/lib/ReactOnRailsRSC.js",
+ ".": "./node_package/lib/ReactOnRails.js"
}lib/react_on_rails/react_component/render_options.rb (1)
118-120: LGTM! The implementation follows Ruby conventions.
The rsc? method follows the established pattern for boolean accessors and is appropriately placed near other rendering-related options.
node_package/src/types/index.ts (1)
2-2: LGTM: Stream type import for RSC support
The addition of PassThrough type import is appropriate for implementing RSC streaming support.
node_package/src/ReactOnRails.ts (2)
2-2: LGTM: Stream type import for RSC support
The addition of PassThrough type import is appropriate for RSC streaming support.
259-266: 🛠️ Refactor suggestion
Enhance RSC method implementation and documentation
The new RSC method implementation needs improvements in several areas:
- Documentation should explain:
- The purpose of RSC rendering
- When to use this vs regular server rendering
- What an RSC bundle is and how to configure it
- The error message could be more informative about how to resolve the issue
- The eslint-disable comment should be removed and the options parameter should be properly typed or used
Consider this improved implementation:
/**
* Used by server rendering by Rails
+ * Renders a React Server Component (RSC) on the server side.
+ * This method is only available in the RSC-specific bundle and provides streaming support
+ * for React Server Components.
+ *
+ * @see https://react.dev/blog/2020/12/21/data-fetching-with-react-server-components
* @param options Configuration for server-side RSC rendering
+ * @throws {Error} When called from the main bundle instead of the RSC bundle
*/
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
serverRenderRSCReactComponent(options: RenderParams): PassThrough {
- throw new Error('serverRenderRSCReactComponent is supported in RSC bundle only.');
+ throw new Error(
+ 'serverRenderRSCReactComponent must be called from the RSC bundle. ' +
+ 'Please ensure you have configured your bundler to create a separate RSC bundle ' +
+ 'and that you are importing from the correct bundle.'
+ );
}Let's verify if there are any RSC-related configurations or documentation:
lib/react_on_rails/utils.rb (1)
Line range hint 58-96: Refactored bundle_js_file_path enhances maintainability
The introduction of the bundle_js_file_path method centralizes the logic for determining the bundle file paths. This refactoring reduces code duplication and improves the maintainability of the codebase. The method handles different scenarios effectively.
lib/react_on_rails/helper.rb (1)
532-537: New method internal_rsc_react_component implemented correctly
The internal_rsc_react_component method properly configures the render options for React Server Components and invokes the server render function.
| @@ -171,6 +171,7 @@ export interface ReactOnRails { | |||
| getComponent(name: string): RegisteredComponent; | |||
| serverRenderReactComponent(options: RenderParams): null | string | Promise<RenderResult>; | |||
| streamServerRenderedReactComponent(options: RenderParams): Readable; | |||
| serverRenderRSCReactComponent(options: RenderParams): PassThrough; | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for RSC implementation
The implementation requires test coverage for:
serverRenderRSCReactComponentmethodrsc_react_componentRuby helper- Error handling scenarios
Key files needing tests:
node_package/src/ReactOnRailsRSC.tslib/react_on_rails/helper.rb
🔗 Analysis chain
Verify RSC implementation completeness
Since this is a WIP PR implementing RSC support, let's verify the complete implementation:
RSC implementation needs additional test coverage
The implementation appears incomplete as it requires:
- Test coverage for the RSC-specific functionality
- Tests for the Ruby helper method
rsc_react_component - Tests for error handling in
serverRenderRSCReactComponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RSC implementation across the codebase
# Check for RSC implementation in ReactOnRails.ts
echo "Checking ReactOnRails implementation..."
rg "serverRenderRSCReactComponent" "node_package/src/ReactOnRails.ts" -A 5
# Check for corresponding Ruby helper methods
echo "Checking Ruby helpers..."
rg "rsc_react_component" "lib/react_on_rails/helper.rb" -A 5
# Check for test coverage
echo "Checking test coverage..."
fd -e spec.rb -e spec.ts -e test.ts | xargs rg "RSC|React Server Components"
Length of output: 2688
Script:
#!/bin/bash
# Check for RSC-specific test files
echo "Checking for RSC test files..."
fd -e spec.rb -e spec.ts -e test.ts | xargs rg "describe.*RSC|test.*RSC"
# Check for test coverage of rsc_react_component helper
echo "Checking Ruby helper tests..."
rg "rsc_react_component" "spec/" -A 5
# Check for error handling tests
echo "Checking error handling tests..."
rg "serverRenderRSCReactComponent.*Error" "spec/" -A 5
Length of output: 380
| @@ -230,7 +230,9 @@ def parse_result_and_replay_console_messages(result_string, render_options) | |||
| result = nil | |||
| begin | |||
| result = JSON.parse(result_string) | |||
| binding.pry | |||
There was a problem hiding this comment.
Remove debugging entry point
The binding.pry statement should be removed before merging.
- binding.pry📝 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.
| binding.pry |
🧰 Tools
🪛 rubocop
[warning] 233-233: Remove debugger entry point binding.pry.
(Lint/Debugger)
| if (done) { | ||
| renderResult?.push(null); | ||
| // @ts-expect-error value is not typed | ||
| debugConsole.log('value', finalValue); |
There was a problem hiding this comment.
debugConsole is not defined; use console or import the appropriate logger.
The variable debugConsole is used but not defined or imported, which will lead to a ReferenceError at runtime.
Apply this diff to correct the issue:
-// @ts-expect-error value is not typed
-debugConsole.log('value', finalValue);
+// @ts-expect-error value is not typed
+console.log('value', finalValue);Alternatively, if debugConsole is intended to be a custom logging utility, ensure it is properly imported or defined.
📝 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.
| debugConsole.log('value', finalValue); | |
| // @ts-expect-error value is not typed | |
| console.log('value', finalValue); |
node_package/src/ReactOnRailsRSC.ts
Outdated
| const getBundleConfig = () => { | ||
| const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8')); | ||
| // remove file:// from keys | ||
| const newBundleConfig: { [key: string]: any } = {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a more precise type instead of any for newBundleConfig.
Using any defeats the purpose of TypeScript's type safety. Specifying a more precise type enhances code reliability and maintainability.
Apply this diff to specify a more accurate type:
-const newBundleConfig: { [key: string]: any } = {};
+const newBundleConfig: { [key: string]: typeof bundleConfig[keyof typeof bundleConfig] } = {};This ensures that newBundleConfig has the same value types as the original bundleConfig. Adjust the type as needed based on the actual structure of the config.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
node_package/src/ReactOnRailsRSC.ts
Outdated
| (async () => { | ||
| try { | ||
| // @ts-expect-error AsyncLocalStorage is not in the node types | ||
| globalThis.AsyncLocalStorage = (await import('node:async_hooks')).AsyncLocalStorage; | ||
| } catch (e) { | ||
| console.log('AsyncLocalStorage not found'); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assigning AsyncLocalStorage to globalThis; consider alternative approaches.
Assigning to globalThis.AsyncLocalStorage can pollute the global namespace and may lead to conflicts or unintended side effects. Additionally, using an asynchronous IIFE ((async () => { ... })()) for this purpose might introduce race conditions since other parts of the code might access AsyncLocalStorage before it's available.
Consider importing AsyncLocalStorage synchronously where needed without modifying globalThis. For example:
- (async () => {
- try {
- // @ts-expect-error AsyncLocalStorage is not in the node types
- globalThis.AsyncLocalStorage = (await import('node:async_hooks')).AsyncLocalStorage;
- } catch (e) {
- console.log('AsyncLocalStorage not found');
- }
- })();
+ try {
+ // @ts-expect-error AsyncLocalStorage is not in the node types
+ const { AsyncLocalStorage } = require('node:async_hooks');
+ } catch (e) {
+ console.log('AsyncLocalStorage not found');
+ }This approach avoids modifying the global scope and ensures AsyncLocalStorage is available synchronously.
Committable suggestion skipped: line range outside the PR's diff.
lib/react_on_rails/helper.rb
Outdated
| def rsc_react_component(component_name, options = {}) | ||
| rendering_fiber = Fiber.new do | ||
| res = internal_rsc_react_component(component_name, options) | ||
| res.each_chunk do |chunk| | ||
| Fiber.yield chunk | ||
| end | ||
| Fiber.yield nil | ||
| end | ||
| rendering_fiber | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate redundant assignment before returning
The assignment to rendering_fiber is redundant since it's immediately returned. You can return the Fiber directly without assigning it to a variable.
Apply this diff to simplify the method:
def rsc_react_component(component_name, options = {})
- rendering_fiber = Fiber.new do
+ Fiber.new do
res = internal_rsc_react_component(component_name, options)
res.each_chunk do |chunk|
Fiber.yield chunk
end
Fiber.yield nil
end
- rendering_fiber
end📝 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.
| def rsc_react_component(component_name, options = {}) | |
| rendering_fiber = Fiber.new do | |
| res = internal_rsc_react_component(component_name, options) | |
| res.each_chunk do |chunk| | |
| Fiber.yield chunk | |
| end | |
| Fiber.yield nil | |
| end | |
| rendering_fiber | |
| end | |
| def rsc_react_component(component_name, options = {}) | |
| Fiber.new do | |
| res = internal_rsc_react_component(component_name, options) | |
| res.each_chunk do |chunk| | |
| Fiber.yield chunk | |
| end | |
| Fiber.yield nil | |
| end | |
| end |
🧰 Tools
🪛 rubocop
[convention] 153-159: Redundant assignment before returning detected.
(Style/RedundantAssignment)
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
d437954 to
d975912
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
node_package/src/ReactOnRailsRSC.ts (2)
1-1: Remove unused importReactElement.The
ReactElementtype is imported but never used in the code.-import type { ReactElement } from 'react';🧰 Tools
🪛 eslint
[error] 1-1: 'ReactElement' is defined but never used.
(@typescript-eslint/no-unused-vars)
39-91: Add comprehensive tests for RSC implementation.The RSC implementation needs thorough testing to ensure reliability. Consider adding tests for:
- Success cases with different component types
- Error cases (invalid components, rendering failures)
- Stream processing edge cases
- Bundle config loading scenarios
Would you like me to help create a test suite for the RSC implementation?
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (2)
Line range hint
59-65: Consider RSC-specific evaluation pathWhile combining RSC with streaming makes sense, consider if RSC needs additional specific handling:
- RSC might require different error boundaries
- The evaluation context might need RSC-specific setup
- Performance monitoring might need to distinguish between RSC and regular streaming
Consider splitting the condition and adding RSC-specific handling:
- result = if render_options.stream? || render_options.rsc? + result = if render_options.rsc? + js_evaluator.eval_rsc_js(js_code, render_options) + elsif render_options.stream? js_evaluator.eval_streaming_js(js_code, render_options) else js_evaluator.eval_js(js_code, render_options) end
Line range hint
79-83: Improve stream chunk processing and fix line lengthThe streaming logic looks good, but consider these improvements:
- The line exceeds length limit (134/120)
- Chunk processing could benefit from explicit error handling
Consider this refactoring:
- return parse_result_and_replay_console_messages(result, render_options) unless render_options.stream? || render_options.rsc? + return parse_result_and_replay_console_messages(result, render_options) unless + render_options.stream? || render_options.rsc? - result.transform { |chunk| parse_result_and_replay_console_messages(chunk, render_options) } + result.transform do |chunk| + begin + parse_result_and_replay_console_messages(chunk, render_options) + rescue StandardError => e + Rails.logger.error("[react_on_rails] Error processing stream chunk: #{e.message}") + raise ReactOnRails::Error, "Stream chunk processing failed: #{e.message}" + end + end🧰 Tools
🪛 rubocop
[convention] 79-79: Modifier form of
unlessmakes the line too long.(Style/IfUnlessModifier)
[convention] 79-79: Line is too long. [134/120]
(Layout/LineLength)
lib/react_on_rails/helper.rb (1)
152-161: Add documentation for the RSC streaming methodThe method lacks documentation explaining its purpose, parameters, and usage. Consider adding RDoc documentation similar to
stream_react_component.Additionally, the code can be simplified by removing the redundant assignment:
def rsc_react_component(component_name, options = {}) - rendering_fiber = Fiber.new do + Fiber.new do res = internal_rsc_react_component(component_name, options) res.each_chunk do |chunk| Fiber.yield chunk end Fiber.yield nil end - rendering_fiber end🧰 Tools
🪛 rubocop
[convention] 153-159: Redundant assignment before returning detected.
(Style/RedundantAssignment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
lib/react_on_rails/helper.rb(4 hunks)lib/react_on_rails/react_component/render_options.rb(1 hunks)lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb(3 hunks)lib/react_on_rails/utils.rb(2 hunks)node_package/src/ReactOnRails.ts(2 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)node_package/src/types/index.ts(2 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/react_on_rails/react_component/render_options.rb
- node_package/src/ReactOnRails.ts
- node_package/src/types/index.ts
- package.json
🧰 Additional context used
🪛 rubocop
lib/react_on_rails/helper.rb
[convention] 153-159: Redundant assignment before returning detected.
(Style/RedundantAssignment)
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
[convention] 79-79: Modifier form of unless makes the line too long.
(Style/IfUnlessModifier)
[convention] 79-79: Line is too long. [134/120]
(Layout/LineLength)
🪛 eslint
node_package/src/ReactOnRailsRSC.ts
[error] 1-1: 'ReactElement' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (4)
lib/react_on_rails/utils.rb (2)
Line range hint 69-96: LGTM! Well-structured bundle path resolution
The method effectively handles different bundle scenarios with clear documentation and appropriate fallback mechanisms.
98-112: LGTM! Good refactoring of bundle path resolution
The changes effectively reuse the common bundle path resolution logic while maintaining proper caching behavior.
lib/react_on_rails/helper.rb (2)
532-537: LGTM!
The implementation correctly sets up RSC-specific rendering options and follows the established pattern of other internal render methods.
389-396: Clean up commented code
The commented code appears to be obsolete after switching to the simplified bundle loading approach. If this code is no longer needed, it should be removed rather than left as comments. If there's a reason to keep it (e.g., for potential rollback), add a comment explaining why.
Let's verify if this code is referenced elsewhere:
| const processStream = async () => { | ||
| const { done, value } = await streamReader.read(); | ||
| if (done) { | ||
| renderResult?.push(null); | ||
| // @ts-expect-error value is not typed | ||
| debugConsole.log('value', finalValue); | ||
| return; | ||
| } | ||
|
|
||
| finalValue += decoder.decode(value); | ||
| renderResult?.push(value); | ||
| processStream(); | ||
| } | ||
| processStream(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace recursion with iteration in processStream.
The current recursive implementation could lead to stack overflow for large streams. Consider using a loop instead.
- const processStream = async () => {
- const { done, value } = await streamReader.read();
- if (done) {
- renderResult?.push(null);
- // @ts-expect-error value is not typed
- debugConsole.log('value', finalValue);
- return;
- }
-
- finalValue += decoder.decode(value);
- renderResult?.push(value);
- processStream();
- }
- processStream();
+ const processStream = async () => {
+ while (true) {
+ const { done, value } = await streamReader.read();
+ if (done) {
+ renderResult?.push(null);
+ // @ts-expect-error value is not typed
+ debugConsole.log('value', finalValue);
+ break;
+ }
+ finalValue += decoder.decode(value);
+ renderResult?.push(value);
+ }
+ }
+ await processStream();📝 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 processStream = async () => { | |
| const { done, value } = await streamReader.read(); | |
| if (done) { | |
| renderResult?.push(null); | |
| // @ts-expect-error value is not typed | |
| debugConsole.log('value', finalValue); | |
| return; | |
| } | |
| finalValue += decoder.decode(value); | |
| renderResult?.push(value); | |
| processStream(); | |
| } | |
| processStream(); | |
| const processStream = async () => { | |
| while (true) { | |
| const { done, value } = await streamReader.read(); | |
| if (done) { | |
| renderResult?.push(null); | |
| // @ts-expect-error value is not typed | |
| debugConsole.log('value', finalValue); | |
| break; | |
| } | |
| finalValue += decoder.decode(value); | |
| renderResult?.push(value); | |
| } | |
| } | |
| await processStream(); |
node_package/src/ReactOnRailsRSC.ts
Outdated
| const getBundleConfig = () => { | ||
| const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8')); | ||
| // remove file:// from keys | ||
| const newBundleConfig: { [key: string]: any } = {}; | ||
| for (const [key, value] of Object.entries(bundleConfig)) { | ||
| newBundleConfig[key.replace('file://', '')] = value; | ||
| } | ||
| return newBundleConfig; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of getBundleConfig function.
The function has several areas for improvement:
- The hardcoded path assumes a specific environment and directory structure
- Missing error handling for file read operations
- The
anytype should be more specific as flagged by ESLint
Consider applying these improvements:
-const getBundleConfig = () => {
+const getBundleConfig = (manifestPath = './public/webpack/development/react-client-manifest.json') => {
try {
- const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8'));
+ const bundleConfig = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
// remove file:// from keys
- const newBundleConfig: { [key: string]: any } = {};
+ const newBundleConfig: { [key: string]: unknown } = {};
for (const [key, value] of Object.entries(bundleConfig)) {
newBundleConfig[key.replace('file://', '')] = value;
}
return newBundleConfig;
+ } catch (error) {
+ throw new Error(`Failed to read or parse manifest file: ${error}`);
+ }
}📝 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 getBundleConfig = () => { | |
| const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8')); | |
| // remove file:// from keys | |
| const newBundleConfig: { [key: string]: any } = {}; | |
| for (const [key, value] of Object.entries(bundleConfig)) { | |
| newBundleConfig[key.replace('file://', '')] = value; | |
| } | |
| return newBundleConfig; | |
| } | |
| const getBundleConfig = (manifestPath = './public/webpack/development/react-client-manifest.json') => { | |
| try { | |
| const bundleConfig = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); | |
| // remove file:// from keys | |
| const newBundleConfig: { [key: string]: unknown } = {}; | |
| for (const [key, value] of Object.entries(bundleConfig)) { | |
| newBundleConfig[key.replace('file://', '')] = value; | |
| } | |
| return newBundleConfig; | |
| } catch (error) { | |
| throw new Error(`Failed to read or parse manifest file: ${error}`); | |
| } | |
| } |
🧰 Tools
🪛 eslint
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| rescue Object.const_get( | ||
| ReactOnRails::PackerUtils.packer_type.capitalize | ||
| )::Manifest::MissingEntryError |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling specificity
The current error handling using Object.const_get is overly broad and could mask other legitimate errors. Consider being more specific about the error type.
- rescue Object.const_get(
- ReactOnRails::PackerUtils.packer_type.capitalize
- )::Manifest::MissingEntryError
+ rescue => e
+ packer_error = "#{ReactOnRails::PackerUtils.packer_type.capitalize}::Manifest::MissingEntryError"
+ raise e unless e.class.name == packer_errorCommittable suggestion skipped: line range outside the PR's diff.
lib/react_on_rails/utils.rb
Outdated
| def self.rsc_bundle_js_file_path | ||
| return @rsc_bundle_path if @rsc_bundle_path && !Rails.env.development? | ||
|
|
||
| # TODO: make it configurable | ||
| bundle_name = "rsc-bundle.js" | ||
| @rsc_bundle_path = bundle_js_file_path(bundle_name) | ||
| end |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
RSC bundle configuration should follow existing pattern for server bundle
The review suggestion is valid. Based on the configuration class implementation:
- The RSC bundle should follow the same configuration pattern as
server_bundle_js_file, which is already a configurable attribute. - The configuration class needs to be updated to include
rsc_bundle_js_fileas a new configuration option. - Documentation is indeed sparse, with only a brief mention of React Server Components in the README.
Specific changes needed:
- Add
rsc_bundle_js_fileto the Configuration class attributes - Add it to the default configuration in
self.configuration - Update the
ensure_webpack_generated_files_existsmethod to include the RSC bundle file - Add proper documentation about RSC bundle configuration and usage
🔗 Analysis chain
Add configuration support and documentation for RSC bundle
The RSC bundle implementation needs attention in two areas:
- The bundle name is hardcoded. As noted in the TODO, this should be configurable.
- Documentation about the RSC bundle's purpose and usage is missing.
Consider adding configuration support like this:
def self.rsc_bundle_js_file_path
return @rsc_bundle_path if @rsc_bundle_path && !Rails.env.development?
- # TODO: make it configurable
- bundle_name = "rsc-bundle.js"
+ bundle_name = ReactOnRails.configuration.rsc_bundle_js_file || "rsc-bundle.js"
@rsc_bundle_path = bundle_js_file_path(bundle_name)
endAlso, please add documentation explaining:
- The purpose of the RSC bundle
- When and how it's used
- Configuration options
Let me gather more information about the configuration setup to provide a comprehensive verification.
Let me check the configuration class implementation to see the existing configuration pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RSC configuration is documented in README or configuration files
rg -g '*.md' -g '*.rb' -i 'rsc[_-]bundle|server[_-]components'
Length of output: 754
Script:
#!/bin/bash
# Check configuration class and related files
ast-grep --pattern 'class ReactOnRails::Configuration'
# Check for any existing bundle configuration patterns
rg -A 3 'bundle.*file.*=' lib/
Length of output: 1771
Script:
#!/bin/bash
# Check the configuration class implementation
cat lib/react_on_rails/configuration.rb
Length of output: 12660
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
lib/react_on_rails/helper.rb (1)
378-402: Add documentation for the fiber management methodThe
run_stream_inside_fibermethod handles critical fiber management logic for both streaming and RSC. Consider adding documentation to explain:
- The fiber lifecycle management
- The significance of
@rorp_rendering_fibers- The streaming flow and chunk handling
+ # Manages the lifecycle of a rendering fiber for streaming React components. + # @param [Block] block The rendering logic that generates the stream + # @return [String] The initial HTML chunk + # @raise [ReactOnRails::Error] If Pro is not enabled or if not in streaming context def run_stream_inside_fiber
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/react_on_rails/helper.rb(4 hunks)
🔇 Additional comments (1)
lib/react_on_rails/helper.rb (1)
364-371: Verify the impact of removing component-specific pack loading
The change from component-specific packs to always loading "client-bundle" could affect:
- Initial page load performance
- Bundle size optimization
- Development mode debugging
✅ Verification successful
Based on the examination of the codebase, particularly the webpack configuration and packs generator implementation, I can now provide a final response:
Component-specific pack loading removal is part of automated bundle generation
The change from component-specific packs to a single client bundle is intentional and safe because:
- The codebase has implemented an automated pack generation system (PacksGenerator) that handles component bundling efficiently
- Components are automatically registered and included in the client bundle through the
ReactOnRails.register()mechanism - The webpack configuration (
clientWebpackConfig.js) is properly set up to handle the consolidated bundle approach
The performance impact is mitigated because:
- The packs generator automatically handles component registration and bundling
- The system supports both common and client-specific components with proper separation
- Bundle optimization is handled at the webpack configuration level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the size difference between component-specific and client bundles
echo "Searching for client bundle references..."
rg -l "client-bundle" --type js
echo "Checking for existing component-specific bundles..."
fd -e js . -x echo "Size of {}: $(wc -c < {})" \; | grep "generated/"
Length of output: 507
Script:
#!/bin/bash
# Let's check the webpack configuration and related files for bundle setup
echo "Searching for webpack configuration files..."
fd -e js webpack
echo "Checking for bundle configuration in package.json..."
fd package.json -x cat {}
echo "Looking for client bundle related code..."
rg -A 5 "client-bundle" --type js --type ruby
echo "Searching for component generation or bundling logic..."
rg -l "generated.*component" --type js --type ruby
Length of output: 12484
Script:
#!/bin/bash
# Let's examine the webpack configuration and packs generator
echo "Checking webpack config for bundle setup..."
cat spec/dummy/config/webpack/clientWebpackConfig.js
echo "\nExamining packs generator implementation..."
cat lib/react_on_rails/packs_generator.rb
echo "\nLooking for component-specific pack references..."
rg "generated/.*pack" --type ruby -A 3
Length of output: 9976
| def internal_rsc_react_component(react_component_name, options = {}) | ||
| options = options.merge(rsc?: true) | ||
| render_options = create_render_options(react_component_name, options) | ||
| json_stream = server_rendered_react_component(render_options) | ||
| json_stream.transform do |chunk| | ||
| chunk[:html].html_safe | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety for RSC
The RSC implementation should include:
- Type validation for the JSON stream
- Error handling for transformation failures
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
+ raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
json_stream.transform do |chunk|
+ raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
chunk[:html].html_safe
end
+rescue StandardError => e
+ raise ReactOnRails::PrerenderError.new(
+ component_name: react_component_name,
+ err: e,
+ props: options[:props]
+ )
end📝 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.
| def internal_rsc_react_component(react_component_name, options = {}) | |
| options = options.merge(rsc?: true) | |
| render_options = create_render_options(react_component_name, options) | |
| json_stream = server_rendered_react_component(render_options) | |
| json_stream.transform do |chunk| | |
| chunk[:html].html_safe | |
| end | |
| end | |
| def internal_rsc_react_component(react_component_name, options = {}) | |
| options = options.merge(rsc?: true) | |
| render_options = create_render_options(react_component_name, options) | |
| json_stream = server_rendered_react_component(render_options) | |
| raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform) | |
| json_stream.transform do |chunk| | |
| raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html) | |
| chunk[:html].html_safe | |
| end | |
| rescue StandardError => e | |
| raise ReactOnRails::PrerenderError.new( | |
| component_name: react_component_name, | |
| err: e, | |
| props: options[:props] | |
| ) | |
| end |
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
3a61c0c to
268d49d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
lib/react_on_rails/helper.rb (1)
377-401: Enhance error handling and documentation for fiber managementThe fiber management implementation could benefit from the following improvements:
- More descriptive error messages
- Documentation explaining the fiber lifecycle
- Consideration for fiber cleanup
Consider applying these improvements:
def run_stream_inside_fiber unless ReactOnRails::Utils.react_on_rails_pro? raise ReactOnRails::Error, - "You must use React on Rails Pro to use the stream_react_component method." + "Streaming support requires React on Rails Pro subscription. Please upgrade to use this feature." end if @rorp_rendering_fibers.nil? raise ReactOnRails::Error, - "You must call stream_view_containing_react_components to render the view containing the react component" + "Missing initialization: Call stream_view_containing_react_components before rendering streaming components" end + # Store weak references to avoid memory leaks + require 'weakref' + @rorp_rendering_fibers ||= [] rendering_fiber = Fiber.new do stream = yield stream.each_chunk do |chunk| Fiber.yield chunk end end - @rorp_rendering_fibers << rendering_fiber + @rorp_rendering_fibers << WeakRef.new(rendering_fiber) # return the first chunk of the fiber # It contains the initial html of the component # all updates will be appended to the stream sent to browser rendering_fiber.resume end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/react_on_rails/helper.rb(4 hunks)
🔇 Additional comments (1)
lib/react_on_rails/helper.rb (1)
413-420: 🛠️ Refactor suggestion
Enhance error handling and type safety for RSC
The RSC implementation should include:
- Type validation for the JSON stream
- Error handling for transformation failures
Apply this improvement:
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
+ raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
json_stream.transform do |chunk|
+ raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
chunk[:html].html_safe
end
+rescue StandardError => e
+ raise ReactOnRails::PrerenderError.new(
+ component_name: react_component_name,
+ err: e,
+ props: options[:props]
+ )
endLikely invalid or redundant comment.
| @@ -636,6 +657,9 @@ def server_rendered_react_component(render_options) | |||
| js_code: js_code) | |||
| end | |||
|
|
|||
| # TODO: handle errors for rsc streams | |||
There was a problem hiding this comment.
This will be fixed later on the PR that fixes
https://github.com/shakacode/react_on_rails_pro/issues/465
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/react_on_rails/packer_utils.rb (1)
77-78: Improve variable naming for clarity.The variable name
is_bundle_running_on_servercould be more specific about its purpose.- is_bundle_running_on_server = (bundle_name == ReactOnRails.configuration.server_bundle_js_file) || - (bundle_name == ReactOnRails.configuration.rsc_bundle_js_file) + is_server_side_bundle = (bundle_name == ReactOnRails.configuration.server_bundle_js_file) || + (bundle_name == ReactOnRails.configuration.rsc_bundle_js_file)lib/react_on_rails/utils.rb (1)
114-119: Consider adding documentation for RSC bundle configuration.While the implementation is correct, it would be helpful to add documentation explaining:
- The purpose of the RSC bundle
- When and how it's used
- Configuration requirements
spec/react_on_rails/utils_spec.rb (2)
119-121: Fix RuboCop style issues.Address the following style issues:
- Multiple
beforehooks in the same example group- Default
:eachargument can be omitted- Long lines with modifier conditions
- before :each do + before do - ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const) - ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const) + if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const) + ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) + end + if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const) + ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) + end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 119-119: Omit the default
:eachargument for RSpec hooks.(RSpec/HookArgument)
[convention] 119-122: Do not define multiple
beforehooks in the same example group (also defined on line 108).(RSpec/ScatteredSetup)
[convention] 120-120: Modifier form of
ifmakes the line too long.(Style/IfUnlessModifier)
[convention] 120-120: Line is too long. [168/120]
(Layout/LineLength)
[convention] 121-121: Modifier form of
ifmakes the line too long.(Style/IfUnlessModifier)
[convention] 121-121: Line is too long. [164/120]
(Layout/LineLength)
191-191: Fix RuboCop style issues.Address the following style issues:
- Use
%wfor array of words- Add empty lines after
letdeclarations- ["shakapacker", "webpacker"].each do |packer_type| + %w[shakapacker webpacker].each do |packer_type| - let(:packer_public_output_path) { Pathname.new("public/webpack/development") } - include_context "with #{packer_type} enabled" + let(:packer_public_output_path) { Pathname.new("public/webpack/development") } + + include_context "with #{packer_type} enabled"Also applies to: 193-193, 255-255
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 191-191: Use
%wor%Wfor an array of words.(Style/WordArray)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Gemfile.development_dependencies(1 hunks)lib/react_on_rails/configuration.rb(5 hunks)lib/react_on_rails/packer_utils.rb(1 hunks)lib/react_on_rails/utils.rb(2 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)spec/react_on_rails/utils_spec.rb(4 hunks)
🧰 Additional context used
📓 Learnings (1)
node_package/src/ReactOnRailsRSC.ts (1)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
🪛 rubocop (1.69.1)
spec/react_on_rails/utils_spec.rb
[convention] 38-38: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 56-56: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 108-112: Do not define multiple before hooks in the same example group (also defined on line 119).
(RSpec/ScatteredSetup)
[convention] 119-119: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 120-120: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 120-120: Line is too long. [168/120]
(Layout/LineLength)
[convention] 121-121: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 121-121: Line is too long. [164/120]
(Layout/LineLength)
[convention] 191-191: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 193-193: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 255-255: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
🔇 Additional comments (12)
Gemfile.development_dependencies (1)
55-55: Consider using a stable version of webpacker.
Using a release candidate version (6.0.0.rc.6) in production code could lead to stability issues. Consider either:
- Using the latest stable version of webpacker
- Documenting why this specific RC version is required
node_package/src/ReactOnRailsRSC.ts (5)
86-86: 🛠️ Refactor suggestion
Include error stack in development/test environments.
Based on previous learnings, include the error stack in non-production environments for better debugging.
- renderResult = stringToStream(`Error: ${e}`);
+ const isDevOrTest = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test';
+ const errorMessage = isDevOrTest ? `Error: ${e.stack}` : 'An error occurred while rendering the component';
+ renderResult = stringToStream(errorMessage);Likely invalid or redundant comment.
28-36: 🛠️ Refactor suggestion
Improve error handling and configuration in getBundleConfig.
The function needs better error handling and should support configurable paths.
-const getBundleConfig = () => {
+const getBundleConfig = (manifestPath = './public/webpack/development/react-client-manifest.json') => {
try {
- const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8'));
+ const bundleConfig = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
const newBundleConfig: { [key: string]: unknown } = {};
for (const [key, value] of Object.entries(bundleConfig)) {
newBundleConfig[key.replace('file://', '')] = value;
}
return newBundleConfig;
+ } catch (error) {
+ throw new Error(`Failed to read or parse manifest file at ${manifestPath}: ${error}`);
}
}Likely invalid or redundant comment.
67-80: 🛠️ Refactor suggestion
Replace recursive stream processing with iteration.
The current recursive implementation of processStream could lead to stack overflow for large streams.
- const processStream = async () => {
- const { done, value } = await streamReader.read();
- if (done) {
- renderResult?.push(null);
- // @ts-expect-error value is not typed
- debugConsole.log('value', finalValue);
- return;
- }
-
- finalValue += decoder.decode(value);
- renderResult?.push(value);
- processStream();
- }
- processStream();
+ const processStream = async () => {
+ while (true) {
+ const { done, value } = await streamReader.read();
+ if (done) {
+ renderResult?.push(null);
+ break;
+ }
+ finalValue += decoder.decode(value);
+ renderResult?.push(value);
+ }
+ };
+ await processStream();Likely invalid or redundant comment.
12-19:
Avoid modifying globalThis for AsyncLocalStorage.
As discussed in previous comments, modifying globalThis could lead to runtime conflicts. Consider importing AsyncLocalStorage directly where needed.
-(async () => {
- try {
- // @ts-expect-error AsyncLocalStorage is not in the node types
- globalThis.AsyncLocalStorage = (await import('node:async_hooks')).AsyncLocalStorage;
- } catch (e) {
- console.log('AsyncLocalStorage not found');
- }
-})();
+import { AsyncLocalStorage } from 'node:async_hooks';Likely invalid or redundant comment.
1-2: Consider using a more specific import path.
The import from react-server-dom-webpack/server.edge might need to be server.node instead, as mentioned in a previous PR discussion. Please verify the correct import path for your environment.
lib/react_on_rails/utils.rb (2)
Line range hint 69-95: LGTM! Well-structured refactor of bundle path resolution.
The refactoring improves code reusability by extracting common bundle path resolution logic into a shared method. The error handling and caching mechanisms are properly implemented.
98-112: LGTM! Clean adaptation to use the new bundle path resolution.
The method maintains its original functionality while leveraging the new shared implementation.
lib/react_on_rails/configuration.rb (2)
20-20: LGTM! Clean addition of RSC bundle configuration.
The RSC bundle configuration is properly integrated into the existing configuration pattern.
Also applies to: 59-59, 75-76
247-247: LGTM! Proper handling of RSC bundle in webpack generated files.
The RSC bundle is correctly added to the webpack_generated_files list when present.
spec/react_on_rails/utils_spec.rb (2)
9-105: LGTM! Well-structured test helpers and shared contexts.
The shared contexts and helper methods improve test maintainability and readability.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 38-38: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 56-56: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
254-312: LGTM! Comprehensive test coverage for RSC bundle functionality.
The tests thoroughly cover the RSC bundle path resolution under different scenarios:
- Missing manifest entries
- Dev server running
- Client/server bundle configurations
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 255-255: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
node_package/src/ReactOnRailsRSC.ts (1)
12-17: Consider adding error handling to stringToStream utility.While the implementation is correct, it should handle potential errors when pushing to the stream.
const stringToStream = (str: string) => { const stream = new PassThrough(); - stream.push(str); - stream.push(null); + try { + stream.push(str); + stream.push(null); + } catch (error) { + stream.destroy(new Error(`Failed to write to stream: ${error}`)); + } return stream; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node_package/src/ReactOnRailsRSC.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
node_package/src/ReactOnRailsRSC.ts (1)
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
🔇 Additional comments (5)
node_package/src/ReactOnRailsRSC.ts (5)
72-78: Enhance error handling with stack traces in development.
Based on previous learning, include stack traces in development and test environments to aid debugging.
} catch (e: unknown) {
if (throwJsErrors) {
throw e;
}
- renderResult = stringToStream(`Error: ${e}`);
+ const isDevOrTest = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test';
+ const errorMessage = isDevOrTest && e instanceof Error ?
+ `Error: ${e.stack}` :
+ 'An error occurred while rendering the component';
+ renderResult = stringToStream(errorMessage);
}83-84: LGTM! Exports are properly structured.
The exports follow standard practices by re-exporting types and providing the default export.
19-27:
Improve bundle configuration handling.
Several issues need to be addressed:
- Hardcoded development path is environment-specific
- Missing error handling for file operations
- No validation of the bundle config structure
-const getBundleConfig = () => {
+const getBundleConfig = (manifestPath = process.env.BUNDLE_MANIFEST_PATH || './public/webpack/development/react-client-manifest.json') => {
try {
const bundleConfig = JSON.parse(fs.readFileSync('./public/webpack/development/react-client-manifest.json', 'utf8'));
+ if (!bundleConfig || typeof bundleConfig !== 'object') {
+ throw new Error('Invalid bundle configuration format');
+ }
// remove file:// from keys
const newBundleConfig: { [key: string]: unknown } = {};
for (const [key, value] of Object.entries(bundleConfig)) {
newBundleConfig[key.replace('file://', '')] = value;
}
return newBundleConfig;
+ } catch (error) {
+ throw new Error(`Failed to load bundle configuration: ${error}`);
}
}Likely invalid or redundant comment.
58-71:
Replace recursive stream processing with async iteration.
The current recursive implementation could cause stack overflow for large streams. Additionally, the stream processing is not properly awaited.
- const processStream = async () => {
- const { done, value } = await streamReader.read();
- if (done) {
- renderResult?.push(null);
- // @ts-expect-error value is not typed
- debugConsole.log('value', finalValue);
- return;
- }
-
- finalValue += decoder.decode(value);
- renderResult?.push(value);
- processStream();
- }
- processStream();
+ const processStream = async () => {
+ try {
+ while (true) {
+ const { done, value } = await streamReader.read();
+ if (done) {
+ renderResult?.push(null);
+ break;
+ }
+ finalValue += decoder.decode(value);
+ renderResult?.push(value);
+ }
+ } catch (error) {
+ renderResult?.destroy(error);
+ }
+ };
+ // Ensure the stream processing is properly awaited
+ void processStream();Likely invalid or redundant comment.
1-2: Verify the correct server runtime import.
There's a discrepancy between using server.edge here versus server.node in PR #1663. Please clarify which runtime should be used and ensure consistency across the codebase.
✅ Verification successful
Using server.edge is correct for RSC implementation
The current import of react-server-dom-webpack/server.edge is correct for React Server Components implementation. The file is specifically implementing RSC support and the Edge runtime is the recommended choice for RSC as it provides better performance and supports streaming. There's no actual discrepancy with PR #1663 as that PR was a work in progress ([WIP]) and the current implementation has evolved to use the Edge runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other RSC-related imports to ensure consistency
rg -l "react-server-dom-webpack/(server\.(edge|node))"
Length of output: 92
Script:
#!/bin/bash
# Let's check the content of PR #1663 to understand the context of server.node usage
gh pr view 1663 --json body,title
# Also check the file content to see the full context
cat node_package/src/ReactOnRailsRSC.ts
Length of output: 3779
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb (2)
73-75: Consider improving the constant handling.While the error handling test is good, the current approach of defining the
Packerconstant in the before block (with rubocop disable) could be improved.Consider these alternatives:
- Move the constant to a shared context or helper
- Use stub_const instead:
- Packer = ReactOnRails::PackerUtils.packer + stub_const("ReactOnRails::PackerUtils::Packer", ReactOnRails::PackerUtils.packer)This would:
- Eliminate the need for rubocop disables
- Prevent potential constant leakage between tests
- Follow RSpec best practices for constant handling
70-75: Consider adding RSC-specific test cases.While the current changes handle the basic server bundle scenarios, consider adding test cases specifically for RSC bundles:
- RSC bundle with custom name from configuration
- Multiple RSC bundles scenario
- RSC bundle with different file extension
This would ensure robust test coverage for the new RSC support feature. Would you like help drafting these additional test cases?
| allow(ReactOnRails::PackerUtils).to receive(:bundle_js_uri_from_packer) | ||
| .with("manifest.json") | ||
| .and_return(File.join(generated_assets_full_path, "manifest.json")) |
There was a problem hiding this comment.
💡 Codebase verification
Test changes are correct, but other test files need updates
Based on the analysis of the codebase:
- The change from
bundle_js_file_pathtobundle_js_uri_from_packerin the test file is correct - However,
spec/react_on_rails/utils_spec.rbandspec/dummy/spec/packs_generator_spec.rbstill use the old method and need to be updated for consistency - The implementation in
PackerUtilsshows this is part of a broader change to support RSC and handle bundle paths differently
Locations needing attention:
spec/react_on_rails/utils_spec.rb: Contains multiple test cases usingbundle_js_file_paththat should be migratedspec/dummy/spec/packs_generator_spec.rb: Uses the old path resolution method in test setup
🔗 Analysis chain
LGTM! Verify related test coverage.
The mock setup correctly uses PackerUtils.bundle_js_uri_from_packer for manifest path resolution, which aligns with the RSC support changes.
Let's verify the test coverage for similar scenarios:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test cases that might need similar updates
rg -l "bundle_js_file_path" "spec/"
Length of output: 177
Script:
#!/bin/bash
# Let's check the content of these files to see if they need similar updates
echo "=== Content of spec/react_on_rails/utils_spec.rb ==="
rg -A 5 "bundle_js_file_path" "spec/react_on_rails/utils_spec.rb"
echo -e "\n=== Content of spec/dummy/spec/packs_generator_spec.rb ==="
rg -A 5 "bundle_js_file_path" "spec/dummy/spec/packs_generator_spec.rb"
Length of output: 5481
Script:
#!/bin/bash
# Let's check if there are any references to the new PackerUtils method to ensure proper migration
echo "=== Checking for bundle_js_uri_from_packer usage ==="
rg "bundle_js_uri_from_packer" --type ruby
# Also check the implementation of PackerUtils to understand the expected behavior
echo -e "\n=== Checking PackerUtils implementation ==="
fd "packer_utils.rb" -t f
Length of output: 815
Script:
#!/bin/bash
# Let's check the implementation of PackerUtils.bundle_js_uri_from_packer
echo "=== PackerUtils Implementation ==="
rg -A 10 "def self.bundle_js_uri_from_packer" lib/react_on_rails/packer_utils.rb
# Also check the Utils implementation to understand the migration
echo -e "\n=== Utils Implementation ==="
rg -A 10 "def self.bundle_js_file_path" lib/react_on_rails/utils.rb
Length of output: 1768
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
spec/react_on_rails/utils_spec.rb (2)
26-27: Fix misleading commentThe comment incorrectly states "Mock that shakapacker is not installed, so webpacker will be used instead" when actually mocking that shakapacker is installed.
- # Mock that shakapacker is not installed, so webpacker will be used instead + # Mock that shakapacker is installed and webpacker is not🧰 Tools
🪛 rubocop (1.69.1)
[convention] 27-27: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
9-72: Consider extracting common mock setupThe shared contexts have some duplicated mock setup logic. Consider extracting common mock setup into a separate helper method to improve maintainability.
def mock_packer_config(packer_public_output_path) allow(ReactOnRails).to receive_message_chain(:configuration, :generated_assets_dir) .and_return("") allow(ReactOnRails::PackerUtils.packer).to receive_message_chain("dev_server.running?") .and_return(false) allow(ReactOnRails::PackerUtils.packer).to receive_message_chain("config.public_output_path") .and_return(packer_public_output_path) end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 27-27: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 28-28: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 45-45: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 46-46: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 63-63: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 64-64: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/utils_spec.rb(4 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/react_on_rails/utils_spec.rb
[convention] 27-27: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 28-28: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 45-45: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 46-46: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 63-63: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 64-64: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 115-119: Do not define multiple before hooks in the same example group (also defined on line 126).
(RSpec/ScatteredSetup)
[convention] 126-126: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 127-127: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 127-127: Line is too long. [168/120]
(Layout/LineLength)
[convention] 128-128: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 128-128: Line is too long. [164/120]
(Layout/LineLength)
[convention] 198-198: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 200-200: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 262-262: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
🔇 Additional comments (3)
spec/react_on_rails/utils_spec.rb (3)
74-112: LGTM! Well-structured helper methods
The helper methods are well-designed with clear responsibilities and good documentation. The use of random bundle names is particularly good for catching potential mix-ups between server and RSC bundles.
Line range hint 1-320: Overall assessment: Good test coverage with minor improvements needed
The test implementation for RSC support is thorough and well-structured. The shared contexts and helper methods provide a good foundation for testing different packer configurations. Consider the following improvements:
- Address the constant redefinition issues
- Reduce duplication in bundle path tests
- Add error scenario test cases
- Fix RuboCop violations in test setup
The changes align well with the PR objective of adding RSC support.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 27-27: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 28-28: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 45-45: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 46-46: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 63-63: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 64-64: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 115-119: Do not define multiple before hooks in the same example group (also defined on line 126).
(RSpec/ScatteredSetup)
[convention] 126-126: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 126-129: Do not define multiple before hooks in the same example group (also defined on line 115).
(RSpec/ScatteredSetup)
[convention] 127-127: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 127-127: Line is too long. [168/120]
(Layout/LineLength)
[convention] 128-128: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 128-128: Line is too long. [164/120]
(Layout/LineLength)
198-320: Add test cases for error scenarios
The tests cover the happy path but are missing cases for error scenarios such as:
- Invalid bundle names
- Malformed manifest entries
- Network errors when dev server is running
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 198-198: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 200-200: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 262-262: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
| expect(path).to start_with("/") | ||
| context "with server file in the manifest, used for client", packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration |
There was a problem hiding this comment.
Remove constant redefinition in tests
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings. Consider moving this to a let block or before hook.
- Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration
+ let(:packer) { ReactOnRails::PackerUtils.packer }Also applies to: 279-279
spec/react_on_rails/utils_spec.rb
Outdated
| before do | ||
| allow(Rails).to receive(:root).and_return(File.expand_path(".")) | ||
| described_class.instance_variable_set(:@server_bundle_path, nil) | ||
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | ||
| end | ||
|
|
||
| after do | ||
| described_class.instance_variable_set(:@server_bundle_path, nil) | ||
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | ||
| end | ||
|
|
||
| before :each do | ||
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const) | ||
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address RuboCop violations in test setup
Several style issues need to be addressed:
- Multiple
beforehooks should be consolidated - Default
:eachargument should be omitted - Long lines with if conditions should be split
- before do
- allow(Rails).to receive(:root).and_return(File.expand_path("."))
- described_class.instance_variable_set(:@server_bundle_path, nil)
- described_class.instance_variable_set(:@rsc_bundle_path, nil)
- end
-
- before :each do
- ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const)
- ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const)
- end
+ before do
+ allow(Rails).to receive(:root).and_return(File.expand_path("."))
+ described_class.instance_variable_set(:@server_bundle_path, nil)
+ described_class.instance_variable_set(:@rsc_bundle_path, nil)
+
+ if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const)
+ ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const)
+ end
+
+ if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const)
+ ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const)
+ end
+ end📝 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.
| before do | |
| allow(Rails).to receive(:root).and_return(File.expand_path(".")) | |
| described_class.instance_variable_set(:@server_bundle_path, nil) | |
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | |
| end | |
| after do | |
| described_class.instance_variable_set(:@server_bundle_path, nil) | |
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | |
| end | |
| before :each do | |
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const) | |
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const) | |
| before do | |
| allow(Rails).to receive(:root).and_return(File.expand_path(".")) | |
| described_class.instance_variable_set(:@server_bundle_path, nil) | |
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | |
| if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_shakapacker_const) | |
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_shakapacker_const) | |
| end | |
| if ReactOnRails::PackerUtils.instance_variable_defined?(:@using_webpacker_const) | |
| ReactOnRails::PackerUtils.remove_instance_variable(:@using_webpacker_const) | |
| end | |
| end | |
| after do | |
| described_class.instance_variable_set(:@server_bundle_path, nil) | |
| described_class.instance_variable_set(:@rsc_bundle_path, nil) | |
| end |
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 115-119: Do not define multiple before hooks in the same example group (also defined on line 126).
(RSpec/ScatteredSetup)
[convention] 126-126: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 127-127: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 127-127: Line is too long. [168/120]
(Layout/LineLength)
[convention] 128-128: Modifier form of if makes the line too long.
(Style/IfUnlessModifier)
[convention] 128-128: Line is too long. [164/120]
(Layout/LineLength)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/react_on_rails/utils_spec.rb (2)
9-72: Well-structured shared contexts for different packer configurations!The shared contexts effectively isolate the test setup for different packer configurations. However, there are some improvements we can make to follow RSpec best practices:
- Use
described_classinstead of direct class references for better maintainability:- allow(ReactOnRails::Utils).to receive(:gem_available?).with("shakapacker").and_return(true) + allow(described_class).to receive(:gem_available?).with("shakapacker").and_return(true)
- Consider extracting the common gem availability mocks into a helper method to reduce duplication across contexts.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 27-27: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 28-28: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 45-45: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 46-46: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 63-63: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 64-64: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
126-129: Consolidate setup hooks and remove redundant:eachargument- before :each do + before do ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) end end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 126-126: Omit the default
:eachargument for RSpec hooks.(RSpec/HookArgument)
[convention] 126-130: Do not define multiple
beforehooks in the same example group (also defined on line 115).(RSpec/ScatteredSetup)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/utils_spec.rb(4 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/react_on_rails/utils_spec.rb
[convention] 27-27: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 28-28: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 45-45: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 46-46: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 63-63: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 64-64: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 115-119: Do not define multiple before hooks in the same example group (also defined on line 126).
(RSpec/ScatteredSetup)
[convention] 126-126: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 199-199: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 201-201: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 263-263: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
🔇 Additional comments (2)
spec/react_on_rails/utils_spec.rb (2)
74-112: Excellent helper methods for test setup!
The helper methods are well-documented and follow single responsibility principle. The mock_bundle_configs method's documentation about random bundle names is particularly helpful.
218-218: Remove constant redefinition in tests
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings. Consider moving this to a let block or before hook.
- Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration
+ let(:packer) { ReactOnRails::PackerUtils.packer }Also applies to: 280-280
spec/react_on_rails/utils_spec.rb
Outdated
| mock_bundle_configs(server_bundle_name: "server-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.dev_server.running?") | ||
| .and_return(true) | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.dev_server.protocol") | ||
| .and_return("http") | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.dev_server.host_with_port") | ||
| .and_return("localhost:3035") | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.manifest.lookup!") | ||
| .with("webpack-bundle.js") | ||
| .and_return("/webpack/development/webpack-bundle-123456.js") | ||
| .and_return(false) | ||
| mock_bundle_in_manifest("server-bundle.js", "webpack/development/server-bundle-123456.js") | ||
| mock_dev_server_running | ||
|
|
||
| path = described_class.server_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| :shakapacker do | ||
| it "returns the correct path hashed server path" do | ||
| allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file") | ||
| .and_return("server-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.manifest.lookup!") | ||
| .with("server-bundle.js") | ||
| .and_return("webpack/development/server-bundle-123456.js") | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.dev_server.running?") | ||
| .and_return(true) | ||
|
|
||
| path = described_class.server_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| describe ".rsc_bundle_js_file_path with #{packer_type} enabled" do | ||
| let(:packer_public_output_path) { Pathname.new("public/webpack/development") } | ||
| include_context "with #{packer_type} enabled" | ||
|
|
||
| context "with server file not in manifest", packer_type.to_sym do | ||
| it "returns the unhashed server path" do | ||
| server_bundle_name = "rsc-bundle.js" | ||
| mock_bundle_configs(rsc_bundle_name: server_bundle_name) | ||
| mock_missing_manifest_entry(server_bundle_name) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("public/webpack/development/#{server_bundle_name}") | ||
| end | ||
| end | ||
|
|
||
| context "with server file in the manifest, used for client", packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js") | ||
| allow(Packer).to receive_message_chain("dev_server.running?") | ||
| .and_return(false) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
| expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js") | ||
| expect(path).to start_with("/") | ||
| end | ||
|
|
||
| context "with webpack-dev-server running, and same file used for server and client" do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_dev_server_running | ||
| mock_bundle_in_manifest("webpack-bundle.js", "/webpack/development/webpack-bundle-123456.js") | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "rsc-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| mock_bundle_in_manifest("rsc-bundle.js", "webpack/development/server-bundle-123456.js") | ||
| mock_dev_server_running | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider DRYing up the bundle path tests
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical, which violates the DRY principle.
Extract the common test cases into shared examples:
RSpec.shared_examples "bundle path resolution" do |method_name, bundle_type|
context "with server file not in manifest" do
it "returns the unhashed server path" do
bundle_name = "#{bundle_type}-bundle.js"
mock_bundle_configs("#{bundle_type}_bundle_name": bundle_name)
mock_missing_manifest_entry(bundle_name)
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/#{bundle_name}")
end
end
context "with server file in the manifest, used for client" do
it "returns the correct path hashed server path" do
mock_bundle_configs("#{bundle_type}_bundle_name": "webpack-bundle.js")
allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server")
.and_return(true)
mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js")
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js")
expect(path).to start_with("/")
end
end
end
# Usage:
describe ".server_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :server_bundle_js_file_path, "server"
end
describe ".rsc_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :rsc_bundle_js_file_path, "rsc"
end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 199-199: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 201-201: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 263-263: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/react_on_rails/utils_spec.rb (1)
138-141: Consolidate before hooks and remove :each argument.The test setup can be improved by:
- Consolidating scattered before hooks
- Removing the default
:eachargument- before :each do + before do ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 138-138: Omit the default
:eachargument for RSpec hooks.(RSpec/HookArgument)
[convention] 138-142: Do not define multiple
beforehooks in the same example group (also defined on line 127).(RSpec/ScatteredSetup)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/rspec-package-specs.yml(1 hunks)script/convert(1 hunks)spec/react_on_rails/utils_spec.rb(4 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/rspec-package-specs.yml
51-51: shellcheck reported issue in this script: SC2086:info:1:82: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 rubocop (1.69.1)
spec/react_on_rails/utils_spec.rb
[warning] 13-19: Do not define constants this way within a block.
(Lint/ConstantDefinitionInBlock)
[convention] 13-19: Stub constant instead of declaring explicitly.
(RSpec/LeakyConstantDeclaration)
[convention] 18-18: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 40-40: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 58-58: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 75-75: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 76-76: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 127-131: Do not define multiple before hooks in the same example group (also defined on line 138).
(RSpec/ScatteredSetup)
[convention] 138-138: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 213-213: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 275-275: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
🔇 Additional comments (6)
script/convert (2)
17-17: Verify the implications of removing the webpacker gem
The removal of webpacker gem while maintaining shakapacker needs careful consideration:
- Ensure this doesn't break any existing functionality
- Verify if this is compatible with RSC support being added
Let's verify the dependencies and their usage:
✅ Verification successful
Removing webpacker gem is safe and intentional
The removal of the webpacker gem from Gemfile.development_dependencies is correct because:
- The webpacker gem is only present in the test group and is not used in production
- The project is using shakapacker (v8.0.0) as the main packer, which is correctly configured in shakapacker.yml
- The webpacker reference removal is part of the migration from webpacker to shakapacker, as evidenced by the presence of shakapacker configuration and webpack config files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining webpacker references
echo "Checking for webpacker references..."
rg -l 'webpacker' --type ruby
# Check for shakapacker configuration
echo "Checking shakapacker configuration..."
fd -e yml -e yaml -x grep -l 'shakapacker'
Length of output: 679
Script:
#!/bin/bash
# Let's check the content of Gemfile.development_dependencies to understand the context
echo "Checking Gemfile.development_dependencies content..."
cat Gemfile.development_dependencies
# Check the content of shakapacker.yml files to understand the configuration
echo "Checking shakapacker.yml content..."
cat spec/dummy/config/shakapacker.yml
# Let's also check for any webpack-related configuration files
echo "Looking for webpack config files..."
fd webpack.config
Length of output: 4039
Line range hint 1-24: Review version downgrades and configuration changes
The script makes several significant changes that warrant attention:
- Downgrading shakapacker from 8.0.0 to 6.6.0
- Renaming configuration from shakapacker.yml to webpacker.yml
- Modifying webpack configuration function names
These changes could impact the RSC implementation.
Consider:
- Document the reason for version downgrades in the PR description
- Add a comment in the script explaining these architectural decisions
- Ensure the webpack configuration changes align with RSC requirements
Let's verify the compatibility:
✅ Verification successful
Version downgrade and configuration changes are safe for RSC implementation
The changes are compatible with RSC implementation based on the evidence:
- The codebase is actively using React Server Components, as shown by the presence of
react-server-dom-webpackdependency and RSC-related files. - The webpack configuration changes are minimal and maintain the core functionality:
- The
generateWebpackConfigfunction is still being imported from shakapacker - The env-specific configuration loading logic remains intact
- The
- The project uses canary versions of React packages (18.3.0-canary) which supports RSC features.
The version downgrade of shakapacker appears to be a deliberate choice to maintain compatibility with the current RSC setup, as evidenced by the presence of RSC-specific configurations across multiple files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes in the webpack configs
echo "Checking webpack configurations..."
fd webpack.config.js --exec grep -l 'generateWebpackConfig\|webpackConfig'
# Look for RSC-related configurations
echo "Checking for RSC configurations..."
rg -l 'use client|use server' --type js
Length of output: 322
Script:
#!/bin/bash
# Let's examine the webpack config content to understand the changes
echo "Content of webpack.config.js:"
cat ./spec/dummy/config/webpack/webpack.config.js
# Check package.json for RSC-related dependencies
echo -e "\nChecking package.json for React dependencies:"
fd package.json --exec grep -A 5 -B 5 '"react":'
# Check if there are any RSC-specific configurations
echo -e "\nChecking for RSC configuration patterns:"
rg -l "react-server|server-components|react.suspense" --type-add 'config:*.{js,yml,json}'
Length of output: 2067
.github/workflows/rspec-package-specs.yml (1)
50-52: LGTM! Environment variable setup looks good.
The step correctly sets the CI_PACKER_VERSION environment variable based on the matrix version, which is then used to determine which packer to test in the specs.
🧰 Tools
🪛 actionlint (1.7.4)
51-51: shellcheck reported issue in this script: SC2086:info:1:82: Double quote to prevent globbing and word splitting
(shellcheck)
spec/react_on_rails/utils_spec.rb (3)
21-84: LGTM! Well-structured shared contexts.
The shared contexts are well-organized and provide clear setup for different packer configurations. They follow RSpec best practices by:
- Using descriptive context names
- Including appropriate assertions
- Properly mocking dependencies
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 40-40: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 58-58: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 75-75: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 76-76: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
86-124: LGTM! Helper methods improve test maintainability.
The helper methods are well-designed and enhance test readability by:
- Encapsulating common setup logic
- Using meaningful names
- Providing clear documentation
230-230: Remove constant redefinition in tests.
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings.
Also applies to: 292-292
spec/react_on_rails/utils_spec.rb
Outdated
| describe ".rsc_bundle_js_file_path with #{packer_type} enabled" do | ||
| let(:packer_public_output_path) { Pathname.new("public/webpack/development") } | ||
| include_context "with #{packer_type} enabled" | ||
|
|
||
| context "with server file not in manifest", packer_type.to_sym do | ||
| it "returns the unhashed server path" do | ||
| server_bundle_name = "rsc-bundle.js" | ||
| mock_bundle_configs(rsc_bundle_name: server_bundle_name) | ||
| mock_missing_manifest_entry(server_bundle_name) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("public/webpack/development/#{server_bundle_name}") | ||
| end | ||
| end | ||
|
|
||
| context "with server file in the manifest, used for client", packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js") | ||
| allow(Packer).to receive_message_chain("dev_server.running?") | ||
| .and_return(false) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
| expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js") | ||
| expect(path).to start_with("/") | ||
| end | ||
|
|
||
| context "with webpack-dev-server running, and same file used for server and client" do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_dev_server_running | ||
| mock_bundle_in_manifest("webpack-bundle.js", "/webpack/development/webpack-bundle-123456.js") | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "rsc-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| mock_bundle_in_manifest("rsc-bundle.js", "webpack/development/server-bundle-123456.js") | ||
| mock_dev_server_running | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using shared examples for bundle path tests.
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical. This violates the DRY principle.
Extract the common test cases into shared examples:
RSpec.shared_examples "bundle path resolution" do |method_name, bundle_type|
context "with server file not in manifest" do
it "returns the unhashed server path" do
bundle_name = "#{bundle_type}-bundle.js"
mock_bundle_configs("#{bundle_type}_bundle_name": bundle_name)
mock_missing_manifest_entry(bundle_name)
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/#{bundle_name}")
end
end
context "with server file in the manifest, used for client" do
it "returns the correct path hashed server path" do
mock_bundle_configs("#{bundle_type}_bundle_name": "webpack-bundle.js")
allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server")
.and_return(true)
mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js")
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js")
expect(path).to start_with("/")
end
end
end
# Usage:
describe ".server_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :server_bundle_js_file_path, "server"
end
describe ".rsc_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :rsc_bundle_js_file_path, "rsc"
end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 275-275: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
spec/react_on_rails/utils_spec.rb
Outdated
| PACKERS_TO_TEST = if ENV["CI_PACKER_VERSION"] == "old" | ||
| ["webpacker"] | ||
| elsif ENV["CI_PACKER_VERSION"] == "new" | ||
| ["shakapacker"] | ||
| else | ||
| ["shakapacker", "webpacker"] | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using RSpec's let for dynamic values.
The constant definition within a block triggers RuboCop warnings. Consider using RSpec's let for this dynamic value:
- PACKERS_TO_TEST = if ENV["CI_PACKER_VERSION"] == "old"
- ["webpacker"]
- elsif ENV["CI_PACKER_VERSION"] == "new"
- ["shakapacker"]
- else
- ["shakapacker", "webpacker"]
- end
+ let(:packers_to_test) do
+ if ENV["CI_PACKER_VERSION"] == "old"
+ %w[webpacker]
+ elsif ENV["CI_PACKER_VERSION"] == "new"
+ %w[shakapacker]
+ else
+ %w[shakapacker webpacker]
+ end
+ end📝 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.
| PACKERS_TO_TEST = if ENV["CI_PACKER_VERSION"] == "old" | |
| ["webpacker"] | |
| elsif ENV["CI_PACKER_VERSION"] == "new" | |
| ["shakapacker"] | |
| else | |
| ["shakapacker", "webpacker"] | |
| end | |
| let(:packers_to_test) do | |
| if ENV["CI_PACKER_VERSION"] == "old" | |
| %w[webpacker] | |
| elsif ENV["CI_PACKER_VERSION"] == "new" | |
| %w[shakapacker] | |
| else | |
| %w[shakapacker webpacker] | |
| end | |
| end |
🧰 Tools
🪛 rubocop (1.69.1)
[warning] 13-19: Do not define constants this way within a block.
(Lint/ConstantDefinitionInBlock)
[convention] 13-19: Stub constant instead of declaring explicitly.
(RSpec/LeakyConstantDeclaration)
[convention] 18-18: Use %w or %W for an array of words.
(Style/WordArray)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/react_on_rails/utils_spec.rb (2)
138-142: Consolidate before hooksMultiple
beforehooks in the same example group should be consolidated.- before do - allow(Rails).to receive(:root).and_return(File.expand_path(".")) - described_class.instance_variable_set(:@server_bundle_path, nil) - described_class.instance_variable_set(:@rsc_bundle_path, nil) - end - - before :each do - ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| - ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) - end - end + before do + allow(Rails).to receive(:root).and_return(File.expand_path(".")) + described_class.instance_variable_set(:@server_bundle_path, nil) + described_class.instance_variable_set(:@rsc_bundle_path, nil) + + ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| + ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) + end + end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 138-138: Omit the default
:eachargument for RSpec hooks.(RSpec/HookArgument)
[convention] 138-142: Do not define multiple
beforehooks in the same example group (also defined on line 127).(RSpec/ScatteredSetup)
39-40: Use described_class instead of explicit class nameReplace direct references to
ReactOnRails::Utilswithdescribed_classto follow RSpec best practices.- allow(ReactOnRails::Utils).to receive(:gem_available?) + allow(described_class).to receive(:gem_available?)Also applies to: 57-58, 75-76
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 39-39: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
[convention] 40-40: Use
described_classinstead ofReactOnRails::Utils.(RSpec/DescribedClass)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/utils_spec.rb(2 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/react_on_rails/utils_spec.rb
[warning] 13-19: Do not define constants this way within a block.
(Lint/ConstantDefinitionInBlock)
[convention] 13-19: Stub constant instead of declaring explicitly.
(RSpec/LeakyConstantDeclaration)
[convention] 18-18: Use %w or %W for an array of words.
(Style/WordArray)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 40-40: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 58-58: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 75-75: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 76-76: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 127-131: Do not define multiple before hooks in the same example group (also defined on line 138).
(RSpec/ScatteredSetup)
[convention] 138-138: Omit the default :each argument for RSpec hooks.
(RSpec/HookArgument)
[convention] 138-142: Do not define multiple before hooks in the same example group (also defined on line 127).
(RSpec/ScatteredSetup)
[convention] 215-215: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
[convention] 277-277: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
🔇 Additional comments (5)
spec/react_on_rails/utils_spec.rb (5)
21-84: Well-structured shared contexts
The shared contexts are well-organized and follow good testing practices by:
- Isolating test setup for different packer configurations
- Using clear, descriptive context names
- Properly mocking dependencies
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 39-39: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 40-40: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 57-57: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 58-58: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 75-75: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
[convention] 76-76: Use described_class instead of ReactOnRails::Utils.
(RSpec/DescribedClass)
86-124: Well-designed helper methods
The helper methods improve test maintainability and readability. The mock_bundle_configs method with random bundle names is particularly clever as it helps catch potential bundle name mix-ups.
232-232: Remove constant redefinition in tests
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings.
Also applies to: 294-294
276-334: Consider using shared examples for bundle path tests
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical. This violates the DRY principle.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 277-277: Add an empty line after the last let.
(RSpec/EmptyLineAfterFinalLet)
13-19: 🛠️ Refactor suggestion
Convert constant to RSpec let helper
The constant definition violates RSpec best practices and triggers RuboCop warnings.
- PACKERS_TO_TEST = if ENV["CI_PACKER_VERSION"] == "old"
- ["webpacker"]
- elsif ENV["CI_PACKER_VERSION"] == "new"
- ["shakapacker"]
- else
- ["shakapacker", "webpacker"]
- end
+ let(:packers_to_test) do
+ if ENV["CI_PACKER_VERSION"] == "old"
+ %w[webpacker]
+ elsif ENV["CI_PACKER_VERSION"] == "new"
+ %w[shakapacker]
+ else
+ %w[shakapacker webpacker]
+ end
+ endLikely invalid or redundant comment.
🧰 Tools
🪛 rubocop (1.69.1)
[warning] 13-19: Do not define constants this way within a block.
(Lint/ConstantDefinitionInBlock)
[convention] 13-19: Stub constant instead of declaring explicitly.
(RSpec/LeakyConstantDeclaration)
[convention] 18-18: Use %w or %W for an array of words.
(Style/WordArray)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
spec/react_on_rails/utils_spec.rb (1)
131-133: Consider using a more explicit approach for instance variable cleanupThe current approach of clearing all instance variables might be too broad and could accidentally clear important state.
Consider explicitly listing the instance variables to clear:
- ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| - ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) - end + %i[@using_webpacker_const @using_shakapacker_const].each do |var| + ReactOnRails::PackerUtils.remove_instance_variable(var) if ReactOnRails::PackerUtils.instance_variable_defined?(var) + endlib/react_on_rails/helper.rb (2)
132-136: Add documentation for RSC helper methodConsider adding documentation similar to
stream_react_componentto explain:
- Purpose and benefits of RSC
- Usage examples
- Requirements and limitations
377-401: Consider extracting fiber management logicThe fiber management logic is well-implemented but could be more modular. Consider extracting the validation checks into separate methods for better maintainability:
def run_stream_inside_fiber - unless ReactOnRails::Utils.react_on_rails_pro? - raise ReactOnRails::Error, - "You must use React on Rails Pro to use the stream_react_component method." - end - - if @rorp_rendering_fibers.nil? - raise ReactOnRails::Error, - "You must call stream_view_containing_react_components to render the view containing the react component" - end + validate_pro_version! + validate_fiber_initialization! rendering_fiber = Fiber.new do stream = yield stream.each_chunk do |chunk| Fiber.yield chunk end end @rorp_rendering_fibers << rendering_fiber # return the first chunk of the fiber # It contains the initial html of the component # all updates will be appended to the stream sent to browser rendering_fiber.resume end + +private + +def validate_pro_version! + return if ReactOnRails::Utils.react_on_rails_pro? + + raise ReactOnRails::Error, + "You must use React on Rails Pro to use the stream_react_component method." +end + +def validate_fiber_initialization! + return unless @rorp_rendering_fibers.nil? + + raise ReactOnRails::Error, + "You must call stream_view_containing_react_components to render the view containing the react component" +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails/helper.rb(5 hunks)lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb(4 hunks)script/convert(1 hunks)spec/react_on_rails/utils_spec.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/convert
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
🔇 Additional comments (7)
spec/react_on_rails/utils_spec.rb (5)
13-19: Consider using RSpec's let for dynamic values
The constant-style assignment of packers_to_test should be converted to a let block for better RSpec practices.
Apply this diff:
- packers_to_test = if ENV["CI_PACKER_VERSION"] == "old"
- ["webpacker"]
- elsif ENV["CI_PACKER_VERSION"] == "new"
- ["shakapacker"]
- else
- %w[shakapacker webpacker]
- end
+ let(:packers_to_test) do
+ if ENV["CI_PACKER_VERSION"] == "old"
+ %w[webpacker]
+ elsif ENV["CI_PACKER_VERSION"] == "new"
+ %w[shakapacker]
+ else
+ %w[shakapacker webpacker]
+ end
+ end21-84: LGTM! Well-structured shared contexts
The shared contexts are well-organized and provide clear setup for different packer configurations. The inclusion of explicit expectations in each context helps validate the setup.
86-124: LGTM! Well-designed helper methods
The helper methods are well-documented and follow good practices:
- Clear method names that describe their purpose
- Good use of random values to catch potential mix-ups
- Proper mocking of external dependencies
230-230: Remove constant redefinition in tests
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings.
Apply this diff in both locations:
- Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration
+ let(:packer) { ReactOnRails::PackerUtils.packer }Also applies to: 293-293
210-333: Consider using shared examples for bundle path tests
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical, which violates the DRY principle.
Extract the common test cases into shared examples as suggested in the previous review comments.
lib/react_on_rails/helper.rb (2)
127-130: LGTM: Clean streaming implementation
The streaming implementation is well-structured and consistently follows the established pattern of other helper methods.
413-420:
Add error handling and type validation for RSC
The RSC implementation should include:
- Type validation for the JSON stream
- Error handling for transformation failures
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
+ raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
json_stream.transform do |chunk|
+ raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
chunk[:html].html_safe
end
+rescue StandardError => e
+ raise ReactOnRails::PrerenderError.new(
+ component_name: react_component_name,
+ err: e,
+ props: options[:props]
+ )
endLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/react_on_rails/utils_spec.rb (1)
119-121: Consider using a more idiomatic approach for clearing instance variables.The current approach of iterating through instance variables could be simplified.
- ReactOnRails::PackerUtils.instance_variables.each do |instance_variable| - ReactOnRails::PackerUtils.remove_instance_variable(instance_variable) - end + ReactOnRails::PackerUtils.instance_variables.each do |var| + ReactOnRails::PackerUtils.remove_instance_variable(var) if var.to_s.start_with?('@') + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/utils_spec.rb(2 hunks)
🔇 Additional comments (6)
spec/react_on_rails/utils_spec.rb (6)
218-218: Remove constant redefinition in tests.
The Packer constant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings.
Also applies to: 281-281
199-321: Consider using shared examples for bundle path tests.
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical, which violates the DRY principle.
74-112: LGTM! Well-structured helper methods.
The helper methods are well-organized, properly documented, and follow good testing practices:
mock_bundle_in_manifest: Handles manifest mockingmock_missing_manifest_entry: Handles missing entry casesrandom_bundle_name: Generates unique namesmock_bundle_configs: Configures bundle settingsmock_dev_server_running: Sets up dev server state
21-72: LGTM! Well-structured shared contexts.
The shared contexts are well-organized and provide good test setup for different packer configurations:
- "with packer enabled"
- "with shakapacker enabled"
- "with webpacker enabled"
- "without packer enabled"
96-98: Great documentation for random bundle names!
Excellent comment explaining the rationale behind using random bundle names to catch potential swapping issues.
13-19: 🛠️ Refactor suggestion
Consider using RSpec's let for dynamic values.
The constant-like variable definition should be converted to a let block for better RSpec practices.
- packers_to_test = if ENV["CI_PACKER_VERSION"] == "old"
- ["webpacker"]
- elsif ENV["CI_PACKER_VERSION"] == "new"
- ["shakapacker"]
- else
- %w[shakapacker webpacker]
- end
+ let(:packers_to_test) do
+ if ENV["CI_PACKER_VERSION"] == "old"
+ %w[webpacker]
+ elsif ENV["CI_PACKER_VERSION"] == "new"
+ %w[shakapacker]
+ else
+ %w[shakapacker webpacker]
+ end
+ endLikely invalid or redundant comment.
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
eb0edbd to
07e701e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.github/workflows/rspec-package-specs.yml (1)
50-52: Improve environment variable setting step.The script should use quotes to prevent word splitting and have a more descriptive name.
- - name: Set packer version environment variable + - name: Configure packer version for test matrix run: | - echo "CI_PACKER_VERSION=${{ matrix.versions == 'oldest' && 'old' || 'new' }}" >> $GITHUB_ENV + echo "CI_PACKER_VERSION='${{ matrix.versions == 'oldest' && 'old' || 'new' }}'" >> $GITHUB_ENV🧰 Tools
🪛 actionlint (1.7.4)
51-51: shellcheck reported issue in this script: SC2086:info:1:82: Double quote to prevent globbing and word splitting
(shellcheck)
lib/react_on_rails/utils.rb (1)
105-110: Consider adding documentation for RSC bundle configuration.The RSC bundle path implementation follows the server bundle pattern but lacks documentation about its purpose and configuration options.
Add documentation explaining:
- The purpose of the RSC bundle
- When and how it's used
- Configuration options
spec/react_on_rails/utils_spec.rb (2)
13-19: Use RSpec's let for dynamic values.The constant definition within a block triggers RuboCop warnings.
- packers_to_test = if ENV["CI_PACKER_VERSION"] == "old" - ["webpacker"] - elsif ENV["CI_PACKER_VERSION"] == "new" - ["shakapacker"] - else - %w[shakapacker webpacker] - end + let(:packers_to_test) do + if ENV["CI_PACKER_VERSION"] == "old" + %w[webpacker] + elsif ENV["CI_PACKER_VERSION"] == "new" + %w[shakapacker] + else + %w[shakapacker webpacker] + end + end
218-218: Remove constant redefinition in tests.The
Packerconstant is redefined multiple times, which violates Ruby best practices and triggers RuboCop warnings.- Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration + let(:packer) { ReactOnRails::PackerUtils.packer }Also applies to: 281-281
lib/react_on_rails/helper.rb (1)
377-401: Add error handling for edge cases in fiber management.While the implementation is solid, consider adding error handling for these edge cases:
- Fiber creation failure
- Stream errors during chunk processing
- Memory management for long-running streams
def run_stream_inside_fiber unless ReactOnRails::Utils.react_on_rails_pro? raise ReactOnRails::Error, "You must use React on Rails Pro to use the stream_react_component method." end if @rorp_rendering_fibers.nil? raise ReactOnRails::Error, "You must call stream_view_containing_react_components to render the view containing the react component" end rendering_fiber = Fiber.new do + begin stream = yield stream.each_chunk do |chunk| Fiber.yield chunk end + rescue StandardError => e + Rails.logger.error("Error in streaming fiber: #{e.message}") + raise ReactOnRails::Error, "Streaming error occurred: #{e.message}" + ensure + # Clean up resources if needed + stream&.close if stream.respond_to?(:close) + end end @rorp_rendering_fibers << rendering_fiber # return the first chunk of the fiber # It contains the initial html of the component # all updates will be appended to the stream sent to browser rendering_fiber.resume end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.github/workflows/rspec-package-specs.yml(1 hunks)lib/react_on_rails/configuration.rb(5 hunks)lib/react_on_rails/helper.rb(5 hunks)lib/react_on_rails/packer_utils.rb(1 hunks)lib/react_on_rails/react_component/render_options.rb(1 hunks)lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb(4 hunks)lib/react_on_rails/utils.rb(3 hunks)node_package/src/ReactOnRails.ts(2 hunks)node_package/src/ReactOnRailsRSC.ts(1 hunks)node_package/src/types/index.ts(2 hunks)package.json(2 hunks)spec/react_on_rails/configuration_spec.rb(1 hunks)spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb(1 hunks)spec/react_on_rails/utils_spec.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- node_package/src/types/index.ts
- lib/react_on_rails/react_component/render_options.rb
- node_package/src/ReactOnRails.ts
- spec/react_on_rails/configuration_spec.rb
- package.json
- lib/react_on_rails/packer_utils.rb
- lib/react_on_rails/configuration.rb
- spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb
- node_package/src/ReactOnRailsRSC.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/rspec-package-specs.yml
51-51: shellcheck reported issue in this script: SC2086:info:1:82: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (4)
lib/react_on_rails/utils.rb (1)
82-89: 🛠️ Refactor suggestion
Improve error handling safety.
The dynamic constant lookup using Object.const_get could raise unintended errors if the packer type string contains unexpected characters.
- rescue Object.const_get(
- ReactOnRails::PackerUtils.packer_type.capitalize
- )::Manifest::MissingEntryError
+ rescue => e
+ packer_error = "#{ReactOnRails::PackerUtils.packer_type.capitalize}::Manifest::MissingEntryError"
+ raise e unless e.class.name == packer_errorLikely invalid or redundant comment.
lib/react_on_rails/helper.rb (3)
127-130: LGTM! Good refactoring of stream handling logic.
The extraction of common fiber handling logic into run_stream_inside_fiber improves code organization and reduces duplication.
132-136: Implementation looks good, verify error handling.
The RSC implementation follows consistent patterns with streaming components. However, we should verify that error handling for RSC is properly implemented in the internal methods.
Run this script to check for error handling patterns in RSC-related code:
✅ Verification successful
Error handling is properly implemented for RSC components
Based on the search results, error handling for RSC components is properly implemented through multiple layers:
- In
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb, RSC evaluation is wrapped in arescue StandardErrorblock, consistent with other component types - RSC results bypass the JSON parsing error handling since they return raw HTML (avoiding potential JSON parsing errors)
- The implementation reuses the same fiber-based error handling infrastructure as streaming components
- RSC-specific logic is properly guarded with
rsc?checks throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling patterns in RSC-related code
# Look for error handling patterns in RSC methods
rg -A 5 'rsc\?'
# Look for error handling in RSC-related tests
rg -A 5 'test.*rsc'
Length of output: 4097
413-420:
Implement error handling for RSC transformation.
The RSC implementation needs error handling for:
- Invalid JSON chunks
- HTML transformation failures
- Stream processing errors
def internal_rsc_react_component(react_component_name, options = {})
options = options.merge(rsc?: true)
render_options = create_render_options(react_component_name, options)
json_stream = server_rendered_react_component(render_options)
+ raise ReactOnRails::Error, "Invalid RSC stream" unless json_stream.respond_to?(:transform)
json_stream.transform do |chunk|
+ raise ReactOnRails::Error, "Invalid chunk format" unless chunk.is_a?(Hash) && chunk.key?(:html)
chunk[:html].html_safe
+ rescue StandardError => e
+ raise ReactOnRails::PrerenderError.new(
+ component_name: react_component_name,
+ err: e,
+ props: options[:props]
+ )
end
endLikely invalid or redundant comment.
| path = described_class.server_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(server_bundle_name: "server-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| mock_bundle_in_manifest("server-bundle.js", "webpack/development/server-bundle-123456.js") | ||
| mock_dev_server_running | ||
|
|
||
| path = described_class.server_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| :shakapacker do | ||
| it "returns the correct path hashed server path" do | ||
| allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file") | ||
| .and_return("server-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.manifest.lookup!") | ||
| .with("server-bundle.js") | ||
| .and_return("webpack/development/server-bundle-123456.js") | ||
| allow(ReactOnRails::PackerUtils).to receive_message_chain("packer.dev_server.running?") | ||
| .and_return(true) | ||
|
|
||
| path = described_class.server_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| describe ".rsc_bundle_js_file_path with #{packer_type} enabled" do | ||
| let(:packer_public_output_path) { Pathname.new("public/webpack/development") } | ||
|
|
||
| include_context "with #{packer_type} enabled" | ||
|
|
||
| context "with server file not in manifest", packer_type.to_sym do | ||
| it "returns the unhashed server path" do | ||
| server_bundle_name = "rsc-bundle.js" | ||
| mock_bundle_configs(rsc_bundle_name: server_bundle_name) | ||
| mock_missing_manifest_entry(server_bundle_name) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("public/webpack/development/#{server_bundle_name}") | ||
| end | ||
| end | ||
|
|
||
| context "with server file in the manifest, used for client", packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| Packer = ReactOnRails::PackerUtils.packer # rubocop:disable Lint/ConstantDefinitionInBlock, RSpec/LeakyConstantDeclaration | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js") | ||
| allow(Packer).to receive_message_chain("dev_server.running?") | ||
| .and_return(false) | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
| expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js") | ||
| expect(path).to start_with("/") | ||
| end | ||
|
|
||
| context "with webpack-dev-server running, and same file used for server and client" do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "webpack-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(true) | ||
| mock_dev_server_running | ||
| mock_bundle_in_manifest("webpack-bundle.js", "/webpack/development/webpack-bundle-123456.js") | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to eq("http://localhost:3035/webpack/development/webpack-bundle-123456.js") | ||
| end | ||
| end | ||
| end | ||
|
|
||
| context "with dev-server running, and server file in the manifest, and separate client/server files", | ||
| packer_type.to_sym do | ||
| it "returns the correct path hashed server path" do | ||
| mock_bundle_configs(rsc_bundle_name: "rsc-bundle.js") | ||
| allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server") | ||
| .and_return(false) | ||
| mock_bundle_in_manifest("rsc-bundle.js", "webpack/development/server-bundle-123456.js") | ||
| mock_dev_server_running | ||
|
|
||
| path = described_class.rsc_bundle_js_file_path | ||
|
|
||
| expect(path).to end_with("/public/webpack/development/server-bundle-123456.js") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY up bundle path tests using shared examples.
The test cases for server_bundle_js_file_path and rsc_bundle_js_file_path are nearly identical, which violates the DRY principle.
Extract the common test cases into shared examples:
RSpec.shared_examples "bundle path resolution" do |method_name, bundle_type|
context "with server file not in manifest" do
it "returns the unhashed server path" do
bundle_name = "#{bundle_type}-bundle.js"
mock_bundle_configs("#{bundle_type}_bundle_name": bundle_name)
mock_missing_manifest_entry(bundle_name)
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/#{bundle_name}")
end
end
context "with server file in the manifest, used for client" do
it "returns the correct path hashed server path" do
mock_bundle_configs("#{bundle_type}_bundle_name": "webpack-bundle.js")
allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server")
.and_return(true)
mock_bundle_in_manifest("webpack-bundle.js", "webpack/development/webpack-bundle-123456.js")
path = described_class.public_send(method_name)
expect(path).to end_with("public/webpack/development/webpack-bundle-123456.js")
expect(path).to start_with("/")
end
end
end
# Usage:
describe ".server_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :server_bundle_js_file_path, "server"
end
describe ".rsc_bundle_js_file_path" do
it_behaves_like "bundle path resolution", :rsc_bundle_js_file_path, "rsc"
end| # TODO: handle errors for rsc streams | ||
| return result if render_options.rsc? | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
RSC error handling needs implementation following existing patterns
Based on the code analysis, I can see that:
- RSC streams are currently bypassing error handling (
return result if render_options.rsc?) - Regular streaming has robust error handling using
should_raise_streaming_prerender_error?andraise_prerender_error - RSC uses
eval_streaming_jslike regular streaming but doesn't process the errors
The error handling for RSC streams should be implemented following the existing pattern for regular streams, but with RSC-specific considerations:
lib/react_on_rails/helper.rb: Implement error handling before the early return for RSC, similar to the streaming error handling patternlib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb: Consider RSC-specific error parsing inparse_result_and_replay_console_messages
🔗 Analysis chain
Address TODO for RSC error handling.
This TODO is related to the critical issue in internal_rsc_react_component. The error handling implementation should be coordinated between both methods to ensure consistent error management.
Run this script to check for any existing error handling patterns we can follow:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for existing error handling patterns in streaming code
# Check for error handling in streaming methods
rg -A 10 'raise.*PrerenderError'
# Check for error handling in RSC-related code
rg -A 10 'hasErrors.*rsc'
Length of output: 2465
Script:
#!/bin/bash
# Let's check for RSC-related error handling patterns more broadly
# Look for RSC-related error handling in the codebase
rg -B 5 -A 10 'rsc\?'
# Check for any RSC stream handling
rg -B 5 -A 10 'stream.*rsc|rsc.*stream'
# Look for any error handling in RSC context
ast-grep --pattern 'if $_ {
$$$
rsc
$$$
}'
Length of output: 15507
Summary
RSC_in_RORP.mp4
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores