Fix async props race condition with lazy AsyncPropsManager initialization

[AbanoubGhadban]

Summary

• Fixes race condition where update chunks arrive before AsyncPropsManager is initialized
• Adds getOrCreateAsyncPropsManager(sharedExecutionContext) for lazy initialization
• Removes debug console.log statements

Fixes #2270

Test plan

• Test stream_react_component_with_async_props without sleep delays before emit calls
• Verify async props are delivered correctly regardless of timing

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/getOrCreateAsyncPropsManager-lazy-init

---

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]

Code Review: Fix async props race condition with lazy AsyncPropsManager initialization

Summary

This PR effectively addresses the race condition described in #2270 by implementing lazy initialization of AsyncPropsManager through a getOrCreateAsyncPropsManager function. The fix ensures update chunks can arrive and execute before the initial render completes without losing async props.

---

✅ Strengths

1. Excellent Architecture & Documentation

• The implementation is well-documented with clear design explanations in both TypeScript and Ruby code
• The AsyncPropsManager.ts header (lines 28-57) provides excellent context about promise caching, order independence, and stream lifecycle
• Ruby code includes helpful protocol documentation explaining NDJSON communication between Rails and Node

2. Clean Lazy Initialization Pattern

• getOrCreateAsyncPropsManager() function (AsyncPropsManager.ts:152-164) implements the singleton pattern correctly
• Uses sharedExecutionContext Map for request-scoped state (avoiding global variables that would leak across requests)
• Both initial render and update chunks use the same function, ensuring they always get the same manager instance

3. Minimal API Surface Changes

• Only adds one new exported function (getOrCreateAsyncPropsManager)
• Ruby-side changes are minimal (just switching from direct manager creation to lazy getter)
• Type definitions properly updated (types/index.ts:142-146, 520)

4. Good Error Handling

• Maintains proper rejection of unresolved props when stream ends
• Handles the case where manager doesn't exist (returns undefined when stream is closed)

---

⚠️ Issues & Concerns

1. CRITICAL: Missing Test Coverage
This PR has NO test coverage for the race condition fix. The "Test plan" checkbox in the PR description is unchecked:

• Test stream_react_component_with_async_props without sleep delays before emit calls
• Verify async props are delivered correctly regardless of timing

Per CLAUDE.md guidelines:

CRITICAL - LOCAL TESTING REQUIREMENTS:

1. NEVER claim a test is "fixed" without running it locally first

Recommendation: Before merging, you should:

1. Add an integration test that calls emit.call immediately (without sleep) to verify props are delivered
2. Add a unit test for getOrCreateAsyncPropsManager that verifies it returns the same instance on multiple calls
3. Consider adding a test that simulates update chunks arriving before the initial render

2. Potential Memory Leak Concern
The AsyncPropsManager is stored in sharedExecutionContext but there's no explicit cleanup mechanism shown. Questions:

• Is sharedExecutionContext automatically garbage collected after the HTTP request completes?
• Should there be an explicit cleanup call somewhere?

Looking at handleIncrementalRenderRequest.ts, the executionContext appears to be scoped to the request lifecycle, which should be fine. But it would be good to verify this doesn't leak memory in long-running processes.

3. Debug Console.log Cleanup
The PR description mentions "Removes debug console.log statements" but I don't see these in the diff. Were they already removed, or is this still pending?

4. Type Safety Consideration
In AsyncPropsManager.ts:155, the type assertion could be safer:

let manager = sharedExecutionContext.get(ASYNC_PROPS_MANAGER_KEY) as AsyncPropsManager | undefined;

Consider using type guard instead:

const manager = sharedExecutionContext.get(ASYNC_PROPS_MANAGER_KEY);
if (manager && manager instanceof AsyncPropsManager) {
  return manager;
}

This provides runtime type checking, not just compile-time assertions.

---

🔍 Code Quality

JavaScript/TypeScript:

• ✅ Well-documented with JSDoc comments
• ✅ Proper TypeScript types
• ✅ Follows functional programming patterns
• ✅ No linting issues visible

Ruby:

• ✅ Follows Ruby style conventions
• ✅ Good use of heredocs for JavaScript generation
• ✅ Proper error handling in AsyncPropsEmitter.call

---

🛡️ Security Considerations

No security concerns identified:

• Uses JSON serialization properly
• No eval() or unsafe code execution
• Request-scoped state prevents cross-request data leakage

---

📊 Performance Considerations

Positive:

• Lazy initialization means no overhead if async props aren't used
• Single manager instance per request (no redundant object creation)

Neutral:

• Map lookup on every getOrCreateAsyncPropsManager call is O(1) and negligible

---

📝 Missing from PR

1. Tests (as mentioned above)

2. CHANGELOG entry - Per CLAUDE.md:

   Update CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes...)

   This is a bug fix that affects Pro users, so it should be added to CHANGELOG_PRO.md.

3. RuboCop/Prettier check - Per CLAUDE.md:

   BEFORE EVERY COMMIT/PUSH:

   1. ALWAYS run bundle exec rubocop and fix ALL violations
   2. ALWAYS ensure files end with a newline character

   Please verify these were run locally.

---

🎯 Recommendations Before Merge

1. Add test coverage (integration test for race condition scenario)
2. Add CHANGELOG_PRO.md entry
3. Verify linting passes (bundle exec rubocop and pnpm run format.listDifferent)
4. Manual testing - Test with and without sleep delays to verify fix works
5. Consider type guard instead of type assertion for runtime safety

---

Final Verdict

✅ Approve with required changes

The core fix is sound and well-implemented. The lazy initialization pattern correctly solves the race condition. However, test coverage is mandatory before merging to ensure this fix works and prevent regressions.

The code quality is high, documentation is excellent, and the approach is architecturally clean. Once tests are added and the checklist items completed, this is ready to merge.

Great work on the analysis and fix! 🎉

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	62.53 KB (0%)
react-on-rails/client bundled (gzip) (time)	62.53 KB (0%)
react-on-rails/client bundled (brotli)	53.71 KB (0%)
react-on-rails/client bundled (brotli) (time)	53.71 KB (0%)
react-on-rails-pro/client bundled (gzip)	63.53 KB (+0.06% 🔺)
react-on-rails-pro/client bundled (gzip) (time)	63.53 KB (+0.06% 🔺)
react-on-rails-pro/client bundled (brotli)	54.59 KB (+0.05% 🔺)
react-on-rails-pro/client bundled (brotli) (time)	54.59 KB (+0.05% 🔺)
registerServerComponent/client bundled (gzip)	127.42 KB (+0.03% 🔺)
registerServerComponent/client bundled (gzip) (time)	127.42 KB (+0.03% 🔺)
registerServerComponent/client bundled (brotli)	61.52 KB (+0.12% 🔺)
registerServerComponent/client bundled (brotli) (time)	61.52 KB (+0.12% 🔺)
wrapServerComponentRenderer/client bundled (gzip)	121.96 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time)	121.96 KB (-0.04% 🔽)
wrapServerComponentRenderer/client bundled (brotli)	56.66 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time)	56.66 KB (-0.01% 🔽)

[greptile-apps]

Greptile Overview

Greptile Summary

This PR successfully addresses a race condition in async props handling where update chunks could arrive before AsyncPropsManager initialization. The solution introduces a lazy initialization pattern via getOrCreateAsyncPropsManager() that ensures whichever code path executes first (initial render or update chunk) will create the manager, and subsequent calls will reuse it.

Key Changes

Core Fix:

• Added getOrCreateAsyncPropsManager(sharedExecutionContext) function that implements lazy initialization
• Updated addAsyncPropsCapabilityToComponentProps to accept sharedExecutionContext parameter and use the lazy initialization pattern
• Modified update chunk generation in Ruby to call getOrCreateAsyncPropsManager instead of directly accessing sharedExecutionContext.get("asyncPropsManager")

Architecture:
The fix leverages the fact that sharedExecutionContext is a Map scoped to a single HTTP request within an ExecutionContext. Both the initial render request and subsequent update chunks share the same ExecutionContext, allowing them to safely share the AsyncPropsManager instance without race conditions.

Issues Found

Critical Issue:
A test fixture (asyncComponentsTreeForTestingRenderingRequest.js) still uses the old API signature where addAsyncPropsCapabilityToComponentProps returned { asyncPropManager, props }. The new API returns only { props } and requires sharedExecutionContext as a parameter. This will cause test failures and must be updated before merging.

Code Quality:

• Removed debug console.log and log.info statements as intended
• Improved error logging in async_props_emitter.rb
• Simplified error handling by relying on VM error reporting

Race Condition Resolution

The lazy initialization pattern correctly handles all timing scenarios:

1. Initial render first: Creates manager, stores in context, update chunks reuse it
2. Update chunk first: Creates manager, stores in context, initial render reuses it
3. Concurrent access: First call creates manager, subsequent calls return existing instance

The implementation is thread-safe because sharedExecutionContext is not shared across requests - each HTTP request gets its own isolated ExecutionContext.

Confidence Score: 2/5

• This PR has a critical test fixture bug that must be fixed before merging
• While the core race condition fix is architecturally sound and well-implemented, there is a critical bug in the test fixture that uses the old API signature. This will cause test failures and indicates the changes weren't fully tested. The test fixture at asyncComponentsTreeForTestingRenderingRequest.js uses the old addAsyncPropsCapabilityToComponentProps API (expecting { asyncPropManager, props } return value) instead of the new API (requiring sharedExecutionContext parameter and returning only { props }). This must be fixed before the PR can be safely merged.
• Critical: packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js - Must be updated to use new API signature

Important Files Changed

File Analysis

Filename	Score	Overview
packages/react-on-rails-pro/src/AsyncPropsManager.ts	5/5	Adds getOrCreateAsyncPropsManager function for lazy initialization - clean implementation that properly handles race conditions
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts	5/5	Updates addAsyncPropsCapabilityToComponentProps to use getOrCreateAsyncPropsManager and accept sharedExecutionContext parameter - properly implements lazy initialization
react_on_rails_pro/lib/react_on_rails_pro/async_props_emitter.rb	5/5	Updates generated JS to use getOrCreateAsyncPropsManager instead of direct sharedExecutionContext access - correctly implements race condition fix with improved logging
react_on_rails_pro/lib/react_on_rails_pro/server_rendering_js_code.rb	5/5	Updates async_props_setup_js to pass sharedExecutionContext to addAsyncPropsCapabilityToComponentProps and removes manual manager storage - properly aligns with new API
packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js	0/5	Test fixture uses OLD API signature - critical bug that will cause test failures, needs immediate update to use new addAsyncPropsCapabilityToComponentProps signature

Sequence Diagram

sequenceDiagram
    participant Rails as Rails Server
    participant Worker as Node Worker
    participant VM as VM Context
    participant Manager as AsyncPropsManager
    participant React as React Component

    Note over Rails,React: Race Condition Fix: Lazy Initialization Pattern

    Rails->>Worker: POST /incremental-render<br/>(First NDJSON line: renderingRequest)
    Worker->>VM: handleIncrementalRenderRequest()
    VM->>VM: buildExecutionContext()<br/>Creates sharedExecutionContext Map
    
    Note over VM,Manager: INITIAL RENDER PATH
    VM->>VM: Execute renderingRequest JS
    VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
    alt Manager doesn't exist
        Manager->>Manager: new AsyncPropsManager()
        Manager->>VM: Store in sharedExecutionContext["asyncPropsManager"]
    end
    VM->>React: addAsyncPropsCapabilityToComponentProps(props, sharedExecutionContext)
    React->>Manager: getReactOnRailsAsyncProp("users")
    Manager->>React: Returns Promise (suspends)
    React-->>Worker: Initial HTML shell
    
    par Update Chunks Can Arrive Anytime
        Note over Rails,Manager: UPDATE CHUNK PATH (can race with initial render)
        Rails->>Worker: NDJSON line 2: updateChunk for "users"
        Worker->>VM: sink.add(updateChunk)
        VM->>VM: Execute updateChunk JS
        VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
        alt Manager already exists
            Manager->>Manager: Return existing instance
        else Manager doesn't exist yet (race condition)
            Manager->>Manager: new AsyncPropsManager()
            Manager->>VM: Store in sharedExecutionContext["asyncPropsManager"]
        end
        VM->>Manager: setProp("users", userData)
        Manager->>React: Resolve Promise
        React-->>Worker: More HTML chunks
    end
    
    Rails->>Worker: Close HTTP request
    Worker->>VM: sink.handleRequestClosed()
    VM->>Manager: getOrCreateAsyncPropsManager(sharedExecutionContext)
    VM->>Manager: endStream()
    Manager->>Manager: Reject unresolved promises
    React-->>Worker: Final HTML chunks

Loading

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js
This test fixture uses the OLD API signature for addAsyncPropsCapabilityToComponentProps, which is incompatible with the changes in this PR. The function signature has changed:

Old (what this fixture uses):

var { props: propsWithAsyncProps, asyncPropManager } = 
  ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps);

New (what it should be):

var { props: propsWithAsyncProps } = 
  ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);

The new signature:

1. Requires sharedExecutionContext as a second parameter
2. No longer returns asyncPropManager (it's managed internally via getOrCreateAsyncPropsManager)
3. The manual sharedExecutionContext.set("asyncPropsManager", asyncPropManager) line is no longer needed

  if (ReactOnRails.isRSCBundle) {
    var { props: propsWithAsyncProps } = ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);
    usedProps = propsWithAsyncProps;
  }

This fixture needs to be updated to match the new API, otherwise tests using this fixture will fail.

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/react-on-rails-pro-node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js
Line: 23:25

Comment:
This test fixture uses the OLD API signature for `addAsyncPropsCapabilityToComponentProps`, which is incompatible with the changes in this PR. The function signature has changed:

**Old (what this fixture uses):**
```javascript
var { props: propsWithAsyncProps, asyncPropManager } = 
  ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps);
```

**New (what it should be):**
```javascript
var { props: propsWithAsyncProps } = 
  ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);
```

The new signature:
1. Requires `sharedExecutionContext` as a second parameter
2. No longer returns `asyncPropManager` (it's managed internally via `getOrCreateAsyncPropsManager`)
3. The manual `sharedExecutionContext.set("asyncPropsManager", asyncPropManager)` line is no longer needed

```suggestion
  if (ReactOnRails.isRSCBundle) {
    var { props: propsWithAsyncProps } = ReactOnRails.addAsyncPropsCapabilityToComponentProps(usedProps, sharedExecutionContext);
    usedProps = propsWithAsyncProps;
  }
```

This fixture needs to be updated to match the new API, otherwise tests using this fixture will fail.

How can I resolve this? If you propose a fix, please make it concise.

[claude]

Code Review - PR #2297

Summary

This PR fixes a race condition in async props handling by implementing lazy initialization of the AsyncPropsManager. The core issue is that update chunks could arrive before the initial render creates the manager, causing async props to be lost.

---

✅ Strengths

1. Solid Architecture - Lazy Initialization Pattern

The getOrCreateAsyncPropsManager function is well-designed:

• Uses the shared execution context as the source of truth
• Handles race conditions elegantly (whoever arrives first creates it)
• Simple and maintainable implementation

2. Excellent Documentation

The JSDoc comments clearly explain:

• The race condition being solved
• How the lazy initialization works
• The relationship with sharedExecutionContext

This is particularly well done in server_rendering_js_code.rb:61-67.

3. Type Safety

Proper TypeScript types added to index.ts with clear parameter documentation.

4. Backward Compatibility

The change is internal - no breaking changes to the public API.

---

⚠️ Issues & Recommendations

1. CRITICAL: Missing Test Coverage

The PR description shows:

## Test plan
- [ ] Test `stream_react_component_with_async_props` without sleep delays
- [ ] Verify async props are delivered correctly regardless of timing

Both checkboxes are unchecked. Per CLAUDE.md:

CRITICAL - LOCAL TESTING REQUIREMENTS:

1. NEVER claim a test is "fixed" without running it locally first

Recommendation:

• Add a test case that reproduces the race condition (e.g., immediate emit without sleep)
• Verify the fix works locally before merging
• Consider adding this to react_on_rails_pro/spec/react_on_rails_pro/async_props_emitter_spec.rb

2. Excessive Logging in Production Code

In async_props_emitter.rb:34-40:

Rails.logger.info do
  "[ReactOnRailsPro::AsyncProps] Sending prop '\#{prop_name}' (\#{chunk_json.bytesize} bytes)..."
end
@request_stream << chunk_json
Rails.logger.info do
  "[ReactOnRailsPro::AsyncProps] Sent prop '\#{prop_name}' successfully"
end

Issues:

• Two info-level logs for EVERY async prop sent (could be dozens per request)
• Will clutter production logs significantly
• "Sent successfully" log doesn't add value - it just means the stream write didn't raise

Recommendations:

• Change to debug level: Rails.logger.debug
• Remove the "sent successfully" log entirely
• Keep only the error logging (which is well-done)

3. Inconsistent Import Order

In handleIncrementalRenderRequest.ts:1-4:

-import log from '../shared/log';
 import { getRequestBundleFilePath } from '../shared/utils';
+import log from '../shared/log';

Why this change? It seems unrelated to the PR's purpose. If it's a linter fix, that's fine, but:

• The PR description doesn't mention import reordering
• This should ideally be in a separate commit or explained

4. Minor: Whitespace Change

In handleIncrementalRenderStream.ts:66:

 let boundary = buffer.indexOf('\n');
+
 while (boundary !== -1) {

Again, not directly related to the fix. Consider whether these formatting changes should be in the scope.

---

🔍 Security & Performance

Security: ✅ Good

• No security concerns identified
• The sharedExecutionContext is properly scoped to the request
• No global state pollution

Performance: ✅ Good

• Lazy initialization adds minimal overhead (one Map lookup per request)
• No memory leaks - manager is scoped to request lifecycle

---

📋 Testing Recommendations

Add a test case like this (pseudo-code):

# In async_props_emitter_spec.rb
it 'handles race condition when emit is called before initial render' do
  # Simulate update chunk arriving before initial render
  emit.call('users', [{ name: 'Alice' }])
  
  # Now render the component
  result = stream_react_component_with_async_props(...)
  
  # Verify the prop was delivered
  expect(result).to include('Alice')
end

---

🎯 Before Merging

Per the repository guidelines:

1. ✅ Run linters:

   bundle exec rubocop
   pnpm run format.listDifferent

2. ⚠️ Complete the test plan:

   • Test the race condition scenario locally
   • Update the checkboxes in the PR description

3. 🔄 Consider addressing the logging verbosity to avoid production log pollution

4. 📝 Update CHANGELOG_PRO.md if not already done (this is a bug fix in Pro functionality)

---

Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

The core fix is architecturally sound and solves a real problem elegantly. The main concerns are:

• Missing test verification (critical per CLAUDE.md)
• Excessive logging that will clutter production logs
• Minor formatting changes that obscure the actual fix

Once the test plan is completed and logging is adjusted, this will be a solid merge.

Great work identifying and fixing this race condition! 🚀

[claude]

Code Review - PR #2297

This PR successfully addresses a race condition in async props delivery by implementing lazy initialization of AsyncPropsManager. The fix is well-architected and follows good engineering principles.

Strengths

1. Excellent Problem Analysis - The root cause identification is spot-on
2. Clean Architecture - getOrCreateAsyncPropsManager() is idempotent and type-safe
3. Consistent Implementation - Used correctly in both code paths
4. Good Documentation - Comprehensive JSDoc and comments
5. Improved Logging - Enhanced error messages with byte sizes and backtraces

Critical Issue: Missing Test Coverage

The existing test (test_incremental_rendering.html.erb) uses sleep 1 which MASKS the race condition this PR fixes. You need a test that exercises the race condition without sleep delays.

Please add a test that emits props immediately to verify the fix prevents regression.

Other Suggestions

1. Parameter Validation - getOrCreateAsyncPropsManager should validate sharedExecutionContext is a valid Map
2. Test Plan - Update PR description to check off completed tests or explain why manual testing wasn't performed

Verdict

Approved with suggestions. Core fix is solid, but please add the automated test before merging.

Code Quality: 8.5/10
Documentation: 9/10
Test Coverage: 4/10 (would be 9/10 with race condition test)

Great work identifying and fixing this subtle timing issue!

[claude]

Code Review - PR 2297

Overall Assessment

Strong fix for a critical race condition. The implementation is well-thought-out and follows good engineering practices.

Strengths

1. Problem Identification - Correctly fixes the race condition where update chunks arrive before AsyncPropsManager initialization
2. Code Quality - Clean, well-documented implementation with excellent JSDoc comments
3. API Design - Backwards-compatible signature changes, simplified return value
4. Error Handling - Improved error logging with backtrace

Issues and Recommendations

CRITICAL: Missing Test Coverage

The PR has NO tests for the race condition fix. This is a serious gap for a concurrency issue.

Required tests:

1. TypeScript unit test for getOrCreateAsyncPropsManager
2. Integration test for the actual race condition (test emit without sleep delays)

The test plan checkbox in the PR description is unfilled. This needs local testing before merge per CLAUDE.md guidelines.

MODERATE: RSpec Test Needs Update

The existing test at react_on_rails_pro/spec/react_on_rails_pro/server_rendering_js_code_spec.rb lines 38-39 and 84 checks for the OLD implementation and will likely fail because asyncPropManager variable no longer exists in generated JS.

Security and Performance

• No security concerns
• Performance is positive (eliminates wasted update chunks, O(1) Map lookup)

Before Merging Checklist

• Add unit tests for getOrCreateAsyncPropsManager (TypeScript)
• Add integration test for race condition scenario (Ruby/RSpec)
• Update existing RSpec expectations (lines 38-40, 84)
• Test the actual scenario from issue 2270 locally
• Run bundle exec rubocop and pnpm run lint
• Fill in test plan checkboxes

Conclusion

This is a solid fix for a real production issue, but it needs test coverage before merging. The race condition is subtle and could easily regress without tests.

Recommendation: Request changes for test coverage, then approve once tests are added.

Great work identifying and solving this timing issue!

[claude]

Code Review Feedback

Overall Assessment

This PR effectively solves a critical race condition in async props handling. The lazy initialization pattern is a solid architectural choice that properly addresses the core issue where update chunks could arrive before the AsyncPropsManager was initialized.

---

Strengths

1. Clean API Design: The getOrCreateAsyncPropsManager function follows the single responsibility principle and provides a clear contract.

2. Good Documentation: JSDoc comments clearly explain the race condition handling and the purpose of the shared execution context.

3. Backward Compatibility: The changes are additive and do not break existing functionality.

4. Proper Error Handling: Enhanced error logging in async_props_emitter.rb with backtrace is helpful for debugging.

5. Correct Bundle Selection: The fix to force RSC bundle when async_props_block is present is crucial and well-commented.

---

CRITICAL Issues Found

1. Outdated Test Assertion

File: react_on_rails_pro/spec/react_on_rails_pro/async_props_emitter_spec.rb:23

The test on line 23 still expects the OLD code pattern:

• sharedExecutionContext.get("asyncPropsManager")

But the implementation NOW uses:

• ReactOnRails.getOrCreateAsyncPropsManager(sharedExecutionContext)

This test will FAIL! It needs to be updated to expect the new function call pattern.

2. Missing Test Coverage

The PR test plan mentions testing stream_react_component_with_async_props without sleep delays, but:

• Test checkboxes are unchecked
• No new integration test verifies the race condition is fixed
• The existing test file still has sleep 1 delays on lines 6 and 9

Recommendation: Add an integration test that calls stream_react_component_with_async_props with NO sleep delays and verifies async props are delivered correctly.

3. Type Safety Issue

File: packages/react-on-rails-pro/src/AsyncPropsManager.ts:158

The type assertion assumes the value is either AsyncPropsManager or undefined, but Map.get() could return ANY value if something else pollutes the key. Consider adding an instanceof check for defensive programming against accidental key collisions.

---

Security & Performance

Security:

• No SQL injection or XSS vulnerabilities introduced
• Proper error boundary handling
• Minor concern: Shared execution context pollution (addressed by type safety suggestion)

Performance:

• Lazy initialization reduces overhead
• O(1) Map lookup
• No memory leaks (manager lifecycle tied to request scope)

---

Required Actions Before Merge

1. Fix the failing test in async_props_emitter_spec.rb:23
2. Run the test suite locally: bundle exec rake run_rspec:dummy
3. Add integration test for the no-sleep scenario (the core bug being fixed)
4. Consider adding type guard in getOrCreateAsyncPropsManager
5. Update test plan checkboxes or explain why manual testing was sufficient

---

Status: Needs work - Fix the test assertion before merging. Otherwise, solid implementation of the race condition fix.

Generated with Claude Code