Conversation
WalkthroughThis pull request introduces comprehensive support for React Server Components (RSC) across the Rails application. It revises dependency and configuration management in Ruby and JavaScript, updates gem and package files to use new loaders and GitHub branches, and adds new constants and methods in the configuration, request, and rendering modules. New React components, streaming support, and enhanced webpack and node renderer configurations have been integrated. In addition, extensive documentation, test suites, and CI configuration updates have been added or modified to support the RSC payload, streaming, selective hydration, and improved error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant RailsServer
participant RSCPayloadRenderer
participant RendererPool
Browser->>RailsServer: Request page with React component
RailsServer->>RSCPayloadRenderer: Invoke rsc_payload method
RSCPayloadRenderer->>RendererPool: Render RSC payload view (stream view)
RendererPool-->>RSCPayloadRenderer: Return streamed HTML
RSCPayloadRenderer-->>RailsServer: Deliver complete RSC payload
RailsServer-->>Browser: Stream HTML with embedded RSC content
Suggested labels
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
6a138c9 to
da0d2ea
Compare
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
1913c16 to
81d31f6
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
3 times, most recently
from
e8858b9 to
c8b5c5b
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
77ab066 to
2847b05
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro403-use-render-to-readable-stream-to-render-async
branch
from
4b0614e to
6cc2b0c
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
67adcdd to
05f8a5d
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
3ee81a4 to
291ab7e
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
413face to
a0b9ad6
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
eeaadf7 to
7821a0e
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
from
6894f7a to
c433e2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (21)
spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (1)
35-75: Consider refactoring thesendmethod to reduce complexityThe
sendmethod has a high Assignment Branch Condition size (ABC size) of 32.95, which exceeds the RuboCop recommended maximum of 28. Refactoring this method into smaller, focused methods can improve readability and maintainability.One approach is to extract the mock lookup and response handling into separate methods:
def send(request) request_uri = request.uri.to_s - mock = MockStream.mock_responses.find do |pattern, _responses| - case pattern - when String - pattern == request_uri - when Regexp - pattern.match?(request_uri) - end - end + mock = find_mock_for_request(request_uri) raise "Unmocked request detected! URI: #{request_uri}, Method: #{request.verb}" unless mock pattern, responses = mock current_mock = responses.first status, mock_block, count, request_data = current_mock request_data[:request] = request response = build_mock_response(request, status) request.response = response request.emit(:response, response) yielder = lambda { |value| response << value } mock_block.call(yielder, request) manage_mock_count(pattern, responses, current_mock, count) end +private +def find_mock_for_request(request_uri) + MockStream.mock_responses.find do |pattern, _responses| + case pattern + when String + pattern == request_uri + when Regexp + pattern.match?(request_uri) + end + end +end +def build_mock_response(request, status) + request.options.response_class.new(request, status, "2.0", {}).tap do |res| + res.mocked = true + end +end +def manage_mock_count(pattern, responses, current_mock, count) + return if count == Float::INFINITY + + count -= 1 + if count.zero? + responses.shift + MockStream.mock_responses.delete(pattern) if responses.empty? + else + current_mock[2] = count + end +end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 35-75: Assignment Branch Condition size for send is too high. [<19, 25, 10> 32.95/28]
(Metrics/AbcSize)
lib/react_on_rails_pro/routes.rb (1)
1-7: Add frozen string literal comment and documentation.The implementation looks good! Consider these enhancements:
- Add frozen string literal comment at the top of the file.
- Add YARD documentation for the method parameters.
+# frozen_string_literal: true + module ReactOnRailsPro module Routes + # Defines a route for handling React Server Components (RSC) requests. + # @param path [String] The base path for RSC routes (default: "rsc") + # @param controller [String] The controller to handle RSC requests (default: "react_on_rails_pro/rsc") + # @param options [Hash] Additional options for route configuration + # @option options [Symbol] :as The route name (default: :rsc) def rsc_route(path: "rsc", controller: "react_on_rails_pro/rsc", **options)🧰 Tools
🪛 rubocop (1.69.1)
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
lib/react_on_rails_pro/server_rendering_js_code.rb (1)
18-22: Fix indentation and line length issues.The code has formatting issues that should be addressed.
rsc_props_if_rsc_request = if render_options.rsc? - "reactClientManifestFileName: '#{ReactOnRails.configuration.react_client_manifest_file}'," - else - "" - end + "reactClientManifestFileName: '#{ReactOnRails.configuration.react_client_manifest_file}'," +else + "" +end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 19-19: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 19-19: Line is too long. [129/120]
(Layout/LineLength)
[convention] 20-20: Align
elsewithif.(Layout/ElseAlignment)
[warning] 22-22:
endat 22, 37 is not aligned withifat 18, 35.(Layout/EndAlignment)
spec/react_on_rails_pro/stream_request_spec.rb (1)
9-9: Use verifying doubles for better test reliability.Replace the normal double with a verifying double to catch interface changes early.
- let(:mock_component) { double("MockComponent") } + let(:mock_component) { instance_double("ReactOnRailsPro::StreamRequest") }🧰 Tools
🪛 rubocop (1.69.1)
[convention] 9-9: Prefer using verifying doubles over normal doubles.
(RSpec/VerifiedDoubles)
spec/dummy/app/controllers/pages_controller.rb (1)
43-45: Add documentation for the RSC-specific method.Please add documentation explaining the purpose and usage of
rsc_posts_page. Consider including:
- Expected behavior
- Required parameters
- Response format
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb (1)
80-85: Improve readability of bundle hash assignment.The line assigning bundle_hash is too long. Consider breaking it into multiple lines for better readability.
- bundle_hash = is_rendering_rsc_payload ? ReactOnRailsPro::Utils.rsc_bundle_hash : ReactOnRailsPro::Utils.bundle_hash + bundle_hash = if is_rendering_rsc_payload + ReactOnRailsPro::Utils.rsc_bundle_hash + else + ReactOnRailsPro::Utils.bundle_hash + end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 81-81: Line is too long. [126/120]
(Layout/LineLength)
lib/react_on_rails_pro/utils.rb (1)
132-137: Review password handling in common form data.The password selection logic exposes both RSC and non-RSC passwords. Consider:
- Using environment variables for passwords
- Adding password rotation support
- Implementing password validation
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 136-136: Line is too long. [138/120]
(Layout/LineLength)
spec/react_on_rails_pro/request_spec.rb (2)
25-27: Improve test readability by combining multiple stubs.Consider using
receive_messagesto combine multiple stubs for better readability:- allow(ReactOnRailsPro.configuration).to receive(:renderer_url).and_return(renderer_url) - allow(ReactOnRailsPro.configuration).to receive(:rsc_renderer_url).and_return(rsc_renderer_url) + allow(ReactOnRailsPro.configuration).to receive_messages( + renderer_url: renderer_url, + rsc_renderer_url: rsc_renderer_url + ) - allow(ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool).to receive(:renderer_bundle_file_name).and_return(renderer_bundle_file_name) - allow(ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool).to receive(:rsc_renderer_bundle_file_name).and_return(rsc_renderer_bundle_file_name) + allow(ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool).to receive_messages( + renderer_bundle_file_name: renderer_bundle_file_name, + rsc_renderer_bundle_file_name: rsc_renderer_bundle_file_name + )Also applies to: 34-35
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 25-25: Use
receive_messagesinstead of multiple stubs on lines [26].(RSpec/ReceiveMessages)
[convention] 26-26: Use
receive_messagesinstead of multiple stubs on lines [25].(RSpec/ReceiveMessages)
151-166: Consider adding test cases for server errors (500-599).The current test suite covers client errors (400-499) but doesn't test server errors. Consider adding test cases for server errors to ensure comprehensive error handling:
(500..599).step(20).each do |status_code| it "raises an error when server returns error with status code #{status_code}" do mocked_block = mock_block mock_streaming_response(render_full_url, status_code) do |yielder| yielder.call("Internal Server Error") end stream = described_class.render_code_as_stream("/render", "console.log('Hello, world!');", false) expect do stream.each_chunk(&mocked_block.block) end.to raise_error(ReactOnRailsPro::Error, /#{status_code}:\nInternal Server Error/) expect(mocked_block).not_to have_received(:call) end endpackages/node-renderer/src/worker/vm.ts (1)
Line range hint
107-124: Consider using a more robust console implementation.The current console implementation might miss some features of the original console object. Consider:
- Adding support for additional console methods like
debug,trace, etc.- Preserving the original method's return value.
- Handling complex objects and formatting.
Here's a more robust implementation:
- debugConsole = console; - console = { - get history() { - return sharedConsoleHistory.getConsoleHistory(); - }, - set history(value) { - // Do nothing. It's just for the backward compatibility. - }, - }; - ['error', 'log', 'info', 'warn'].forEach(function (level) { - console[level] = function () { - var argArray = Array.prototype.slice.call(arguments); - if (argArray.length > 0) { - argArray[0] = '[SERVER] ' + argArray[0]; - } - sharedConsoleHistory.addToConsoleHistory({level: level, arguments: argArray}); - }; - }); + debugConsole = console; + console = Object.keys(console).reduce((acc, key) => { + if (typeof console[key] === 'function') { + acc[key] = function (...args) { + const result = debugConsole[key](...args); + if (['error', 'log', 'info', 'warn', 'debug', 'trace'].includes(key)) { + const formattedArgs = args.length > 0 ? ['[SERVER] ' + args[0], ...args.slice(1)] : args; + sharedConsoleHistory.addToConsoleHistory({ + level: key, + arguments: formattedArgs.map(arg => + typeof arg === 'object' ? JSON.stringify(arg) : String(arg) + ) + }); + } + return result; + }; + } else if (key === 'history') { + Object.defineProperty(acc, key, { + get: () => sharedConsoleHistory.getConsoleHistory(), + set: () => {} // For backward compatibility + }); + } else { + acc[key] = console[key]; + } + return acc; + }, {});spec/react_on_rails_pro/utils_spec.rb (2)
26-27: Improve test readability by combining multiple stubs.Similar to previous suggestions, use
receive_messagesto combine multiple stubs:- allow(ReactOnRails.configuration) - .to receive(:server_bundle_js_file).and_return(nil) - allow(ReactOnRails.configuration) - .to receive(:rsc_bundle_js_file).and_return(nil) + allow(ReactOnRails.configuration).to receive_messages( + server_bundle_js_file: nil, + rsc_bundle_js_file: nil + )🧰 Tools
🪛 rubocop (1.69.1)
[convention] 26-27: Use
receive_messagesinstead of multiple stubs on lines [24].(RSpec/ReceiveMessages)
77-101: Consider adding edge cases to RSC bundle tests.The test coverage for RSC bundle is good but could be enhanced with additional edge cases:
- Test when the RSC bundle file doesn't exist
- Test when the RSC bundle file is empty
- Test when the RSC bundle file is corrupted
Here's an example implementation:
context "with rsc bundle edge cases" do it "handles missing RSC bundle file" do allow(ReactOnRails::Utils).to receive(:rsc_bundle_js_file_path) .and_return("non_existent_file.js") expect { described_class.rsc_bundle_hash }.to raise_error(ReactOnRailsPro::Error) end it "handles empty RSC bundle file" do allow(File).to receive(:read).and_return("") allow(ReactOnRails::Utils).to receive(:rsc_bundle_js_file_path) .and_return(rsc_bundle_js_file_path) result = described_class.rsc_bundle_hash expect(result).not_to be_nil end end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 84-85: Use
receive_messagesinstead of multiple stubs on lines [86].(RSpec/ReceiveMessages)
[convention] 86-87: Use
receive_messagesinstead of multiple stubs on lines [84].(RSpec/ReceiveMessages)
[convention] 88-89: Use
receive_messagesinstead of multiple stubs on lines [90].(RSpec/ReceiveMessages)
[convention] 90-91: Use
receive_messagesinstead of multiple stubs on lines [88].(RSpec/ReceiveMessages)
lib/react_on_rails_pro/configuration.rb (2)
14-14: Add documentation for RSC configuration options.The new RSC configuration options lack documentation. Consider adding YARD-style documentation:
# @!attribute [rw] rsc_renderer_url # @return [String] The URL of the RSC renderer server # @example # config.rsc_renderer_url = "http://localhost:3900" # @!attribute [rw] rsc_renderer_password # @return [String] The password for authenticating with the RSC renderer # @example # config.rsc_renderer_password = "secret"Also applies to: 20-20
154-154: Fix line length to improve readability.The error message line exceeds the 120-character limit. Consider splitting it:
- message = "Unparseable ReactOnRailsPro.config.renderer_url #{renderer_url} or ReactOnRailsPro.config.renderer_url #{rsc_renderer_url} provided.\n#{e.message}" + message = "Unparseable URLs provided.\n" \ + "renderer_url: #{renderer_url}\n" \ + "rsc_renderer_url: #{rsc_renderer_url}\n" \ + "Error: #{e.message}"🧰 Tools
🪛 rubocop (1.69.1)
[convention] 154-154: Line is too long. [164/120]
(Layout/LineLength)
spec/react_on_rails_pro/stream_spec.rb (2)
9-27: Improve block style for better readability.The current block style can be improved:
- let(:http) { HTTPX.plugin(:mock_stream) - .plugin(:retries, max_retries: 1, retry_change_requests: true) - .plugin(:stream) - .with( - origin: url, - fallback_protocol: "h2", - max_concurrent_requests: 10, - persistent: true, - timeout: { - connect_timeout: 30, - read_timeout: 30 - } - ) } + let(:http) do + HTTPX + .plugin(:mock_stream) + .plugin(:retries, max_retries: 1, retry_change_requests: true) + .plugin(:stream) + .with( + origin: url, + fallback_protocol: "h2", + max_concurrent_requests: 10, + persistent: true, + timeout: { + connect_timeout: 30, + read_timeout: 30 + } + ) + end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 9-9: Avoid using
{...}for multi-line blocks.(Style/BlockDelimiters)
[convention] 9-27: Block body expression is on the same line as the block start.
(Layout/MultilineBlockLayout)
[convention] 10-10: Align
.pluginwith.pluginon line 9.(Layout/MultilineMethodCallIndentation)
[convention] 11-11: Align
.pluginwith.pluginon line 9.(Layout/MultilineMethodCallIndentation)
[convention] 12-12: Align
.withwith.pluginon line 9.(Layout/MultilineMethodCallIndentation)
[convention] 27-27: Expression at 27, 7 should be on its own line.
(Layout/BlockEndNewline)
237-237: Remove unused variable.The
chunksvariable is assigned but never used. Consider removing it or using it in the test:- chunks = [] response.each(&mocked_block.block)🧰 Tools
🪛 rubocop (1.69.1)
[warning] 237-237: Useless assignment to variable -
chunks.(Lint/UselessAssignment)
lib/react_on_rails_pro/request.rb (4)
12-14: Consider lazy initialization of RSC connectionThe RSC connection is always initialized alongside the standard connection, which could lead to unnecessary resource usage if RSC is not being used. Consider initializing the RSC connection only when needed.
def reset_connection @connection&.close @rsc_connection&.close @connection = create_connection(url: ReactOnRailsPro.configuration.renderer_url) - @rsc_connection = create_connection(url: ReactOnRailsPro.configuration.rsc_renderer_url) end
30-30: Address the TODO comment for RSC assets supportThe TODO comment indicates incomplete support for uploading RSC assets.
Would you like me to help implement the RSC assets upload support or create a GitHub issue to track this task?
35-37: Use guard clause for better readabilityThe conditional block can be simplified using a guard clause.
-if ReactOnRails.configuration.rsc_bundle_js_file - perform_request("/upload-assets", true, form: form_with_assets_and_bundle(rsc_renderer: true)) -end +return unless ReactOnRails.configuration.rsc_bundle_js_file + +perform_request("/upload-assets", true, form: form_with_assets_and_bundle(rsc_renderer: true))🧰 Tools
🪛 rubocop (1.69.1)
[convention] 35-35: Use a guard clause (
return unless ReactOnRails.configuration.rsc_bundle_js_file) instead of wrapping the code inside a conditional expression.(Style/GuardClause)
57-64: Extract connection selection logicThe method is becoming complex with multiple responsibilities. Consider extracting the connection selection logic into a separate method.
+def select_connection(is_rendering_rsc_payload) + is_rendering_rsc_payload ? rsc_connection : connection +end def perform_request(path, is_rendering_rsc_payload, **post_options) available_retries = ReactOnRailsPro.configuration.renderer_request_retry_limit retry_request = true while retry_request begin start_time = Time.now - used_connection = is_rendering_rsc_payload ? rsc_connection : connection + used_connection = select_connection(is_rendering_rsc_payload) response = used_connection.post(path, **post_options)🧰 Tools
🪛 rubocop (1.69.1)
[convention] 57-103: Perceived complexity for perform_request is too high. [11/10]
(Metrics/PerceivedComplexity)
spec/dummy/config/puma.rb (1)
Line range hint
50-51: Remove extra trailing newlineThere is an extra trailing newline at the end of the file that should be removed to follow the project's conventions.
plugin :tmp_restart -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (99)
.circleci/config.yml(11 hunks).eslintrc(1 hunks)Gemfile.development_dependencies(2 hunks)app/controllers/react_on_rails_pro/rsc_controller.rb(1 hunks)app/views/react_on_rails_pro/rsc.html.erb(1 hunks)lib/react_on_rails_pro.rb(1 hunks)lib/react_on_rails_pro/concerns/rsc_controller_concern.rb(1 hunks)lib/react_on_rails_pro/configuration.rb(6 hunks)lib/react_on_rails_pro/engine.rb(1 hunks)lib/react_on_rails_pro/request.rb(5 hunks)lib/react_on_rails_pro/routes.rb(1 hunks)lib/react_on_rails_pro/server_rendering_js_code.rb(2 hunks)lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb(3 hunks)lib/react_on_rails_pro/server_rendering_pool/pro_rendering.rb(1 hunks)lib/react_on_rails_pro/stream.rb(1 hunks)lib/react_on_rails_pro/stream_request.rb(3 hunks)lib/react_on_rails_pro/utils.rb(3 hunks)package-scripts.yml(2 hunks)packages/node-renderer/src/worker/vm.ts(3 hunks)spec/dummy/Procfile.dev(1 hunks)spec/dummy/app/controllers/graphql_controller.rb(1 hunks)spec/dummy/app/controllers/pages_controller.rb(3 hunks)spec/dummy/app/views/pages/rsc_posts_page.html.erb(1 hunks)spec/dummy/app/views/pages/stream_async_components.html.erb(1 hunks)spec/dummy/app/views/shared/_menu.erb(1 hunks)spec/dummy/client/app/components/ErrorBoundary.ts(1 hunks)spec/dummy/client/app/components/ErrorComponent.jsx(1 hunks)spec/dummy/client/app/components/HelloWorld.jsx(1 hunks)spec/dummy/client/app/components/HelloWorldRedux.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/Comment.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/Comments.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/Post.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/Posts.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx(1 hunks)spec/dummy/client/app/components/RSCPostsPage/User.jsx(1 hunks)spec/dummy/client/app/components/SimpleClientComponent.jsx(1 hunks)spec/dummy/client/app/loadable/loadable-server.imports-loadable.jsx(1 hunks)spec/dummy/client/app/packs/client-bundle.js(1 hunks)spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.server.jsx(2 hunks)spec/dummy/client/app/ror-auto-load-components/CacheDisabled.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ConsoleLogsInAsyncServer.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorld.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldApp.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldES5.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldHooksContext.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldWithLogAndThrow.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/Loadable.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/Loadable.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ManualRenderApp.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/RSCPostsPage.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReactHelmetApp.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReduxApp.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReduxApp.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/RenderedHtml.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/RenderedHtml.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/RouterApp.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/SimpleComponent.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/StreamAsyncComponents.jsx(1 hunks)spec/dummy/client/app/stores/SharedReduxStore.jsx(1 hunks)spec/dummy/client/node-renderer.js(2 hunks)spec/dummy/config/initializers/react_on_rails.rb(2 hunks)spec/dummy/config/initializers/react_on_rails_pro.rb(1 hunks)spec/dummy/config/puma.rb(1 hunks)spec/dummy/config/routes.rb(1 hunks)spec/dummy/config/webpack/ServerClientOrBoth.mjs(1 hunks)spec/dummy/config/webpack/alias.js(0 hunks)spec/dummy/config/webpack/alias.mjs(1 hunks)spec/dummy/config/webpack/clientWebpackConfig.mjs(2 hunks)spec/dummy/config/webpack/commonWebpackConfig.mjs(2 hunks)spec/dummy/config/webpack/development.mjs(2 hunks)spec/dummy/config/webpack/production.js(0 hunks)spec/dummy/config/webpack/production.mjs(1 hunks)spec/dummy/config/webpack/rsc-transform-loader.mjs(1 hunks)spec/dummy/config/webpack/rscWebpackConfig.mjs(1 hunks)spec/dummy/config/webpack/serverWebpackConfig.mjs(3 hunks)spec/dummy/config/webpack/test.mjs(1 hunks)spec/dummy/config/webpack/webpack.config.js(1 hunks)spec/dummy/package.json(5 hunks)spec/dummy/patches/react-server-dom-webpack+18.3.0-canary-670811593-20240322.patch(1 hunks)spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb(3 hunks)spec/dummy/spec/rails_helper.rb(2 hunks)spec/react_on_rails_pro/configuration_spec.rb(2 hunks)spec/react_on_rails_pro/request_spec.rb(1 hunks)spec/react_on_rails_pro/spec_helper.rb(1 hunks)spec/react_on_rails_pro/stream_request_spec.rb(1 hunks)spec/react_on_rails_pro/stream_spec.rb(1 hunks)spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb(1 hunks)spec/react_on_rails_pro/support/mock_block_helper.rb(1 hunks)spec/react_on_rails_pro/support/mock_stream_helper.rb(1 hunks)spec/react_on_rails_pro/utils_spec.rb(3 hunks)
💤 Files with no reviewable changes (2)
- spec/dummy/config/webpack/production.js
- spec/dummy/config/webpack/alias.js
🚧 Files skipped from review as they are similar to previous changes (76)
- lib/react_on_rails_pro.rb
- spec/dummy/config/webpack/production.mjs
- spec/dummy/client/app/ror-auto-load-components/Loadable.client.jsx
- spec/react_on_rails_pro/spec_helper.rb
- spec/dummy/client/app/components/ErrorBoundary.ts
- spec/dummy/client/app/components/HelloWorldRedux.jsx
- spec/dummy/client/app/components/ErrorComponent.jsx
- spec/dummy/client/app/ror-auto-load-components/Loadable.server.jsx
- spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx
- spec/dummy/client/app/components/SimpleClientComponent.jsx
- lib/react_on_rails_pro/engine.rb
- spec/dummy/client/app/ror-auto-load-components/RenderedHtml.client.jsx
- spec/dummy/client/app/ror-auto-load-components/ManualRenderApp.jsx
- spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.client.jsx
- spec/dummy/app/views/pages/stream_async_components.html.erb
- spec/dummy/client/app/ror-auto-load-components/CacheDisabled.jsx
- spec/dummy/client/app/ror-auto-load-components/RouterApp.server.jsx
- spec/dummy/client/app/stores/SharedReduxStore.jsx
- spec/dummy/client/app/ror-auto-load-components/ReactHelmetApp.server.jsx
- spec/dummy/client/app/ror-auto-load-components/HelloWorldApp.jsx
- spec/dummy/client/app/ror-auto-load-components/HelloWorldWithLogAndThrow.jsx
- spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.client.jsx
- spec/dummy/client/app/ror-auto-load-components/RenderedHtml.server.jsx
- spec/dummy/client/app/components/HelloWorld.jsx
- app/views/react_on_rails_pro/rsc.html.erb
- spec/dummy/client/app/ror-auto-load-components/HelloWorldHooks.jsx
- spec/dummy/app/views/shared/_menu.erb
- spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.server.jsx
- spec/dummy/config/routes.rb
- spec/dummy/config/initializers/react_on_rails_pro.rb
- app/controllers/react_on_rails_pro/rsc_controller.rb
- lib/react_on_rails_pro/server_rendering_pool/pro_rendering.rb
- spec/dummy/client/app/components/RSCPostsPage/Posts.jsx
- spec/dummy/config/initializers/react_on_rails.rb
- spec/dummy/client/app/components/RSCPostsPage/Comment.jsx
- spec/dummy/client/app/ror-auto-load-components/ConsoleLogsInAsyncServer.client.jsx
- spec/dummy/config/webpack/development.mjs
- spec/dummy/app/controllers/graphql_controller.rb
- spec/dummy/client/app/ror-auto-load-components/HelloWorldHooksContext.jsx
- spec/dummy/Procfile.dev
- spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx
- spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsx
- spec/dummy/config/webpack/rsc-transform-loader.mjs
- spec/dummy/config/webpack/webpack.config.js
- spec/react_on_rails_pro/configuration_spec.rb
- spec/dummy/app/views/pages/rsc_posts_page.html.erb
- spec/dummy/patches/react-server-dom-webpack+18.3.0-canary-670811593-20240322.patch
- spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.server.jsx
- spec/dummy/client/app/loadable/loadable-server.imports-loadable.jsx
- Gemfile.development_dependencies
- spec/react_on_rails_pro/support/mock_block_helper.rb
- spec/react_on_rails_pro/support/mock_stream_helper.rb
- spec/dummy/client/app/ror-auto-load-components/ReduxApp.client.jsx
- spec/dummy/config/webpack/clientWebpackConfig.mjs
- spec/dummy/client/app/ror-auto-load-components/ReduxApp.server.jsx
- spec/dummy/config/webpack/alias.mjs
- spec/dummy/client/app/ror-auto-load-components/HelloWorldES5.jsx
- spec/dummy/config/webpack/rscWebpackConfig.mjs
- .eslintrc
- spec/dummy/config/webpack/test.mjs
- spec/dummy/config/webpack/commonWebpackConfig.mjs
- spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx
- spec/dummy/client/app/components/RSCPostsPage/Comments.jsx
- spec/dummy/client/app/packs/client-bundle.js
- spec/dummy/config/webpack/ServerClientOrBoth.mjs
- spec/dummy/client/app/ror-auto-load-components/RSCPostsPage.jsx
- spec/dummy/client/app/ror-auto-load-components/StreamAsyncComponents.jsx
- spec/dummy/client/app/ror-auto-load-components/HelloWorld.jsx
- spec/dummy/client/app/components/RSCPostsPage/Post.jsx
- spec/dummy/config/webpack/serverWebpackConfig.mjs
- lib/react_on_rails_pro/concerns/rsc_controller_concern.rb
- spec/dummy/client/app/components/RSCPostsPage/User.jsx
- spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx
- spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsx
- package-scripts.yml
- spec/dummy/package.json
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/react_on_rails_pro/utils_spec.rb
[convention] 26-27: Use receive_messages instead of multiple stubs on lines [24].
(RSpec/ReceiveMessages)
[convention] 47-48: Use receive_messages instead of multiple stubs on lines [45].
(RSpec/ReceiveMessages)
[convention] 84-85: Use receive_messages instead of multiple stubs on lines [86].
(RSpec/ReceiveMessages)
[convention] 86-87: Use receive_messages instead of multiple stubs on lines [84].
(RSpec/ReceiveMessages)
[convention] 88-89: Use receive_messages instead of multiple stubs on lines [90].
(RSpec/ReceiveMessages)
[convention] 90-91: Use receive_messages instead of multiple stubs on lines [88].
(RSpec/ReceiveMessages)
lib/react_on_rails_pro/utils.rb
[convention] 136-136: Line is too long. [138/120]
(Layout/LineLength)
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb
[convention] 81-81: Line is too long. [126/120]
(Layout/LineLength)
lib/react_on_rails_pro/configuration.rb
[convention] 154-154: Line is too long. [164/120]
(Layout/LineLength)
lib/react_on_rails_pro/request.rb
[convention] 35-35: Use a guard clause (return unless ReactOnRails.configuration.rsc_bundle_js_file) instead of wrapping the code inside a conditional expression.
(Style/GuardClause)
[convention] 113-113: Line is too long. [148/120]
(Layout/LineLength)
[convention] 119-119: Line is too long. [123/120]
(Layout/LineLength)
[convention] 166-166: Line is too long. [140/120]
(Layout/LineLength)
spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb
[convention] 35-75: Assignment Branch Condition size for send is too high. [<19, 25, 10> 32.95/28]
(Metrics/AbcSize)
lib/react_on_rails_pro/routes.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
lib/react_on_rails_pro/server_rendering_js_code.rb
[convention] 19-19: Use 2 (not 4) spaces for indentation.
(Layout/IndentationWidth)
[convention] 19-19: Line is too long. [129/120]
(Layout/LineLength)
[convention] 20-20: Align else with if.
(Layout/ElseAlignment)
[warning] 22-22: end at 22, 37 is not aligned with if at 18, 35.
(Layout/EndAlignment)
spec/dummy/config/puma.rb
[convention] 50-51: 1 trailing blank lines detected.
(Layout/TrailingEmptyLines)
spec/react_on_rails_pro/request_spec.rb
[convention] 25-25: Use receive_messages instead of multiple stubs on lines [26].
(RSpec/ReceiveMessages)
[convention] 26-26: Use receive_messages instead of multiple stubs on lines [25].
(RSpec/ReceiveMessages)
[convention] 34-34: Use receive_messages instead of multiple stubs on lines [35].
(RSpec/ReceiveMessages)
[convention] 34-34: Line is too long. [143/120]
(Layout/LineLength)
[convention] 35-35: Use receive_messages instead of multiple stubs on lines [34].
(RSpec/ReceiveMessages)
[convention] 35-35: Line is too long. [151/120]
(Layout/LineLength)
[convention] 36-36: Use receive_messages instead of multiple stubs on lines [38].
(RSpec/ReceiveMessages)
[convention] 38-38: Use receive_messages instead of multiple stubs on lines [36].
(RSpec/ReceiveMessages)
spec/react_on_rails_pro/stream_request_spec.rb
[convention] 6-6: Do not use multiple top-level example groups - try to nest them.
(RSpec/MultipleDescribes)
[convention] 9-9: Prefer using verifying doubles over normal doubles.
(RSpec/VerifiedDoubles)
[convention] 13-13: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 16-16: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 27-27: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 32-32: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 34-34: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 39-39: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 41-41: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 46-46: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 48-48: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 53-53: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
[convention] 58-58: Name your test subject if you need to reference it explicitly.
(RSpec/NamedSubject)
spec/react_on_rails_pro/stream_spec.rb
[convention] 9-9: Avoid using {...} for multi-line blocks.
(Style/BlockDelimiters)
[convention] 9-27: Block body expression is on the same line as the block start.
(Layout/MultilineBlockLayout)
[convention] 10-10: Align .plugin with .plugin on line 9.
(Layout/MultilineMethodCallIndentation)
[convention] 11-11: Align .plugin with .plugin on line 9.
(Layout/MultilineMethodCallIndentation)
[convention] 12-12: Align .with with .plugin on line 9.
(Layout/MultilineMethodCallIndentation)
[convention] 27-27: Expression at 27, 7 should be on its own line.
(Layout/BlockEndNewline)
[warning] 237-237: Useless assignment to variable - chunks.
(Lint/UselessAssignment)
🪛 Biome (1.9.4)
spec/dummy/client/app/ror-auto-load-components/SimpleComponent.jsx
[error] 15-21: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (21)
spec/dummy/spec/rails_helper.rb (2)
44-45: Approved inclusion of mocking utilitiesIncluding
MockStreamHelperand theMockStreamplugin enhances the test suite's capabilities to mock streaming responses, improving test reliability and scope.
82-82: Approved inclusion ofMockStreamHelperin RSpec configurationAdding
MockStreamHelperto the RSpec configuration makes its methods readily available in tests, streamlining the testing of streaming components.spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (3)
185-185: Approved update of component name toTestingStreamableComponentChanging the
component_nameto"TestingStreamableComponent"aligns with the updated component being tested. Ensure that this component is correctly defined and available in the test environment.
204-212: Approved updates toreact_component_specification_tagThe inclusion of
data-store-dependencies="[]"anddata-force-load="true"attributes in the component specification tag reflects the necessary configuration for the updated component. These changes are appropriate for testing the new component specifications.
227-235: Verify the impact of resetting class instance variables and stubbing methodsThe
mock_request_and_responsemethod resets the class instance variables@connectionand@rsc_connectiononReactOnRailsPro::Requestand stubsHTTPX.method(:plugin). This could potentially affect other tests or lead to unexpected behaviors due to shared state.Consider using more isolated mocking techniques, such as RSpec's
beforeandafterhooks, to ensure that these changes do not impact other tests. Ensure that the original state is restored after the test completes.lib/react_on_rails_pro/server_rendering_js_code.rb (1)
11-17: LGTM! Clear conditional logic for render function names.The implementation correctly handles different rendering modes (RSC, streaming, and default).
lib/react_on_rails_pro/stream.rb (2)
5-7: Consider converting to a concern.The module includes
ActionController::Liveand could benefit from being converted to a concern.-module Stream +module Stream + extend ActiveSupport::Concern + + included do + include ActionController::Live + end - def self.included(base) - base.include ActionController::Live - end
Line range hint
28-42: LGTM! Well-structured implementation with clear error handling.The method correctly handles:
- Extraction of options
- Newline handling to prevent hydration errors
- Stream closing
spec/react_on_rails_pro/stream_request_spec.rb (1)
67-74: Add more test coverage for StreamRequest.The StreamRequest specs only cover the basic
.createmethod. Consider adding tests for:
- Error handling
- Different chunk types
- RSC-specific functionality
Would you like me to generate additional test cases for these scenarios?
spec/dummy/app/controllers/pages_controller.rb (2)
4-4: Verify streaming capabilities after removing ActionController::Live.The removal of
ActionController::Livemight affect streaming functionality. Please verify thatReactOnRailsPro::RscControllerConcernprovides equivalent streaming capabilities.✅ Verification successful
Streaming capabilities are maintained ✓
The streaming functionality is preserved as
ReactOnRailsPro::RscControllerConcernincludesReactOnRailsPro::Stream, which providesActionController::Livecapabilities and the same streaming methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for streaming-related functionality in RscControllerConcern rg -A 5 "include ActionController::Live" . ast-grep --pattern 'module ReactOnRailsPro module RscControllerConcern $$$ end end'Length of output: 3658
Line range hint
7-11: Review XSS payload usage in test data.The
XSS_PAYLOADconstant contains potentially dangerous XSS payloads. While this appears to be test data, ensure it's properly escaped during rendering and never exposed in production.✅ Verification successful
XSS payload is safely contained within test code.
The
XSS_PAYLOADconstant is properly isolated within the test dummy application (spec/dummy) and is not accessible from production code. This is an appropriate use of XSS test data for verifying security mechanisms.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for XSS_PAYLOAD usage across the codebase rg -A 5 "XSS_PAYLOAD" .Length of output: 1573
Script:
#!/bin/bash # Check if XSS_PAYLOAD is imported or used outside of spec/dummy rg -g '!spec/**' "XSS_PAYLOAD" . # Look for the actual file structure to confirm this is indeed a test directory fd -t d "spec/dummy"Length of output: 399
Script:
#!/bin/bash # Verify directory structure properly fd --full-path 'spec/dummy' # Check if the controller is imported anywhere rg -g '!spec/**' "pages_controller" .Length of output: 18006
spec/dummy/client/node-renderer.js (2)
58-58: Good addition of queueMicrotask for better Node.js compatibility.Adding
queueMicrotasktoadditionalContextimproves compatibility with modern Node.js features.
73-74: Good debugging support with single worker mode.Setting
workersCountto 1 when debugging is active is a good practice as it simplifies debugging by preventing concurrent execution.lib/react_on_rails_pro/stream_request.rb (3)
82-90: Robust error handling with status code checks.Good improvement in error handling by:
- Checking status code inside the loop
- Accumulating error body for better error messages
- Preventing premature error handling
117-135: Well-implemented streaming enhancement.The
loop_response_linesmethod is a good addition that:
- Properly handles line-by-line streaming
- Ensures the last chunk is yielded
- Uses binary strings for better encoding support
92-92: Verify whitespace handling differences between RSC and non-RSC modes.The conditional whitespace handling (
@is_rsc ? chunk : chunk.strip) might affect component rendering. Please ensure this doesn't cause layout issues.✅ Verification successful
Whitespace handling difference is intentional and safe
The different whitespace handling between RSC and non-RSC modes is an intentional design choice in the streaming implementation. RSC mode preserves whitespace to maintain proper HTML streaming for React Server Components, while non-RSC mode strips whitespace for cleaner traditional React rendering output. This difference won't cause layout issues as it aligns with the respective rendering modes' requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering RSC vs non-RSC whitespace handling rg -A 5 "whitespace|strip" spec/Length of output: 14814
Script:
#!/bin/bash # Search for StreamRequest tests and RSC-related code rg -A 5 "StreamRequest|@is_rsc" spec/ rg -A 5 "RSC.*stream|stream.*RSC" spec/ # Look for documentation about RSC streaming fd README -e md | xargs rg -A 5 "RSC.*stream|stream.*RSC"Length of output: 1097
Script:
#!/bin/bash # Get content of StreamRequest spec file cat spec/react_on_rails_pro/stream_request_spec.rbLength of output: 2229
Script:
#!/bin/bash # Get content of StreamRequest implementation cat lib/react_on_rails_pro/stream_request.rbLength of output: 4479
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb (1)
29-31: Good separation of RSC bundle handling.The addition of
rsc_renderer_bundle_file_namemaintains a clean separation between RSC and non-RSC bundles.lib/react_on_rails_pro/utils.rb (1)
58-66: Well-structured RSC bundle hash implementation.The
rsc_bundle_hashmethod follows the same pattern asbundle_hash, maintaining consistency in the codebase.packages/node-renderer/src/worker/vm.ts (1)
77-82: Fix TypeScript configuration for ReadableStream.The code includes a TODO comment about fixing the TypeScript configuration. This should be addressed to ensure type safety.
Let's verify the TypeScript configuration:
Would you like me to help fix the TypeScript configuration or open a GitHub issue to track this task?
lib/react_on_rails_pro/request.rb (1)
22-27: Use keyword argument for better readabilityThe
is_rendering_rsc_payloadparameter would be more readable as a keyword argument.-def render_code_as_stream(path, js_code, is_rendering_rsc_payload) +def render_code_as_stream(path, js_code, rsc_payload: false) Rails.logger.info { "[ReactOnRailsPro] Perform rendering request as a stream #{path}" } - ReactOnRailsPro::StreamRequest.create(is_rendering_rsc_payload) do |send_bundle| + ReactOnRailsPro::StreamRequest.create(rsc_payload: rsc_payload) do |send_bundle| perform_request(path, is_rendering_rsc_payload, form: form_with_code(js_code, send_bundle, is_rendering_rsc_payload), stream: true) end end.circleci/config.yml (1)
91-95: LGTM! Cache configuration follows best practicesThe cache key includes both branch and revision, which provides good granularity for cache invalidation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb (1)
20-26: LGTM! Consider reducing code duplication.The logic for handling both server and RSC bundle hashes is correct. However, the duplicate conditional logic could be extracted into a helper method.
Consider refactoring to reduce duplication:
+ def self.should_update_hash? + @server_bundle_hash.blank? || ReactOnRails.configuration.development_mode + end def self.reset_pool_if_server_bundle_was_modified - if @server_bundle_hash.blank? || ReactOnRails.configuration.development_mode + if should_update_hash? @server_bundle_hash = ReactOnRailsPro::Utils.bundle_hash end - return unless @rsc_bundle_hash.blank? || ReactOnRails.configuration.development_mode + return unless should_update_hash? @rsc_bundle_hash = ReactOnRailsPro::Utils.rsc_bundle_hash end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb (3)
33-35: LGTM! Clear and consistent implementation.The new method follows the established pattern and maintains consistency with the existing bundle filename generation.
80-86: LGTM! Clean implementation of memoized accessors.The methods follow Ruby's idiomatic memoization pattern and provide a clean interface for accessing bundle hashes.
54-54: Verify error handling for RSC streaming.While the addition of
render_options.rsc?is correct, please ensure that error handling inrender_code_as_streamproperly handles RSC-specific failures.✅ Verification successful
Error handling for RSC streaming is properly implemented ✅
The implementation includes:
- Dedicated RSC connection handling
- Comprehensive error catching for HTTP, timeout, and connection issues
- Bundle validation for RSC-specific paths
- Proper error propagation with retries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling in render_code_as_stream implementation rg -A 10 "def render_code_as_stream" lib/react_on_rails_pro/request.rbLength of output: 663
Script:
#!/bin/bash # Search for StreamRequest implementation echo "=== StreamRequest.create and error handling ===" rg -A 10 "class StreamRequest" lib/ rg -A 10 "def create" lib/react_on_rails_pro/stream_request.rb echo -e "\n=== perform_request implementation ===" rg -A 10 "def perform_request" lib/react_on_rails_pro/request.rb echo -e "\n=== form_with_code implementation ===" rg -A 10 "def form_with_code" lib/react_on_rails_pro/request.rb echo -e "\n=== RSC error handling patterns ===" rg "rescue|error.*rsc" lib/react_on_rails_pro/Length of output: 3678
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/react_on_rails_pro/request.rb (1)
Line range hint
148-166: Fix error handling in asset upload loopThe rescue block has an undefined error variable 'e'. This could mask important error details.
- rescue e + rescue StandardError => e warn "[ReactOnRailsPro] Error uploading asset #{asset_path}: #{e}" end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 129-129: Line is too long. [148/120]
(Layout/LineLength)
[convention] 135-135: Line is too long. [123/120]
(Layout/LineLength)
🧹 Nitpick comments (6)
lib/react_on_rails_pro/configuration.rb (2)
38-38: Consider documenting the RSC-related constantsThe new RSC-related constants would benefit from documentation explaining:
- Why port 3900 was chosen for RSC
- The implications of enabling RSC support
- The purpose and usage of the RSC rendering URL path
Also applies to: 55-56
198-203: Consider extracting common password setup logicThe
setup_rsc_renderer_passwordmethod duplicates logic fromsetup_renderer_password. Consider extracting the common logic into a shared private method.Example refactor:
+ def extract_password_from_uri(url) + uri = URI(url) + uri.password + end + def setup_renderer_password return if renderer_password.present? - uri = URI(renderer_url) - self.renderer_password = uri.password + self.renderer_password = extract_password_from_uri(renderer_url) end def setup_rsc_renderer_password return if rsc_renderer_password.present? - uri = URI(rsc_renderer_url) - self.rsc_renderer_password = uri.password + self.rsc_renderer_password = extract_password_from_uri(rsc_renderer_url) endlib/react_on_rails_pro/request.rb (4)
11-17: Consider adding timeout handling for connection closureThe connection closure operations could potentially hang if the connections are unresponsive. Consider adding timeout protection.
- @connection&.close - @rsc_connection&.close + [@connection, @rsc_connection].compact.each do |conn| + Timeout.timeout(5) { conn.close } + rescue Timeout::Error + Rails.logger.warn "Connection close operation timed out" + end
134-138: Simplify renderer bundle file name selectionThe nested conditional for selecting the renderer bundle file name could be simplified using a more concise approach.
- renderer_bundle_file_name = if is_rendering_rsc_payload - ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool.rsc_renderer_bundle_file_name - else - ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool.renderer_bundle_file_name - end + bundle_type = is_rendering_rsc_payload ? :rsc_renderer : :renderer + renderer_bundle_file_name = ReactOnRailsPro::ServerRenderingPool::NodeRenderingPool.public_send("#{bundle_type}_bundle_file_name")🧰 Tools
🪛 rubocop (1.69.1)
[convention] 135-135: Line is too long. [123/120]
(Layout/LineLength)
Line range hint
191-225: Improve connection configuration managementThe connection configuration could be externalized to make it more maintainable and testable.
+ def connection_options + { + fallback_protocol: "h2", + max_concurrent_requests: ReactOnRailsPro.configuration.renderer_http_pool_size, + persistent: true, + timeout: { + connect_timeout: ReactOnRailsPro.configuration.renderer_http_pool_timeout, + read_timeout: ReactOnRailsPro.configuration.ssr_timeout + } + } + end def create_connection(url:) Rails.logger.info do "[ReactOnRailsPro] Setting up Node Renderer connection to #{url}" end HTTPX .plugin(:retries, max_retries: 1, retry_change_requests: true) .plugin(:stream) - .with( - origin: url, - fallback_protocol: "h2", - max_concurrent_requests: ReactOnRailsPro.configuration.renderer_http_pool_size, - persistent: true, - timeout: { - connect_timeout: ReactOnRailsPro.configuration.renderer_http_pool_timeout, - read_timeout: ReactOnRailsPro.configuration.ssr_timeout - } - ) + .with(**connection_options, origin: url)
Line range hint
220-229: Enhance error message structureThe error message for connection creation failures could be better structured using a more readable format.
- message = <<~MSG - [ReactOnRailsPro] Error creating HTTPX connection. - renderer_http_pool_size = #{ReactOnRailsPro.configuration.renderer_http_pool_size} - renderer_http_pool_timeout = #{ReactOnRailsPro.configuration.renderer_http_pool_timeout} - renderer_http_pool_warn_timeout = #{ReactOnRailsPro.configuration.renderer_http_pool_warn_timeout} - renderer_url = #{url} - Be sure to use a url that contains the protocol of http or https. - Original error is - #{e} - MSG + message = { + error: "[ReactOnRailsPro] Error creating HTTPX connection", + config: { + pool_size: ReactOnRailsPro.configuration.renderer_http_pool_size, + pool_timeout: ReactOnRailsPro.configuration.renderer_http_pool_timeout, + warn_timeout: ReactOnRailsPro.configuration.renderer_http_pool_warn_timeout, + url: url + }, + note: "Be sure to use a url that contains the protocol of http or https", + original_error: e.message + }.transform_keys(&:to_s).to_yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails_pro/configuration.rb(6 hunks)lib/react_on_rails_pro/request.rb(5 hunks)spec/dummy/config/initializers/react_on_rails_pro.rb(1 hunks)spec/dummy/spec/requests/upload_asset_spec.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/dummy/config/initializers/react_on_rails_pro.rb
🧰 Additional context used
🪛 rubocop (1.69.1)
lib/react_on_rails_pro/request.rb
[convention] 129-129: Line is too long. [148/120]
(Layout/LineLength)
[convention] 135-135: Line is too long. [123/120]
(Layout/LineLength)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (8)
spec/dummy/spec/requests/upload_asset_spec.rb (2)
45-45: LGTM! Improved test focus.Good change. Removing the response status check makes the test more focused on the actual outcome (assets being present on the renderer) rather than implementation details.
16-16: Consider adding test coverage for RSC-enabled configuration.The addition of
enable_rsc_support: falseis good for ensuring backward compatibility. However, since this PR adds RSC support, consider adding a new context block to test the behavior when RSC is enabled.lib/react_on_rails_pro/configuration.rb (4)
14-14: LGTM! RSC configurations properly integratedThe RSC-related configurations (
rsc_renderer_urlandrsc_renderer_password) are well-integrated into the existing configuration pattern.Also applies to: 20-20
58-65: LGTM! Well-organized attribute declarationsThe new RSC-related attributes are properly integrated and follow the existing pattern.
67-75: LGTM! Proper initialization of RSC configurationsThe RSC-related parameters are well-integrated into the initialization method, maintaining consistency with the existing pattern.
Also applies to: 77-77, 79-79, 98-99
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 67-100: Assignment Branch Condition size for initialize is too high. [<24, 24, 2> 34/28]
(Metrics/AbcSize)
152-152: LGTM! Proper URL configuration and validationThe RSC renderer URL is properly configured with defaults and validated.
Also applies to: 157-157
lib/react_on_rails_pro/request.rb (2)
60-70: Optimize RSC connection initializationThe RSC connection initialization could be more DRY by extracting the configuration check into a separate method. Also, consider lazy initialization only when actually needed to avoid unnecessary overhead.
+ def ensure_rsc_support! + return if ReactOnRailsPro.configuration.enable_rsc_support + + raise ReactOnRailsPro::Error, + "RSC support is not enabled. Please set enable_rsc_support to true in your " \ + "config/initializers/react_on_rails_pro.rb file before " \ + "rendering any RSC payload." + end def rsc_connection @rsc_connection ||= begin - unless ReactOnRailsPro.configuration.enable_rsc_support - raise ReactOnRailsPro::Error, - "RSC support is not enabled. Please set enable_rsc_support to true in your " \ - "config/initializers/react_on_rails_pro.rb file before " \ - "rendering any RSC payload." - end + ensure_rsc_support! create_connection(url: ReactOnRailsPro.configuration.rsc_renderer_url) end end
73-80: Improve parameter readability using keyword argumentsThe boolean parameter
is_rendering_rsc_payloadcould be made more readable using keyword arguments.- def perform_request(path, is_rendering_rsc_payload, **post_options) + def perform_request(path, rsc_payload: false, **post_options) available_retries = ReactOnRailsPro.configuration.renderer_request_retry_limit retry_request = true while retry_request begin start_time = Time.now - used_connection = is_rendering_rsc_payload ? rsc_connection : connection + used_connection = rsc_payload ? rsc_connection : connection🧰 Tools
🪛 rubocop (1.69.1)
[convention] 73-119: Perceived complexity for perform_request is too high. [11/10]
(Metrics/PerceivedComplexity)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.circleci/config.yml (1)
123-124: 🛠️ Refactor suggestionAvoid hardcoding branch names in CI configuration
The branch name is hardcoded, which could cause issues when switching branches or maintaining the configuration.
Apply this diff to parameterize the branch name:
-git clone -b abanoubghadban/pro488/handle-errors-happen-in-rsc-payload https://github.com/shakacode/react_on_rails.git +git clone -b ${REACT_ON_RAILS_BRANCH:-main} https://github.com/shakacode/react_on_rails.git
🧹 Nitpick comments (4)
spec/dummy/client/app/components/HelloWorldHooks.jsx (3)
8-8: Remove or replace console.log statement.The console.log statement appears to be for debugging purposes. Consider removing it or replacing it with proper logging if it's intended for production.
7-19: Consider adding PropTypes and performance optimizations.The component could benefit from:
- PropTypes validation for the
helloWorldDataprop- Memoization using React.memo() since it's a functional component
Here's how you could improve it:
import React, { useState } from 'react'; +import PropTypes from 'prop-types'; -function HelloWorldHooks(props) { +const HelloWorldHooks = React.memo(function HelloWorldHooks(props) { console.log('This is HelloWorldHooks component and this is a unique message'); const [name, setName] = useState(props.helloWorldData.name); return ( <div> <h3>Hello, {name}!</h3> <p> Say hello to: <input type="text" value={name} onChange={(e) => setName(e.target.value)} /> </p> </div> ); -} +}); + +HelloWorldHooks.propTypes = { + helloWorldData: PropTypes.shape({ + name: PropTypes.string.isRequired, + }).isRequired, +};
1-21: Well-structured client component for RSC architecture.The component correctly implements the client-side requirements for React Server Components:
- Properly marked with 'use client' directive
- Correctly handles client-side interactivity with useState
- Follows the controlled component pattern for form inputs
This aligns well with RSC architecture where interactive components need to be explicitly marked as client components.
.circleci/config.yml (1)
179-183: Consolidate duplicate cache saving stepsThe same cache saving step for react-on-rails is duplicated across multiple jobs. This could lead to maintenance issues and inconsistencies.
Consider moving the cache saving step to a shared alias and reusing it across jobs:
aliases: + - &save-react-on-rails-cache + save_cache: + name: Save react-on-rails to cache + key: v1-react-on-rails-{{ .Branch }}-{{ .Revision }} + paths: + - ~/react_on_rails jobs: install-dummy-app-node-packages: steps: # ... other steps ... - - save_cache: - name: Save react-on-rails to cache - key: v1-react-on-rails-{{ .Branch }}-{{ .Revision }} - paths: - - ~/react_on_rails + - run: *save-react-on-rails-cacheAlso applies to: 200-204, 221-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.circleci/config.yml(12 hunks)spec/dummy/client/app/components/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/AsyncComponentsTreeForTesting.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/HelloWorldHooks.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ReactHelmetApp.client.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/RouterApp.client.jsx(1 hunks)spec/dummy/package.json(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- spec/dummy/client/app/ror-auto-load-components/RouterApp.client.jsx
- spec/dummy/client/app/ror-auto-load-components/ReactHelmetApp.client.jsx
- spec/dummy/client/app/ror-auto-load-components/AsyncComponentsTreeForTesting.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/dummy/client/app/ror-auto-load-components/HelloWorldHooks.jsx
- spec/dummy/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (6)
spec/dummy/client/app/components/HelloWorldHooks.jsx (3)
1-5: LGTM! Proper RSC setup with client directive.The 'use client' directive is correctly placed at the top of the file, and imports are appropriately minimized.
21-21: LGTM! Proper export statement.The default export is correctly implemented following React conventions.
6-6: Please clarify the TODO comment.The TODO comment lacks context about what specifically needs to be aligned with HelloWorld.jsx. Consider either implementing the changes or providing more details about the required alignment.
Let's check what we're trying to align with:
✅ Verification successful
The TODO comment refers to several missing features from HelloWorld.jsx
The functional component should be aligned with HelloWorld.jsx by adding:
- PropTypes validation for
helloWorldDataand optionalrailsContext- CSS modules import and usage (brightColor class)
- RailsContext component integration
- Console.log demonstration (if needed for educational purposes)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and display the HelloWorld.jsx file content for comparison fd -t f "HelloWorld.jsx" --exclude "HelloWorldHooks.jsx" --exclude "HelloWorldRedux.jsx" -X catLength of output: 2636
.circleci/config.yml (3)
91-95: LGTM: Cache configuration for react-on-railsThe cache configuration for react-on-rails follows CircleCI's best practices with proper versioning and key structure.
350-361: LGTM: Workflow dependencies are properly configuredThe new job dependencies are correctly set up, ensuring that
clone-and-cache-react-on-railsruns before dependent jobs.
318-321: Verify RSC Node renderer port configurationA new background process for RSC Node renderer has been added, but there's no explicit port configuration or validation that the port is available.
Run this script to check for potential port conflicts:
✅ Verification successful
Port configuration is properly handled and no conflicts detected
The RSC Node renderer is configured to use port 3900 with proper fallback to 3800, and there are no conflicts with other services (Puma: 3000, Shakapacker: 3035).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the RSC Node renderer port is explicitly configured and if it might conflict with other services # Check for port configuration in package.json scripts echo "Checking package.json for RSC renderer port configuration:" cd spec/dummy && jq '.scripts["rsc-node-renderer"]' package.json # Check if the port is used by other services in the configuration echo "Checking for potential port conflicts in other service configurations:" rg -l 'port.*\d{4}' .Length of output: 497
Script:
#!/bin/bash # Check the content of files for port configurations echo "=== Node renderer port configuration ===" cat spec/dummy/client/node-renderer.js | grep -A 5 "port" echo -e "\n=== Shakapacker port configuration ===" cat spec/dummy/config/shakapacker.yml | grep -A 5 "port" echo -e "\n=== Puma port configuration ===" cat spec/dummy/config/puma.rb | grep -A 5 "port"Length of output: 2338
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/react_on_rails_pro/configuration.rb (1)
159-161:⚠️ Potential issueFix the error message in validate_url.
The error message has two issues:
- It refers to
renderer_urltwice instead ofrsc_renderer_urlfor the second URL.- There's inconsistent string concatenation using both
+and\.Apply this diff to fix the error message:
- message = "Unparseable ReactOnRailsPro.config.renderer_url #{renderer_url} or " \ - "ReactOnRailsPro.config.renderer_url #{rsc_renderer_url} provided.\n" \ - "#{e.message}" + message = "Unparseable ReactOnRailsPro.config.renderer_url #{renderer_url} or " \ + "ReactOnRailsPro.config.rsc_renderer_url #{rsc_renderer_url} provided.\n" \ + "#{e.message}"
🧹 Nitpick comments (5)
spec/react_on_rails_pro/stream_decorator_spec.rb (2)
11-18: Consider adding edge cases for empty chunks.The test suite would benefit from additional test cases:
- Empty chunks handling
- Null/nil chunks
- Very large chunks
Also applies to: 38-43
52-64: Consider using shared examples for error scenarios.The test suite could be enhanced by adding shared examples for error scenarios:
- Invalid transformations
- Exceptions during chunk processing
spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (1)
35-42: Consider extracting mock response handling logic.The
sendmethod could be simplified by extracting the mock response handling logic into smaller, more focused methods.def send(request) - request_uri = request.uri.to_s - mock = find_mock(request_uri) - validate_mock!(request_uri, request.verb, mock) - - pattern, responses = mock - handle_mock_response(request, pattern, responses) + mock = find_and_validate_mock(request) + handle_mock_response(request, *mock) end +private + +def find_and_validate_mock(request) + request_uri = request.uri.to_s + mock = find_mock(request_uri) + validate_mock!(request_uri, request.verb, mock) + mock +endspec/react_on_rails_pro/request_spec.rb (2)
25-27: Consolidate multiple stubs usingreceive_messages.Per RuboCop's suggestion, multiple stubs can be consolidated for better readability:
-allow(ReactOnRailsPro.configuration).to receive(:renderer_url).and_return(renderer_url) -allow(ReactOnRailsPro.configuration).to receive(:rsc_renderer_url).and_return(rsc_renderer_url) +allow(ReactOnRailsPro.configuration).to receive_messages( + renderer_url: renderer_url, + rsc_renderer_url: rsc_renderer_url +) -allow(ReactOnRails::Utils).to receive(:server_bundle_js_file_path).and_return(server_bundle_path) -allow(ReactOnRails::Utils).to receive(:rsc_bundle_js_file_path).and_return(rsc_server_bundle_path) +allow(ReactOnRails::Utils).to receive_messages( + server_bundle_js_file_path: server_bundle_path, + rsc_bundle_js_file_path: rsc_server_bundle_path +)Also applies to: 34-40
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 25-25: Use
receive_messagesinstead of multiple stubs on lines [26].(RSpec/ReceiveMessages)
[convention] 26-26: Use
receive_messagesinstead of multiple stubs on lines [25].(RSpec/ReceiveMessages)
17-41: Consider extracting test setup into shared contexts.The setup block is quite large and could be more maintainable if split into focused shared contexts:
- File system setup
- Configuration mocks
- HTTP client setup
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 25-25: Use
receive_messagesinstead of multiple stubs on lines [26].(RSpec/ReceiveMessages)
[convention] 26-26: Use
receive_messagesinstead of multiple stubs on lines [25].(RSpec/ReceiveMessages)
[convention] 34-35: Use
receive_messagesinstead of multiple stubs on lines [36].(RSpec/ReceiveMessages)
[convention] 36-37: Use
receive_messagesinstead of multiple stubs on lines [34].(RSpec/ReceiveMessages)
[convention] 38-38: Use
receive_messagesinstead of multiple stubs on lines [40].(RSpec/ReceiveMessages)
[convention] 40-40: Use
receive_messagesinstead of multiple stubs on lines [38].(RSpec/ReceiveMessages)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/react_on_rails_pro/configuration.rb(6 hunks)lib/react_on_rails_pro/request.rb(4 hunks)lib/react_on_rails_pro/routes.rb(1 hunks)lib/react_on_rails_pro/server_rendering_js_code.rb(2 hunks)lib/react_on_rails_pro/stream_request.rb(3 hunks)lib/react_on_rails_pro/utils.rb(3 hunks)spec/react_on_rails_pro/configuration_spec.rb(3 hunks)spec/react_on_rails_pro/request_spec.rb(1 hunks)spec/react_on_rails_pro/stream_decorator_spec.rb(1 hunks)spec/react_on_rails_pro/stream_request_spec.rb(1 hunks)spec/react_on_rails_pro/stream_spec.rb(1 hunks)spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- spec/react_on_rails_pro/stream_request_spec.rb
- lib/react_on_rails_pro/routes.rb
- lib/react_on_rails_pro/server_rendering_js_code.rb
- spec/react_on_rails_pro/stream_spec.rb
- lib/react_on_rails_pro/stream_request.rb
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/react_on_rails_pro/request_spec.rb
[convention] 25-25: Use receive_messages instead of multiple stubs on lines [26].
(RSpec/ReceiveMessages)
[convention] 26-26: Use receive_messages instead of multiple stubs on lines [25].
(RSpec/ReceiveMessages)
[convention] 34-35: Use receive_messages instead of multiple stubs on lines [36].
(RSpec/ReceiveMessages)
[convention] 36-37: Use receive_messages instead of multiple stubs on lines [34].
(RSpec/ReceiveMessages)
[convention] 38-38: Use receive_messages instead of multiple stubs on lines [40].
(RSpec/ReceiveMessages)
[convention] 40-40: Use receive_messages instead of multiple stubs on lines [38].
(RSpec/ReceiveMessages)
🔇 Additional comments (8)
spec/react_on_rails_pro/stream_decorator_spec.rb (1)
6-18: LGTM! Well-structured test organization.The test suite is well-organized with proper use of
instance_doublefor mocking theStreamRequestcomponent.spec/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (1)
120-130: 🛠️ Refactor suggestionAdd memory management for infinite mocks.
The
mock_streaming_responsemethod should include a mechanism to clean up infinite mocks to prevent memory leaks during long test runs.lib/react_on_rails_pro/utils.rb (2)
58-66: LGTM!The implementation follows the same pattern as the existing
bundle_hashmethod, ensuring consistent behavior for RSC bundle hashing.
132-142: LGTM!The function has been correctly updated to handle both RSC and non-RSC renderer passwords.
spec/react_on_rails_pro/configuration_spec.rb (1)
158-187: LGTM!The test suite comprehensively covers all scenarios for the
rsc_renderer_passwordconfiguration:
- Direct password assignment
- Password extraction from RSC URL
- Blank password when not provided
lib/react_on_rails_pro/configuration.rb (1)
38-38: LGTM!The new constants follow the naming convention and have appropriate default values:
DEFAULT_RSC_RENDERER_URLuses a different port (3900) than the default renderer (3800)DEFAULT_ENABLE_RSC_SUPPORTis disabled by defaultDEFAULT_RSC_RENDERING_URL_PATHuses a clear and descriptive pathAlso applies to: 55-56
lib/react_on_rails_pro/request.rb (2)
12-17: LGTM!The connection management is correctly implemented:
- RSC connection is created only when RSC support is enabled
- Each connection uses its own URL
62-67: LGTM!The error handling provides a clear and actionable error message when RSC support is not enabled.
| [true, false].each do |use_delay| | ||
| it "processes each chunk immediately when use_delay is #{use_delay}" do | ||
| mocked_block = mock_block | ||
|
|
||
| mock_streaming_response(render_full_url, 200) do |yielder| | ||
| sleep(0.2) if use_delay | ||
| yielder.call("First chunk\n") | ||
| expect(mocked_block).to have_received(:call).with("First chunk") | ||
|
|
||
| sleep(0.2) if use_delay | ||
| yielder.call("Second chunk\n ") | ||
| expect(mocked_block).to have_received(:call).with("Second chunk") | ||
|
|
||
| sleep(0.2) if use_delay | ||
| yielder.call("Final chunk\n") | ||
| expect(mocked_block).to have_received(:call).with("Final chunk") | ||
| end | ||
|
|
||
| stream = described_class.render_code_as_stream("/render", "console.log('Hello, world!');", false) | ||
| stream.each_chunk(&mocked_block.block) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout scenario tests.
The test suite should include scenarios for handling timeouts during streaming:
- Connection timeout
- Read timeout
- Overall request timeout
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
2 times, most recently
from
ae9daa0 to
45ef6e9
Compare
4 participants
RSC_in_RORP.mp4
Summary by CodeRabbit
New Features
RscPayloadControllerfor handling RSC payloads.Postscomponent with streaming capabilities and integrated interactivity using theToggleContainer.config.enable_rsc_supportandconfig.rsc_payload_generation_url_path.HydrationStatuscomponent to manage hydration states during rendering.Changed
copy_assetsmethod to returnnilon error, enhancing clarity in error scenarios.clientWebpackConfigto include theRSCWebpackPluginfor improved server-side rendering support.Fixed
queueMicrotaskuse in the server bundle to support React 19.