Claude/remove pro warning badge #1993
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (15)
WalkthroughRemoved Pro warning UI and Pro-only utilities; global immediate_hydration config removed. Per-component immediate_hydration now resolves from explicit option or ReactOnRails::Utils.react_on_rails_pro?, with a logged warning if non‑Pro users explicitly request immediate_hydration. Tests and docs updated to match new behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant View as View / Helper
participant RenderOpts as RenderOptions
participant Utils as ReactOnRails::Utils
participant ProHelper as ProHelper
rect rgb(240,248,255)
View->>RenderOpts: initialize(options)
Note right of RenderOpts: stores options directly\n(no explicitly_disabled_pro_options)
alt immediate_hydration omitted
RenderOpts->>Utils: react_on_rails_pro?
Utils-->>RenderOpts: true/false
end
RenderOpts-->>View: resolved immediate_hydration
end
rect rgb(255,250,240)
View->>ProHelper: generate_component_script(..., immediate_hydration)
Note right of ProHelper: returns hydration/store script HTML\n(no Pro-warning UI)
ProHelper-->>View: hydration_script_html
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Pull Request ReviewThank you for this PR! This is a clean refactoring that removes the Pro license warning badges. Here's my comprehensive review: ✅ Positive AspectsCode Quality
Implementation Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review - PR #1993: Remove Pro Warning BadgeOverviewThis PR removes the visual warning badge that appeared when users tried to enable Pro-only features (like ✅ Strengths
🔍 Observations & Questions1. Divergent Warning Behavior Between Ruby and TypeScriptThe Ruby side now silently disables Pro features, but the TypeScript side still logs warnings: File: const IMMEDIATE_HYDRATION_PRO_WARNING =
"[REACT ON RAILS] The 'immediate_hydration' feature requires a React on Rails Pro license. " +
'Please visit https://shakacode.com/react-on-rails-pro to get a license.';File: if (isImmediateHydrationRequested && \!hasProLicense) {
console.warn(IMMEDIATE_HYDRATION_PRO_WARNING);
// Fallback to standard behavior...
}Question: Should the TypeScript warning also be removed for consistency? If not, this creates an asymmetric user experience where:
2. User Experience ConsiderationThe PR removes all user-facing notifications when Pro features are disabled. Consider: Pros of silent disabling:
Cons of silent disabling:
Suggestion: Consider at least keeping the 3. Missing CHANGELOG EntryThe PR checklist mentions updating the CHANGELOG, but no entry was added. This is a user-visible behavior change that should be documented: Suggested entry: #### Improvements
- Removed visual warning badge when Pro features are used without a license. Pro features now silently fall back to standard behavior. [PR 1993](https://github.com/shakacode/react_on_rails/pull/1993) by [claude](https://github.com/claude)🐛 Potential Issues4. Test Coverage GapThe PR removes ~90 lines of tests but doesn't add tests for the new silent behavior. Consider adding specs that verify: context 'when Pro license is NOT installed and immediate_hydration is true' do
it 'silently disables immediate_hydration without warnings' do
expect(Rails.logger).not_to receive(:warn)
result = react_component('App', props: props, immediate_hydration: true)
expect(result).not_to include('data-immediate-hydration="true"')
end
end5. Code Quality: Redundant ConditionalFile: The logic could be simplified slightly: PRO_ONLY_OPTIONS.each do |option|
# Determine if this option is enabled (either explicitly or via global config)
option_enabled = if raw_options[option].nil?
ReactOnRails.configuration.send(option)
else
raw_options[option]
end
# Silently disable the option if it's enabled but Pro is not available
raw_options_after_disable[option] = false if option_enabled
endSuggestion: Use PRO_ONLY_OPTIONS.each do |option|
option_enabled = raw_options.fetch(option) { ReactOnRails.configuration.send(option) }
raw_options_after_disable[option] = false if option_enabled
end🔒 SecurityNo security concerns identified. The change removes UI elements and simplifies internal logic without affecting security boundaries. ⚡ PerformancePositive impact: Removes string concatenation, HTML generation, and logging operations when Pro features are disabled. Minor performance improvement in the hot path. 📋 Checklist ReviewBased on the PR template:
🎯 Recommendations
✨ ConclusionThis is a well-executed refactoring that successfully removes the badge infrastructure. The code changes are clean, logical, and improve the API surface. However, there are a few areas that need attention:
Once these items are addressed, this PR will be ready to merge. The CI checks are currently running and should pass given the nature of the changes. Overall Assessment: ✅ Approve with minor changes requested Generated by Claude Code - Review based on CLAUDE.md guidelines and React on Rails conventions |
|
@claude update change log |
|
Claude finished @AbanoubGhadban's task —— View job CHANGELOG Updated Successfully ✅I've added the CHANGELOG entry for this PR under the "Changed" section of the Unreleased version. Tasks Completed
What was added:The following entry was added to
The change has been committed and pushed to the branch --- | |
Code Review SummaryThis PR successfully removes the visual warning badge functionality for Pro features. The changes are clean, well-structured, and maintain backward compatibility. Strengths1. Code Quality
2. Architecture and Design
3. Testing
4. Documentation
Suggestions for Enhancement1. Silent Failure ObservabilityThe silent disabling means users may not know why immediate_hydration is not working. Low priority suggestion: Add Rails logger info message when Pro features are disabled for debugging breadcrumbs (optional). 2. Missing Edge Case TestConsider adding a test verifying that when Pro is NOT installed and immediate_hydration: true is passed, the option is silently set to false and immediate hydration scripts are NOT rendered. This would document the new behavior. 3. Code SimplificationIn pro_utils.rb:18-28, could use fetch with block for slightly more idiomatic Ruby, but current version is also fine. Security and Performance
Best Practices AlignmentPer CLAUDE.md requirements:
Final RecommendationAPPROVE with optional enhancements suggested above. The PR achieves its goal cleanly. Code is simpler, tests updated appropriately, and changes are backward compatible. Silent disabling is reasonable for freemium model. Great work on the clean refactoring! The commit-by-commit structure made this very easy to review. |
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #1993Reviewing the changes to remove the Pro warning badge functionality. Tasks
OverviewThis PR removes the visual warning badge that previously appeared when non-Pro users tried to enable Pro-only features like ✅ Strengths1. Clean Code RemovalThe removal of the warning badge infrastructure is systematic and complete:
2. Simplified APIThe Before: result = ProUtils.disable_pro_render_options_if_not_licensed(options)
options = result[:raw_options]
disabled = result[:explicitly_disabled_pro_options]After: options = ProUtils.disable_pro_render_options_if_not_licensed(options)This is a significant improvement in clarity and ease of use. 3. Proper Test CoverageThe test changes are well thought out:
4. Good Commit StructureEach commit is focused and documents the "why":
5. CHANGELOG EntryThe CHANGELOG entry at line 52 properly documents this user-visible change following project conventions.
|
Code Review - PR #1993: Remove Pro Warning BadgeThank you for this well-structured PR! The code quality is excellent with clear commits and comprehensive test coverage. Below is my detailed review: ✅ StrengthsCode Quality
Design Improvements
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)
41-42: Remove redundant pro stub.Line 24 already stubs
ReactOnRails::Utils.react_on_rails_pro?totrueviareceive_messages, so this extraallow(...).and_return(true)adds noise and makes it harder to reason about contexts that flip the stub tofalse. Please drop the duplication and rely on the existing setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)lib/react_on_rails/configuration.rb(2 hunks)lib/react_on_rails/controller.rb(1 hunks)lib/react_on_rails/doctor.rb(2 hunks)lib/react_on_rails/helper.rb(1 hunks)lib/react_on_rails/pro_helper.rb(2 hunks)lib/react_on_rails/pro_utils.rb(0 hunks)lib/react_on_rails/react_component/render_options.rb(2 hunks)spec/dummy/config/initializers/react_on_rails.rb(0 hunks)spec/dummy/spec/helpers/react_on_rails_helper_spec.rb(1 hunks)spec/dummy/spec/system/integration_spec.rb(1 hunks)spec/react_on_rails/react_component/render_options_spec.rb(1 hunks)
💤 Files with no reviewable changes (2)
- spec/dummy/config/initializers/react_on_rails.rb
- lib/react_on_rails/pro_utils.rb
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rbspec/react_on_rails/react_component/render_options_spec.rblib/react_on_rails/controller.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rbspec/react_on_rails/react_component/render_options_spec.rblib/react_on_rails/controller.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rbspec/react_on_rails/react_component/render_options_spec.rblib/react_on_rails/controller.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 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.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rblib/react_on_rails/controller.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/helper.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rb
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/system/integration_spec.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
lib/react_on_rails/configuration.rbspec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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/configuration.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_helper.rblib/react_on_rails/controller.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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/pro_helper.rb
🧬 Code graph analysis (5)
lib/react_on_rails/configuration.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
auto_load_bundle(75-77)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
lib/react_on_rails/helper.rb (3)
lib/react_on_rails/controller.rb (1)
redux_store(17-24)lib/react_on_rails/react_component/render_options.rb (2)
props(27-29)immediate_hydration(99-114)lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
spec/react_on_rails/react_component/render_options_spec.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(99-114)
lib/react_on_rails/controller.rb (3)
lib/react_on_rails/helper.rb (1)
redux_store(160-175)lib/react_on_rails/react_component/render_options.rb (2)
props(27-29)immediate_hydration(99-114)lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
⏰ 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: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (9)
lib/react_on_rails/doctor.rb (1)
704-741: LGTM! Clean removal of immediate_hydration diagnostic output.The removal of immediate_hydration from the performance configuration analysis is consistent with the PR's goal of removing Pro-related configuration. The tightened RuboCop pragmas appropriately reflect the reduced complexity.
lib/react_on_rails/controller.rb (1)
12-18: LGTM! Correctly implements runtime Pro license check for default behavior.The documentation clearly describes the new behavior, and the implementation properly defaults to Pro status when immediate_hydration is nil. This aligns well with the PR's goal of removing configuration-driven Pro options.
lib/react_on_rails/configuration.rb (1)
65-83: LGTM! Complete removal of immediate_hydration from public configuration API.The immediate_hydration option has been cleanly removed from both the attr_accessor list and the initialize signature. This properly implements the shift from configuration-driven to runtime license-driven behavior as described in the PR objectives.
spec/dummy/spec/system/integration_spec.rb (1)
108-109: LGTM! Cleaner test approach using direct stub of Pro status.Replacing the configuration toggle with a direct stub of
ReactOnRails::Utils.react_on_rails_pro?is a more maintainable approach that aligns with the removal of immediate_hydration from the configuration API.lib/react_on_rails/helper.rb (1)
158-161: LGTM! Consistent with runtime Pro license check pattern.The documentation and implementation are consistent with the changes in controller.rb, properly defaulting immediate_hydration to Pro license status when nil. This maintains the same behavior across all view helpers.
spec/react_on_rails/react_component/render_options_spec.rb (1)
167-245: LGTM! Comprehensive test coverage for new immediate_hydration behavior.The test suite thoroughly covers all scenarios:
- Pro-licensed behavior (explicit true/false and defaults to true)
- Non-Pro behavior (explicit true with warning, false, and defaults to false)
- Proper verification of warning logging for non-Pro users setting immediate_hydration: true
This provides excellent confidence in the new license-driven behavior.
lib/react_on_rails/pro_helper.rb (1)
1-62: LGTM! Successfully removes pro warning badge UI while preserving functionality.The removal of pro warning badge logic significantly simplifies these helper methods. The immediate_hydration feature continues to work (script tags are generated when requested), but without the visual pro warning banners. This aligns perfectly with the PR's goal of silently handling Pro features rather than displaying warning badges to end users.
lib/react_on_rails/react_component/render_options.rb (2)
18-21: LGTM! Simplified initialization without ProUtils processing.The direct assignment of options and removal of
explicitly_disabled_pro_optionssimplifies the initialization logic. The removal of ProUtils integration aligns with the PR's goal of handling Pro features through runtime checks rather than preprocessing options.
99-114: LGTM! Well-implemented immediate_hydration with helpful warning.The new implementation correctly:
- Honors explicit settings while warning non-Pro users when they set immediate_hydration: true
- Defaults to Pro license status when not explicitly set
- Provides a clear, actionable warning message with a link to licensing information
This approach maintains backward compatibility (explicit settings are honored) while gently guiding users toward proper Pro licensing.
Code Review: Remove Pro Warning BadgeSummaryThis PR successfully removes the visual warning badge functionality and simplifies Pro feature handling by making immediate hydration automatic for Pro users. The changes are well-structured across focused commits. ✅ Strengths1. Excellent Code Organization
2. Thorough Test Coverage
3. Strong User Experience Improvements
4. Documentation & Communication
🔍 Areas for Improvement1. Potential Logic Issue in render_options.rb:99-112
|
|
/run-skipped-ci |
|
🚀 Skipped CI Checks - Trigger Results ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Use this command when you see skipped checks that you want to run. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/pro_utils.rb(0 hunks)lib/react_on_rails/react_component/render_options.rb(2 hunks)
💤 Files with no reviewable changes (1)
- lib/react_on_rails/pro_utils.rb
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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/react_component/render_options.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 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.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 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/react_component/render_options.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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/react_component/render_options.rb
🧬 Code graph analysis (1)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
⏰ 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). (4)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: markdown-link-check
🔇 Additional comments (1)
lib/react_on_rails/react_component/render_options.rb (1)
20-20: Verified: No code depends on explicitly_disabled_pro_options.Comprehensive search across the codebase confirms zero references to
explicitly_disabled_pro_options. The removal of thisattr_readeris safe and has no internal dependents.
|
/run-skipped-ci |
|
🚀 Skipped CI Checks - Trigger Results ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Use this command when you see skipped checks that you want to run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/react_on_rails/controller.rb (1)
18-28: Extract duplicated immediate_hydration validation logic.This warning and override logic is duplicated in three locations:
lib/react_on_rails/controller.rb(lines 18-28)lib/react_on_rails/helper.rb(lines 161-171)lib/react_on_rails/react_component/render_options.rb(lines 98-109)Consider extracting this to a utility method in
ReactOnRails::Utils:# In lib/react_on_rails/utils.rb def self.validate_immediate_hydration(value, context_name) return value unless value == true && !react_on_rails_pro? Rails.logger.warn <<~WARNING [REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license. #{context_name} will fall back to standard hydration behavior. Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information. WARNING false endThen call it from all three locations:
immediate_hydration = ReactOnRails::Utils.validate_immediate_hydration( immediate_hydration, "Store '#{store_name}'" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/react_on_rails/controller.rb(1 hunks)lib/react_on_rails/helper.rb(1 hunks)lib/react_on_rails/react_component/render_options.rb(2 hunks)spec/dummy/spec/helpers/react_on_rails_helper_spec.rb(2 hunks)spec/react_on_rails/react_component/render_options_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/react_on_rails/helper.rb
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/controller.rblib/react_on_rails/react_component/render_options.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/controller.rblib/react_on_rails/react_component/render_options.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/react_component/render_options.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 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.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/react_component/render_options.rbspec/react_on_rails/react_component/render_options_spec.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 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:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/react_component/render_options.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 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/react_component/render_options.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/react_component/render_options.rb
🧬 Code graph analysis (4)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (3)
lib/react_on_rails/controller.rb (1)
redux_store(17-35)lib/react_on_rails/helper.rb (1)
redux_store(160-185)lib/react_on_rails/react_component/render_options.rb (1)
props(26-28)
lib/react_on_rails/controller.rb (3)
lib/react_on_rails/helper.rb (1)
redux_store(160-185)lib/react_on_rails/react_component/render_options.rb (2)
props(26-28)immediate_hydration(98-114)lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)
spec/react_on_rails/react_component/render_options_spec.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(98-114)
⏰ 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: build
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (7)
spec/react_on_rails/react_component/render_options_spec.rb (1)
167-245: LGTM: Comprehensive test coverage for immediate_hydration behavior.The test suite thoroughly covers all scenarios:
- Pro users: explicit true/false and default behavior
- Non-Pro users: explicit true (with warning), explicit false, and default behavior
- Warning message validation for non-Pro users attempting to enable the feature
The test structure is clear and aligns well with the implementation.
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
41-42: LGTM: Establishes Pro-enabled baseline for tests.This global stub sets a reasonable default that individual tests can override when testing non-Pro scenarios.
435-470: LGTM: Comprehensive test coverage for non-Pro immediate_hydration scenarios.The tests thoroughly verify:
- Warning is logged when non-Pro user explicitly sets
immediate_hydration: true- No warning for explicit
falseornil(default)- Resulting HTML correctly reflects the enforced behavior (no
data-immediate-hydration="true")Test structure mirrors the component tests in
render_options_spec.rb, maintaining consistency across the test suite.lib/react_on_rails/react_component/render_options.rb (4)
15-18: LGTM: ProUtils processing removed as intended.Direct option assignment aligns with the PR objective to remove the Pro warning badge functionality.
20-20: LGTM: Simplified public API by removing explicitly_disabled_pro_options.This removal is consistent with eliminating the Pro warning badge functionality.
98-114: Refactor: Consolidate redundant option access.The logic correctly handles all scenarios, but accesses
options[:immediate_hydration]twice (line 99 and line 111). This can be simplified:def immediate_hydration - explicit_value = options[:immediate_hydration] - - # If non-Pro user explicitly sets immediate_hydration: true, warn and override to false - if explicit_value == true && !ReactOnRails::Utils.react_on_rails_pro? + options.fetch(:immediate_hydration) do + ReactOnRails::Utils.react_on_rails_pro? + end.tap do |value| + # If non-Pro user explicitly sets immediate_hydration: true, warn (enforcement happens here) + if value == true && !ReactOnRails::Utils.react_on_rails_pro? Rails.logger.warn <<~WARNING [REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license. Component '#{react_component_name}' will fall back to standard hydration behavior. Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information. WARNING - return false # Force fallback to standard hydration + end end - - options.fetch(:immediate_hydration) do - ReactOnRails::Utils.react_on_rails_pro? - end endWait - this alternative removes the
return false, which means non-Pro users would still getimmediate_hydration: truein the data attributes. Let me reconsider...Actually, the current implementation is correct because:
- It returns
falseearly when non-Pro user setstrue(line 108)- The warning message accurately reflects this behavior
The only improvement would be to eliminate the redundant access. A better refactor:
def immediate_hydration - explicit_value = options[:immediate_hydration] - - # If non-Pro user explicitly sets immediate_hydration: true, warn and override to false - if explicit_value == true && !ReactOnRails::Utils.react_on_rails_pro? - Rails.logger.warn <<~WARNING - [REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license. - Component '#{react_component_name}' will fall back to standard hydration behavior. - Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information. - WARNING - return false # Force fallback to standard hydration - end - options.fetch(:immediate_hydration) do ReactOnRails::Utils.react_on_rails_pro? + end.tap do |value| + # If non-Pro user explicitly sets immediate_hydration: true, warn + if value == true && !ReactOnRails::Utils.react_on_rails_pro? + Rails.logger.warn <<~WARNING + [REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license. + This setting will be ignored. Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information. + WARNING + end end endBut wait, this ALSO doesn't return false! The tap returns the original value.
Hmm, let me think about this more carefully. The current implementation:
- If user explicitly sets true and not Pro: returns false (feature disabled)
- Otherwise: returns the explicit value or default
Actually, I think the current implementation has a bug. Let me trace through again:
Non-Pro user sets immediate_hydration: true:
- Line 99: explicit_value = true
- Line 102: condition is true (true && !Pro)
- Lines 103-108: Warns and returns false ✓
But then in the client-side code, what happens? Looking at the relevant code snippets, I see the helper/controller sets data-immediate-hydration based on this return value. So returning false here means the feature is actually disabled. Good!
Wait, but the client-side enforcement is separate (per past review comments about ClientSideRenderer.ts). Let me reconsider the architecture:
Looking at past review comments, it seems:
- Server-side: Sets data-immediate-hydration attribute based on immediate_hydration value
- Client-side: Checks both the attribute AND the Pro license
So the current implementation is correct - it returns false when non-Pro user requests true, which prevents the data-immediate-hydration="true" attribute from being set.
However, there's still room for the simplification I mentioned earlier. But actually, the simplification I proposed would NOT enforce the false return value.
Let me think of a better approach. Actually, the current implementation is fine. The redundancy is minor. Let me instead just note that it's correct but could be slightly simplified, making this a lower-priority suggestion.
Actually, on further reflection, the current implementation is clear and correct. The redundancy is minimal and the code is easy to understand. I should just approve it with perhaps a minor note about potential simplification.
[actions]
- Approve the implementation as correct
- Optionally suggest minor simplification if it doesn't compromise clarity
[scratchpad_end] -->
98-114: LGTM: immediate_hydration logic correctly enforces Pro license requirement.The implementation correctly:
- Returns
false(disabling the feature) when non-Pro user explicitly setsimmediate_hydration: true- Logs an appropriate warning in that scenario
- Respects explicit
falsevalues- Defaults to Pro license status when option is omitted
The minor redundancy (accessing options twice) is acceptable for code clarity.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #1993Reviewing the changes to remove the Pro warning badge functionality. Tasks
Review SummaryThis PR successfully removes the Pro warning badge UI and implements a cleaner approach to Pro feature enforcement. The changes are well-implemented with proper test coverage and documentation. The most recent commit (7875349) addresses previous review feedback by properly enforcing the ✅ Strengths1. Clean ImplementationThe removal of the Pro warning badge infrastructure is systematic and complete:
2. Proper Pro Feature EnforcementThe latest commit properly enforces Pro feature requirements: In
In
3. Excellent Test CoverageNew tests in
Updated tests in
4. DocumentationCHANGELOG.md entries (lines 52, 54, 60-72):
5. Code Quality
💡 Observations & Suggestions1. Duplicate Enforcement Logic (Minor - Optional Improvement)The same warning/override logic appears in three places:
Suggestion: Consider extracting this to a utility method in def self.enforce_immediate_hydration_license(value, context_name)
if value == true && !react_on_rails_pro?
Rails.logger.warn <<~WARNING
[REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license.
#{context_name} will fall back to standard hydration behavior.
Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information.
WARNING
return false
end
value.nil? ? react_on_rails_pro? : value
endThis would reduce duplication and make the logic easier to maintain. However, the current approach is also acceptable and clear. 2. Architecture: Server vs Client EnforcementThe server-side code now properly enforces the Pro license by returning Based on past PR discussions, there's also client-side enforcement in 3. Configuration Removal ImpactThe removal of
🔍 Detailed Code Review
|
Remove the visual warning badge HTML generation code: - Remove IMMEDIATE_HYDRATION_PRO_WARNING constant - Remove pro_warning_badge_if_needed method - Remove disabled_pro_features_message method These methods generated a red banner in the corner of the page when non-pro users tried to enable pro-only features. This functionality is being removed as part of the plan to separate pro features from the core gem. The calls to pro_warning_badge_if_needed will be removed in subsequent commits.
Remove the call to pro_warning_badge_if_needed in the generate_component_script method. The method now returns the component specification tag directly without appending the warning badge HTML. This is part of removing the visual warning badge that appeared when non-pro users tried to use pro-only features.
Remove the call to pro_warning_badge_if_needed in the generate_store_script method and simplify the handling of the result from disable_pro_render_options_if_not_licensed. The method now: - Directly assigns the result without extracting sub-hashes - Returns the store hydration scripts without the warning badge This continues the removal of the visual warning badge for pro-only features.
Refactor disable_pro_render_options_if_not_licensed to: - Return raw options directly instead of a hash - No longer track explicitly_disabled_pro_options - Silently disable pro-only options when Pro is not available This simplification removes the need to track which options were disabled, as we no longer display a warning badge. The method now only disables pro-only features silently, which is the desired behavior going forward.
Simplify RenderOptions initialization by: - Removing @explicitly_disabled_pro_options instance variable - Removing explicitly_disabled_pro_options from attr_reader - Directly assigning the result of disable_pro_render_options_if_not_licensed This completes the removal of the tracking mechanism for disabled pro options, as this information is no longer needed without the warning badge.
Auto-fix Rubocop offense: - Remove extra empty line at module body end All Rubocop checks now pass.
Remove test contexts that verified the warning badge HTML and logging behavior: - Remove 'with Pro license warning' contexts from react_component tests - Remove 'with Pro license warning' contexts from react_component_hash tests - Remove 'with Pro license warning' contexts from redux_store tests These tests are no longer needed since the badge functionality has been removed. Pro features are now silently disabled when Pro is not installed, without displaying warnings or badges. Total: ~90 lines removed from test file
Document the removal of the visual warning badge that appeared when non-Pro users attempted to enable Pro-only features. Pro features are now silently disabled when a Pro license is not available. Co-authored-by: Abanoub Ghadban <[email protected]>
…1997) Why Simplify configuration for pro users. Still allows override at react_component level. Summary Removed visual warning badges for Pro features, made immediate hydration automatic for Pro users, and removed global config option. Key improvements - Automatic immediate hydration for Pro users (no config needed) - Rails logger warnings replace visual badges for better UX - Component-level overrides still supported via helper parameters Impact - Existing: Pro users unchanged; immediate hydration auto-enabled - New: Simpler config; non-Pro get warnings in logs, not UI Risks Breaking: config.immediate_hydration removed from initializers (see CHANGELOG migration steps). Security: None.
Remove the intermediate immediate_hydration_enabled? helper method from ProUtils and directly use ReactOnRails::Utils.react_on_rails_pro? instead. This simplifies the code by removing an unnecessary layer of indirection. The immediate_hydration_enabled? method was just a wrapper that called support_pro_features?, which itself just calls react_on_rails_pro?. Changes: - Remove immediate_hydration_enabled? method from ProUtils - Update all callers to use ReactOnRails::Utils.react_on_rails_pro? directly: - render_options.rb: immediate_hydration method - helper.rb: redux_store method - controller.rb: redux_store method - Update test mocks to stub react_on_rails_pro? instead of immediate_hydration_enabled? - Remove unnecessary test stubs in render_options_spec.rb 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After inlining immediate_hydration_enabled?, the ProUtils module only contains a single method (support_pro_features?) that is not used anywhere in the codebase. This commit removes the entire module as dead code. Changes: - Delete lib/react_on_rails/pro_utils.rb - Remove require "react_on_rails/pro_utils" from render_options.rb This simplifies the codebase by removing an unnecessary abstraction layer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…-Pro users This commit addresses several issues identified in code review: 1. Fixed immediate_hydration logic in RenderOptions - Non-Pro users setting immediate_hydration: true now get false enforced - Previously returned true despite warning about fallback - Added explicit return false to match warning message behavior 2. Added warning logic to redux_store methods - Both helper.rb and controller.rb now warn non-Pro users - Enforces fallback by overriding to false when Pro license missing - Provides consistent behavior with react_component helper 3. Updated test expectations - render_options_spec.rb now expects false (not true) for non-Pro users - Added comprehensive redux_store tests for warning behavior - Tests cover all three scenarios: true, false, and nil values 4. Clarified CHANGELOG.md - Explicitly states fallback is enforced (value overridden to false) - Covers both components and stores in the migration notes All tests passing and RuboCop clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Address code review feedback: 1. Extract duplicated warning message to shared utility method - Add ReactOnRails::Utils.immediate_hydration_pro_license_warning helper - Replace inline warning messages in controller, helper, and render_options 2. Add deprecation warning for config.immediate_hydration - Add getter/setter methods that log deprecation warnings - Direct users to remove the deprecated config from initializers - Maintains backward compatibility while guiding migration 3. Fix inconsistent nil handling in render_options.rb - Simplify immediate_hydration method logic - Use consistent pattern: explicit value first, then default to Pro status - Remove confusing mix of hash access methods All changes maintain backward compatibility while improving code maintainability and providing better user guidance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Address code review feedback: 1. Consolidate duplicated immediate_hydration logic - Create ReactOnRails::Utils.normalize_immediate_hydration method - Replace inline logic in controller, helper, and render_options - Reduces code duplication from 3 locations to 1 shared method 2. Document the == true check rationale - Added comprehensive YARD documentation - Explains strict equality is intentional (not truthy check) - Prevents false positives from strings like "yes"/"no" - Type safety handled at call site by Ruby's type system Benefits: - Single source of truth for immediate_hydration normalization - Easier to test and maintain - Clear documentation of behavior - Consistent warning messages across all entry points 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
claude/remove-pro-warning-badge-011CV4Fb98f7Cdn9uPm4ACU6
branch
from
fd25322 to
ce7d8f2
Compare
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #1993Reviewing the changes to remove the Pro warning badge functionality. Tasks
OverviewThis PR successfully removes the Pro warning badge UI and implements a cleaner approach to Pro feature enforcement. The changes are well-implemented with proper test coverage and documentation. The most recent commit (7875349) addresses previous review feedback by properly enforcing the ✅ Strengths1. Clean ImplementationThe removal of the Pro warning badge infrastructure is systematic and complete:
2. Proper Pro Feature EnforcementThe latest commit properly enforces Pro feature requirements: In
In
3. Excellent Test CoverageNew tests in
Updated tests in
4. DocumentationCHANGELOG.md entries (lines 52, 54, 60-72):
5. Code Quality
💡 Observations & Suggestions1. Duplicate Enforcement Logic (Minor - Optional Improvement)The same warning/override logic appears in three places:
Suggestion: Consider extracting this to a utility method in def self.enforce_immediate_hydration_license(value, context_name)
if value == true && !react_on_rails_pro?
Rails.logger.warn <<~WARNING
[REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license.
#{context_name} will fall back to standard hydration behavior.
Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information.
WARNING
return false
end
value.nil? ? react_on_rails_pro? : value
endThis would reduce duplication and make the logic easier to maintain. However, the current approach is also acceptable and clear. 2. Architecture: Server vs Client EnforcementThe server-side code now properly enforces the Pro license by returning Based on past PR discussions, there's also client-side enforcement in 3. Configuration Removal ImpactThe removal of
🔍 Detailed Code Review
|
…pe validation This commit addresses three issues identified during code review: 1. Deprecation warnings now use warning-once pattern - Added class instance variable to track if warning shown - Prevents log flooding when config methods accessed repeatedly - Uses recommended Ruby pattern instead of class variables 2. Added type validation for immediate_hydration parameter - Validates that value is true, false, or nil - Raises ArgumentError with clear message for invalid types - Prevents unexpected behavior from truthy non-boolean values 3. Fixed CHANGELOG formatting for consistency - Changed [PR 1993] to [PR #1993] to match other entries All changes include comprehensive test coverage and pass RuboCop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…and deprecation detection - Add comprehensive unit tests for ReactOnRails::Utils.normalize_immediate_hydration - Tests for Pro/non-Pro license scenarios - Tests for invalid value type validation - Tests for warning messages with component/store names - Add tests for deprecated config.immediate_hydration setter/getter - Tests for deprecation warnings on both setter and getter - Tests for warning suppression (only warns once) - Tests for setter/getter interaction scenarios - Fix thread-safety of deprecation warning flag - Implement Mutex-based synchronization for @immediate_hydration_warned flag - Ensures safe access in multi-threaded environments (Puma, Sidekiq, etc.) - Add doctor.rb detection for deprecated config.immediate_hydration - Helps users identify and remove deprecated configuration during migration - Provides clear guidance on automatic behavior for Pro users - Remove unnecessary blank line in controller.rb for cleaner code All tests passing. Zero RuboCop violations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #1993Reviewing the changes to remove the Pro warning badge functionality. Tasks
OverviewThis PR successfully removes the Pro warning badge UI and implements a cleaner approach to Pro feature enforcement. The changes are well-implemented with proper test coverage and documentation. The most recent commit (7875349) addresses previous review feedback by properly enforcing the ✅ Strengths1. Clean ImplementationThe removal of the Pro warning badge infrastructure is systematic and complete:
2. Proper Pro Feature EnforcementThe latest commit properly enforces Pro feature requirements: In
In
3. Excellent Test CoverageNew tests in
Updated tests in
4. DocumentationCHANGELOG.md entries (lines 52, 54, 60-72):
5. Code Quality
💡 Observations & Suggestions1. Duplicate Enforcement Logic (Minor - Optional Improvement)The same warning/override logic appears in three places:
Suggestion: Consider extracting this to a utility method in def self.enforce_immediate_hydration_license(value, context_name, context_type)
if value == true && !react_on_rails_pro?
Rails.logger.warn <<~WARNING
[REACT ON RAILS] Warning: immediate_hydration: true requires a React on Rails Pro license.
#{context_type} '#{context_name}' will fall back to standard hydration behavior.
Visit https://www.shakacode.com/react-on-rails-pro/ for licensing information.
WARNING
return false
end
value.nil? ? react_on_rails_pro? : value
endThis would reduce duplication and make the logic easier to maintain. However, the current approach is also acceptable and clear. 2. Architecture: Server vs Client EnforcementThe server-side code now properly enforces the Pro license by returning Based on past PR discussions, there's also client-side enforcement in 3. Configuration Removal ImpactThe removal of
🔍 Detailed Code Review
|
…license - Pro users (Shakapacker >= 8.2.0): defaults to :async for optimal performance - Non-Pro users (Shakapacker >= 8.2.0): defaults to :defer for compatibility - Older Shakapacker versions: continues to default to :sync (unchanged) This aligns with the immediate_hydration behavior where Pro features are automatically enabled based on license detection, providing the best default experience for each user type. Updated tests to verify Pro/non-Pro default behaviors separately while maintaining tests for explicit value overrides. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
deleted the
claude/remove-pro-warning-badge-011CV4Fb98f7Cdn9uPm4ACU6
branch
Adds proactive detection of :async loading strategy usage in projects without React on Rails Pro, which can cause component registration race conditions. The doctor now checks for: 1. javascript_pack_tag with :async in view files 2. config.generated_component_packs_loading_strategy = :async in initializer When detected without Pro, provides clear guidance: - Upgrade to React on Rails Pro (recommended) - Change to :defer or :sync loading strategy This complements PR #1993's configuration validation by adding runtime doctor checks that help users identify and fix async usage issues during development. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After PR #1993 introduced sensible defaults for advanced options like generated_component_packs_loading_strategy (auto-configured based on Pro license), we can significantly simplify generated configuration files and documentation to make React on Rails easier to use. Changes: **Generator Template Simplification:** - Reduced react_on_rails.rb.tt from 67 to 42 lines - Removed redundant explanations for options with good defaults - Added clear pointer to docs for advanced configuration - Kept only essential options in generated files - Commented out file-system component registry (optional feature) **Documentation Restructuring:** - Added "Quick Start" section showing minimal config - Reorganized into "Essential" vs "Advanced" configuration - Documented which options auto-configure based on Pro license - Added clear defaults and recommendations for each option - Improved searchability with structured sections - Added deprecation section for immediate_hydration **Essential Configuration (in generated files):** - server_bundle_js_file - Required for SSR - build_test_command - Used for testing - components_subdirectory - Optional file-based registry - auto_load_bundle - Optional auto-loading **Advanced Configuration (docs only, sensible defaults):** - generated_component_packs_loading_strategy (auto: :async for Pro, :defer for non-Pro) - server_bundle_output_path (default: "ssr-generated") - enforce_private_server_bundles (default: false) - All rendering pool, debugging, and customization options **Dummy App Updates:** - Updated all dummy app configs with clear comments - Separated essential from test-specific configuration - Maintained same functionality with better organization Benefits: - New users see only 2-3 required config options - Clear path from simple to advanced configuration - Reduced cognitive load and decision fatigue - Better reflects PR #1993 auto-configuration improvements - Easier to understand what needs customization Closes: Follow-up to PR #1993 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds proactive detection of :async loading strategy usage in projects without React on Rails Pro, which can cause component registration race conditions. The doctor now checks for: 1. javascript_pack_tag with :async in view files 2. config.generated_component_packs_loading_strategy = :async in initializer When detected without Pro, provides clear guidance: - Upgrade to React on Rails Pro (recommended) - Change to :defer or :sync loading strategy This complements PR #1993's configuration validation by adding runtime doctor checks that help users identify and fix async usage issues during development. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After PR #1993 introduced sensible defaults for advanced options like generated_component_packs_loading_strategy (auto-configured based on Pro license), we can significantly simplify generated configuration files and documentation to make React on Rails easier to use. Changes: **Generator Template Simplification:** - Reduced react_on_rails.rb.tt from 67 to 42 lines - Removed redundant explanations for options with good defaults - Added clear pointer to docs for advanced configuration - Kept only essential options in generated files - Commented out file-system component registry (optional feature) **Documentation Restructuring:** - Added "Quick Start" section showing minimal config - Reorganized into "Essential" vs "Advanced" configuration - Documented which options auto-configure based on Pro license - Added clear defaults and recommendations for each option - Improved searchability with structured sections - Added deprecation section for immediate_hydration **Essential Configuration (in generated files):** - server_bundle_js_file - Required for SSR - build_test_command - Used for testing - components_subdirectory - Optional file-based registry - auto_load_bundle - Optional auto-loading **Advanced Configuration (docs only, sensible defaults):** - generated_component_packs_loading_strategy (auto: :async for Pro, :defer for non-Pro) - server_bundle_output_path (default: "ssr-generated") - enforce_private_server_bundles (default: false) - All rendering pool, debugging, and customization options **Dummy App Updates:** - Updated all dummy app configs with clear comments - Separated essential from test-specific configuration - Maintained same functionality with better organization Benefits: - New users see only 2-3 required config options - Clear path from simple to advanced configuration - Reduced cognitive load and decision fatigue - Better reflects PR #1993 auto-configuration improvements - Easier to understand what needs customization Closes: Follow-up to PR #1993 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
## Summary Adds proactive doctor checks to detect `:async` loading strategy usage in projects without React on Rails Pro, which can cause component registration race conditions. ## Changes The doctor now checks for and reports errors when it detects: - `javascript_pack_tag` with `:async` in view files - `config.generated_component_packs_loading_strategy = :async` in initializer When detected without Pro, provides clear guidance to either upgrade to Pro or use `:defer`/`:sync` loading strategies. This complements PR #1993's configuration validation by adding runtime detection during development to help developers identify and fix async usage issues. ## Testing All new tests pass (155+ examples in doctor spec). The checks are skipped when React on Rails Pro is installed. <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/2010) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Diagnostic that detects async component-loading usage when the Pro edition is not present, reporting affected view and initializer locations, distinguishing commented/false-positive cases, and providing guidance and upgrade options. * **Tests** * Expanded tests for async-detection across template formats, comment-aware and multiline scanning, initializer strategies, error handling, and skipping when Pro is installed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
## Summary After PR #1993 introduced sensible defaults for advanced options like `generated_component_packs_loading_strategy` (auto-configured based on Pro license), we can significantly simplify generated configuration files and documentation to make React on Rails easier to use. This PR reduces configuration complexity while maintaining full functionality through smart defaults. ## Key Improvements ### Generator Template Simplification - **Reduced from 67 to 42 lines** (37% reduction) - Removed redundant explanations for options with good defaults - Added clear pointer to docs for advanced configuration - Kept only essential options in generated files - Made file-system component registry commented out (optional feature) ### Documentation Restructuring - Added **"Quick Start" section** showing minimal 2-3 line config - Reorganized into **"Essential" vs "Advanced"** configuration sections - Documented which options **auto-configure based on Pro license** - Added clear defaults and recommendations for each option - Improved searchability with structured sections - Added deprecation section for `immediate_hydration` ### Configuration Categories **Essential Configuration** (in generated files): - `server_bundle_js_file` - Required for SSR - `build_test_command` - Used for testing - `components_subdirectory` - Optional file-based registry - `auto_load_bundle` - Optional auto-loading **Advanced Configuration** (docs only, sensible defaults): - `generated_component_packs_loading_strategy` - Auto: `:async` for Pro, `:defer` for non-Pro - `server_bundle_output_path` - Default: `"ssr-generated"` - `enforce_private_server_bundles` - Default: `false` - All rendering pool, debugging, and customization options ### Dummy App Updates - Updated all dummy app configs with clear comments - Separated essential from test-specific configuration - Maintained same functionality with better organization ## Example: Before and After **Before** (67 lines with lots of inline documentation): ```ruby ReactOnRails.configure do |config| # This configures the script to run to build the production assets by webpack... # [30 lines of comments explaining options] config.server_bundle_js_file = "server-bundle.js" config.server_bundle_output_path = "ssr-generated" config.enforce_private_server_bundles = true # [More options and explanations] end ``` **After** (42 lines, clearer structure): ```ruby ReactOnRails.configure do |config| # Server Rendering config.server_bundle_js_file = "server-bundle.js" # Test Configuration config.build_test_command = "RAILS_ENV=test bin/shakapacker" # File System Based Component Registry (Optional) # config.components_subdirectory = "ror_components" # config.auto_load_bundle = true # Advanced Configuration # See: https://github.com/shakacode/react_on_rails/blob/master/docs/api-reference/configuration.md end ``` ## Benefits 1. **Reduced cognitive load**: New users see only 2-3 required config options 2. **Clear upgrade path**: Easy progression from simple to advanced configuration 3. **Decision fatigue reduction**: Fewer choices to make during setup 4. **Better reflects improvements**: Showcases PR #1993's auto-configuration 5. **Easier onboarding**: Clearer understanding of what needs customization ## Testing - ✅ All configuration specs pass - ✅ RuboCop passes on all modified files - ✅ Pre-commit hooks pass (Prettier, ESLint, trailing newlines) - ✅ Dummy apps load successfully with simplified configs ## Files Changed - `lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt` - Simplified generator template - `docs/api-reference/configuration.md` - Restructured documentation with Essential/Advanced sections - `spec/dummy/config/initializers/react_on_rails.rb` - Updated with better organization - `react_on_rails_pro/spec/dummy/config/initializers/react_on_rails.rb` - Updated Pro dummy - `react_on_rails_pro/spec/execjs-compatible-dummy/config/initializers/react_on_rails.rb` - Updated execjs dummy ## Related - Follow-up to PR #1993 (immediate hydration and loading strategy auto-configuration) - Addresses the goal of making React on Rails easier to use with sensible defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/2011) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a reworked main configuration reference with examples, a deprecated/removed options guide, Pro configuration docs, and a new testing-configuration guide. * **New Features** * Added diagnostics that detect inconsistent server-side rendering and test asset compilation setups and surface actionable guidance. * **Tests** * Updated test app configuration examples and defaults to clarify testing behaviors and recommended setups. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
Summary
This PR removes the
pro_warning_badgefunctionality from React on Rails. The warning badge previously appeared when non-pro users tried to enable pro-only features likeimmediate_hydration. Going forward, these features will be silently disabled without displaying a visual warning badge.Changes Made
Removed Badge HTML Generation
Removed
pro_warning_badge_if_neededmethod that generated the red banner HTMLRemoved
disabled_pro_features_messagehelper methodRemoved
IMMEDIATE_HYDRATION_PRO_WARNINGconstantSimplified Pro Feature Handling
Updated
generate_component_scriptto remove badge renderingUpdated
generate_store_scriptto remove badge rendering and simplified result handlingRefactored
ProUtils.disable_pro_render_options_if_not_licensedto:Return options directly instead of a hash with metadata
Silently disable pro-only features without tracking which were disabled
Simplified
RenderOptionsinitialization to directly use the processed optionsRemoved
explicitly_disabled_pro_optionsinstance variable and attr_readerBehavior Changes
Before: When a non-pro user tried to use
immediate_hydration, a red "React On Rails Pro Required" badge would appear in the top-right corner of the page, and warning messages would be logged.After: The
immediate_hydrationfeature is silently disabled when Pro is not installed. No visual badge or warnings are displayed.Testing
All Rubocop checks pass
The changes maintain backward compatibility
Pro features continue to work normally when Pro is installed
Pro features are silently disabled when Pro is not installed
Commit Structure
The changes are divided into small, focused commits for easy review:
Remove pro_warning_badge helper methods and constant (35 lines deleted)
Remove pro_warning_badge from generate_component_script (1 line changed)
Remove pro_warning_badge from generate_store_script (7 lines changed)
Simplify ProUtils to return options directly (26 lines changed)
Remove explicitly_disabled_pro_options from RenderOptions (7 lines changed)
Fix Rubocop violations (1 line changed)
Total: ~77 lines changed across 3 files
Next Steps
This is the first step in separating pro features from the core gem. Future work will involve:
Moving pro-only features entirely to the React on Rails Pro package
Removing pro feature options from the core gem configuration
This change is
Summary by CodeRabbit
Bug Fixes
Behavior Change