Add Concurrency Tests #1890
Add Concurrency Tests #1890
Conversation
WalkthroughAdds RSC/HTML streaming test infrastructure and concurrency tests, helper utilities (AsyncQueue, StreamReader, chunk-normalizers), a small devDependency for mock-fs, and removes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test
participant ROR as ReactOnRails
participant Component
participant Queue as AsyncQueue
participant Stream
participant Reader as StreamReader
Test->>ROR: serverRenderRSCReactComponent(props)
ROR->>Component: mount (Suspense)
Component->>Queue: await dequeue()
Note right of Queue: Producer can enqueue() later
Queue-->>Component: resolve value -> render fragment
Component->>Stream: emit RSC chunk(s)
Stream->>Reader: pipe chunks
Reader-->>Test: nextChunk() -> chunk string
loop concurrency
Test->>Queue: enqueue(value) for many renders
Reader-->>Test: expectNextChunk() validate normalized chunk
end
Component->>Queue: end()
Reader-->>Test: throw on end-of-stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request ReviewThank you for submitting this PR! I've reviewed the changes and have identified several critical issues that need to be addressed before this can be merged. Critical Issues ❌1. Breaking Change: Removal of
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-on-rails-pro/jest.config.js (1)
35-37: Document NODE_CONDITIONS environment variable.The
NODE_CONDITIONSenvironment variable is used to configure custom export conditions but is not documented. This makes it unclear:
- When and how developers should set this variable
- What valid values are expected (e.g.,
react-server,node, etc.)- How this affects test execution
Add a comment explaining the usage:
rootDir: '.', + // Custom export conditions for testing React Server Components + // Set NODE_CONDITIONS env var to control module resolution (e.g., NODE_CONDITIONS=react-server,node) testEnvironmentOptions: { customExportConditions: process.env.NODE_CONDITIONS?.split(','), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(1 hunks)packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx(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-pro/jest.config.jspackages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/react-on-rails-pro/package.jsonpackages/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-pro/jest.config.jspackages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
🧠 Learnings (16)
📓 Common learnings
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#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
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.
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: 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.
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
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.
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.
📚 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-pro/jest.config.jspackages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/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-pro/jest.config.jspackages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/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/tests/serverRenderRSCReactComponent.test.tsx
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.test.tsxpackages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
packages/react-on-rails-pro/package.json
📚 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-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-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-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
🧬 Code graph analysis (2)
packages/react-on-rails-pro/jest.config.js (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
process(5-5)
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx (1)
packages/react-on-rails/src/types/index.ts (1)
RailsContextWithServerStreamingCapabilities(65-68)
⏰ 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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (3)
packages/react-on-rails-pro/package.json (1)
77-80: LGTM!The addition of
mock-fsand@types/mock-fsas devDependencies is appropriate for supporting filesystem mocking in tests. Version ranges using caret (^) are standard for devDependencies.packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx (2)
1-27: Verify concurrency testing for PR alignment.The PR title is "Add Concurrency Tests," but this test file doesn't contain any concurrency testing scenarios. Tests should verify:
- Concurrent calls to
serverRenderRSCReactComponent- Race conditions in manifest loading
- Thread safety of the
serverRendererPromisesingleton (line 35 in ReactOnRailsRSC.ts)Please confirm whether additional concurrency tests exist elsewhere or if this file should be extended to include them.
15-15: Add missing semicolon.The
afterAllcallback is missing a semicolon, which is inconsistent with the coding style used elsewhere in the file.Apply this diff:
mock.restore(); -}) +});Likely an incorrect or invalid review comment.
| const error = convertToError(e); | ||
| reportError(error); | ||
| const errorHtml = handleError({ e: error, name: options.name, serverSide: true }); | ||
| const errorHtml = JSON.stringify({ e: error, name: options.name, serverSide: true }); |
There was a problem hiding this comment.
Critical issues with error serialization.
This line has several problems:
- Variable name is misleading:
errorHtmlsuggests HTML content, but it now contains JSON. - Error object won't serialize properly:
JSON.stringify()doesn't capture Error properties likemessage,stack, or custom properties. You'll get{}or minimal info. - Missing stack trace: Based on learnings, error stack should be included in development and test environments to aid debugging.
- Unclear property name: The property
eis not descriptive.
Apply this diff to fix the serialization and naming:
- const errorHtml = JSON.stringify({ e: error, name: options.name, serverSide: true });
- writeChunk(errorHtml);
+ const errorJson = JSON.stringify({
+ error: {
+ message: error.message,
+ stack: error.stack,
+ name: error.name,
+ },
+ componentName: options.name,
+ serverSide: true
+ });
+ writeChunk(errorJson);Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/ReactOnRailsRSC.ts around line 88, the
current JSON serialization uses a misleading variable name and will not capture
Error details; replace it with a clearly named variable (e.g., errorPayload)
that constructs a plain object containing error: { message: error.message },
name: options.name, serverSide: true, and include error.stack when
process.env.NODE_ENV is "development" or "test"; then JSON.stringify that
payload so consumers receive message, optional stack (only in dev/test), and
descriptive property names instead of an empty or opaque object.
| test('eeee', () => { | ||
| expect(fs.readFileSync('./server/react-client-manifest.json').toString()).toEqual('{}'); | ||
| ReactOnRails.serverRenderRSCReactComponent({ | ||
| railsContext: {} as unknown as RailsContextWithServerStreamingCapabilities, | ||
| name: 'Component1', | ||
| reactClientManifestFileName: 'react-client-manifest.json', | ||
| renderingReturnsPromises: true, | ||
| throwJsErrors: true, | ||
| domNodeId: 'dom-id', | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Test is incomplete and doesn't match PR objectives.
Several critical issues with this test:
- Test name 'eeee' is a placeholder - use a descriptive name like
'should render RSC component with mocked filesystem' - No assertions on rendering result - the test calls
serverRenderRSCReactComponentbut doesn't verify the output or behavior - Empty railsContext - casting
{}toRailsContextWithServerStreamingCapabilitiesbypasses type safety. The real type requires properties likegetRSCPayloadStreamandaddPostSSRHook(see type definition), which will cause runtime errors - PR title is "Add Concurrency Tests" but this doesn't test any concurrency scenarios
- Missing error handling tests - given the related change in
ReactOnRailsRSC.tsthat modifies error serialization, this test should verify error scenarios
Consider this structure:
-test('eeee', () => {
- expect(fs.readFileSync('./server/react-client-manifest.json').toString()).toEqual('{}');
- ReactOnRails.serverRenderRSCReactComponent({
- railsContext: {} as unknown as RailsContextWithServerStreamingCapabilities,
+test('should render RSC component with mocked filesystem', async () => {
+ const mockRailsContext: RailsContextWithServerStreamingCapabilities = {
+ getRSCPayloadStream: jest.fn(),
+ addPostSSRHook: jest.fn(),
+ reactClientManifestFileName: 'server/react-client-manifest.json',
+ // ... other required properties
+ };
+
+ const result = ReactOnRails.serverRenderRSCReactComponent({
+ railsContext: mockRailsContext,
name: 'Component1',
- reactClientManifestFileName: 'react-client-manifest.json',
+ reactClientManifestFileName: 'server/react-client-manifest.json',
renderingReturnsPromises: true,
throwJsErrors: true,
domNodeId: 'dom-id',
- })
-})
+ });
+
+ // Add assertions on result
+ expect(result).toBeDefined();
+ // Verify the stream output, error handling, etc.
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx
around lines 17 to 27, the test uses a placeholder name, lacks assertions,
improperly casts an empty object to RailsContextWithServerStreamingCapabilities,
doesn't exercise concurrency, and omits error-case checks; fix by renaming the
test to a descriptive title (e.g., "should render RSC component with mocked
filesystem"), mock fs.readFileSync/react-client-manifest.json properly, provide
a realistic railsContext mock implementing getRSCPayloadStream and
addPostSSRHook (and any streaming behavior needed), await and assert on the
serverRenderRSCReactComponent return or side-effects (including verifying output
and that react-client-manifest was used), add a separate concurrent invocation
test (e.g., Promise.all of multiple renders) to exercise concurrency, and add
tests that trigger and assert error serialization/handling paths introduced in
ReactOnRailsRSC.ts.
Comprehensive Pull Request ReviewThank you for submitting this PR! I've conducted a thorough review of the changes and have identified several critical issues that need to be addressed before this can be merged. 🚨 Critical Issues1. Breaking Change: Removal of
|
Code Review for PR #1890: Add Concurrency TestsThank you for working on this PR! I've reviewed the changes and have several important concerns and suggestions. 🚨 Critical Issues1. Breaking Change to Error Handling (packages/react-on-rails-pro/src/ReactOnRailsRSC.ts:88-89)The change from using // Before
const errorHtml = handleError({ e: error, name: options.name, serverSide: true });
// After
const errorHtml = JSON.stringify({ e: error, name: options.name, serverSide: true });Issues:
Questions:
2. Incomplete Test Implementation (packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.test.tsx)The test file has several issues: a) Placeholder test name: test('eeee', () => { // ❌ Not descriptiveShould be: b) No assertions on the actual rendering: ReactOnRails.serverRenderRSCReactComponent({...}) // ❌ Result not captured or testedc) Missing semicolons (lines 10, 14): mock({
'server/react-client-manifest.json': '{}'
}); // ❌ Missing semicolon
afterAll(() => {
mock.restore();
}) // ❌ Missing semicolond) Mock setup outside test:
e) No actual concurrency testing:
3. mock-fs Compatibility IssuesThe
Recommendation: Use
|
AbanoubGhadban
force-pushed
the
abanoubghadban/add-concurrency-tests
branch
from
ef3ce5f to
713033b
Compare
Code Review for PR #1890: Add Concurrency TestsSummaryThis PR adds a test for React Server Components (RSC) rendering, removes Critical Issues1. Debug Console Statements Left in Production Code 🚨Location: console.log("Stream working");
console.log("Stream flushed");
console.log("Stream ended");Issue: Debug console.log statements should never be committed to production code. These will pollute logs and impact performance. Action Required: Remove all debug console.log statements before merging. 2. Test Quality Issues 🚨Location: Multiple problems with the new test: a) Poor test naming: test('eeee', async () => {This is completely unacceptable. Test names should clearly describe what is being tested. b) Incomplete test implementation:
c) Hardcoded file paths: console.log(path.resolve('./src/react-client-manifest.json'));More debug console.log statements that shouldn't be committed. d) Mock filesystem setup issues:
e) Missing cleanup and error scenarios:
Recommendation: Look at
3. Type Change Impact Not Fully ConsideredLocation: The removal of export interface RSCRenderParams extends Omit<RenderParams, 'railsContext'> {
railsContext: RailsContextWithServerStreamingCapabilities;
- reactClientManifestFileName: string;
}Analysis:
Action Required: Add a note in the PR description explaining this type change and why it's safe. Performance Considerations
Security ConcernsNo security issues identified in the changed code. Test Coverage IssuesThe new test provides minimal coverage:
Coverage Gap: For comparison,
Code Quality Issues
RecommendationsBefore this PR can be merged:
If the goal is to add concurrency tests, I'd expect to see:
Positive Notes✅ The type refactoring to remove VerdictChanges Requested - This PR needs significant work before it can be merged. The debug statements are blockers, and the test quality needs substantial improvement to provide value. Please address the critical issues above and feel free to reach out if you need help with the test implementation. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/streamingUtils.ts(3 hunks)packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx(1 hunks)packages/react-on-rails/src/types/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails/src/types/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails-pro/src/streamingUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/package.json
🧰 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
**/*.{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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
🧠 Learnings (8)
📓 Common learnings
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: 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.
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
🧬 Code graph analysis (1)
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx (1)
packages/react-on-rails/src/types/index.ts (2)
ReactOnRails(276-344)RailsContextWithServerStreamingCapabilities(65-68)
⏰ 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). (6)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: examples (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
🔇 Additional comments (2)
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx (2)
19-21: Add missing semicolon.Apply this diff:
afterAll(() => { mock.restore(); -}) +});Likely an incorrect or invalid review comment.
11-11: Missing React import for JSX syntax.The component uses JSX but React is not imported, which will cause a compilation or runtime error.
Apply this diff to add the React import:
+import React from 'react'; + const Component1 = () => <div>HelloWorld</div>;⛔ Skipped due to learnings
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.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.Learnt from: theforestvn88 PR: shakacode/react_on_rails#1620 File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3 Timestamp: 2024-10-08T20:53:47.076Z Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.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: AbanoubGhadban PR: shakacode/react_on_rails#1644 File: lib/react_on_rails/helper.rb:190-197 Timestamp: 2025-02-18T13:08:01.477Z Learning: RSC support validation in React on Rails Pro is handled through a chain of validations: 1. Pro version check in `run_stream_inside_fiber` 2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support` 3. RSC support validation during component registration This makes additional validation in the helper methods unnecessary.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/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: AbanoubGhadban PR: shakacode/react_on_rails#1644 File: lib/react_on_rails/helper.rb:190-197 Timestamp: 2025-02-18T13:08:01.477Z Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.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.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.
| test('eeee', async () => { | ||
| console.log(path.resolve('./src/react-client-manifest.json')); | ||
| expect(fs.readFileSync('./src/react-client-manifest.json').toString()).toEqual('{}'); |
There was a problem hiding this comment.
Improve test name and remove debug code.
The test name "eeee" is not descriptive and should clearly state what functionality is being tested. The console.log on line 24 appears to be debug code that should be removed.
Apply this diff:
-test('eeee', async () => {
- console.log(path.resolve('./src/react-client-manifest.json'));
+test('should render RSC component with server streaming', async () => {
expect(fs.readFileSync('./src/react-client-manifest.json').toString()).toEqual('{}');📝 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.
| test('eeee', async () => { | |
| console.log(path.resolve('./src/react-client-manifest.json')); | |
| expect(fs.readFileSync('./src/react-client-manifest.json').toString()).toEqual('{}'); | |
| test('should render RSC component with server streaming', async () => { | |
| expect(fs.readFileSync('./src/react-client-manifest.json').toString()).toEqual('{}'); |
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
around lines 23 to 25, the test uses a non-descriptive name "eeee" and contains
a console.log debug statement; rename the test to a meaningful description
(e.g., "reads react-client-manifest and expects empty JSON") and remove the
console.log line so the test only reads and asserts the file content.
| const readable = ReactOnRails.serverRenderRSCReactComponent({ | ||
| railsContext: { | ||
| reactClientManifestFileName: 'react-client-manifest.json', | ||
| reactServerClientManifestFileName: 'react-server-client-manifest.json', | ||
| } as unknown as RailsContextWithServerStreamingCapabilities, | ||
| name: 'Component1', | ||
| renderingReturnsPromises: true, | ||
| throwJsErrors: true, | ||
| domNodeId: 'dom-id', | ||
| }); |
There was a problem hiding this comment.
Fix incomplete railsContext and remove unsafe type assertion.
The railsContext object is missing required methods getRSCPayloadStream and addPostSSRHook from the RailsContextWithServerStreamingCapabilities interface. Additionally, the type assertion as unknown as bypasses type checking, hiding this issue. The property reactClientManifestFileName was removed from the interface according to the PR changes.
You need to provide a complete mock implementation of the railsContext. Here's a suggested approach:
- const readable = ReactOnRails.serverRenderRSCReactComponent({
- railsContext: {
- reactClientManifestFileName: 'react-client-manifest.json',
- reactServerClientManifestFileName: 'react-server-client-manifest.json',
- } as unknown as RailsContextWithServerStreamingCapabilities,
+ const mockRailsContext: RailsContextWithServerStreamingCapabilities = {
+ reactServerClientManifestFileName: 'react-server-client-manifest.json',
+ getRSCPayloadStream: async (componentName: string, props: unknown) => {
+ // Mock implementation
+ const { Readable } = require('stream');
+ return Readable.from([Buffer.from('mock stream data')]);
+ },
+ addPostSSRHook: (hook: () => void) => {
+ // Mock implementation
+ },
+ // Add any other required properties from RailsContextWithServerComponentMetadata
+ };
+
+ const readable = ReactOnRails.serverRenderRSCReactComponent({
+ railsContext: mockRailsContext,
name: 'Component1',🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
around lines 26-35, the railsContext mock is incomplete and uses an unsafe "as
unknown as" assertion; replace it with a properly typed mock that implements the
RailsContextWithServerStreamingCapabilities interface (remove the now-removed
reactClientManifestFileName property and only include the valid manifest name),
provide a getRSCPayloadStream method that returns a readable stream (or a simple
mock Readable like an object with a pipe/reader compatible for the test), and
implement addPostSSRHook as a no-op or collector function; then declare the mock
with the correct type (no unknown cast) and pass that to
serverRenderRSCReactComponent so TypeScript verifies the required methods exist.
| const decoder = new TextDecoder(); | ||
| readable.on('data', chunk => { | ||
| console.log(decoder.decode(chunk)); | ||
| }) | ||
| await new Promise((resolve, reject) => { | ||
| readable.on('end', resolve); | ||
| readable.on('error', reject); | ||
| }) | ||
| }, 100000) |
There was a problem hiding this comment.
Add assertions and remove debug logging.
The test lacks meaningful assertions on the stream output from serverRenderRSCReactComponent. Currently, it only verifies the mock file exists but doesn't validate the actual rendering result. The console.log on line 39 appears to be debug code. Additionally, the 100-second timeout is excessive and should be reduced or explained.
Apply this diff:
const decoder = new TextDecoder();
+ let output = '';
readable.on('data', chunk => {
- console.log(decoder.decode(chunk));
+ output += decoder.decode(chunk);
})
await new Promise((resolve, reject) => {
readable.on('end', resolve);
readable.on('error', reject);
- })
-}, 100000)
+ });
+
+ // Add assertions to verify the rendered output
+ expect(output).toBeTruthy();
+ expect(output).toContain('HelloWorld'); // or whatever the expected output format is
+}, 10000); // Reduce timeout to 10 seconds🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
around lines 37 to 45, remove the debug console.log and replace the ad-hoc data
handler with collection of decoded chunks into a buffer string, then assert that
the final buffered output contains the expected rendered content (e.g., expected
component HTML/marker or specific text produced by
serverRenderRSCReactComponent) after the stream ends; keep error handling for
'error' events and reduce the test timeout from 100000ms to a reasonable value
(e.g., 5000ms) or remove the explicit timeout if unnecessary.
AbanoubGhadban
force-pushed
the
abanoubghadban/add-concurrency-tests
branch
from
713033b to
add0cd8
Compare
Code Review for PR #1890: Add Concurrency TestsThank you for contributing these concurrency tests for React Server Components! This is valuable test coverage for ensuring RSC streaming works correctly under concurrent rendering scenarios. Code Quality and Best PracticesStrengths:
Issues to Address:
Potential Bugs and Issues
Performance Considerations
Security Concerns
Test Coverage and QualityStrengths:
Improvements Needed:
Breaking Change AnalysisCritical Issue:
Required Actions Before MergeCritical (Must Fix):
High Priority (Should Fix):
Nice to Have:
VerdictThis PR adds valuable test coverage for concurrency scenarios in RSC rendering. The test utilities are well-designed and reusable. However, there are several issues that need to be addressed before merging, particularly around code quality, the breaking change, and adherence to the project's mandatory linting requirements per CLAUDE.md. Please ensure you run Looking forward to seeing these improvements! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx (1)
65-86: Mock the full streaming railsContext instead of casting.
RailsContextWithServerStreamingCapabilitiesrequiresgetRSCPayloadStreamandaddPostSSRHook. Casting a plain object with only manifest names (as unknown as) meansReactOnRails.serverRenderRSCReactComponentwill throw at runtime the first time it tries to call either hook, so this test currently cannot run once the production code exercises those methods. Please build a single shared mock that actually implements the interface—e.g. create a helper that wiresbuildServerRenderer+createReactOutputto return aReadablestream, defineaddPostSSRHookas a no-op collector, drop the obsoletereactClientManifestFileName, and pass that typed object into both renders without any unsafe casts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/tests/AsyncQueue.ts(1 hunks)packages/react-on-rails-pro/tests/StreamReader.ts(1 hunks)packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx(1 hunks)packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx(1 hunks)packages/react-on-rails/src/types/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails/src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/package.json
🧰 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/AsyncQueue.tspackages/react-on-rails-pro/tests/StreamReader.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
**/*.{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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/AsyncQueue.tspackages/react-on-rails-pro/tests/StreamReader.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
🧠 Learnings (12)
📓 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: 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.
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.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCProvider.tsx:0-0
Timestamp: 2025-06-11T12:34:58.182Z
Learning: `RSCProvider` intentionally keeps failed `getServerComponent` promises cached (even if rejected) to avoid repeated fetch attempts; callers must use `refetchComponent` to retry.
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 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/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsxpackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
📚 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/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
🧬 Code graph analysis (3)
packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx (1)
packages/react-on-rails/src/types/index.ts (1)
RailsContextWithServerStreamingCapabilities(65-68)
packages/react-on-rails-pro/tests/StreamReader.ts (1)
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js (1)
decoder(88-88)
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx (4)
packages/react-on-rails/src/types/index.ts (1)
RailsContextWithServerStreamingCapabilities(65-68)packages/react-on-rails-pro/tests/testUtils.js (1)
stream(14-21)packages/react-on-rails-pro/tests/AsyncQueue.ts (1)
enqueue(8-18)packages/react-on-rails-pro/tests/StreamReader.ts (1)
nextChunk(28-30)
⏰ 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). (9)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
| const bufferValueIfExist = this.buffer.shift(); | ||
| if (bufferValueIfExist) { | ||
| resolve(bufferValueIfExist); | ||
| } else if (this.isEnded) { | ||
| reject(new Error("Queue Ended")); | ||
| } else { | ||
| let teardown = () => {} | ||
| const onData = (value: T) => { | ||
| resolve(value); | ||
| teardown(); | ||
| } | ||
|
|
||
| const onEnd = () => { | ||
| reject(new Error("Queue Ended")); | ||
| teardown(); | ||
| } | ||
|
|
||
| this.eventEmitter.on('data', onData); | ||
| this.eventEmitter.on('end', onEnd); | ||
| teardown = () => { | ||
| this.eventEmitter.off('data', onData); | ||
| this.eventEmitter.off('end', onEnd); | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Dequeue must honor falsy buffered values.
buffer.shift() can legitimately return '', 0, or false, but the current truthiness check falls through to the “queue ended” branch and rejects even though data is buffered. Compare against undefined instead so every enqueued value is delivered.
- const bufferValueIfExist = this.buffer.shift();
- if (bufferValueIfExist) {
- resolve(bufferValueIfExist);
+ const bufferValue = this.buffer.shift();
+ if (bufferValue !== undefined) {
+ resolve(bufferValue);
} else if (this.isEnded) {
reject(new Error("Queue Ended"));
} else {📝 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.
| const bufferValueIfExist = this.buffer.shift(); | |
| if (bufferValueIfExist) { | |
| resolve(bufferValueIfExist); | |
| } else if (this.isEnded) { | |
| reject(new Error("Queue Ended")); | |
| } else { | |
| let teardown = () => {} | |
| const onData = (value: T) => { | |
| resolve(value); | |
| teardown(); | |
| } | |
| const onEnd = () => { | |
| reject(new Error("Queue Ended")); | |
| teardown(); | |
| } | |
| this.eventEmitter.on('data', onData); | |
| this.eventEmitter.on('end', onEnd); | |
| teardown = () => { | |
| this.eventEmitter.off('data', onData); | |
| this.eventEmitter.off('end', onEnd); | |
| } | |
| } | |
| }) | |
| } | |
| const bufferValue = this.buffer.shift(); | |
| if (bufferValue !== undefined) { | |
| resolve(bufferValue); | |
| } else if (this.isEnded) { | |
| reject(new Error("Queue Ended")); | |
| } else { | |
| let teardown = () => {} | |
| const onData = (value: T) => { | |
| resolve(value); | |
| teardown(); | |
| } | |
| const onEnd = () => { | |
| reject(new Error("Queue Ended")); | |
| teardown(); | |
| } | |
| this.eventEmitter.on('data', onData); | |
| this.eventEmitter.on('end', onEnd); | |
| teardown = () => { | |
| this.eventEmitter.off('data', onData); | |
| this.eventEmitter.off('end', onEnd); | |
| } | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/AsyncQueue.ts around lines 27 to 52, the
current truthiness check on the shifted buffer value causes legitimate falsy
values (0, '', false) to be treated as missing and triggers the "Queue Ended"
rejection; change the check to compare explicitly against undefined (e.g. const
bufferValue = this.buffer.shift(); if (bufferValue !== undefined) {
resolve(bufferValue); }) so that all queued values, including falsy ones, are
delivered while preserving the existing end/emit logic.
| return ReactOnRails.serverRenderRSCReactComponent({ | ||
| railsContext: { | ||
| reactClientManifestFileName: 'react-client-manifest.json', | ||
| reactServerClientManifestFileName: 'react-server-client-manifest.json', | ||
| } as unknown as RailsContextWithServerStreamingCapabilities, | ||
| name: 'AsyncQueueContainer', | ||
| renderingReturnsPromises: true, | ||
| throwJsErrors: true, | ||
| domNodeId: 'dom-id', | ||
| props, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Provide a real RailsContextWithServerStreamingCapabilities implementation.
This helper reuses the same incomplete railsContext mock: no getRSCPayloadStream, no addPostSSRHook, and it still references the removed reactClientManifestFileName. The unsafe as unknown as masks the type error, but the first production call to either missing method will explode. Extract a properly typed mock (shared with the other test) that actually implements the streaming hooks—using buildServerRenderer/createReactOutput to supply a Readable payload and a no-op addPostSSRHook—and drop the deprecated manifest field. Then pass that typed object directly so the compiler enforces the contract.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
around lines 63–74, the test is using an unsafe incomplete railsContext mock
(casting with as unknown as) that lacks getRSCPayloadStream and addPostSSRHook
and still contains the removed reactClientManifestFileName; replace this with a
real RailsContextWithServerStreamingCapabilities object: extract a properly
typed mock shared with the other test, remove the deprecated
reactClientManifestFileName field, implement getRSCPayloadStream to return a
Readable payload (use the existing buildServerRenderer/createReactOutput helpers
to produce the stream) and implement addPostSSRHook as a no-op, then pass that
typed object directly (no unsafe cast) so the compiler enforces the contract.
| readableStream.on('data', (chunk) => { | ||
| const decodedChunk = decoder.decode(chunk); | ||
| this.asyncQueue.enqueue(decodedChunk); | ||
| }); | ||
|
|
||
| if (readableStream.closed) { | ||
| this.asyncQueue.end(); | ||
| } else { | ||
| readableStream.on('end', () => { | ||
| this.asyncQueue.end(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Use streaming TextDecoder decoding and flush at end.
Decoding each chunk with decoder.decode(chunk) resets the decoder state, so any multi-byte character split across chunks will be corrupted or replaced. We also drop the final buffered bytes because we never flush at end. Decode with { stream: true }, flush once on end, and only then close the queue; this keeps UTF‑8 intact and preserves the tail chunk.
- readableStream.on('data', (chunk) => {
- const decodedChunk = decoder.decode(chunk);
- this.asyncQueue.enqueue(decodedChunk);
- });
-
- if (readableStream.closed) {
- this.asyncQueue.end();
- } else {
- readableStream.on('end', () => {
- this.asyncQueue.end();
- });
- }
+ readableStream.on('data', (chunk) => {
+ const decodedChunk = decoder.decode(chunk, { stream: true });
+ if (decodedChunk.length > 0) {
+ this.asyncQueue.enqueue(decodedChunk);
+ }
+ });
+
+ readableStream.on('end', () => {
+ const flushed = decoder.decode();
+ if (flushed.length > 0) {
+ this.asyncQueue.enqueue(flushed);
+ }
+ this.asyncQueue.end();
+ });📝 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.
| readableStream.on('data', (chunk) => { | |
| const decodedChunk = decoder.decode(chunk); | |
| this.asyncQueue.enqueue(decodedChunk); | |
| }); | |
| if (readableStream.closed) { | |
| this.asyncQueue.end(); | |
| } else { | |
| readableStream.on('end', () => { | |
| this.asyncQueue.end(); | |
| }); | |
| } | |
| readableStream.on('data', (chunk) => { | |
| const decodedChunk = decoder.decode(chunk, { stream: true }); | |
| if (decodedChunk.length > 0) { | |
| this.asyncQueue.enqueue(decodedChunk); | |
| } | |
| }); | |
| readableStream.on('end', () => { | |
| const flushed = decoder.decode(); | |
| if (flushed.length > 0) { | |
| this.asyncQueue.enqueue(flushed); | |
| } | |
| this.asyncQueue.end(); | |
| }); |
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/StreamReader.ts around lines 14–25, the
code decodes each chunk with decoder.decode(chunk) which resets decoder state
and breaks multi-byte characters; change to decoder.decode(chunk, { stream: true
}) for each 'data' chunk, and on both the immediate closed path and the 'end'
handler call decoder.decode() (or decoder.decode(undefined)) once to flush any
buffered bytes, enqueue the flushed string if non-empty, then call
this.asyncQueue.end().
Code Review: Add Concurrency TestsThank you for this PR! This is a valuable addition that tests concurrent React Server Component rendering. Here's my detailed review: Positive Aspects
Issues to AddressCritical Issues1. Incomplete PR Description
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx (1)
63-75: Provide a real RailsContextWithServerStreamingCapabilities implementation.The railsContext mock uses an unsafe
as unknown ascast and references the removedreactClientManifestFileNamefield (line 66). Additionally, it lacks the requiredgetRSCPayloadStreamandaddPostSSRHookmethods from theRailsContextWithServerStreamingCapabilitiestype. Extract a properly typed mock that implements these methods correctly.
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts (1)
4-4: Fix typo in variable name.The variable is misspelled as
santizedHtmlinstead ofsanitizedHtml.Apply this diff:
- const santizedHtml = html.split('\n').map(chunkLine => { + const sanitizedHtml = html.split('\n').map(chunkLine => {And update line 22:
return JSON.stringify({ ...parsedJson, - html: santizedHtml, + html: sanitizedHtml, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx(1 hunks)packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx(1 hunks)packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts(1 hunks)packages/react-on-rails-pro/tests/utils/removeRSCStackFromAllChunks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/tests/serverRenderRSCReactComponent.rsc.test.tsx
🧰 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-pro/tests/utils/removeRSCStackFromAllChunks.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsxpackages/react-on-rails-pro/tests/utils/removeRSCChunkStack.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-pro/tests/utils/removeRSCStackFromAllChunks.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsxpackages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts
🧠 Learnings (11)
📓 Common learnings
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.
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.
📚 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-pro/tests/utils/removeRSCStackFromAllChunks.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsxpackages/react-on-rails-pro/tests/utils/removeRSCChunkStack.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/tests/utils/removeRSCStackFromAllChunks.tspackages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsxpackages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts
📚 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-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 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-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsxpackages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
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/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts
🧬 Code graph analysis (1)
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx (4)
packages/react-on-rails/src/types/index.ts (1)
RailsContextWithServerStreamingCapabilities(65-68)packages/react-on-rails-pro/tests/testUtils.js (1)
stream(14-21)packages/react-on-rails-pro/tests/AsyncQueue.ts (1)
enqueue(8-18)packages/react-on-rails-pro/tests/StreamReader.ts (1)
nextChunk(28-30)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx
[error] 1-1: Test suite failed to run. ENOENT, no such file or directory '/home/runner/work/react_on_rails/react_on_rails/node_modules/jest-worker/build/index.js'
⏰ 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: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (5)
packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx (5)
1-29: LGTM! Test setup is well-structured.The Jest environment configuration, mock filesystem setup, and cleanup are correctly implemented.
31-59: LGTM! Test components are well-designed.The
AsyncQueueItemandAsyncQueueContainercomponents effectively test concurrent RSC streaming with proper Suspense boundaries and async queue integration.
77-98: LGTM! Parallel rendering helper is well-implemented.The
createParallelRendersfunction provides a clean abstraction for testing concurrent RSC streams with proper reader management and assertion helpers.
100-141: LGTM! Comprehensive test coverage for concurrent RSC streaming.The test effectively validates both single-stream and concurrent (50 parallel) RSC payload generation with proper assertions.
1-1: Pipeline failure appears to be a CI environment issue.The test suite failure about missing
jest-worker/build/index.jssuggests a dependency installation problem in the CI environment rather than an issue with the test code itself. Consider re-running the CI pipeline or checking the dependency installation step.
| const santizedHtml = html.split('\n').map(chunkLine => { | ||
| if (!chunkLine.includes('"stack":')) { | ||
| return chunkLine; | ||
| } | ||
|
|
||
| const regexMatch = /(^\d+):\{/.exec(chunkLine) | ||
| if (!regexMatch) { | ||
| return; | ||
| } | ||
|
|
||
| const chunkJsonString = chunkLine.slice(chunkLine.indexOf('{')); | ||
| const chunkJson = JSON.parse(chunkJsonString); | ||
| delete chunkJson.stack; | ||
| return `${regexMatch[1]}:${JSON.stringify(chunkJson)}` | ||
| }); |
There was a problem hiding this comment.
Return original line instead of undefined.
When the regex doesn't match on line 10, the function returns undefined on line 11. This will cause the array to contain undefined values, which will be stringified as "undefined" when reconstructing the JSON, corrupting the output.
Apply this diff to return the original line instead:
const regexMatch = /(^\d+):\{/.exec(chunkLine)
if (!regexMatch) {
- return;
+ return chunkLine;
}🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts around lines 4
to 18, the map callback returns undefined when the regex doesn't match (line
11), which inserts undefined into the result array and corrupts output; change
that early return to return the original chunkLine (i.e., replace `return;` with
`return chunkLine;`) so non-matching lines are preserved unchanged.
| const removeRSCStackFromAllChunks = (allChunks: string) => { | ||
| return allChunks.split('\n') | ||
| .map((chunk) => chunk.trim().length > 0 ? removeRSCChunkStack(chunk) : chunk) | ||
| .join('\n'); | ||
| } |
There was a problem hiding this comment.
Handle undefined return from removeRSCChunkStack.
The removeRSCChunkStack function can return undefined (see line 11 in packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts), which would cause .join('\n') to convert it to the string "undefined". This will corrupt the output.
Apply this diff to filter out undefined values:
const removeRSCStackFromAllChunks = (allChunks: string) => {
return allChunks.split('\n')
- .map((chunk) => chunk.trim().length > 0 ? removeRSCChunkStack(chunk) : chunk)
+ .map((chunk) => chunk.trim().length > 0 ? removeRSCChunkStack(chunk) : chunk)
+ .filter((chunk): chunk is string => chunk !== undefined)
.join('\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.
| const removeRSCStackFromAllChunks = (allChunks: string) => { | |
| return allChunks.split('\n') | |
| .map((chunk) => chunk.trim().length > 0 ? removeRSCChunkStack(chunk) : chunk) | |
| .join('\n'); | |
| } | |
| const removeRSCStackFromAllChunks = (allChunks: string) => { | |
| return allChunks.split('\n') | |
| .map((chunk) => chunk.trim().length > 0 ? removeRSCChunkStack(chunk) : chunk) | |
| .filter((chunk): chunk is string => chunk !== undefined) | |
| .join('\n'); | |
| } |
🤖 Prompt for AI Agents
packages/react-on-rails-pro/tests/utils/removeRSCStackFromAllChunks.ts around
lines 3 to 7: the current map can produce undefined values from
removeRSCChunkStack which will become the string "undefined" when joined; update
the pipeline to filter out undefined results before joining (e.g. map(...), then
.filter((s): s is string => s !== undefined) or use flatMap to only return
defined strings) so only real chunk strings are joined with '\n'.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.ts (1)
58-61: Connection serialization logic is correct.The implementation prevents concurrent
connect()calls by reusing a shared connection promise. This resolves potential race conditions.Consider clearing
connectionPromiseafter successful connection for clarity:if (!connectionPromise) { connectionPromise = sharedRedisClient.connect(); } await connectionPromise; + connectionPromise = undefined; isClientConnected = true;This is optional since the
isClientConnectedflag already prevents reuse of the stale promise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts(1 hunks)react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js(1 hunks)react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts(1 hunks)react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.ts(3 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:
react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.tsreact_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.tsreact_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.jsreact_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.tsreact_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.tsreact_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.jsreact_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
🧠 Learnings (3)
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 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:
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 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:
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
🧬 Code graph analysis (2)
react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts (2)
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js (7)
form(22-22)app(7-7)app(23-23)makeRequest(20-87)client(24-24)request(25-29)chunks(32-32)react_on_rails_pro/packages/node-renderer/tests/helper.ts (1)
readRenderingRequest(137-145)
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts (2)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/RedisReceiver.jsx (1)
value(7-7)react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts (1)
makeRequest(80-166)
🪛 ast-grep (0.39.6)
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
[warning] 71-71: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 103-103: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 108-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 113-113: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 118-118: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 123-123: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 128-128: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: React on Rails Pro - Lint
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
[error] 15-15: Prettier formatting error. Delete the extra space character at column 42.
🪛 GitHub Actions: React on Rails Pro - Package Tests
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js
[error] 27-27: ReferenceError: SERVER_BUNDLE_TIMESTAMP is not defined.
🪛 GitHub Check: lint-js-and-ruby
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
[failure] 60-60:
Replace redisRequestId with (redisRequestId)
[failure] 59-59:
Replace .fill(null) with ⏎····.fill(null)⏎····
[failure] 56-56:
Insert ;
[failure] 55-55:
Unsafe member access .html on an any value
[failure] 55-55:
Unsafe return of a value of type any
[failure] 55-55:
Replace .split("\n").map(chunk·=>·chunk.trim().length·>·0·?·JSON.parse(chunk).html·:·chunk).join("" with ⏎····.split('\n')⏎····.map((chunk)·=>·(chunk.trim().length·>·0·?·JSON.parse(chunk).html·:·chunk))⏎····.join(''
[failure] 43-43:
Replace "Closed·redis" with 'Closed·redis'
[failure] 41-41:
Replace "Closed·app" with 'Closed·app'
[failure] 39-39:
Replace "Closing·app" with 'Closing·app'
[failure] 15-15:
Delete ·
🔇 Additional comments (9)
react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts (3)
9-11: LGTM!The test timestamp constants are clearly defined with appropriate test values.
13-65: LGTM!The
createFormfunction correctly assembles a multipart form payload with all necessary components for testing server render requests.
158-166: LGTM!The return object provides a well-structured API for tests to monitor streaming responses, with proper encapsulation via
getBufferreturning a copy of the internal buffer.react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.ts (1)
39-39: LGTM! Connection promise tracking added.This variable enables serialization of concurrent connection attempts, preventing race conditions during Redis client initialization.
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts (5)
32-44: LGTM! Test lifecycle setup is appropriate.The test setup properly initializes Redis client and app, with appropriate cleanup in
afterAll. The 20-second timeout accommodates potential delays in shutting down the server and closing connections.
46-52: LGTM! Redis helper functions are clear.These helper functions provide a clean abstraction for sending values to Redis streams in the test.
67-75: RegExp constructed from variable (low risk).Static analysis warns about constructing RegExp from the variable
redisRequestId(lines 72, and similar warnings throughout). While this could theoretically enable ReDoS attacks,redisRequestIdis a UUID generated byrandomUUID(), so it's controlled and safe.If you want to be extra defensive, you could escape the UUID:
const escapedId = redisRequestId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); expect(extractHtmlFromChunks(chunk.replace(new RegExp(escapedId, 'g'), '')))However, since UUIDs have a fixed format with only alphanumeric characters and hyphens, this is not necessary for security.
76-84: LGTM! Helper functions complete the test orchestration.These functions properly coordinate sending values and waiting for completion across all parallel instances.
134-147: LGTM! Parallel execution test validates concurrent behavior.The test properly validates that 20 concurrent renders produce the same chunked output as a single render, ensuring thread-safety and correctness of the streaming implementation.
| // const runningPromises: (string | undefined)[] = []; | ||
| // let isRunning = false; | ||
| // const OldPromise = globalThis.Promise; | ||
| // globalThis.Promise = class Promise<T> extends OldPromise<T> { | ||
| // constructor(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) { | ||
| // super(executor); // call native Promise constructor | ||
| // if (!isRunning) { | ||
| // isRunning = true; | ||
| // const stack = new Error().stack; | ||
| // runningPromises.push(stack); | ||
| // this.then(() => { | ||
| // const index = runningPromises.indexOf(stack); | ||
| // runningPromises.splice(index, 1); | ||
| // }); | ||
| // isRunning = false; | ||
| // } | ||
| // } | ||
| // }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
This commented-out Promise tracking code should be removed before merging. If this debugging utility is needed in the future, it can be retrieved from git history.
Apply this diff:
-// const runningPromises: (string | undefined)[] = [];
-// let isRunning = false;
-// const OldPromise = globalThis.Promise;
-// globalThis.Promise = class Promise<T> extends OldPromise<T> {
-// constructor(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) {
-// super(executor); // call native Promise constructor
-// if (!isRunning) {
-// isRunning = true;
-// const stack = new Error().stack;
-// runningPromises.push(stack);
-// this.then(() => {
-// const index = runningPromises.indexOf(stack);
-// runningPromises.splice(index, 1);
-// });
-// isRunning = false;
-// }
-// }
-// };
-📝 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.
| // const runningPromises: (string | undefined)[] = []; | |
| // let isRunning = false; | |
| // const OldPromise = globalThis.Promise; | |
| // globalThis.Promise = class Promise<T> extends OldPromise<T> { | |
| // constructor(executor: (resolve: (value: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) { | |
| // super(executor); // call native Promise constructor | |
| // if (!isRunning) { | |
| // isRunning = true; | |
| // const stack = new Error().stack; | |
| // runningPromises.push(stack); | |
| // this.then(() => { | |
| // const index = runningPromises.indexOf(stack); | |
| // runningPromises.splice(index, 1); | |
| // }); | |
| // isRunning = false; | |
| // } | |
| // } | |
| // }; |
🧰 Tools
🪛 GitHub Actions: React on Rails Pro - Lint
[error] 15-15: Prettier formatting error. Delete the extra space character at column 42.
🪛 GitHub Check: lint-js-and-ruby
[failure] 15-15:
Delete ·
🤖 Prompt for AI Agents
In
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
around lines 13 to 30, remove the entire commented-out Promise
tracking/debugging block (the multiple commented lines replacing
globalThis.Promise) so no dead/commented code remains; if any imports or
references were only for that debug snippet, remove them as well and run tests
to confirm nothing else depended on it.
| const extractHtmlFromChunks = (chunks: string) => { | ||
| chunks.split("\n").map(chunk => chunk.trim().length > 0 ? JSON.parse(chunk).html : chunk).join(""); | ||
| } |
There was a problem hiding this comment.
Fix formatting, type safety, and add semicolon.
This function has multiple issues flagged by static analysis and the linter.
Apply this diff to fix all issues:
const extractHtmlFromChunks = (chunks: string) => {
- chunks.split("\n").map(chunk => chunk.trim().length > 0 ? JSON.parse(chunk).html : chunk).join("");
-}
+ return chunks
+ .split('\n')
+ .map((chunk) => (chunk.trim().length > 0 ? (JSON.parse(chunk) as { html: string }).html : chunk))
+ .join('');
+};This addresses:
- Missing return statement and semicolon
- Type safety by casting JSON.parse result
- Prettier formatting (quotes, spacing, wrapping)
As per coding guidelines
📝 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.
| const extractHtmlFromChunks = (chunks: string) => { | |
| chunks.split("\n").map(chunk => chunk.trim().length > 0 ? JSON.parse(chunk).html : chunk).join(""); | |
| } | |
| const extractHtmlFromChunks = (chunks: string) => { | |
| return chunks | |
| .split('\n') | |
| .map((chunk) => (chunk.trim().length > 0 ? (JSON.parse(chunk) as { html: string }).html : chunk)) | |
| .join(''); | |
| }; |
🧰 Tools
🪛 GitHub Check: lint-js-and-ruby
[failure] 56-56:
Insert ;
[failure] 55-55:
Unsafe member access .html on an any value
[failure] 55-55:
Unsafe return of a value of type any
[failure] 55-55:
Replace .split("\n").map(chunk·=>·chunk.trim().length·>·0·?·JSON.parse(chunk).html·:·chunk).join("" with ⏎····.split('\n')⏎····.map((chunk)·=>·(chunk.trim().length·>·0·?·JSON.parse(chunk).html·:·chunk))⏎····.join(''
🤖 Prompt for AI Agents
In
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
around lines 54 to 56, the helper lacks a return statement and semicolon, is not
type-safe when using JSON.parse, and needs Prettier-friendly formatting; update
the function to return the joined string, add a trailing semicolon, cast the
JSON.parse result to an appropriate typed shape (e.g., { html: string }) before
accessing .html, and reformat spacing/quotes/wrapping to match project Prettier
rules.
| const createParallelRenders = (size: number) => { | ||
| const redisRequestIds = Array(size).fill(null).map(() => randomUUID()); | ||
| const renderRequests = redisRequestIds.map(redisRequestId => { | ||
| return makeRequest(app, { | ||
| componentName: 'RedisReceiver', | ||
| props: { requestId: redisRequestId }, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix formatting issues in parallel render setup.
The array initialization and mapping have formatting issues flagged by the linter.
Apply this diff:
const createParallelRenders = (size: number) => {
- const redisRequestIds = Array(size).fill(null).map(() => randomUUID());
- const renderRequests = redisRequestIds.map(redisRequestId => {
+ const redisRequestIds = Array(size)
+ .fill(null)
+ .map(() => randomUUID());
+ const renderRequests = redisRequestIds.map((redisRequestId) => {
return makeRequest(app, {
componentName: 'RedisReceiver',
props: { requestId: redisRequestId },
});
});As per coding guidelines
📝 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.
| const createParallelRenders = (size: number) => { | |
| const redisRequestIds = Array(size).fill(null).map(() => randomUUID()); | |
| const renderRequests = redisRequestIds.map(redisRequestId => { | |
| return makeRequest(app, { | |
| componentName: 'RedisReceiver', | |
| props: { requestId: redisRequestId }, | |
| }); | |
| }); | |
| const createParallelRenders = (size: number) => { | |
| const redisRequestIds = Array(size) | |
| .fill(null) | |
| .map(() => randomUUID()); | |
| const renderRequests = redisRequestIds.map((redisRequestId) => { | |
| return makeRequest(app, { | |
| componentName: 'RedisReceiver', | |
| props: { requestId: redisRequestId }, | |
| }); | |
| }); |
🧰 Tools
🪛 GitHub Check: lint-js-and-ruby
[failure] 60-60:
Replace redisRequestId with (redisRequestId)
[failure] 59-59:
Replace .fill(null) with ⏎····.fill(null)⏎····
🤖 Prompt for AI Agents
In
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
around lines 58 to 66, the array initialization and mapping are misformatted;
replace the Array(size).fill(null).map(...) pattern with a clearer Array.from({
length: size }, () => randomUUID()) for redisRequestIds, simplify the
renderRequests mapping to a concise arrow callback (no unnecessary
return/blocks), and fix spacing/semicolon placement to match project lint rules
so the function is consistently formatted.
| const expectNextChunk = async (expectedNextChunk: string) => { | ||
| const nextChunks = await Promise.all(renderRequests.map(renderRequest => renderRequest.waitForNextChunk())); | ||
| nextChunks.forEach((chunk, index) => { | ||
| const redisRequestId = redisRequestIds[index]!; | ||
| console.log("Asserting Chunk") | ||
| expect(extractHtmlFromChunks(chunk.replace(new RegExp(redisRequestId, 'g'), ''))) | ||
| .toEqual(extractHtmlFromChunks(expectedNextChunk)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statement.
The console.log("Asserting Chunk") on line 71 should be removed as it's debug output not needed in the test.
Apply this diff:
const expectNextChunk = async (expectedNextChunk: string) => {
const nextChunks = await Promise.all(renderRequests.map(renderRequest => renderRequest.waitForNextChunk()));
nextChunks.forEach((chunk, index) => {
const redisRequestId = redisRequestIds[index]!;
- console.log("Asserting Chunk")
expect(extractHtmlFromChunks(chunk.replace(new RegExp(redisRequestId, 'g'), '')))
.toEqual(extractHtmlFromChunks(expectedNextChunk));
});📝 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.
| const expectNextChunk = async (expectedNextChunk: string) => { | |
| const nextChunks = await Promise.all(renderRequests.map(renderRequest => renderRequest.waitForNextChunk())); | |
| nextChunks.forEach((chunk, index) => { | |
| const redisRequestId = redisRequestIds[index]!; | |
| console.log("Asserting Chunk") | |
| expect(extractHtmlFromChunks(chunk.replace(new RegExp(redisRequestId, 'g'), ''))) | |
| .toEqual(extractHtmlFromChunks(expectedNextChunk)); | |
| }); | |
| } | |
| const expectNextChunk = async (expectedNextChunk: string) => { | |
| const nextChunks = await Promise.all(renderRequests.map(renderRequest => renderRequest.waitForNextChunk())); | |
| nextChunks.forEach((chunk, index) => { | |
| const redisRequestId = redisRequestIds[index]!; | |
| expect(extractHtmlFromChunks(chunk.replace(new RegExp(redisRequestId, 'g'), ''))) | |
| .toEqual(extractHtmlFromChunks(expectedNextChunk)); | |
| }); | |
| } |
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 71-71: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
around lines 67 to 75, remove the debug console.log("Asserting Chunk") statement
on line 71 so the test output is not polluted; simply delete that line and keep
the surrounding await and assertions intact.
| test('Happy Path', async () => { | ||
| const parallelInstances = 20; | ||
| expect.assertions(parallelInstances*7 + 7); | ||
| const redisRequestId = randomUUID(); | ||
| const { waitForNextChunk, finishedPromise, getBuffer } = makeRequest(app, { | ||
| componentName: 'RedisReceiver', | ||
| props: { requestId: redisRequestId }, | ||
| }); | ||
| const chunks: string[] = []; | ||
| let chunk = await waitForNextChunk(); | ||
| expect(chunk).not.toContain('Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| sendRedisItemValue(redisRequestId, 0, 'First Unique Value'); | ||
| chunk = await waitForNextChunk(); | ||
| expect(chunk).toContain('First Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| sendRedisItemValue(redisRequestId, 4, 'Fifth Unique Value'); | ||
| chunk = await waitForNextChunk(); | ||
| expect(chunk).toContain('Fifth Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| sendRedisItemValue(redisRequestId, 2, 'Third Unique Value'); | ||
| chunk = await waitForNextChunk(); | ||
| expect(chunk).toContain('Third Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| sendRedisItemValue(redisRequestId, 1, 'Second Unique Value'); | ||
| chunk = await waitForNextChunk(); | ||
| expect(chunk).toContain('Second Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| sendRedisItemValue(redisRequestId, 3, 'Forth Unique Value'); | ||
| chunk = await waitForNextChunk(); | ||
| expect(chunk).toContain('Forth Unique Value'); | ||
| chunks.push(chunk.replace(new RegExp(redisRequestId, 'g'), '')); | ||
|
|
||
| await finishedPromise; | ||
| expect(getBuffer).toHaveLength(0); |
There was a problem hiding this comment.
Fix critical bug: missing function call parentheses.
Line 132 checks getBuffer directly instead of calling it. This will always pass since it's checking the function reference, not the buffer contents.
Apply this diff:
await finishedPromise;
- expect(getBuffer).toHaveLength(0);
+ expect(getBuffer()).toHaveLength(0);This is consistent with line 83 which correctly calls getBuffer():
renderRequests.forEach(renderRequest => expect(renderRequest.getBuffer()).toHaveLength(0));🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 103-103: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 108-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 113-113: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 118-118: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 123-123: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 128-128: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(redisRequestId, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In
react_on_rails_pro/packages/node-renderer/tests/concurrentHtmlStreaming.test.ts
around lines 93 to 132, the final assertion checks the getBuffer function
reference instead of its result; update the assertion to call getBuffer() so it
asserts the buffer length (i.e., replace expect(getBuffer).toHaveLength(0) with
expect(getBuffer()).toHaveLength(0)).
| const getAppUrl = (app: ReturnType<typeof buildApp>) => { | ||
| const addresssInfo = app.server.address(); | ||
| if (!addresssInfo) { | ||
| throw new Error('The app has no address, ensure to run the app before running tests'); | ||
| } | ||
|
|
||
| if (typeof addresssInfo === 'string') { | ||
| return addresssInfo; | ||
| } | ||
|
|
||
| return `http://localhost:${addresssInfo.port}`; | ||
| }; |
There was a problem hiding this comment.
Fix typo in variable name.
Line 68 has a typo: addresssInfo should be addressInfo (two s's, not three).
Apply this diff:
- const addresssInfo = app.server.address();
- if (!addresssInfo) {
+ const addressInfo = app.server.address();
+ if (!addressInfo) {
throw new Error('The app has no address, ensure to run the app before running tests');
}
- if (typeof addresssInfo === 'string') {
- return addresssInfo;
+ if (typeof addressInfo === 'string') {
+ return addressInfo;
}
- return `http://localhost:${addresssInfo.port}`;
+ return `http://localhost:${addressInfo.port}`;📝 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.
| const getAppUrl = (app: ReturnType<typeof buildApp>) => { | |
| const addresssInfo = app.server.address(); | |
| if (!addresssInfo) { | |
| throw new Error('The app has no address, ensure to run the app before running tests'); | |
| } | |
| if (typeof addresssInfo === 'string') { | |
| return addresssInfo; | |
| } | |
| return `http://localhost:${addresssInfo.port}`; | |
| }; | |
| const getAppUrl = (app: ReturnType<typeof buildApp>) => { | |
| const addressInfo = app.server.address(); | |
| if (!addressInfo) { | |
| throw new Error('The app has no address, ensure to run the app before running tests'); | |
| } | |
| if (typeof addressInfo === 'string') { | |
| return addressInfo; | |
| } | |
| return `http://localhost:${addressInfo.port}`; | |
| }; |
🤖 Prompt for AI Agents
In react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts around
lines 67 to 78, there's a typo: the variable is declared as addresssInfo (three
s's) but should be addressInfo; rename all occurrences of addresssInfo to
addressInfo so the variable name is consistent (declaration, null check, typeof
check, and property access) and ensure references compile.
| const rejectPendingChunkPromise = () => { | ||
| if (rejectChunksPromise && buffer.length === 0) { | ||
| rejectChunksPromise('Request already eneded'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix typo in error message.
Line 129 has a typo: "Request already eneded" should be "Request already ended".
Apply this diff:
const rejectPendingChunkPromise = () => {
if (rejectChunksPromise && buffer.length === 0) {
- rejectChunksPromise('Request already eneded');
+ rejectChunksPromise('Request already ended');
}
};📝 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.
| const rejectPendingChunkPromise = () => { | |
| if (rejectChunksPromise && buffer.length === 0) { | |
| rejectChunksPromise('Request already eneded'); | |
| } | |
| }; | |
| const rejectPendingChunkPromise = () => { | |
| if (rejectChunksPromise && buffer.length === 0) { | |
| rejectChunksPromise('Request already ended'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts around
lines 127 to 131, fix the typo in the error message inside
rejectPendingChunkPromise: change "Request already eneded" to "Request already
ended" so the error string is spelled correctly.
| const finishedPromise = new Promise<void>((resolve, reject) => { | ||
| request.on('end', () => { | ||
| client.destroy(); | ||
| resolve(); | ||
| rejectPendingChunkPromise(); | ||
| }); | ||
| request.on('error', (err) => { | ||
| client.destroy(); | ||
| reject(err); | ||
| rejectPendingChunkPromise(); | ||
| }); | ||
| }).then(() => console.log("Finished Request")); |
There was a problem hiding this comment.
Remove debug console.log statement.
Line 144 contains a console.log statement that should be removed from production test utility code.
Apply this diff:
request.on('error', (err) => {
client.destroy();
reject(err);
rejectPendingChunkPromise();
});
- }).then(() => console.log("Finished Request"));
+ });📝 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.
| const finishedPromise = new Promise<void>((resolve, reject) => { | |
| request.on('end', () => { | |
| client.destroy(); | |
| resolve(); | |
| rejectPendingChunkPromise(); | |
| }); | |
| request.on('error', (err) => { | |
| client.destroy(); | |
| reject(err); | |
| rejectPendingChunkPromise(); | |
| }); | |
| }).then(() => console.log("Finished Request")); | |
| const finishedPromise = new Promise<void>((resolve, reject) => { | |
| request.on('end', () => { | |
| client.destroy(); | |
| resolve(); | |
| rejectPendingChunkPromise(); | |
| }); | |
| request.on('error', (err) => { | |
| client.destroy(); | |
| reject(err); | |
| rejectPendingChunkPromise(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts around
lines 133 to 144, remove the debug console.log("Finished Request") from the
finishedPromise .then() chain so the test utility doesn't print to stdout;
update the promise chain to simply return the promise (or remove the .then()
entirely) and ensure there are no other stray debug console calls left in this
file.
| const waitForNextChunk = () => | ||
| new Promise<string>((resolve, reject) => { | ||
| if (client.closed && buffer.length === 0) { | ||
| reject('Request already eneded'); | ||
| } | ||
| resolveChunksPromise = resolve; | ||
| rejectChunksPromise = reject; | ||
| if (buffer.length > 0) { | ||
| scheduleResolveChunkPromise(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix typo in error message.
Line 149 has a typo: "Request already eneded" should be "Request already ended" (same typo as Line 129).
Apply this diff:
const waitForNextChunk = () =>
new Promise<string>((resolve, reject) => {
if (client.closed && buffer.length === 0) {
- reject('Request already eneded');
+ reject('Request already ended');
}
resolveChunksPromise = resolve;
rejectChunksPromise = reject;
if (buffer.length > 0) {
scheduleResolveChunkPromise();
}
});📝 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.
| const waitForNextChunk = () => | |
| new Promise<string>((resolve, reject) => { | |
| if (client.closed && buffer.length === 0) { | |
| reject('Request already eneded'); | |
| } | |
| resolveChunksPromise = resolve; | |
| rejectChunksPromise = reject; | |
| if (buffer.length > 0) { | |
| scheduleResolveChunkPromise(); | |
| } | |
| }); | |
| const waitForNextChunk = () => | |
| new Promise<string>((resolve, reject) => { | |
| if (client.closed && buffer.length === 0) { | |
| reject('Request already ended'); | |
| } | |
| resolveChunksPromise = resolve; | |
| rejectChunksPromise = reject; | |
| if (buffer.length > 0) { | |
| scheduleResolveChunkPromise(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In react_on_rails_pro/packages/node-renderer/tests/httpRequestUtils.ts around
lines 146 to 156, fix the typo in the rejection message ("Request already
eneded") by changing it to "Request already ended" to match the same message
used at line 129; update only the string literal in the reject(...) call.
| async function closeRedisClient() { | ||
| if (sharedRedisClient && isClientConnected) { | ||
| await sharedRedisClient.quit(); | ||
| const currendRedisClient = sharedRedisClient; |
There was a problem hiding this comment.
Fix the typo in variable name.
The variable name has a typo: currendRedisClient should be currentRedisClient.
Apply this diff:
- const currendRedisClient = sharedRedisClient;
+ const currentRedisClient = sharedRedisClient;
sharedRedisClient = null;
connectionPromise = undefined;
- await currendRedisClient.quit();
+ await currentRedisClient.quit();📝 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.
| const currendRedisClient = sharedRedisClient; | |
| const currentRedisClient = sharedRedisClient; | |
| sharedRedisClient = null; | |
| connectionPromise = undefined; | |
| await currentRedisClient.quit(); |
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/client/app/utils/redisReceiver.ts around line
73, there is a typo in the variable name `currendRedisClient`; rename it to
`currentRedisClient` and update all references/usages in this file to use the
corrected name so identifiers remain consistent and compile without errors.
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 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
Summary by CodeRabbit
Changes
Chores
Tests