Refactor generateRSCPayload from global to parameter

[AbanoubGhadban]

Summary

Fix data leakage issue by passing generateRSCPayload as a parameter instead of defining it as a global function in the VM context.

Problem

Currently, generateRSCPayload is defined as globalThis.generateRSCPayload in the VM, which causes:

• Data leakage between concurrent requests sharing the same VM context
• Race conditions where Request A's component could use Request B's parameters
• Global state pollution that makes testing and debugging harder

Solution

Create generateRSCPayload as a local function inside the IIFE, then pass it as a parameter to the server rendering functions. This ensures:

• Request-scoped isolation (no global state)
• Proper parameter binding to the specific request
• Clean separation of concerns

Changes

• ✅ Changed globalThis.generateRSCPayload to local const in server_rendering_js_code.rb
• ✅ Pass generateRSCPayload as parameter to ReactOnRails rendering functions
• ✅ Update RSCRequestTracker to accept generateRSCPayload in constructor
• ✅ Update streamingUtils to extract and pass generateRSCPayload from options
• ✅ Add GenerateRSCPayloadFunction type and add to Params interface
• ✅ Update test fixtures to reflect new local function pattern

Files Changed

• lib/react_on_rails_pro/server_rendering_js_code.rb
• packages/react-on-rails/src/types/index.ts
• packages/react-on-rails-pro/src/RSCRequestTracker.ts
• packages/react-on-rails-pro/src/streamingUtils.ts
• packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js
• packages/node-renderer/tests/incrementalHtmlStreaming.test.ts

Tests

• ✅ Incremental HTML streaming tests (3/3 passed)
• ✅ serverRenderRSCReactComponent tests (7/7 passed)

Impact

This prevents concurrent requests from sharing state and eliminates potential race conditions where one request could use another's context.

🤖 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/generate-rsc-payload-parameter

---

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 - PR #2061: Refactor generateRSCPayload from global to parameter

Summary

This PR addresses a critical data leakage and race condition issue by converting generateRSCPayload from a global function to a request-scoped parameter. The refactoring is well-executed and significantly improves the safety of concurrent request handling.

---

✅ Strengths

1. Excellent Problem Identification

• The root cause analysis is spot-on: global state in VM contexts causes request isolation issues
• The solution properly addresses race conditions in concurrent request scenarios

2. Clean Architecture

• Converting to parameter passing maintains single responsibility principle
• Request-scoped isolation through constructor injection is the right pattern
• The RSCRequestTracker now owns its dependencies explicitly

3. Type Safety

• Added GenerateRSCPayloadFunction type provides clear contracts
• Optional parameter handling is appropriate (backward compatibility)
• TypeScript interfaces properly extended

4. Good Test Coverage

• Tests updated to reflect new pattern
• Re-enabled previously skipped test (incrementalHtmlStreaming.test.ts:164) - great catch!
• All 10 tests passing (3 incremental HTML + 7 serverRenderRSCReactComponent)

5. Documentation

• Removed outdated global function documentation from RSCRequestTracker.ts:8-22
• Updated error messages to be more helpful
• Clear comments in Ruby code explaining the IIFE parameter pattern

---

🔍 Potential Issues & Suggestions

1. Error Handling Edge Case (Minor)

Location: RSCRequestTracker.ts:114

if (!this.generateRSCPayload) {
  throw new Error(
    'generateRSCPayload is not defined. Please ensure that you are using at least version 4.0.0 of ' +
      'React on Rails Pro and the Node renderer, and that ReactOnRailsPro.configuration.enable_rsc_support ' +
      'is set to true.',
  );
}

Issue: The error message still references version requirements and configuration, but the actual issue could now be:

1. Function not passed as parameter
2. Incorrect parameter extraction in streamingUtils.ts
3. Configuration issue (as before)

Suggestion: Consider making the error message more specific:

if (!this.generateRSCPayload) {
  throw new Error(
    'generateRSCPayload function is not available. This could mean: ' +
    '(1) The function was not passed to RSCRequestTracker constructor, ' +
    '(2) ReactOnRailsPro.configuration.enable_rsc_support is not enabled, or ' +
    '(3) You are using an incompatible version of React on Rails Pro (requires 4.0.0+).'
  );
}

2. Undefined Check Pattern (Minor)

Location: server_rendering_js_code.rb:95

generateRSCPayload: typeof generateRSCPayload !== 'undefined' ? generateRSCPayload : undefined,

Observation: This is a safe pattern, but since generateRSCPayload is now a local const, it will always be defined in the IIFE scope (though it may not be defined in the RSC bundle context).

Suggestion: Consider adding a comment explaining when this would be undefined:

# generateRSCPayload is undefined when rendering on the RSC bundle (see line 22-26)
generateRSCPayload: typeof generateRSCPayload !== 'undefined' ? generateRSCPayload : undefined,

3. Type Consistency (Very Minor)

Location: types/index.ts:219-223

The GenerateRSCPayloadFunction type uses RailsContext as the third parameter, but RSCRequestTracker uses RailsContextWithServerComponentMetadata. This is fine (subtype), but worth noting for future type refinements.

4. Potential Memory Leak Prevention (Question)

Location: RSCRequestTracker.ts:41-46

The generateRSCPayload function is now stored as an instance property. Since RSCRequestTracker is request-scoped and garbage collected after the request, this is fine. However:

Question: Should the clear() method also null out the function reference to help GC?

clear(): void {
  // ... existing stream cleanup ...
  this.generateRSCPayload = undefined; // Help GC
}

This is likely premature optimization, but worth considering if memory profiling shows issues.

---

🛡️ Security Considerations

✅ No new security concerns introduced

• Eliminates global state pollution (security improvement)
• Prevents cross-request data leakage (major security win)
• No injection vulnerabilities in the JS code generation
• Proper escaping maintained in Ruby string interpolation

---

⚡ Performance Considerations

✅ Performance impact: Negligible to positive

• Parameter passing has minimal overhead
• Eliminates potential race conditions (improves reliability)
• No additional memory allocations per request
• Function binding happens once per request (acceptable)

---

🧪 Test Coverage Assessment

Current Coverage: Good ✅

• ✅ Incremental HTML streaming (3 tests)
• ✅ RSC component rendering (7 tests)
• ✅ Re-enabled previously failing test

Potential Gaps:

1. Concurrent request test: While the fix addresses concurrency issues, there's no explicit test that spawns multiple concurrent requests to verify isolation
2. Error path test: No test for when generateRSCPayload is undefined/null
3. Backward compatibility: No test ensuring old code paths still work

Recommendation: Add a test that verifies request isolation:

it('isolates generateRSCPayload between concurrent requests', async () => {
  // Spawn two requests with different generateRSCPayload implementations
  // Verify each request uses its own function, not the other's
});

---

📋 Code Quality

✅ Follows Best Practices

• Uses dependency injection pattern
• Avoids global state
• Maintains backward compatibility with optional parameters
• Clean separation of concerns
• Proper TypeScript typing

✅ Follows Repository Conventions

• Code style matches existing patterns
• Documentation updated appropriately
• Test fixtures updated consistently
• Follows the IIFE pattern used elsewhere in the codebase

---

🎯 Recommendations Summary

Required Changes: None ❌

This PR is ready to merge as-is.

Suggested Improvements (Optional):

1. 🟡 Low Priority: Enhance error message to reflect new parameter-based architecture
2. 🟡 Low Priority: Add comment explaining when generateRSCPayload is undefined
3. 🟢 Nice to Have: Add explicit concurrent request isolation test
4. 🟢 Nice to Have: Consider nulling function reference in clear() method

---

Final Verdict: ✅ APPROVE

This is a high-quality refactoring that:

• Fixes a real concurrency bug
• Improves code architecture
• Maintains backward compatibility
• Has good test coverage
• Follows best practices

The changes are well-thought-out and the implementation is clean. The optional suggestions above are minor improvements that could be addressed in follow-up PRs if desired.

Great work! 🎉

---

Review conducted using repository's CLAUDE.md guidelines

[claude]

Code Review: Refactor generateRSCPayload from global to parameter

Summary

This PR successfully addresses a critical data leakage and race condition issue by converting generateRSCPayload from a global function to a request-scoped parameter. The refactoring is well-executed and improves the architecture significantly.

---

✅ Strengths

1. Excellent Problem Identification & Solution

• Root Cause: Global state in VM context causing data leakage between concurrent requests
• Solution: Request-scoped function passing eliminates shared state
• Impact: Prevents race conditions where Request A could use Request B's parameters

2. Strong Type Safety

• Added GenerateRSCPayloadFunction type in packages/react-on-rails/src/types/index.ts:219-223
• Properly typed as optional parameter (generateRSCPayload?) throughout the chain
• Type definitions are clear and self-documenting

3. Backward Compatibility

• Graceful handling when generateRSCPayload is undefined
• Error messages guide users on version requirements and configuration
• Guard: typeof generateRSCPayload !== 'undefined' ? generateRSCPayload : undefined

4. Test Coverage Improvements

• MAJOR WIN: Re-enabled previously skipped test (incrementalHtmlStreaming.test.ts:161)
  • Comment changed from TODO: fix the problem of having a global shared runOnOtherBundle function to test now passing
  • This validates the fix actually solves the concurrent request isolation issue
• Added handleRequestClosed assertions in multiple tests
• Converted to unique test configs to prevent race conditions in test suite itself

5. Clean Architecture

• Constructor injection pattern in RSCRequestTracker (line 41-46)
• Dependency explicitly passed through the call chain
• Clear separation of concerns

---

🔍 Potential Issues & Suggestions

1. Error Message Clarity (Minor - Suggestion)

In RSCRequestTracker.ts:115-119, consider adding a third scenario to the error message: '(3) The function was not properly passed through the rendering options chain.' This would help developers debug integration issues.

2. Test Config Duplication (Minor - Observation)

In testingNodeRendererConfigs.ts, the new createTestConfig function creates isolated bundle paths per test file. Verify that CI cleanup properly removes all tmp/node-renderer-bundles-test-* directories to prevent accumulation.

3. Missing Type Import Location Check (Minor)

In RSCRequestTracker.ts:20, verify the import from 'react-on-rails/types' doesn't create circular dependencies between Pro and OSS packages.

4. Ruby String Interpolation Edge Case (Low Priority)

In server_rendering_js_code.rb:43, consider testing with component props containing special characters (quotes, backticks, newlines) to verify JSON.stringify handles edge cases.

---

🔒 Security Considerations

✅ No Issues Found

• Isolation: Request-scoped function prevents cross-request data leakage
• No XSS risk: JSON.stringify(props) properly escapes user input
• No injection risk: Function is created in controlled server context

---

⚡ Performance Considerations

✅ Positive Impact

• No performance degradation: Function creation is negligible overhead
• Memory: Functions are request-scoped and garbage collected automatically
• Concurrency: Eliminates potential mutex/locking needs that global state might have required

---

📝 Test Coverage Assessment

✅ Excellent Coverage

• Unit tests: 7/7 serverRenderRSCReactComponent tests passing
• Integration tests: 3/3 incremental HTML streaming tests passing
• Concurrency: Tests verify isolated request handling
• Re-enabled test: The previously skipped test now passing validates the core fix

Suggested Additional Tests (Optional)

1. Concurrent request test: Explicitly test two simultaneous requests with different props
2. Props with special characters: Test component names/props with quotes, backticks, newlines
3. Missing function graceful degradation: Verify error message when enable_rsc_support = false

---

🎯 Code Quality

✅ Follows Best Practices

• SOLID principles: Dependency injection, single responsibility
• Immutability: No global state mutation
• Type safety: Proper TypeScript typing throughout
• Error handling: Appropriate try-catch and validation
• Testing: Tests updated to reflect new behavior

---

🎉 Overall Assessment

Rating: ⭐⭐⭐⭐⭐ Excellent

This is a high-quality refactoring that:

• Solves a real concurrency bug
• Improves architecture and maintainability
• Maintains backward compatibility
• Has comprehensive test coverage
• Follows TypeScript and Ruby best practices

The fact that a previously skipped test now passes is strong evidence the fix works as intended.

Recommendation: ✅ APPROVE - Ready to merge after confirming lint checks pass.

---

📋 Checklist

• ✅ Code quality and best practices
• ✅ No bugs or critical issues found
• ✅ Performance improvements (eliminates race conditions)
• ✅ No security concerns
• ✅ Excellent test coverage
• ⚠️ Run bundle exec rubocop per CLAUDE.md requirements
• ⚠️ Ensure trailing newlines in all modified files

---

Reviewed by: Claude Code

[claude]

Code Review: Refactor generateRSCPayload from global to parameter

Summary

This PR addresses a critical concurrency issue by eliminating global state in the RSC payload generation system. The refactoring is well-architected and significantly improves request isolation. ✅

---

Strengths

1. Excellent Problem Identification & Solution 🎯

• Root Cause: Correctly identified that globalThis.generateRSCPayload caused data leakage between concurrent requests sharing the same VM context
• Solution: Converting to a request-scoped parameter passed through the call chain is the right architectural approach
• Impact: This prevents race conditions where Request A could inadvertently use Request B parameters

2. Type Safety 💪

Clear type definition added to types/index.ts with proper integration into Params interface and optional parameter handling with undefined checks.

3. Comprehensive Test Coverage ✅

• Enabled previously skipped test for error handling
• New concurrent test verifies 5 parallel requests with request-specific data
• Test isolation via createTestConfig pattern prevents interference
• All existing tests pass

4. Clean Implementation 🏗️

• Ruby side: Changed from globalThis.generateRSCPayload to local const
• TypeScript side: Constructor injection in RSCRequestTracker, extracted from options in streamingUtils
• Bound to instance, not global scope

5. Backwards Compatibility 🔄

• Optional parameter handling ensures graceful degradation
• Clear error messages guide users on configuration requirements

---

Code Quality Observations

Positive:

1. Improved error messages explaining configuration requirements
2. Good inline documentation of architecture decisions
3. Excellent testing infrastructure with createTestConfig refactor

Minor notes:

1. props: unknown type is defensible for flexibility
2. Test configuration per-file is good for isolation

---

Security & Performance

✅ Security: No concerns - removing global state improves security by preventing data leakage
✅ Performance: Neutral to positive - negligible overhead, prevents race conditions

---

Potential Issues & Suggestions

1. Cleanup verification (Question)

Is RSCRequestTracker.clear() called explicitly in finally blocks, or does it rely on GC? Explicit cleanup is safer for long-running processes.

2. Test cleanup (Minor)

Consider adding afterAll hooks to clean up tmp directories created by createTestConfig to prevent bloat during development.

3. Stricter validation (Enhancement)

Could add early validation in streamingUtils.ts when extracting generateRSCPayload from options, though current lazy validation is acceptable.

---

Testing Recommendations

Already covered: ✅ Concurrent requests, data isolation, error cases

Optional additions:

1. Memory leak verification for tracker GC
2. Edge case: generateRSCPayload throwing during concurrent requests
3. Performance benchmark with 50+ concurrent requests

---

Changelog & Documentation

✅ REQUIRED: Update /react_on_rails_pro/CHANGELOG.md per CLAUDE.md guidelines

This is user-visible because it fixes critical data leakage in RSC with streaming.

Suggested entry:

#### Bug Fixes
- [PR 2061](https://github.com/shakacode/react_on_rails/pull/2061) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Fixed critical data leakage issue in React Server Components where concurrent requests could share state due to global generateRSCPayload function. Refactored to use request-scoped parameters for proper isolation.

---

Final Verdict

✅ APPROVE with minor suggestions

This is high-quality work that:

1. ✅ Fixes critical concurrency bug
2. ✅ Improves architecture by eliminating global state
3. ✅ Maintains backwards compatibility
4. ✅ Has comprehensive test coverage
5. ✅ Follows project conventions

Before merge:

1. Update /react_on_rails_pro/CHANGELOG.md (required)
2. Consider test cleanup hooks (optional)
3. Verify CI passes

Great work! 🎉