Remove immediate_hydration config - Pro always hydrates immediately #1997
Conversation
Simplifies immediate_hydration to be automatic based on Pro availability: - Pro users: Always get immediate hydration - Non-Pro users: Never get immediate hydration - Can still be explicitly overridden via method parameter Changes ProUtils to provide immediate_hydration_enabled? method that returns true for Pro, false for non-Pro. Updates helper.rb, controller.rb, and render_options.rb to use this method instead of checking configuration.
Removes immediate_hydration configuration option from ReactOnRails core. This feature is now automatically enabled for Pro users and disabled for non-Pro users without requiring any configuration. Removed from: - Default configuration values - attr_accessor list - initialize parameters - initialize assignments
- Removes immediate_hydration check from doctor.rb config analysis - Updates tests to stub ReactOnRails::ProUtils.immediate_hydration_enabled? instead of setting config.immediate_hydration - Removes invalid config.immediate_hydration from dummy app initializer Tests now properly verify that Pro users get immediate hydration and non-Pro users don't, without requiring explicit configuration.
WalkthroughRemoves the public Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Helper/Controller
participant ProUtils as ReactOnRails::ProUtils
Note right of Caller: immediate_hydration param may be\ntrue/false or nil
alt explicit immediate_hydration provided
Caller->>Caller: use provided boolean
else immediate_hydration is nil
Caller->>ProUtils: immediate_hydration_enabled?()
ProUtils-->>Caller: true/false
end
Caller->>Caller: assemble hydration scripts / render payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 - PR #1997Thank you for this refactoring! The goal of simplifying the immediate_hydration configuration is excellent. However, I've identified several critical issues that need to be addressed before merging. 🚨 Critical Issues1. Broken Method Call - Will Cause Runtime ErrorsLocation: Problem: The code still calls # In pro_helper.rb:41
redux_store_data = ReactOnRails::ProUtils.disable_pro_render_options_if_not_licensed(redux_store_data)
# In render_options.rb:18
@options = ReactOnRails::ProUtils.disable_pro_render_options_if_not_licensed(options)Impact: This will cause Solution: Since the method only handled # pro_helper.rb:41 - Just use redux_store_data directly
# No transformation needed anymore
# render_options.rb:18 - Just use options directly
@options = options2. Incomplete Refactoring in pro_helper.rbLocation: The method call on line 41 suggests there might be other Pro options that need disabling when Pro is not licensed. However:
Question: Are there any other Pro options that need to be disabled for non-Pro users? If not, this transformation should be removed entirely.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/react_on_rails/doctor.rb (1)
704-741: Doctor: detect removedconfig.immediate_hydrationand guide migrationSince the option was removed, add a deprecation/breaking-change warning when it’s present in
config/initializers/react_on_rails.rb.Apply this diff (outside this hunk) inside
check_deprecated_configuration_settings:def check_deprecated_configuration_settings return unless File.exist?("config/initializers/react_on_rails.rb") content = File.read("config/initializers/react_on_rails.rb") deprecated_settings = [] + # Removed settings (will error on boot) + if content.include?("config.immediate_hydration") + deprecated_settings << "immediate_hydration (REMOVED) — delete this line. Immediate hydration now defaults based on Pro availability and can be overridden per helper call." + end + # Check for deprecated settings if content.include?("config.generated_assets_dirs") deprecated_settings << "generated_assets_dirs (use generated_assets_dir)" end
🧹 Nitpick comments (4)
CHANGELOG.md (1)
52-53: Align entry with this PR and call out config removal
- Link should reference PR #1997, not #1993.
- Add a note that the
config.immediate_hydrationoption was removed and immediate hydration now follows Pro availability (override via helper param).Example tweak:
- - **Removed Pro Warning Badge**: Removed the visual warning badge that appeared when non-Pro users attempted to enable Pro-only features like `immediate_hydration`. Pro features are now silently disabled when a Pro license is not available, providing a cleaner user experience without intrusive warning banners. [PR 1993](https://github.com/shakacode/react_on_rails/pull/1993) by [AbanoubGhadban](https://github.com/AbanoubGhadban). + - **Removed Pro Warning Badge**: Removed the visual warning badge shown when non‑Pro users attempted to enable Pro‑only features. Pro features are now silently disabled. Also removed `config.immediate_hydration`; immediate hydration now defaults based on Pro availability (can be overridden per helper call). [PR 1997](https://github.com/shakacode/react_on_rails/pull/1997) by [AbanoubGhadban](https://github.com/AbanoubGhadban).spec/dummy/spec/system/integration_spec.rb (1)
108-110: Redundant stubbing; keep only the ProUtils stubYou already stub
ReactOnRails::Utils.react_on_rails_pro?above. Since immediate hydration now depends onProUtils.immediate_hydration_enabled?, the Utils stub is unnecessary for these expectations. Consider removing it for clarity.spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)
41-43: Good stub; consider adding a non‑Pro default testAdd a spec that stubs
ReactOnRails::ProUtils.immediate_hydration_enabled?to false and asserts no immediate hydration script is emitted when not explicitly set.Example:
it "does not immediate-hydrate by default when Pro is unavailable" do allow(ReactOnRails::ProUtils).to receive(:immediate_hydration_enabled?).and_return(false) result = react_component("App", props: { name: "X" }) expect(result).not_to include("reactOnRailsComponentLoaded('App-react-component-0')") endlib/react_on_rails/helper.rb (1)
158-162: LGTM; optional DRY helper for hydration defaultBehavior is correct. To avoid duplication (here, Controller, RenderOptions), consider extracting a tiny helper, e.g.,
ProUtils.resolve_immediate_hydration(value)that returnsvalue.nil? ? immediate_hydration_enabled? : value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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(1 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)
💤 Files with no reviewable changes (1)
- spec/dummy/config/initializers/react_on_rails.rb
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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: 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: 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.
📚 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/configuration.rblib/react_on_rails/helper.rblib/react_on_rails/react_component/render_options.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.rblib/react_on_rails/controller.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/configuration.rbspec/dummy/spec/system/integration_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.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/configuration.rblib/react_on_rails/react_component/render_options.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.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:
lib/react_on_rails/configuration.rblib/react_on_rails/helper.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/doctor.rblib/react_on_rails/pro_helper.rblib/react_on_rails/controller.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/configuration.rblib/react_on_rails/helper.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rbCHANGELOG.mdspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.rblib/react_on_rails/controller.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/configuration.rbspec/dummy/spec/system/integration_spec.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rbCHANGELOG.mdspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.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.rblib/react_on_rails/pro_utils.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/configuration.rblib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro_utils.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rblib/react_on_rails/pro_helper.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/helper.rblib/react_on_rails/pro_utils.rblib/react_on_rails/pro_helper.rblib/react_on_rails/controller.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/system/integration_spec.rbspec/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/system/integration_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_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/system/integration_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.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/pro_utils.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 (3)
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-103)lib/react_on_rails/pro_utils.rb (1)
immediate_hydration_enabled?(14-16)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/pro_utils.rb (1)
immediate_hydration_enabled?(14-16)
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-103)lib/react_on_rails/pro_utils.rb (1)
immediate_hydration_enabled?(14-16)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
🔇 Additional comments (5)
lib/react_on_rails/pro_utils.rb (1)
11-16: LGTM: simple, clear API for hydration toggleThe method accurately expresses intent and centralizes the default.
lib/react_on_rails/controller.rb (1)
12-19: LGTM: default resolution via ProUtilsThe doc and nil-default resolution are consistent with Helper/RenderOptions; behavior is uniform.
lib/react_on_rails/react_component/render_options.rb (1)
99-103: LGTM: immediate_hydration defaults via ProUtilsClean fallback to license-based default when option not provided.
lib/react_on_rails/configuration.rb (1)
65-83: Clean removal ofimmediate_hydrationconfiguration option.The removal of
immediate_hydrationfrom theattr_accessorlist andinitializeparameters is complete and consistent. The hydration behavior now delegates to Pro licensing logic viaProUtils.immediate_hydration_enabled?, which centralizes the feature gating.Note: This is a breaking change—projects with
config.immediate_hydrationin their initializers will need to remove that line.lib/react_on_rails/pro_helper.rb (1)
40-64: Pro warning UI removed in favor of centralized licensing check.The changes correctly centralize Pro feature gating through
ProUtils.disable_pro_render_options_if_not_licensed(line 41), which ensuresimmediate_hydrationis only true for licensed Pro users. The removal of warning badges and related UI simplifies the code and reduces visual clutter.Note: Users without a Pro license will no longer see warning badges when Pro features are disabled—the features will simply not activate. This is a deliberate UX change but may reduce discoverability for users evaluating Pro features.
The disable_pro_render_options_if_not_licensed method was removed from ProUtils as part of simplifying immediate_hydration. This removes the remaining calls to that method: - render_options.rb: Options now passed directly without processing - pro_helper.rb: Store data now used as-is since immediate_hydration is already set correctly by helper.rb and controller.rb immediate_hydration is now automatically set to the correct value (ProUtils.immediate_hydration_enabled?) before the data reaches these methods, so no additional processing is needed.
Code Review - PR #1997: Remove immediate_hydration configI've completed a thorough review of this PR. Here's my detailed feedback: ✅ Overall Assessment: HIGH QUALITYThis PR demonstrates excellent software engineering practices with a well-structured approach to removing configuration complexity. 🎯 Strengths
|
Updates render_options_spec.rb to handle immediate_hydration correctly: - Removes immediate_hydration from configurable_options list since it's no longer a ReactOnRails configuration option - Adds dedicated tests for immediate_hydration that verify: - Explicit true/false values work - Default behavior uses ProUtils.immediate_hydration_enabled? This fixes test failures caused by trying to set immediate_hydration on ReactOnRails.configuration which no longer exists.
Code Review: Remove immediate_hydration configThis PR simplifies the architecture by removing the ✅ Strengths1. Excellent Code Simplification
2. Proper Breaking Change Management
3. Comprehensive Test Updates
4. Documentation Updates
🔍 Areas for Improvement1. Potential Edge Case: Explicit Override BehaviorIn def immediate_hydration
options.fetch(:immediate_hydration) do
ReactOnRails::ProUtils.immediate_hydration_enabled?
end
endQuestion: Should non-Pro users be able to explicitly set
Current behavior: A non-Pro user passing 2. CHANGELOG FormattingThe CHANGELOG entry references PR 1993 instead of the current PR (1997): [PR 1993](https://github.com/shakacode/react_on_rails/pull/1993) by [AbanoubGhadban](https://github.com/AbanoubGhadban).This should be: [PR 1997](https://github.com/shakacode/react_on_rails/pull/1997) by [AbanoubGhadban](https://github.com/AbanoubGhadban).Note: Also consider adding a Breaking Changes section since this removes a public configuration option. 3. Migration Path DocumentationWhile the PR description is clear, consider adding:
4. Test Coverage: Error HandlingThe tests don't cover what happens when someone tries to set the config: config.immediate_hydration = true # This should now error since it's removed from attr_accessorConsider adding a test to verify this raises 🔒 Security Considerations✅ No security concerns identified. The change doesn't introduce any security vulnerabilities. ⚡ Performance Considerations✅ Performance improved: Removing the 🐛 Potential Bugs1. Missing Edge Case: Pro Module Not LoadedIn def self.immediate_hydration_enabled?
support_pro_features?
endThis relies on
Recommendation: Review the implementation of 2. Unused Variable in RenderOptionsIn 📚 Code Quality & Best Practices✅ Excellent commit structure: 4 focused commits make reviewing easy. 🧪 Test CoverageGood coverage, but consider adding:
📝 Summary Recommendations
✅ Approval RecommendationThis is a high-quality refactoring that significantly simplifies the codebase. The issues identified are minor and mostly documentation-related. Recommendation: ✅ Approve with minor changes (fix CHANGELOG PR number and consider the explicit override validation). Great work on simplifying the architecture! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/react_on_rails/react_component/render_options_spec.rb (1)
190-199: Consider testing both true and false cases for the ProUtils fallback.The test currently verifies that
ProUtils.immediate_hydration_enabled?is called and used when the option is not provided, but only tests the true case. Adding a test for when it returns false would provide complete coverage of the fallback logic.Add this test case after line 199:
context "without immediate_hydration option and ProUtils returns false" do it "returns false" do allow(ReactOnRails::ProUtils).to receive(:immediate_hydration_enabled?).and_return(false) attrs = the_attrs opts = described_class.new(**attrs) expect(opts.immediate_hydration).to be false end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/react_component/render_options_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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: 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.
📚 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/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/react_on_rails/react_component/render_options_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/react_component/render_options_spec.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(99-103)
⏰ 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
Code Review - PR #1997: Remove immediate_hydration configThank you for this refactoring! The goal of simplifying the ✅ What's Working Well
🎯 Key Observations1. User Control Trade-offsCurrent Behavior (Before PR):
New Behavior (After PR):
Design Question: Should non-Pro users be able to pass def immediate_hydration
options.fetch(:immediate_hydration) do
ReactOnRails::ProUtils.immediate_hydration_enabled?
end
endIf this is a Pro-only feature, you might want: def immediate_hydration
requested = options.fetch(:immediate_hydration) do
ReactOnRails::ProUtils.immediate_hydration_enabled?
end
# Only allow true if Pro is licensed
requested && ReactOnRails::ProUtils.support_pro_features?
end2. Migration PathCurrent Approach: Users will get Suggestion: Consider adding a helpful error message in def immediate_hydration=(value)
raise ReactOnRails::Error,
"config.immediate_hydration has been removed in React on Rails X.X. " \
"Immediate hydration is now automatic based on Pro license availability. " \
"To override, pass immediate_hydration: true/false to react_component() or redux_store() calls. " \
"See CHANGELOG.md for migration details."
endThis provides a clear, actionable error message instead of a confusing 📋 Suggestions for ConsiderationTest CoverageWhile existing tests were updated well, consider adding these edge cases:
CHANGELOG EnhancementThe current entry references PR #1993 (wrong PR). Update to: - **Removed immediate_hydration Configuration**: Removed the `config.immediate_hydration` option. Immediate hydration is now automatically determined based on Pro license availability - Pro users get immediate hydration by default, non-Pro users get deferred hydration. You can still override on a per-component basis by passing `immediate_hydration: true/false` to `react_component()` or `redux_store()` helpers. This change also removes Pro warning badges for a cleaner user experience. [PR 1997](https://github.com/shakacode/react_on_rails/pull/1997) by [AbanoubGhadban](https://github.com/AbanoubGhadban).Doctor.rb EnhancementConsider adding a check in if content.include?("config.immediate_hydration")
checker.add_warning(" ⚠️ config.immediate_hydration is no longer supported")
checker.add_info(" Immediate hydration now defaults based on Pro availability")
checker.add_info(" Override per-component with immediate_hydration: true/false parameter")
end🔍 Code Quality Notes
🎬 SummaryOverall Assessment: ✅ Approve with Suggestions This is a solid refactoring that simplifies the codebase and provides clearer semantics. The core implementation is correct and tests are properly updated. Recommended Actions (non-blocking):
The current implementation is safe to merge as-is, though the suggestions above would improve the user experience during migration. Great work on simplifying this feature! 🎉 Reviewed according to CLAUDE.md guidelines - React on Rails coding standards |
AbanoubGhadban
changed the base branch from
master
to
claude/remove-pro-warning-badge-011CV4Fb98f7Cdn9uPm4ACU6
Code ReviewI've reviewed PR #1997 and analyzed the changes to remove the ✅ Strengths1. Clean Architecture
2. Excellent Test Coverage
3. Good Documentation
4. Proper Cleanup
|
…rides This commit improves the immediate_hydration feature by adding proper validation and comprehensive test coverage for component-level overrides. Changes: - Add warning when non-Pro users explicitly set immediate_hydration: true - Add comprehensive unit tests for both Pro and non-Pro scenarios - Update CHANGELOG with breaking change documentation The warning provides clear feedback to users attempting to use Pro features without a license, while still allowing graceful fallback behavior. All tests pass and code is RuboCop compliant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Pull Request Review: Remove immediate_hydration configOverall AssessmentThis is a well-structured PR that successfully simplifies the codebase by removing a configuration option and making the behavior automatic based on Pro licensing. The implementation is clean, well-tested, and follows good practices. ✅ Strengths1. Excellent Code Quality
2. Comprehensive Testing
3. Well-Documented Changes
4. Good Git Hygiene
|
To align with PR #1997 which removes the immediate_hydration config and makes it automatic for Pro users, updated the README to use the correct terminology "Immediate hydration" instead of "Early hydration". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #1997Thank you for this well-structured PR! The refactoring to remove ✅ Strengths1. Excellent Code Organization
2. Strong Documentation
3. Thorough Test Coverage
4. User Experience
🔍 Issues & Concerns1. Critical: Inconsistent Behavior Between render_options.rb and pro_helper.rbIn def immediate_hydration
explicit_value = options[:immediate_hydration]
# Warn if non-Pro user explicitly sets immediate_hydration: true
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.
WARNING
end
options.fetch(:immediate_hydration) do
ReactOnRails::ProUtils.immediate_hydration_enabled?
end
endProblem: The warning message says it will "fall back to standard hydration behavior", but the code actually returns true. This means non-Pro users can bypass the Pro feature by explicitly setting it. Expected behavior (based on warning message): The method should return Suggested fix: def immediate_hydration
explicit_value = options[:immediate_hydration]
# Warn and disable if non-Pro user explicitly sets immediate_hydration: true
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.
WARNING
return false # Force disable for non-Pro users
end
options.fetch(:immediate_hydration) do
ReactOnRails::ProUtils.immediate_hydration_enabled?
end
end2. Removed Code Still Referenced in TestsThe PR removes Action needed: Verify that 3. Test Coverage GapThe new tests in Missing test: "Non-Pro user sets The current test only verifies the warning is logged and that the method returns true. But according to the warning message, it should fall back to standard behavior. There should be an integration test verifying the actual hydration behavior on the client side. 4. Potential Configuration Validation IssueSince ReactOnRails.configure do |config|
config.immediate_hydration = true # This will raise NoMethodError
endThe PR description mentions "Projects setting Suggestion: Consider adding a custom error message to guide users, rather than letting them encounter def method_missing(method, *args)
if method == :immediate_hydration= || method == :immediate_hydration
raise ReactOnRails::Error,
"config.immediate_hydration has been removed. Immediate hydration is now " \
"automatic for Pro users. See CHANGELOG.md for migration guide."
end
super
end5. Documentation: Component-Level Override ExamplesThe CHANGELOG mentions component-level overrides are supported, but doesn't show examples for all helpers. Consider adding examples for:
🎯 Minor Suggestions1. Simplify ProUtilsThe new module ReactOnRails
module ProUtils
# Checks if React on Rails Pro features are available
# @return [Boolean] true if Pro is installed and licensed, false otherwise
def self.support_pro_features?
ReactOnRails::Utils.react_on_rails_pro?
end
# Returns whether immediate hydration should be enabled
# Pro users always get immediate hydration, non-Pro users never do
# @return [Boolean] true if Pro is available, false otherwise
def self.immediate_hydration_enabled?
support_pro_features?
end
end
endNote: Looking at the diff again, I see 2. Consistency in Warning MessagesThe warning in 3. Doctor.rb Check EnhancementThe doctor check for if content.match?(/config\.immediate_hydration/)
checker.add_error("config.immediate_hydration is deprecated and removed. Remove it from your initializer.")
end🔒 Security Considerations✅ No security concerns identified. The change properly enforces Pro licensing without introducing vulnerabilities. ⚡ Performance Considerations✅ No performance regressions. In fact, this simplifies the code path by removing the 📊 Test Coverage AssessmentOverall: Very Good ✅
🎬 Recommended Actions Before MergeHigh Priority
Medium Priority
Nice to Have
📝 SummaryThis is a well-executed refactoring with excellent test coverage and documentation. The main concern is the inconsistency in Issue #1 where non-Pro users might be able to enable immediate hydration despite the warning. Once that's addressed, this will be a solid improvement to the codebase. Great work on maintaining backward compatibility for component-level overrides while simplifying the global configuration! 🚀 Review based on React on Rails conventions from CLAUDE.md |
Code Review: Remove immediate_hydration config - PR #1997SummaryThis PR successfully removes the ✅ Strengths1. Clean Architecture Simplification
2. Comprehensive Test CoverageThe test coverage is excellent, covering all critical scenarios:
3. User-Friendly Warning SystemThe warning message in
4. Excellent Documentation
|
|
/run-skipped-ci |
1 similar comment
|
/run-skipped-ci |
|
🚀 Full CI Suite Results ✅ Successfully triggered and verified all workflows Verified workflows:
These will run all CI jobs including those normally skipped on PRs:
View progress in the Actions tab. |
justin808
merged commit 6b69abd
into
claude/remove-pro-warning-badge-011CV4Fb98f7Cdn9uPm4ACU6
justin808
deleted the
claude/remove-immediate-hydration-config-011CV4Fb98f7Cdn9uPm4ACU6
branch
…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.
Summary
This PR removes the
immediate_hydrationconfiguration option and adds validation for component-level overrides. Immediate hydration is now automatically enabled for Pro users and disabled for non-Pro users, with clear warnings when non-Pro users attempt to use this Pro feature.Based on: PR removing pro_warning_badge
Changes
1. Simplify ProUtils to return hardcoded values
immediate_hydration_enabled?method that returnssupport_pro_features?PRO_ONLY_OPTIONSconstant anddisable_pro_render_options_if_not_licensedcomplexity2. Update all code to use new method
helper.rbredux_store to useProUtils.immediate_hydration_enabled?controller.rbredux_store to useProUtils.immediate_hydration_enabled?render_options.rbimmediate_hydration to useProUtils.immediate_hydration_enabled?3. Remove from ReactOnRails configuration
immediate_hydrationfrom default config hash4. Update tests and doctor
ReactOnRails::ProUtils.immediate_hydration_enabled?instead of setting configconfig.immediate_hydration(no longer valid)5. Fix linting
6. Add validation and comprehensive tests ✨ NEW
immediate_hydration: trueon componentsImpact
config.immediate_hydrationwill get an errorimmediate_hydration: trueon a component, a warning will be loggedBenefits
Testing
Validation Behavior
immediate_hydration: truefalseimmediate_hydration: falsetrueimmediate_hydration: trueimmediate_hydration: falsefalseimmediate_hydration: falsetrueimmediate_hydration: true(falls back)Commit Structure
6 focused commits for easy review:
This change is