Add suppress_unused_component_warnings config option

[justin808]

Summary

• Adds suppress_unused_component_warnings configuration option to disable console warnings about registered components that are not used on a page
• This is useful in production environments where many React components are registered globally but only a subset are used on each page
• Set config.suppress_unused_component_warnings = true in your React on Rails initializer to suppress these warnings

Changes

• Added suppress_unused_component_warnings config option to Ruby configuration (default: false)
• Pass option to railsContext as suppressUnusedComponentWarnings
• Updated CallbackRegistry in Pro package to check the option before logging warnings
• Updated TypeScript types (RailsContext) and RBS signatures
• Added documentation in docs/api-reference/configuration.md
• Updated test fixtures to include the new property

Test plan

• Run bundle exec rubocop - passed
• Run bundle exec rake run_rspec:gem - 290 examples, 0 failures
• Run bundle exec rake rbs:validate - passed
• Run pnpm run type-check - passed for both react-on-rails and react-on-rails-pro packages

Closes #2117

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 22 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 16b3908 and 76a6429.

📒 Files selected for processing (10)

• CHANGELOG.md (1 hunks)
• docs/api-reference/configuration.md (1 hunks)
• packages/react-on-rails-pro/src/CallbackRegistry.ts (1 hunks)
• packages/react-on-rails/src/types/index.ts (1 hunks)
• packages/react-on-rails/tests/ClientRenderer.test.ts (4 hunks)
• react_on_rails/lib/react_on_rails/configuration.rb (4 hunks)
• react_on_rails/lib/react_on_rails/helper.rb (1 hunks)
• react_on_rails/sig/react_on_rails/configuration.rbs (2 hunks)
• react_on_rails/spec/react_on_rails/configuration_spec.rb (1 hunks)
• react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jg/suppress-unused-warnings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review for PR #2136: Add suppress_unused_component_warnings config option

✅ Overall Assessment

This is a well-implemented feature that addresses a legitimate production use case. The implementation is clean, follows project conventions, and includes proper documentation and type safety.

---

🎯 Strengths

1. Comprehensive Type Safety

   • ✅ TypeScript type added to RailsContext interface (packages/react-on-rails/src/types/index.ts:23)
   • ✅ RBS signature properly updated (react_on_rails/sig/react_on_rails/configuration.rbs:35,72)
   • ✅ All test fixtures updated to include the new property
   • ✅ Type checking passed (pnpm run type-check and bundle exec rake rbs:validate)

2. Clean Implementation

   • ✅ Configuration follows existing patterns (component_registry_timeout as a reference)
   • ✅ Default value (false) preserves backward compatibility - warnings remain enabled by default
   • ✅ Proper propagation: Ruby config → rails_context → TypeScript → Pro package
   • ✅ Sensible placement in CallbackRegistry.ts:57-65 - only suppresses when configured

3. Good Documentation

   • ✅ Clear explanation in docs/api-reference/configuration.md
   • ✅ Appropriate note that this is Pro-only functionality
   • ✅ Well-written CHANGELOG entry with proper PR reference format
   • ✅ Inline comments explain the purpose (configuration.rb:67-69)

4. Testing & Quality

   • ✅ All existing tests updated (290 examples passing)
   • ✅ RuboCop passing
   • ✅ Follows project code style and conventions

---

🔍 Observations & Minor Suggestions

1. Test Coverage Gap (Low Priority)

While existing tests were updated to include the new property, there are no dedicated tests for the new functionality itself. Consider adding:

Ruby side test:

# In react_on_rails/spec/react_on_rails/configuration_spec.rb
it 'defaults suppress_unused_component_warnings to false' do
  expect(ReactOnRails.configuration.suppress_unused_component_warnings).to be false
end

it 'allows setting suppress_unused_component_warnings' do
  ReactOnRails.configure do |config|
    config.suppress_unused_component_warnings = true
  end
  expect(ReactOnRails.configuration.suppress_unused_component_warnings).to be true
end

it 'includes suppressUnusedComponentWarnings in rails_context' do
  # Test that the helper passes this through to rails_context
end

TypeScript side test:

// In packages/react-on-rails-pro/tests/CallbackRegistry.test.ts (create if needed)
describe('CallbackRegistry unused component warnings', () => {
  it('logs warnings when suppressUnusedComponentWarnings is false', () => {
    // Mock getRailsContext to return suppressUnusedComponentWarnings: false
    // Register a component but don't use it
    // Trigger timeout
    // Expect console.warn to have been called
  });

  it('suppresses warnings when suppressUnusedComponentWarnings is true', () => {
    // Mock getRailsContext to return suppressUnusedComponentWarnings: true
    // Register a component but don't use it
    // Trigger timeout
    // Expect console.warn NOT to have been called
  });
});

Note: This is not blocking, but would improve confidence in the feature and prevent regressions.

2. Potential Null Safety Enhancement

In CallbackRegistry.ts:58, you're accessing getRailsContext()?.suppressUnusedComponentWarnings. This is safe, but the behavior when getRailsContext() returns undefined is implicit (warnings will be shown).

Consider making this explicit:

const suppressWarnings = getRailsContext()?.suppressUnusedComponentWarnings ?? false;

This makes it clear that the default is false (warnings enabled) when context is unavailable, matching the Ruby default.

3. Configuration Validation

The Ruby Configuration class has validation for component_registry_timeout (configuration.rb:217-223) but none for suppress_unused_component_warnings. While boolean validation is less critical, consider:

def check_suppress_unused_component_warnings
  # Coerce nil to false (already done in initialize)
  self.suppress_unused_component_warnings = false if suppress_unused_component_warnings.nil?
  
  # Validate it's actually a boolean (not just truthy/falsy)
  unless [true, false].include?(suppress_unused_component_warnings)
    raise ReactOnRails::Error, "suppress_unused_component_warnings must be true or false"
  end
end

This would catch cases where someone sets it to a string or other value.

---

🔒 Security & Performance

• ✅ No security concerns - this only affects logging behavior
• ✅ No performance impact - just a boolean check before logging
• ✅ No backward compatibility issues - defaults to existing behavior

---

📋 Checklist (Based on CLAUDE.md)

• ✅ RuboCop passing
• ✅ Tests passing (290 examples)
• ✅ RBS validation passing
• ✅ TypeScript type-check passing
• ✅ CHANGELOG updated (correct format with PR link)
• ✅ Documentation added
• ✅ No formatting issues (Prettier/RuboCop would catch these)
• ⚠️ Test coverage for new functionality could be improved (see above)

---

🎬 Recommendation

APPROVE with minor suggestions

This PR is ready to merge. The suggestions above are non-blocking improvements that could be addressed in a follow-up PR if desired. The core implementation is solid, well-documented, and follows all project conventions.

Great work on addressing issue #2117! This will be helpful for production environments with many registered components.

---

References:

• Closes Ability to suppress console.warns #2117
• Related to component_registry_timeout feature (introduced earlier)
• Affects Pro package only (CallbackRegistry is Pro-specific)

[claude]

Code Review - PR #2136: Add suppress_unused_component_warnings config option

✅ Overall Assessment

This is a well-implemented feature that addresses a legitimate production concern (issue #2117). The implementation is clean, follows project conventions, and the test plan confirms all critical checks passed.

---

🎯 Strengths

1. Clean Implementation: The feature is implemented with minimal changes across the stack (Ruby config → Rails context → TypeScript types → Pro package)

2. Proper Type Safety:

   • RBS signatures updated correctly in configuration.rbs
   • TypeScript types updated in RailsContext interface
   • Test fixtures updated to include the new property

3. Good Documentation: The docs/api-reference/configuration.md entry is clear and explains the use case well

4. Backwards Compatible: Defaults to false, maintaining existing behavior

5. Follows Project Conventions: Uses proper naming, follows Ruby/Rails idioms, and integrates seamlessly with existing config system

---

🔍 Issues & Concerns

❌ CRITICAL: Wrong PR Number in CHANGELOG

The CHANGELOG entry references:

[PR 2125](https://github.com/shakacode/react_on_rails/pull/2125)

But this is PR #2136. This needs to be corrected before merge.

---

⚠️ Missing: Test Coverage

The test plan shows manual verification, but there are no automated tests for this feature:

1. No unit tests in configuration_spec.rb to verify:

   • Default value is false
   • Value can be set to true
   • Value is properly passed to rails_context

2. No integration tests to verify the warning suppression actually works in CallbackRegistry

Recommendation: Add at least basic unit tests in react_on_rails/spec/react_on_rails/configuration_spec.rb:

describe "suppress_unused_component_warnings" do
  it "defaults to false" do
    expect(ReactOnRails.configuration.suppress_unused_component_warnings).to eq(false)
  end

  it "can be set to true" do
    ReactOnRails.configure do |config|
      config.suppress_unused_component_warnings = true
    end
    expect(ReactOnRails.configuration.suppress_unused_component_warnings).to eq(true)
  end
end

---

💡 Minor: Configuration Initialization

In configuration.rb:174:

self.suppress_unused_component_warnings = suppress_unused_component_warnings || false

This is slightly awkward. Since the default is defined in the configuration method (line 67), and the parameter is optional, this could be simplified to:

self.suppress_unused_component_warnings = suppress_unused_component_warnings.nil? ? false : suppress_unused_component_warnings

Or even simpler, follow the pattern used for component_registry_timeout which sets the default separately. But this is a very minor style issue and doesn't affect functionality.

---

📝 Minor: Missing Configuration Entry

Looking at configuration.rb:34-69, the default configuration hash is missing the suppress_unused_component_warnings key. While this doesn't break functionality (because of line 174), it's inconsistent with the pattern used for other config options.

Recommendation: Add to the configuration defaults around line 67:

suppress_unused_component_warnings: false,

---

🔐 Security

✅ No security concerns. This is a simple boolean flag that only affects console logging.

---

⚡ Performance

✅ No performance impact. The check happens once during timeout, not in hot paths.

---

🏗️ Architecture

✅ Follows the established pattern:

1. Ruby config option
2. Passed via rails_context
3. Read in Pro package via getRailsContext()
4. Used to conditionally log warnings

This is the correct approach for passing Rails config to the client-side.

---

📋 Action Items Before Merge

1. MUST FIX: Update CHANGELOG.md PR reference from 2125 → 2136
2. SHOULD ADD: Unit tests for the configuration option
3. NICE TO HAVE: Add default to configuration hash (line ~67)

---

🎓 Testing Notes

Per CLAUDE.md requirements, I note that you've run the following tests locally:

• ✅ RuboCop passed
• ✅ RSpec gem tests passed (290 examples)
• ✅ RBS validation passed
• ✅ TypeScript type checking passed

This meets the local testing requirements. However, automated test coverage for the new feature itself would strengthen the PR.

---

📊 Final Recommendation

APPROVE WITH CHANGES: Fix the CHANGELOG PR number (critical), then this is good to merge. Adding tests is recommended but not blocking given the simplicity of the feature and thorough manual verification.

Great work addressing a real production pain point! 🎉

[justin808]

Addressed Review Feedback

I've addressed the review comments in commit 6088576:

✅ Fixed Issues

1. CHANGELOG PR number corrected: Changed [PR 2125] → [PR 2136]

2. Added unit tests: New tests in react_on_rails/spec/react_on_rails/configuration_spec.rb:

   • Test that default value is false
   • Test that it can be set to true
   • Test that it can be set to false explicitly

ℹ️ Notes on Other Suggestions

Re: default already in configuration hash: The default is already present at lines 67-70 in configuration.rb:

# Set to true to suppress warnings about registered components not being used on a page.
# This can be useful in production environments where many components are registered globally
# but only a subset are used on each page.
suppress_unused_component_warnings: false,

Re: explicit null coalescing in TypeScript: The current code getRailsContext()?.suppressUnusedComponentWarnings already returns undefined when context is unavailable, and the if (!suppressWarnings) check treats both false and undefined/null as "warnings enabled" which is the correct behavior. Adding ?? false would be slightly more explicit but functionally identical.

---

Monitoring CI for the updated commit.

[claude]

Code Review for PR 2136

Summary

This PR adds a suppress_unused_component_warnings configuration option to allow users to disable console warnings about registered components that aren't used on a page. Overall, the implementation is solid and well-tested, but there's ONE CRITICAL BUG that needs to be fixed.

---

CRITICAL ISSUE - Missing Configuration Default

Location: react_on_rails/lib/react_on_rails/configuration.rb:69

Problem: The new suppress_unused_component_warnings option is NOT included in the default configuration hash passed to Configuration.new. Looking at the code, component_registry_timeout is there, but suppress_unused_component_warnings is missing from the hash on line 34-70.

Impact: When users call ReactOnRails.configuration without explicitly configuring this option, it will be nil instead of false. While line 177 has a fallback (|| false), this creates inconsistency and could cause unexpected behavior if the configuration object is inspected or serialized before initialization completes.

Fix Required: Add suppress_unused_component_warnings: false to the default configuration hash in ReactOnRails.configuration (around line 67, after component_registry_timeout)

---

Positive Aspects

1. Complete Type Coverage: TypeScript types, RBS signatures, and test fixtures all updated correctly
2. Good Documentation: Clear explanation in docs/api-reference/configuration.md with appropriate note about Pro-only feature
3. Proper Implementation: The TypeScript check in CallbackRegistry.ts:58 correctly uses optional chaining
4. Consistent Naming: Ruby uses snake_case, JS uses camelCase
5. Changelog Entry: Properly formatted with PR reference and note about Pro-only feature

---

Minor Observations

Attr Accessor Placement (Non-blocking): The suppress_unused_component_warnings attr_accessor is separated from component_registry_timeout by other unrelated attributes. Consider placing it immediately after component_registry_timeout for better logical grouping since they're related to the component registry feature.

Test Coverage (Non-blocking): The PR updates test fixtures but doesn't add explicit tests for the new configuration option. Consider adding tests verifying: (1) default value is false, (2) option can be set to true, (3) it's properly passed to railsContext. However, given the simplicity of the feature and comprehensive manual testing noted in the PR description, this is optional.

Optional Chaining Safety: The code uses getRailsContext()?.suppressUnusedComponentWarnings which is good defensive programming. This will safely default to undefined (falsy) if the context isn't available.

---

CI Failure Analysis

I see there's one CI failure in rspec-dummy-app-node-renderer. This appears to be unrelated to your changes since: (1) Your changes are purely configuration-related, (2) No changes to node renderer code, (3) Other integration tests pass successfully.

However, per the repo's CLAUDE.md guidelines, you should verify this isn't a pre-existing failure by checking if master is passing for this workflow.

---

Recommendations

Must Fix:

• Add suppress_unused_component_warnings: false to the default configuration hash in ReactOnRails.configuration

Should Consider:

• Group the attr_accessor with component_registry_timeout for better code organization
• Verify the CI failure is pre-existing (check master branch status)

Nice to Have:

• Add explicit unit tests for the new configuration option

---

The implementation is well-designed and thoroughly documented, but needs ONE CRITICAL FIX before merging. Once the default configuration value is added, this will be ready to ship. Great work on maintaining consistency across Ruby, TypeScript, and RBS type definitions!

[claude]

Code Review - PR #2136

This is a well-implemented feature that adds the ability to suppress unused component warnings. Here's my detailed feedback:

✅ Strengths

1. Clean Implementation: The feature is implemented consistently across the stack:

   • Ruby configuration (with proper defaults and validation)
   • RailsContext propagation to client
   • TypeScript type definitions
   • RBS signatures for type checking
   • Pro package CallbackRegistry checks the flag before logging

2. Excellent Documentation:

   • Clear API reference documentation added to docs/api-reference/configuration.md
   • CHANGELOG entry properly explains the feature and notes it's Pro-only
   • Inline comments explain the purpose

3. Proper Testing:

   • Configuration tests cover default value and both true/false settings
   • Test fixtures updated to include the new property
   • Author ran the full test suite (290 examples passed)

4. Backwards Compatible:

   • Defaults to false (warnings enabled), maintaining existing behavior
   • No breaking changes for users

5. Type Safety:

   • RBS signatures updated and validated
   • TypeScript types properly extended
   • All type checks pass

🔍 Minor Observations

1. Configuration Initialization (line 177 in configuration.rb):

   self.suppress_unused_component_warnings = suppress_unused_component_warnings || false

   This handles nil correctly, but consider whether explicit false values should be preserved. The current implementation will coerce nil to false, which is correct.

2. Pro-Only Feature Documentation:
   The CHANGELOG and documentation clearly note this is a Pro-only feature since the warnings come from CallbackRegistry in the Pro package. Well done!

3. No Test Coverage for Pro Package:
   While the Ruby configuration is well-tested, there's no test verifying the actual suppression behavior in CallbackRegistry.ts. Consider adding a test like:

   it('suppresses warnings when suppressUnusedComponentWarnings is true', () => {
     // Mock getRailsContext to return suppressUnusedComponentWarnings: true
     // Register a component
     // Trigger timeout
     // Verify console.warn was not called
   });

📋 Checklist Review

Based on CLAUDE.md requirements:

• ✅ RuboCop: Author confirms it passed
• ✅ RSpec tests: 290 examples, 0 failures
• ✅ RBS validation: Passed
• ✅ Type checking: Passed for both packages
• ✅ CHANGELOG: Updated with proper PR link format
• ✅ Configuration: Safe defaults, proper validation
• ✅ Documentation: Clear and comprehensive

🎯 Recommendation

APPROVE - This PR is production-ready and follows all repository conventions. The implementation is clean, well-tested, and properly documented.

The only suggestion is adding a test for the CallbackRegistry behavior, but this is optional since the integration is straightforward and the configuration is well-tested.

Great work addressing issue #2117! 🚀

[claude]

Code Review - PR #2136: Add suppress_unused_component_warnings config option

Summary

This PR adds a new configuration option to suppress console warnings about unused registered components. The implementation is clean and follows React on Rails conventions well.

---

Strengths

1. Complete Implementation: All layers properly updated - Ruby configuration, RBS type signatures, TypeScript types, documentation, and comprehensive tests

2. Good Type Safety: RBS signatures match the Ruby implementation, TypeScript RailsContext type updated correctly, test fixtures updated

3. Backward Compatible: Defaults to false preserving existing behavior

4. Well Documented: Clear documentation with helpful context about when to use this option

5. Proper Testing: Good test coverage with three test cases covering default, true, and false values

---

Issues and Suggestions

1. Missing Configuration Attribute (CRITICAL)

The new suppress_unused_component_warnings attribute is NOT added to the attr_accessor list in configuration.rb:74-86. This means the getter/setter methods are not defined and the configuration won't work at runtime.

Fix needed in react_on_rails/lib/react_on_rails/configuration.rb line 85

2. Inconsistent Default Handling

The pattern in configuration.rb:177 uses || false which will incorrectly treat explicit false as nil. Consider using the ternary pattern used elsewhere or relying on the default value.

3. Pro-Only Feature Clarity

The changelog notes this is Pro-only but the configuration is in the open-source gem. Consider adding a comment in configuration.rb noting this only affects Pro users.

4. Potential Race Condition (Minor)

In CallbackRegistry.ts:58, the getRailsContext() call happens during the timeout callback. Consider caching the config value at initialization for more predictable behavior.

---

Security

No security concerns identified.

---

Performance

Negligible impact. The check happens during timeout callback only.

---

Test Coverage

Good coverage of the configuration behavior. Consider adding an integration test that verifies warnings are actually suppressed.

---

Action Items

Must Fix Before Merge:

• Add suppress_unused_component_warnings to attr_accessor list in configuration.rb:85

Should Fix:

• Review default value handling to avoid || false pattern

Nice to Have:

• Cache suppressUnusedComponentWarnings value at timeout initialization
• Add integration test verifying warnings are actually suppressed

---

Verdict

Good implementation overall with proper documentation and testing. CRITICAL ISSUE: Missing attr_accessor will cause runtime failures. Please fix before merging.

Review by Claude Code

[claude]

Code Review for PR #2136

Overall Assessment

This is a well-implemented feature that addresses a legitimate use case (issue #2117). The code quality is high, and the implementation follows React on Rails conventions. However, there is one critical issue that needs to be addressed before merging.

---

Critical Issue

Missing Default Value in Configuration Hash

Location: react_on_rails/lib/react_on_rails/configuration.rb:33-70

The suppress_unused_component_warnings option is not included in the default configuration hash passed to Configuration.new. Looking at the current code, the hash includes component_registry_timeout but is missing the new option.

Impact: Without this, the configuration option will be nil until the initialize method applies the || false fallback. While this works, it is fragile and inconsistent with other configuration options.

Fix Required: Add suppress_unused_component_warnings: false to the default configuration hash, between component_registry_timeout and generated_component_packs_loading_strategy.

---

Code Quality Observations

Excellent Documentation

The documentation in docs/api-reference/configuration.md is clear and helpful:

• Explains the purpose
• Shows example usage
• Clarifies it is Pro-only
• Describes when to use it

Comprehensive Test Coverage

The RSpec tests in configuration_spec.rb properly test:

• Default value (false)
• Setting to true
• Setting to false explicitly

TypeScript test fixtures updated to include the new property in ClientRenderer.test.ts

Type Safety Maintained

• RBS signatures properly updated in react_on_rails/sig/react_on_rails/configuration.rbs
• TypeScript types updated in packages/react-on-rails/src/types/index.ts

---

Implementation Review

CallbackRegistry Implementation (packages/react-on-rails-pro/src/CallbackRegistry.ts)

The implementation correctly wraps the warning forEach loop in a conditional check. The optional chaining on getRailsContext() provides safe fallback behavior - if the context is missing, suppressWarnings will be undefined, which is falsy, so warnings will be shown by default. This is correct!

Configuration Initialization Pattern

The line self.suppress_unused_component_warnings = suppress_unused_component_warnings || false provides correct default behavior but is fragile because it relies on the parameter being nil rather than having an explicit default in the configuration hash. This is why adding it to the default hash is important.

Runtime Behavior

The code safely handles missing configuration via optional chaining. If getRailsContext() returns null/undefined, warnings will be shown (correct default behavior).

---

Testing Checklist Review

According to the PR description:

• bundle exec rubocop - passed
• bundle exec rake run_rspec:gem - 290 examples, 0 failures
• bundle exec rake rbs:validate - passed
• pnpm run type-check - passed

Question: Was the actual behavior manually tested in a browser?

• Warnings shown when suppress_unused_component_warnings = false (default)?
• Warnings suppressed when suppress_unused_component_warnings = true?

It would be helpful to confirm manual browser testing occurred.

---

Strengths

• Clean, focused implementation
• Excellent documentation
• Comprehensive type coverage (RBS + TypeScript)
• Follows existing code patterns consistently
• Safe defaults (warnings shown by default)
• Optional chaining prevents null reference errors
• Proper attr_accessor declaration

---

Security & Performance

Security: No concerns identified - boolean config value, no user input, client-side only

Performance: Minimal impact - single boolean check before logging, only runs during timeout callback

---

Final Verdict

Status: Needs Changes (one critical fix required)

Required Before Merge:

1. Add suppress_unused_component_warnings: false to the default configuration hash in react_on_rails/lib/react_on_rails/configuration.rb lines 33-70

Optional:

2. Confirm manual testing in browser

Once the default configuration value is added, this PR will be ready to merge. The implementation is solid and follows all project conventions.

Great work overall!

---

References:

• Related Issue: Ability to suppress console.warns #2117
• CLAUDE.md guidelines: Followed
• All automated tests: Passing

[justin808]

CI Fix Pushed

Pushed commit 76a6429 to fix the failing Pro integration test.

Issue: The test fixture in react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb was checking for an exact match of the rails_context JSON structure, which didn't include the new suppressUnusedComponentWarnings field.

Fix: Added suppressUnusedComponentWarnings:false to the test fixture to match the updated rails_context structure.

---

Note on other CI failure: The "JS unit tests for Renderer package" workflow is a pre-existing failure on master (failing since Nov 23rd), not related to this PR.

[claude]

Code Review: Add suppress_unused_component_warnings config option

✅ Overall Assessment

This is a well-implemented, clean addition that solves a real problem in production environments. The code quality is high, testing is comprehensive, and documentation is clear.

---

🎯 Strengths

1. Clean Implementation

• The feature follows the existing configuration pattern perfectly
• Default value (false) maintains backward compatibility
• Proper propagation from Ruby config → rails_context → TypeScript

2. Excellent Type Safety

• ✅ TypeScript types updated in RailsContext
• ✅ RBS signatures updated with proper boolean type
• ✅ All test fixtures updated consistently

3. Smart Design Choice

The check getRailsContext()?.suppressUnusedComponentWarnings in CallbackRegistry.ts:58 is well-placed:

• Only executes when timeout triggers
• Uses optional chaining for safety
• Minimal performance impact

4. Comprehensive Testing

• ✅ Configuration spec covers default, true, and false cases
• ✅ Test fixtures updated in 4 places
• ✅ All linting and type checking passed

5. Good Documentation

• Clear explanation in docs/api-reference/configuration.md
• Appropriate note that this only affects Pro users
• CHANGELOG entry is user-focused and clear

---

💡 Minor Suggestions (Optional Improvements)

1. Consider Adding Integration Test

While unit tests are solid, consider adding an integration test that verifies:

• When config is true, warnings are NOT logged
• When config is false (default), warnings ARE logged

Example location: react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb

2. CHANGELOG Placement Consideration

The feature is correctly placed in the main CHANGELOG.md since it affects the open-source package's configuration. However, you might also consider mentioning it in CHANGELOG_PRO.md since:

• The actual warning suppression only works in Pro
• Pro users are the primary beneficiaries

Per CLAUDE.md guidelines:

Changes affecting both → Update BOTH changelogs

This is a judgment call—current placement is valid, but dual-changelog entry would be more discoverable for Pro users.

---

🔍 Technical Details Verified

Ruby Configuration (configuration.rb:177)

self.suppress_unused_component_warnings = suppress_unused_component_warnings || false

✅ Correct use of || false to handle nil case

Rails Context Propagation (helper.rb:277)

suppressUnusedComponentWarnings: ReactOnRails.configuration.suppress_unused_component_warnings,

✅ Properly camelCased for JavaScript consumption

TypeScript Integration (CallbackRegistry.ts:58)

const suppressWarnings = getRailsContext()?.suppressUnusedComponentWarnings;
if (\!suppressWarnings) {
  // log warnings
}

✅ Safe optional chaining
✅ Correct boolean logic (only logs when NOT suppressed)

Test Consistency

All 4 test fixtures updated consistently:

• ClientRenderer.test.ts (4 instances)
• react_on_rails_pro_helper_spec.rb (1 instance)

---

🎨 Code Quality Notes

Follows Project Conventions ✅

• Matches existing config option patterns (component_registry_timeout, etc.)
• RBS signature follows established format
• Documentation style consistent with other options
• CHANGELOG format matches existing entries

Performance Considerations ✅

• No performance impact when disabled (default)
• Check only runs during timeout event (rare)
• getRailsContext() called once per timeout, not per component

Security Considerations ✅

• Read-only configuration—no injection risks
• Boolean value—no complex parsing needed
• No external input processing

---

📋 Checklist Review

Based on PR description, all testing requirements met:

• ✅ bundle exec rubocop - passed
• ✅ bundle exec rake run_rspec:gem - 290 examples, 0 failures
• ✅ bundle exec rake rbs:validate - passed
• ✅ pnpm run type-check - passed for both packages

---

🚀 Recommendation

APPROVED - This PR is ready to merge as-is. The optional suggestions above are minor enhancements that could be addressed in follow-up work if desired.

Great work! This is a clean, professional implementation that will improve the production experience for users with large component registries.

Closes #2117 appropriately.

[claude]

Code Review: Add suppress_unused_component_warnings config option

Overall, this is a well-implemented feature with good test coverage and documentation. The implementation is clean, follows project conventions, and addresses a real need for production environments. Below are my observations:

---

✅ Strengths

1. Comprehensive Implementation

   • Configuration properly added across all layers: Ruby config, Rails helper, TypeScript types, and RBS signatures
   • Good default value (false) - warnings are enabled by default for development/debugging
   • Proper validation and type checking in place

2. Documentation

   • Clear documentation in docs/api-reference/configuration.md
   • Helpful CHANGELOG entry with clear explanation of the feature
   • Good inline comments explaining the purpose

3. Test Coverage

   • RSpec tests cover all cases (default, explicit true, explicit false)
   • TypeScript fixtures updated consistently
   • All linting and type checking passes

4. Pro-Only Feature Properly Scoped

   • The warning logic is in CallbackRegistry.ts (Pro package)
   • Documentation correctly notes this only affects Pro users
   • CHANGELOG entry clarifies Pro-only scope

---

🔍 Observations & Potential Issues

1. Missing from self.configuration defaults (Minor Bug)

Location: react_on_rails/lib/react_on_rails/configuration.rb:64-70

The self.configuration method creates a default Configuration object but doesn't include suppress_unused_component_warnings in the hash:

def self.configuration
  @configuration ||= Configuration.new(
    # ... other defaults ...
    component_registry_timeout: DEFAULT_COMPONENT_REGISTRY_TIMEOUT,
    # suppress_unused_component_warnings is missing here\!
    generated_component_packs_loading_strategy: nil,

While this works because the initializer defaults it to false at line 174, best practice would be to include it in the default hash for consistency with other config options like component_registry_timeout.

Suggested fix:

component_registry_timeout: DEFAULT_COMPONENT_REGISTRY_TIMEOUT,
suppress_unused_component_warnings: false,  # Add this line
generated_component_packs_loading_strategy: nil,

---

2. TypeScript Type Definition Placement

Location: packages/react-on-rails/src/types/index.ts:22-23

The suppressUnusedComponentWarnings property is added to the RailsContext type:

export type RailsContext = {
  componentRegistryTimeout: number;
  suppressUnusedComponentWarnings: boolean;  // Added here
  railsEnv: string;
  // ...

This is correct, but there's a comment at line 20 that says:

// Keep these in sync with method lib/react_on_rails/helper.rb#rails_context

Verification needed: Ensure the property is actually added to the rails_context method in helper.rb.

Looking at the diff, I can see it is added at line 277 in helper.rb:

suppressUnusedComponentWarnings: ReactOnRails.configuration.suppress_unused_component_warnings,

✅ This is correct and properly synchronized.

---

3. CallbackRegistry Implementation - Potential Timing Issue

Location: packages/react-on-rails-pro/src/CallbackRegistry.ts:56-60

The implementation reads getRailsContext()?.suppressUnusedComponentWarnings inside the triggerTimeout callback:

const triggerTimeout = () => {
  // ...
  const suppressWarnings = getRailsContext()?.suppressUnusedComponentWarnings;
  if (\!suppressWarnings) {
    this.notUsedItems.forEach((itemName) => {
      console.warn(/* ... */);
    });
  }
};

Question: Is getRailsContext() guaranteed to be available when triggerTimeout is called?

Looking at the broader code, triggerTimeout is only called after onPageLoaded() (line 63), which should ensure the Rails context is set. ✅ This appears safe.

---

4. attr_accessor Missing in Configuration Class (Critical Bug)

Location: react_on_rails/lib/react_on_rails/configuration.rb:85

Looking at the diff, I see the attr_accessor was updated to include suppress_unused_component_warnings:

:component_registry_timeout, :suppress_unused_component_warnings,

✅ This is correct. Without this, the getter/setter wouldn't work.

---

5. RBS Signature Consistency

Location: react_on_rails/sig/react_on_rails/configuration.rbs

The RBS signature adds:

attr_accessor suppress_unused_component_warnings: bool

And in the initializer parameters:

?suppress_unused_component_warnings: bool?,

✅ Correct. The initializer parameter is optional (?) and can be nil (bool?), which matches the implementation at line 174:

self.suppress_unused_component_warnings = suppress_unused_component_warnings || false

---

🛡️ Security Considerations

No security concerns. This is a client-side logging configuration that doesn't affect authentication, authorization, or data handling.

---

⚡ Performance Considerations

Minimal impact:

• The check adds one property read from RailsContext during the timeout callback
• The timeout only fires once per page load (if at all)
• No performance concerns

---

🧪 Test Coverage Assessment

Ruby tests (configuration_spec.rb):

• ✅ Tests default value
• ✅ Tests explicit true
• ✅ Tests explicit false

TypeScript tests:

• ✅ Test fixtures updated to include the new property

Missing tests:

• ⚠️ Integration test for the actual warning suppression: There's no test in CallbackRegistry.test.ts (if it exists) that verifies warnings are actually suppressed when the flag is true. This would be a good addition to ensure the feature works end-to-end.

---

📝 Suggested Improvements

1. Add default to self.configuration (see observation TODO for first version #1)
2. Consider adding an integration test for the Pro package that verifies:
   • When suppressUnusedComponentWarnings: false, warnings appear
   • When suppressUnusedComponentWarnings: true, warnings are suppressed

---

✅ Final Verdict

Recommendation: Approve with minor suggestions

The implementation is solid and follows all project conventions. The only notable issue is the missing default value in self.configuration (observation #1), which is a minor consistency issue that doesn't affect functionality. The missing integration test is a nice-to-have but not blocking.

Before merging:

• Consider adding the default value to self.configuration for consistency
• Optionally add integration test for warning suppression behavior

Great work! 🎉

[claude]

Code Review Feedback

✅ Overall Assessment

This PR successfully implements the suppress_unused_component_warnings configuration option. The implementation is clean, well-documented, and follows the project's conventions. All critical areas are covered.

---

📋 Code Quality & Best Practices

Strengths:

• ✅ Consistent naming convention: Uses camelCase in TypeScript (suppressUnusedComponentWarnings) and snake_case in Ruby (suppress_unused_component_warnings)
• ✅ Proper default value: Defaults to false, maintaining backward compatibility
• ✅ Good documentation: Updated docs/api-reference/configuration.md with clear usage examples
• ✅ CHANGELOG updated: Properly formatted entry with PR link and author credit
• ✅ Type safety: Updated TypeScript types (RailsContext) and RBS signatures
• ✅ Test coverage: Added comprehensive tests in configuration_spec.rb covering default, true, and false cases
• ✅ Test fixtures updated: All test fixtures correctly include the new property

Minor Observation:

• The implementation is minimal and focused, which is good. The warning suppression is implemented exactly where it needs to be in CallbackRegistry.ts:56-62.

---

🐛 Potential Issues

No critical issues found. However, one observation:

Configuration Initialization (line 174 in configuration.rb):

self.suppress_unused_component_warnings = suppress_unused_component_warnings || false

This line uses || false which means:

• If suppress_unused_component_warnings is nil, it becomes false ✅
• If suppress_unused_component_warnings is false, it stays false ✅

This is correct, but could be simplified for clarity:

self.suppress_unused_component_warnings = suppress_unused_component_warnings.nil? ? false : suppress_unused_component_warnings

Or even simpler since false || false is false:

self.suppress_unused_component_warnings = \!\!suppress_unused_component_warnings

However, the current implementation is consistent with how other boolean config options are handled in the same file (e.g., see line 158 for random_dom_id), so it's fine to keep as-is for consistency.

---

🚀 Performance Considerations

No performance concerns. The implementation:

• Only checks the config value once during timeout trigger (CallbackRegistry.ts:58)
• Uses getRailsContext() which is already memoized
• The conditional check adds negligible overhead

---

🔒 Security Concerns

No security issues. This is a client-side display configuration that:

• Only affects console warning output
• Doesn't expose sensitive information
• Doesn't change application behavior beyond suppressing warnings

---

🧪 Test Coverage

Excellent test coverage:

✅ Ruby configuration tests (configuration_spec.rb):

• Tests default value (false)
• Tests setting to true
• Tests setting to false explicitly

✅ Integration tests (react_on_rails_pro_helper_spec.rb):

• Updated fixture to include suppressUnusedComponentWarnings: false in rails context JSON

✅ TypeScript tests (ClientRenderer.test.ts):

• Updated all test fixtures to include suppressUnusedComponentWarnings: false

Missing (but optional) test coverage:

• Unit tests for CallbackRegistry verifying the suppression logic works correctly
• However, this is Pro package code and may have separate test coverage

Recommendation: Consider adding a test that verifies when suppressUnusedComponentWarnings: true, the warning is NOT logged. This could be done in a Pro package test suite if it doesn't exist already.

---

📚 Documentation

Excellent documentation:

• ✅ Clear explanation in docs/api-reference/configuration.md
• ✅ Includes usage example
• ✅ Explains when to use it (production environments with many globally registered components)
• ✅ Notes that it only affects Pro users (important!)
• ✅ CHANGELOG entry is clear and well-formatted

---

✨ Additional Suggestions

1. RBS Validation: The PR description mentions running bundle exec rake rbs:validate ✅, but consider also running bundle exec rake rbs:steep to ensure type checking passes.

2. Integration Testing: While unit tests are comprehensive, consider manually testing in a Pro setup to verify:

   • Warnings appear when config is false (default)
   • Warnings are suppressed when config is true
   • The configuration is correctly passed through the entire stack

3. Error Handling: The implementation uses optional chaining (getRailsContext()?.suppressUnusedComponentWarnings), which gracefully handles missing config. This is excellent defensive programming. ✅

---

🎯 Conclusion

This PR is ready to merge after:

1. ✅ Verifying all CI checks pass
2. ✅ Confirming manual testing was done (author's test plan looks comprehensive)

The implementation is solid, follows project conventions, and solves the stated problem (issue #2117) effectively. Great work! 🚀

---

📝 Nitpicks (Optional)

None! The code is clean and follows the project's style guide.

🤖 Review conducted by Claude Code

[justin808]

Response to Code Review

Thank you for the thorough review! Here's my response to the observations:

✅ Observation #1: Default in self.configuration - Already Addressed

The default value is already present in self.configuration at lines 67-70:

component_registry_timeout: DEFAULT_COMPONENT_REGISTRY_TIMEOUT,
# Set to true to suppress warnings about registered components not being used on a page.
# This can be useful in production environments where many components are registered globally
# but only a subset are used on each page.
suppress_unused_component_warnings: false,
generated_component_packs_loading_strategy: nil,

The diff may have truncated this section, but it was included in the initial implementation.

✅ Observations #2-5 - All Verified

• TypeScript types properly synchronized with helper.rb
• getRailsContext() is safely accessed after onPageLoaded()
• attr_accessor properly includes the new config option
• RBS signatures are consistent with the implementation

ℹ️ Re: Integration Test for Warning Suppression

Adding an integration test for the actual warning suppression behavior would require mocking the timeout and console.warn in the Pro package tests. While this is a good suggestion, the current implementation is straightforward enough that the unit tests provide sufficient coverage. The behavior is:

1. Check suppressUnusedComponentWarnings from getRailsContext()
2. If falsy, log warnings; if truthy, skip warnings

If desired, I can add such a test in a follow-up PR to avoid scope creep.

---

Summary: All items in the review are either already addressed or are nice-to-haves for future improvements. The PR is ready for merge.