Add AsyncPropManager to react-on-rails-pro package

[AbanoubGhadban]

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/update test to cover these changes
• Update documentation
• Update CHANGELOG file

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.

[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 implement-async-props-manager-at-react-on-rails-pro-package

---

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: Add AsyncPropManager to react-on-rails-pro package

Thank you for this contribution! I've reviewed the PR and have identified several issues that need to be addressed before merging.

---

🚨 Critical Issues

1. Typo in Error Message (packages/react-on-rails-pro/src/AsyncPropsManager.ts:89)

There's a typo in the error message:

`The async prop "${propName}" is not received. Esnure to send the async prop from ruby side`

Should be: **Ensure** not Esnure

2. Missing Implementation of resolved Flag

The resolved flag is declared in the PromiseController type and initialized to false, but it's never set to true when a promise is resolved. This means:

• The endStream() method won't work correctly - it checks !promiseController.resolved but this will always be true
• You should set resolved = true in the setProp method after calling resolve()

Fix needed in setProp():

setProp(propName: string, propValue: unknown) {
  const promiseController = this.getOrCreatePromiseController(propName);
  if (!promiseController) {
    throw new Error(`Can't set the async prop "${propName}" because the stream is already closed`);
  }

  promiseController.resolve(propValue);
  promiseController.resolved = true; // <-- ADD THIS LINE
}

3. Incomplete Pull Request Checklist

The PR description shows that none of the checklist items are completed:

• Add/update test to cover these changes ✅ Tests are present but see testing gaps below
• Update documentation ❌ No documentation added
• Update CHANGELOG file ❌ Not updated (per CLAUDE.md, this should be done for user-visible features)

---

📋 Required Actions Before Merge

1. Fix the typo in the error message (line 89)
2. Set resolved = true in the setProp() method
3. Update CHANGELOG.md following the project conventions (see CLAUDE.md for format)
4. Add documentation explaining:
   • What AsyncPropsManager is used for
   • How to use it (API documentation)
   • When to use it vs other approaches
5. Mark checklist items as complete in the PR description

---

🧪 Testing Concerns

Missing Test Coverage

While the existing tests are comprehensive for basic scenarios, consider adding:

1. Test for the resolved flag behavior - Verify that once a prop is resolved, endStream() doesn't reject it
2. Edge case: Multiple setProp calls - What happens if you call setProp('foo', 'bar') twice with different values? Should document expected behavior
3. Edge case: Setting undefined or null values - Are these valid prop values?

Test File Organization

The test file has good structure with descriptive test suites. Good work on covering:

• Access before setting ✅
• Access after setting ✅
• Access after closing ✅
• Non-existent props ✅
• Complex scenarios ✅

---

💡 Design & Architecture

Positive Aspects

• Good separation of concerns - Clear responsibility for managing async props
• Promise reuse - Smart design ensuring the same promise is returned for multiple calls (prevents rendering issues)
• Clear error messages - Helpful error messages for debugging
• Proper licensing header - Follows project conventions

Suggestions

1. Consider exporting the type PromiseController if it might be useful for consumers (or document why it's private)

2. Memory management: Once a promise is resolved, should we remove it from the map to prevent memory leaks in long-running applications? Or is the expectation that each manager instance is short-lived?

3. API Design Question: Should getProp allow requesting a prop after endStream() has been called if the prop was already set? Current behavior allows this, which seems reasonable, but worth documenting explicitly.

---

🎨 Code Style

The code follows the project's TypeScript conventions well:

• ✅ Proper type annotations
• ✅ Clear naming conventions
• ✅ Consistent with other Pro package files (similar to CallbackRegistry)
• ✅ Good use of private methods

---

🔒 Security

No security concerns identified. The manager handles data passing without executing arbitrary code.

---

⚡ Performance

No performance concerns. The use of a Map for lookups is efficient, and promise reuse prevents unnecessary work.

---

📝 Summary

This is a solid implementation with good test coverage, but requires:

1. Fix the critical bug with the resolved flag not being set
2. Fix the typo in the error message
3. Complete the PR checklist (CHANGELOG, documentation)

Once these issues are addressed, this will be ready to merge. The core logic is sound and well-tested!

---

Next Steps:

1. Fix the resolved flag bug
2. Fix the typo
3. Update CHANGELOG.md using the format: [PR 2049](https://github.com/shakacode/react_on_rails/pull/2049) by [AbanoubGhadban](https://github.com/AbanoubGhadban)
4. Add documentation (either inline JSDoc or separate docs)
5. Update PR description to check off completed items

Let me know if you have questions about any of these suggestions!

[claude]

Code Review for PR #2049: Add AsyncPropManager to react-on-rails-pro package

Summary

This PR introduces an AsyncPropsManager class to support incremental/streaming async props in React Server Components (RSC). The implementation allows React components to request props that will be provided asynchronously via a streaming mechanism.

---

Issues Found

🔴 Critical Issues

1. Typo in Error Message (AsyncPropsManager.ts:93)

"Esnure to send the async prop from ruby side"

Issue: "Esnure" should be "Ensure"

Impact: User-facing error message has a spelling mistake.

---

2. Missing resolved Flag Update (AsyncPropsManager.ts:45)

setProp(propName: string, propValue: unknown) {
  const promiseController = this.getOrCreatePromiseController(propName);
  if (!promiseController) {
    throw new Error(`Can't set the async prop "${propName}" because the stream is already closed`);
  }

  promiseController.resolve(propValue);
}

Issue: After calling promiseController.resolve(propValue), the resolved flag is never set to true. This means in endStream(), already-resolved promises might be rejected again.

Impact:

• The endStream() method checks !promiseController.resolved to decide whether to reject pending promises
• Without setting this flag, resolved props could be incorrectly rejected when the stream ends
• This could cause race conditions where a prop is successfully resolved but then gets rejected

Fix: Add promiseController.resolved = true; after resolving:

setProp(propName: string, propValue: unknown) {
  const promiseController = this.getOrCreatePromiseController(propName);
  if (!promiseController) {
    throw new Error(`Can't set the async prop "${propName}" because the stream is already closed`);
  }

  promiseController.resolve(propValue);
  promiseController.resolved = true; // Add this line
}

---

3. Debug Logging Left in Production Code (worker.ts:325)

log.info(`Received a new update chunk ${JSON.stringify(obj)}`);

Issue: This appears to be debug logging that should be removed or downgraded to log.debug().

Impact:

• Will clutter production logs with potentially large JSON objects
• May contain sensitive data in props
• Performance impact from JSON stringification on every chunk

Fix: Remove or change to log.debug()

---

🟡 Design & Best Practices Concerns

4. Type Safety: Use of unknown vs Generics

The AsyncPropsManager uses unknown for prop values rather than leveraging TypeScript generics.

Current:

getProp(propName: string): Promise<unknown>
setProp(propName: string, propValue: unknown): void

Consideration: While the type safety is achieved at the component level via WithAsyncProps, the manager itself doesn't enforce type safety between setProp and getProp calls. This is likely acceptable given the runtime nature of the streaming, but worth noting.

---

5. Promise Rejection vs Error Throwing Inconsistency

• getProp() returns Promise.reject() when stream is closed
• setProp() throws an error when stream is closed

Consideration: This inconsistency might be intentional (getProp is async, setProp is sync), but it creates different error handling patterns. Consider documenting this behavior or making it consistent.

---

6. Memory Leak Potential

Once promises are created and stored in propNameToPromiseController, they're never cleaned up even after resolution.

Impact: For long-lived instances or many props, this could accumulate memory. However, since this appears to be per-component/render lifecycle, it's likely acceptable.

Suggestion: Consider adding a clear() or dispose() method for explicit cleanup if needed.

---

🟢 Testing

Strengths:

• ✅ Comprehensive test coverage with multiple scenarios
• ✅ Tests cover edge cases (access before/after setting, stream closure, multiple props)
• ✅ Good test organization with descriptive test names

Gaps:

• ❌ Missing test for the critical bug: No test verifies that already-resolved props are NOT rejected when endStream() is called
• ❌ Missing concurrency tests: What happens if setProp() is called multiple times for the same prop?
• ❌ Missing error handling tests: What happens if a promise consumer throws an error?

Suggested Additional Test:

it('does not reject already resolved props when stream ends', async () => {
  const manager = new AsyncPropsManager();
  const getPropPromise = manager.getProp('resolvedProp');
  manager.setProp('resolvedProp', 'value');
  await expect(getPropPromise).resolves.toBe('value');
  
  manager.endStream(); // Should not affect already resolved promise
  await expect(getPropPromise).resolves.toBe('value'); // Should still resolve
});

---

📝 PR Checklist Issues

The PR template checklist is still incomplete:

• Add/update test to cover these changes - Partially done (new tests added but gaps exist)
• Update documentation - Not done (no documentation for this new feature)
• Update CHANGELOG - Not done

Per CLAUDE.md, the CHANGELOG should document user-visible features with proper formatting.

---

🔒 Security Concerns

Low Risk:

The log.info(JSON.stringify(obj)) in worker.ts could potentially log sensitive data. Ensure props don't contain passwords, tokens, or PII, or sanitize before logging.

---

⚡ Performance Considerations

1. Map Lookups: The use of Map for prop storage is efficient - good choice ✅
2. Promise Creation: Promises are created lazily only when needed - good ✅
3. JSON Stringification: The debug logging adds unnecessary overhead (see issue Add linting and CI scripts #3)

---

🎯 Recommendations

Must Fix Before Merge:

1. Fix the resolved flag bug in setProp()
2. Fix the typo in error message
3. Remove or downgrade the debug logging in worker.ts

Should Fix:
4. Add test for resolved props not being rejected on stream end
5. Update CHANGELOG per repository conventions
6. Add documentation explaining the async props feature

Nice to Have:
7. Add concurrency tests
8. Consider adding JSDoc comments to public methods
9. Document the error handling inconsistency between getProp and setProp

---

✅ Positives

1. Clean API Design: The WithAsyncProps type and integration is elegant
2. Good Test Coverage: Most scenarios are well-tested
3. Type Safety: Good use of TypeScript generics at the component level
4. Integration: Well-integrated into the existing ReactOnRails architecture
5. Error Messages: Clear, actionable error messages (minus the typo)

---

📋 Summary

This is a solid implementation of async props streaming functionality. The core logic is sound, but there's a critical bug with the resolved flag that must be fixed before merging. Once the critical issues are addressed, this will be a valuable addition to the Pro package.

Recommendation: Request changes for the critical bugs, then approve after fixes.

[claude]

Code Review

Overview

This PR adds an AsyncPropsManager to handle asynchronous prop delivery in React Server Components for the Pro package. The implementation looks solid overall with good test coverage, but there are several issues that should be addressed before merging.

---

Critical Issues

1. Typo in Error Message 🐛

// packages/react-on-rails-pro/src/AsyncPropsManager.ts:92
return new Error(
  `The async prop "${propName}" is not received. Esnure to send the async prop from ruby side`,
);

Issue: "Esnure" should be "Ensure"

Impact: User-facing error message contains a typo

Fix: Change "Esnure" → "Ensure"

---

2. Debugging Code Left in Production 🚨

// react_on_rails_pro/packages/node-renderer/src/worker.ts:325
log.info(`Received a new update chunk ${JSON.stringify(obj)}`);

Issue: This debug logging should not be committed to the main codebase. It will:

• Create noise in production logs
• Potentially log sensitive data
• Impact performance by stringifying large objects

Fix: Remove this line or make it conditional based on a debug flag

---

3. Incomplete Test Implementation ⚠️

// react_on_rails_pro/packages/node-renderer/tests/incrementalHtmlStreaming.test.ts:99-103
// it("uploads the bundles", async () => {
//   const { status, body } = await makeRequest();
//   expect(body).toBe("");
//   expect(status).toBe(200);
// });

Issue: This test is commented out. If it is needed, uncomment it. If not, remove it.

Fix: Either uncomment and ensure it passes, or remove it entirely

---

4. Excessive Console Logging in Tests 📝

// react_on_rails_pro/packages/node-renderer/tests/incrementalHtmlStreaming.test.ts:107-121
console.log("starting");
// ...
request.on("response", (headers) => {
  console.log(headers);
});

request.on("data", (data: Buffer) => {
  console.log(data.toString());
});

Issue: Tests contain multiple console.log statements that should be removed or replaced with proper assertions

Fix: Remove console.log statements and add proper expect() assertions to verify the response

---

Code Quality Issues

5. Missing Assertions in Test ❌

The incrementalHtmlStreaming.test.ts test does not verify the actual HTML output or response status. It only logs data but does not assert expectations.

Fix: Add assertions like:

let responseStatus: number | undefined;
let responseData = "";

request.on("response", (headers) => {
  responseStatus = headers[":status"];
});

request.on("data", (data: Buffer) => {
  responseData += data.toString();
});

// After request completes:
expect(responseStatus).toBe(200);
expect(responseData).toContain("expected content");

---

6. Type Safety Concern 🔒

// packages/react-on-rails-pro/src/AsyncPropsManager.ts:74-75
let resolvePromise: PromiseController["resolve"] = () => {};
let rejectPromise: PromiseController["reject"] = () => {};

Observation: While this pattern works, the empty function initializers could be avoided with a more explicit Promise executor pattern:

let resolvePromise\!: PromiseController["resolve"];
let rejectPromise\!: PromiseController["reject"];

This uses the definite assignment assertion to indicate these will be assigned before use.

---

Positive Aspects ✅

1. Excellent Test Coverage: The AsyncPropsManager has comprehensive unit tests covering edge cases:

   • Props accessed before setting
   • Props accessed after setting
   • Stream closure scenarios
   • Multiple prop handling
   • Error cases

2. Good TypeScript Usage: Strong typing with generics for WithAsyncProps and addAsyncPropsCapabilityToComponentProps

3. Clean API Design: The getProp/setProp/endStream interface is intuitive

4. Promise Reuse: Correctly returns the same promise instance to prevent re-renders (line 17-18 comments explain this well)

5. Proper State Management: The isClosed flag and resolved tracking prevent invalid operations

6. Error Handling: Good error messages (except for the typo) for debugging

---

Performance Considerations

Memory Management

The propNameToPromiseController Map will hold references to all props until stream closure. For components with many props, this is fine, but consider:

• Is there a maximum number of props expected?
• Should there be cleanup for resolved props in long-running streams?

Current implementation is acceptable for typical use cases, but worth documenting limitations if any.

---

Security Considerations

1. No Security Issues Detected: The implementation does not expose any obvious security vulnerabilities
2. Input Validation: The code does not validate prop names, but this is reasonable since they come from controlled sources (Ruby backend)
3. Error Messages: Error messages do not leak sensitive information

---

Documentation & Changelog

Based on CLAUDE.md guidelines:

CHANGELOG Entry Needed: This is a user-visible feature (async props support for RSC)

Suggested CHANGELOG entry:

### Added
- [PR 2049](https://github.com/shakacode/react_on_rails/pull/2049) by [contributor](https://github.com/contributor): Add AsyncPropsManager for incremental prop delivery in React Server Components streaming

---

Recommendations Before Merging

Must Fix:

1. Fix the "Esnure" → "Ensure" typo
2. Remove debug logging from worker.ts
3. Remove console.log statements from tests and add proper assertions
4. Either uncomment or remove the commented test

Should Fix:
5. Add meaningful assertions to incrementalHtmlStreaming.test.ts
6. Update CHANGELOG.md

Nice to Have:
7. Consider the definite assignment pattern for promise callbacks
8. Add JSDoc comments to public API methods

---

Testing Checklist

Before merging, ensure:

• bundle exec rubocop passes with zero offenses
• yarn run lint passes
• All tests pass: rake run_rspec
• JS tests pass: yarn run test
• Manual testing of the async props functionality in Pro dummy app

---

Great work on implementing this feature! The core implementation is solid. Just needs cleanup of debug code and test assertions before it is ready to merge. 🚀

[claude]

Code Review: AsyncPropsManager Implementation

I've reviewed PR #2049 which adds AsyncPropsManager to the react-on-rails-pro package. Here's my detailed feedback:

---

🎯 Overall Assessment

This is a well-architected implementation of an async props management system. The code quality is generally high with good test coverage. However, there are several important issues that should be addressed before merging.

---

🐛 Critical Issues

1. Typo in Error Message (AsyncPropsManager.ts:93)

Change 'Esnure' to 'Ensure' in the error message.

2. Missing resolved Flag Update

In setProp, the resolved flag is never set to true. This could cause issues in endStream. Add: promiseController.resolved = true after calling resolve.

3. Memory Leak Risk

The propNameToPromiseController Map never clears resolved promises. In long-lived streams with many props, this could accumulate memory. Document the memory management strategy or consider cleanup.

---

Test Coverage Issues

4. Remove Console.log Statements

The incrementalHtmlStreaming.test.ts has multiple console.log statements (lines 107, 116, 120) that should be removed.

5. Commented Test Code

Line 99 in incrementalHtmlStreaming.test.ts has a commented test. Either enable it or remove it.

---

Design Concerns

6. Log Level in Production Code

In worker.ts:325, consider changing log.info to log.debug for the update chunk logging, as this could be noisy and JSON.stringify could be expensive.

---

PR Checklist

Based on the PR template and CLAUDE.md:

• Tests exist but need cleanup (remove console.logs)
• No CHANGELOG entry (should add per CLAUDE.md format)
• No documentation updates

---

What's Good

1. Excellent promise caching - returning same promise for multiple getProp calls
2. Clean separation of concerns
3. Comprehensive test suite covering set-before-get and get-before-set scenarios
4. Good TypeScript type safety with WithAsyncProps
5. Proper error handling
6. Well-designed integration pattern

---

Before Merging - Must Fix

1. Fix typo Esnure to Ensure
2. Add promiseController.resolved = true in setProp
3. Remove console.log statements from tests
4. Remove or enable commented test
5. Add CHANGELOG entry

Great work overall! The implementation is solid, just needs these fixes before merging.

[claude]

Code Review for PR #2049: Add AsyncPropManager to react-on-rails-pro package

Summary

This PR introduces an AsyncPropsManager class to handle asynchronous props in React Server Components (RSC), enabling components to receive props that resolve over time via streaming. The implementation appears well-structured with comprehensive test coverage.

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive test suite covering all major scenarios (142 lines)
• Tests cover edge cases: accessing before/after setting, stream closure, error scenarios
• Good use of descriptive test names that explain the behavior

2. Type Safety

• Strong TypeScript typing with generic constraints
• WithAsyncProps type utility provides good DX for component authors
• Proper type definitions exported from the main types file

3. Architecture

• Clean separation of concerns
• Promise-based API is idiomatic for async operations
• Proper error handling for closed streams and missing props

---

🔴 Critical Issues

1. Typo in Error Message (AsyncPropsManager.ts:93)

// Line 93 - TYPO: "Esnure" should be "Ensure"
`The async prop "${propName}" is not received. Esnure to send the async prop from ruby side`

Fix:

`The async prop "${propName}" is not received. Ensure the async prop is sent from the Ruby side`

2. Missing resolved Flag Update (AsyncPropsManager.ts:44)

The setProp method resolves the promise but never sets promiseController.resolved = true. This could lead to:

• Incorrect behavior in endStream() (line 54) which checks !promiseController.resolved
• Potential double-rejection of already-resolved promises

Fix:

setProp(propName: string, propValue: unknown) {
  const promiseController = this.getOrCreatePromiseController(propName);
  if (!promiseController) {
    throw new Error(`Can't set the async prop "${propName}" because the stream is already closed`);
  }

  promiseController.resolve(propValue);
  promiseController.resolved = true; // ADD THIS LINE
}

3. Test Does Not Verify the Bug (AsyncPropManager.test.ts)

The existing tests do not catch the missing resolved = true flag because they do not test calling endStream() after setProp().

Add this test case:

describe('endStream behavior after setProp', () => {
  it('should not reject already-resolved props when endStream is called', async () => {
    const manager = new AsyncPropsManager();
    const propPromise = manager.getProp('myProp');
    manager.setProp('myProp', 'value');
    manager.endStream(); // This should NOT reject 'myProp'
    
    await expect(propPromise).resolves.toBe('value');
  });
});

---

⚠️ High Priority Issues

4. Debug Logging Left in Production Code (worker.ts:325)

log.info(`Received a new update chunk ${JSON.stringify(obj)}`);

• This logs every incremental update, which could be noisy in production
• Consider changing to log.debug() or adding conditional logging
• If this is intentional for debugging the new feature, add a TODO comment

5. Test File Named Inconsistently

• File: AsyncPropManager.test.ts (singular "Prop")
• Class: AsyncPropsManager (plural "Props")
• Should be: AsyncPropsManager.test.ts

6. Incomplete Test in incrementalHtmlStreaming.test.ts

// Line 99-103: Commented out test
// it('uploads the bundles', async () => {
//   const { status, body } = await makeRequest();
//   expect(body).toBe('');
//   expect(status).toBe(200);
// });

• Either remove or uncomment this test
• If intentionally disabled, add a comment explaining why

---

🟡 Medium Priority Issues

7. Generic Type Constraint Could Be More Flexible

// Current signature requires Record<string, unknown>
addAsyncPropsCapabilityToComponentProps<
  AsyncPropsType extends Record<string, unknown>,
  PropsType extends Record<string, unknown>,
>

Consider allowing {} or object types that might not strictly be Records. However, this may be intentional - verify the constraint is necessary.

8. Missing PR Checklist Items

The PR description shows unchecked boxes:

• Add/update test to cover these changes ✅ (Done, but uncheck)
• Update documentation ❓ (Needed?)
• Update CHANGELOG file ❓ (Needed per CLAUDE.md?)

Per CLAUDE.md:

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

This is a new feature - CHANGELOG update is required. Use /update-changelog command.

9. Potential Memory Leak Risk

The propNameToPromiseController Map grows indefinitely. For long-running streams with many props:

• Consider adding a clear() method called after endStream()
• Or document that the manager should be discarded after use

10. Example Component Has Unused React Import

// AsyncPropsComponent.tsx:3
import * as React from 'react';
import { Suspense } from 'react';

If React is not used directly (JSX transform handles it), remove the import.

---

🔵 Low Priority / Suggestions

11. Type Assertions Could Be Avoided

In ReactOnRailsRSC.ts:116, the type assertion might be avoidable with better generics, but current approach is acceptable.

12. Test Timing Reliance

incrementalHtmlStreaming.test.ts uses hardcoded timeouts (1000ms, 1500ms, 100ms). Consider:

• Using more reliable synchronization mechanisms if possible
• Adding comments explaining why these specific timings are needed
• Potential flakiness in slower CI environments

13. Console.log in Test File

// incrementalHtmlStreaming.test.ts:107, 117, 120
console.log('starting');
console.log(headers);
console.log(data.toString());

Remove or convert to proper test logging/assertions.

14. Documentation Gaps

Add JSDoc comments to:

• AsyncPropsManager class (explaining use case and lifecycle)
• addAsyncPropsCapabilityToComponentProps function (usage example)
• The resolved flag purpose in PromiseController

15. TypeScript Migration

testingNodeRendererConfigs.js → testingNodeRendererConfigs.ts migration is good! Consider adding stricter types if Config allows it.

---

🔒 Security Considerations

✅ No obvious security issues identified. The code:

• Properly isolates async prop resolution per manager instance
• Does not expose internal state unsafely
• Has appropriate error handling for malformed inputs

---

⚡ Performance Considerations

1. Promise Creation Overhead: Minimal - promises are created lazily
2. Map Lookups: O(1) average case - appropriate data structure choice
3. Memory: See issue Support configuration options #9 about potential memory accumulation

---

📋 Action Items Summary

Must Fix Before Merge:

1. ✅ Fix typo: "Esnure" → "Ensure" (AsyncPropsManager.ts:93)
2. ✅ Set promiseController.resolved = true in setProp()
3. ✅ Add test case for endStream() after setProp()
4. ✅ Rename test file: AsyncPropManager.test.ts → AsyncPropsManager.test.ts
5. ✅ Update CHANGELOG.md using /update-changelog

Should Fix:

6. Remove or explain commented test in incrementalHtmlStreaming.test.ts
7. Remove console.log statements from tests
8. Decide on debug logging in worker.ts:325

Consider:

9. Add documentation/JSDoc comments
10. Document memory management expectations
11. Test stability improvements (timing-based tests)

---

📝 Additional Notes

• RSC Integration: The integration with the existing RSC rendering pipeline looks clean
• Backward Compatibility: Proper error messages in open-source bundle for Pro-only features
• Code Style: Follows existing patterns in the codebase well

---

Overall Assessment: Strong implementation with comprehensive tests. The core logic is sound, but there is a critical bug (#2) that needs fixing before merge. With the identified fixes, this will be a solid addition to the Pro package.

Great work on the test coverage and type safety! 🎉

[claude]

Code Review: AsyncPropsManager Implementation

Strengths

1. Well-tested implementation with comprehensive test coverage
2. Good TypeScript typing with proper generic types
3. Promise stability - correctly returns same promise instance
4. Clean separation - Pro-only feature properly gated

Critical Issues

1. Typo (AsyncPropsManager.ts:93): Esnure should be Ensure

2. Missing resolved flag (AsyncPropsManager.ts:45): The resolved flag is NEVER set to true after resolving. This is critical because in endStream() even resolved promises might get rejected. Fix: Add promiseController.resolved = true before calling resolve.

3. Type validation bug (handleIncrementalRenderRequest.ts:52): Validation logic appears incorrect for onRequestClosedUpdateChunk. Type says string but validation checks for object.

Other Issues

• Memory leak: Map never cleared, consider clearing in endStream
• No guard against multiple setProp calls on same property
• Debug log at worker.ts:325 might be noisy

Missing per CLAUDE.md

• CHANGELOG entry (required for features)
• Documentation updates
• Complete PR description

Required Before Merge

1. Fix typo
2. CRITICAL: Add resolved = true in setProp
3. Fix type validation
4. Add CHANGELOG
5. Run bundle exec rubocop

Great architecture overall! Please address the critical resolved flag bug.