Fix default layout handling for render component

[justin808]

Summary

• restore the implicit current-layout behavior for render component: when no explicit layout: option is passed
• preserve explicit layout overrides such as layout: false
• add regression coverage for the render-option contract and controller-level layout behavior
• update the changelog for issue Rails layout not applied when using render :component #1356

Closes #1356.

Testing

• mise exec ruby@3.2.9 -- env BUNDLE_PATH=/Users/justin/codex/react-rails/repo/vendor/bundle bundle exec ruby -Itest test/react/rails/railtie_test.rb
• mise exec ruby@3.2.9 -- env BUNDLE_PATH=/Users/justin/codex/react-rails/repo/vendor/bundle bundle exec ruby -Itest test/react/rails/component_renderer_test.rb
• mise exec ruby@3.2.9 -- env BUNDLE_GEMFILE=/Users/justin/codex/react-rails/issue-1356/LintingGemfile BUNDLE_PATH=/Users/justin/codex/react-rails/issue-1356/vendor/bundle bundle exec rubocop lib/react/rails/railtie.rb test/react/rails/railtie_test.rb test/react/rails/component_renderer_test.rb

[claude]

PR Review: Fix default layout handling for render component

This is a well-scoped fix for #1356. The logic is correct and the test coverage is solid. A few observations below.

---

Core logic (lib/react/rails/railtie.rb)

Concern: send used to call a private method in production code

render_options = Railtie.send(:component_render_options, options, html)

Using .send to invoke a private_class_method from outside the class is a code smell — it signals the method is internal while simultaneously bypassing that signal. Since component_render_options is called from a block that runs in controller context (not inside Railtie itself), it effectively has to be reachable from outside.

Two cleaner alternatives:

1. Remove it from private_class_method — it's part of the renderer contract now, not a pure implementation detail.
2. Inline the logic — it's only three lines, which avoids the false privacy signal entirely:

   ActionController::Renderers.add :component do |component_name, options|
     renderer = ::React::Rails::ControllerRenderer.new(controller: self)
     html = renderer.call(component_name, options)
     render_options = options.merge(inline: html)
     render_options = render_options.merge(layout: true) unless render_options.key?(:layout)
     render(render_options)
   end

The current approach does work and has the benefit of making the method unit-testable in railtie_test.rb, which is a reasonable trade-off. But it's worth a deliberate decision either way.

---

Tests

Minor gap: no test for a named layout override

The railtie_test covers layout: false, but not layout: "some_layout". Since the intent is to preserve any explicit layout, one more case would be cheap to add and would make the contract explicit:

test "component render options preserve an explicit named layout" do
  render_options = React::Rails::Railtie.send(:component_render_options, { layout: "admin" }, "<div/>")
  assert_equal "admin", render_options[:layout]
end

FakeRenderer constant placement

Defining FakeRenderer inside ComponentRendererTest is fine, but because Ruby hoists inner class constants, it becomes ComponentRendererTest::FakeRenderer. If another test file later defines a similarly named constant it could conflict. A top-level name like FakeComponentRenderer or wrapping it in a module would be more explicit. Not a blocker.

---

What's good

• The fix is correct: layout: true is exactly how Rails applies the controller's default layout, and key?(:layout) correctly distinguishes an explicitly-passed nil/false/name from no option at all.
• ControllerRenderer#call slices recognized keys (options.slice(:data, :aria, :tag, ...)) so the :layout option never bleeds into the component render — no accidental side-effects there.
• The two-level test approach (unit tests in railtie_test for the extracted method + controller integration in component_renderer_test) is the right pattern for this kind of change.
• The changelog entry and issue reference are clear.

---

Overall this is a clean, low-risk fix. The main point worth discussing before merge is whether to keep the private_class_method + send pattern or inline/expose the method.

[claude]

Code Review

Overall, this is a clean, well-scoped fix for the Rails 8 layout regression. The approach is correct and the test coverage is solid. A few notes below.

---

lib/react/rails/railtie.rb

The core fix is correct. Using render_options.key?(:layout) to distinguish between "no layout specified" and "layout explicitly set to false/nil" is the right approach — a truthiness check would incorrectly treat layout: false as absent.

Minor: component_render_options is not in private_class_method

The existing helpers versioned_assets_for and versioned_assets? are private class methods. component_render_options is currently public, which is intentional for direct unit testing (consistent with how append_react_build_to_assets_version! is tested). This is fine, but worth a deliberate decision since it becomes part of the public API surface.

Edge case: layout: nil

If a caller passes layout: nil, key?(:layout) returns true and the method returns { layout: nil } rather than forcing layout: true. In Rails, layout: nil and layout: true both resolve to the controller layout, so this is functionally equivalent — but it's a subtle distinction worth noting in a comment or doc if this is intentional.

---

test/react/rails/railtie_test.rb

Nitpick: loose assertions on layout value

# current
assert render_options[:layout]          # passes for any truthy value
refute render_options[:layout]          # passes for nil too, not just false

Prefer explicit equality checks to pin down the exact returned values:

assert_equal true,  render_options[:layout]   # default case
assert_equal false, render_options[:layout]   # explicit layout: false

This makes the contract of component_render_options explicit and catches regressions if the return value changes type.

---

test/react/rails/component_renderer_test.rb

FakeRenderer constant placement

Defining a named constant inside the test class (ComponentRendererTest::FakeRenderer) is fine for namespacing, but a Struct with a do...end block at the class level is a little unusual. Consider a plain inner class or an anonymous struct assigned to a local for slightly cleaner read. Not a blocker.

Missing integration test: named layout

The unit tests cover layout: "admin", but the integration test only covers the default and layout: false cases. Adding a test where the controller action renders with layout: "admin" would round out the coverage and give confidence the full render path works end-to-end for named overrides.

RuboCop suppression

The # rubocop:disable Minitest/MultipleAssertions comments are reasonable here — the assertions are tightly related and splitting them into separate tests would hurt readability. Alternatively, the project could configure a higher threshold for this cop in .rubocop.yml for test files.

---

CHANGELOG.md

Other entries include a PR link and author attribution (e.g., [PR 1345](https://github.com/reactjs/react-rails/pull/1345) by [elektronaut](...)). For consistency:

- Preserve default controller layouts for `render component:` after the Rails 8 render pipeline change.
  [PR #1418](https://github.com/reactjs/react-rails/pull/1418) by [justin808](https://github.com/justin808).
  Fixes [#1356](https://github.com/reactjs/react-rails/issues/1356).

---

Summary

Area	Status
Bug fix correctness	✅ Correct — key? guard handles layout: false properly
Unit test coverage	✅ Good — all three option combinations tested
Integration test coverage	✅ Good — could add named layout case
Assertion precision	⚠️ Minor — prefer assert_equal over truthiness checks
Changelog format	⚠️ Minor — add PR link + author attribution
Security / performance	✅ No concerns

The fix is sound and ready to merge once the minor items above are addressed (or consciously left as-is).

[justin808]

@G-Rath please take a look. :-)

[claude]

Code Review

Overall: Clean, targeted fix with good test coverage. Approachable for merge with one minor note.

---

What the PR Does

Rails 8 changed the render pipeline so that the inline: key in render options now suppresses layout application before layouts are resolved. This PR restores the pre-Rails 8 behavior by explicitly passing layout: true when no layout option is provided, while correctly preserving any explicit layout: override (including layout: false).

---

Code Quality

lib/react/rails/railtie.rb

The extraction into component_render_options is clean and well-reasoned:

def self.component_render_options(options, html)
  render_options = options.merge(inline: html)
  return render_options if render_options.key?(:layout)
  render_options.merge(layout: true)
end

• The early-return on .key?(:layout) correctly handles all explicit cases: layout: false, layout: "name", and even layout: nil (preserving the caller's intent in all cases).
• The logic is easy to follow and the method has a single responsibility.

Minor: Visibility inconsistency. The other two helper class methods (versioned_assets_for, versioned_assets?) are declared private_class_method. component_render_options is left public, presumably to allow direct unit testing in railtie_test.rb. This is a pragmatic choice, but it slightly diverges from the class's established pattern. Consider either:

1. Adding component_render_options to the private_class_method call and testing it via .send, or
2. Documenting with :nodoc: (like append_react_build_to_assets_version!) to signal it's not part of the public API.

Neither is a blocker — just worth a conscious decision.

---

Test Coverage

test/react/rails/railtie_test.rb — Three focused unit tests for component_render_options cover the three distinct cases (default, false, named string). Well done.

test/react/rails/component_renderer_test.rb — Integration-level controller tests using a FakeRenderer stub are a solid addition. They verify the full request/response cycle, not just the options hash.

One small observation: FakeRenderer stores both html and calls, but the tests only assert on calls.dig(0, 0) (the component name). The html assertion is covered indirectly via assert_match on the response body, which is fine. The # rubocop:disable Minitest/MultipleAssertions comments are expected here and acceptable for controller-level tests.

---

Potential Edge Case

layout: nil — if a caller explicitly passes layout: nil, .key?(:layout) returns true, so the nil is preserved rather than replaced with true. In Rails, render inline: ..., layout: nil may behave differently from render inline: ..., layout: true. This is likely an uncommon case and the current behavior (preserve the caller's explicit nil) is defensible, but it may be worth a brief comment or test if you anticipate it.

---

Security / Performance

No concerns.

---

CHANGELOG

Entry is accurate and well-placed.

---

Summary: The fix is correct and minimal. The test suite is thorough. Addressing the private_class_method inconsistency (or consciously leaving it public) would round this out nicely.

[claude]

Code Review

Summary: Correct, minimal fix for the Rails 8 layout regression. Good to merge with the one minor note below.

---

Correctness

The fix is sound. render_options.key?(:layout) correctly distinguishes all explicit cases:

• layout: false → preserved (no wrapping layout)
• layout: "name" → preserved (named layout)
• layout: nil → preserved (Rails treats nil the same as the default layout, so functionally fine)
• No :layout key → adds layout: true, restoring the controller's default layout

The layout: true value is the correct Rails idiom for "use the controller's configured layout."

---

One note: visibility inconsistency

versioned_assets_for and versioned_assets? are declared private_class_method, but component_render_options is left public (just # :nodoc:). The current choice is reasonable — it makes the method directly unit-testable without .send — but it's a divergence from the class's established pattern. Either approach is defensible; just worth a conscious decision before merge.

---

Tests

Both test layers are solid:

• railtie_test.rb: three focused unit tests cover all three option branches (default, false, named string) — good use of assert_same for boolean identity checks.
• component_renderer_test.rb: controller integration tests using a FakeRenderer stub verify the full request/response cycle for all three cases.

Note: An earlier review comment incorrectly stated there was no test for the named layout override. Both railtie_test.rb and component_renderer_test.rb already cover that case.

---

CHANGELOG

Format and placement are consistent with the rest of the file. ✅

---

Overall: The change is correct, well-tested, and low-risk. Ready to merge once the private_class_method visibility question is resolved (or consciously left as-is).