Skip to content

Conversation

@AbanoubGhadban
Copy link
Collaborator

@AbanoubGhadban AbanoubGhadban commented Oct 29, 2025

Summary

Solves #1886

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.


This change is Reviewable

Summary by CodeRabbit

  • Refactor
    • Restructured server-side rendering streaming infrastructure for improved modularity and reduced complexity while maintaining public API compatibility.
    • Reorganized error handling module to enhance code organization.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Walkthrough

The PR refactors React streaming infrastructure by introducing a new streamingUtils.ts module that consolidates streaming orchestration, buffering, and error handling logic. The existing streamServerRenderedReactComponent.ts is simplified to delegate core streaming work to this new module, while ReactOnRailsRSC.ts updates re-export paths. Additionally, the handleError export is redirected to resolve from generateRenderingErrorMessage.js across the codebase.

Changes

Cohort / File(s) Summary
Streaming utilities consolidation
packages/react-on-rails-pro/src/streamingUtils.ts (new)
Introduces new module providing bufferStream function for buffering events, transformRenderStreamChunksToResultObject for converting render chunks to JSON, and streamServerRenderedComponent for orchestrating per-request streaming with tracker creation, error handling, and RSC payload injection. Exports StreamingTrackers type.
Streaming orchestration refactor
packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
Simplifies implementation to delegate core streaming to streamingUtils.ts. Removes exported StreamRenderer, StreamingTrackers, and streamServerRenderedComponent. Retains public API (default export) but now internally uses streamRenderReactComponent and delegates to streamingUtils.streamServerRenderedComponent.
Streaming re-export paths
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
Updates import sources for three re-exports from streamServerRenderedReactComponent.ts to streamingUtils.ts, maintaining export names.
Error handling export redirection
packages/react-on-rails/package.json
Changes ./handleError export alias to resolve to ./lib/generateRenderingErrorMessage.js instead of ./lib/handleError.js.
Import path updates
packages/react-on-rails/src/base/full.ts,
packages/react-on-rails/src/serverRenderReactComponent.ts
Updates import source of handleError from ../handleError.ts to ../generateRenderingErrorMessage.ts (and ./generateRenderingErrorMessage.ts respectively) to align with package export changes.

Sequence Diagram

sequenceDiagram
    participant Client
    participant StreamServerRenderedReactComponent
    participant StreamingUtils
    participant RenderEngine as React Render Engine
    participant BufferStream
    
    Client->>StreamServerRenderedReactComponent: render(options)
    
    StreamServerRenderedReactComponent->>StreamingUtils: streamServerRenderedComponent(options, renderStrategy)
    
    StreamingUtils->>StreamingUtils: Create PostSSRHookTracker & RSCRequestTracker
    StreamingUtils->>StreamingUtils: Augment Rails context with streaming capabilities
    
    StreamingUtils->>RenderEngine: renderToPipeableStream(component)
    RenderEngine->>RenderEngine: Render component
    
    alt Streaming Success
        RenderEngine->>StreamingUtils: emit chunks (shell ready, content)
        StreamingUtils->>StreamingUtils: transformRenderStreamChunksToResultObject(chunks)
        StreamingUtils->>StreamingUtils: injectRSCPayload(payload)
        StreamingUtils->>BufferStream: Buffer transformed chunks
        BufferStream->>Client: Stream results
    else Error Occurred
        RenderEngine->>StreamingUtils: emit error
        StreamingUtils->>StreamingUtils: handleError(construct error HTML)
        StreamingUtils->>BufferStream: emitError(errorHTML)
        BufferStream->>Client: Stream error response
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • bufferStream implementation – Verify event buffering logic, replay order, and error propagation mechanics, particularly ensuring events are correctly sequenced once reading begins
  • transformRenderStreamChunksToResultObject – Confirm JSON serialization of render chunks, console replay integration, and render state reset logic
  • streamServerRenderedComponent orchestration – Validate per-request tracker lifecycle, Rails context augmentation, registry lookups, error handling paths, and stream cleanup semantics
  • Delegation correctness – Ensure streamServerRenderedReactComponent properly composes with streamingUtils and maintains backward compatibility of the public API signature
  • Import consistency – Verify all re-export paths align and handleError redirect doesn't introduce unexpected behavioral changes

Possibly related PRs

  • #1680 – Refactors streamServerRenderedReactComponent and streaming exports, overlapping core streaming architecture changes
  • #1841 – Package-splitting work that reorganizes pro code and affects the same streaming-related files and import paths
  • #1648 – Refactors streaming/server-rendering logic including transformRenderStreamChunksToResultObject and related streaming utilities

Suggested reviewers

  • alexeyr-ci
  • justin808
  • ihabadham

Poem

🐰 Streams now flow through utilsUtils,
Where buffered echoes hop and dance—
No more tangled render threads,
Just orchestrated elegance! ✨
Error handling takes its place,
Simpler paths, a cleaner face.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Prepare for removing react server dom from the RSC bundle" is partially related to the changeset. It correctly identifies that the changes relate to React Server Components and preparatory infrastructure work, which aligns with the actual refactoring of streaming utilities and reorganization of server-side rendering modules described in the changes. However, the title is overly broad and abstract—it doesn't clearly convey the specific technical changes being made, such as the creation of a new streamingUtils.ts module, the reorganization of streaming logic, or the redirecting of import paths for handleError. The title describes the general area and intent of the work rather than concisely summarizing the main implementation changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch prepare-for-removing-react-server-dom-from-rsc-bundle

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Oct 29, 2025

Code Review for PR #1889: Prepare for removing react server dom from the RSC bundle

Summary

This PR is a preparatory refactoring that extracts streaming-related utilities from streamServerRenderedReactComponent.ts into a new streamingUtils.ts file, and renames handleError.ts to generateRenderingErrorMessage.ts. This lays the groundwork for PR #1888, which aims to remove react-dom/server from the RSC bundle.

✅ Strengths

  1. Good refactoring approach: Breaking this into two PRs makes the changes easier to review
  2. Code organization: Extracting streaming utilities into a dedicated file improves modularity
  3. Trailing newlines: All files properly end with newlines ✓
  4. Well-documented: The bufferStream function and streamServerRenderedComponent have excellent documentation

❌ Critical Issues

1. Import path inconsistency (Breaking Change)

The package.json export mapping was updated:

"./handleError": "./lib/generateRenderingErrorMessage.js"

However, 3 files in react-on-rails-pro still import from the old path:

  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts:25
  • packages/react-on-rails-pro/src/streamingUtils.ts:21
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts:17

Impact: This will cause runtime import errors when the code is built and used.

Fix: These files should either:

  • Continue using react-on-rails/handleError (keep the export name the same), OR
  • Update to react-on-rails/generateRenderingErrorMessage

My recommendation: Keep the export name as handleError since it's the public API. The internal filename can be generateRenderingErrorMessage.ts, but the export path should remain stable.

2. Formatting issue in import statement

packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts:27-31:

import { 
  StreamingTrackers,
  transformRenderStreamChunksToResultObject,
  streamServerRenderedComponent,
 } from './streamingUtils.ts';

The closing brace has inconsistent spacing (note the leading space before }). This violates Prettier formatting rules.

Fix: Run rake autofix or yarn start format before committing.

⚠️ Issues

3. Missing checklist items

The PR description shows all checklist items are unchecked:

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Questions:

  • Are there tests that verify the refactored code still works correctly?
  • Does the CHANGELOG need an entry for this refactoring?

4. PR description not updated

The PR body still contains the template text asking to remove paragraphs and fill in details. Please provide:

💡 Suggestions

5. Consider adding JSDoc comments to exports

The newly exported transformRenderStreamChunksToResultObject and streamServerRenderedComponent functions would benefit from JSDoc comments explaining their purpose and parameters, since they're now part of the module's public API (within the pro package).

6. Type export location

The StreamingTrackers type is now exported from streamingUtils.ts, but it's also used in other files. Verify this is the intended location rather than a shared types file.

🔍 Code Quality Assessment

Positive aspects:

  • ✅ No logic changes - pure refactoring
  • ✅ Maintains existing error handling patterns
  • ✅ Good separation of concerns
  • ✅ Type safety preserved

Risk level: Low (assuming issues #1 and #2 are fixed)

📋 Action Items Before Merge

Must fix:

  1. ❌ Resolve import path inconsistency (issue TODO for first version #1)
  2. ❌ Fix formatting (issue Make work with turbolinks and better helper #2)
  3. ❌ Run bundle exec rubocop and fix any violations (per CLAUDE.md)

Should address:
4. ⚠️ Update PR description
5. ⚠️ Complete checklist items or mark as N/A
6. ⚠️ Confirm tests pass

Nice to have:
7. 💡 Add JSDoc comments to newly exposed functions

🧪 Testing Recommendation

Since this is a refactoring PR, please verify:

  1. All existing tests pass: rake run_rspec and yarn run test
  2. The RSC-specific tests still work
  3. Consider adding a test that the new file structure doesn't accidentally include server-only code in client bundles

Verdict: Needs changes before merge due to the breaking import path issue. Once issues #1 and #2 are resolved, this is a solid refactoring that sets up well for PR #1888.

Let me know if you'd like me to help identify the specific lines that need updating!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e02c7a and 6cc995e.

📒 Files selected for processing (6)
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (1 hunks)
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1 hunks)
  • packages/react-on-rails-pro/src/streamingUtils.ts (1 hunks)
  • packages/react-on-rails/package.json (1 hunks)
  • packages/react-on-rails/src/base/full.ts (1 hunks)
  • packages/react-on-rails/src/serverRenderReactComponent.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Prettier is the sole authority for formatting all non-Ruby files; never manually format them

Files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Use ESLint for JS/TS code (lint via rake lint or yarn lint)

Files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.

Applied to files:

  • packages/react-on-rails/src/serverRenderReactComponent.ts
  • packages/react-on-rails/src/base/full.ts
  • packages/react-on-rails/package.json
  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-04-09T13:02:16.009Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.

Applied to files:

  • packages/react-on-rails-pro/src/streamingUtils.ts
  • packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
🧬 Code graph analysis (1)
packages/react-on-rails-pro/src/streamingUtils.ts (4)
packages/react-on-rails/src/types/index.ts (3)
  • StreamRenderState (473-476)
  • assertRailsContextWithServerComponentMetadata (80-94)
  • RailsContextWithServerStreamingCapabilities (65-68)
packages/react-on-rails/src/serverRenderUtils.ts (3)
  • createResultObject (9-24)
  • validateComponent (30-36)
  • convertToError (26-28)
packages/react-on-rails/src/createReactOutput.ts (1)
  • createReactOutput (36-92)
packages/react-on-rails/src/isServerRenderResult.ts (1)
  • isServerRenderHash (8-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: dummy-app-integration-tests (3.4, 22)
  • GitHub Check: dummy-app-integration-tests (3.2, 20)
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.4, minimum)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: rspec-package-tests (3.2, latest)
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: claude-review
  • GitHub Check: build

Comment on lines +112 to +151
const transformStream = new PassThrough({
transform(chunk: Buffer, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState));
this.push(`${jsonChunk}\n`);

// Reset the render state to ensure that the error is not carried over to the next chunk
// eslint-disable-next-line no-param-reassign
renderState.error = undefined;
// eslint-disable-next-line no-param-reassign
renderState.hasErrors = false;

callback();
},
});

let pipedStream: PipeableOrReadableStream | null = null;
const pipeToTransform = (pipeableStream: PipeableOrReadableStream) => {
pipeableStream.pipe(transformStream);
pipedStream = pipeableStream;
};
// We need to wrap the transformStream in a Readable stream to properly handle errors:
// 1. If we returned transformStream directly, we couldn't emit errors into it externally
// 2. If an error is emitted into the transformStream, it would cause the render to fail
// 3. By wrapping in Readable.from(), we can explicitly emit errors into the readableStream without affecting the transformStream
// Note: Readable.from can merge multiple chunks into a single chunk, so we need to ensure that we can separate them later
const { stream: readableStream, emitError } = bufferStream(transformStream);

const writeChunk = (chunk: string) => transformStream.write(chunk);
const endStream = () => {
transformStream.end();
if (pipedStream && 'abort' in pipedStream) {
pipedStream.abort();
}
};
return { readableStream, pipeToTransform, writeChunk, emitError, endStream };
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix the transform pipeline: PassThrough ignores the custom transform.

transformRenderStreamChunksToResultObject instantiates a PassThrough with a transform callback. In Node.js, PassThrough always forwards chunks unchanged and ignores any custom _transform implementation, so the JSON serialization never runs—downstream readers see raw HTML instead of the expected JSON result envelopes, breaking the streaming contract. Use Transform (or explicitly override _transform) instead so the chunks are rewritten before they hit the buffered readable.(nodejs.org)

-import { PassThrough, Readable } from 'stream';
+import { Transform, Readable } from 'stream';-  const transformStream = new PassThrough({
-    transform(chunk: Buffer, _, callback) {
+  const transformStream = new Transform({
+    transform(chunk: Buffer, _, callback) {
       const htmlChunk = chunk.toString();
       const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
       previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
       const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState));
       this.push(`${jsonChunk}\n`);
…
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const transformStream = new PassThrough({
transform(chunk: Buffer, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState));
this.push(`${jsonChunk}\n`);
// Reset the render state to ensure that the error is not carried over to the next chunk
// eslint-disable-next-line no-param-reassign
renderState.error = undefined;
// eslint-disable-next-line no-param-reassign
renderState.hasErrors = false;
callback();
},
});
let pipedStream: PipeableOrReadableStream | null = null;
const pipeToTransform = (pipeableStream: PipeableOrReadableStream) => {
pipeableStream.pipe(transformStream);
pipedStream = pipeableStream;
};
// We need to wrap the transformStream in a Readable stream to properly handle errors:
// 1. If we returned transformStream directly, we couldn't emit errors into it externally
// 2. If an error is emitted into the transformStream, it would cause the render to fail
// 3. By wrapping in Readable.from(), we can explicitly emit errors into the readableStream without affecting the transformStream
// Note: Readable.from can merge multiple chunks into a single chunk, so we need to ensure that we can separate them later
const { stream: readableStream, emitError } = bufferStream(transformStream);
const writeChunk = (chunk: string) => transformStream.write(chunk);
const endStream = () => {
transformStream.end();
if (pipedStream && 'abort' in pipedStream) {
pipedStream.abort();
}
};
return { readableStream, pipeToTransform, writeChunk, emitError, endStream };
};
import { Transform, Readable } from 'stream';
// ... earlier code ...
const transformStream = new Transform({
transform(chunk: Buffer, _, callback) {
const htmlChunk = chunk.toString();
const consoleReplayScript = buildConsoleReplay(consoleHistory, previouslyReplayedConsoleMessages);
previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState));
this.push(`${jsonChunk}\n`);
// Reset the render state to ensure that the error is not carried over to the next chunk
// eslint-disable-next-line no-param-reassign
renderState.error = undefined;
// eslint-disable-next-line no-param-reassign
renderState.hasErrors = false;
callback();
},
});
let pipedStream: PipeableOrReadableStream | null = null;
const pipeToTransform = (pipeableStream: PipeableOrReadableStream) => {
pipeableStream.pipe(transformStream);
pipedStream = pipeableStream;
};
// We need to wrap the transformStream in a Readable stream to properly handle errors:
// 1. If we returned transformStream directly, we couldn't emit errors into it externally
// 2. If an error is emitted into the transformStream, it would cause the render to fail
// 3. By wrapping in Readable.from(), we can explicitly emit errors into the readableStream without affecting the transformStream
// Note: Readable.from can merge multiple chunks into a single chunk, so we need to ensure that we can separate them later
const { stream: readableStream, emitError } = bufferStream(transformStream);
const writeChunk = (chunk: string) => transformStream.write(chunk);
const endStream = () => {
transformStream.end();
if (pipedStream && 'abort' in pipedStream) {
pipedStream.abort();
}
};
return { readableStream, pipeToTransform, writeChunk, emitError, endStream };
};

@claude
Copy link

claude bot commented Oct 29, 2025

Pull Request Review - PR #1889

This PR refactors streaming server-rendering infrastructure by extracting utilities into streamingUtils.ts. Overall good modularization, but has critical issues that must be fixed before merge.

Positive Aspects

  • Clean separation of concerns improves code organization
  • File complexity reduced from 230 to 107 lines
  • Backward compatibility preserved
  • Good error handling architecture with new handleError.ts

Critical Issues

1. BROKEN package.json Export (packages/react-on-rails/package.json:49)

The file was renamed handleError.ts to generateRenderingErrorMessage.ts, but package.json line 49 still exports './handleError' pointing to non-existent './lib/handleError.js'. This will cause runtime errors. MUST remove this line.

2. Missing Tests

No tests added for this non-trivial refactoring. The bufferStream function is complex (handles race conditions), and error handling flows have changed. Tests needed for:

  • bufferStream with early errors
  • Error paths through streamServerRenderedComponent
  • Verify handleError stream output matches previous behavior

3. Undocumented Signature Change

streamServerRenderedComponent now requires handleError as 3rd parameter (dependency injection). Good for testing but not documented.

4. Error Handling Difference

Old: writeChunk(htmlString) then endStream()
New: pipeToTransform(htmlResultStream)
These are semantically different - need verification they produce identical output.

5. Missing CHANGELOG Entry

Per CLAUDE.md guidelines, this should be documented.

Action Items (Must Fix)

  1. Remove './handleError' export from package.json line 49
  2. Add tests for refactored code
  3. Update CHANGELOG
  4. Run bundle exec rubocop before commit
  5. Run rake autofix for formatting

Summary

Good refactoring improving modularity, but needs the package.json fix and tests before merge. Risk level: Medium (untested with signature changes).

@claude
Copy link

claude bot commented Oct 29, 2025

Pull Request Review - PR #1889

Summary

This PR refactors streaming infrastructure by extracting utilities from streamServerRenderedReactComponent.ts into a new streamingUtils.ts module. The changes prepare the codebase for removing react-dom/server from the RSC bundle (related to issue #1886).

Code Quality & Best Practices

Strengths:

  • Clean separation of concerns - moving reusable utilities to a dedicated module improves modularity
  • Excellent inline documentation, particularly the comprehensive JSDoc for bufferStream()
  • Well-structured design principles documented at the module level
  • Type safety maintained throughout with TypeScript

Issues Identified:

  1. Breaking Change in streamServerRenderedComponent Signature (CRITICAL)

    • The PR diff shows the function in streamingUtils.ts now accepts a handleError parameter (line 182 in new file)
    • However, the old version in streamServerRenderedReactComponent.ts (being moved) did NOT have this parameter
    • The caller in streamServerRenderedReactComponent.ts:104 now passes handleError correctly
    • This signature change is not mentioned in the PR description and could break other callers
  2. Import Path Inconsistency

    • In packages/react-on-rails/package.json:49, the export path for handleError is changed to point to generateRenderingErrorMessage.js
    • However, handleError and generateRenderingErrorMessage are two distinct modules with different purposes
    • The Pro version has its own handleError.ts that imports from react-on-rails/generateRenderingErrorMessage
    • This export mapping appears incorrect and could cause import failures
  3. Missing File in Diff

    • The diff shows packages/react-on-rails/src/generateRenderingErrorMessage.ts with 0 additions/0 deletions
    • This looks like a file rename from handleError.ts, but the diff doesn't show the actual rename
    • Git may not have tracked this as a proper rename operation

Code Structure Analysis

Current State (after this PR):

streamServerRenderedReactComponent.ts
  ├─ imports from streamingUtils.ts
  │   ├─ streamServerRenderedComponent<T>()  [generic function]
  │   ├─ transformRenderStreamChunksToResultObject()
  │   └─ StreamingTrackers type
  └─ imports from handleError.ts (Pro-specific)
      └─ handleError() [returns Readable stream]

This is a cleaner architecture, but the signature change needs documentation.

Performance Considerations

  • No performance concerns identified
  • Stream buffering logic remains unchanged
  • The refactoring is purely organizational with no algorithmic changes

Security Concerns

  • No security issues identified
  • Error handling paths properly maintained
  • No sensitive data exposure risks introduced

Test Coverage

Major Concern:

  • No test files found for streamServerRenderedReactComponent or streamingUtils
  • This refactoring moves critical streaming infrastructure with no apparent test coverage
  • Recommendation: Add unit tests for:
    • bufferStream() - event ordering, error buffering
    • transformRenderStreamChunksToResultObject() - chunk transformation, state management
    • streamServerRenderedComponent() - error handling paths

Missing PR Checklist Items

According to the PR description, these items are unchecked:

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Recommendations

  1. Fix the package.json export - The handleError export should not point to generateRenderingErrorMessage
  2. Document the signature change - Add a note about the new handleError parameter in streamServerRenderedComponent
  3. Verify git rename - Ensure handleError.tsgenerateRenderingErrorMessage.ts is tracked as a rename
  4. Add tests - Critical for streaming infrastructure refactoring
  5. Update CHANGELOG - Document this as a refactoring/preparation step
  6. Complete PR checklist - Address the unchecked items

Conclusion

This is a well-structured refactoring that improves code organization. However, there are critical issues that need addressing:

  • The package.json export mapping appears incorrect
  • The function signature change should be documented
  • Test coverage is needed for this critical path

The code itself is high quality, but the PR needs completion of checklist items and resolution of the issues above before merging.

Recommendation: Request changes to fix the package.json issue and add test coverage.

@claude
Copy link

claude bot commented Oct 29, 2025

Pull Request Review - PR #1889

Summary

This PR refactors the streaming infrastructure by extracting shared utilities from streamServerRenderedReactComponent.ts into a new streamingUtils.ts module, and renames handleError.ts to generateRenderingErrorMessage.ts for better clarity. This prepares the codebase for removing react-dom/server from the RSC bundle.


✅ Code Quality and Best Practices

Excellent Refactoring Structure

  • Clean separation of concerns: The extraction of streamServerRenderedComponent, transformRenderStreamChunksToResultObject, bufferStream, and StreamingTrackers into streamingUtils.ts is well-organized and logical
  • Improved modularity: The new file structure allows both streamServerRenderedReactComponent.ts and ReactOnRailsRSC.ts to share common streaming utilities without duplication
  • Better naming: Renaming handleError to generateRenderingErrorMessage more accurately describes the function's purpose (it generates error messages, not handling them in the broader sense)

Architecture Design

The new architecture is sound:

  1. streamingUtils.ts - Core streaming utilities (DRY principle)
  2. streamServerRenderedReactComponent.ts - Standard SSR streaming with react-dom/server
  3. ReactOnRailsRSC.ts - RSC streaming with react-server-dom-webpack/server
  4. handleError.ts & handleErrorRSC.ts - Separate error handlers for different rendering contexts

This separation enables future removal of react-dom/server from the RSC bundle without affecting standard SSR.


🐛 Potential Issues

1. CRITICAL: Signature Change in streamServerRenderedComponent

In streamingUtils.ts:179-183, the function signature has changed:

export const streamServerRenderedComponent = <T, P extends RenderParams>(
  options: P,
  renderStrategy: StreamRenderer<T, P>,
  handleError: (options: ErrorOptions) => PipeableOrReadableStream,  // ⚠️ NEW PARAMETER
): T => {

However, in streamServerRenderedReactComponent.ts:104, the call site passes handleError:

const streamServerRenderedReactComponent = (options: RenderParams): Readable =>
  streamServerRenderedComponent(options, streamRenderReactComponent, handleError);

But the import in streamServerRenderedReactComponent.ts:31 is:

import handleError from './handleError.ts';

Issue: The handleError import in streamServerRenderedReactComponent.ts:31 shadows the renamed import from line 17:

import handleError from 'react-on-rails/handleError';  // Line 17 (now removed)

Status: ✅ Actually correct! The diff shows:

  • Line 7 removes the old import: import handleError from 'react-on-rails/handleError';
  • Line 26 adds new import: import handleError from './handleError.ts';
  • Line 31 uses local handleError (from new Pro-specific module)

This is intentional - the Pro version now has its own handleError.ts that returns a Readable stream, while the base package has generateRenderingErrorMessage.ts that returns a string.

2. Missing Import in streamingUtils.ts

In streamingUtils.ts:29, there's an import of ErrorOptions type:

import {
  // ... other imports
  ErrorOptions,
} from 'react-on-rails/types';

But in the actual usage at line 182:

handleError: (options: ErrorOptions) => PipeableOrReadableStream,

And at line 247:

const htmlResultStream = handleError({ e: error, name: componentName, serverSide: true });

Status: ✅ This is correct - ErrorOptions is imported and properly typed.

3. Import Path Change - Breaking Change?

The handleError export path has changed in package.json:49:

"./handleError": "./lib/handleError.js",  // OLD - now points to generateRenderingErrorMessage.js
"./generateRenderingErrorMessage": "./lib/generateRenderingErrorMessage.js",  // NEW

Wait, there's a discrepancy! The diff shows:

-    "./handleError": "./lib/handleError.js",
+    "./handleError": "./lib/generateRenderingErrorMessage.js",

So ./handleError now points to generateRenderingErrorMessage.js. This means:

  • External code importing react-on-rails/handleError will still work
  • But it now gets the renamed module

Potential Issue: This could be confusing if the export name doesn't match the file name. Consider adding a deprecation warning or updating the export name to match.

Recommendation: For clarity, consider:

"./handleError": "./lib/generateRenderingErrorMessage.js",  // Keep for backwards compatibility
"./generateRenderingErrorMessage": "./lib/generateRenderingErrorMessage.js",  // Preferred new path

And add JSDoc deprecation notice in the export.

4. Missing handleError Import in streamingUtils.ts

Looking at streamingUtils.ts:247, it calls handleError() but doesn't import it:

const htmlResultStream = handleError({ e: error, name: componentName, serverSide: true });

Problem: handleError is a parameter (line 182) but the old code in the diff shows it was imported from 'react-on-rails/handleError'. The new refactored version expects it as a function parameter, which is correct! ✅


🚀 Performance Considerations

Positive Impact

  1. No performance regression: The refactoring moves code without changing logic
  2. Better code splitting: Separating utilities allows better tree-shaking
  3. Stream buffering: The bufferStream function is well-designed to prevent process crashes from unhandled errors

Stream Handling

The bufferStream implementation (lines 55-105 in streamingUtils.ts) is solid:

  • Properly buffers events until reading begins
  • Prevents race conditions with error handlers
  • Maintains event ordering

🔒 Security Concerns

✅ No New Security Issues

  • Error handling maintains proper error information exposure (server-side vs client-side)
  • No new user input handling or injection vectors
  • Stream handling doesn't introduce new vulnerabilities

Good Security Practice

The error handling in generateRenderingErrorMessage.ts:53-58 properly includes stack traces only for server-side rendering, which is correct security practice.


🧪 Test Coverage

✅ Excellent Test Coverage

The test file streamServerRenderedReactComponent.test.jsx has comprehensive coverage:

  1. Basic streaming (line 126)
  2. Shell errors with throwJsErrors (line 143)
  3. Shell errors without throwJsErrors (line 159)
  4. Async errors with throwJsErrors (line 175)
  5. Async errors without throwJsErrors (line 198)
  6. Render function variants (line 221)
  7. Error handling in render functions (line 241, 257, 279, 301)
  8. Promise-based components (line 326)

Test Coverage Status

  • ✅ All existing tests pass (based on comprehensive test suite)
  • ✅ Error scenarios well-covered
  • ✅ Both sync and async error paths tested
  • ✅ Various component types tested

Recommendations

  1. Add integration test for the new streamingUtils.ts module independently
  2. Add test for handleError.ts (Pro) and handleErrorRSC.ts separately
  3. Test that the signature change in streamServerRenderedComponent works with both SSR and RSC render strategies

📋 Additional Recommendations

Documentation

  1. Update CHANGELOG: Add entry for this refactoring under "Internal Changes" or "Refactoring"
  2. Add JSDoc comments: The new streamingUtils.ts module is internal but would benefit from better documentation of the refactoring purpose

Code Improvements

1. Consider Export Consistency

In package.json, consider this structure for clarity:

{
  "./handleError": "./lib/generateRenderingErrorMessage.js",  // Deprecated, backwards compat
  "./generateRenderingErrorMessage": "./lib/generateRenderingErrorMessage.js"  // Preferred
}

2. Add Type Safety

In streamingUtils.ts:237-250, the error handling block could benefit from more specific error types:

} catch (e) {
  const error = convertToError(e);
  // ... rest of error handling
}

Consider adding a type guard or comment explaining what errors are expected here.


Summary & Verdict

🎯 Overall Assessment: Excellent Refactoring

Strengths:

  • Clean separation of concerns
  • Maintains backward compatibility through package.json exports
  • Well-tested with comprehensive test coverage
  • Prepares codebase for future RSC bundle optimization
  • No breaking changes to public API

Minor Concerns:

  1. Export path naming could be clearer (./handleError vs ./generateRenderingErrorMessage)
  2. Missing CHANGELOG entry for this refactoring
  3. Could benefit from additional unit tests for new streamingUtils.ts module

Recommendation: Approve with minor suggestions

The refactoring is sound, well-architected, and maintains the existing test coverage. The minor suggestions above would improve clarity but don't block merging.


Great work on this refactoring! 👏 The separation of concerns will make future RSC optimizations much cleaner.

@AbanoubGhadban AbanoubGhadban merged commit 4f7f773 into master Oct 30, 2025
21 of 27 checks passed
@AbanoubGhadban AbanoubGhadban deleted the prepare-for-removing-react-server-dom-from-rsc-bundle branch October 30, 2025 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants