Add support for incremental rendering at ruby side #2076
Conversation
|
Caution Review failedFailed to post review comments 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)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
|
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.