Add bidirectional streaming for async props (pull mode)#4048
Conversation
size-limit report 📦
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a bidirectional "pull mode" for async props in the incremental streaming pipeline. Node-side ChangesPull Mode Async Props Core Protocol
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Rails as Rails (StreamRequest)
participant Emitter as AsyncPropsEmitter
participant Node as Node Renderer
participant APM as AsyncPropsManager
participant React as React Component
Browser->>Rails: HTTP GET /lazy_props_for_testing
Rails->>Node: initial NDJSON chunk (pullEnabled: true, pushProps: [])
Node->>React: render component
React->>APM: getProp("users")
APM->>Node: propRequest chunk → PassThrough stream
Node->>Rails: propRequest control message
Rails->>Emitter: pull_requests.enqueue("users")
Emitter->>Rails: dequeue → "users"
Rails->>Node: NDJSON async prop update (users data)
Node->>APM: resolve "users" promise
APM->>React: resolved users data
React->>Node: React stream ends
Node->>Rails: renderComplete control message
Rails->>Emitter: render_complete! → pull_requests.close
Rails->>Browser: streaming HTML complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0f5d17b7e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Review triage summaryScan window: Full PR history Addressed
Declined (with rationale)
Skipped
All review threads resolved. Monitoring CI. |
Greptile SummaryThis PR implements bidirectional (pull-mode) streaming for async props, complementing the existing push model. React components can now call
Confidence Score: 3/5The core streaming path is well-designed and the double render_complete! call is safely guarded, but the write-failure path in AsyncPropsEmitter permanently poisons the pushed_props set before the write completes, leaving React unable to recover a failed prop through pull mode. The @pushed_props.add(prop_name) mutation happens unconditionally on entry to call, before the stream write that can fail. In pull mode this turns a transient write error into a silent permanent loss: pull requests for the prop are filtered out, no explicit rejection reaches React, and only a generic endStream rejection surfaces much later. The rest of the architecture — the PassThrough multiplexing, PullRequestQueue close-guarding, and the buffered-then-flush emitter setup — is sound. async_props_emitter.rb needs the @pushed_props.add moved to after a successful write (or rolled back in the rescue block). handleIncrementalRenderRequest.ts and streamingUtils.ts have duplicated wire-format functions that will silently diverge if the protocol changes. Important Files Changed
Sequence DiagramsequenceDiagram
participant Rails
participant NodeRenderer as Node Renderer
participant ReactVM as React (VM)
Rails->>NodeRenderer: "NDJSON line 1: {renderingRequest, pullEnabled, pushProps, onRequestClosedUpdateChunk}"
NodeRenderer->>ReactVM: Execute rendering request
ReactVM->>ReactVM: AsyncPropsManager.getProp("lazyProp") → buffer propRequest
NodeRenderer->>NodeRenderer: Set PROP_REQUEST_EMITTER on sharedExecutionContext
NodeRenderer->>ReactVM: flushPendingPullRequests()
ReactVM-->>NodeRenderer: propRequest chunk (via injectableStream)
NodeRenderer-->>Rails: "propRequest {messageType: propRequest, propName: lazyProp}"
Rails->>Rails: PullRequestQueue.enqueue("lazyProp")
Rails->>Rails: User block: emit.call("lazyProp", value)
Rails->>NodeRenderer: "NDJSON: {updateChunk: asyncPropsManager.setProp(...)}"
NodeRenderer->>ReactVM: Execute setProp → promise resolves
ReactVM-->>NodeRenderer: HTML chunk (rendered with prop)
NodeRenderer-->>Rails: HTML chunk (via injectableStream)
Rails-->>Rails: yield HTML chunk to ActionController::Live
ReactVM-->>NodeRenderer: React stream ends
NodeRenderer-->>Rails: renderComplete chunk
Rails->>Rails: render_complete!() → PullRequestQueue.close()
Rails->>Rails: while dequeue → nil → block exits
Rails->>NodeRenderer: Close output stream (END_STREAM)
NodeRenderer->>ReactVM: Execute onRequestClosedUpdateChunk → endStream()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8ca32f1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d24902b9b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bac1e61d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.ts`:
- Around line 195-202: The propRequest emitter callback writes to
injectableStream without checking if the stream has already ended or been
destroyed. Add a guard condition before the injectableStream.write() call to
check injectableStream.writableEnded or injectableStream.destroyed, and only
proceed with writing the formatted prop request chunk if the stream is still
writable. This prevents late prop requests from attempting to write to a closed
stream and potentially causing async stream errors that bypass the current
try-catch block.
In `@react_on_rails_pro/lib/react_on_rails_pro/async_props_emitter.rb`:
- Around line 53-58: The AsyncPropsEmitter class uses Set on line 56 and
Async::Queue on lines 157, 167, and 174 without explicitly requiring these
dependencies at the top of the file. Add explicit require statements at the
beginning of the file to require the Set standard library and the Async gem (or
its Queue component), ensuring that AsyncPropsEmitter is load-order independent
and all its dependencies are clearly declared locally.
In `@react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb`:
- Around line 441-474: The read_lazy_props_from_redis method creates a Redis
client instance on line 442 but never closes it, which causes resource leaks
from accumulated open sockets. Wrap the Redis operations in an ensure block to
guarantee the connection is closed after all work is complete, regardless of
whether the method exits normally or raises an exception. Close the redis
connection in the ensure block to prevent file descriptor accumulation.
In `@react_on_rails/lib/react_on_rails/length_prefixed_parser.rb`:
- Around line 130-139: The control message handling branch (identified by the
messageType check) currently silently discards any payload without validating
that it is actually zero-length. Add a validation check before yielding the
result to ensure the payload has zero length for control messages like
propRequest and renderComplete. If the payload is non-zero when a control
message is detected, raise an error to reject the malformed message and fail
loudly rather than silently proceeding with incomplete data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af08a8af-ef3b-4fc9-be5c-409144d7e355
📒 Files selected for processing (22)
packages/react-on-rails-pro-node-renderer/src/worker/handleIncrementalRenderRequest.tspackages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro/src/AsyncPropsManager.tsreact_on_rails/lib/react_on_rails/length_prefixed_parser.rbreact_on_rails_pro/lib/react_on_rails_pro/async_props_emitter.rbreact_on_rails_pro/lib/react_on_rails_pro/request.rbreact_on_rails_pro/lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rbreact_on_rails_pro/lib/react_on_rails_pro/stream_request.rbreact_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/dummy/app/views/pages/lazy_props_for_testing.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/lazy_props_redis_for_testing.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/mixed_props_for_testing.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/mixed_props_redis_for_testing.html.erbreact_on_rails_pro/spec/dummy/app/views/pages/rejection_props_for_testing.html.erbreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyPropRejectionComponent.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyPropsComponent.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/MixedPropsComponent.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/PropErrorBoundary.client.tsxreact_on_rails_pro/spec/dummy/config/routes.rbreact_on_rails_pro/spec/dummy/e2e-tests/fixture.tsreact_on_rails_pro/spec/dummy/e2e-tests/lazyProps.spec.tsreact_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/node_rendering_pool_spec.rb
|
CI process update after #4036 merged: Please rebase or otherwise update this PR onto current Post-merge audit tracker: #4055 Recommended update path:
Vocabulary changes:
|
Code Review: Add bidirectional streaming for async props (pull mode)OverviewThis PR implements a sophisticated bidirectional streaming mechanism for async props. The architecture is well thought out: React components emit Critical Issues1. Nil
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code Review: Bidirectional Streaming for Async Props (Pull Mode)Overall Assessment: Solid architecture — the layered approach (React → Node → Ruby → React) is well thought out and the buffering/flush mechanism handles the tricky timing window cleanly. A few issues worth addressing before merge. Critical / Bug-Risk1. If 2. E2E tests do not exercise the actual bidirectional pull path All three Redis E2E test pages route through The core bidirectional path (React emits Consider adding at least one test where the controller calls Correctness3. Double
4. Both are called in sequence in Minor5. Magic string
6. Test helper format inconsistency
7. Both format functions include 8. PR description vs. diff inconsistency The architecture table lists Test Coverage Summary
|
Code Review: Bidirectional Streaming for Async Props (Pull Mode)This is a well-structured PR implementing a significant new capability. The architecture is sound and test coverage is solid. [Security] Rejection reasons are exposed verbatim to the browser. The [Correctness] [Correctness] [Correctness] The two-phase catch-up mechanism ( [Code Duplication] [API Design] The [Tests] No test covers the TOCTOU path in [Tests] Spec helper wire format differs from production. [Minor] [Minor] Generated with Claude Code |
Code Review — PR #4048: Bidirectional async props streaming (pull mode)OverviewThis PR adds a pull channel to the existing async-props push model: React components can now call StrengthsSecurity — rejection reason sanitization is correctly implemented. Backward compatibility via legacy bridge methods. Protocol constant sync tests are a strong safeguard. Safety-net IssuesMedium —
|
| promiseController.resolved = true; | ||
| } | ||
|
|
||
| rejectProp(propName: string, reason: string) { |
There was a problem hiding this comment.
rejectProp has no explicit isClosed guard. It relies on getOrCreatePromiseController returning undefined when isClosed is true (line 253), which then triggers the if (!promiseController) return at line 129. The test for "rejectProp after endStream does not change the endStream rejection reason" confirms the behaviour is correct, but the coupling is non-obvious — especially because setProp and endStream both open with an explicit if (this.isClosed) check. A matching guard here would make the intent clear to future readers:
| rejectProp(propName: string, reason: string) { | |
| rejectProp(propName: string, reason: string) { | |
| if (this.isClosed) { | |
| return; | |
| } | |
| const promiseController = this.getOrCreatePromiseController(propName); |
|
test comment |
|
Code Review: PR 4048 - Bidirectional async props streaming (pull mode) Overview: This PR adds a pull channel to the existing push-based async props streaming, allowing React components to lazily request props from Rails on demand. The architecture layers cleanly: React emits propRequest control messages, the Node renderer multiplexes them onto the response stream, Rails parses and routes them to a fiber-safe queue, and the view block dequeues and resolves them. Positives:
DISCUSS - CONTROL_MESSAGE_TYPES duplicated without a sync test: CONTROL_MESSAGE_TYPES = %w[propRequest renderComplete] is defined independently in both ReactOnRails::LengthPrefixedParser and ReactOnRailsPro::StreamRequest. The cross-language test checks TS constants against the Ruby prop-name length constant, but nothing validates that the two Ruby constants stay in sync. A future addition (e.g., a renderError type) must be added in both places, and a typo would silently misroute chunks. Consider asserting StreamRequest::CONTROL_MESSAGE_TYPES == LengthPrefixedParser::CONTROL_MESSAGE_TYPES in a spec, or extracting to a single constant one class re-exports. FOLLOWUP - sharedExecutionContext newly exposed on ExecutionContext type: vm.ts adds sharedExecutionContext: Map<string, unknown> to the exported ExecutionContext type. This raw mutable map bridges the VM sandbox and the host Node.js process. Any code holding an ExecutionContext can now read or write pull-mode keys (pullEnabled, propRequestEmitter, etc.), potentially spoofing or disrupting the protocol. Narrowing the exposure (e.g., a typed accessor instead of the raw map) would reduce attack surface. FOLLOWUP - blocking Redis read (block: 30_000) can delay cleanup in E2E tests: In read_lazy_props_from_redis, the loop alternates between emit.pull_requests.dequeue and redis.xread with block: LAZY_PROP_REDIS_BLOCK_MS. If renderComplete arrives while the fiber is inside the Redis call, the queue is closed but the fiber stays blocked for up to 30 seconds before reaching dequeue. Correctness is fine (the ensure in stream_request.rb covers the stream side), but this can slow E2E teardown. Checking emitter.pull_requests.closed? after redis_stream_messages returns, or using a shorter block timeout, would tighten the window. FOLLOWUP - prop_name length not validated in AsyncPropsEmitter#reject: AsyncPropsEmitter#reject(prop_name, reason) passes prop_name directly to generate_reject_prop_js without checking its length. In normal pull-mode flow, route_control_message pre-validates the name before enqueuing. But reject is a public API that callers invoke directly (as in the test views), so there is no guarantee the name has been pre-validated. A defensive guard matching the one in call would provide defense-in-depth. NON_BLOCKING_DECISION - @received_first_chunk semantics changed: record_first_chunk is now called only when yielded_content is true, so the slow-first-chunk warning fires on the first HTML chunk, not the first any-chunk. This is correct (control messages should not satisfy the marker), but the variable name @received_first_chunk no longer reflects the semantic. A comment or rename to @received_first_content_chunk would help future readers. Test Coverage Gaps:
Summary: The design is sound and the implementation is thorough. Main actionable items: (1) DISCUSS - Add a test or structural guard to keep the two Ruby CONTROL_MESSAGE_TYPES constants in sync. (2) FOLLOWUP - Restrict sharedExecutionContext exposure on the ExecutionContext type. (3) FOLLOWUP - Add a length guard to AsyncPropsEmitter#reject for defense-in-depth. Great work on the legacy bridge, the safety-net ensure block, and the security-aware rejection sanitization. |
| def http_status | ||
| @status | ||
| end | ||
| MAX_PULL_PROP_NAME_LENGTH = 256 |
There was a problem hiding this comment.
DISCUSS: CONTROL_MESSAGE_TYPES duplicated across two Ruby classes
This constant is also defined verbatim in ReactOnRails::LengthPrefixedParser (OSS gem). The cross-language test (asyncPropsProtocol.test.ts) keeps the TS constants in sync with Ruby, but nothing ensures the two Ruby definitions stay in sync.
A future renderError message type added to one but not the other would silently misroute chunks — LengthPrefixedParser would pass it through as an HTML chunk while StreamRequest would ignore it.
Suggestion: add an RSpec assertion, or move the constant to LengthPrefixedParser and have StreamRequest reference it:
# In stream_request.rb
CONTROL_MESSAGE_TYPES = ReactOnRails::LengthPrefixedParser::CONTROL_MESSAGE_TYPESThat removes the duplication without restructuring either class.
| ) => Promise<RenderResult>; | ||
| getVMContext: (bundleFilePath: string) => VMContext | undefined; | ||
| release: () => void; | ||
| sharedExecutionContext: Map<string, unknown>; |
There was a problem hiding this comment.
FOLLOWUP: raw Map exposed on the public type increases attack surface
sharedExecutionContext is now part of the exported ExecutionContext contract. Any downstream code holding an ExecutionContext (tests, renderers, future integrations) can read or overwrite protocol keys like pullEnabled, propRequestEmitter, and asyncPropsManager without going through the intended pull-mode setup path.
Consider narrowing the exposure: expose a typed accessor method instead of the raw map, or keep the map internal and add purpose-built getters/setters for pull-mode state. The pull-mode setup code in handleIncrementalRenderRequest.ts would then call e.g. executionContext.setPropRequestEmitter(fn) rather than writing directly into the map.
| @request_stream << "#{update_chunk.to_json}\n" | ||
| @pushed_props.add(prop_name) | ||
| rescue StandardError => e | ||
| # Continue streaming: one failed async prop write should not abort the |
There was a problem hiding this comment.
FOLLOWUP: reject doesn't validate prop_name length
call (a few lines above) doesn't validate prop_name length either, but in the normal pull-mode flow both names arrive from PullRequestQueue which has already been filtered by route_control_message (max 256 chars, non-empty). However, reject is also invoked directly from user-written ERB view code — e.g., emit.reject("forbiddenData", "Access denied") in the rejection test views. Nothing prevents a caller from passing an arbitrarily long name here.
A defensive guard would match the protection applied on the TS side:
def reject(prop_name, reason)
if prop_name.length > ReactOnRailsPro::StreamRequest::MAX_PULL_PROP_NAME_LENGTH
Rails.logger.warn { "[ReactOnRailsPro::AsyncProps] Ignoring reject for oversized prop name (#{prop_name.length} chars)" }
return
end
# ... rest of implementation
| [result, nil] | ||
| end | ||
|
|
||
| def record_first_chunk(request_start_time) |
There was a problem hiding this comment.
NON_BLOCKING_DECISION: @received_first_chunk now means "first content chunk", not "first any chunk"
The change is intentional and correct — control-only frames should not satisfy the slow-first-chunk marker. But the variable name @received_first_chunk and this helper name still suggest "any chunk", which will confuse a future reader who wonders why a stream whose first byte is a propRequest hasn't triggered the callback.
A short comment here (or a rename to @received_first_content_chunk) would make the intent self-documenting:
# Only HTML/content chunks satisfy the first-chunk timing marker.
# Control messages (propRequest, renderComplete) are excluded intentionally.
def record_first_chunk(request_start_time)
return if @received_first_chunk
Code Review: Bidirectional Async Props Streaming (Pull Mode)This is a well-architected feature with clearly thought-out cross-layer coordination. The protocol design, error handling, and test coverage are all solid. Below is a mix of issues and observations. Overall AssessmentArchitecture ✅
Security ✅
Test coverage ✅
Issues Found1. Dead instance variable: 2. Sensitive data in debug log (medium / security) 3. Drain handler has no timeout/error guard (low / reliability) 4. 5. Incidental bug fix not mentioned in the PR description (FYI) 6. QuestionsThread safety for No back-pressure for propRequest floods: A component that calls Overall this is production-quality work. The most actionable items are 2 (sensitive data in debug log) and 3 (drain timeout documentation). Items 1, 4–6 are low priority. |
| def initialize(first_chunk_warn_callback: nil, pull_enabled: false, &request_block) | ||
| @request_executor = request_block | ||
| @first_chunk_warn_callback = first_chunk_warn_callback | ||
| @pull_enabled = pull_enabled |
There was a problem hiding this comment.
@pull_enabled is set here but never read anywhere else in this class. Control message routing is already gated entirely on @emitter being non-nil (see route_control_message), so this variable is dead code. It can be removed along with the pull_enabled: keyword in initialize and create (or kept as a no-op documentation aid if that's preferred).
| @pull_enabled = pull_enabled | |
| @emitter = nil |
| # operators; keep it below info level because staging log aggregators may | ||
| # persist those details. | ||
| def sanitized_rejection_reason(reason) | ||
| Rails.logger.debug { "[ReactOnRailsPro::AsyncProps] Prop rejected (internal reason): #{reason}" } |
There was a problem hiding this comment.
The comment above correctly flags that staging log aggregators may persist debug-level output, but then logs the full raw reason anyway. If reason can contain SQL errors, credentials, or file paths (all common in Rails exceptions), these land in any log sink that enables debug logging.
Consider one of:
- Truncate/sanitize before logging:
reason.to_s.slice(0, 200) - Log at
debugonly whenRails.application.config.log_level <= Logger::DEBUGto make the risk explicit in config - Or simply document the expected type of
reasonso callers know not to pass raw exception messages
The current approach is fine if callers always pass human-written strings (e.g., "Unknown prop: #{prop_name}"), but it creates a footgun for callers who pass e.message directly.
| // even if the source stream has already buffered all its data. | ||
| sourceStream.on('end', () => { | ||
| if (injectableStream.writableNeedDrain) { | ||
| injectableStream.once('drain', writeRenderCompleteAndEnd); |
There was a problem hiding this comment.
If injectableStream is draining when sourceStream ends, writeRenderCompleteAndEnd is deferred to the 'drain' event. This is correct in the happy path, but if the downstream consumer (Rails/Fastify) stalls indefinitely without draining, the 'drain' event never fires and renderComplete is never written. The Ruby side would then sit in pull_requests.dequeue forever (until the safety-net ensure in consume_with_bundle_reupload fires, which requires the HTTP connection to close).
In practice the HTTP/2 connection teardown is the backstop, but it may be worth adding a comment here to explain the expected cleanup path, or an 'error' / 'close' guard on injectableStream to cancel the pending drain listener:
injectableStream.once('drain', writeRenderCompleteAndEnd);
// If the stream is destroyed before draining, cancel the deferred write.
injectableStream.once('close', () => injectableStream.removeListener('drain', writeRenderCompleteAndEnd));
|
|
||
| render() { | ||
| if (this.state.error) { | ||
| const message = this.state.error.message.replace(rejectedByServerPrefix(this.props.propName), ''); |
There was a problem hiding this comment.
String.prototype.replace (not replaceAll) only strips the first match, but the real concern here is the mismatch path: if an error reaches this boundary that was not thrown by rejectProp (e.g., a React rendering invariant, a thrown string, or a network error from a different fetch), the prefix won't match and the full raw error message is rendered in the DOM. That could expose internal details to the user in production.
This is fine for a dummy/test component, but if PropErrorBoundary is ever promoted to a shared utility, consider guarding on the expected prefix:
const prefix = rejectedByServerPrefix(this.props.propName);
const message = this.state.error.message.startsWith(prefix)
? this.state.error.message.slice(prefix.length)
: 'An unexpected error occurred.';
| `${this.overCapBufferedPropRequestCount} over-cap propRequest(s) have stayed unbuffered until catch-up. ` + | ||
| 'The prop request emitter may not have been installed.', | ||
| ); | ||
| return false; |
There was a problem hiding this comment.
The return value semantics here are subtle: true means "tracked and will be emitted eventually" (either immediately or via catchUpPropRequests), while false means "over-cap — not buffered, but still eligible for catch-up because pullRequested stays false".
The over-cap path works correctly because catchUpPropRequests iterates all controllers and picks up any where !controller.pullRequested. But the asymmetry (buffered returns true, over-cap returns false, yet both get caught up) may confuse future readers. A brief comment at the call sites in getProp and catchUpPropRequests noting that pullRequested = false after a dropped buffer means "catch-up eligible" would help.
| stream_id = "stream:#{request_id}" | ||
| until ended | ||
| received_messages = redis.xread(stream_id, last_received_id, block: 0)[stream_id] | ||
| received_messages = redis_stream_messages(redis, stream_id, last_received_id) |
There was a problem hiding this comment.
This is a silent fix to the existing read_async_props_from_redis method: the original code called received_messages.last.first before the loop, which would raise NoMethodError when xread returned nil (stream key not yet created) or an empty array. The redis_stream_messages helper now returns [] in those cases, preventing the crash.
Worth mentioning this as a bug fix in the PR description so it's visible in the changelog rather than being buried in a feature PR.
|
Confidence note:
|
2 participants
Summary
Implements #4046: bidirectional streaming of async props where React components can lazily request props from Rails via HTTP/2 streaming, complementing the existing push model.
User-facing API
Architecture
getProp()inAsyncPropsManageremitspropRequestcontrol messages for non-push propsStreamRequestparses control messages, routespropRequest→PullRequestQueue,renderComplete→ closes queuemessageTypemetadata field for control messagesKey changes
AsyncPropsManager.tsrejectProp(), buffered requests,flushPendingPullRequests()streamingUtils.tsformatPropRequestChunk(),formatRenderCompleteChunk()handleIncrementalRenderRequest.tsvm.tssharedExecutionContextonExecutionContexttypelength_prefixed_parser.rbmessageTypecontrol messages without HTML payloadasync_props_emitter.rbPullRequestQueue,reject(),render_complete!(), pushed props trackingrequest.rbpull_enabledflag,pullEnabled/pushPropsin NDJSON, return[response, emitter]tuplestream_request.rbrender_complete!node_rendering_pool.rbpush_propsto incremental renderTest plan
LengthPrefixedParserspecs pass (8/8)react-on-rails-proandreact-on-rails-pro-node-renderer/stream_async_components,/test_incremental_rendering) have no regressions🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests