Concurrently drain streaming fibers

[ihabadham]

Implements #550

• Concurrent draining of streamed React components (Async tasks + single writer)
• Default backpressure via Async::Semaphore; handle client disconnects
• Specs for sequential vs concurrent, per-component ordering, edge cases, and backpressure

Example:

• With two stream components fibers, chunks interleave (first-ready wins) while each component's own chunk order is preserved.

Summary by CodeRabbit

• Improvements

  • Concurrent streaming for React components replaces sequential processing, reducing latency for stream rendering.

• Added

  • New configuration option concurrent_component_streaming_buffer_size (default: 64) to tune memory vs. performance.
  • Runtime dependency on the async gem (>= 2.6) to enable concurrent streaming.

• Tests

  • Expanded test coverage for concurrent component streaming, ordering, and backpressure scenarios.

---

Migrated from: shakacode/react_on_rails_pro#552

---

This change is 

[coderabbitai]

Walkthrough

This PR adds concurrent streaming for React components using the async gem, replaces sequential fiber draining with a bounded producer/consumer model, introduces a configurable buffer size (default 64), and updates tests and changelog to exercise the new async streaming behavior.

Changes

Cohort / File(s)	Summary
Changelog react_on_rails_pro/CHANGELOG.md	Added Unreleased sections documenting concurrent component streaming, new buffer-size config, and async gem dependency.
Core streaming implementation react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb	Replaced sequential fiber draining with drain_streams_concurrently: launches Async producer tasks to enqueue [index, chunk] into a bounded queue and a writer task that dequeues and writes chunks in order; adds queue close semantics and ensure/cleanup.
Configuration react_on_rails_pro/lib/react_on_rails_pro/configuration.rb	Added DEFAULT_CONCURRENT_COMPONENT_STREAMING_BUFFER_SIZE = 64, new concurrent_component_streaming_buffer_size attribute, constructor wiring, inclusion in public config, and validation method.
Dependency / Gemspec react_on_rails_pro/react_on_rails_pro.gemspec	Added runtime dependency async, version >= 2.6.
Comments / docs react_on_rails_pro/lib/react_on_rails_pro/utils.rb	Updated inline comments to reference Shakapacker instead of Webpacker; no functional changes.
Tests — helpers react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb	Extended test helpers to accept Async::Queue or arrays, added execute_stream_view_containing_react_components helper, and updated tests to exercise Async-based streaming.
Tests — streaming specs react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb	Added StreamController test scaffolding, Async and Async::Queue usage, and a "Component streaming concurrency" suite covering concurrent writes, per-component ordering, empty/single components, and backpressure behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Stream as drain_streams_concurrently()
    participant Producers as Producer Tasks
    participant Queue as LimitedQueue
    participant Writer as Writer Task
    participant Response

    Client->>Stream: stream_view_containing_react_components()
    activate Stream

    Stream->>Producers: start producer tasks (fibers → enqueue [idx,chunk])
    Stream->>Writer: start writer task (dequeue → write)
    activate Producers
    activate Writer

    par Producers produce
        Producers->>Queue: enqueue([index, chunk])
    and Writer consume
        Writer->>Queue: dequeue()
        Queue-->>Writer: [index, chunk]
        Writer->>Response: write(chunk)
    end

    Producers->>Queue: enqueue(nil)  -- signal end
    Writer->>Queue: dequeue()
    Queue-->>Writer: nil
    Writer->>Response: close()
    deactivate Producers
    deactivate Writer

    Stream-->>Client: streaming complete
    deactivate Stream

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

• Pay special attention to stream.rb for: Async task lifecycle, queue close semantics, ordering guarantees, and exception propagation/cleanup.
• Validate concurrent_component_streaming_buffer_size validation and default wiring in configuration.rb.
• Review updated tests (stream_spec.rb, helper spec) for reliable async behavior and flakiness/backpressure scenarios.
• Confirm the added async dependency and gemspec formatting.

Possibly related issues

• Implement concurrent streaming of React components (Promise.any-style fiber draining) react_on_rails_pro#550 — The changes implement the requested Promise.any-style concurrent draining using Async and a bounded queue, matching the issue description.

Suggested reviewers

• Judahmeek
• AbanoubGhadban
• justin808

Poem

🐰 I hopped through queues and tasks so spry,

Chunks in flight beneath the sky—
Producers hum, the writer sings,
Streams converge on silver strings,
Fast components, off they fly!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.33% 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 'Concurrently drain streaming fibers' directly and clearly summarizes the main change: implementing concurrent draining of React component streams instead of sequential processing.

✨ 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 concurrently-drain-streaming-fibers

---

📜 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 de973fe and 38377bc.

📒 Files selected for processing (1)

• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

PR Review: Concurrently drain streaming fibers

This PR introduces concurrent streaming of React components, which is a significant performance improvement. Here's my detailed review:

✅ Strengths

1. Excellent Implementation of Concurrent Streaming

• The use of Async::LimitedQueue with a configurable buffer size (default 64) is a smart choice for backpressure management
• The producer-consumer pattern with multiple producers (fibers) and a single writer is a clean architecture
• The ensure block guarantees proper cleanup even on errors (lines 69-72 in stream.rb)

2. Strong Test Coverage

• Comprehensive test suite covering:
  • Concurrent interleaving (streams components concurrently)
  • Per-component ordering guarantees (maintains per-component ordering)
  • Edge cases (empty list, single component)
  • Backpressure validation (applies backpressure with slow writer)
• Tests use realistic timing checks with sleep to verify async behavior

3. Good Configuration Design

• New concurrent_component_streaming_buffer_size config option with sensible default (64)
• Validation method validate_concurrent_component_streaming_buffer_size ensures positive numeric value (lines 212-218)

4. Documentation

• Clear CHANGELOG entries explaining performance improvements
• Good inline comments explaining the async structure

---

⚠️ Issues & Recommendations

1. Missing Configuration Validation Call (CRITICAL)

Location: configuration.rb:115-123

The new validate_concurrent_component_streaming_buffer_size method is defined but never called in setup_config_values.

Fix:

def setup_config_values
  configure_default_url_if_not_provided
  validate_url
  validate_remote_bundle_cache_adapter
  validate_concurrent_component_streaming_buffer_size  # ADD THIS LINE
  setup_renderer_password
  setup_assets_to_copy
  setup_execjs_profiler_if_needed
  check_react_on_rails_support_for_rsc
end

Without this, invalid buffer sizes could be set without raising errors.

2. Unused Index in Writer Task (Minor Code Quality)

Location: stream.rb:97

The idx from the queue pair is extracted but never used (_idx_from_queue). If it's not needed for debugging or future features, consider simplifying:

def build_writer_task(parent:, queue:)
  parent.async do
    loop do
      chunk = queue.dequeue
      break if chunk.nil?
      
      response.stream.write(chunk)
    end
  end
end

# And in build_producer_tasks:
queue.enqueue(chunk)  # Instead of [idx, chunk]

If the index is kept for future debugging/monitoring, add a comment explaining why.

3. Error Handling: Fiber Resume Errors

Location: stream.rb:81

If a fiber raises an error during fiber.resume, it will propagate to the producer task. The current structure handles this via the ensure block, but consider:

• Adding error logging before the error propagates
• Whether partial results should be written or the entire stream should fail

Suggestion: Add a comment documenting expected behavior when a fiber errors.

4. Potential Race Condition in Tests (Test Quality)

Location: stream_spec.rb:399-415

Tests rely heavily on sleep for synchronization, which can be flaky in CI environments:

queues[1].enqueue("B1")
sleep 0.05  # Assumes processing completes in 50ms
expect(stream).to have_received(:write).with("B1")

Recommendation: Consider using more robust synchronization:

• Use wait_for helpers with timeout
• Check for write call counts rather than exact timing
• Consider adding explicit synchronization primitives

5. Documentation: Memory Trade-offs

The buffer size controls memory usage, but there's no guidance on:

• How to tune this value for different use cases
• Memory implications (e.g., "each buffered chunk uses ~X KB")
• When to increase/decrease the buffer size

Suggestion: Add documentation about tuning this parameter.

6. Gemspec Dependency Constraint

Location: react_on_rails_pro.gemspec:40

s.add_runtime_dependency "async", ">= 2.6"

Consider adding an upper bound constraint to avoid breaking changes in future major versions:

s.add_runtime_dependency "async", ">= 2.6", "< 3"

---

🔒 Security Considerations

✅ No security concerns identified:

• No user input directly controls async operations
• Buffer size is configuration-controlled (not user-supplied at runtime)
• Proper cleanup prevents resource leaks

---

📊 Performance Considerations

✅ Excellent improvements:

• Concurrent draining significantly reduces latency for multi-component pages
• Backpressure prevents memory bloat
• First-ready-wins approach maximizes throughput

⚠️ Minor consideration:

• Each producer task adds overhead. For pages with many components (e.g., 50+), consider whether a work-stealing pool would be more efficient than one-task-per-component.

---

📝 Summary

This is a well-designed PR with solid concurrent programming practices. The main issue is the missing validation call in setup_config_values, which should be added before merge.

Required before merge:

1. Add validate_concurrent_component_streaming_buffer_size call in setup_config_values

Recommended improvements:

1. Document the unused index or remove it
2. Make tests more robust with better synchronization
3. Add tuning guidance for buffer size
4. Consider upper-bound constraint on async gem dependency

Nice to have:

1. Error handling documentation
2. Performance characteristics documentation for high-component-count scenarios

Great work on the implementation! 🎉

[coderabbitai]

Actionable comments posted: 0

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/configuration.rb (1)

115-123: Call the new buffer-size validation.

validate_concurrent_component_streaming_buffer_size is never invoked, so config.concurrent_component_streaming_buffer_size can silently accept nil, 0, or negative values until we hit Async::LimitedQueue.new at runtime and get a low-level exception. Wire the validation into setup_config_values so misconfiguration surfaces as the intended ReactOnRailsPro::Error.

Apply this diff:

     def setup_config_values
       configure_default_url_if_not_provided
       validate_url
       validate_remote_bundle_cache_adapter
       setup_renderer_password
       setup_assets_to_copy
+      validate_concurrent_component_streaming_buffer_size
       setup_execjs_profiler_if_needed
       check_react_on_rails_support_for_rsc
     end

🧹 Nitpick comments (2)

react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (2)

468-487: Tighten the backpressure test assertions.

The assertion expect(write_timestamps.length).to be >= 2 is too permissive. Since 5 chunks are enqueued plus the initial TEMPLATE write, consider asserting the exact count:

-      expect(write_timestamps.length).to be >= 2
+      expect(write_timestamps.length).to eq(6)  # TEMPLATE + 5 chunks

Alternatively, if the exact count varies, at least verify all chunks were written:

-      expect(write_timestamps.length).to be >= 2
+      expect(stream).to have_received(:write).with("TEMPLATE")
+      5.times { |i| expect(stream).to have_received(:write).with("Chunk#{i}") }

---

374-488: Consider adding error handling tests.

The test suite covers the happy path well, but consider adding tests for error scenarios:

• What happens when a component fiber raises an error during streaming?
• How does the system handle a queue that closes prematurely?
• What's the behavior when a client disconnects mid-stream?

The PR description mentions "handling for client disconnects," so tests validating this behavior would be valuable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3f06be and f95d0f2.

⛔ Files ignored due to path filters (1)

• react_on_rails_pro/Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• react_on_rails_pro/.gitignore (1 hunks)
• react_on_rails_pro/CHANGELOG.md (2 hunks)
• react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1 hunks)
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (6 hunks)
• react_on_rails_pro/lib/react_on_rails_pro/utils.rb (2 hunks)
• react_on_rails_pro/react_on_rails_pro.gemspec (1 hunks)
• react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (7 hunks)
• react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (2 hunks)

🧰 Additional context used

🧠 Learnings (10)

📚 Learning: 2024-10-08T20:53:47.076Z

Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

• react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
• react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
• react_on_rails_pro/lib/react_on_rails_pro/utils.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/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb

📚 Learning: 2025-09-16T08:01:11.146Z

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
• react_on_rails_pro/react_on_rails_pro.gemspec
• react_on_rails_pro/.gitignore
• react_on_rails_pro/lib/react_on_rails_pro/utils.rb
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb

📚 Learning: 2025-04-26T21:55:55.874Z

Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/react_on_rails_pro.gemspec
• react_on_rails_pro/lib/react_on_rails_pro/utils.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:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/react_on_rails_pro.gemspec
• react_on_rails_pro/.gitignore
• react_on_rails_pro/lib/react_on_rails_pro/utils.rb

📚 Learning: 2025-02-13T16:50:47.848Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

• react_on_rails_pro/CHANGELOG.md

📚 Learning: 2025-09-15T21:24:48.207Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.

Applied to files:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/react_on_rails_pro.gemspec
• react_on_rails_pro/.gitignore

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

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/react_on_rails_pro.gemspec
• react_on_rails_pro/.gitignore
• react_on_rails_pro/lib/react_on_rails_pro/configuration.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:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb

📚 Learning: 2025-07-08T05:57:29.630Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.

Applied to files:

• react_on_rails_pro/CHANGELOG.md
• react_on_rails_pro/lib/react_on_rails_pro/utils.rb

🧬 Code graph analysis (3)

react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1)

react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1)

• stream_view_containing_react_components (33-46)

react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1)

react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)

• configuration (9-38)

react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (2)

react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (4)

• initialize (4-64)
• initialize (5-13)
• initialize (66-149)
• initialize (67-69)

react_on_rails_pro/lib/react_on_rails_pro/concerns/stream.rb (1)

• stream_view_containing_react_components (33-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). (6)

• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: rspec-package-specs (3.3.7)
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: lint-js-and-ruby
• GitHub Check: claude-review
• GitHub Check: markdown-link-check

🔇 Additional comments (7)

react_on_rails_pro/lib/react_on_rails_pro/utils.rb (1)

111-121: Documentation correctly updated to reflect Shakapacker.

The comment changes accurately replace outdated "webpacker" references with "Shakapacker" to align with the current bundler. No functional changes present. Other "webpacker" references in the codebase are appropriately scoped to test files or serve a specific purpose (e.g., the doctor module's deprecation check for Webpacker v16 removal), so they do not conflict with this documentation update.

react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (3)

7-31: Well-designed test double for streaming.

The StreamController provides a clean, minimal implementation for testing concurrent streaming behavior. The fiber-based approach correctly mirrors the production streaming model.

---

374-394: Clean test helpers for async streaming.

The run_stream and setup_stream_test helpers provide good abstractions for testing concurrent streaming scenarios. The use of Sync properly wraps async operations.

---

396-487: Consider CI environment robustness for timing-based tests.

These tests rely on sleep calls (0.05s-1s) to control timing assertions. While acceptable for streaming tests, consider whether these values are sufficient for slower CI environments. If tests become flaky, you may need to increase sleep durations or use more robust wait mechanisms.

react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (3)

332-361: Good backward-compatible enhancement.

The dual handling of Async::Queue and Array inputs maintains backward compatibility while enabling async streaming tests. The queue dequeue loop with nil termination follows the expected pattern.

---

452-470: Effective helper for testing async streaming.

The execute_stream_view_containing_react_components helper provides a good abstraction for testing the async streaming flow. The progressive enqueueing with timing checks validates that chunks are written immediately upon receipt.

Note that the sleep 0.05 calls and the assertion at line 465 create a timing dependency that could be flaky in slower environments.

---

472-515: Tests successfully adapted to async streaming.

The tests have been cleanly migrated to use the new async streaming helper while maintaining comprehensive coverage of Rails context, component specification tags, and view content rendering.

[coderabbitai]

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 f95d0f2 and de973fe.

📒 Files selected for processing (1)

• react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (7 hunks)

🧰 Additional context used

🧠 Learnings (4)

📚 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/lib/react_on_rails_pro/configuration.rb

📚 Learning: 2025-09-16T08:01:11.146Z

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

• react_on_rails_pro/lib/react_on_rails_pro/configuration.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:

• react_on_rails_pro/lib/react_on_rails_pro/configuration.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:

• react_on_rails_pro/lib/react_on_rails_pro/configuration.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: build
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: lint-js-and-ruby
• GitHub Check: claude-review
• GitHub Check: markdown-link-check

🔇 Additional comments (5)

react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (5)

63-63: LGTM!

The default buffer size of 64 is reasonable for concurrent component streaming. Since this is configurable, users can tune it based on their specific workload requirements.

---

73-73: LGTM!

The attribute accessor is properly added following the established pattern for configuration options.

---

84-86: LGTM!

The parameter is properly added to the constructor with appropriate default value, and the assignment follows the established pattern. Adding it at the end maintains backward compatibility.

Also applies to: 112-112

---

121-121: LGTM!

The validation is properly invoked during configuration setup, following the established pattern for other configuration validations.

---

35-36: LGTM!

The new configuration option is properly integrated into the default configuration instantiation, maintaining consistency with other configuration parameters.

[coderabbitai]

Thank you for addressing the validation issue! The change from Numeric to Integer ensures buffer sizes are whole numbers as intended. The implementation looks good! ✨

[claude]

Code Review

Thank you for this PR! This is a significant improvement to the streaming infrastructure. I've reviewed the changes and have the following feedback:

✅ Strengths

1. Excellent Performance Improvement: Moving from sequential to concurrent streaming is a major win for latency reduction, especially with multiple components.

2. Well-Structured Concurrency: Using the async gem with a producer-consumer pattern via Async::LimitedQueue is a solid approach. The separation of concerns (producers vs. writer) is clean.

3. Comprehensive Test Coverage: The new test suite in stream_spec.rb covers key scenarios:

   • Concurrent streaming behavior
   • Per-component ordering guarantees
   • Edge cases (empty list, single component)
   • Backpressure handling

4. Proper Error Handling: The begin/ensure block in drain_streams_concurrently ensures the queue is closed and the writer finishes draining even if producers fail.

5. Configuration: The concurrent_component_streaming_buffer_size option with validation provides good tuning flexibility.

🔍 Issues & Suggestions

1. CRITICAL: Client Disconnect Handling ⚠️

The current implementation doesn't handle client disconnects gracefully. If the client closes the connection, producer tasks will continue processing and enqueueing chunks unnecessarily.

Suggested fix in build_producer_tasks:

def build_producer_tasks(parent:, queue:)
  @rorp_rendering_fibers.each_with_index.map do |fiber, idx|
    parent.async do
      loop do
        # Check if client disconnected before expensive operations
        break if response.stream.closed?
        
        chunk = fiber.resume
        break unless chunk

        queue.enqueue([idx, chunk])
      end
    rescue IOError, Errno::EPIPE
      # Client disconnected - stop producing
      break
    end
  end
end

And in build_writer_task:

def build_writer_task(parent:, queue:)
  parent.async do
    loop do
      pair = queue.dequeue
      break if pair.nil?

      _idx_from_queue, item = pair
      response.stream.write(item)
    end
  rescue IOError, Errno::EPIPE
    # Client disconnected - stop writing
    nil
  end
end

2. Test Reliability: Race Conditions

The test in react_on_rails_pro_helper_spec.rb uses sleep 0.05 for synchronization, which is brittle:

chunks_to_write = chunks.dup
while (chunk = chunks_to_write.shift)
  queue.enqueue(chunk)
  sleep 0.05  # ⚠️ Race condition: no guarantee chunk is processed

  expect(written_chunks.count).to eq(chunks_read.count)
end

Suggestion: Use a condition variable or callback mechanism to ensure chunks are actually processed before asserting. Consider:

allow(mocked_stream).to receive(:write) do |chunk|
  written_chunks << chunk
  chunk_written_cv.signal  # Signal that write completed
end

3. Ordering Guarantees: Index Not Used

The code enqueues [idx, chunk] pairs but never uses the index for ordering:

queue.enqueue([idx, chunk])  # idx is stored
# Later...
_idx_from_queue, item = pair  # idx is discarded
response.stream.write(item)

Question: What guarantees per-component ordering? The Async::LimitedQueue preserves FIFO order globally, but if components produce chunks at different rates, they'll interleave. Is this intentional?

If you want to preserve per-component order while allowing interleaving between components, you might need:

• To use the index to track which component each chunk belongs to
• Document that chunks from different components may interleave, but each component's chunks remain ordered

Suggested documentation addition to the method:

# Drains all streaming fibers concurrently.
# 
# Ordering guarantees:
# - Chunks from the same component maintain their order
# - Chunks from different components may interleave based on production timing
# - The first component to produce a chunk will have it written first

4. Configuration Validation Timing

The validation in validate_concurrent_component_streaming_buffer_size happens at setup time, but the value could be changed after initialization. Consider making this an attr_reader with a setter that validates:

def concurrent_component_streaming_buffer_size=(value)
  unless value.is_a?(Integer) && value.positive?
    raise ReactOnRailsPro::Error,
          "config.concurrent_component_streaming_buffer_size must be a positive integer"
  end
  @concurrent_component_streaming_buffer_size = value
end

5. Memory Considerations

With buffer_size = 64 (default), if chunks are large (e.g., 1MB each), you could buffer up to 64MB in memory. Consider:

• Document the memory implications in the configuration description
• Add a warning if chunks are unusually large
• Consider making buffer size adaptive based on chunk size

6. Changelog: Dependency Should Be Under 'Changed'

The async gem dependency addition should be under a "Changed" section or explicitly called out as a breaking change if users need to add it:

### ⚠️ Dependencies
- Added `async` gem dependency (>= 2.6) to support concurrent streaming functionality.

📝 Minor Improvements

1. Code Comments: Add comments explaining the producer-consumer pattern for maintainability:

   # Producer tasks: Each fiber drains its stream and enqueues chunks
   tasks = build_producer_tasks(parent: parent, queue: queue)
   
   # Consumer task: Single writer dequeues and writes to response stream
   writer = build_writer_task(parent: parent, queue: queue)

2. Error Logging: Consider logging when producers or the writer encounter errors for debugging in production.

3. Metrics: Consider adding instrumentation (e.g., ActiveSupport::Notifications) for:

   • Time to drain all components
   • Queue size over time
   • Number of components streamed concurrently

🧪 Test Suggestions

1. Add test for client disconnect scenario:

   it "stops producing when client disconnects" do
     queues, controller, stream = setup_stream_test
     
     allow(stream).to receive(:write).and_raise(IOError.new("client disconnected"))
     
     run_stream(controller) do |_parent|
       queues[0].enqueue("A1")
       queues[0].enqueue("A2")  # Should not be processed
       queues[0].close
       sleep 0.1
     end
     
     expect(stream).to have_received(:write).once  # Only first chunk
   end

2. Add test for producer error handling:

   it "continues streaming other components when one fails" do
     # Test that one failing fiber doesn't block others
   end

3. Add performance benchmark to quantify improvement:

   it "streams faster than sequential" do
     # Compare time to drain N components concurrently vs sequentially
   end

🎯 Summary

This is a solid PR that delivers real performance value. The main concerns are:

1. MUST FIX: Client disconnect handling to prevent resource waste
2. SHOULD FIX: Test race conditions using sleep
3. SHOULD CLARIFY: Ordering guarantees and whether the index is needed
4. NICE TO HAVE: Better documentation, metrics, and additional edge case tests

The code quality is good, the architecture is sound, and the test coverage is comprehensive. With the client disconnect handling addressed, this will be production-ready.

Let me know if you'd like help implementing any of these suggestions!

---

Review generated with assistance from Claude Code

[justin808]

@ihabadham respond with why you don't want to address the review comments.

CC: @AbanoubGhadban