Handle errors happen during streaming components#1648
Handle errors happen during streaming components#1648AbanoubGhadban merged 16 commits intomasterfrom
Conversation
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/pro426-add-support-for-console-replay-while-streaming-component
WalkthroughThe changes introduce enhancements to the Changes
Possibly related PRs
Suggested reviewers
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.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
lib/react_on_rails/helper.rb (2)
539-543: Remove trailing whitespace.The new method
should_raise_streaming_prerender_erroris correctly implemented and integrates well with the existing structure. However, there is trailing whitespace on lines 540, 541, and 605.Apply this diff to remove the trailing whitespace:
- chunk_json_result["hasErrors"] && + chunk_json_result["hasErrors"] && - ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) || + ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) || - +Tools
rubocop
[convention] 540-540: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 541-541: Trailing whitespace detected.
(Layout/TrailingWhitespace)
601-603: Refactor nestedifstatement.Convert
ifnested insideelsetoelsiffor better readability.Apply this diff to refactor the nested
ifstatement:- else - if result["hasErrors"] && render_options.raise_on_prerender_error + elsif result["hasErrors"] && render_options.raise_on_prerender_errorTools
rubocop
[convention] 601-601: Convert
ifnested insideelsetoelsif.(Style/IfInsideElse)
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 (6 hunks)
- lib/react_on_rails/react_component/render_options.rb (3 hunks)
- lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (4 hunks)
- node_package/src/ReactOnRails.ts (2 hunks)
- node_package/src/buildConsoleReplay.ts (2 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 (2 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)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional context used
Biome
node_package/src/buildConsoleReplay.ts
[error] 17-17: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
rubocop
lib/react_on_rails/react_component/render_options.rb
[convention] 134-134: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
lib/react_on_rails/helper.rb
[convention] 540-540: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 541-541: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 601-601: Convert
ifnested insideelsetoelsif.(Style/IfInsideElse)
[convention] 605-605: Trailing whitespace detected.
(Layout/TrailingWhitespace)
Additional comments not posted (30)
jest.config.js (1)
4-4: LGTM!The addition of the
setupFilesarray is correctly implemented and enhances the testing environment by allowing pre-test setup tasks.The code changes are approved.
spec/dummy/config/webpack/alias.js (1)
7-7: LGTM!The addition of the alias mapping for
streamtostream-browserifyis correctly implemented and enhances compatibility with browser environments.The code changes are approved.
node_package/tests/jest.setup.js (1)
1-13: LGTM!The polyfill setup for
TextEncoderandTextDecoderis correctly implemented and ensures compatibility with thejsdomenvironment.The code changes are approved.
spec/dummy/config/webpack/webpackConfig.js (1)
7-7: LGTM!The addition of the fallback for
streamto usestream-browserifyis correctly implemented to ensure compatibility with certain libraries that expect Node.js-style streams.The code changes are approved.
node_package/src/buildConsoleReplay.ts (1)
42-43: LGTM!The function is correctly implemented to accept the new parameter and pass it to
consoleReplay.The code changes are approved.
spec/dummy/config/webpack/commonWebpackConfig.js (1)
44-44: LGTM!The addition of
process: 'process/browser'to theProvidePluginis correctly implemented to ensure compatibility with certain libraries that expect theprocessobject to be defined.The code changes are approved.
package.json (4)
19-19: Verify compatibility with ReactDOM 18.The upgrade to
@types/react-domversion^18.2.0may introduce breaking changes and new features. Ensure that the codebase is compatible with ReactDOM 18.Run the following script to verify the usage of ReactDOM 18 features:
18-18: Verify compatibility with React 18.The upgrade to
@types/reactversion^18.2.0may introduce breaking changes and new features. Ensure that the codebase is compatible with React 18.Run the following script to verify the usage of React 18 features:
42-42: Verify compatibility with React 18.The upgrade to
reactversion^18.2.0may introduce breaking changes and new features. Ensure that the codebase is compatible with React 18.Run the following script to verify the usage of React 18 features:
Verification successful
React 18 Compatibility Verified
The codebase does not appear to use any React 18 specific features or deprecated lifecycle methods that could cause compatibility issues. The upgrade to
reactversion^18.2.0is likely safe.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of React 18 features. # Test: Search for React 18 specific features. Expect: No breaking changes. rg --type js --type ts 'ReactDOM.createRoot'Length of output: 44
Script:
#!/bin/bash # Description: Verify compatibility with React 18 by searching for other React 18 features and changes. # Search for the use of other React 18 specific features. rg --type js --type ts 'useTransition|useDeferredValue|SuspenseList|startTransition' # Check for deprecated lifecycle methods that might have been affected by the upgrade. rg --type js --type ts 'componentWillMount|componentWillReceiveProps|componentWillUpdate'Length of output: 178
43-43: Verify compatibility with ReactDOM 18.The upgrade to
react-domversion^18.2.0may introduce breaking changes and new features. Ensure that the codebase is compatible with ReactDOM 18.Run the following script to verify the usage of ReactDOM 18 features:
Verification successful
ReactDOM 18 Compatibility Verified with Caution
The codebase uses
ReactDOM.hydrateRoot, a React 18 feature, indicating compatibility with React 18. However, ensure that its implementation is correct and does not introduce any issues.
node_package/src/clientStartup.ts: Usage ofReactDOM.hydrateRoot.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ReactDOM 18 features. # Test: Search for ReactDOM 18 specific features. Expect: No breaking changes. rg --type js --type ts 'ReactDOM.createRoot'Length of output: 44
Script:
#!/bin/bash # Description: Verify the usage of other ReactDOM 18 features and potential breaking changes. # Test: Search for other ReactDOM 18 specific features and changes. rg --type js --type ts 'ReactDOM.unstable_' # Check for any usage of unstable features. rg --type js --type ts 'ReactDOM.flushSync' # Check for usage of flushSync, a new API in React 18. rg --type js --type ts 'ReactDOM.hydrateRoot' # Check for usage of hydrateRoot, another new API in React 18.Length of output: 264
lib/react_on_rails/react_component/render_options.rb (3)
90-92: Verify configuration value for non-shell server rendering errors.The method retrieves a configuration value for non-shell server rendering errors. Ensure that the configuration value is correctly set in the configuration files.
Run the following script to verify the configuration value:
110-112: Verify streaming option.The method checks the streaming option from the
optionshash. Ensure that the streaming option is correctly set in the configuration files.Run the following script to verify the streaming option:
132-137: Verify Pro-specific settings.The method attempts to fetch configuration values from the
optionshash and falls back to theReactOnRailsProconfiguration if thereact_on_rails_pro?condition is met. Ensure that the Pro-specific settings are accessible when applicable.Run the following script to verify the Pro-specific settings:
Tools
rubocop
[convention] 134-134: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
node_package/tests/ReactOnRails.test.js (1)
22-30: Verify impact on other test cases.The test case has been modified to programmatically create the root div element and validate the text content of the root div instead of the component type. Ensure that the changes do not affect other test cases.
Run the following script to verify the impact on other test cases:
Verification successful
No impact on other test cases.
The changes in
ReactOnRails.test.jsare isolated to creating a div element and do not affect the test cases inAuthenticity.test.js, which involve creating a meta tag for CSRF tokens. There is no shared state or dependency between these test cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact on other test cases. # Test: Search for the test cases. Expect: No breaking changes. rg --type js --type ts 'document.createElement'Length of output: 220
Script:
#!/bin/bash # Extract relevant lines from Authenticity.test.js to analyze the usage of document.createElement rg --type js --type ts -A 5 'document.createElement' node_package/tests/Authenticity.test.jsLength of output: 261
node_package/src/types/index.ts (2)
2-2: LGTM!The import statement for
Readablefrom thestreammodule is correctly added.The code changes are approved.
141-141: LGTM!The method
streamServerRenderedReactComponentis correctly added to theReactOnRailsinterface.The code changes are approved.
node_package/src/serverRenderReactComponent.ts (3)
170-175: LGTM!The helper function
stringToStreamis correctly implemented to convert a string to a readable stream usingPassThrough.The code changes are approved.
177-265: LGTM!The function
streamServerRenderedReactComponentis correctly implemented to handle streaming server-rendered React components with proper error handling and logging mechanisms.The code changes are approved.
267-267: LGTM!The default export remains unchanged and there are no issues.
The code changes are approved.
lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (3)
59-63: LGTM!The conditional logic correctly switches between
eval_streaming_jsandeval_jsbased on thestreamflag.The code changes are approved.
79-83: LGTM!The refactoring to encapsulate the parsing logic in a separate method
parse_result_and_replay_console_messagesimproves code clarity and maintainability.The code changes are approved.
229-247: LGTM!The method
parse_result_and_replay_console_messagesis correctly implemented to handle result parsing and console message replaying.The code changes are approved.
node_package/src/ReactOnRails.ts (2)
2-2: LGTM!The import statement has been correctly updated to include
streamServerRenderedReactComponent.The code changes are approved.
Also applies to: 8-8
245-251: LGTM!The new method
streamServerRenderedReactComponentis correctly implemented and integrates well with the existing structure.The code changes are approved.
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
10-10: LGTM!The inclusion of
ActionView::Helpers::TagHelperis correctly done.The code changes are approved.
350-373: LGTM!The new method
rails_context_if_not_already_renderedis correctly implemented and the tests ensure its correct behavior.The code changes are approved.
lib/react_on_rails/helper.rb (4)
94-114: LGTM!The new method
stream_react_componentis correctly implemented and integrates well with the existing structure.The code changes are approved.
355-363: LGTM!The new method
internal_stream_react_componentis correctly implemented and integrates well with the existing structure.The code changes are approved.
396-420: LGTM!The new method
build_react_component_result_for_server_streamed_contentis correctly implemented and integrates well with the existing structure.The code changes are approved.
529-537: LGTM!The new method
raise_prerender_erroris correctly implemented and integrates well with the existing structure.The code changes are approved.
Comments failed to post (1)
node_package/src/buildConsoleReplay.ts (1)
14-21: Use
Array.isArrayinstead ofinstanceof Array.
instanceof Arrayreturns false for array-like objects and arrays from other execution contexts. UseArray.isArrayinstead for better compatibility and reliability.Apply this diff to fix the issue:
- if (!(console.history instanceof Array)) { + if (!Array.isArray(console.history)) {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.export function consoleReplay(skipFirstNumberOfMessages: number = 0): string { // console.history is a global polyfill used in server rendering. // $FlowFixMe if (!Array.isArray(console.history)) { return ''; } const lines = console.history.slice(skipFirstNumberOfMessages).map(msg => {Tools
Biome
[error] 17-17: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.(lint/suspicious/useIsArray)
justin808
left a comment
justin808
left a comment
There was a problem hiding this comment.
@alexeyr-ci @AbanoubGhadban merge if you guys are confident.
Should I do a release after?
AbanoubGhadban
force-pushed
the
abanoubghadban/pro426-add-support-for-console-replay-while-streaming-component
branch
from
bcfddd6 to
6ad16f1
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro426-add-support-for-console-replay-while-streaming-component
branch
5 times, most recently
from
bd57e3d to
49547bb
Compare
AbanoubGhadban
force-pushed
the
pro431-handle-erros-happen-during-streaming-components
branch
from
7ce5cb5 to
ad9b93c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
lib/react_on_rails/react_component/render_options.rb (1)
90-92: Consider renaming the method to better reflect its purpose.
The current method name raise_non_shell_server_rendering_errors implies that it raises errors, but it actually retrieves a configuration value that determines whether errors should be raised. Consider renaming it to follow Ruby's predicate method naming convention:
-def raise_non_shell_server_rendering_errors
+def raise_non_shell_server_rendering_errors?
retrieve_react_on_rails_pro_config_value_for(:raise_non_shell_server_rendering_errors)
endlib/react_on_rails/helper.rb (2)
573-581: Add unit tests for raise_prerender_error method
Consider adding unit tests for the new raise_prerender_error method to ensure it raises the expected exceptions under various conditions. This will help maintain robustness and prevent future regressions.
583-588: Improve readability of complex condition in should_raise_streaming_prerender_error?
The conditional logic within should_raise_streaming_prerender_error? is complex and may benefit from added clarity. Consider adding inline comments or refactoring the condition to enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- lib/react_on_rails/helper.rb (2 hunks)
- lib/react_on_rails/react_component/render_options.rb (2 hunks)
- node_package/src/serverRenderReactComponent.ts (5 hunks)
🔇 Additional comments (2)
lib/react_on_rails/react_component/render_options.rb (1)
90-92: Verify integration with streaming error handling.
The new configuration retrieval methods are part of the error handling improvements for streaming components. Let's verify their usage in the streaming context:
Also applies to: 136-142
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Error handling configuration is correctly integrated with streaming components
The raise_non_shell_server_rendering_errors configuration is properly integrated in the error handling flow:
- It's used in
helper.rbto control error handling during streaming component rendering - The configuration is retrieved through
render_options.rband used to determine whether to raise errors when the shell is not ready - The error handling system supports both streaming and non-streaming scenarios with proper error class hierarchies (PrerenderError, JsonParseError)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these configuration values are used in streaming-related code
# Test 1: Find files that handle streaming and reference these configurations
echo "=== Files handling streaming and error configurations ==="
rg -l "stream.*raise_non_shell_server_rendering_errors"
# Test 2: Check for potential error handling patterns in streaming code
echo "=== Streaming error handling patterns ==="
rg "rescue.*stream" -A 5
Length of output: 302
Script:
#!/bin/bash
# Let's broaden our search to understand the error handling context better
# Find all files referencing raise_non_shell_server_rendering_errors
echo "=== Files using raise_non_shell_server_rendering_errors ==="
rg "raise_non_shell_server_rendering_errors" -l
# Check for streaming-related code
echo "=== Files with streaming functionality ==="
rg "stream" --type ruby -l
# Look for error handling in component rendering
echo "=== Error handling in component rendering ==="
rg "rescue" --type ruby -A 5 lib/react_on_rails/
# Check the configuration usage
echo "=== Configuration usage ==="
rg "ReactOnRails\.configuration" --type ruby -A 3
Length of output: 29632
Script:
#!/bin/bash
# Let's check how the error handling configuration is used in the codebase
# Check for error handling in helper.rb where most rendering logic resides
echo "=== Error handling in helper.rb ==="
rg "raise_non_shell_server_rendering_errors" lib/react_on_rails/helper.rb -A 5
# Check for error handling in render_options.rb
echo "=== Error handling in render_options.rb ==="
rg "raise_non_shell_server_rendering_errors" lib/react_on_rails/react_component/render_options.rb -A 5
# Check for server rendering error handling
echo "=== Server rendering error handling ==="
rg "PrerenderError|ReactOnRails::Error" --type ruby -A 5
Length of output: 25888
lib/react_on_rails/helper.rb (1)
641-641: Previous comment still applies: Correct the grammar in the inline comment
The inline comment on line 641 has grammatical errors that were previously pointed out. Please update the comment for clarity.
| def retrieve_react_on_rails_pro_config_value_for(key) | ||
| options.fetch(key) do | ||
| return nil unless ReactOnRails::Utils.react_on_rails_pro? | ||
|
|
||
| ReactOnRailsPro.configuration.public_send(key) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add documentation for Pro configuration retrieval.
The method silently returns nil for non-Pro installations and might not handle missing configuration keys in ReactOnRailsPro gracefully. Consider:
- Adding documentation to clarify the behavior
- Adding error handling for missing Pro configuration keys
- Consider logging a warning when accessed in non-Pro installations
+ # Retrieves a configuration value from ReactOnRailsPro if available
+ # @param key [Symbol] the configuration key to retrieve
+ # @return [Object, nil] the configuration value or nil if Pro is not enabled
+ # @raise [ReactOnRails::Error] if the configuration key doesn't exist in Pro config
def retrieve_react_on_rails_pro_config_value_for(key)
options.fetch(key) do
- return nil unless ReactOnRails::Utils.react_on_rails_pro?
+ unless ReactOnRails::Utils.react_on_rails_pro?
+ Rails.logger.debug("Attempted to access Pro configuration '#{key}' in non-Pro installation")
+ return nil
+ end
- ReactOnRailsPro.configuration.public_send(key)
+ unless ReactOnRailsPro.configuration.respond_to?(key)
+ raise ReactOnRails::Error, "Missing configuration key '#{key}' in ReactOnRailsPro"
+ end
+
+ ReactOnRailsPro.configuration.public_send(key)
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.
| def retrieve_react_on_rails_pro_config_value_for(key) | |
| options.fetch(key) do | |
| return nil unless ReactOnRails::Utils.react_on_rails_pro? | |
| ReactOnRailsPro.configuration.public_send(key) | |
| end | |
| end | |
| # Retrieves a configuration value from ReactOnRailsPro if available | |
| # @param key [Symbol] the configuration key to retrieve | |
| # @return [Object, nil] the configuration value or nil if Pro is not enabled | |
| # @raise [ReactOnRails::Error] if the configuration key doesn't exist in Pro config | |
| def retrieve_react_on_rails_pro_config_value_for(key) | |
| options.fetch(key) do | |
| unless ReactOnRails::Utils.react_on_rails_pro? | |
| Rails.logger.debug("Attempted to access Pro configuration '#{key}' in non-Pro installation") | |
| return nil | |
| end | |
| unless ReactOnRailsPro.configuration.respond_to?(key) | |
| raise ReactOnRails::Error, "Missing configuration key '#{key}' in ReactOnRailsPro" | |
| end | |
| ReactOnRailsPro.configuration.public_send(key) | |
| end | |
| end |
| consoleHistory = console.history; | ||
| const transformStream = new PassThrough({ |
There was a problem hiding this comment.
Ensure 'console.history' is available
console.history may not be defined in all environments. To prevent potential runtime errors, consider providing a default value when it's undefined.
Apply this diff:
- consoleHistory = console.history;
+ consoleHistory = console.history || [];📝 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.
| consoleHistory = console.history; | |
| const transformStream = new PassThrough({ | |
| consoleHistory = console.history || []; | |
| const transformStream = new PassThrough({ |
| stream.write(str); | ||
| stream.end(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor 'stringToStream' using 'Readable.from'
You can simplify the stringToStream function by utilizing Readable.from, which is available in Node.js v12 and above. This approach makes the code more concise and leverages built-in stream utilities.
Apply this diff:
-const stringToStream = (str: string): Readable => {
- const stream = new PassThrough();
- stream.write(str);
- stream.end();
- return stream;
-};
+const stringToStream = (str: string): Readable => Readable.from([str]);📝 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.
| stream.write(str); | |
| stream.end(); | |
| const stringToStream = (str: string): Readable => Readable.from([str]); |
| onError() { | ||
| // Can't through error here if throwJsErrors is true because the error will happen inside the stream | ||
| // And will not be handled by any catch clause | ||
| hasErrors = true; | ||
| }, |
There was a problem hiding this comment.
Handle errors in 'onError' callback consistently
In the onError callback, the error is currently not being handled or communicated to the client. To ensure consistent error handling, consider processing the error similarly to onShellError.
Apply this diff:
- onError() {
+ onError(e) {
// Can't throw an error here if throwJsErrors is true because the error will happen inside the stream
// and will not be handled by any catch clause
hasErrors = true;
+ const error = e instanceof Error ? e : new Error(String(e));
+ transformStream.write(handleError({
+ e: error,
+ name: componentName,
+ serverSide: true,
+ }));
+ transformStream.end();
},This ensures that errors encountered during rendering are properly relayed to the client.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
node_package/src/types/index.ts (1)
141-141: Add JSDoc comment to document the isShellReady property.The addition of
isShellReadylooks good and aligns with the PR's objective of improving streaming component handling. However, consider adding documentation to explain its purpose and usage.export interface RenderResult { html: string | null; consoleReplayScript: string; hasErrors: boolean; renderingError?: RenderingError; + /** Indicates whether the shell component is ready during streaming render */ isShellReady?: boolean; }node_package/tests/streamServerRenderedReactComponent.test.jsx (2)
43-51: Add error handling for JSON parsing inexpectStreamChunkWhen parsing
chunkas JSON, it's good practice to handle potential parsing errors to prevent unhandled exceptions during tests.Apply this diff to add error handling:
const expectStreamChunk = (chunk) => { expect(typeof chunk).toBe('string'); - const jsonChunk = JSON.parse(chunk); + let jsonChunk; + try { + jsonChunk = JSON.parse(chunk); + } catch (error) { + throw new Error(`Failed to parse chunk as JSON: ${error.message}`); + } expect(typeof jsonChunk.html).toBe('string'); expect(typeof jsonChunk.consoleReplayScript).toBe('string'); expect(typeof jsonChunk.hasErrors).toBe('boolean'); expect(typeof jsonChunk.isShellReady).toBe('boolean'); return jsonChunk; };
99-99: Simplify regular expressions in.toMatchassertionsThe regular expressions used in
.toMatchassertions can be simplified for clarity. Using[\s\S]*effectively matches any character including line breaks.Apply this diff to simplify the regular expressions:
-expect(chunks[0].html).toMatch(/<pre>Exception in rendering[.\s\S]*Sync Error[.\s\S]*<\/pre>/); +expect(chunks[0].html).toMatch(/<pre>Exception in rendering[\s\S]*Sync Error[\s\S]*<\/pre>/); ... -expect(chunks[1].html).toMatch(/<script>[.\s\S]*Async Error[.\s\S]*<\/script>/); +expect(chunks[1].html).toMatch(/<script>[\s\S]*Async Error[\s\S]*<\/script>/);Also applies to: 113-113, 132-132, 151-151
📜 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 (4)
node_package/src/serverRenderReactComponent.ts(5 hunks)node_package/src/types/index.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)package.json(1 hunks)
🔇 Additional comments (9)
package.json (1)
42-43: Caution: Using React canary build in production
Switching to a specific canary build (18.3.0-canary-670811593-20240322) raises stability concerns:
- Canary builds are experimental and not recommended for production use
- They may contain breaking changes or unstable features
- Consider using the latest stable version (18.2.0) unless there's a specific streaming-related feature in this canary that's essential for the error handling improvements
Let's verify if this specific canary version is required for the streaming error handling improvements:
✅ Verification successful
Let me search for more specific information about streaming error handling improvements in the codebase.
Let me try a different search approach focusing on the test files and error handling code.
React canary version required for improved streaming error handling
Based on the test files, the canary version is justified as it adds enhanced error handling capabilities for streaming components:
- Support for both synchronous and asynchronous error handling in streamed components
- Granular control over error propagation with
throwJsErrorsflag - Proper error state management in streaming chunks with
hasErrorsandisShellReadyflags
The specific canary version (18.3.0-canary-670811593-20240322) appears to be required for these streaming error handling improvements, making it a necessary upgrade despite being a pre-release version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for streaming-related changes that might depend on React 18.3 features
rg -g '!package*.json' -i "18.3|canary"
# Look for streaming-related comments or documentation
rg -g '!package*.json' -i "streaming.*error|error.*streaming"
Length of output: 38961
Script:
#!/bin/bash
# Look for streaming error handling related code and changes
rg -A 5 -B 5 "streaming.*error|error.*streaming|throwSyncError|throwAsyncError" --type js --type jsx --type ts --type tsx
# Look for any documentation about streaming error handling
fd -e md -e mdx | xargs rg -l "streaming.*error|error.*streaming"
# Check for any test files related to streaming errors
fd -e test.js -e test.jsx -e test.ts -e test.tsx | xargs rg -l "streaming.*error|error.*streaming"
Length of output: 320
Script:
#!/bin/bash
# Search for streaming and error related test files
rg -A 5 -B 5 "throwSyncError|throwAsyncError|streaming.*error" node_package/tests/
# Look for any documentation about streaming error handling in docs
rg -i "streaming.*error|error.*streaming" docs/
# Check the commit message or PR description for context
git log -p -1
Length of output: 14041
node_package/src/types/index.ts (1)
141-141: Verify integration with streaming render components.
Let's verify that this property is properly integrated with the streaming render components.
✅ Verification successful
Let me try a different search strategy to verify the integration.
Property isShellReady is properly integrated with streaming render components
The verification shows that isShellReady is well integrated within the streaming render system:
- Defined in
StreamRenderStatetype inserverRenderReactComponent.ts - Used in error handling logic for streaming renders
- Properly tested in streaming render component tests with both success and error scenarios
- Correctly initialized as
falseand set totrueinonShellReadycallback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of isShellReady in streaming-related files
# Test 1: Find streaming-related files that use isShellReady
echo "=== Files using isShellReady ==="
rg "isShellReady" --type ts --type tsx
# Test 2: Check StreamRenderState type definition
echo -e "\n=== StreamRenderState definition ==="
ast-grep --pattern 'type StreamRenderState = {
$$$
isShellReady: $_
$$$
}'
Length of output: 313
Script:
#!/bin/bash
# Search for files containing isShellReady with proper context
echo "=== Files using isShellReady ==="
rg "isShellReady" -A 3 -B 3
# Search for any streaming render related types or components
echo -e "\n=== Streaming render related files ==="
rg "StreamRender|StreamingRender" -l
# Look for type definitions that might include isShellReady
echo -e "\n=== Type definitions with shell ready state ==="
ast-grep --pattern 'type $_ = {
$$$
isShellReady?: boolean
$$$
}'
Length of output: 9844
node_package/tests/streamServerRenderedReactComponent.test.jsx (2)
40-41: Ensure proper cleanup of component registrations between tests
While ComponentRegistry.components().clear() is called in beforeEach, it's important to verify that components are correctly registered and unregistered to prevent side effects between tests.
Also applies to: 58-59
10-16: 🛠️ Refactor suggestion
Verify the use of async function components in React
Defining AsyncContent as an async function component may cause unexpected behavior, as React components are generally expected to be synchronous functions. While React 18 introduces support for asynchronous rendering and server-side Suspense, using async functions as components is not recommended. Consider refactoring AsyncContent to be a regular function or use a higher-order component to handle the asynchronous behavior.
Apply this diff to refactor AsyncContent:
-const AsyncContent = async ({ throwAsyncError }) => {
- await new Promise((resolve) => setTimeout(resolve, 0));
- if (throwAsyncError) {
- throw new Error('Async Error');
- }
- return <div>Async Content</div>;
-};
+const AsyncContent = ({ throwAsyncError }) => {
+ if (throwAsyncError) {
+ throw new Error('Async Error');
+ }
+ return <div>Async Content</div>;
+};If asynchronous data fetching is necessary, consider using a data-fetching library compatible with server-side Suspense.
node_package/src/serverRenderReactComponent.ts (5)
1-2: LGTM!
The import statements are correct, and using type in the import enhances clarity for TypeScript types.
18-21: LGTM!
The StreamRenderState type is appropriately defined, extending RenderState and including the new properties.
109-109: LGTM!
Including isShellReady in the result object conditionally is a sensible approach.
204-205: Duplicate comment regarding 'stringToStream' function
210-210: Duplicate comment: Ensure 'console.history' is available
| "react": "18.3.0-canary-670811593-20240322", | ||
| "react-dom": "18.3.0-canary-670811593-20240322", |
There was a problem hiding this comment.
Update @types/react to match React version*
There's a version mismatch between React and its type definitions:
- React/React-DOM: 18.3.0-canary-670811593-20240322
- @types/react & @types/react-dom: ^18.2.0
This could lead to type checking issues as the type definitions might not cover new features or changes in the canary build.
Apply this diff to align the versions:
- "@types/react": "^18.2.0",
- "@types/react-dom": "^18.2.0",
+ "@types/react": "^18.3.0",
+ "@types/react-dom": "^18.3.0",Committable suggestion was skipped due to low confidence.
| const decodedText = new TextDecoder().decode(chunk); | ||
| chunks.push(expectStreamChunk(decodedText)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify chunk decoding in the stream handler
Using TextDecoder to decode chunks may not be necessary if the stream is set to emit strings. You can set the stream's encoding to 'utf8' to receive chunks as strings directly, simplifying the code.
Apply this diff to simplify chunk handling:
-renderResult.on('data', (chunk) => {
- const decodedText = new TextDecoder().decode(chunk);
- chunks.push(expectStreamChunk(decodedText));
-});
+renderResult.setEncoding('utf8');
+renderResult.on('data', (chunk) => {
+ chunks.push(expectStreamChunk(chunk));
+});Committable suggestion was skipped due to low confidence.
| it('streamServerRenderedReactComponent streams the rendered component', async () => { | ||
| const { renderResult, chunks } = setupStreamTest(); | ||
| await new Promise((resolve) => renderResult.on('end', resolve)); | ||
|
|
||
| expect(chunks).toHaveLength(2); | ||
| expect(chunks[0].html).toContain('Header In The Shell'); | ||
| expect(chunks[0].consoleReplayScript).toBe(''); | ||
| expect(chunks[0].hasErrors).toBe(false); | ||
| expect(chunks[0].isShellReady).toBe(true); | ||
| expect(chunks[1].html).toContain('Async Content'); | ||
| expect(chunks[1].consoleReplayScript).toBe(''); | ||
| expect(chunks[1].hasErrors).toBe(false); | ||
| expect(chunks[1].isShellReady).toBe(true); | ||
| }); | ||
|
|
||
| it('emits an error if there is an error in the shell and throwJsErrors is true', async () => { | ||
| const { renderResult, chunks } = setupStreamTest({ throwSyncError: true, throwJsErrors: true }); | ||
| const onError = jest.fn(); | ||
| renderResult.on('error', onError); | ||
| await new Promise((resolve) => renderResult.on('end', resolve)); | ||
|
|
||
| expect(onError).toHaveBeenCalled(); | ||
| expect(chunks).toHaveLength(1); | ||
| expect(chunks[0].html).toMatch(/<pre>Exception in rendering[.\s\S]*Sync Error[.\s\S]*<\/pre>/); | ||
| expect(chunks[0].consoleReplayScript).toBe(''); | ||
| expect(chunks[0].hasErrors).toBe(true); | ||
| expect(chunks[0].isShellReady).toBe(false); | ||
| }); | ||
|
|
||
| it("doesn't emit an error if there is an error in the shell and throwJsErrors is false", async () => { | ||
| const { renderResult, chunks } = setupStreamTest({ throwSyncError: true, throwJsErrors: false }); | ||
| const onError = jest.fn(); | ||
| renderResult.on('error', onError); | ||
| await new Promise((resolve) => renderResult.on('end', resolve)); | ||
|
|
||
| expect(onError).not.toHaveBeenCalled(); | ||
| expect(chunks).toHaveLength(1); | ||
| expect(chunks[0].html).toMatch(/<pre>Exception in rendering[.\s\S]*Sync Error[.\s\S]*<\/pre>/); | ||
| expect(chunks[0].consoleReplayScript).toBe(''); | ||
| expect(chunks[0].hasErrors).toBe(true); | ||
| expect(chunks[0].isShellReady).toBe(false); | ||
| }); | ||
|
|
||
| it('emits an error if there is an error in the async content and throwJsErrors is true', async () => { | ||
| const { renderResult, chunks } = setupStreamTest({ throwAsyncError: true, throwJsErrors: true }); | ||
| const onError = jest.fn(); | ||
| renderResult.on('error', onError); | ||
| await new Promise((resolve) => renderResult.on('end', resolve)); | ||
|
|
||
| expect(onError).toHaveBeenCalled(); | ||
| expect(chunks).toHaveLength(2); | ||
| expect(chunks[0].html).toContain('Header In The Shell'); | ||
| expect(chunks[0].consoleReplayScript).toBe(''); | ||
| expect(chunks[0].hasErrors).toBe(false); | ||
| expect(chunks[0].isShellReady).toBe(true); | ||
| // Script that fallbacks the render to client side | ||
| expect(chunks[1].html).toMatch(/<script>[.\s\S]*Async Error[.\s\S]*<\/script>/); | ||
| expect(chunks[1].consoleReplayScript).toBe(''); | ||
| expect(chunks[1].hasErrors).toBe(true); | ||
| expect(chunks[1].isShellReady).toBe(true); | ||
| }); | ||
|
|
||
| it("doesn't emit an error if there is an error in the async content and throwJsErrors is false", async () => { | ||
| const { renderResult, chunks } = setupStreamTest({ throwAsyncError: true, throwJsErrors: false }); | ||
| const onError = jest.fn(); | ||
| renderResult.on('error', onError); | ||
| await new Promise((resolve) => renderResult.on('end', resolve)); | ||
|
|
||
| expect(onError).not.toHaveBeenCalled(); | ||
| expect(chunks).toHaveLength(2); | ||
| expect(chunks[0].html).toContain('Header In The Shell'); | ||
| expect(chunks[0].consoleReplayScript).toBe(''); | ||
| expect(chunks[0].hasErrors).toBe(false); | ||
| expect(chunks[0].isShellReady).toBe(true); | ||
| // Script that fallbacks the render to client side | ||
| expect(chunks[1].html).toMatch(/<script>[.\s\S]*Async Error[.\s\S]*<\/script>/); | ||
| expect(chunks[1].consoleReplayScript).toBe(''); | ||
| expect(chunks[1].hasErrors).toBe(true); | ||
| expect(chunks[1].isShellReady).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor tests to reduce duplicated code
Multiple tests have similar setup and assertion logic, leading to code duplication. Consider refactoring common patterns into helper functions to improve maintainability.
For example, create a helper to wait for the stream to end:
+const waitForStreamEnd = (renderResult) =>
+ new Promise((resolve) => renderResult.on('end', resolve));
// Usage in tests:
-await new Promise((resolve) => renderResult.on('end', resolve));
+await waitForStreamEnd(renderResult);Additionally, you can abstract repeated assertions:
+const expectChunk = (chunk, expectedValues) => {
+ expect(chunk.html).toMatch(expectedValues.html);
+ expect(chunk.consoleReplayScript).toBe(expectedValues.consoleReplayScript);
+ expect(chunk.hasErrors).toBe(expectedValues.hasErrors);
+ expect(chunk.isShellReady).toBe(expectedValues.isShellReady);
+};
// Usage in tests:
-expect(chunks[0].html).toContain('Header In The Shell');
-expect(chunks[0].consoleReplayScript).toBe('');
-expect(chunks[0].hasErrors).toBe(false);
-expect(chunks[0].isShellReady).toBe(true);
+expectChunk(chunks[0], {
+ html: expect.stringContaining('Header In The Shell'),
+ consoleReplayScript: '',
+ hasErrors: false,
+ isShellReady: true,
+});Committable suggestion was skipped due to low confidence.
| ReactDOMServer.renderToPipeableStream(reactRenderingResult).pipe(transformStream); | ||
|
|
||
| renderResult = transformStream; | ||
| return streamRenderReactComponen(reactRenderingResult, options); |
There was a problem hiding this comment.
Update function call to match corrected function name
After correcting the function name, update the function call accordingly to maintain consistency.
Apply this diff:
- return streamRenderReactComponen(reactRenderingResult, options);
+ return streamRenderReactComponent(reactRenderingResult, options);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return streamRenderReactComponen(reactRenderingResult, options); | |
| return streamRenderReactComponent(reactRenderingResult, options); |
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)
641-641: Fix grammatical error in comment.The comment has a grammatical error.
- # It doesn't make any transformation, it listens and raises error if a chunk has errors + # It doesn't make any transformation, it listens and raises an error if a chunk has errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/react_on_rails/helper.rb(2 hunks)
🔇 Additional comments (1)
lib/react_on_rails/helper.rb (1)
573-581: LGTM! Well-structured error handling.
The method effectively centralizes error handling logic and includes all necessary debugging information (sanitized props, console messages) in the error object.
| def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | ||
| chunk_json_result["hasErrors"] && | ||
| ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) || | ||
| (render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"])) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving readability of error condition checks.
The boolean logic could be more readable by extracting conditions into well-named variables.
Consider this refactor:
- def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
- chunk_json_result["hasErrors"] &&
- ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) ||
- (render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"]))
- end
+ def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
+ has_errors = chunk_json_result["hasErrors"]
+ is_shell_ready = chunk_json_result["isShellReady"]
+
+ raise_non_shell_error = render_options.raise_on_prerender_error && !is_shell_ready
+ raise_shell_error = render_options.raise_non_shell_server_rendering_errors && is_shell_ready
+
+ has_errors && (raise_non_shell_error || raise_shell_error)
+ 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 should_raise_streaming_prerender_error?(chunk_json_result, render_options) | |
| chunk_json_result["hasErrors"] && | |
| ((render_options.raise_on_prerender_error && !chunk_json_result["isShellReady"]) || | |
| (render_options.raise_non_shell_server_rendering_errors && chunk_json_result["isShellReady"])) | |
| end | |
| def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | |
| has_errors = chunk_json_result["hasErrors"] | |
| is_shell_ready = chunk_json_result["isShellReady"] | |
| raise_non_shell_error = render_options.raise_on_prerender_error && !is_shell_ready | |
| raise_shell_error = render_options.raise_non_shell_server_rendering_errors && is_shell_ready | |
| has_errors && (raise_non_shell_error || raise_shell_error) | |
| end |
Co-authored-by: Alexey Romanov <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
lib/react_on_rails/helper.rb(2 hunks)
🧰 Additional context used
🪛 rubocop
lib/react_on_rails/helper.rb
[convention] 585-585: Line is too long. [142/120]
(Layout/LineLength)
🔇 Additional comments (1)
lib/react_on_rails/helper.rb (1)
573-581: Well-structured error handling implementation!
The method effectively encapsulates error raising logic with proper sanitization of props and inclusion of console messages for debugging.
| if render_options.stream? | ||
| result.transform do |chunk_json_result| | ||
| if should_raise_streaming_prerender_error?(chunk_json_result, render_options) | ||
| raise_prerender_error(chunk_json_result, react_component_name, props, js_code) | ||
| end | ||
| # It doesn't make any transformation, it listens and raises error if a chunk has errors | ||
| chunk_json_result | ||
| end | ||
| elsif result["hasErrors"] && render_options.raise_on_prerender_error |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve clarity of streaming error handling.
The current implementation uses a transform block for error checking, which might be misleading since no actual transformation occurs.
Consider this refactor to make the intent clearer:
if render_options.stream?
- result.transform do |chunk_json_result|
- if should_raise_streaming_prerender_error?(chunk_json_result, render_options)
- raise_prerender_error(chunk_json_result, react_component_name, props, js_code)
- end
- # It doesn't make any transformation, it listens and raises error if a chunk has errors
- chunk_json_result
- end
+ result.each_chunk do |chunk_json_result|
+ validate_streaming_chunk(
+ chunk_json_result,
+ render_options,
+ react_component_name,
+ props,
+ js_code
+ )
+ chunk_json_result
+ endAdd this private method:
def validate_streaming_chunk(chunk_json_result, render_options, react_component_name, props, js_code)
if should_raise_streaming_prerender_error?(chunk_json_result, render_options)
raise_prerender_error(chunk_json_result, react_component_name, props, js_code)
end
endThis refactor:
- Uses a more appropriate method name (
each_chunkinstead oftransform) - Extracts validation logic into a dedicated method
- Makes the error checking purpose more explicit
| def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | ||
| chunk_json_result["hasErrors"] && | ||
| (chunk_json_result["isShellReady"] ? render_options.raise_non_shell_server_rendering_errors : render_options.raise_on_prerender_error) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability of error condition check.
The current implementation, while functional, could be more readable by breaking down the complex conditional logic.
Consider this refactor for better readability:
- def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
- chunk_json_result["hasErrors"] &&
- (chunk_json_result["isShellReady"] ? render_options.raise_non_shell_server_rendering_errors : render_options.raise_on_prerender_error)
- end
+ def should_raise_streaming_prerender_error?(chunk_json_result, render_options)
+ return false unless chunk_json_result["hasErrors"]
+
+ if chunk_json_result["isShellReady"]
+ render_options.raise_non_shell_server_rendering_errors
+ else
+ render_options.raise_on_prerender_error
+ 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.
| def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | |
| chunk_json_result["hasErrors"] && | |
| (chunk_json_result["isShellReady"] ? render_options.raise_non_shell_server_rendering_errors : render_options.raise_on_prerender_error) | |
| end | |
| def should_raise_streaming_prerender_error?(chunk_json_result, render_options) | |
| return false unless chunk_json_result["hasErrors"] | |
| if chunk_json_result["isShellReady"] | |
| render_options.raise_non_shell_server_rendering_errors | |
| else | |
| render_options.raise_on_prerender_error | |
| end | |
| end |
🧰 Tools
🪛 rubocop
[convention] 585-585: Line is too long. [142/120]
(Layout/LineLength)
3 participants
Summary
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
ReactOnRails.registerStoremethod, renaming it toregisterStoreGeneratorsfor clarity.Tests
streamServerRenderedReactComponentfunction to validate streaming behavior and error handling.Chores