Move Pro Ruby code to MIT license #1855
Conversation
Moved lib/react_on_rails/pro/ code to MIT-licensed locations: - lib/react_on_rails/pro/helper.rb → lib/react_on_rails/pro_helper.rb - lib/react_on_rails/pro/utils.rb → lib/react_on_rails/pro_utils.rb Changes: - Removed Pro license headers from moved files - Changed namespace from ReactOnRails::Pro:: to ReactOnRails:: - Removed Pro warning badge functionality (no longer needed) - Deleted lib/react_on_rails/pro/ directory entirely - Updated LICENSE.md to reflect all lib/react_on_rails/ is MIT - Updated documentation references Benefits: - Ruby signaling code now in MIT gem for marketing visibility - Actual functionality still requires Pro npm package - Clean license boundaries maintained - Users can't bypass Pro features by editing Ruby code
WalkthroughLicensing text updated to MIT-license all of lib/react_on_rails/. Pro-licensed NOTICE and modules under lib/react_on_rails/pro/ were removed. New flat Pro helper/utils introduced at lib/react_on_rails/pro_helper.rb and lib/react_on_rails/pro_utils.rb. Helper include paths and requires were adjusted. Render options now reference ReactOnRails::ProUtils. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor View
participant Helper as ReactOnRails::Helper
participant ProUtils as ReactOnRails::ProUtils
participant ProHelper as ReactOnRails::ProHelper
Note over View,Helper: Component render
View->>Helper: generate_component_script(render_options)
Helper->>ProUtils: disable_pro_render_options_if_not_licensed(options)
ProUtils-->>Helper: { raw_options, explicitly_disabled_pro_options }
Helper->>ProHelper: generate_component_script(processed_options)
ProHelper-->>Helper: main script + optional immediate script
Helper-->>View: HTML scripts
rect rgb(245,245,255)
Note right of ProHelper: Immediate hydration path (if enabled)
end
sequenceDiagram
autonumber
actor View
participant Helper as ReactOnRails::Helper
participant ProUtils as ReactOnRails::ProUtils
participant ProHelper as ReactOnRails::ProHelper
Note over View,Helper: Store hydration
View->>Helper: generate_store_script(store_data)
Helper->>ProUtils: disable_pro_render_options_if_not_licensed(options)
ProUtils-->>Helper: { raw_options, explicitly_disabled_pro_options }
Helper->>ProHelper: generate_store_script(processed_store_data)
ProHelper-->>Helper: main script + optional immediate script
Helper-->>View: HTML scripts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code Review for PR 1855: Move Pro Ruby code to MIT license - I have reviewed the changes and have detailed feedback. Main strengths: (1) Clear intent and rationale with excellent documentation (2) Consistent namespace migration from ReactOnRails::Pro:: to ReactOnRails:: (3) Proper file hygiene with trailing newlines (4) Complete cleanup of Pro license headers. Main concerns: (1) Removed Pro warning badge functionality without replacement - is this intentional? (2) generate_store_script still calls disable_pro_render_options_if_not_licensed but ignores the explicitly_disabled_pro_options result (3) No test file changes in PR - need to verify tests still pass. Code quality notes: Redundant .strip_heredoc on squiggly heredoc at line 53 of pro_helper.rb. Inconsistent string interpolation styles between line 27 and 53. Security: Good use of escape_javascript for XSS prevention. Before merging: Run bundle exec rubocop, run full test suite, verify namespace migration is complete. Overall this is a well-structured refactoring with clear business value. Once the concerns about removed warning functionality and test coverage are addressed, this should be ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/react_on_rails/pro_helper.rb (2)
38-48: Remove unnecessary.html_safecall on attribute value.Line 45: Calling
.html_safeonstore_namebefore using it as an attribute value is unnecessary and potentially unsafe. Rails'content_tagautomatically escapes attribute values, and marking the value ashtml_safebypasses this protection. Whilestore_nameis typically a trusted value, this violates security best practices.Apply this diff to remove the unnecessary call:
- "data-js-react-on-rails-store" => redux_store_data[:store_name].html_safe, + "data-js-react-on-rails-store" => redux_store_data[:store_name],
50-60: Consider removing redundant.strip_heredoc.Line 53: The
.strip_heredoccall is redundant when using the<<~heredoc syntax, which already strips leading indentation in Ruby 2.3+. While harmless, removing it would make the code cleaner.Apply this diff to remove the redundant call:
- immediate_script = content_tag(:script, <<~JS.strip_heredoc.html_safe + immediate_script = content_tag(:script, <<~JS.html_safe typeof ReactOnRails === 'object' && ReactOnRails.reactOnRailsStoreLoaded('#{escaped_store_name}'); JS )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
LICENSE.md(1 hunks)docs/MONOREPO_MERGER_PLAN.md(1 hunks)lib/react_on_rails/helper.rb(1 hunks)lib/react_on_rails/pro/NOTICE(0 hunks)lib/react_on_rails/pro/helper.rb(0 hunks)lib/react_on_rails/pro/utils.rb(0 hunks)lib/react_on_rails/pro_helper.rb(1 hunks)lib/react_on_rails/pro_utils.rb(1 hunks)lib/react_on_rails/react_component/render_options.rb(2 hunks)
💤 Files with no reviewable changes (3)
- lib/react_on_rails/pro/helper.rb
- lib/react_on_rails/pro/NOTICE
- lib/react_on_rails/pro/utils.rb
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rb
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
docs/MONOREPO_MERGER_PLAN.mdLICENSE.md
🧠 Learnings (5)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.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 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/react_component/render_options.rblib/react_on_rails/pro_utils.rblib/react_on_rails/helper.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/react_on_rails/pro_helper.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/react_on_rails/helper.rb
🧬 Code graph analysis (3)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/pro_utils.rb (1)
disable_pro_render_options_if_not_licensed(13-35)
lib/react_on_rails/pro_utils.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro_licence_valid?(249-262)
lib/react_on_rails/pro_helper.rb (3)
lib/react_on_rails/helper.rb (1)
json_safe_and_pretty(350-361)lib/react_on_rails/react_component/render_options.rb (5)
client_props(34-46)dom_id(52-60)trace(82-84)store_dependencies(142-144)immediate_hydration(102-104)lib/react_on_rails/pro_utils.rb (1)
disable_pro_render_options_if_not_licensed(13-35)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (10)
LICENSE.md (1)
14-14: LGTM! License scope correctly updated.The MIT license now properly covers the entire
lib/react_on_rails/directory (including its subdirectories), aligning with the PR's objective to move Pro Ruby signaling code under MIT licensing. Pro licensing is correctly scoped to the separate packages and directories.docs/MONOREPO_MERGER_PLAN.md (1)
375-375: LGTM! Documentation aligns with license restructuring.The monorepo plan now correctly documents that
lib/react_on_rails/(entire directory) is MIT-licensed, consistent with the changes in LICENSE.md and the removal of the nested Pro directory structure.lib/react_on_rails/react_component/render_options.rb (2)
4-4: LGTM! Require path updated to new module structure.The require path correctly references the new flat module structure (
react_on_rails/pro_utilsinstead of the nestedreact_on_rails/pro/utils).
19-19: LGTM! Namespace correctly updated.The call site properly uses the new flattened namespace
ReactOnRails::ProUtilsinstead of the previous nestedReactOnRails::Pro::Utils, maintaining the same functionality while aligning with the restructured module organization.lib/react_on_rails/helper.rb (2)
14-14: LGTM! Require path updated for new module structure.The require correctly references the new flat file structure (
react_on_rails/pro_helperinstead of the nestedreact_on_rails/pro/helper).
19-19: LGTM! Module inclusion updated to new namespace.The include statement properly uses the flattened namespace
ReactOnRails::ProHelperinstead of the previous nestedReactOnRails::Pro::Helper, maintaining functionality while aligning with the restructured module organization.lib/react_on_rails/pro_utils.rb (2)
9-11: LGTM! Clean delegation to license validation.The method provides a clear interface for checking Pro feature availability by delegating to the established license validation logic.
13-35: LGTM! Pro option disabling logic is sound.The method correctly:
- Returns unchanged options when Pro features are supported
- Duplicates options to avoid mutating the original when Pro features are not supported
- Identifies enabled Pro options by checking both explicit option values and global configuration fallbacks
- Disables only the Pro options that were explicitly enabled
- Returns both the modified options and the list of disabled options for transparency
The logic properly handles the case where an option might not be explicitly set in
raw_optionsby falling back to the global configuration.lib/react_on_rails/pro_helper.rb (2)
1-6: LGTM!The module declaration follows Ruby best practices with
frozen_string_literal: trueand the flat namespace structure aligns with the PR objective of moving fromReactOnRails::Pro::toReactOnRails::.
7-34: Licensing check handled in RenderOptions initializer
ReactOnRails::ReactComponent::RenderOptions#initialize(lib/react_on_rails/react_component/render_options.rb:19) invokesProUtils.disable_pro_render_options_if_not_licensed, sogenerate_component_scriptreceives already-validated options.
Removed all test contexts for Pro warning badge functionality since the badge feature has been removed from the codebase. Tests removed: - react_component Pro license warning contexts - react_component_hash Pro license warning contexts - redux_store Pro license warning contexts
Code ReviewI've reviewed PR #1855 that moves Pro Ruby code from ✅ Strengths
🔍 Code Quality Observations1. Simplified Logic Flow (
2. Reduced Complexity (
|
Moved lib/react_on_rails/pro/ code to MIT-licensed locations: - lib/react_on_rails/pro/helper.rb → lib/react_on_rails/pro_helper.rb - lib/react_on_rails/pro/utils.rb → lib/react_on_rails/pro_utils.rb Changes: - Removed Pro license headers from moved files - Changed namespace from ReactOnRails::Pro:: to ReactOnRails:: - PRESERVED Pro warning badge functionality (shows when Pro features used without license) - Deleted lib/react_on_rails/pro/ directory - Updated LICENSE.md to reflect all lib/react_on_rails/ is MIT - Updated documentation references Benefits: - Ruby signaling code now in MIT gem for marketing visibility - Pro warning badge alerts users about Pro features - Actual functionality still requires Pro npm package - Clean license boundaries maintained
Pull Request Review - PR #1855: Move Pro Ruby code to MIT licenseSummaryThis PR refactors the Pro Ruby code by moving it from lib/react_on_rails/pro/ to the main codebase and changing the namespace from ReactOnRails::Pro:: to ReactOnRails::. The core functionality remains unchanged, with only organizational and licensing changes. Strengths
Observations and Minor Concerns1. Missing Newline CharacterPer CLAUDE.md requirements, all files MUST end with a newline character. Please verify lib/react_on_rails/pro_helper.rb:106 and lib/react_on_rails/pro_utils.rb:37 end with newlines. 2. RuboCop ComplianceAccording to CLAUDE.md, you MUST run bundle exec rubocop before every commit. Please run bundle exec rubocop on the new files and fix any violations. 3. Code Quality - Minor SuggestionsRedundant conditionals: The check in disabled_pro_features_message (line 96) is redundant since pro_warning_badge_if_needed already guards this. Consider removing. Magic strings: Consider extracting URLs and messages to constants for maintainability. 4. Test CoveragePlease verify tests properly import the new namespaces and handle edge cases like nil explicitly_disabled_pro_options. Security and LicensingSecurity posture is sound - moving signaling code to MIT does not create a bypass vector. Pro functionality enforcement still occurs in the Pro npm package via ReactOnRails::Utils.react_on_rails_pro_licence_valid?. Licensing is clear - LICENSE.md accurately reflects the new structure with proper separation between MIT (signaling) and Pro (functionality). Pre-Merge Checklist
RecommendationApprove with minor fixes required - This is a well-executed refactoring. The only blockers are:
Great work maintaining test coverage and documentation! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/react_on_rails/pro_helper.rb (2)
61-64: Redundantstrip_heredocwith squiggly heredoc.The
<<~syntax already strips leading whitespace, making the.strip_heredoccall redundant. While harmless, removing it would simplify the code.- immediate_script = content_tag(:script, <<~JS.strip_heredoc.html_safe + immediate_script = content_tag(:script, <<~JS.html_safe typeof ReactOnRails === 'object' && ReactOnRails.reactOnRailsStoreLoaded('#{escaped_store_name}'); JS )
74-94: Consider explicit HTML attribute escaping for defense in depth.While the
tooltip_textis built from controlled option names (fromPRO_ONLY_OPTIONSconstant), interpolating directly into the HTMLtitleattribute without explicit escaping could be improved for defensive programming.Since the option names are controlled and unlikely to contain HTML special characters, the current implementation is safe. However, for defense in depth, you could use
ERB::Util.html_escapeor Rails'hhelper ontooltip_textbefore interpolation:tooltip_text = ERB::Util.html_escape("#{disabled_features_message} Click to learn more.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/react_on_rails/pro_helper.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/pro_helper.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/pro_helper.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/react_on_rails/pro_helper.rb
🧬 Code graph analysis (1)
lib/react_on_rails/pro_helper.rb (3)
lib/react_on_rails/helper.rb (1)
json_safe_and_pretty(350-361)lib/react_on_rails/react_component/render_options.rb (5)
client_props(34-46)dom_id(52-60)trace(82-84)store_dependencies(142-144)immediate_hydration(102-104)lib/react_on_rails/pro_utils.rb (1)
disable_pro_render_options_if_not_licensed(13-35)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (3)
lib/react_on_rails/pro_helper.rb (3)
3-8: LGTM! Clean module structure.The module definition and warning constant are clear and well-structured, aligning with the MIT licensing approach for Pro signaling code.
11-41: Correct use of escape_javascript for XSS prevention.The method properly escapes the
dom_idbefore interpolating it into the JavaScript string literal, preventing potential XSS vulnerabilities. The conditional immediate hydration logic is clear and the html_safe usage is appropriate.
96-104: LGTM! Proper pluralization and defensive programming.The method correctly handles both singular and plural cases and includes a defensive empty check. The grammatically correct message building is well-implemented.
1 participant
Summary
This PR moves the Pro Ruby code from
lib/react_on_rails/pro/to the main MIT-licensed codebase atlib/react_on_rails/. This change improves marketing visibility while maintaining proper separation of Pro features.Changes
Files Moved
lib/react_on_rails/pro/helper.rb→lib/react_on_rails/pro_helper.rblib/react_on_rails/pro/utils.rb→lib/react_on_rails/pro_utils.rbCode Changes
ReactOnRails::Pro::toReactOnRails::lib/react_on_rails/pro/directory entirelyDocumentation Updates
LICENSE.mdto reflectlib/react_on_rails/is entirely MIT licenseddocs/MONOREPO_MERGER_PLAN.mdto match new license structureRationale
Marketing Benefits:
Technical Benefits:
packages/react-on-rails-pro/)License Compliance:
Testing
Closes #1854 (supersedes previous approach)
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores