Skip to content

Conversation

@AbanoubGhadban
Copy link
Collaborator

@AbanoubGhadban AbanoubGhadban commented Sep 15, 2025

Summary

image

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.


This change is Reviewable

Summary by CodeRabbit

  • New Features
    • Added a “React On Rails Pro Required” persistent badge overlay when force-loading features without an active Pro license; shown for server-rendered components, component-hash responses, and Redux stores, linking to Pro details and logging a warning.
  • Tests
    • Expanded tests to cover badge visibility and warning logs across Pro vs non‑Pro and force_load scenarios (per-component, hash, store, and global).
  • Chores
    • Improved Pro-license detection with caching to stabilize badge behavior.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 15, 2025

Walkthrough

Adds Pro-license detection and a conditional "React On Rails Pro Required" badge; the helper now injects the badge when force_load is requested but the Pro licence is not valid. Introduces Utils.react_on_rails_pro_licence_valid? and adds tests covering badge rendering and warning logs.

Changes

Cohort / File(s) Summary
Helper: Pro badge + flow integration
lib/react_on_rails/helper.rb
Added pro_warning_badge_if_needed(force_load) to render a top-right Pro-required badge when force_load is true and Pro licence is not valid. Prepends badge to server-rendered HTML in react_component (String), to COMPONENT_HTML_KEY in react_component_hash (Hash), and to store HTML in redux_store when rendering immediately. Logs and prints a warning when badge is shown. Removed a nearby private marker, making subsequent methods public.
Utils: Pro licence detection
lib/react_on_rails/utils.rb
Added ReactOnRails::Utils.react_on_rails_pro_licence_valid? with caching. Returns false if Pro gem missing; delegates to ReactOnRailsPro::Utils.licence_valid? when available, otherwise treats licence as valid.
Specs: Pro gating, force_load, and logging
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
Stubs ReactOnRails::Utils.react_on_rails_pro_licence_valid? and adds contexts to verify badge presence/absence and Rails.logger warnings under various force_load scenarios (per-component, per-hash, per-store, and global). Updates test calls to pass force_load where relevant.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor View
  participant Helper as ReactOnRails::Helper
  participant Utils as ReactOnRails::Utils
  participant Renderer as Server/Client Renderer

  View->>Helper: react_component / react_component_hash / redux_store(..., force_load?)
  Helper->>Utils: react_on_rails_pro_licence_valid?
  alt Pro valid OR force_load is false
    Helper->>Renderer: render content
    Renderer-->>Helper: server_rendered_html (String/Hash)
    Helper-->>View: html (no badge)
  else Pro missing AND force_load true
    Helper->>Helper: pro_warning_badge_if_needed(force_load) -> badge HTML
    Helper->>Renderer: render content
    Renderer-->>Helper: server_rendered_html (String/Hash)
    Helper-->>View: badge + html (prepended or placed in COMPONENT_HTML_KEY)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • Judahmeek

Poem

A little rabbit hops through code at night,
Leaves a badge that sparkles top-right.
If Pro's not here and force is pressed,
I whisper "license" and warn the rest.
Tests peek in my burrow—everything's bright. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Show warning badge when force_load feature is enabled and RORP is not used" succinctly and accurately summarizes the main change: adding a warning badge when force_load is requested but React On Rails Pro (RORP) is not present, and it is a single clear sentence that communicates the primary intent to reviewers.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch abanoubghadban/make-force-load-feature-works-with-rorp-only

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

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

@claude
Copy link

claude bot commented Sep 15, 2025

Code Review for PR #1780: Show warning badge when force_load feature is enabled and RORP is not used

📋 Overall Assessment

The implementation adds a visible warning badge when the force_load feature is used without a React on Rails Pro license. While the functionality works, there are several areas that need attention before merging.

✅ Strengths

  • Comprehensive test coverage with multiple scenarios
  • Consistent warning across all helper methods (react_component, react_component_hash, redux_store)
  • User-friendly visual indicator with tooltip and link to Pro information

🔴 Critical Issues

1. Method Visibility

The pro_warning_badge_if_needed method is currently public but should be private:

# lib/react_on_rails/helper.rb:450
def pro_warning_badge_if_needed(force_load)  # Should be private

This exposes internal implementation details. Move it to the private section.

2. Performance Concern - Repeated Badge Generation

The badge HTML is generated on every call when conditions are met. Consider memoizing or caching the badge HTML:

def pro_warning_badge_if_needed(force_load)
  @pro_warning_badge ||= begin
    # Generate badge HTML once
  end
end

🟡 Code Quality Issues

1. Inline Styles

The extensive inline CSS (lines 461-467) violates separation of concerns. Consider:

  • Moving styles to a CSS class in the asset pipeline
  • Or at minimum, extracting style constants

2. Console Output Side Effect

Line 455: puts warning_message outputs to console on every render, which can spam logs in production. Consider:

  • Remove the puts statement
  • Use Rails.logger only
  • Add a flag to show warning only once per session

3. HTML Safety

While .html_safe is used correctly, the concatenation pattern could be cleaner:

# Current
(badge + html).html_safe

# Better
safe_join([badge, html])

🟠 Security Considerations

  • The external link includes target="_blank" with rel="noopener noreferrer"
  • No XSS vulnerabilities identified ✅
  • However, consider adding CSP headers for inline styles

📊 Test Coverage

Excellent test coverage! The specs properly test:

  • Pro vs non-Pro scenarios ✅
  • force_load true/false/global configurations ✅
  • Logger warning calls ✅

Minor suggestion: Add a test for multiple components on the same page to ensure badge doesn't duplicate.

🚀 Performance Implications

  • The badge adds DOM manipulation on every page render
  • Multiple components will trigger multiple pro license checks
  • Consider adding a request-level cache for the license check

📝 Documentation & PR Description

  • PR description needs completion (template sections not filled)
  • CHANGELOG entry is missing
  • Documentation update needed for the new warning behavior

🔧 Suggested Improvements

  1. Extract badge generation to a separate class:
class ProLicenseWarningBadge
  def self.render(force_load_enabled)
    return "" unless should_show_warning?(force_load_enabled)
    # Badge generation logic
  end
end
  1. Add configuration option to disable warning:
ReactOnRails.configure do |config|
  config.suppress_pro_warning = true # For users who acknowledge but can't upgrade
end
  1. Consider less intrusive warning options:
  • Console warning only
  • Development-only visual indicator
  • One-time banner instead of persistent badge

✅ Action Items Before Merge

  1. Move pro_warning_badge_if_needed to private section
  2. Remove or throttle puts statement
  3. Complete PR description sections
  4. Add CHANGELOG entry
  5. Consider extracting inline styles
  6. Add performance optimization for repeated calls

Summary

The feature successfully warns users about Pro requirements, but needs refinement in code organization, performance, and user experience. The persistent fixed-position badge might be too intrusive for production use - consider making it configurable or less prominent.

Great work on the test coverage! With the suggested improvements, this will be a valuable addition to help users understand Pro feature requirements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/react_on_rails/helper.rb (1)

473-497: Critical: Method visibility has been inadvertently changed.

The removal of the private keyword before run_stream_inside_fiber has made this method and all subsequent methods (registered_stores, registered_stores_defer_render, registered_stores_including_deferred, create_render_options, etc.) public instead of private. This is a breaking change that exposes internal implementation details.

Add the private keyword before run_stream_inside_fiber to maintain proper encapsulation:

+    private
+
     def run_stream_inside_fiber
       unless ReactOnRails::Utils.react_on_rails_pro?
         raise ReactOnRails::Error,
               "You must use React on Rails Pro to use the stream_react_component method."
       end

Also applies to: 499-509

🧹 Nitpick comments (4)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)

25-29: Good test setup, but Pro stub at the global level may affect all tests.

The global stubbing of ReactOnRails::Utils to simulate a Pro license installed affects all tests in this file, even those that don't explicitly test Pro features. This could mask issues where non-Pro behavior should be tested. Consider moving this stub to specific contexts that require Pro functionality.

Consider moving the Pro stubs to specific contexts:

   before do
     allow(self).to receive(:request) {
       Struct.new("Request", :original_url, :env)
       Struct::Request.new(
         "http://foobar.com/development",
         { "HTTP_ACCEPT_LANGUAGE" => "en" }
       )
     }
-
-    allow(ReactOnRails::Utils).to receive_messages(
-      react_on_rails_pro?: true,
-      react_on_rails_pro_version: "4.0.0",
-      rsc_support_enabled?: false
-    )
   end

Then add the Pro stubs only to contexts that need them:

context "with Pro license features" do
  before do
    allow(ReactOnRails::Utils).to receive_messages(
      react_on_rails_pro?: true,
      react_on_rails_pro_version: "4.0.0",
      rsc_support_enabled?: false
    )
  end
  # ... Pro-specific tests
end
lib/react_on_rails/helper.rb (3)

450-471: Consider extracting badge HTML to a constant and improving console output.

The method implementation is solid, but could benefit from some refinements:

  1. The badge HTML string could be extracted to a constant for maintainability
  2. Using puts for warnings may not be appropriate in production environments
  3. The inline styles make the badge difficult to customize
+    PRO_WARNING_BADGE_STYLES = {
+      container: "position: fixed; top: 0; right: 0; width: 180px; height: 180px; overflow: hidden; z-index: 9999; pointer-events: none;",
+      badge: "position: absolute; top: 50px; right: -40px; transform: rotate(45deg); background-color: rgba(220, 53, 69, 0.85); color: white; padding: 7px 40px; text-align: center; font-weight: bold; font-family: sans-serif; font-size: 12px; box-shadow: 0 2px 8px rgba(0,0,0,0.3); pointer-events: auto;"
+    }.freeze
+
     def pro_warning_badge_if_needed(force_load)
       return "".html_safe if !force_load && !ReactOnRails.configuration.force_load
       return "".html_safe if ReactOnRails::Utils.react_on_rails_pro?
 
       warning_message = "[REACT ON RAILS] The 'force_load' feature requires a React on Rails Pro license. " \
                         "Please visit https://shakacode.com/react-on-rails-pro to learn more."
-      puts warning_message
+      Rails.logger.info(warning_message) unless Rails.env.test?
       Rails.logger.warn warning_message
 
       tooltip_text = "The 'force_load' feature requires a React on Rails Pro license. Click to learn more."
 
       badge_html = <<~HTML
         <a href="https://shakacode.com/react-on-rails-pro" target="_blank" rel="noopener noreferrer" title="#{tooltip_text}">
-          <div style="position: fixed; top: 0; right: 0; width: 180px; height: 180px; overflow: hidden; z-index: 9999; pointer-events: none;">
-            <div style="position: absolute; top: 50px; right: -40px; transform: rotate(45deg); background-color: rgba(220, 53, 69, 0.85); color: white; padding: 7px 40px; text-align: center; font-weight: bold; font-family: sans-serif; font-size: 12px; box-shadow: 0 2px 8px rgba(0,0,0,0.3); pointer-events: auto;">
+          <div style="#{PRO_WARNING_BADGE_STYLES[:container]}">
+            <div style="#{PRO_WARNING_BADGE_STYLES[:badge]}">
               React On Rails Pro Required
             </div>
           </div>
         </a>
       HTML
       badge_html.strip.html_safe
     end

450-471: Consider performance impact of badge injection.

The badge HTML is being generated and injected for every component, store, and hash render when force_load is enabled. This could impact performance if there are many components on a page.

Consider implementing a singleton pattern to ensure the badge is only rendered once per page:

def pro_warning_badge_if_needed(force_load)
  return "".html_safe if !force_load && !ReactOnRails.configuration.force_load
  return "".html_safe if ReactOnRails::Utils.react_on_rails_pro?
  
  # Only render badge once per request
  return "".html_safe if @pro_warning_badge_rendered
  @pro_warning_badge_rendered = true
  
  # ... rest of the implementation
end

456-457: Duplicate logging may cause confusion.

The warning message is being output twice - once with puts and once with Rails.logger.warn. This could lead to duplicate messages in logs.

Remove the puts statement as Rails.logger is sufficient:

       warning_message = "[REACT ON RAILS] The 'force_load' feature requires a React on Rails Pro license. " \
                         "Please visit https://shakacode.com/react-on-rails-pro to learn more."
-      puts warning_message
       Rails.logger.warn warning_message
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20e4455 and 8a639dc.

📒 Files selected for processing (2)
  • lib/react_on_rails/helper.rb (5 hunks)
  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
lib/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)

Files:

  • lib/react_on_rails/helper.rb
spec/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)

Files:

  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • lib/react_on_rails/helper.rb
  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
🧬 Code graph analysis (2)
lib/react_on_rails/helper.rb (3)
lib/react_on_rails/react_component/render_options.rb (1)
  • force_load (98-100)
lib/react_on_rails/configuration.rb (1)
  • configuration (16-57)
lib/react_on_rails/utils.rb (1)
  • react_on_rails_pro? (199-203)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (3)
lib/react_on_rails/helper.rb (3)
  • react_component (56-94)
  • react_component_hash (211-242)
  • redux_store (257-273)
lib/react_on_rails/react_component/render_options.rb (2)
  • props (26-28)
  • force_load (98-100)
lib/react_on_rails/controller.rb (1)
  • redux_store (15-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: dummy-app-integration-tests (oldest)
  • GitHub Check: dummy-app-integration-tests (newest)
  • GitHub Check: build
  • GitHub Check: rspec-package-tests (oldest)
  • GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (7)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (4)

380-451: Comprehensive Pro license warning test coverage.

The tests thoroughly cover various scenarios with Pro license presence/absence and force_load configuration. The test structure properly isolates contexts and verifies both badge inclusion and logger warnings.


453-507: Good test coverage for react_component_hash Pro warnings.

The tests properly validate that the Pro warning badge is injected into the componentHtml key when needed, maintaining consistency with other rendering methods.


511-511: Test updated correctly to include force_load parameter.

The redux_store call now properly passes the force_load parameter, aligning with the new API signature.


533-577: Well-structured Pro license warning tests for redux_store.

The tests follow the same comprehensive pattern as other Pro warning tests, checking badge inclusion and logger warnings across different Pro license and force_load scenarios.

lib/react_on_rails/helper.rb (3)

61-61: Pro warning badge correctly integrated into react_component.

The badge is properly computed and prepended to the HTML output for server-rendered components, ensuring it appears before the component content.

Also applies to: 71-71


217-217: Badge properly added to component hash output.

The Pro warning badge is correctly injected into the COMPONENT_HTML_KEY of the hash result, maintaining consistency with the react_component implementation.

Also applies to: 230-230


259-259: Badge correctly integrated with redux_store rendering.

The badge is properly computed and prepended to the rendered redux store data along with the Rails context.

Also applies to: 271-271

@claude
Copy link

claude bot commented Sep 15, 2025

Code Review for PR #1780: Show warning badge when force_load feature is enabled and RORP is not used

✅ Strengths

  1. Good feature implementation: The warning badge effectively communicates that the force_load feature requires a Pro license
  2. Comprehensive test coverage: Tests cover all scenarios - Pro/non-Pro licenses, force_load enabled/disabled, and different helper methods
  3. Consistent implementation: Badge warning is properly implemented across all three helper methods (react_component, react_component_hash, redux_store)
  4. Dual notification approach: Both visual badge and console/logger warnings ensure developers are aware of the limitation

🔍 Areas for Improvement

1. Method Visibility Issue 🚨

The pro_warning_badge_if_needed method should be private since it is an internal helper method not intended for public use.

2. Performance Concern ⚠️

The method logs to both puts and Rails.logger.warn on every render when force_load is enabled without Pro. This could flood logs in production. Consider:

  • Using a class-level flag to log only once per app lifecycle
  • Or rate-limiting the logging

3. HTML Safety Consideration

While the code uses .html_safe, the inline styles and fixed positioning could conflict with existing page layouts. Consider:

  • Adding a unique CSS class that users can override
  • Making the badge position configurable
  • Adding a way to dismiss or minimize the badge

4. Code Duplication

The badge insertion logic is repeated in all three helper methods. Consider extracting a common pattern for better maintainability.

5. Security: Tooltip Text Escaping

The tooltip text should be properly escaped to prevent potential XSS if the message ever becomes configurable.

📝 Checklist Items

Looking at the PR checklist:

  • ✅ Tests are added and comprehensive
  • ❌ Documentation needs updating - the CHANGELOG and README should mention this new behavior
  • ❌ The PR description still has the template text that should be filled in

🎯 Recommendations

  1. High Priority: Make pro_warning_badge_if_needed private
  2. High Priority: Update CHANGELOG.md with this feature
  3. Medium Priority: Consider performance implications of repeated logging
  4. Medium Priority: Update documentation to explain the force_load Pro requirement
  5. Low Priority: Consider making the badge styling more flexible/configurable

✨ Overall Assessment

This is a well-implemented feature that clearly communicates licensing requirements to developers. The code is clean and well-tested. With the suggested improvements (especially making the method private and updating documentation), this PR would be ready to merge.

Great work on the comprehensive test coverage! 👍

@claude
Copy link

claude bot commented Sep 15, 2025

Code Review for PR #1780: Show warning badge when force_load feature is enabled and RORP is not used

Overall Assessment

This PR successfully implements a visual warning system for unauthorized use of the force_load feature. The implementation is clean and well-tested. However, there are several areas that could be improved.

🟢 Strengths

  • ✅ Clear visual indicator with fixed positioning badge
  • ✅ Comprehensive test coverage for all scenarios (Pro/non-Pro, force_load on/off)
  • ✅ Consistent implementation across all three helper methods
  • ✅ Good use of memoization for license validation
  • ✅ Proper HTML escaping with html_safe

🟡 Code Quality & Best Practices

  1. Method visibility concern (lib/react_on_rails/helper.rb:450)

    • The pro_warning_badge_if_needed method is public but should likely be private
    • Consider moving it to the private section unless there's a specific reason for public visibility
  2. Module length issue (lib/react_on_rails/utils.rb:10)

    • Adding the Rubocop disable comment # rubocop:disable Metrics/ModuleLength indicates technical debt
    • Consider refactoring the Utils module into smaller, focused modules instead
  3. Inline styles (lib/react_on_rails/helper.rb:461-469)

    • The badge uses extensive inline styles which makes it hard to maintain
    • Consider extracting styles to a CSS class or at least constants for reusability

🟠 Potential Issues

  1. Z-index conflict risk

    • The badge uses z-index: 9999 which could conflict with other overlays (modals, tooltips)
    • Consider using a more reasonable z-index value or making it configurable
  2. Accessibility concerns

    • The badge might overlap important UI elements
    • No keyboard accessibility for the link (though pointer-events handles mouse events)
    • Consider adding ARIA attributes for screen readers
  3. Console output in production

    • The puts statement on line 456 outputs to STDOUT which may not be appropriate for production
    • Consider using only Rails.logger.warn or making console output configurable

🔴 Security Considerations

  • No security issues identified. The implementation properly escapes HTML and uses html_safe appropriately.

📊 Performance Considerations

  1. Memoization is good

    • The react_on_rails_pro_licence_valid? method properly memoizes the result
  2. Minor optimization opportunity

    • The badge HTML generation could be memoized since it's static content
    • Consider caching the badge HTML string at the class level

🧪 Test Coverage

  • Excellent test coverage for all scenarios
  • Tests properly mock the Pro license status
  • Good use of shared examples and contexts

📝 Suggestions for Improvement

  1. Extract badge HTML to a constant or method
  2. Make the warning message DRY by extracting to a constant
  3. Consider environment-specific behavior (maybe only show in dev/staging)
  4. Add configuration option to disable the badge

Checklist Items

  • Tests have been added/updated ✅
  • Documentation needs updating ❌ (README or docs should mention this new behavior)
  • CHANGELOG needs updating ❌ (as noted in PR checklist)

💬 Final Thoughts

This is a well-implemented feature that achieves its goal of clearly warning users about Pro license requirements. The main improvements would be around code organization (moving the method to private section, extracting constants) and addressing the minor UX/accessibility concerns. The test coverage is excellent.

Great work on the implementation! With the suggested improvements, this will be a solid addition to the codebase.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
lib/react_on_rails/utils.rb (1)

10-10: Scope RuboCop disable or justify it.

Consider limiting Metrics/ModuleLength disable to the smallest scope or add a brief justification. Long-term, splitting Utils into cohesive modules would remove the need to disable the cop.

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (3)

377-447: Strengthen assertions for badge prepend and add log check for global flag.

  • Verify the badge is prepended (not just included) by asserting order relative to the component HTML.
  • In the “global force_load” context, also assert a warning is logged for parity with the per‑component case.
@@
       context "when Pro license is NOT installed and force_load is true" do
         subject(:react_app) { react_component("App", props: props, force_load: true) }
@@
         it { is_expected.to include(badge_html_string) }
+
+        it "prepends the badge before the component HTML" do
+          html = react_app.to_s
+          expect(html.index(badge_html_string)).to be < html.index(react_component_div)
+        end
@@
       context "when Pro license is NOT installed and global force_load is true" do
         subject(:react_app) { react_component("App", props: props) }
@@
         it { is_expected.to include(badge_html_string) }
+
+        it "logs a warning" do
+          react_app
+          expect(Rails.logger).to have_received(:warn).with(a_string_matching(/The 'force_load' feature requires/))
+        end

Consider extracting shared_examples for the badge/log behavior across helpers to reduce duplication.


450-504: react_component_hash: assert prepend order and logging for invalid license.

Add an order check to ensure badge is prepended to componentHtml, and mirror the logging assertion.

@@
       context "when Pro license is NOT installed and force_load is true" do
         subject(:react_app) { react_component_hash("App", props: props, force_load: true) }
@@
-        it "adds badge to componentHtml" do
-          expect(react_app["componentHtml"]).to include(badge_html_string)
-        end
+        it "adds badge to componentHtml" do
+          expect(react_app["componentHtml"]).to include(badge_html_string)
+        end
+
+        it "prepends the badge before the component HTML" do
+          html = react_app["componentHtml"]
+          expect(html.index(badge_html_string)).to be < html.index("<div>Test</div>")
+        end
+
+        it "logs a warning" do
+          react_app
+          expect(Rails.logger).to have_received(:warn).with(a_string_matching(/The 'force_load' feature requires/))
+        end

529-572: redux_store: add prepend order, global flag, and defer-path coverage.

  • Verify badge is prepended ahead of the store script.
  • Add a global force_load true context similar to react_component coverage.
  • Add a defer: true case to assert that no badge is injected on deferred registration.
@@
       context "when Pro license is NOT installed and force_load is true" do
         subject(:store) { redux_store("reduxStore", props: props, force_load: true) }
@@
         it { is_expected.to include(badge_html_string) }
+
+        it "prepends the badge before the store script" do
+          html = store.to_s
+          expect(html.index(badge_html_string)).to be < html.index('<script type="application/json" data-js-react-on-rails-store="reduxStore"')
+        end
@@
       context "when Pro license IS installed and force_load is true" do
         subject(:store) { redux_store("reduxStore", props: props, force_load: true) }
@@
         it { is_expected.not_to include(badge_html_string) }
       end
+
+      context "when Pro license is NOT installed and global force_load is true" do
+        subject(:store) { redux_store("reduxStore", props: props) }
+        before { allow(ReactOnRails::Utils).to receive(:react_on_rails_pro_licence_valid?).and_return(false) }
+        around do |example|
+          ReactOnRails.configure { |config| config.force_load = true }
+          example.run
+          ReactOnRails.configure { |config| config.force_load = false }
+        end
+        it { is_expected.to include(badge_html_string) }
+        it "logs a warning" do
+          store
+          expect(Rails.logger).to have_received(:warn).with(a_string_matching(/The 'force_load' feature requires/))
+        end
+      end
+
+      context "when Pro license is NOT installed with defer: true" do
+        subject(:store) { redux_store("reduxStore", props: props, force_load: true, defer: true) }
+        before { allow(ReactOnRails::Utils).to receive(:react_on_rails_pro_licence_valid?).and_return(false) }
+        it { is_expected.not_to include(badge_html_string) }
+      end
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a639dc and 4d9014b.

📒 Files selected for processing (3)
  • lib/react_on_rails/helper.rb (5 hunks)
  • lib/react_on_rails/utils.rb (2 hunks)
  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/react_on_rails/helper.rb
🧰 Additional context used
📓 Path-based instructions (2)
lib/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)

Files:

  • lib/react_on_rails/utils.rb
spec/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)

Files:

  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
🧠 Learnings (4)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • lib/react_on_rails/utils.rb
  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.

Applied to files:

  • lib/react_on_rails/utils.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.

Applied to files:

  • lib/react_on_rails/utils.rb
🧬 Code graph analysis (1)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
lib/react_on_rails/helper.rb (3)
  • react_component (56-94)
  • react_component_hash (211-242)
  • redux_store (257-273)
lib/react_on_rails/react_component/render_options.rb (2)
  • props (26-28)
  • force_load (98-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: dummy-app-integration-tests (newest)
  • GitHub Check: dummy-app-integration-tests (oldest)
  • GitHub Check: rspec-package-tests (oldest)
  • GitHub Check: rspec-package-tests (newest)
  • GitHub Check: claude-review
  • GitHub Check: build
🔇 Additional comments (2)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)

24-27: Good default stub to isolate tests.

Stubbing react_on_rails_pro_licence_valid? to true by default reduces noise for unrelated specs. LGTM.


507-507: Subject defaults force_load: true – confirm no unintended coupling.

This default is fine, but ensure other contexts override as needed so badge expectations don’t depend on implicit state.

Comment on lines +216 to +229
def self.react_on_rails_pro_licence_valid?
return @react_on_rails_pro_licence_valid if defined?(@react_on_rails_pro_licence_valid)

@react_on_rails_pro_licence_valid = begin
return false unless react_on_rails_pro?

# Maintain compatibility with legacy versions of React on Rails Pro:
# Earlier releases did not require license validation, as they were distributed as private gems.
# This check ensures that the method works correctly regardless of the installed version.
return true unless ReactOnRailsPro::Utils.respond_to?(:licence_valid?)

ReactOnRailsPro::Utils.licence_valid?
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Memoization bug: early returns bypass the cache assignment.

return inside the begin block exits the method before assigning @react_on_rails_pro_licence_valid, so calls where Pro isn’t installed (or legacy without licence_valid?) won’t be cached. Refactor to compute and assign without early returns.

Apply:

-    def self.react_on_rails_pro_licence_valid?
-      return @react_on_rails_pro_licence_valid if defined?(@react_on_rails_pro_licence_valid)
-
-      @react_on_rails_pro_licence_valid = begin
-        return false unless react_on_rails_pro?
-
-        # Maintain compatibility with legacy versions of React on Rails Pro:
-        # Earlier releases did not require license validation, as they were distributed as private gems.
-        # This check ensures that the method works correctly regardless of the installed version.
-        return true unless ReactOnRailsPro::Utils.respond_to?(:licence_valid?)
-
-        ReactOnRailsPro::Utils.licence_valid?
-      end
-    end
+    def self.react_on_rails_pro_licence_valid?
+      return @react_on_rails_pro_licence_valid if defined?(@react_on_rails_pro_licence_valid)
+
+      @react_on_rails_pro_licence_valid =
+        if react_on_rails_pro?
+          # Maintain compatibility with legacy versions of React on Rails Pro:
+          # Earlier releases did not require license validation, as they were distributed as private gems.
+          # This check ensures that the method works correctly regardless of the installed version.
+          if ReactOnRailsPro::Utils.respond_to?(:licence_valid?)
+            ReactOnRailsPro::Utils.licence_valid?
+          else
+            true
+          end
+        else
+          false
+        end
+    end

Optionally add a test-only reset helper to avoid cross‑example leakage:

def self.reset_react_on_rails_pro_licence_cache!
  remove_instance_variable(:@react_on_rails_pro_licence_valid) if instance_variable_defined?(:@react_on_rails_pro_licence_valid)
end
🤖 Prompt for AI Agents
In lib/react_on_rails/utils.rb around lines 216 to 229, the method currently
returns early inside the begin block which prevents assigning the computed value
to @react_on_rails_pro_licence_valid; refactor so you compute a local result (no
early returns), assign it to @react_on_rails_pro_licence_valid, and then return
it — specifically: check react_on_rails_pro? and whether ReactOnRailsPro::Utils
responds_to?(:licence_valid?) using conditional expressions that set a local
variable (true/false or the call to licence_valid?), assign that variable to the
instance variable, and return it; optionally add the suggested
reset_react_on_rails_pro_licence_cache! helper to clear the cache in tests by
removing the instance variable if defined.

@justin808 justin808 merged commit d5873f1 into master Sep 15, 2025
13 of 14 checks passed
@justin808 justin808 deleted the abanoubghadban/make-force-load-feature-works-with-rorp-only branch September 15, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants