-
-
Notifications
You must be signed in to change notification settings - Fork 638
Improve Streamed Components retry logic #1995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Streamed Components retry logic #1995
Conversation
Pull Request ReviewHi @AbanoubGhadban! I've reviewed PR #1995 "Improve Streamed Components retry logic". Current StatusThis PR currently has:
Required Actions Before Review
Review Criteria (Once Code is Pushed)When code is available, I'll review for: Code Quality:
Testing:
Performance:
Documentation:
Repository GuidelinesPer CLAUDE.md, remember to:
Please push your changes and I'll provide a detailed code review! 🚀 |
WalkthroughConsolidates HTTPX connections to a single connection with retries; removes the no-retries pool and related tests; adds streaming rescue infrastructure in StreamDecorator; updates streaming error propagation in helper; changes mock/test helpers; and deletes a test React component file. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App code
participant Request as ReactOnRailsPro::Request
participant HTTPX as HTTPX (single connection + retries)
participant Server as Rendering server
participant Stream as StreamDecorator
App->>Request: perform_request(payload, streaming?)
Note right of Request `#DDEBF7`: create_connection (plugin :retries configured)
Request->>HTTPX: post(payload)
HTTPX->>Server: HTTP POST
Server-->>HTTPX: response (streamed chunks or full body)
HTTPX-->>Request: response
alt streaming response
Request->>Stream: each_chunk { block.call(chunk) }
Stream->>Stream: on StandardError -> invoke registered rescue_blocks
opt rescue yields
Stream->>App: yield transformed/added chunks
end
else non-streaming
Request->>App: return body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #1995: Improve Streamed Components retry logicOverviewThis PR simplifies the HTTP connection management by removing the dual connection pool approach and consolidating retry logic. The change addresses issue #1895 regarding body duplication in streaming responses. ✅ Strengths
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
react_on_rails_pro/lib/react_on_rails_pro/request.rb(2 hunks)react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb(0 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx(0 hunks)react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb(0 hunks)react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb(0 hunks)
💤 Files with no reviewable changes (4)
- react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
- react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
- react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx
- react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb
🧰 Additional context used
🧬 Code graph analysis (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)
configuration(9-37)
🔇 Additional comments (2)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (2)
10-13: LGTM: Connection reset simplified.The consolidation to a single connection is correctly reflected here. The removal of
@connection_without_retriescleanup is consistent with the architectural change.
89-95: Verify the retry interaction between HTTPX and application layers for TimeoutError scenarios.The consolidation to a single connection means all requests now use
connection.postwith HTTPX retries enabled (max_retries: 1, retry_change_requests: true). For streaming requests, this creates a compound retry pattern:
- HTTPError during streaming: Re-raised immediately at line 119 (intentional—delegated to
StreamRequestfor error handling)- TimeoutError during streaming: Falls through the HTTPError check, allowing the application retry loop (lines 90–116) to retry again
This means
TimeoutErrorduring streaming could be retried twice: once by HTTPX and again by the application-level retry loop. Confirm this is the intended behavior. File bodies (Pathname objects) are rewindable, so body duplication during HTTPX retries is safe.
Code Review - PR #1995: Improve Streamed Components retry logicSummaryThis PR simplifies the HTTP connection management by removing the dual-connection approach (one with retries, one without) and consolidating to a single connection pool with retries enabled for all requests. This addresses issue #1895 regarding body duplication in streaming responses. ✅ Positive Changes
|
Pull Request ReviewThank you for working on this important fix! I've reviewed the changes and have the following feedback: ✅ Strengths
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (1)
157-167:retry_aftercallback checks wrong object and lacks nil safety—fix object identity and add defensive guardThe instance variable
@react_on_rails_received_first_chunkis set on theresponseobject (stream_request.rb:157) but checked onrequest.stream(request.rb:239). In the HTTPX API, these are separate objects;request.streamis the stream callback handler, not the response. Additionally, there is no nil check onrequest.stream, which will crash with aNoMethodErrorif the request is not a streaming request.Without the fix, the first-chunk detection won't work, and HTTPX may still retry mid-stream, causing duplicated SSR HTML.
Update request.rb line 238-242 to:
- retry_after: ->(request, response) do - if (request.stream.instance_variable_get(:@react_on_rails_received_first_chunk)) + retry_after: ->(request, response) do + stream = request.stream || response + if stream&.instance_variable_get(:@react_on_rails_received_first_chunk) e = response.errorAlso ensure stream_request.rb continues setting the marker on the response object (already correct at line 157).
♻️ Duplicate comments (1)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
220-254: HTTPX retry configuration: guardrequest.stream, align with first‑chunk marker, and re‑check retryability of streaming POSTsThe new HTTPX connection setup is heading in the right direction (single pool, retries handled at the client), but there are a few sharp edges here:
- Possible
NoMethodErroron non‑stream requests
retry_afterassumesrequest.streamis present:if (request.stream.instance_variable_get(:@react_on_rails_received_first_chunk))For non‑streaming requests,
request.streammay benilor a different object, which would blow up inside the retry plugin instead of cleanly handling the original error.
- Sync with the marker set in
StreamRequest#loop_response_linesThe first‑chunk flag is currently set on the streaming
responseobject, whileretry_afterinspectsrequest.stream. Please ensure they’re actually the same object (or share that ivar); otherwise this condition may never become true and HTTPX might still retry mid‑stream, causing duplicated SSR HTML when some chunks have already been sent. This directly relates to the concern previously raised about avoiding retries once bytes have gone over the wire for streamed renders.
- Retrying change requests with multipart/file bodies
retry_change_requests: truecombined with non‑rewindable bodies (e.g.,Pathnamefromget_form_body_for_file) means HTTPX’s retries may not be able to safely resend the request body for streaming POSTs. The earlier review already highlighted this risk; please either ensure all such POST bodies are rewindable or document that streaming requests with file bodies might not be retried.A more defensive
retry_afterimplementation could look like:- .plugin( - :retries, max_retries: 1, - retry_change_requests: true, - retry_after: ->(request, response) do - if (request.stream.instance_variable_get(:@react_on_rails_received_first_chunk)) + .plugin( + :retries, max_retries: 1, + retry_change_requests: true, + retry_after: ->(request, response) do + stream = request.stream || response + if stream && stream.instance_variable_get(:@react_on_rails_received_first_chunk) e = response.error raise ReactOnRailsPro::Error, "An error happened during server side render streaming of a component.\n" \ "Original error:\n#{e}\n#{e.backtrace}" end - Rails.logger.info do - "[ReactOnRailsPro] An error happneding while making a request to the Node Renderer.\n" \ + "[ReactOnRailsPro] An error happening while making a request to the Node Renderer.\n" \ "Error: #{response.error}.\n" \ "Retrying by HTTPX \"retries\" plugin..." end nil end )This:
- Avoids
NoMethodErrorwhen there’s no stream object.- Ensures the same flag used in
StreamRequestis visible to the retry logic.- Fixes the small typo in the log message (“happneding”).
To double‑check HTTPX behavior, you can consult the HTTPX retries plugin docs and confirm:
- What
request.streampoints to forstream: truerequests.- How retries behave for non‑rewindable multipart bodies.
🧹 Nitpick comments (2)
lib/react_on_rails/helper.rb (1)
591-598: Good addition for streaming error handling.The rescue block correctly handles renderer-level exceptions during streaming, providing symmetry with the non-streaming rescue (lines 573-579). This complements the transform block (lines 583-589) which handles errors reported in chunk JSON.
Consider extracting the duplicated error-raising logic into a helper method, as similar code appears in three places (lines 575-579, 593-597, and the
raise_prerender_errormethod at lines 516-524):def create_prerender_error(react_component_name, props, js_code, err: nil, console_messages: nil) ReactOnRails::PrerenderError.new( component_name: react_component_name, props: sanitized_props_string(props), err: err, js_code: js_code, console_messages: console_messages ) endThen use it consistently across all three locations.
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb (1)
66-153: Excellent coverage of streaming rescue behavior (minor naming nits only)These examples exercise all the important paths for the new rescue mechanism: initial error, mid‑stream error, yielding from rescue, error conversion, and chained rescues. They align well with the implementation in
ReactOnRailsPro::StreamDecorator#each_chunk.Only very minor nits:
- Descriptions like
"catches the error happens inside ..."could be tweaked for grammar.fist_rescue_block→first_rescue_blockfor readability.Functionally, this looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/react_on_rails/helper.rb(1 hunks)react_on_rails_pro/lib/react_on_rails_pro/request.rb(2 hunks)react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb(4 hunks)react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb(1 hunks)react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
lib/react_on_rails/helper.rbreact_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/helper.rbreact_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/helper.rbreact_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
🧬 Code graph analysis (3)
lib/react_on_rails/helper.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (1)
rescue(43-46)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)
configuration(9-37)
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb (2)
react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb (1)
mock_block(12-21)react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (3)
rescue(43-46)each_chunk(54-82)each_chunk(92-112)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/helper.rb
[error] 590-590: RuboCop: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 595-595: RuboCop: Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[error] 595-595: RuboCop: Layout/HashAlignment: Align the keys of a hash literal if they span more than one line.
[error] 596-596: RuboCop: Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[error] 596-596: RuboCop: Layout/HashAlignment: Align the keys of a hash literal if they span more than one line.
[error] 597-597: RuboCop: Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[error] 597-597: RuboCop: Layout/HashAlignment: Align the keys of a hash literal if they span more than one line.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (6)
react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb (1)
12-16: Block‑aware mock helper looks goodForwarding both arguments and the inner block into the provided block is exactly what the new streaming/rescue specs need, and keeps
have_received(:call)expectations working. No issues from my side.react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (4)
13-14: Rescue blocks registry initialization is appropriateInitializing
@rescue_blocksalongside@actionskeeps the decorator’s state explicit and avoids accidentalnilhandling when registering rescue handlers.
43-46: Rescue registration API is straightforward and chainableStoring rescue handlers as procs and returning
selffromrescue(&block)fits the fluent API used in the specs (stream_decorator.rescue(...).append(...)). The semantics are clear and match the tests.
54-82: Rescue-chain handling ineach_chunkmatches the intended semanticsThe updated
each_chunkdoes the right things:
- Behaves as an enumerator when no block is given.
- Applies prepend/transform/append via
handle_chunkbefore yielding each chunk.- On any
StandardError, walks@rescue_blocksin order, allowing:
- Rescue blocks to handle the error and optionally yield extra chunks via the original
&block.- Rescue blocks to transform the error and hand it off to the next handler.
- If no handler clears the error, the final
raise current_errorpreserves failure visibility.This aligns with the new specs for error conversion and chained rescues.
108-110: No issues found—the code correctly handles streaming read timeouts.HTTPX::ReadTimeoutError is the correct exception class for read timeout scenarios with streaming. The rescue block at lines 108–110 properly wraps this in a
ReactOnRailsPro::Errorfor consistent error handling.Regarding retry logic: The structure shows
HTTPX::ReadTimeoutErroras a separate rescue clause (line 108) distinct fromHTTPX::HTTPError(line 102). The HTTPError rescue handles retries viahandle_http_error; the ReadTimeoutError rescue intentionally raises without retry (fail-fast behavior). No conflict exists.react_on_rails_pro/lib/react_on_rails_pro/request.rb (1)
89-123: Single connection use inperform_requestkeeps behavior coherent for streaming and non‑streamingSwitching to a single
connection.postpath for both regular and streaming requests simplifies connection management while preserving:
- Timeout retry behavior via the outer loop on
HTTPX::TimeoutError.- Delegation of
HTTPX::HTTPErrorhandling for streaming (post_options[:stream]) toReactOnRailsPro::StreamRequest.- Consistent wrapping of non‑streaming HTTPX errors as
ReactOnRailsPro::Error.This looks correct given the surrounding streaming logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/react_on_rails/helper.rb (1)
590-598: RuboCop violations flagged in previous review.The linting errors on line 590 (trailing whitespace) and lines 595-597 (argument/hash alignment) were already identified in a previous review and must be addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/react_on_rails/helper.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/helper.rb
🧬 Code graph analysis (1)
lib/react_on_rails/helper.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (1)
rescue(43-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (1)
lib/react_on_rails/helper.rb (1)
582-598: Streaming error handling looks good.The separation of concerns is appropriate:
- The
transformblock (lines 583-589) handles errors reported in chunk metadata (hasErrors)- The
rescueblock (lines 591-598) catches exceptions during streaming iterationThis provides comprehensive error coverage for streaming scenarios.
| result.rescue do |err| | ||
| # This error came from the renderer | ||
| raise ReactOnRails::PrerenderError.new(component_name: react_component_name, | ||
| # Sanitize as this might be browser logged | ||
| props: sanitized_props_string(props), | ||
| err: err, | ||
| js_code: js_code) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Consider extracting duplicate error-raising logic.
The error-raising code in this rescue block (lines 591-598) duplicates the rescue block at lines 573-580. Both construct identical ReactOnRails::PrerenderError objects with the same parameters.
Extract the error construction into a helper method:
def build_prerender_error(react_component_name, props, err, js_code)
ReactOnRails::PrerenderError.new(
component_name: react_component_name,
# Sanitize as this might be browser logged
props: sanitized_props_string(props),
err: err,
js_code: js_code
)
endThen use it in both places:
rescue StandardError => err
# This error came from the renderer
- raise ReactOnRails::PrerenderError.new(component_name: react_component_name,
- # Sanitize as this might be browser logged
- props: sanitized_props_string(props),
- err: err,
- js_code: js_code)
+ raise build_prerender_error(react_component_name, props, err, js_code)
end
if render_options.streaming?
# ... transform block ...
result.rescue do |err|
# This error came from the renderer
- raise ReactOnRails::PrerenderError.new(component_name: react_component_name,
- # Sanitize as this might be browser logged
- props: sanitized_props_string(props),
- err: err,
- js_code: js_code)
+ raise build_prerender_error(react_component_name, props, err, js_code)
end🤖 Prompt for AI Agents
In lib/react_on_rails/helper.rb around lines 573-580 and 591-598, the rescue
blocks duplicate construction of a ReactOnRails::PrerenderError object; extract
that construction into a private helper method (e.g. build_prerender_error) that
accepts react_component_name, props, err, and js_code, returns the new
ReactOnRails::PrerenderError with sanitized props, and replace both rescue
blocks to call this helper to raise the error instead of inlining the object
creation.
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
Refactor
New Features
Bug Fixes
Tests / Chores