Skip to content

Conversation

@AbanoubGhadban
Copy link
Collaborator

@AbanoubGhadban AbanoubGhadban commented Nov 12, 2025

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/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

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 Reviewable

Summary by CodeRabbit

  • Refactor

    • Simplified HTTP request handling to a single connection with retries always enabled and unified timeout behavior.
  • New Features

    • Streaming pipeline can register rescue handlers to capture, transform, or emit values when streaming errors occur.
  • Bug Fixes

    • Streaming errors now surface consistently as prerender errors; streaming timeout handling and body-duplication edge cases addressed.
  • Tests / Chores

    • Updated tests and helpers for streaming rescue behavior, removed an obsolete example component, and updated changelog entries.

@claude
Copy link

claude bot commented Nov 12, 2025

Pull Request Review

Hi @AbanoubGhadban! I've reviewed PR #1995 "Improve Streamed Components retry logic".

Current Status

This PR currently has:

  • 0 additions / 0 deletions
  • No files changed
  • The PR description is still using the template placeholder text

Required Actions Before Review

  1. Push your changes: The branch abanoub/improve-streamed-components-retry-logic appears to have no commits yet. Please push your implementation.

  2. Complete PR Description: Replace the template text with:

    • A clear summary of what retry logic improvements are being made
    • The specific problem this solves (e.g., which scenarios were failing?)
    • How the new retry logic differs from the old approach
    • Any relevant issue numbers
  3. Complete PR Checklist:

    • Add/update tests for the retry logic
    • Update documentation (if retry behavior changes affect users)
    • Update CHANGELOG.md (see /update-changelog command or grep -A 3 "^#### " CHANGELOG.md | head -30 for examples)

Review Criteria (Once Code is Pushed)

When code is available, I'll review for:

Code Quality:

  • Retry logic implementation (exponential backoff? max retries? jitter?)
  • Error handling and edge cases
  • Code readability and maintainability

Testing:

  • Unit tests for retry scenarios
  • Tests for max retry limits
  • Tests for different failure modes
  • Integration tests if applicable

Performance:

  • Resource cleanup between retries
  • Memory leak prevention
  • Impact on server load with multiple retries

Documentation:

  • Clear explanation of retry behavior
  • Configuration options (if any)
  • Migration notes if behavior changes

Repository Guidelines

Per CLAUDE.md, remember to:

  • Run bundle exec rubocop before committing (git hooks should handle this automatically)
  • Ensure all files end with newlines
  • Use rake autofix for formatting
  • Run relevant tests locally before pushing

Please push your changes and I'll provide a detailed code review! 🚀

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

Walkthrough

Consolidates 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

Cohort / File(s) Change Summary
Request core refactor
react_on_rails_pro/lib/react_on_rails_pro/request.rb
Removed connection_without_retries and branching for streaming. create_connection no longer accepts enable_retries: and always configures HTTPX retries (plugin :retries). reset_connection closes only the main connection. perform_request always uses the single connection and relies on HTTPX retries with a retry_after handler and explicit timeout handling.
Streaming decorator & request
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
Added @rescue_blocks and rescue(&block) to StreamDecorator; changed each_chunk to accept an explicit block and call registered rescue blocks on StandardError. StreamRequest#each_chunk now rescues HTTPX::ReadTimeoutError and raises a formatted ReactOnRailsPro::Error; marks response once first chunk received and ensures final last_chunk is processed.
Helper: prerender streaming errors
lib/react_on_rails/helper.rb
Added a rescue path for streaming renderer exceptions to raise ReactOnRails::PrerenderError with component name, sanitized props, original error, and js_code context (mirrors non-streaming error path).
Tests: spec updates & removed test
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb, react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb, react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
Added comprehensive tests for StreamDecorator#rescue. Removed a test that asserted creation/use of connection_without_retries. Adjusted mock setup to stop resetting @connection_without_retries when preparing mocks.
Mocks helper change
react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb
mock_block signature changed from return_value: to accepting a block (&block); the mocked :call now delegates to the provided block for dynamic behavior.
Test fixture removal
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/TestingStreamableComponent.jsx
Deleted the test React component file and its default export.
Changelog
CHANGELOG.md
Added Unreleased bug-fix entries documenting the fix for body-duplication during streaming (duplicate entry appears in diff).

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas to focus on:
    • react_on_rails_pro/lib/react_on_rails_pro/request.rb — verify retry_after/retries integration doesn't cause duplicate-body or streaming-safety issues.
    • react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb — ensure rescue block chaining, final-chunk handling, and first-chunk marker are correct.
    • Tests & mocks — confirm mock_block behavioral change and removed test component align with test expectations.

Possibly related PRs

Suggested reviewers

  • justin808
  • Judahmeek
  • alexeyr-ci

Poem

🐇 I hopped through sockets, kept one faithful line,
Retries stitched in, stream handlers learn to shine,
Chunks call rescue blocks, errors caught or reborn,
A test file waved goodbye, mocks shaped for the morn,
A small rabbit cheer for a tidier pipeline 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and specifically describes the main change: improving retry logic for streamed components, which is reflected throughout the changeset with connection pooling simplification, retry plugin consolidation, and enhanced error handling in streaming.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch abanoub/improve-streamed-components-retry-logic

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80fedc3 and 51886cb.

📒 Files selected for processing (1)
  • lib/react_on_rails/helper.rb (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/react_on_rails/helper.rb
⏰ 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)
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: build
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: markdown-link-check
  • GitHub Check: claude-review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot added the bug label Nov 12, 2025
@claude
Copy link

claude bot commented Nov 12, 2025

Code Review - PR #1995: Improve Streamed Components retry logic

Overview

This 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

  1. Simplified Architecture: Removing the dual connection pool (@connection and @connection_without_retries) significantly reduces code complexity and memory footprint.

  2. Clear Intent: The PR correctly identifies that the HTTPx retry plugin was causing body duplication issues when retrying mid-stream, and the fix properly delegates retry handling to StreamRequest.

  3. Proper Error Handling: The StreamRequest#each_chunk method at line 78-89 already has robust retry logic that starts fresh requests, avoiding the duplication problem.

  4. Code Cleanup: Removed unnecessary comments and test code that are no longer relevant.


⚠️ Issues & Concerns

1. CRITICAL: Missing PR Description

The PR template placeholders are still present. Please provide:

2. CRITICAL: Missing Checklist Items

  • Tests: The PR removes a test (request_spec.rb:197-223) but doesn't add replacement tests
  • Documentation: No documentation updates
  • CHANGELOG: Not updated (required per CLAUDE.md)

3. Potential Regression: Non-Streaming Requests

The removed dual-pool approach was designed to prevent issues. Questions:

  • Have you verified that non-streaming requests still work correctly with retries enabled?
  • What happens if a non-streaming request gets a descriptor closed error mid-response?
  • The old code comment mentioned "body duplication" - is this only a concern for streaming, or could it affect non-streaming too?

Recommendation: Add integration tests for non-streaming requests with simulated connection errors.

4. Removed Test Coverage

The deleted test request_spec.rb:197-223 verified the dual-pool behavior. You should add a replacement test that:

5. Deleted Test Component

TestingStreamableComponent.jsx was removed. Was this component:

  • Only used for the deleted test?
  • Still referenced elsewhere in the codebase?

Recommendation: Verify no other tests depend on this component.

6. Performance Consideration

The old comment mentioned "doubles the memory footprint" but deemed it acceptable. Now with a single pool:

  • Pro: Halves memory usage ✅
  • Con: All requests (streaming & non-streaming) share the same pool

Have you verified this doesn't cause connection starvation when mixing many streaming and non-streaming requests?


🔍 Code-Specific Feedback

request.rb:89 - Complexity Reduction

def perform_request(path, **post_options) # rubocop:disable Metrics/AbcSize,Metrics/CyclomaticComplexity

You removed the Metrics/PerceivedComplexity cop disable - great! But consider if you can simplify further to remove the remaining two.

request.rb:95 - Retry Plugin Always Enabled

response = connection.post(path, **post_options)

Now all requests (including streaming) go through the retry plugin. The key question:

Q: Does the HTTPx retry plugin detect that it's a streaming response and avoid retrying mid-stream?
A: Based on issue #1895, it seems it does retry, which is why StreamRequest catches HTTPX::HTTPError at line 87-89 and handles retries itself.

Verify: The exception handling at request.rb:119 re-raises HTTPX::HTTPError for streaming requests, which is correct. But ensure this is tested.

stream_request.rb:78 - Retry Logic

loop do
  stream_response = @request_executor.call(send_bundle)
  process_response_chunks(stream_response, error_body, &block)
  break
rescue HTTPX::HTTPError => e
  send_bundle = handle_http_error(e, error_body, send_bundle)
end

This is the core retry logic for streaming. It's solid, but:

  • Add a test that simulates an HTTPX::HTTPError mid-stream
  • Verify that @request_executor.call(send_bundle) starts a fresh request (not a resume)
  • Ensure no chunks from the first failed request leak through

🧪 Testing Recommendations

Add these test cases:

  1. Streaming with mid-request connection error

    it "retries streaming request without body duplication on connection error" do
      # Simulate descriptor closed error mid-stream
      # Verify chunks are not duplicated
    end
  2. Non-streaming with retry

    it "successfully retries non-streaming requests on connection error" do
      # Ensure HTTPx retry plugin works for non-streaming
    end
  3. Streaming with HTTPx retry plugin interaction

    it "StreamRequest handles HTTPError and performs fresh retry" do
      # Verify StreamRequest's loop handles HTTPx errors correctly
    end

📋 Action Items

Before merging:

  1. ✏️ Fill out PR description - explain the change and reference [Important] Fix the body duplication problem when an retry happen after a connection error #1895
  2. 🧪 Add missing tests - replace the deleted test with better coverage
  3. 📝 Update CHANGELOG.md - use /update-changelog command per CLAUDE.md
  4. 🔍 Verify deleted component - ensure TestingStreamableComponent.jsx isn't used elsewhere
  5. Test under load - verify connection pool works well with mixed request types
  6. 🏃 Run full test suite - bundle exec rubocop && rake per CLAUDE.md requirements

Nice to have:

  • Add code comments explaining why streaming requests now use the retry plugin (they're handled by StreamRequest)
  • Document the retry flow in StreamRequest for future maintainers

🎯 Conclusion

Architecture: ✅ The simplified approach is sound
Implementation: ⚠️ Needs more test coverage
Documentation: ❌ Missing PR description and CHANGELOG

This is a good refactoring that simplifies the codebase, but it needs proper documentation and tests before merging. The core logic appears correct, but the lack of replacement tests for the removed functionality is concerning.

Recommendation: Request changes - add tests and documentation before approval.


Review performed following CLAUDE.md guidelines. If you have questions about any feedback, please ask!

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2d94bc4 and d867965.

📒 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_retries cleanup 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.post with 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 StreamRequest for error handling)
  • TimeoutError during streaming: Falls through the HTTPError check, allowing the application retry loop (lines 90–116) to retry again

This means TimeoutError during 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.

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review - PR #1995: Improve Streamed Components retry logic

Summary

This 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

  1. Simplified Architecture: Removing the dual connection pool approach (@connection and @connection_without_retries) significantly simplifies the codebase and reduces memory footprint (previously using 2x connections).

  2. Code Cleanup: Proper cleanup of:

    • Removed connection_without_retries method and instance variable
    • Removed outdated comments about body duplication prevention strategy
    • Removed test component that's no longer needed
    • Removed test that was testing the old implementation
  3. Comment Updates: Updated inline comments to reflect the new unified approach.


⚠️ Critical Concerns

1. Missing Critical Information in PR Description

The PR body is still using the template placeholder text. It should explain:

Action Required: Update the PR description before merging.

2. Test Coverage Gap - THE MOST IMPORTANT ISSUE

The deleted test at line 198-223 in request_spec.rb was specifically verifying the fix for issue #1895. Removing this test without replacing it is dangerous because:

  • It removes verification that body duplication doesn't occur
  • Future changes could reintroduce the bug without detection
  • The test title explicitly references the GitHub issue

Recommendation:

  • Add a new test that verifies the unified connection approach handles streaming correctly without body duplication
  • Consider an integration test that simulates a connection error during streaming and verifies no duplication occurs
  • Document why the old test is no longer applicable

Example test structure:

it "handles retries without body duplication using unified connection" do
  # Test that simulates a connection error during streaming
  # and verifies chunks are not duplicated
end

3. Potential Body Duplication Risk

The original issue #1895 described how HTTPX retries cause body duplication when:

  1. Streaming starts and sends chunks
  2. Connection error occurs mid-stream
  3. HTTPX retries plugin retries the request
  4. Already-sent chunks get duplicated

The PR doesn't explain how this is resolved by the change. Questions:

  • Does HTTPX's retry plugin now handle streaming differently?
  • Has the StreamRequest class retry logic been updated to prevent this?
  • What prevents the same issue from occurring with the unified connection?

Looking at stream_request.rb:78-81, the comments still reference the old approach:

# Retry logic for streaming requests is handled here by starting fresh requests.
# The HTTPx connection used for streaming has retries disabled (see Request#connection_without_retries)
# to prevent body duplication when partial chunks are already sent to the client.

This comment is now outdated since connection_without_retries no longer exists.

Action Required:

  • Update or remove the outdated comment in stream_request.rb
  • Explain in the PR description how body duplication is prevented with the new approach

4. Potential Race Condition

In reset_connection (line 10-13 of the diff), the code now only closes @connection:

def reset_connection
  @connection&.close
  @connection = create_connection
end

This is correct for the new approach, but ensure all callers of reset_connection don't rely on the old dual-connection behavior.


📋 Checklist Items

According to the PR template, these items need attention:

  • Add/update test to cover these changes - REQUIRED: Replace the deleted test
  • Update documentation - Consider updating any docs that mention the dual connection approach
  • Update CHANGELOG file - This is a significant bug fix and should be documented

According to CLAUDE.md:

  • CHANGELOG format: [PR 1995](https://github.com/shakacode/react_on_rails/pull/1995) by [AbanoubGhadban](https://github.com/AbanoubGhadban)
  • This qualifies as a user-visible change (bug fix for streaming component body duplication)

🔍 Code Quality

Positive:

  • RuboCop complexity metrics improved (removed Metrics/PerceivedComplexity)
  • Code is cleaner and more maintainable
  • Proper nil-safe operator usage (@connection&.close)

Minor Issues:

  • The outdated comment in stream_request.rb (lines 78-80) must be updated/removed

🚀 Performance Considerations

Improvement: Halving the connection pool size (from 2x to 1x) is a significant memory improvement, especially in high-load scenarios.

Question: Has there been any performance testing to ensure the retry behavior doesn't negatively impact streaming performance?


🔐 Security Considerations

No security concerns identified. The change is internal to connection management and doesn't affect data handling or validation.


📝 Recommendations Before Merge

  1. CRITICAL: Add test coverage to verify no body duplication occurs with the unified connection approach
  2. REQUIRED: Update PR description to explain the fix properly
  3. REQUIRED: Remove/update outdated comment in stream_request.rb:78-80
  4. REQUIRED: Update CHANGELOG.md with this bug fix
  5. NICE TO HAVE: Add integration test that simulates connection errors during streaming
  6. NICE TO HAVE: Document the testing approach used to verify the fix

🎯 Overall Assessment

This is a promising simplification that addresses a real issue, but it needs:

  • Better documentation/explanation of how the fix works
  • Test coverage to prevent regression
  • Comment cleanup to match the new implementation

The code change itself looks reasonable, but without proper testing and documentation, it's risky to merge.

Recommendation: Request changes - specifically the test coverage and PR description updates before merging.

@claude
Copy link

claude bot commented Nov 13, 2025

Pull Request Review

Thank you for working on this important fix! I've reviewed the changes and have the following feedback:

Strengths

  1. Significant Simplification: Removing the dual connection pool architecture (@connection and @connection_without_retries) is a major improvement that:

    • Reduces memory footprint by 50% (from 20 to 10 connections with default pool size)
    • Eliminates code complexity around managing two separate connection pools
    • Makes the codebase more maintainable
  2. Cleaner Approach: The solution is elegant - instead of disabling retries at the HTTP layer for streaming requests, you're leveraging the existing retry logic in StreamRequest class which already handles fresh requests properly.

  3. Well-Targeted Fix: Addresses issue [Important] Fix the body duplication problem when an retry happen after a connection error #1895 about body duplication during streaming retries.

⚠️ Critical Issues

1. Missing CHANGELOG Entry

According to CLAUDE.md, you must update the CHANGELOG for user-visible changes. This is a significant bug fix that users should know about. The PR checklist even has this item unchecked.

Action Required: Add a CHANGELOG entry under the Fixed section (create it if it doesn't exist under Unreleased). Use the /update-changelog command for guided assistance.

Suggested entry:

  • Streamed Components Retry Logic: Fixed body duplication issue when streaming requests encounter connection errors and are retried. Previously, HTTPx retries would append duplicate chunks to the response. Now uses a single unified connection pool with retries, as the StreamRequest class already handles retries properly by starting fresh requests. This reduces memory footprint by 50% while improving reliability. PR 1995 by AbanoubGhadban.

2. Incomplete PR Template

The PR description still contains template placeholders and all checklist items are unchecked. Please provide a clear summary of the changes and explain the fix for issue #1895.

3. Test Coverage Gap

The removed test at request_spec.rb:198-223 was specifically verifying that streaming requests don't use the retries plugin. With your changes, ALL requests now use retries.

Concern: How do we verify that streaming retries won't cause body duplication? The StreamRequest class has retry logic, but there's no test confirming that when a streaming request fails mid-transmission and StreamRequest retries it, the body is NOT duplicated.

Recommendation: Add a new test that simulates a mid-stream failure and verifies no duplication occurs.

🤔 Questions and Considerations

1. HTTPx Retry Behavior with Streams

The original code disabled retries because retries cause body duplication (line 244-245 in old code). Your change now enables retries for streaming requests.

Question: Does HTTPx's retry plugin know when a streaming request has partially transmitted? If HTTPx retries a streaming request that already sent chunks, won't those chunks be duplicated?

Analysis: Looking at stream_request.rb:78-81, the comments suggest that retry logic is handled by StreamRequest starting fresh requests. But if HTTPx ALSO retries (due to the :retries plugin), could we get BOTH HTTPx retrying the partially-sent request (causing duplication) AND StreamRequest retrying with a fresh request (correct behavior)?

Recommendation: Verify that HTTPx's retry plugin doesn't interfere with streaming. Consider testing with artificial mid-stream failures.

2. Error Handling Flow

In perform_request (request.rb:133 after changes), there's a line that re-raises HTTP errors for streaming requests so StreamRequest can handle them. But with HTTPx retries enabled, will this line even be reached, or will HTTPx retry first?

Concern: If HTTPx retries happen BEFORE this error is raised to StreamRequest, then StreamRequest never gets to handle the retry with a fresh request.

3. Connection Timeout vs HTTP Errors

The retry logic in perform_request handles HTTPX::TimeoutError but delegates HTTPX::HTTPError to StreamRequest. With retries enabled at the HTTPx level, which layer handles which errors for streaming requests?

📝 Minor Issues

  1. Deleted Test Component: TestingStreamableComponent.jsx was removed. Was this used anywhere? If it was only for the deleted test, that's fine, but verify it's not referenced elsewhere.

  2. Comment Clarity: The comments in stream_request.rb:78-80 now reference connection_without_retries which no longer exists. These should be updated or removed.

🎯 Summary

This PR has the right idea - simplifying the architecture is good. However, I have concerns about whether HTTPx retries could still cause body duplication for streaming requests. The key question is: Do HTTPx's automatic retries happen before or after StreamRequest gets a chance to handle errors?

Recommended Actions:

  1. Add CHANGELOG entry (REQUIRED per CLAUDE.md)
  2. Complete PR template
  3. Add test coverage for mid-stream retry scenarios
  4. Clarify/verify the interaction between HTTPx retries and StreamRequest retry logic
  5. Update outdated comments in stream_request.rb

Verdict: Needs work before merging. The simplification is valuable, but we need confidence that it doesn't reintroduce the body duplication bug from #1895.


Review generated by Claude Code following the repository CLAUDE.md guidelines

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_after callback checks wrong object and lacks nil safety—fix object identity and add defensive guard

The instance variable @react_on_rails_received_first_chunk is set on the response object (stream_request.rb:157) but checked on request.stream (request.rb:239). In the HTTPX API, these are separate objects; request.stream is the stream callback handler, not the response. Additionally, there is no nil check on request.stream, which will crash with a NoMethodError if 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.error

Also 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: guard request.stream, align with first‑chunk marker, and re‑check retryability of streaming POSTs

The 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:

  1. Possible NoMethodError on non‑stream requests

retry_after assumes request.stream is present:

if (request.stream.instance_variable_get(:@react_on_rails_received_first_chunk))

For non‑streaming requests, request.stream may be nil or a different object, which would blow up inside the retry plugin instead of cleanly handling the original error.

  1. Sync with the marker set in StreamRequest#loop_response_lines

The first‑chunk flag is currently set on the streaming response object, while retry_after inspects request.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.

  1. Retrying change requests with multipart/file bodies

retry_change_requests: true combined with non‑rewindable bodies (e.g., Pathname from get_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_after implementation 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 NoMethodError when there’s no stream object.
  • Ensures the same flag used in StreamRequest is 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.stream points to for stream: true requests.
  • 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_error method 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
  )
end

Then 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_blockfirst_rescue_block for readability.

Functionally, this looks solid.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d867965 and a379c0a.

📒 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.rb
  • react_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.rb
  • react_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.rb
  • react_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 good

Forwarding 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 appropriate

Initializing @rescue_blocks alongside @actions keeps the decorator’s state explicit and avoids accidental nil handling when registering rescue handlers.


43-46: Rescue registration API is straightforward and chainable

Storing rescue handlers as procs and returning self from rescue(&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 in each_chunk matches the intended semantics

The updated each_chunk does the right things:

  • Behaves as an enumerator when no block is given.
  • Applies prepend/transform/append via handle_chunk before yielding each chunk.
  • On any StandardError, walks @rescue_blocks in 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_error preserves 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::Error for consistent error handling.

Regarding retry logic: The structure shows HTTPX::ReadTimeoutError as a separate rescue clause (line 108) distinct from HTTPX::HTTPError (line 102). The HTTPError rescue handles retries via handle_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 in perform_request keeps behavior coherent for streaming and non‑streaming

Switching to a single connection.post path for both regular and streaming requests simplifies connection management while preserving:

  • Timeout retry behavior via the outer loop on HTTPX::TimeoutError.
  • Delegation of HTTPX::HTTPError handling for streaming (post_options[:stream]) to ReactOnRailsPro::StreamRequest.
  • Consistent wrapping of non‑streaming HTTPX errors as ReactOnRailsPro::Error.

This looks correct given the surrounding streaming logic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between a379c0a and 300cedd.

📒 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 transform block (lines 583-589) handles errors reported in chunk metadata (hasErrors)
  • The rescue block (lines 591-598) catches exceptions during streaming iteration

This provides comprehensive error coverage for streaming scenarios.

Comment on lines 591 to 598
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
Copy link
Contributor

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
  )
end

Then 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.

@AbanoubGhadban AbanoubGhadban merged commit 12151f9 into master Nov 15, 2025
19 of 20 checks passed
@AbanoubGhadban AbanoubGhadban deleted the abanoub/improve-streamed-components-retry-logic branch November 15, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants