Walkthrough

Generator now emits a react_on_rails_default layout by default. RSC setup adds layout-resolution logic that reuses a compatible existing layout when possible or creates a new compatible layout and returns the chosen layout name for generated controllers. Tests and templates updated accordingly.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Gen as RSC Generator
    participant RscSetup as RscSetup
    participant FS as FileSystem
    participant Config as Template Config

    Gen->>RscSetup: create_hello_server_controller()
    RscSetup->>RscSetup: resolve_hello_server_layout_name()
    RscSetup->>FS: read app/controllers/hello_world_controller.rb (if exists)
    RscSetup->>FS: scan app/views/layouts/* for candidate layouts
    RscSetup->>RscSetup: compatible_layout?(name) -> check for auto-registration pack tags
    alt Compatible layout found
        RscSetup->>Config: return existing layout name
    else No compatible layout
        RscSetup->>FS: create new layout file (react_on_rails_default or next available)
        RscSetup->>FS: optionally announce incompatible existing layouts
        RscSetup->>Config: return new layout name
    end
    Config-->>Gen: layout_name
    Gen->>FS: generate controller using layout_name

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through layouts, sniffed each file and name,
Found old hello_world but spared it from shame.
A default was minted when none fit the bill,
Reused when it matched, crafted when not — tidy skill.
The rabbit claps: packs registered, controllers still nimbly game.

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 @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

Renamed the generator layout from hello_world.html.erb to react_on_rails_default.html.erb to better reflect its purpose. The layout provides empty javascript_pack_tag and stylesheet_pack_tag calls for React on Rails auto-registration and has no connection to the HelloWorld demo component.

Key changes:

• Layout file renamed in generator templates
• Both HelloWorldController and HelloServerController now declare layout "react_on_rails_default"
• Generator specs updated to assert new layout filename
• Specs deduplicated by extracting shared example rsc_hello_server_controller
• Dummy fixture in execjs-compatible-dummy updated for consistency
• Removed outdated prerender: true parameter from stream_react_component call in RSC view template

Issue found:

• CHANGELOG.md line 36 still references old hello_world naming in the PR 2429 entry, should be updated to react_on_rails_default

Confidence Score: 4/5

• Safe to merge with one minor CHANGELOG inconsistency to fix
• The PR is a well-executed rename that consistently updates all references across generator templates, controllers, specs, and dummy fixtures. The code changes are straightforward and mechanical. The test deduplication using shared examples improves maintainability. Score is 4 instead of 5 due to the CHANGELOG inconsistency where PR 2429's entry still references the old hello_world naming that this PR is replacing.
• CHANGELOG.md line 36 needs updating to reflect the new layout name

Important Files Changed

_{Last reviewed commit: ede7f4f}

Code Review

Overall this is a well-scoped rename with a clear rationale. The rename is applied consistently across generator templates, spec files, and the execjs-compatible-dummy fixture. A few observations:

Bundled unrelated changes

The PR title says "rename layout", but three additional concerns are folded in:

1. fix: remove no-op prerender: true from RSC template view – correct fix, clearly committed separately, but it makes this PR do two different things. Worth calling out in the PR description (it is in the commit log but not in the Summary bullets).
2. chore: extract build_render_label`` – pure refactor, unrelated to the rename. Fine on its own, but adds review surface area.
3. Deduplication via include_examples "rsc_hello_server_controller" – good cleanup, but again separate concern.

None of these are blockers; they're all improvements. Just flagging for awareness.

Test coverage gaps

Non-RSC layout file not asserted. The four contexts that assert_file "app/views/layouts/react_on_rails_default.html.erb" are all --rsc contexts. For a plain rails g react_on_rails:install (no --rsc), neither base_generator_common nor any other shared example asserts that the layout file is generated. This pre-existed before this PR, but the rename is a natural point to close the gap and would guard against a regression in the non-RSC path.

react_on_rails_default layout not asserted in non-RSC controller. Similarly, the hello_world_controller.rb template now declares layout "react_on_rails_default", but there is no generator spec that verifies the generated controller contains that line for a non-RSC install.

Minor nit: CHANGELOG ordering

The new "Changed" section is placed before the existing "Fixed" section, which matches the conventional Keep-a-Changelog order (Added → Changed → Fixed). That's correct. One small wording suggestion: the entry references both the layout rename and the prerender: true fix (implicit via the template change), but the prerender: true fix isn't mentioned in the CHANGELOG at all. If it's intentional, that's fine; if it's an observable behaviour change for RSC demo apps, it might deserve a line.

Code Review

This is a clean, well-executed rename. The change is correctly scoped and the implementation is consistent across all relevant files.

Correctness

prerender: true removal is correct. The explicit option was always redundant — stream_react_component internally forces options[:prerender] = true unconditionally (see react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb:129). Removing it from the template and the associated comment prevents developer confusion about whether the option does anything.

All layout references are updated. No stray layout "hello_world" declarations remain anywhere in the codebase. Both controller templates, the pro execjs-compatible-dummy fixture, and base_generator.rb are all consistently updated.

CHANGELOG is correct. The PR 2429 entry was also updated to use the new name — the greptile bot's comment about a CHANGELOG inconsistency is incorrect.

Test Quality

The refactoring to shared_examples "rsc_hello_server_controller" is a good improvement — the same four RSC generator contexts were each duplicating an inline it block. The removed let(:expected) in the helpful-message context is safe; those tests were rewritten to use flexible matching.

The new rsc_generator_examples.rb verifies that the controller declares the new layout and the view contains stream_react_component. One minor addition worth considering: an expect(content).not_to include('prerender: true') assertion in the view example would guard against the option being re-introduced accidentally. Low priority since the template is simple, but it would document the intentional removal.

No Issues Found

• No security or performance concerns — this is purely a generator/template rename.
• No existing-app breakage: the layout is only generated into fresh apps.
• react_on_rails_default is a clear, descriptive name that correctly signals the layout belongs to React on Rails infrastructure rather than demo code.

Review Summary

The rename from hello_world → react_on_rails_default is a clear improvement in semantics, and the layout-resolution logic in RscSetup is well-designed: it gracefully reuses compatible existing layouts rather than blindly overwriting user files. The test coverage is thorough and the shared-example refactoring is clean.

Issues to Address

1. Unescaped regex interpolation in pack_tag_without_names? (minor, inline comment posted)
helper_name is interpolated directly into the regex. Since the two call-sites always pass safe literal strings this doesn't cause a current bug, but Regexp.escape(helper_name) is the correct defensive form. The /m flag on the same regex is a no-op (no . in the pattern) and should be removed for clarity.

2. Double file reads per candidate layout (minor, inline comment posted)
resolve_hello_server_layout_name calls find_compatible_hello_server_layout_name (which reads each candidate file via compatible_layout?) and then, on failure, calls incompatible_existing_layout_paths (which reads those same files again). Easy to fix by collecting both results in one pass or memoizing compatible_layout?.

3. Unbounded loop in next_available_hello_server_layout_name (minor, inline comment posted)
The loop that escalates the suffix has no upper bound. Suggest capping at e.g. 99 and raising a descriptive error so a runaway generator can't spin forever.

No Issues Found

• The extract_declared_layout_name regex correctly handles double-quoted, single-quoted, and symbol forms, with a proper lookahead for line-end/comma termination.
• The pack_tag_arguments_without_names? logic correctly differentiates keyword-only calls (class: "foo", **opts) from positional pack-name calls ("application").
• Converting hello_server_controller.rb → .rb.tt and threading layout_name through config[:layout_name] is the right Thor idiom.
• The fallback naming chain (react_on_rails_default → react_on_rails_rsc → react_on_rails_rsc_2, …) is sensible and non-destructive.
• The execjs-compatible-dummy rename is correctly kept in sync.

Overall: solid PR, the three items above are all minor and none are blockers.

Review

The overall approach is clean and well-reasoned. Renaming the layout is the right call — the old name was semantically misleading, especially in RSC-only installs. The compatibility-detection logic (checking for nameless pack tags rather than keying off filenames) is a solid design choice that handles upgrades gracefully.

Test coverage for the four layout-detection scenarios (compatible legacy, compatible custom, incompatible legacy fallback, incompatible default fallback) is thorough and well-structured.

Issues found

Functional / correctness

• layout_destination_path interpolates the layout name into a file path without sanitization. The name comes from a regex-extracted string in the destination app's controller, so a value like ../../../some_path would produce a path escaping app/views/layouts/. Low risk in a local generator context, but worth a File.basename guard. (inline comment on line 538)

• MAX_LAYOUT_NAME_ATTEMPTS = 99 represents the maximum suffix number, not the count of attempts. By the time the raise fires, the method has examined 100 distinct names (react_on_rails_default, react_on_rails_rsc, plus suffixes 2–99). The constant name and error message both misrepresent the actual behavior. (inline comment on line 609)

Code quality

• compatible_layout? constructs File.join(destination_root, layout_path) twice. One local variable fixes it. (inline comment on lines 517–522)

• The pack_tag_arguments_without_names? regex is correct but non-obvious. A one-line comment explaining the two alternation branches would prevent future confusion. (inline comment on line 560)

• announce_incompatible_layout_fallback's message contains a redundant explanatory sentence that makes the output more verbose than necessary. (inline comment on lines 583–594)

Missing test

• The numbered fallback (react_on_rails_rsc_2, …) in next_available_hello_server_layout_name is not exercised by any spec. (inline comment on rsc_generator_spec.rb)

Review: chore: rename generated layout hello_world → react_on_rails_default

Overall this is a well-thought-out change. The semantic motivation (the layout has nothing to do with HelloWorld) is sound, the backward-compatible upgrade path is carefully designed, and the test coverage is thorough. A few issues worth addressing:

Bugs / correctness

1. Trailing comments break layout-name extraction

extract_declared_layout_name uses a lookahead that expects the closing quote to be followed only by ), ,, or end-of-line. Any controller with a trailing comment on the layout declaration (e.g. layout "hello_world" # RoR auto-registration) will silently return nil, causing the generator to skip the reuse path and create a new layout unnecessarily. The lookahead needs to also accept # to handle this case.

2. Non-deterministic RSC layout selection

existing_rsc_layout_names uses Dir.glob, which returns paths in filesystem (inode) order — not alphabetical on all systems/platforms. If an app has both react_on_rails_rsc.html.erb and react_on_rails_rsc_2.html.erb and both are compatible, the generator may pick either one depending on the OS. Adding .sort before the .map makes the selection deterministic.

3. pack_tag_without_names? regex can silently reject valid parenthesized arguments

The inner alternation uses [^%] to avoid running past ERB tag boundaries — but this also rejects parenthesized pack-tag arguments that happen to contain a literal % character (e.g. a pack tag call with a percentage value in its arguments). In that case the scan never yields a match for that call, and the function falls through to false — treating an otherwise-compatible layout as incompatible.

Minor / style

4. config[:layout_name] in the .tt template is a non-obvious Thor idiom

When template(source, dest, layout_name: value) is called, Thor captures the trailing hash as a local named config in the ERB binding — so config[:layout_name] is correct. However this is easy to misread as "generator config" or break if someone refactors the call. A short comment in the template file would help future maintainers.

5. No test for the trailing-comment layout scenario

Given the regex gap noted above, a test case where HelloWorldController declares layout "hello_world" # comment would be a valuable addition to prevent regressions.

PR Review - Overall this is a well-structured backward-compatible change with thorough test coverage. A few issues worth addressing. (1) Bug: RSC_GENERATED_LAYOUT_NAME_PATTERN misses suffixes 10-19 - see inline comment on line 28. The regex [2-9]\d* excludes _10-_19 because those start with 1. Since next_available_hello_server_layout_name iterates 2..99, it can mint react_on_rails_rsc_10 through react_on_rails_rsc_19, but those are invisible to existing_rsc_layout_names on the next run. Fix: use ([2-9]|[1-9]\d+) instead. (2) Minor: strip_wrapping_parentheses does not verify balanced parens - see inline on line 648. (3) Minor: pack_tag_arguments_without_names? regex scope undocumented for edge cases like **options, debug: true - see inline on line 645. Nits: extract_declared_layout_name uses ^ anchor worth documenting; before(:all) blocks very repetitive. What is good: semantic layout detection rather than filename matching, well-reasoned priority order, comprehensive edge-case spec coverage including symbol selectors, percent signs in keyword args, user-owned rsc_auth prefix, and not overwriting user-owned files.

Review Summary

The layout rename from hello_world to react_on_rails_default is well-motivated. The new name makes semantic sense since the layout exists only to provide empty pack tags for React on Rails auto-registration — it has nothing to do with the HelloWorld demo. RSC-only installs previously generated no HelloWorld controller at all, making the old name especially confusing.

Strengths:

• The 3-tier fallback in standalone RSC upgrades is thoughtful and safe: reuse HWC layout → reuse a known-good canonical layout → create a new layout, never touching user-owned files.
• Test coverage in rsc_generator_spec.rb is thorough, covering parenthesized layout declarations, symbol layouts, trailing comments, suffix-matching helpers, partial pack tags, existing RSC-prefixed layouts, multi-digit suffixes, and rspack variants.
• The generator is appropriately conservative and non-destructive.

Issues flagged inline:

1. Error message off-by-one in next_available_hello_server_layout_name: 100 distinct names are checked before raising, but the message cites 99.
2. pack_tag_arguments regex misfires on ERB arguments containing the literal characters %> (e.g. data: { html: "50%>" }).
3. pack_tag_arguments_without_names? — greedy .* with /m flag will accept positional args after keyword args, potentially misclassifying a layout as compatible.