Fix RSC install generator layout wiring and post-install helper hint#2429
Fix RSC install generator layout wiring and post-install helper hint#2429
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe generator now always copies Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryFixes RSC install generator to prevent Key changes:
Impact: Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[rails g react_on_rails:install --rsc] --> B{RSC Flag?}
B -->|Yes| C[base_generator: Copy hello_world.html.erb]
B -->|No| D[base_generator: Copy hello_world.html.erb]
C --> E[Generate HelloServerController template]
E --> F[Set layout hello_world]
F --> G[install_generator: Call helpful_message_after_installation]
G --> H[Pass rsc: true]
H --> I[generator_messages: build_render_example]
I --> J[Return stream_react_component helper]
D --> K[Generate HelloWorldController]
K --> L[Uses default application layout]
L --> M[install_generator: Call helpful_message_after_installation]
M --> N[Pass rsc: false]
N --> O[generator_messages: build_render_example]
O --> P[Return react_component helper]
F --> Q[Fresh Rails app runs without MissingEntryError]
L --> R[Uses javascript_pack_tag application]
Last reviewed commit: 1722d8a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@react_on_rails/lib/generators/react_on_rails/generator_messages.rb`:
- Around line 92-95: The generator example incorrectly shows passing prerender:
true for stream_react_component; update build_render_example to omit the
prerender param when helper_name is "stream_react_component" (i.e., when rsc is
true) by returning "<%= #{helper_name}(\"#{component_name}\", props:
@#{route}_props) %>" for that case; change the logic in the build_render_example
method (referenced by its name and the helper_name/rsc variables) so it doesn't
include "prerender: true" for stream_react_component.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 32: Update the CHANGELOG entry to correct the root cause: state that
HelloServerController lacked an explicit layout and fell back to Rails'
application.html.erb (which contained javascript_pack_tag "application" not
produced by the RSC flow), and that the fix was to declare layout "hello_world"
on HelloServerController and ensure hello_world.html.erb is always copied by the
generator; also mention the layout now references the correct pack
(hello-server-bundle) and that post-install messaging was improved to mention
react_on_rails_helper for RSC setups.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justin808
force-pushed
the
codex/fix-rsc-generator-layout-and-message
branch
from
c297787 to
95306eb
Compare
PR Review: Fix RSC install generator layout wiring and post-install helper hintThe root cause fix is correct and well-targeted. The Notable Issues1. Test coverage gap for The 2. Redundant branching in def build_render_example(component_name:, route:, rsc:)
helper_name = rsc ? "stream_react_component" : "react_component"
if rsc
"<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props) %>"
else
"<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props, prerender: true) %>"
end
endThe Minor Observations
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| private | ||
|
|
||
| def build_render_example(component_name:, route:, rsc:) |
There was a problem hiding this comment.
helper_name is set by branching on rsc, then immediately the same if rsc branch decides which string to produce — helper_name is used exactly once in each branch. The intermediate variable adds indirection without benefit. Each branch can embed the helper name directly:
| def build_render_example(component_name:, route:, rsc:) | |
| def build_render_example(component_name:, route:, rsc:) | |
| if rsc | |
| "<%= stream_react_component(\"#{component_name}\", props: @#{route}_props) %>" | |
| else | |
| "<%= react_component(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" | |
| end | |
| end |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1)
806-810: Missingapp/views/layouts/hello_world.html.erbassertion in--rsc --redux"copies common files" test.
base_generator.rbnow adds the layout unconditionally for every install flow, so the--rsc --reduxcombo should assert its presence too. The--rsccontext already covers it (line 699), but RSC+Redux is a distinct path that also exercises the same code path — and is the scenario most likely to regress.🧪 Suggested test coverage addition
it "copies common files" do %w[config/initializers/react_on_rails.rb Procfile.dev Procfile.dev-static-assets - Procfile.dev-prod-assets].each { |file| assert_file(file) } + Procfile.dev-prod-assets + app/views/layouts/hello_world.html.erb].each { |file| assert_file(file) } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb` around lines 806 - 810, The "copies common files" example in react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb for the --rsc --redux path is missing an assertion for the layout added by base_generator.rb; update the test example (the it "copies common files" block) to include an assert_file("app/views/layouts/hello_world.html.erb") alongside the existing %w[...] assertions so the RSC+Redux install flow verifies the layout is copied.react_on_rails/lib/generators/react_on_rails/generator_messages.rb (1)
92-99: Previousprerender: trueconcern is correctly resolved —stream_react_componentpath omits it.The
if rscbranch correctly dropsprerender: trueforstream_react_component. Thehelper_nameintermediate variable is slightly redundant since theif rsccondition already drives both the helper name and the argument list; consider collapsing:♻️ Optional simplification of `build_render_example`
def build_render_example(component_name:, route:, rsc:) - helper_name = rsc ? "stream_react_component" : "react_component" if rsc - "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props) %>" + "<%= stream_react_component(\"#{component_name}\", props: @#{route}_props) %>" else - "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" + "<%= react_component(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/generators/react_on_rails/generator_messages.rb` around lines 92 - 99, The build_render_example method currently sets helper_name then uses an if rsc branch to return two different render strings; simplify by removing the intermediate helper_name variable and directly return the appropriate string based on rsc so the logic is clearer: in build_render_example use the rsc flag to choose between returning "<%= stream_react_component(..., props: @#{route}_props) %>" and "<%= react_component(..., props: @#{route}_props, prerender: true) %>" (referencing component_name and route), eliminating helper_name while preserving existing output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/generators/react_on_rails/generator_messages.rb`:
- Around line 92-99: The build_render_example method currently sets helper_name
then uses an if rsc branch to return two different render strings; simplify by
removing the intermediate helper_name variable and directly return the
appropriate string based on rsc so the logic is clearer: in build_render_example
use the rsc flag to choose between returning "<%= stream_react_component(...,
props: @#{route}_props) %>" and "<%= react_component(..., props:
@#{route}_props, prerender: true) %>" (referencing component_name and route),
eliminating helper_name while preserving existing output.
In `@react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb`:
- Around line 806-810: The "copies common files" example in
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb for the
--rsc --redux path is missing an assertion for the layout added by
base_generator.rb; update the test example (the it "copies common files" block)
to include an assert_file("app/views/layouts/hello_world.html.erb") alongside
the existing %w[...] assertions so the RSC+Redux install flow verifies the
layout is copied.
There was a problem hiding this comment.
Fix itself looks right. Before this merges though:
1. hello_world.html.erb naming debt
The fix is correct and low-risk. The layout file needs to exist and the controller needs to declare it explicitly. But in a --rsc-only install there is no HelloWorld controller, component, or route anywhere. A developer opening their generated app will find app/views/layouts/hello_world.html.erb with no HelloWorld anything, and HelloServerController declaring layout "hello_world". The name does not reflect what the file actually does (inject pack tags via the empty javascript_pack_tag / stylesheet_pack_tag pattern). Worth a follow-up to introduce a neutral layout name so the layout is decoupled from the HelloWorld demo.
2. prerender: true in the generated view template is a no-op
react_on_rails/lib/generators/react_on_rails/templates/rsc/base/app/views/hello_server/index.html.erb#L19 (not changed in this PR) generates:
<%= stream_react_component('HelloServer', props: @hello_server_props, prerender: true) %>But stream_react_component always forces options[:prerender] = true internally (see react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb#L127-L129) and its own comment says it "doesn't have the prerender option." This PR correctly drops prerender: true from the post-install hint but leaves it in the generated view file, so a developer reading the template is still taught a no-op option. The comment above it on line 13 even says "prerender: true enables server-side rendering with streaming," which is misleading since the flag does nothing. Worth cleaning up the template in a follow-up.
| def build_render_example(component_name:, route:, rsc:) | ||
| helper_name = rsc ? "stream_react_component" : "react_component" | ||
| if rsc | ||
| "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props) %>" | ||
| else | ||
| "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" | ||
| end | ||
| end |
There was a problem hiding this comment.
helper_name is derived from rsc on this line, then if rsc is evaluated again in the very next line. The variable is used exactly once per branch and adds no clarity. Each branch can embed the name directly:
| def build_render_example(component_name:, route:, rsc:) | |
| helper_name = rsc ? "stream_react_component" : "react_component" | |
| if rsc | |
| "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props) %>" | |
| else | |
| "<%= #{helper_name}(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" | |
| end | |
| end | |
| def build_render_example(component_name:, route:, rsc:) | |
| if rsc | |
| "<%= stream_react_component(\"#{component_name}\", props: @#{route}_props) %>" | |
| else | |
| "<%= react_component(\"#{component_name}\", props: @#{route}_props, prerender: true) %>" | |
| end | |
| end |
| Procfile.dev-static-assets | ||
| Procfile.dev-prod-assets].each { |file| assert_file(file) } | ||
| Procfile.dev-prod-assets | ||
| app/views/layouts/hello_world.html.erb].each { |file| assert_file(file) } |
There was a problem hiding this comment.
The --rsc context now asserts hello_world.html.erb, but the --rsc --redux context and --rsc --typescript context have the same "copies common files" block without this assertion. The fix in base_generator.rb is unconditional so the file is always copied for all install types. Both contexts should also verify it:
%w[config/initializers/react_on_rails.rb
Procfile.dev
Procfile.dev-static-assets
Procfile.dev-prod-assets
app/views/layouts/hello_world.html.erb].each { |file| assert_file(file) }| it "creates HelloServer controller and view" do | ||
| assert_file "app/controllers/hello_server_controller.rb" do |content| | ||
| expect(content).to include("class HelloServerController") | ||
| expect(content).to include('layout "hello_world"') |
There was a problem hiding this comment.
This assertion was added for --rsc, but --rsc --redux also creates HelloServerController and has no assertion on its content. If the layout "hello_world" declaration were missing or regressed in the Redux combination, no test would catch it. Suggest adding to the --rsc --redux context:
it "creates HelloServer controller with hello_world layout" do
assert_file "app/controllers/hello_server_controller.rb" do |content|
expect(content).to include("class HelloServerController")
expect(content).to include('layout "hello_world"')
expect(content).to include("ReactOnRailsPro::Stream")
end
endThere was a problem hiding this comment.
Fine, although I'm not sure we need the support for RSC and Redux, which is adding a bunch more stuff for us to test. I'm not sure it makes sense in the context of RSC.
There was a problem hiding this comment.
I did consider blocking this combination, but kept it for the upgrade path: existing apps with Redux that want to add RSC on top. For new installs it doesn't make much sense, agreed.
The intermediate variable added no clarity — each branch already uses a distinct helper name inline. Inline the strings directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The --rsc --redux, --rsc --typescript, and --rsc --rspack contexts were missing assertions that app/views/layouts/hello_world.html.erb is generated. Add it to each context's "copies common files" test to match coverage in the plain --rsc context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…texts The --rsc --redux, --rsc --typescript, and --rsc --rspack contexts were missing assertions that HelloServerController is generated with layout "hello_world". Add a matching test block to each context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ihabadham
left a comment
ihabadham
left a comment
There was a problem hiding this comment.
All inline comments addressed: helper_name refactored away, and test coverage extended to --rsc --redux, --rsc --typescript, and --rsc --rspack contexts for both the layout file copy and HelloServerController layout declaration. Follow-up items (layout naming debt, prerender no-op in generated template) tracked for a separate PR.
Review SummaryThe fix correctly addresses the A few observations: Test isolation in
|
| .to contain_exactly(described_class.format_info("Test info message")) | ||
| end | ||
|
|
||
| it "shows stream_react_component in RSC install message" do |
There was a problem hiding this comment.
The spec file has no before { described_class.clear } between examples. GeneratorMessages stores messages in a module-level @output array, so the existing tests that call add_error/add_warning/add_info accumulate state. If those tests run before others, any contain_exactly assertion will see unexpected extra messages.
The new tests here are safe (they call helpful_message_after_installation, which returns a string without touching @output), but the file overall is fragile. Consider adding a before { described_class.clear } at the top of the describe block.
There was a problem hiding this comment.
No action needed — generator_spec_helper.rb has a global config.after { GeneratorMessages.clear } that clears state after every example.
| "• Streaming server rendering in app/views/#{route}/index.html.erb:" | ||
| else | ||
| "• Server-side rendering - Enabled with prerender option in app/views/#{route}/index.html.erb:" | ||
| end |
There was a problem hiding this comment.
render_label is computed inline here while render_example is extracted to the private build_render_example method. For consistency, consider extracting this to a build_render_label private method alongside build_render_example.
There was a problem hiding this comment.
Pre-existing, not introduced in this PR. Agreed it's worth cleaning up — will track as a follow-up.
| end | ||
| end | ||
|
|
||
| it "creates HelloServer controller with hello_world layout" do |
There was a problem hiding this comment.
This test body is identical across --rsc, --rsc --redux, --rsc --typescript, and --rsc --rspack contexts. A shared example group would eliminate the duplication:
shared_examples "hello_server_controller_uses_hello_world_layout" do
it "creates HelloServer controller with hello_world layout" do
assert_file "app/controllers/hello_server_controller.rb" do |content|
expect(content).to include("class HelloServerController")
expect(content).to include('layout "hello_world"')
expect(content).to include("ReactOnRailsPro::Stream")
end
end
endThen each context can use include_examples "hello_server_controller_uses_hello_world_layout".
There was a problem hiding this comment.
The --rsc context test at line 774 is different (also asserts the view file), so there are 3 identical blocks, not 4. Agreed on the style point — will track as a follow-up.
HelloServerController had no explicit layout declaration, causing Rails to fall back to application.html.erb, which references javascript_pack_tag "application" — a pack that does not exist in the RSC webpack flow. This triggered a Shakapacker::Manifest::MissingEntryError on the first request. Changes: - Always copy app/views/layouts/hello_world.html.erb during install so RSC apps have a pack-driven layout available (empty javascript_pack_tag / stylesheet_pack_tag, filled by React on Rails auto-registration) - Set generated HelloServerController to layout "hello_world" so fresh Rails apps never fall back to application.html.erb - Update post-install messaging to show stream_react_component(...) for RSC installs instead of react_component(...) with prerender: true - Extend generator specs to assert layout file copy and HelloServerController layout declaration across --rsc, --rsc --redux, --rsc --typescript, and --rsc --rspack contexts - Refactor build_render_example to inline helper names directly Verified manually with a fresh Rails app: GET /hello_server returns 200 with layouts/hello_world.html.erb and RSC-specific packs injected correctly. Closes #2429
2 participants
Summary
app/views/layouts/hello_world.html.erbduring install so--rscapps have a pack-driven layout availableHelloServerControllertolayout "hello_world"so fresh Rails apps do not depend onjavascript_pack_tag "application"stream_react_component(...)when--rscis usedWhy
Generating a fresh Rails app with
rails g react_on_rails:install --rsc --typescriptproduced a runtimeShakapacker::Manifest::MissingEntryErrorbecauseapplicationlayout referencesjavascript_pack_tag "application"while noapplicationpack is generated in this flow.Verification
mise exec ruby@3.3 -- bundle exec rspec react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb react_on_rails/spec/react_on_rails/generators/generator_messages_spec.rbmise exec ruby@3.3 -- bundle exec rubocop react_on_rails/lib/generators/react_on_rails/base_generator.rb react_on_rails/lib/generators/react_on_rails/templates/rsc/base/app/controllers/hello_server_controller.rb react_on_rails/lib/generators/react_on_rails/generator_messages.rb react_on_rails/lib/generators/react_on_rails/install_generator.rb react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb react_on_rails/spec/react_on_rails/generators/generator_messages_spec.rbSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Note
Low Risk
Generator/template and messaging changes with limited runtime impact beyond newly generated apps; risk is mainly incorrect layout selection affecting fresh RSC installs.
Overview
Fixes a fresh
rails g react_on_rails:install --rscruntime failure whereHelloServerControllercould fall back to Rails’ defaultapplication.html.erb(referencing a non-existentapplicationpack), causingShakapacker::Manifest::MissingEntryError.The generator now always copies
app/views/layouts/hello_world.html.erb, sets the RSCHelloServerControllertemplate tolayout "hello_world", and updates the post-install message to showstream_react_component(...)for RSC (with new specs covering the updated messaging and RSC file generation).Written by Cursor Bugbot for commit 95306eb. Configure here.