Add Pro-only feature validation and clean up spec/dummy config #1983
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 8 minutes and 16 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 ignored due to path filters (1)
📒 Files selected for processing (14)
WalkthroughThis PR gates async loading and immediate hydration as Pro-only, defaults non‑Pro users to :defer, adds runtime validation and doctor checks for async/immediate_hydration usage, updates tests/docs/templates, removes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Config as Configuration.setup_config_values
participant Pro as ReactOnRailsPro check
participant Validator as validate_pro_only_features / validate_generated_component_packs_loading_strategy
participant Doctor as ReactOnRails::Doctor.check_async_usage
participant Env as Environment (prod/dev)
Config->>Pro: detect ReactOnRailsPro presence
Pro-->>Validator: indicate Pro availability
Validator->>Config: set loading strategy (:async / :defer / :sync) or raise
Doctor->>Config: scan initializer for :async
Doctor->>Doctor: scan view files for async pack tags
alt Pro not available and async detected
Doctor->>Env: if production -> log error; else -> raise error
else immediate_hydration enabled without Pro
Doctor->>Env: warn about race risk, suggest Pro or change
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review: PR #1983 - Verify shared store tests pass after revertSummaryThis PR validates that the Redux shared store integration tests pass correctly after PR #1981 reverted the defer loading strategy changes from PR #1972. The changes include dependency updates and removal of some performance metrics from documentation. ✅ Positive Aspects
📋 Code Quality & Best PracticesGemfile.lock Dependency Updates# Updated dependencies:
- capybara: 3.39.2 → 3.40.0
- matrix: 0.4.2 → 0.4.3
- rack: 3.2.1 → 3.2.4
- regexp_parser: 2.8.1 → 2.11.3Analysis:
Documentation ChangesThe removal of Airbnb and Meta RSC metrics from Concerns:
Source verification check: 🔒 Security Considerations✅ Good: Updated dependencies include security patches:
🧪 Test Coverage✅ Excellent: All 6 shared store integration tests explicitly verified:
Verification command used: bundle exec rspec spec/system/integration_spec.rb -e "shared_store"Recommendation: Consider adding this test command to the PR description or running it in CI to auto-verify. ⚡ Performance ConsiderationsImpact: Neutral
🐛 Potential Issues
📚 Adherence to CLAUDE.md Guidelines✅ Met requirements:
🎯 Recommendations
✅ Overall AssessmentApproval Status: ✅ Approve with minor recommendations This is a straightforward verification PR that confirms the revert in #1981 didn't break shared store functionality. The dependency updates appear safe, though the documentation changes could use more context. Key strengths:
Suggested improvements:
Reviewed by Claude Code 🤖 |
Code ReviewSummaryThis PR verifies that Redux shared store tests pass after PR #1981 reverted PR #1972. The changes include a Gemfile.lock update (dependency bumps) and removal of unverified performance claims from documentation. 📋 Code Quality & Best Practices✅ GOOD:
🐛 Potential IssuesMINOR:
NO CRITICAL ISSUES DETECTED 🔒 Security Considerations✅ NO SECURITY CONCERNS:
⚡ Performance Considerations✅ NO PERFORMANCE IMPACT:
🧪 Test Coverage✅ TESTING APPROACH IS SOLID:
RECOMMENDATION: Wait for CI to complete before merging to ensure Gemfile.lock changes don't cause unexpected issues. 💡 Suggestions for Improvement
✅ RecommendationAPPROVE with minor suggestions - The changes are safe and appropriate. The documentation cleanup is valuable. However, clarify the relationship between the Gemfile.lock updates and the stated purpose of verifying shared store tests. Wait for CI to complete before merging to ensure no unexpected issues from dependency updates. 📚 Context UnderstandingThis PR is part of a larger story:
The revert suggests the fix in #1972 may have had unintended consequences, and the original issue has been resolved through other means (or wasn't reproducible). This verification PR confirms no regression occurred. Review conducted following repository conventions from CLAUDE.md |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONDUCTOR_SETUP.md (1)
38-41: Add language specifiers to code blocks for better rendering.The code blocks at lines 38-41, 64-66, and 119-121 should have language specifiers for proper syntax highlighting and rendering. Consider adding appropriate identifiers:
Apply these changes:
-``` +```text OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 state=error: certificate verify failed (unable to get certificate CRL)```diff -``` +```text NameError: uninitialized constant Rack::Handler```diff -``` +```text spec/dummy/tmp/capybara/failures_*.pngBased on coding guidelines. Also applies to: 64-66, 119-121 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4fd1b9b921d23ce387869718f0014426164483df and 257214dfc208933b3b0aa881a1f750b5afec2404. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `.gitignore` (1 hunks) * `CONDUCTOR_SETUP.md` (1 hunks) * `PR_1972_INVESTIGATION.md` (1 hunks) * `spec/dummy/TESTING_LOCALLY.md` (1 hunks) * `spec/dummy/config/initializers/react_on_rails.rb` (1 hunks) * `spec/dummy/spec/rails_helper.local.rb.example` (1 hunks) * `spec/dummy/spec/rails_helper.rb` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * .gitignore </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (10)</summary> <details> <summary>📓 Common learnings</summary>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 underapp-react16directories are copied/moved to corresponding/appdirectories during the conversion process (removing the-react16suffix), which affects their relative import paths at runtime.Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.</details> <details> <summary>📚 Learning: 2025-02-13T14:29:49.267Z</summary>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/TESTING_LOCALLY.md` - `spec/dummy/spec/rails_helper.local.rb.example` - `spec/dummy/spec/rails_helper.rb` </details> <details> <summary>📚 Learning: 2025-09-15T21:24:48.207Z</summary>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 explicitdata-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/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2025-09-16T08:01:11.146Z</summary>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 syntaximport * 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/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2025-04-26T21:55:55.874Z</summary>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 underapp-react16directories are copied/moved to corresponding/appdirectories during the conversion process (removing the-react16suffix), which affects their relative import paths at runtime.**Applied to files:** - `spec/dummy/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2024-07-27T10:08:35.868Z</summary>Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: TheRailsContextimport inspec/dummy/client/app/startup/HelloTurboStream.jsxis used later in the project, as clarified by the user theforestvn88.**Applied to files:** - `spec/dummy/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2025-02-12T16:38:06.537Z</summary>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 filenode_package/lib/ReactOnRails.full.jsis autogenerated during the build process and should not be present in the repository.**Applied to files:** - `spec/dummy/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2025-02-18T13:08:01.477Z</summary>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:
- Pro version check in
run_stream_inside_fiber- RSC support check during pack generation via
ReactOnRailsPro.configuration.enable_rsc_support- RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.**Applied to files:** - `spec/dummy/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2025-10-23T17:22:01.074Z</summary>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. Thereact_on_rails_pro?method validates licenses and should raise errors early (including during path resolution in methods likeserver_bundle?) to enforce licensing requirements rather than failing later with obscure errors.**Applied to files:** - `spec/dummy/config/initializers/react_on_rails.rb` </details> <details> <summary>📚 Learning: 2024-06-27T05:22:46.156Z</summary>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, usebeforeandafterhooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808: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 endThis practice ensures test isolation and reliability.
**Applied to files:** - `spec/dummy/spec/rails_helper.local.rb.example` - `spec/dummy/spec/rails_helper.rb` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>CONDUCTOR_SETUP.md</summary> 38-38: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 64-64: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 119-119: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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). (3)</summary> * GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest) * GitHub Check: markdown-link-check * GitHub Check: claude-review </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>spec/dummy/config/initializers/react_on_rails.rb (1)</summary><blockquote> `46-50`: **LGTM! Clean environment-controlled configuration.** The dynamic loading strategy configuration is well-implemented. Using an environment variable to toggle between `:defer` (default) and `:async` (for testing) allows testing both strategies without code changes, which directly supports the PR objectives of verifying shared store tests after the revert. </blockquote></details> <details> <summary>PR_1972_INVESTIGATION.md (1)</summary><blockquote> `1-324`: **Excellent investigation documentation.** This comprehensive analysis clearly documents the race condition between generated component packs and the main client bundle, explains why PR #1972's solution was problematic (forcing defer everywhere impacts performance and masks the root cause), and proposes better alternatives. The document serves as valuable context for understanding the testing strategy implemented in this PR. </blockquote></details> <details> <summary>spec/dummy/spec/rails_helper.rb (1)</summary><blockquote> `18-21`: **LGTM! Clean local configuration pattern.** The conditional loading of `rails_helper.local.rb` provides a safe mechanism for environment-specific test configuration without affecting other developers or CI. The `File.exist?` guard ensures no errors when the file is absent, and the comments clearly document the purpose and gitignore status. </blockquote></details> <details> <summary>spec/dummy/TESTING_LOCALLY.md (1)</summary><blockquote> `3-47`: **Excellent testing documentation.** The new section clearly explains how to reproduce CI failures related to the component registration race condition, documents the environment variable approach for testing both `:defer` and `:async` strategies, and provides practical commands for debugging intermittent failures. This directly supports the PR objectives and will help developers understand the testing approach. </blockquote></details> <details> <summary>spec/dummy/spec/rails_helper.local.rb.example (1)</summary><blockquote> `1-25`: **LGTM! Security warnings are appropriate.** This example file provides a helpful template for local test configuration. The SSL verification disabling is properly safeguarded: - Commented out by default - Includes explicit security warnings - Documented as only for isolated test environments - File is gitignored and won't be committed The template serves its purpose well for developers encountering SSL issues in Conductor workspaces or similar isolated environments. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
justin808
changed the title
Code Review - PR #1983: Add Pro-only feature validation and clean up spec/dummy configI've completed a thorough review of this PR. Overall, this is excellent work with strong implementation quality and comprehensive test coverage. Here's my detailed feedback: ✅ Strengths1. Well-Architected Pro Feature ValidationThe validate_pro_only_features method in lib/react_on_rails/configuration.rb:150-180 is well-designed:
2. Comprehensive Test CoverageThe test suite in spec/react_on_rails/configuration_spec.rb is exemplary:
3. Config CleanupRemoving Pro-only settings from spec/dummy/config/initializers/react_on_rails.rb is correct - these settings were unnecessary for the open-source dummy app and could confuse contributors. 4. CHANGELOG Follows GuidelinesThe CHANGELOG entry is properly formatted per CLAUDE.md requirements:
🔍 Code Quality AnalysisConfiguration Logic (lib/react_on_rails/configuration.rb:213-218)Analysis: This logic is sound. It ensures:
Error Message Quality (configuration.rb:164-175)The error message is excellent:
💡 Minor Suggestions1. TypeScript Linting FixThe change in packages/react-on-rails/src/context.ts:4 appears to be just an ESLint rule ordering fix. This is good, but ensure you run yarn run lint to verify all linting passes. 2. Documentation Files ScopeThe PR includes two substantial new documentation files:
Question: Are these intentionally part of this PR? They seem unrelated to Pro feature validation:
Recommendation: If these are accidentally included, consider moving them to a separate PR. If they're intentionally included, the PR description should mention them in the summary. 3. Consider Adding Integration TestWhile unit tests are comprehensive, consider adding a system/integration test that actually tries to configure Pro features without the Pro gem and verifies the error is raised. This would catch any regression in the validation logic. 🔒 Security ConsiderationsSSL Verification Disabling (spec/dummy/spec/rails_helper.local.rb.example:17)Status: ✅ Safe
This is the correct pattern for environment-specific test workarounds. 🚀 Performance ConsiderationsNo performance concerns. The validation runs once at configuration time, adding negligible overhead. 🐛 Potential IssuesNone FoundI reviewed the code carefully and found no bugs or logic errors. The implementation is solid. 📋 Pre-Merge ChecklistBefore merging, please verify:
📊 Summary
Overall Recommendation: ✅ Approve with minor clarifications The core feature implementation is excellent and ready to merge. Just clarify the documentation files and ensure linting passes. Great work on this PR! The Pro feature validation is exactly what's needed to maintain clear boundaries between open-source and Pro functionality. |
Code Review - PR #1983SummaryThis PR adds Pro-only feature validation to prevent non-Pro users from using immediate_hydration and generated_component_packs_loading_strategy features. The implementation is solid overall, with good test coverage and clear error messaging. Strengths1. Clear Separation of Concerns - The Pro-only feature validation is well-encapsulated in validate_pro_only_features. 2. Excellent Error Messages - Actionable and user-friendly with clear identification of problematic settings and links to documentation. 3. Production Safety - Graceful degradation in production (logging vs raising) prevents crashes. 4. Comprehensive Test Coverage - Tests cover both features individually/together, production vs non-production, with/without ReactOnRailsPro. 5. Good Documentation - Clear CHANGELOG entry and helpful spec/dummy config comments. Issues and Concerns1. CRITICAL: Unintended Files CommittedCONDUCTOR_SETUP.md and PR_1972_INVESTIGATION.md appear to be development/investigation notes that should NOT be in version control. Remove these files from this PR. Context.ts ESLint comment change (line 4) removed no-var from eslint-disable. This seems unrelated to Pro-only feature validation. Either revert or explain this change. 2. Logic Issue: Auto-Setting for Non-Pro UsersIn configuration.rb:215-218, there is an early return for non-Pro users, BUT auto-setting on line 221 happens AFTER validation (line 145). This means non-Pro users could get :async auto-set without triggering validation! Fix: Add conditional to line 221: 3. Test Gap: Auto-Setting BehaviorTests do not verify what happens when non-Pro user does not set generated_component_packs_loading_strategy with Shakapacker >= 8.2.0. Add test to verify non-Pro users do not get auto-setting. 4. spec/dummy Configuration Inconsistencyspec/dummy config sets Document or fix this inconsistency. Recommendations PriorityMust Fix Before Merge:
Should Fix:
Nice to Have:
Overall AssessmentStatus: Needs Changes The core feature is solid with good error handling and test coverage. However, logic issues with auto-setting and unrelated files need to be addressed before merging. Estimated effort to fix: 30-60 minutes Great work on the clear error messaging and comprehensive testing! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)lib/react_on_rails/configuration.rb(2 hunks)packages/react-on-rails/src/context.ts(1 hunks)spec/dummy/config/initializers/react_on_rails.rb(1 hunks)spec/react_on_rails/configuration_spec.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/dummy/config/initializers/react_on_rails.rb
🧰 Additional context used
🧠 Learnings (16)
📓 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: 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: 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 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-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.rbspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 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/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 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.rbpackages/react-on-rails/src/context.tsspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 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.rbspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 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/configuration.rbpackages/react-on-rails/src/context.tsspec/react_on_rails/configuration_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:
lib/react_on_rails/configuration.rbpackages/react-on-rails/src/context.tsspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 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.rbpackages/react-on-rails/src/context.tsspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
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:
packages/react-on-rails/src/context.tsspec/react_on_rails/configuration_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:
packages/react-on-rails/src/context.tsspec/react_on_rails/configuration_spec.rbCHANGELOG.md
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails/src/context.tsspec/react_on_rails/configuration_spec.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:
packages/react-on-rails/src/context.tsCHANGELOG.md
📚 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:
packages/react-on-rails/src/context.tsCHANGELOG.md
📚 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/react_on_rails/configuration_spec.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/react_on_rails/configuration_spec.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:
CHANGELOG.md
🧬 Code graph analysis (2)
lib/react_on_rails/configuration.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)
spec/react_on_rails/configuration_spec.rb (3)
lib/react_on_rails/configuration.rb (1)
configure(6-9)react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)
configure(4-7)lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)
⏰ 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). (7)
- GitHub Check: build-dummy-app-webpack-test-bundles (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 (1)
packages/react-on-rails/src/context.ts (1)
4-7: LGTM—ESLint directive cleanup is appropriate.Removing
no-varfrom the disable directives is correct, as TypeScript ambient declarations indeclare globalblocks usevarsyntax by convention, and ESLint should not enforce theno-varrule in this context. The remaining directives (vars-on-top,no-underscore-dangle) are still needed for lines 5–6.This cleanup doesn't affect runtime behavior and is unrelated to the PR's main objective of Pro-only feature validation, but is acceptable as general code hygiene.
| def validate_pro_only_features | ||
| return if defined?(ReactOnRailsPro) | ||
|
|
||
| pro_only_features = [] | ||
|
|
||
| pro_only_features << "config.immediate_hydration = true" if immediate_hydration == true | ||
|
|
||
| if generated_component_packs_loading_strategy.present? | ||
| pro_only_features << "config.generated_component_packs_loading_strategy = " \ | ||
| ":#{generated_component_packs_loading_strategy}" | ||
| end | ||
|
|
||
| return if pro_only_features.empty? | ||
|
|
||
| msg = <<~MSG | ||
| **ERROR** ReactOnRails: You are using Pro-only features without React on Rails Pro: | ||
| #{pro_only_features.map { |f| " - #{f}" }.join("\n")} | ||
| These features are only available with a React on Rails Pro license. | ||
| Please either: | ||
| 1. Remove these settings from your config/initializers/react_on_rails.rb | ||
| 2. Purchase a React on Rails Pro license at https://www.shakacode.com/react-on-rails-pro | ||
| For more information, see: https://www.shakacode.com/react-on-rails/docs/ | ||
| MSG | ||
|
|
||
| return Rails.logger.error(msg) if Rails.env.production? | ||
|
|
||
| raise ReactOnRails::Error, msg | ||
| end |
There was a problem hiding this comment.
Reset Pro-only settings after logging to prevent bypass.
In production we currently log the warning but we leave immediate_hydration and generated_component_packs_loading_strategy untouched. Core helpers still consume those fields to emit immediate-hydration hooks and async/defer script attributes, so a non-Pro install can ignore the log and keep the gated behavior. To actually “prevent non-Pro users from using Pro-only features,” we should reset these knobs to safe defaults before returning in the production branch so the features are neutralized unless ReactOnRailsPro is present.
Apply this diff to clear the settings when we log:
- return Rails.logger.error(msg) if Rails.env.production?
-
- raise ReactOnRails::Error, msg
+ if Rails.env.production?
+ self.immediate_hydration = false if immediate_hydration == true
+ self.generated_component_packs_loading_strategy = nil if generated_component_packs_loading_strategy.present?
+ Rails.logger.error(msg)
+ return
+ end
+
+ raise ReactOnRails::Error, msg🤖 Prompt for AI Agents
In lib/react_on_rails/configuration.rb around lines 150 to 180, the production
branch only logs Pro-only usage but does not neutralize the Pro-only settings;
update the production path so that after logging you reset the affected config
knobs to safe defaults (set immediate_hydration = false and
generated_component_packs_loading_strategy = nil) before returning, ensuring
non-Pro installs cannot keep using immediate hydration or altered pack loading
behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/react_on_rails/configuration.rb (1)
150-181: Reset Pro-only settings in production to actually enforce the restriction.Currently in production, the code only logs the error but leaves
immediate_hydration = trueandgenerated_component_packs_loading_strategy = :asyncuntouched. This means non-Pro users can ignore the log and continue using Pro-only features in production, defeating the purpose of the validation.Apply this diff to neutralize the settings after logging:
- return Rails.logger.error(msg) if Rails.env.production? - - raise ReactOnRails::Error, msg + if Rails.env.production? + # Reset Pro-only settings to safe defaults in production + self.immediate_hydration = false if immediate_hydration == true + self.generated_component_packs_loading_strategy = :defer if generated_component_packs_loading_strategy == :async + Rails.logger.error(msg) + return + end + + raise ReactOnRails::Error, msgThis ensures the Pro-only features are actually disabled in production for non-Pro users, not just logged.
🧹 Nitpick comments (1)
docs/upgrading/release-notes/16.0.0.md (1)
49-53: Fix compound adjective hyphenation in Line 50.On Line 50, "full page load" used as a compound adjective should be hyphenated as "full-page load" for proper grammar per EN_COMPOUND_ADJECTIVE_INTERNAL rule.
Apply this diff to fix the compound adjective:
- - `:defer` - Defers script execution until after page load (default for non-Pro, doesn't work well with Streamed HTML as it will wait for the full page load before hydrating the components) + - `:defer` - Defers script execution until after page load (default for non-Pro, doesn't work well with Streamed HTML as it will wait for the full-page load before hydrating the components)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)docs/api-reference/configuration.md(1 hunks)docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md(1 hunks)docs/upgrading/release-notes/16.0.0.md(2 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/react_on_rails/configuration.rb(2 hunks)lib/react_on_rails/doctor.rb(2 hunks)spec/dummy/TESTING_LOCALLY.md(0 hunks)spec/dummy/app/controllers/application_controller.rb(0 hunks)spec/dummy/app/views/layouts/application.html.erb(1 hunks)spec/dummy/config/initializers/react_on_rails.rb(1 hunks)spec/dummy/spec/rails_helper.local.rb(1 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)spec/react_on_rails/configuration_spec.rb(4 hunks)
💤 Files with no reviewable changes (2)
- spec/dummy/app/controllers/application_controller.rb
- spec/dummy/TESTING_LOCALLY.md
✅ Files skipped from review due to trivial changes (1)
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
🧰 Additional context used
🧠 Learnings (16)
📓 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: 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: 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 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-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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdlib/react_on_rails/configuration.rbspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rbspec/dummy/app/views/layouts/application.html.erbdocs/upgrading/release-notes/16.0.0.md
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/configuration.rbspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rbspec/dummy/app/views/layouts/application.html.erblib/react_on_rails/doctor.rbdocs/upgrading/release-notes/16.0.0.md
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rbspec/dummy/app/views/layouts/application.html.erbdocs/upgrading/release-notes/16.0.0.md
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/lib/react_on_rails/doctor_spec.rbspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/dummy/app/views/layouts/application.html.erbdocs/upgrading/release-notes/16.0.0.md
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/dummy/app/views/layouts/application.html.erb
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/configuration.rbspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rblib/react_on_rails/doctor.rbdocs/upgrading/release-notes/16.0.0.md
📚 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:
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.mddocs/api-reference/configuration.mdspec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/configuration.rbspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rblib/react_on_rails/doctor.rbdocs/upgrading/release-notes/16.0.0.md
📚 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/rails_helper.local.rbspec/lib/react_on_rails/doctor_spec.rbspec/react_on_rails/configuration_spec.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/rails_helper.local.rbspec/react_on_rails/configuration_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:
docs/api-reference/configuration.mdspec/dummy/config/initializers/react_on_rails.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rblib/react_on_rails/doctor.rbdocs/upgrading/release-notes/16.0.0.md
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
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/lib/react_on_rails/doctor_spec.rbspec/dummy/config/initializers/react_on_rails.rbspec/react_on_rails/configuration_spec.rbspec/dummy/app/views/layouts/application.html.erb
📚 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/lib/react_on_rails/doctor_spec.rblib/react_on_rails/configuration.rbCHANGELOG.mdspec/react_on_rails/configuration_spec.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/configuration.rbspec/react_on_rails/configuration_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/configuration.rbspec/react_on_rails/configuration_spec.rbspec/dummy/app/views/layouts/application.html.erblib/react_on_rails/doctor.rbdocs/upgrading/release-notes/16.0.0.md
📚 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:
CHANGELOG.md
🧬 Code graph analysis (3)
lib/react_on_rails/configuration.rb (2)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)lib/react_on_rails/packer_utils.rb (1)
supports_async_loading?(35-37)
spec/react_on_rails/configuration_spec.rb (3)
lib/react_on_rails/configuration.rb (2)
configure(6-9)configuration(14-54)react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (2)
configure(4-7)configuration(9-37)lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)
lib/react_on_rails/doctor.rb (3)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(216-224)lib/react_on_rails/system_checker.rb (3)
add_warning(20-22)add_info(28-30)add_error(16-18)lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)
🪛 LanguageTool
docs/upgrading/release-notes/16.0.0.md
[uncategorized] ~50-~50: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
🔇 Additional comments (13)
spec/dummy/app/views/layouts/application.html.erb (1)
12-14: LGTM! Simplification to always use defer loading.This change correctly:
- Removes the conditional logic that selected between defer and async based on Redux store usage
- Defaults to the safe
defer: trueoption for all scenarios- Documents that async loading requires React on Rails Pro
This aligns with the PR's goal of ensuring spec/dummy tests use non-Pro features and preventing race conditions.
docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md (1)
50-78: Excellent documentation for the loading strategy configuration.This new section clearly explains:
- The three loading options and their trade-offs
- Pro requirements for
:async- Default behavior differences between Community and Pro editions
- Shakapacker version requirements
The warning about race conditions with
:asyncwithout Pro is particularly important for user safety.spec/dummy/config/initializers/react_on_rails.rb (1)
45-48: Clear documentation and appropriate configuration for test environment.The changes correctly:
- Set
generated_component_packs_loading_strategyto:deferto prevent race conditions in tests- Document that
:asyncrequires React on Rails Pro- Clarify that
:deferand:syncare available to all usersThis ensures the spec/dummy app uses only non-Pro features as intended.
spec/react_on_rails/configuration_spec.rb (1)
470-595: Comprehensive test coverage for Pro-only feature validation.These tests thoroughly verify:
- Pro vs non-Pro behavior for
immediate_hydrationandgenerated_component_packs_loading_strategy- Error raising in non-production environments
- Logging without raising in production
- Proper error message aggregation when multiple Pro features are used
- Safe defaults for non-Pro users
The use of
stub_constandhide_constfor controllingReactOnRailsPropresence is appropriate.Based on learnings
spec/lib/react_on_rails/doctor_spec.rb (1)
192-388: Thorough test coverage for Doctor's async usage detection.These tests verify:
- Pro gem presence skips the check (correct behavior)
- Detection of
:asyncin view files and configuration- Different messaging when
immediate_hydrationis enabled- Proper warnings vs errors
The tests correctly mock filesystem interactions and ReactOnRails configuration.
lib/react_on_rails/doctor.rb (1)
1148-1235: Well-structured async usage detection with helpful diagnostics.The implementation correctly:
- Skips checking when Pro is installed (line 1153)
- Detects async usage in both view files and configuration
- Provides different messaging based on
immediate_hydrationstatus- Offers actionable remediation steps (upgrade to Pro or change strategy)
The separation into focused helper methods improves maintainability.
lib/react_on_rails/configuration.rb (1)
214-228: Smart auto-configuration defaults for Pro vs non-Pro users.The logic correctly:
- Sets
:asyncas default for Pro users with modern Shakapacker (line 217-218)- Sets
:deferas safe default for non-Pro users with modern Shakapacker (line 219-221)- Falls back to
:syncwith a warning for older Shakapacker versions (line 222-225)- Validates that
:asyncrequires Shakapacker >= 8.2.0 (line 226-228)This provides optimal defaults while preventing unsafe configurations.
docs/api-reference/configuration.md (2)
269-280: Clear documentation of loading strategy options with proper Pro gating.The expanded documentation effectively distinguishes between non-Pro defaults (
:defer) and Pro capabilities (:async), with helpful context about race conditions and Shakapacker requirements. The default assignment in Line 280 properly reflects the non-Pro safe default.
288-313: Well-structured Pro features section with clear configuration guidance.The new section clearly separates Pro-only features from core configuration, properly directs users to
ReactOnRailsPro.configureblock, and provides a helpful example. This is an excellent addition for preventing misconfiguration.docs/upgrading/release-notes/16.0.0.md (2)
60-70: Comprehensive documentation of Pro feature defaults and streaming behavior.The documentation clearly delineates Pro vs non-Pro defaults for
generated_component_packs_loading_strategyand properly explains howimmediate_hydration(Pro-only) works with async loading, particularly in streaming contexts. The Redux store support documentation is also helpful.
87-91: Clear migration guidance with Pro optimization path.The migration instructions properly map old
defer_generated_component_packssettings to the newgenerated_component_packs_loading_strategyoptions, and Line 91 effectively communicates the optimal Pro configuration for users with appropriate Shakapacker versions.CHANGELOG.md (2)
40-45: Excellent documentation of Pro-only feature validation with clear behavior differentiation.The CHANGELOG entry clearly documents the Pro-only feature validation, properly distinguishes between development/test error handling (raises with actionable message) and production graceful degradation (logs without crashing), and references the PR. This level of detail helps users understand the migration path and expected behavior.
47-52: Shakapacker upgrade documentation is clear and well-structured.The documentation of the Shakapacker 9.0.0 upgrade changes is comprehensive, explaining the Babel transpiler configuration, CSS Modules settings, and webpack fixes with proper technical detail.
| # Disable webdrivers auto-update to avoid SSL issues in Conductor environment | ||
| require "webdrivers" | ||
| Webdrivers.cache_time = 86_400 * 365 # Cache for 1 year (effectively disable updates) |
There was a problem hiding this comment.
Consider the implications of disabling webdrivers updates.
Caching webdrivers for 1 year prevents automatic security and compatibility updates. While this may solve immediate SSL issues, it means:
- Security vulnerabilities in browser drivers won't be patched
- Browser version updates may break tests due to driver incompatibility
Consider a shorter cache period (e.g., 7-30 days) or use fixed driver versions with explicit updates:
Webdrivers.cache_time = 86_400 * 7 # 1 week🤖 Prompt for AI Agents
In spec/dummy/spec/rails_helper.local.rb around lines 6 to 8, caching Webdrivers
for one year disables automatic driver updates and risks security and
compatibility issues; change to a shorter cache_time (e.g., 7–30 days) or switch
to pinning a fixed driver version and explicitly update it as part of CI; update
the Webdrivers.cache_time value to a shorter duration (for example 86_400 * 7)
or replace the long cache with logic that uses a fixed driver version (and
document/update process for bumping it).
| # Disable SSL verification globally for tests (Conductor environment workaround) | ||
| # WARNING: This is a security risk and should ONLY be used in isolated test environments | ||
| require "openssl" | ||
| OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE |
There was a problem hiding this comment.
Critical security issue: Global SSL verification bypass is dangerous and may not work as intended.
Setting OpenSSL::SSL::VERIFY_PEER to VERIFY_NONE is problematic:
- Security risk: This disables SSL certificate validation for the entire Ruby process, not just tests
- Implementation issue:
VERIFY_PEERis a constant integer; reassigning it doesn't change OpenSSL's behavior in most Ruby versions - Scope pollution: This affects all SSL connections system-wide during the test run
Consider these safer alternatives:
Option 1 (Recommended): Use WebMock to stub external requests
require "webmock/rspec"
WebMock.disable_net_connect!(allow_localhost: true)
# Stub specific external endpoints as neededOption 2: Configure specific HTTP clients
# For Faraday
Faraday.default_connection_options.ssl.verify = false
# For Net::HTTP
http = Net::HTTP.new(uri.host, uri.port)
http.use_ssl = true
http.verify_mode = OpenSSL::SSL::VERIFY_NONEOption 3: Fix the Conductor environment's SSL certificates (best long-term solution)
Contact your infrastructure team to provide valid CA certificates in the test environment.
🤖 Prompt for AI Agents
In spec/dummy/spec/rails_helper.local.rb around lines 10 to 13, the file
currently disables SSL verification globally by reassigning
OpenSSL::SSL::VERIFY_PEER which is insecure, ineffective in many Ruby versions,
and pollutes process-wide state; replace this with a scoped, safer approach:
remove the global reassignment and either (1) add WebMock (require
"webmock/rspec" and call WebMock.disable_net_connect!(allow_localhost: true))
and stub external endpoints used in tests, or (2) if tests use a specific HTTP
client, configure that client’s SSL verify flag locally (e.g., set Faraday or
Net::HTTP verify options for only the connections in tests), or (3) better yet,
fix the test environment’s CA certificates so real verification passes;
implement one of these alternatives and remove the OpenSSL::SSL::VERIFY_PEER
override.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review: PR #1983 - Add Pro-only feature validation and clean up spec/dummy configOverviewThis PR adds runtime validation to prevent non-Pro users from using Pro-only features ( 🔴 Critical IssueProduction Enforcement Not Working as IntendedLocation: Problem: In production, the code only logs the error but does not disable the Pro-only features. This means non-Pro users can ignore the log and continue using Pro features in production, defeating the purpose of the validation. # Current code (lines 178-180)
return Rails.logger.error(msg) if Rails.env.production?
raise ReactOnRails::Error, msgThe settings Recommended Fix: if Rails.env.production?
# Reset Pro-only settings to safe defaults
self.immediate_hydration = false if immediate_hydration == true
self.generated_component_packs_loading_strategy = :defer if generated_component_packs_loading_strategy == :async
Rails.logger.error(msg)
return
end
raise ReactOnRails::Error, msgThis ensures Pro features are actually disabled in production for non-Pro users, not just logged. ✅ Strengths1. Excellent Error MessagesThe error messages in
2. Comprehensive Test CoverageThe test suite in
3. Smart Default ConfigurationThe auto-configuration logic in
4. Doctor IntegrationThe new
5. Documentation Quality
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Clean separation of concerns, environment-aware behavior |
| Test Coverage | ⭐⭐⭐⭐⭐ | Comprehensive with 9 new test cases, all passing |
| Error Messages | ⭐⭐⭐⭐⭐ | Clear, actionable, user-friendly |
| Documentation | ⭐⭐⭐⭐ | Good overall; question about extra docs |
| Production Safety | ⭐⭐⭐ | Needs fix to actually disable features (not just log) |
| Best Practices | ⭐⭐⭐⭐⭐ | Follows project conventions perfectly |
🔒 Security Considerations
✅ No security concerns with the Pro validation logic itself
📝 Pre-Merge Checklist
Before merging, please address:
- CRITICAL: Fix production enforcement to actually disable Pro features (not just log)
- Run
bundle exec rubocopand ensure 0 offenses (per CLAUDE.md requirements) - Run
yarn run lintto verify TypeScript changes - Verify all files end with newline characters (per CLAUDE.md requirements)
- Clarify whether
PR_1972_INVESTIGATION.mdand Conductor docs should be included - Consider safer alternative to SSL verification bypass in test helper
- Explain or revert
context.tsESLint comment change
💡 Recommendations
Must Fix Before Merge:
- Production enforcement - Actually disable Pro features, don't just log (see Critical Issue above)
Should Fix:
- SSL verification bypass - Use safer testing approach
- Unrelated documentation files - Clarify intent or move to separate PR
- Context.ts change - Explain or revert
Nice to Have:
- Consider adding integration test that actually tries to use Pro features without Pro gem and verifies they're disabled
✨ Summary
Recommendation: ✅ Approve with changes required
The core Pro feature validation implementation is excellent with strong test coverage and clear error messaging. However, the production enforcement logic needs to be fixed to actually disable Pro features rather than just logging them. Once the critical production enforcement issue is addressed, this PR will be ready to merge.
Great work on maintaining clear boundaries between open-source and Pro functionality! The error messages and documentation are particularly well done.
| private | ||
|
|
||
| def validate_pro_only_features | ||
| return if defined?(ReactOnRailsPro) |
There was a problem hiding this comment.
@claude should it use the already defined ReactOnRails::UItils.react_on_rails_pro? function?
Gemfile.lock dependencies updated to latest compatible versions: - capybara 3.40.0 (nokogiri ~> 1.11) - matrix 0.4.3 - rack 3.2.4 - regexp_parser 2.11.3 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add rails_helper.local.rb pattern for environment-specific settings - Create example template for Conductor SSL workarounds - Update .gitignore to exclude local config files - Add CONDUCTOR_SETUP.md with comprehensive setup guide This allows developers to easily configure SSL verification bypass and other Conductor-specific settings without committing them to git. The pattern is reusable for any environment-specific test configuration.
Makes it easy to reproduce CI failures locally by testing with different script loading strategies (async vs defer) without code changes. Changes: - spec/dummy/config/initializers/react_on_rails.rb: Use REACT_ON_RAILS_LOADING_STRATEGY env var - spec/dummy/TESTING_LOCALLY.md: Add section on reproducing race condition failures Usage: # Test with defer (default, prevents race conditions) bundle exec rspec spec/system/integration_spec.rb -e "shared_store" # Test with async (library default for users, may show race conditions) env REACT_ON_RAILS_LOADING_STRATEGY=async bundle exec rspec spec/system/integration_spec.rb -e "shared_store" This eliminates the need for .example files and provides a simple, documented way to test both loading strategies locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…_component_packs_loading_strategy Prevents non-Pro users from using these Pro-only features while maintaining backward compatibility for Pro users. Changes: - Add validate_pro_only_features method to detect Pro-only settings in non-Pro - In production: logs error without crashing - In non-production: raises ReactOnRails::Error with helpful message - Update validate_generated_component_packs_loading_strategy to skip auto-setting for non-Pro - Remove Pro-only settings from spec/dummy initializer - Add comprehensive unit tests for Pro-only validation - Update existing tests to stub ReactOnRailsPro for Pro feature tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…pec/dummy for tests The spec/dummy tests expect :defer as the default loading strategy to prevent race conditions during testing. Since ReactOnRailsPro is stubbed in tests, the configuration would default to :async, but tests expect :defer. This explicitly sets it to :defer for testing purposes while maintaining the Pro-only validation for production use.
Per code review feedback: - Removed spec/dummy/TESTING_LOCALLY.md (mentions async which is Pro-only) - Removed spec/dummy/spec/rails_helper.local.rb.example - Reverted spec/dummy/spec/rails_helper.rb to remove local config loading These files will be moved to a separate PR focused on local testing setup.
- Removed CONDUCTOR_SETUP.md (separate PR) - Reverted .gitignore to remove rails_helper.local.rb entry (separate PR) These Conductor workspace setup files will be in a separate PR.
Make it crystal clear that generated_component_packs_loading_strategy is a Pro-only feature and should NOT be used without a license. The setting is only present in spec/dummy because tests stub the ReactOnRailsPro constant.
… feature Per code review feedback: 1. generated_component_packs_loading_strategy itself is NOT Pro-only - :defer and :sync are available to all users - Only :async requires React on Rails Pro - Non-Pro users now default to :defer (safe choice) 2. Updated validation to only error on :async for non-Pro users 3. Updated CHANGELOG to clarify: - immediate_hydration should be in react_on_rails_pro.rb initializer - Only :async loading strategy requires Pro - :defer and :sync are available to all users 4. Updated tests to verify :defer and :sync work for non-Pro 5. Updated spec/dummy comments to reflect correct Pro requirements
CI requires no-var to be disabled for var declarations in declare global blocks. This is necessary because TypeScript global augmentation requires var, not let/const.
…mplate Per feedback, this setting should be in the base template (not Pro-only): - Added to react_on_rails.rb.tt template with clear documentation - Default is :defer (recommended for most apps) - Documents that :async requires React on Rails Pro for better performance - Explains the differences between :defer, :sync, and :async - Prevents component registration race conditions with :defer default This ensures new Rails apps get the configuration with sensible defaults.
Per code review feedback, immediate_hydration should be configured in the Pro initializer, not the base initializer: 1. Updated base initializer template to include Pro features section - Documents that immediate_hydration requires Pro - Directs users to configure in react_on_rails_pro.rb - Provides link to Pro information 2. Updated docs/api-reference/configuration.md - Removed immediate_hydration from base config example - Added Pro features section with clear example - Shows proper usage in ReactOnRailsPro.configure block This ensures users understand immediate_hydration is Pro-only and know where to configure it properly.
CI detects no-var violations in declare global blocks, while local ESLint does not. This appears to be an environment difference in how TypeScript ESLint processes ambient declarations. Adding the disable directive to match CI requirements. The declare global syntax requires var (not let/const) per TypeScript spec.
Implements async usage detection in the doctor program to prevent race conditions when using :async loading strategy or javascript_pack_tag without React on Rails Pro. Key features: - Scans view files (.erb, .haml) for javascript_pack_tag with :async - Checks config for generated_component_packs_loading_strategy = :async - Skips validation when Pro is installed (async is safe and default) - Reports errors when async used without Pro or immediate_hydration - Reports warnings when immediate_hydration enabled but Pro not installed - Provides clear, actionable error messages with upgrade guidance Also updates generator template to comment out explicit generated_component_packs_loading_strategy setting, allowing defaults to "do the right thing" (:defer for non-Pro, :async for Pro). Comprehensive test coverage added for all scenarios. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
CI requires no-var to be disabled for var declarations in declare global blocks. This is necessary because TypeScript global augmentation requires var, not let/const.
Focus this PR solely on validating that :async loading strategy requires Pro. immediate_hydration will be completely removed in PR 1997, so no need to validate it here. Changes: - Remove immediate_hydration from Pro-only feature validation - Simplify validate_pro_only_features to only check :async loading strategy - Remove immediate_hydration checks from doctor.rb - Update tests to only test :async Pro validation - Simplify error messages to focus on :async requirement 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/fix-shared-store-tests
branch
from
0a70e53 to
958fb97
Compare
Summary
This PR adds validation to prevent non-Pro users from using Pro-only features and cleans up the spec/dummy configuration to remove Pro-only settings.
Key Changes
1. Pro-Only Feature Validation (
lib/react_on_rails/configuration.rb)validate_pro_only_featuresmethod to detect and prevent usage of:immediate_hydration = truegenerated_component_packs_loading_strategy(any value)ReactOnRails::Errorwith clear actionable messagevalidate_generated_component_packs_loading_strategyto skip auto-setting for non-Pro users2. Spec/Dummy Config Cleanup (
spec/dummy/config/initializers/react_on_rails.rb)config.immediate_hydration = false(Pro-only, unnecessary)config.generated_component_packs_loading_strategy(Pro-only, unnecessary)3. Comprehensive Test Coverage (
spec/react_on_rails/configuration_spec.rb)4. CHANGELOG Update
Testing
✅ 44 RSpec examples, 0 failures
✅ RuboCop: 0 offenses
Error Message Example
When a non-Pro user tries to use these features:
Benefits
This change is
Summary by CodeRabbit
New Features
Configuration Changes
Documentation
Tests / Chores