Add support for incremental rendering at ruby side #2076
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces async props capabilities for React Server Components through a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker as Node Renderer Worker
participant VM as Execution Context / VM
participant Handler as Render Handler
Client->>Worker: POST /bundles/:bundleTimestamp/incremental-render<br/>(NDJSON stream)
Worker->>Worker: performRequestPrechecks()
Worker->>Handler: handleIncrementalRenderRequest(first chunk)
Handler->>VM: buildExecutionContext([bundlePath])
Handler->>Handler: handleRenderRequest()
Handler->>VM: executionContext.runInVM(code)
VM-->>Handler: { response, executionContext }
Handler-->>Worker: { response, sink }
Worker->>Client: stream response
loop for each update chunk
Client->>Worker: send update chunk (NDJSON)
Worker->>Handler: sink.add(chunk)
Handler->>VM: executionContext.runInVM(updateCode)
VM-->>Handler: result
Worker->>Client: stream update
end
Client->>Worker: close stream
Worker->>Handler: sink.handleRequestClosed()
Handler->>VM: executionContext.runInVM(closeCode)
VM-->>Handler: result
Worker->>Client: stream end
sequenceDiagram
participant RSC as RSC Component
participant AsyncMgr as AsyncPropsManager
participant Tracker as RSCRequestTracker
RSC->>AsyncMgr: addAsyncPropsCapabilityToComponentProps(props)
AsyncMgr->>AsyncMgr: create new AsyncPropsManager instance
AsyncMgr-->>RSC: { asyncPropManager, props: {..., getReactOnRailsAsyncProp} }
RSC->>RSC: render with getReactOnRailsAsyncProp
RSC->>AsyncMgr: getReactOnRailsAsyncProp('propName')
AsyncMgr->>AsyncMgr: getProp('propName') creates Promise
AsyncMgr-->>RSC: Promise<unknown>
Tracker->>Tracker: stream sends data
Tracker->>AsyncMgr: setProp('propName', value)
AsyncMgr->>AsyncMgr: resolve Promise with value
RSC->>RSC: await promise resolves
RSC-->>RSC: render with prop value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/pro509/make-renderer-use-ndjson-for-communication
Code Review for PR #2076SummaryThis PR changes the HTTPX plugin from :stream to :stream_bidi for the HTTP connection used in the Node Renderer request handling (react_on_rails_pro/lib/react_on_rails_pro/request.rb:269). Critical Issues1. Missing PR DescriptionThe PR description is incomplete - it still contains the template placeholders. Please provide:
2. Missing TestsThe PR checklist indicates tests need to be added, but I only see one file changed:
The existing tests in spec/react_on_rails_pro/request_spec.rb mock the streaming plugin, but they may not catch issues specific to :stream_bidi. 3. Missing CHANGELOG EntryPer CLAUDE.md guidelines, this appears to be a user-visible change (enabling incremental rendering) and should have a CHANGELOG entry in /react_on_rails_pro/CHANGELOG.md (Pro-only feature). 4. Missing Documentation
Technical ConsiderationsUnderstanding the ChangeBased on HTTP/2 bidirectional streaming capabilities, :stream_bidi likely enables:
However, without documentation or context, I cannot verify:
Potential Impact AreasUnlikely to break (based on code analysis):
Needs verification:
Action ItemsBefore this PR can be merged:
Suggestions
What Looks Good
Overall Assessment: The code change itself appears safe, but the PR lacks essential context, documentation, and testing. Please address the action items above before merging. |
Code Review for PR #2076Thank you for working on incremental rendering support! I've reviewed the changes and have the following feedback: 🔴 Critical Issues1. Incomplete PR DescriptionThe PR description still contains placeholder text and doesn't explain:
Action Required: Please update the PR description with:
2. Missing Test UpdatesThe code changes the streaming plugin from Action Required: Update the test to use # In react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb:40
.plugin(:stream_bidi) # Changed from .plugin(:stream)3. Pull Request Checklist Not CompletedAll three checklist items are unchecked:
Action Required:
|
Code Review - PR #2076: Add support for incremental rendering at ruby sideThank you for working on this incremental rendering feature! I've reviewed the changes and have several important observations and recommendations. 🔍 Summary of ChangeThis PR changes a single line in
This change appears to be part of the larger incremental rendering work tracked in issue #2075 and the async props implementation in PR #1853.
|
Code Review for PR #2076OverviewThis PR changes the httpx plugin from :stream to :stream_bidi (bidirectional streaming) to enable incremental rendering support. This is a critical infrastructure change for async props and incremental rendering (related to #1853). Positive Aspects
Critical Issues1. Missing PR DescriptionThe PR description template was not filled out. Please complete it with:
2. Checklist Not CompletedPer CLAUDE.md, all checklist items must be completed. None are checked. 3. Test Coverage ConcernsThe test file react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb:40 still uses .plugin(:stream), but production code now uses .plugin(:stream_bidi). These need to match. 4. Missing CHANGELOG EntryThis needs an entry in CHANGELOG_PRO.md (Pro-only change) 5. CI FailuresThe PR shows 2 test failures that must be fixed:
Per CLAUDE.md master health monitoring guidelines, never merge with failing tests. 6. Documentation NeededConsider adding an inline comment in request.rb explaining why stream_bidi is used for incremental rendering. Code Quality Assessment
Required Actions Before Merge
SummaryThis is an important infrastructure change that blocks async props functionality. The code change itself is correct and necessary, but the PR needs improvements in documentation, testing, and changelog entries before merge. Merge recommendation: Do not merge until:
Once addressed, this will be ready to merge. Great work on identifying the need for bidirectional streaming! Review completed following React on Rails CLAUDE.md guidelines |
Code Review FeedbackSummaryThis PR changes the HTTPX plugin from 🔴 Critical Issues1. Incomplete PR DescriptionThe PR description is completely unfilled - it still contains the template text. For a change affecting streaming behavior, we need:
2. Missing Test UpdatesThe test file Location: 3. No CHANGELOG EntryAccording to 4. CI FailuresThe PR has failing CI jobs:
These failures need to be investigated and resolved before merging. They may be related to this plugin change.
|
Code Review: PR #2076 - Add support for incremental rendering at ruby sideSummaryThis PR changes the HTTPX plugin from
|
Code Review - Plugin Change: stream → stream_bidiOverviewThis PR changes the HTTPX plugin from What's Good
Issues and Concerns1. Missing Test Updates (HIGH PRIORITY)The test file Impact: Tests won't match production behavior, potentially missing bugs. 2. Incomplete PR Description (MEDIUM PRIORITY)The PR description is entirely placeholder text. For a critical infrastructure change like this, please provide:
3. Changelog Update Missing (REQUIRED)Per CLAUDE.md guidelines, this change should be documented in 4. No Verification of Plugin Availability (LOW PRIORITY)HTTPx 1.5.1 is in use (per Gemfile.lock). While
Security ConsiderationsNo apparent security issues - the plugin change maintains the same security posture as the existing streaming implementation. Required Actions Before Merge
Testing RecommendationsSince this changes core streaming behavior, please verify:
SummaryRecommendation: Request changes - The core change is sound and necessary, but test coverage and documentation must be updated before merging. This is infrastructure code that affects all incremental rendering functionality, so we need complete test alignment. Risk Level: Medium (tests don't match production, but change itself is straightforward) Reviewed with repository guidelines from CLAUDE.md |
Code Review for PR #2076: Add support for incremental rendering at ruby sideOverviewThis PR makes a single-line change to the HTTPX plugin configuration, changing from
|
Code Review for PR #2076OverviewThis PR changes the HTTPX plugin from 🔴 Critical Issues1. Incomplete PR DescriptionThe PR description is completely missing. The template placeholders remain unfilled:
Action Required: Please fill out the PR description explaining:
2. CI FailuresThe PR has 2 failing CI jobs:
Action Required:
3. Missing TestsThe change modifies core HTTP connection behavior but adds no tests:
Action Required:
4. Missing Documentation
Action Required:
|
Code Review FeedbackSummaryThis PR changes the HTTPX plugin from 🔴 Critical Issues1. Incomplete PR DescriptionThe PR description still contains placeholder text and doesn't explain:
Action Required: Fill out the PR description with actual context about this change. 2. Missing Changelog EntryPer the checklist, this needs a CHANGELOG_PRO.md entry explaining:
Note: If this is purely internal with no user-facing impact, that should be stated. 3. PR Checklist Not CompletedAll three checklist items are unchecked
|
Code Review FeedbackThank you for this PR! I've reviewed the change to use 🎯 SummaryThis PR changes the httpx plugin from ✅ What's Good
🚨 Critical Issues1. Incomplete PR Description
|
AbanoubGhadban
force-pushed
the
abanoubghadban/pro509/make-renderer-use-ndjson-for-communication
branch
2 times, most recently
from
c0dac9a to
931d45e
Compare
Implement AsyncPropsEmitter class to support sending async props incrementally during streaming render. This is the first component for Ruby-side incremental rendering support. Features: - call(prop_name, value) API for emitting async props - Generates NDJSON update chunks with bundleTimestamp and updateChunk - Creates JavaScript code to call asyncPropsManager.setProp() - Error handling: logs errors but continues (doesn't abort render) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Extract common path generation logic into build_render_path and add
support for incremental rendering endpoint paths.
Changes:
- Add prepare_incremental_render_path method for generating
/bundles/{hash}/incremental-render/{digest} paths
- Extract shared logic to private build_render_path method
- Supports both server and RSC bundle selection
- Add minimal tests covering path format and bundle selection
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Step 3: Implement render_code_with_incremental_updates and supporting infrastructure This commit adds support for incremental rendering using HTTP2 bidirectional streaming, enabling progressive resolution of async props during server-side rendering. ## Key Changes: **AsyncPropsEmitter** (lib/react_on_rails_pro/async_props_emitter.rb): - Created emitter class for sending async prop updates to Node renderer - Implements `call(prop_name, prop_value)` for emitting updates - Owns generation of `end_stream_chunk` for onRequestClosedUpdateChunk - Encapsulates all update chunk and end stream JavaScript generation **StreamRequest** (lib/react_on_rails_pro/stream_request.rb): - Added Async::Barrier support for non-blocking concurrent task management - Wraps each_chunk execution in Sync block with barrier - Passes barrier to request_executor for spawning async tasks - Ensures proper cleanup and synchronization of async operations **Request** (lib/react_on_rails_pro/request.rb): - Refactored connection architecture with two separate connections: - standard_connection: Uses :stream plugin for regular streaming - incremental_connection: Uses :stream_bidi for bidirectional streaming - Implemented render_code_with_incremental_updates: - Creates bidirectional streaming request with NDJSON format - Spawns async props block in background using barrier.async - Properly closes request stream after async block completes - Selects bundle timestamp based on RSC vs standard rendering - Added build_initial_incremental_request helper using emitter **Tests**: - Added comprehensive tests for render_code_with_incremental_updates - Tests verify NDJSON request format, barrier.async spawning, and emitter usage - Tests for rsc_bundle_hash selection when is_rsc_payload is true - Uses unverified doubles for HTTPX streaming interfaces with justification - Updated stream_request_spec to test barrier parameter passing ## Technical Details: - Uses Async gem with Async::Barrier for concurrent async prop resolution - HTTP2 bidirectional streaming via HTTPX :stream_bidi plugin - NDJSON protocol for streaming communication - Proper separation of concerns: AsyncPropsEmitter owns chunk generation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Modify eval_streaming_js to detect async_props_block in render_options - Route to incremental rendering path when async_props_block is present - Fall back to standard streaming when async_props_block is absent - Add tests for both execution paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add async_props_setup_js helper method to generate AsyncPropsManager setup code - Generate code when async_props_block is present in render_options - Initialize AsyncPropsManager and store in sharedExecutionContext - Add tests for both async_props_setup_js and render method 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add monkey-patch for missing inflight? method in HTTPX::Plugins::StreamBidi::Signal class. This fixes NoMethodError when closing persistent bidirectional streaming connections, which is required for incremental rendering feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…reaming Add comprehensive integration tests to verify incremental rendering functionality: - Test bundle upload to node renderer using fixture bundles - Test simple non-streaming render request using ReactOnRails.dummy - Test incremental rendering with stream values - Test bidirectional streaming to ensure chunks are received concurrently Tests use simple fixture bundles from packages/node-renderer/tests/fixtures/ to avoid complexity and focus on testing the HTTP/streaming protocol between Ruby and the Node renderer. Key testing approach: - Mock populate_form_with_bundle_and_assets to use real fixture bundles - Mock AsyncPropsEmitter to generate proper update chunks - Use Async::Condition for bidirectional streaming verification - Timeout wrapper prevents deadlock if streaming isn't truly bidirectional 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
AbanoubGhadban
force-pushed
the
add-support-for-incremental-rendering-at-ruby-side
branch
from
e6c9b94 to
a4725e1
Compare
AbanoubGhadban
merged commit 8703f5d
into
abanoubghadban/pro509/make-renderer-use-ndjson-for-communication
AbanoubGhadban
deleted the
add-support-for-incremental-rendering-at-ruby-side
branch
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.