Phase 6: Restructure Ruby Gems to Final Monorepo Layout #2108
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 7 minutes and 32 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 (2)
📒 Files selected for processing (1)
WalkthroughThis PR completes the Phase‑6 monorepo restructure: it moves Pro Ruby sources to Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application code
participant Utils as ReactOnRails::Utils
participant Bundler as Bundler environment
participant Pro as react_on_rails_pro (lazy load)
Note over Bundler,Pro: Detect Pro gem presence before requiring
App->>Utils: react_on_rails_pro?()
Utils->>Bundler: pro_gem_in_bundle?()
alt pro gem not in bundle
Utils-->>App: false
else pro gem in bundle
Utils->>Pro: require "react_on_rails_pro" (guarded)
alt require succeeds
Pro-->>Utils: module defined
Utils-->>App: true (or license-based result)
else LoadError/NameError
Utils-->>App: false
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Code Review: Phase 6 Restructure Ruby Gems✅ Overall AssessmentThis is a well-executed monorepo restructure with excellent attention to detail. The changes successfully flatten the directory structure and make both gems equal siblings. Here are my findings: 🎯 Strengths
|
Code Review: Phase 6 Monorepo RestructuringSummaryThis PR successfully restructures the React on Rails Pro gem from a nested subdirectory into a sibling structure. The changes are well-executed with proper attention to path updates and licensing boundaries. Critical Issue: Gemspec File PatternBLOCKER: The react_on_rails_pro.gemspec has a dangerous exclusion pattern on line 26 that excludes lib/tasks/. This will exclude critical Pro rake tasks from the gem:
These tasks provide critical Pro functionality and MUST be included. Fix: Change the gemspec to use a whitelist or exclude lib/tasks/ from the exclusion pattern. Verify before merging: Strengths
RecommendationDO NOT MERGE until gemspec pattern is fixed. Once fixed, this is excellent work! |
Detailed AnalysisThe Gemspec Problem ExplainedCurrent line 26 excludes lib/tasks/ entirely. This was probably fine when Pro had its own separate lib/tasks/, but now lib/tasks/ contains BOTH MIT and Pro tasks mixed together. Current lib/tasks/ contents:
Recommended Solution - Whitelist ApproachReplace the s.files pattern with a clear whitelist: Use Dir.glob to explicitly include only Pro files. Include lib/react_on_rails_pro.rb, lib/react_on_rails_pro/**/* and the two specific Pro rake tasks. This makes it crystal clear what belongs in the Pro gem and prevents accidentally excluding critical files. TestingBefore merging, run: gem build react_on_rails_pro.gemspec Then unpack and verify lib/tasks/assets_pro.rake and lib/tasks/v8_log_processor.rake are present. |
Additional Observations✅ What You Did Right
📋 Non-Blocking Questions
🧪 Post-Fix Testing ChecklistAfter fixing the gemspec:
Great work overall! This is a significant architectural improvement. |
![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)
rakelib/release.rake (1)
169-181: <!-- [scratchpad_start]
[task verify_pro_paths_3/3 COMPLETE]
Final verification complete.[observations]
react_on_rails_pro/directory EXISTS (contains node/JS Pro code)react_on_rails_pro.gemspecEXISTS at repo root (not in subdirectory)- Pro Ruby code at:
lib/react_on_rails_pro/version.rb(repo root level)- Current code resolves to:
- pro_version_file:
gem_root/react_on_rails_pro/lib/react_on_rails_pro/version.rb(WRONG - does not exist)- publish_gem_with_retry runs from:
gem_root/react_on_rails_pro/(WRONG - no gemspec there)[analysis]
- CONFIRMED: Review comment is CORRECT
- ISSUE 1 (Line 172): File.read(pro_version_file) will fail with ENOENT
- Should read from:
gem_root/lib/react_on_rails_pro/version.rb- ISSUE 2 (Line 303): gem release will fail to find gemspec
- publish_gem_with_retry should run from gem_root, not pro_gem_root
- Both are blocking bugs that prevent Pro gem release
[done]
- Issues verified and confirmed
- Ready to output final rewritten review comment with tag
Fix Pro version file path and gem release working directory
The Pro Ruby code has been moved to the repo root (
lib/react_on_rails_pro/version.rb), but the release script still uses the old nested path structure:
Line 172:
pro_version_fileresolves togem_root/react_on_rails_pro/lib/react_on_rails_pro/version.rb, which doesn't exist. The actual file is atgem_root/lib/react_on_rails_pro/version.rb.Line 303:
publish_gem_with_retry(pro_gem_root, ...)runsgem releasefromgem_root/react_on_rails_pro/, but the gemspec is atgem_root/react_on_rails_pro.gemspec. This will causegem releaseto fail.Update line 172:
- pro_version_file = File.join(pro_gem_root, "lib", "react_on_rails_pro", "version.rb") + pro_version_file = File.join(gem_root, "lib", "react_on_rails_pro", "version.rb")And update line 303 to run gem release from the correct directory:
- publish_gem_with_retry(pro_gem_root, "react_on_rails_pro", otp: rubygems_otp) + publish_gem_with_retry(gem_root, "react_on_rails_pro", otp: rubygems_otp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.rubocop.yml(1 hunks)Gemfile(1 hunks)LICENSE.md(1 hunks)rakelib/release.rake(1 hunks)react_on_rails_pro.gemspec(1 hunks)react_on_rails_pro/spec/dummy/spec/rails_helper.rb(1 hunks)script/ci-changes-detector(2 hunks)spec/pro/react_on_rails_pro/assets_precompile_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
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-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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbGemfilerakelib/release.rakereact_on_rails_pro.gemspecLICENSE.md.rubocop.yml
📚 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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbGemfilereact_on_rails_pro.gemspecLICENSE.md.rubocop.yml
📚 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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbGemfilerakelib/release.rakereact_on_rails_pro.gemspecLICENSE.md.rubocop.yml
📚 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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbGemfilereact_on_rails_pro.gemspecLICENSE.md
📚 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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbGemfilereact_on_rails_pro.gemspecLICENSE.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:
script/ci-changes-detectorreact_on_rails_pro/spec/dummy/spec/rails_helper.rbGemfilereact_on_rails_pro.gemspecLICENSE.md
📚 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:
react_on_rails_pro/spec/dummy/spec/rails_helper.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbGemfilereact_on_rails_pro.gemspec
📚 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:
react_on_rails_pro/spec/dummy/spec/rails_helper.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:
react_on_rails_pro/spec/dummy/spec/rails_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:
react_on_rails_pro/spec/dummy/spec/rails_helper.rbGemfilereact_on_rails_pro.gemspec
📚 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:
react_on_rails_pro.gemspec
📚 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:
react_on_rails_pro.gemspec
🪛 GitHub Actions: React on Rails Pro - Lint
Gemfile
[error] 6-6: Bundler parsing error: There are no gemspecs at /home/runner/work/react_on_rails/react_on_rails/react_on_rails_pro. Bundler cannot continue. Command: bundle 2.5.4 check || bundle 2.5.4 install --jobs=4 --retry=3
🪛 GitHub Actions: React on Rails Pro - Package Tests
Gemfile
[error] 6-6: Bundler failed to parse Gemfile: There are no gemspecs at /home/runner/work/react_on_rails/react_on_rails/react_on_rails_pro. Bundler cannot continue. Gemfile line 6: gemspec
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (6)
spec/pro/react_on_rails_pro/assets_precompile_spec.rb (1)
4-4: Require path correctly updated to new Pro lib layoutThe new
require_relative "../../../lib/react_on_rails_pro/assets_precompile"correctly targetslib/react_on_rails_pro/fromspec/pro/react_on_rails_pro/and aligns with the monorepo restructuring.LICENSE.md (1)
22-27: Pro license scope matches new directory layoutMarking
lib/react_on_rails_pro/,spec/pro/,react_on_rails_pro/, andreact_on_rails_pro.gemspecas Pro-licensed cleanly reflects the new monorepo structure and keeps Pro Ruby code and specs under explicit Pro scope.react_on_rails_pro.gemspec (1)
9-9: Corrected core version require for root-level gemspecUsing
require_relative "lib/react_on_rails/version"is the right adjustment now that the Pro gemspec lives at the repo root; it ensuresReactOnRails::VERSIONis loaded at build time for the dependency constraint.react_on_rails_pro/spec/dummy/spec/rails_helper.rb (1)
43-45: Dummy app rails_helper now correctly loads shared Pro support filesThe updated
require_relativepaths tospec/pro/react_on_rails_pro/support/...are correct given the new shared Pro spec/support location and preserve the existing RSpec configuration (e.g.,MockStreamHelperinclusion)..rubocop.yml (1)
30-35: RuboCop excludes correctly updated for relocated Pro code and specsExcluding
lib/react_on_rails_pro/**/*,react_on_rails_pro/**/*, andspec/pro/**/*from the root RuboCop run matches the new Pro layout and avoids mixing Pro-specific lint rules with core configuration.script/ci-changes-detector (1)
109-112: CI change detector correctly tracks relocated Pro Ruby and spec pathsUpdating the Pro Ruby glob to
lib/react_on_rails_pro/*|lib/react_on_rails_pro/**/*and the Pro gem-specs glob tospec/pro/*|spec/pro/**/*|.github/workflows/pro-gem-tests.ymlkeeps the CI classification in sync with the new Pro layout so the right Pro jobs still trigger on relevant changes.Also applies to: 121-124
|
Code Review: Phase 6 Ruby Monorepo RestructuringThis is a well-executed monorepo restructuring PR that successfully moves the Pro Ruby gem to a sibling layout. The migration is comprehensive and the changes follow the project's conventions. Here's my detailed review: ✅ Strengths1. Clear Architecture & Intent
2. Comprehensive Path UpdatesAll critical path references have been updated correctly:
3. Correct Rails Engine PracticesThe Pro engine (
4. Proper Gemspec ConfigurationThe
5. Test Infrastructure Updates
|
Code Review - Phase 6: Restructure Ruby Gems to Final Monorepo LayoutOverviewThis is a well-structured refactoring that successfully moves the Pro gem from a nested structure to a sibling structure alongside the open-source gem. The changes are comprehensive and appear to maintain backward compatibility for users while providing a cleaner monorepo structure for contributors. ✅ Strengths1. Excellent Structural Improvements
2. Comprehensive Path UpdatesYou've correctly updated:
3. Smart File Handling
4. Good Documentation
|
Code Review: Phase 6 Monorepo RestructureSummaryThis is a well-executed directory restructure that successfully flattens the Pro gem structure to be a sibling of the MIT gem. The changes are comprehensive and methodical. ✅ Strengths1. Clear Architecture ImprovementThe new structure is much cleaner - both gems are now equal siblings at the root level rather than nested, making the licensing boundaries and project organization more obvious. 2. Comprehensive Path UpdatesAll the critical path references have been updated:
3. Good File OrganizationThe explicit whitelisting in s.files = Dir.glob("{lib/react_on_rails_pro.rb,lib/react_on_rails_pro/**/*}") +
Dir.glob("lib/tasks/{assets_pro.rake,v8_log_processor.rake}") +
%w[react_on_rails_pro.gemspec CHANGELOG_PRO.md LICENSE README.md]This ensures only Pro-specific files are included in the Pro gem. 4. Rake Task Conflict PreventionRenaming 5. LICENSE.md ClarityUpdated LICENSE.md accurately reflects the new structure, making it clear which directories are Pro-licensed.
|
Code Review - Phase 6: Restructure Ruby Gems to Final Monorepo LayoutThis PR successfully restructures the monorepo to make both gems equal siblings instead of a nested structure. The changes are well-executed and align with the monorepo migration plan. Here's my detailed review: ✅ Strengths
|
Code Review: Phase 6 - Restructure Ruby Gems to Final Monorepo LayoutSummaryThis PR successfully restructures the Pro Ruby gem from a nested structure to a sibling layout. The changes are well-organized and comprehensive. Overall, this is a solid refactoring that improves the project structure. Below are my observations organized by category. ✅ Strengths1. Excellent Documentation
2. Comprehensive Path Updates
3. Clean Git History
4. Proper Gemfile Configuration
|
PR Review: Phase 6 Monorepo RestructureOverall Assessment✅ Excellent work on this major refactor! The PR successfully restructures the Pro gem to be a proper sibling to the core gem, which significantly improves the project's organization and clarity. ✅ Strengths1. Comprehensive Path UpdatesAll critical configuration files were properly updated:
2. Improved Gemspec File ListingThe change from
3. Git History PreservationAll file moves properly preserve git history (visible in the diff as renames, not delete+add). This is crucial for future debugging and understanding the codebase evolution. 4. Thorough require_relative UpdatesThe relative path updates in test files show attention to detail: # Before: react_on_rails_pro/spec/dummy/spec/rails_helper.rb
require_relative "../../react_on_rails_pro/support/caching"
# After: (moved 2 levels up in directory tree)
require_relative "../../../../spec/pro/react_on_rails_pro/support/caching"🔍 Critical Issues to Address1.
|
Code Review: Phase 6 - Restructure Ruby Gems to Final Monorepo LayoutSummaryThis is a well-executed monorepo restructuring that successfully transforms the nested Pro gem structure into a clean sibling layout. The changes are comprehensive, methodical, and show careful attention to detail. ✅ Strengths1. Excellent Architecture DecisionThe move from nested to sibling structure is the right call:
2. Comprehensive Path UpdatesThe PR correctly updates all critical configuration:
3. Smart Rake Task Conflict PreventionRenaming 4. Proper Gemspec File WhitelistingThe Pro gemspec now uses explicit whitelisting instead of dangerous exclusions: s.files = Dir.glob("{lib/react_on_rails_pro.rb,lib/react_on_rails_pro/**/*}") +
Dir.glob("lib/tasks/{assets_pro.rake,v8_log_processor.rake}") +
%w[react_on_rails_pro.gemspec CHANGELOG_PRO.md LICENSE README.md]This prevents accidentally excluding critical files. 5. Git History PreservationAll moves are done as
|
Code Review: Phase 6 Ruby Gems RestructureOverviewWell-executed structural refactoring moving Pro gem to sibling structure. Changes are comprehensive and systematic. ✅ Strengths
|
Code Review: Phase 6 Ruby Gem RestructureI've reviewed this monorepo restructuring PR. Overall, this is a well-executed architectural improvement with thorough planning and comprehensive path updates. Here's my detailed feedback: ✅ Strengths1. Excellent Documentation & Planning
2. Thorough Path UpdatesThe PR correctly updates paths in all critical locations:
3. Critical Gemspec Fix (Commit 2)Excellent catch fixing the gemspec file inclusion pattern:
4. Git History PreservedAll files properly moved with
|
PR Review: Phase 6 Monorepo RestructureI've reviewed this PR thoroughly. Overall, this is a well-executed refactor with excellent attention to detail. Here's my comprehensive feedback: ✅ Strengths1. Excellent Documentation
2. Well-Structured Changes
3. Proper Path Management
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
react_on_rails.gemspec (2)
19-25: Good exclusion of Pro-specific files; minor style inconsistency.The explicit exclusions for
lib/react_on_rails_proand Pro-specific rake tasks correctly prevent Pro-only code from being packaged in the MIT gem, addressing the critical bug mentioned in the PR objectives.Minor style note: Line 21 uses
/.../regex syntax while lines 23-24 use%r{...}. Both are valid, but consistent syntax improves readability.Consider unifying the regex literal syntax for consistency:
- f.match(/^(jest\.config\.js|book\.json|package\.json|package-scripts\.yml|yarn\.lock|\..*)/) || + f.match(%r{^(jest\.config\.js|book\.json|package\.json|package-scripts\.yml|yarn\.lock|\..*)}) ||
39-41: Verify if gem-release must be a gemspec dependency.The
Gemspec/DevelopmentDependenciescop is intentionally disabled here. Best practice is to declare development dependencies in theGemfilerather than the gemspec. Ifgem-releasedoesn't require gemspec placement for release tooling, consider moving it to the Gemfile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rakelib/shakapacker_examples.rake(1 hunks)react_on_rails.gemspec(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
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-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:
react_on_rails.gemspecrakelib/shakapacker_examples.rake
📚 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:
react_on_rails.gemspec
📚 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:
react_on_rails.gemspecrakelib/shakapacker_examples.rake
📚 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:
react_on_rails.gemspecrakelib/shakapacker_examples.rake
📚 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:
react_on_rails.gemspecrakelib/shakapacker_examples.rake
📚 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:
react_on_rails.gemspec
📚 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:
rakelib/shakapacker_examples.rake
📚 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:
rakelib/shakapacker_examples.rake
⏰ 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: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (2)
rakelib/shakapacker_examples.rake (1)
50-55: LGTM! Clear explanation for skip validation flag.The updated comments clearly explain why
REACT_ON_RAILS_SKIP_VALIDATIONis needed during pack generation. The approach is consistent with the flag usage on line 46 for generator commands.react_on_rails.gemspec (1)
49-49: Excellent security enhancement!Adding
rubygems_mfa_requiredmetadata enforces multi-factor authentication for gem releases, protecting against unauthorized publishes.
PR Review: Phase 6 Monorepo RestructureI've completed a thorough review of this PR. Overall, this is an excellent refactoring that significantly improves the monorepo structure. ✅ Strengths1. Excellent Licensing Separation
2. Comprehensive Path Updates
3. Smart Validation Handling
4. Critical Bug Fix in utils.rb:262
5. Good Commit Hygiene
|
PR Review: Phase 6 Monorepo RestructureSummaryThis PR successfully completes Phase 6 of the monorepo migration by restructuring the React on Rails Pro Ruby gem to be a sibling of the MIT gem rather than a nested subdirectory. The changes are well-executed and thoroughly documented. ✅ Strengths1. Clear Architecture
2. Comprehensive Updates
3. Excellent Documentation
4. Security & Publishing Safety
|
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 (4)
.github/workflows/examples.yml(2 hunks).github/workflows/integration-tests.yml(3 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks)react_on_rails.gemspec(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/integration-tests.yml
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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.
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: 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.
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.
📚 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:
.github/workflows/lint-js-and-ruby.ymlreact_on_rails.gemspec
📚 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:
.github/workflows/lint-js-and-ruby.yml.github/workflows/examples.ymlreact_on_rails.gemspec
📚 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:
.github/workflows/lint-js-and-ruby.ymlreact_on_rails.gemspec
📚 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:
react_on_rails.gemspec
📚 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:
react_on_rails.gemspec
⏰ 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: examples (3.4, latest)
- GitHub Check: rspec-gem-specs (3.3.7)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (5)
.github/workflows/lint-js-and-ruby.yml (1)
121-122: Build step placement looks correct; verify yarn build command is defined in package.json.The step correctly precedes the yalc publish, ensuring compiled artifacts are ready. Confirm that
packages/react-on-rails/package.jsondefines a build script and that the build is deterministic..github/workflows/examples.yml (2)
104-104: Verify that REACT_ON_RAILS_PRO_LICENSE is consumed downstream.The Pro license environment variable is now injected into the examples job, but there's no visible usage in the provided code snippet. Ensure it is actually consumed by the rake task on line 180 (
bundle exec rake run_rspec:shakapacker_examples) or other downstream steps.
148-151: Build step is well-motivated and correctly placed.The
yarn workspaces run buildstep appropriately builds all workspace packages before yalc publish, aligning with the Phase 6 monorepo restructuring. The step precedes yalc and the comment clearly documents the intent.Verify that the
buildscript is defined in each workspace package'spackage.json(react-on-rails, react-on-rails-pro, react-on-rails-pro-node-renderer) and that the workspaces build is deterministic.react_on_rails.gemspec (2)
50-50: MFA requirement metadata is a strong security practice.Adding
s.metadata["rubygems_mfa_required"] = "true"enforces multi-factor authentication for future gem releases, which aligns with modern supply-chain security standards.
19-26: Pro file exclusion patterns are correct and properly targeted.Verification confirms the three exclusion patterns (lines 23–25) correctly separate Pro files from the MIT gem:
^react_on_rails_proexcludes the top-level Pro directory (~300+ files)^lib/react_on_rails_proexcludes Pro library code (~24 files)^lib/tasks/(assets_pro|v8_log_processor)\.rake$precisely excludes only the two Pro rake tasks with no over-matchingCore MIT files are present and not accidentally excluded:
lib/react_on_rails.rb,lib/generators/react_on_rails/**, and core rake tasks (assets.rake,doctor.rake,generate_packs.rake,locale.rake).
| # rubocop:disable Gemspec/DevelopmentDependencies | ||
| s.add_development_dependency "gem-release" | ||
| # rubocop:enable Gemspec/DevelopmentDependencies |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the intent and scope of the RuboCop disable block.
The Gemspec/DevelopmentDependencies rule is disabled only for the gem-release dependency. Verify:
- This rule check is intentional and documented (e.g., is
gem-releaselegitimately a development-only dependency with a known exception?) - The rule and its context are clear to future maintainers (consider adding an inline comment if not already self-evident)
🏁 Script executed:
#!/bin/bash
# Description: Verify RuboCop configuration and gem-release intent.
echo "=== Checking .rubocop.yml for Gemspec/DevelopmentDependencies context ==="
rg -A5 -B5 "Gemspec/DevelopmentDependencies" .rubocop.yml
echo -e "\n=== Verifying gem-release is only a dev/release tool ==="
rg -C3 "gem-release" Gemfile react_on_rails.gemspecLength of output: 594
Based on my verification:
Add a clarifying comment to document why gem-release requires this RuboCop exception.
The gem-release gem adds commands like bump, tag, and release to the rubygems gem command, making it purely a release/build tool, not a runtime or test dependency. The Gemspec/DevelopmentDependencies rule enforces that development dependencies are specified in Gemfile rather than in the gemspec.
The inline RuboCop disable is valid since gem-release legitimately belongs in development, but the exception lacks context for future maintainers. Add a brief comment explaining that gem-release is intentionally specified here as a release tool:
# rubocop:disable Gemspec/DevelopmentDependencies
# gem-release is a release automation tool needed by developers/CI
s.add_development_dependency "gem-release"
# rubocop:enable Gemspec/DevelopmentDependenciesAlternatively, move it to the Gemfile and configure the rule's AllowedGems in .rubocop.yml for consistency.
🤖 Prompt for AI Agents
In react_on_rails.gemspec around lines 40 to 42, add a brief clarifying comment
explaining why the RuboCop Gemspec/DevelopmentDependencies rule is disabled for
gem-release and that gem-release is a release/build tool used by developers/CI
(or alternatively move it to the Gemfile and update .rubocop.yml AllowedGems);
update the inline RuboCop disable block to include that single-line explanatory
comment above s.add_development_dependency "gem-release" and leave the enable
line as-is.
Code Review: Phase 6 Restructure Ruby GemsThis is an excellent monorepo restructuring PR with comprehensive testing and documentation. ✅ Strengths
|
Code Review: Phase 6 Monorepo RestructureI've completed a comprehensive review of this PR. Overall, this is an excellent refactoring that significantly improves the monorepo structure. The changes are well-thought-out, thoroughly tested, and properly documented. ✅ Strengths1. Excellent Directory StructureThe new sibling layout is much clearer than the previous nested structure:
2. Comprehensive TestingThe new
This is a critical safeguard that was missing before. 3. Proper Path ManagementAll path updates were done correctly:
4. Excellent DocumentationThe CONTRIBUTING.md additions about gemspec file management are clear and helpful:
5. CI Workflow ImprovementsAdded necessary build steps before yalc publish: - name: Build packages before yalc publish
run: yarn workspaces run buildThis ensures 6. License ClarityLICENSE.md updates clearly delineate MIT vs Pro:
|
justin808
force-pushed
the
jg/phase6-ruby-sibling
branch
from
a400903 to
b42600f
Compare
Pull Request Review - Phase 6: Restructure Ruby Gems to Final Monorepo LayoutI've completed a comprehensive review of this PR. Overall, this is excellent work with thorough attention to detail and proper testing. Here's my feedback: ✅ Strengths1. Excellent Problem-Solving ProcessThe commit history shows a methodical approach to fixing issues:
This incremental fixing approach is exemplary for a complex refactor. 2. Critical Bug Discovery and FixThe discovery that the MIT gem was including Pro files (commits 8-9) shows excellent attention to licensing boundaries. The fix using explicit whitelisting for the Pro gemspec is the right approach. 3. Comprehensive Testing Infrastructure
This is exactly the kind of validation test that prevents serious licensing violations. 4. Documentation Quality
🔍 Code Quality ObservationsGemspec File Managementreact_on_rails.gemspec (lines 19-28): react_on_rails_pro.gemspec (lines 24-31): Suggestion: Consider adding a comment explaining why whitelisting is used here vs blacklisting in the MIT gemspec: # Explicitly whitelist Pro files to ensure we only include what belongs in this gem.
# This is safer than blacklisting because it requires conscious inclusion of new files
# and prevents accidental Pro file leakage into MIT gem.Workspace Dependenciespackages/react-on-rails-pro/package.json (line 58): "react-on-rails": "^16.2.0-beta.12"✅ Correct fix - Using explicit version range instead of The final commit fixing this shows good understanding of Yarn workspace resolution and webpack module resolution. CI Workflow UpdatesBuild Before Publish Pattern: - name: Build packages before yalc publish
run: yarn workspaces run build
- name: yalc publish for react-on-rails
run: yarn run yalc:publish✅ Critical fix - Publishing empty lib/ directories would cause silent failures Pro Workflow Restructuring.github/workflows/pro-lint.yml & pro-test-package-and-gem.yml:
✅ Correct - Reflects new directory structure
|
Code Review: Phase 6 - Ruby Gems RestructuringThis is a well-executed major refactoring that successfully restructures the Pro Ruby gem from a nested to sibling architecture. ✅ Strengths1. Clear Architecture & Licensing
2. Comprehensive Path Updates
3. Gemspec Design
4. CI Workflow Updates
5. Build System
|
Pull Request Review: Phase 6 - Restructure Ruby Gems to Final Monorepo LayoutOverall Assessment: ✅ Excellent Work with Minor RecommendationsThis is a well-executed monorepo restructuring that significantly improves the codebase organization. 🌟 Strengths1. Excellent Architecture
2. Outstanding Test Coverage
3. Proper File Management
4. Great Documentation
5. CI/CD Updates
|
When asked to fix tests, Claude will now automatically: - Run linting (rubocop, prettier) - Create descriptive commit - Push immediately without waiting - Check CI status after push Exceptions: only skip auto-push if fix is highly speculative, user explicitly requests no push, or multiple unrelated changes need separation.
Use ?? operator instead of || for type-safe fallback when data.plan is undefined. This prevents runtime TypeError when calling .startsWith() on non-string values while satisfying ESLint's no-unnecessary-type-conversion rule. Fix: plan = data.plan ?? '' The ?? operator ensures we only use the fallback when plan is null/undefined, making the code both type-safe and lint-compliant.
justin808
force-pushed
the
jg/phase6-ruby-sibling
branch
from
ed621e5 to
0173f7a
Compare
Auto-fix 26 files with formatting issues using 'yarn start format' All RuboCop checks pass.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
react_on_rails.gemspec (1)
42-44: Add clarifying comment for RuboCop exception.The RuboCop disable block lacks context for why
gem-releaserequires this exception. Consider adding a brief comment explaining thatgem-releaseis a release automation tool.Example:
# rubocop:disable Gemspec/DevelopmentDependencies +# gem-release is a release automation tool needed by developers/CI s.add_development_dependency "gem-release" # rubocop:enable Gemspec/DevelopmentDependencies
🧹 Nitpick comments (11)
CLAUDE.md (1)
58-79: Clarify the relationship between local testing requirements and auto-push guidance.The new "AUTOMATIC PUSHING FOR TEST FIXES" section (lines 58-79) sits directly after the "LOCAL TESTING REQUIREMENTS" section (lines 32-56), which emphasizes not pushing speculative/untested fixes. While these can be reconciled (auto-push assumes local testing has been done), the implicit assumption deserves explicit statement.
Consider adding a brief preamble like:
~**CRITICAL - AUTOMATIC PUSHING FOR TEST FIXES:** ~ ~When the user asks you to "fix the tests" or "fix failing tests": ~ ~**Assumption**: You have already tested the fixes locally per the LOCAL TESTING REQUIREMENTS above.This clarifies that the automatic push is conditional on local verification, preventing confusion about when to push vs. when to mark fixes as untested.
spec/pro/react_on_rails_pro/support/fixtures_helper.rb (1)
1-11: Consider extracting duplicate helper code (optional).This FixturesHelper module is duplicated across three locations (spec/react_on_rails/, spec/pro/react_on_rails_pro/, and react_on_rails_pro/spec/). While the duplication is minimal and each serves a different test suite, consider extracting to a shared location if more shared test utilities emerge.
spec/pro/react_on_rails_pro/assets_precompile_spec.rb (8)
34-72: Avoid leaking ENV['NODE_ENV'] changes across examples
ENV["NODE_ENV"] = "production"is set in this example but never restored, so other specs may observe a mutated environment depending on run order.Consider capturing and restoring the original value in an
aroundblock orensureclause so this spec is self-contained.
34-72: bundles_cache_key test covers the right inputs but could assert the digest call more directlyThe setup nicely constructs the union of
source_pathanddependency_globs, and stubsReactOnRailsPro::Utils.digest_of_globswithexpected_parameters. However, the test only indirectly validates that via the final cache key.If you want stronger coupling to the intended API, you could also assert that
digest_of_globswas invoked withexpected_parameters(e.g.,expect(ReactOnRailsPro::Utils).to have_received(:digest_of_globs).with(expected_parameters)), while keeping the existing key assertion.
102-117: build_bundles spec is asserting Ruby arity, not adapter behaviorThis example verifies that calling
build_bundleswith no arguments raisesArgumentErrorfrom Ruby’s method arity, not from any explicit guard in your code.If the long‑term intent is to enforce a single required argument, keeping this is fine. If you care more about adapter behavior, consider adding (or replacing with) a test that exercises a successful call path with the expected filename argument.
119-164: Clean up ENV['DISABLE_PRECOMPILE_CACHE'] and simplify stubbing in build_or_fetch_bundlesTwo related points here:
ENV["DISABLE_PRECOMPILE_CACHE"]is set/cleared inbeforeblocks but never restored to its original value, which can leak into other specs.- In the
"can't be fetched"example,fetch_and_unzip_cached_bundlesandbuild_bundlesare both configured withexpect(...).to receiveand then overridden withallow(...).to receive_messages, which is harder to reason about and risks masking expectations.Suggestion:
- Wrap ENV mutations in an
aroundblock that saves and restores the previous value.- Replace the
expect+receive_messagescombo with a single expectation that also sets the return value, e.g.:expect(instance).to receive(:fetch_and_unzip_cached_bundles).once.and_return(false) expect(instance).to receive(:build_bundles).once.and_return(nil) expect(instance).to receive(:cache_bundles).once.and_return(nil)This keeps expectations and stubbing aligned and avoids double configuration.
166-191: fetch_bundles test is good; consider asserting binwrite argumentsThe spec correctly verifies that
remote_bundle_cache_adapter.fetchis called with thezipped_bundles_filenameand thatFile.binwriteis invoked once, which is the important behavior.If you want the test to be slightly stricter, you could also assert that
File.binwritereceives the expectedzipped_bundles_filepathargument so regressions in file path handling are caught.
224-265: cache_bundles spec validates adapter upload but not the Rake/packer sideThe example correctly ensures that
remote_bundle_cache_adapter.uploadreceives the resolvedzipped_bundles_filepath, which is the critical behavior.If you later refactor the implementation to rely more heavily on the Shakapacker/Rake integration (e.g., pre-archiving bundles), consider extending this spec to assert that the expected
Rake.shcommand (or equivalent) is invoked as well. For now, what you have is sufficient.
267-300: Make filesystem cleanup in copy_extra_files_to_cache_dir more robustThe
afterblock callsFileUtils.remove_dir("extra_files_cache_dir")withoutforce: true, which can raise if the directory was never created (e.g., due to early failures) and obscure the original failure.Consider:
after do FileUtils.remove_dir("extra_files_cache_dir", true) endto tolerate a missing directory. The positive assertions on the copied files look correct.
302-329: extract_extra_files_from_cache_dir leaves its cache dir on diskThis example creates
extra_files_cache_dirbut theafterblock only removesextra_files_extract_destination, so the cache directory will remain on disk after the test run.Adding removal of
extra_files_cache_dirin theafterblock (withforce: true) would keep the filesystem clean and avoid test-order‑dependent artifacts.spec/pro/react_on_rails_pro/utils_spec.rb (1)
141-180: with_trace tests rely on as_null_object; consider making logger expectations more explicitUsing
logger_mock.as_null_objectmeansRails.logger.info?becomes truthy implicitly in the “tracing on” context, which is clever but not immediately obvious when reading the test.For readability, you could explicitly stub
logger_mock(orRails.logger) to respond toinfo?withtruein the tracing-on context andfalsein the tracing-off context, while keeping the existing expectations oninfo. Behavior-wise, the current tests are correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (62)
.github/read-me.md(1 hunks).github/workflows/examples.yml(2 hunks).github/workflows/integration-tests.yml(3 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/pro-integration-tests.yml(2 hunks).github/workflows/pro-lint.yml(3 hunks).github/workflows/pro-test-package-and-gem.yml(6 hunks).rubocop.yml(2 hunks)CLAUDE.md(1 hunks)CONTRIBUTING.md(1 hunks)Gemfile(1 hunks)LICENSE.md(1 hunks)TODO.md(0 hunks)eslint.config.ts(5 hunks)lib/react_on_rails/utils.rb(1 hunks)lib/react_on_rails_pro/license_public_key.rb(1 hunks)packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts(1 hunks)packages/react-on-rails-pro-node-renderer/src/worker.ts(1 hunks)packages/react-on-rails-pro-node-renderer/src/worker/vm.ts(1 hunks)packages/react-on-rails/src/buildConsoleReplay.ts(0 hunks)packages/react-on-rails/src/context.ts(1 hunks)rakelib/release.rake(1 hunks)rakelib/run_rspec.rake(1 hunks)rakelib/shakapacker_examples.rake(1 hunks)react_on_rails.gemspec(2 hunks)react_on_rails_pro.gemspec(2 hunks)react_on_rails_pro/Gemfile(1 hunks)react_on_rails_pro/spec/dummy/Gemfile(1 hunks)react_on_rails_pro/spec/dummy/spec/rails_helper.rb(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/Gemfile(1 hunks)react_on_rails_pro/spec/react_on_rails_pro/assets_precompile_spec.rb(2 hunks)react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb(1 hunks)react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb(1 hunks)react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb(0 hunks)react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb(0 hunks)react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb(0 hunks)react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb(1 hunks)react_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb(1 hunks)script/ci-changes-detector(2 hunks)spec/dummy/Gemfile(1 hunks)spec/pro/.rubocop.yml(1 hunks)spec/pro/empty_spec.rb(1 hunks)spec/pro/react_on_rails_pro/assets_precompile_spec.rb(1 hunks)spec/pro/react_on_rails_pro/cache_spec.rb(1 hunks)spec/pro/react_on_rails_pro/configuration_spec.rb(1 hunks)spec/pro/react_on_rails_pro/fixtures/app/views/comments/_comment.json.jbuilder(1 hunks)spec/pro/react_on_rails_pro/fixtures/app/views/comments/index.json.jbuilder(1 hunks)spec/pro/react_on_rails_pro/fixtures/app/views/comments/show.json.jbuilder(1 hunks)spec/pro/react_on_rails_pro/react_on_rails_pro_spec.rb(1 hunks)spec/pro/react_on_rails_pro/request_spec.rb(1 hunks)spec/pro/react_on_rails_pro/simplecov_helper.rb(1 hunks)spec/pro/react_on_rails_pro/spec_helper.rb(1 hunks)spec/pro/react_on_rails_pro/stream_decorator_spec.rb(1 hunks)spec/pro/react_on_rails_pro/stream_request_spec.rb(1 hunks)spec/pro/react_on_rails_pro/stream_spec.rb(1 hunks)spec/pro/react_on_rails_pro/support/caching.rb(1 hunks)spec/pro/react_on_rails_pro/support/fixtures_helper.rb(1 hunks)spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rb(1 hunks)spec/pro/react_on_rails_pro/support/mock_block_helper.rb(1 hunks)spec/pro/react_on_rails_pro/support/mock_stream_helper.rb(1 hunks)spec/pro/react_on_rails_pro/utils_spec.rb(1 hunks)spec/react_on_rails/gemspec_file_inclusion_spec.rb(1 hunks)
💤 Files with no reviewable changes (5)
- packages/react-on-rails/src/buildConsoleReplay.ts
- TODO.md
- react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb
- react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
- react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb
✅ Files skipped from review due to trivial changes (4)
- spec/pro/empty_spec.rb
- spec/dummy/Gemfile
- react_on_rails_pro/spec/react_on_rails_pro/assets_precompile_spec.rb
- spec/pro/react_on_rails_pro/fixtures/app/views/comments/_comment.json.jbuilder
🚧 Files skipped from review as they are similar to previous changes (23)
- react_on_rails_pro/spec/dummy/spec/rails_helper.rb
- lib/react_on_rails/utils.rb
- packages/react-on-rails/src/context.ts
- spec/pro/.rubocop.yml
- script/ci-changes-detector
- .rubocop.yml
- react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb
- CONTRIBUTING.md
- .github/workflows/pro-integration-tests.yml
- rakelib/run_rspec.rake
- react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb
- react_on_rails_pro.gemspec
- packages/react-on-rails-pro-node-renderer/src/worker.ts
- react_on_rails_pro/Gemfile
- .github/workflows/pro-test-package-and-gem.yml
- packages/react-on-rails-pro-node-renderer/src/worker/vm.ts
- rakelib/shakapacker_examples.rake
- .github/workflows/examples.yml
- react_on_rails_pro/spec/execjs-compatible-dummy/Gemfile
- rakelib/release.rake
- react_on_rails_pro/spec/dummy/Gemfile
- packages/react-on-rails-pro-node-renderer/src/shared/licenseValidator.ts
- Gemfile
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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.
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: 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.
📚 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:
.github/read-me.mdspec/pro/react_on_rails_pro/utils_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/react_on_rails/gemspec_file_inclusion_spec.rbspec/pro/react_on_rails_pro/request_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails.gemspec.github/workflows/pro-lint.ymlLICENSE.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:
.github/read-me.mdspec/pro/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/support/caching.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/cache_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rbLICENSE.md
📚 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/pro/react_on_rails_pro/stream_request_spec.rbspec/pro/react_on_rails_pro/stream_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/stream_decorator_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
spec/pro/react_on_rails_pro/utils_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/react_on_rails/gemspec_file_inclusion_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails.gemspecLICENSE.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:
spec/pro/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/stream_spec.rbspec/pro/react_on_rails_pro/support/caching.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/react_on_rails/gemspec_file_inclusion_spec.rbspec/pro/react_on_rails_pro/request_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbspec/pro/react_on_rails_pro/stream_decorator_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails.gemspec.github/workflows/pro-lint.ymlLICENSE.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:
spec/pro/react_on_rails_pro/utils_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/react_on_rails/gemspec_file_inclusion_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb.github/workflows/lint-js-and-ruby.ymlspec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails.gemspec.github/workflows/pro-lint.yml.github/workflows/integration-tests.ymlLICENSE.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/pro/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/support/caching.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/cache_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rbspec/pro/react_on_rails_pro/assets_precompile_spec.rbspec/pro/react_on_rails_pro/support/mock_block_helper.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/pro/react_on_rails_pro/support/caching.rbspec/pro/react_on_rails_pro/spec_helper.rbspec/pro/react_on_rails_pro/support/mock_block_helper.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:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/react_on_rails/gemspec_file_inclusion_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb.github/workflows/lint-js-and-ruby.ymlspec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails.gemspec.github/workflows/integration-tests.ymlLICENSE.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:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/react_on_rails_pro_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbspec/pro/react_on_rails_pro/spec_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/utils_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:
react_on_rails_pro/spec/react_on_rails_pro/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:
spec/pro/react_on_rails_pro/request_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:
spec/pro/react_on_rails_pro/react_on_rails_pro_spec.rb
🧬 Code graph analysis (9)
spec/pro/react_on_rails_pro/support/fixtures_helper.rb (2)
spec/react_on_rails/support/fixtures_helper.rb (1)
fixtures_dir(3-11)react_on_rails_pro/spec/react_on_rails_pro/support/fixtures_helper.rb (1)
fixtures_dir(3-11)
spec/pro/react_on_rails_pro/stream_request_spec.rb (1)
lib/react_on_rails_pro/stream_request.rb (1)
create(142-144)
spec/pro/react_on_rails_pro/utils_spec.rb (2)
lib/react_on_rails_pro/utils.rb (12)
bundle_file_name(112-118)bundle_hash(91-99)rsc_bundle_js_file_path(22-27)rsc_bundle_hash(101-109)digest_of_globs(69-85)with_trace(155-167)mine_type_from_file_name(184-187)printable_cache_key(190-202)rsc_support_enabled?(50-52)react_client_manifest_file_path(29-34)react_server_client_manifest_file_path(38-48)pro_attribution_comment(206-220)lib/react_on_rails/utils.rb (1)
server_bundle_js_file_path(190-195)
spec/pro/react_on_rails_pro/stream_spec.rb (5)
spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (3)
initialize(15-18)initialize(30-33)mock_streaming_response(140-150)lib/react_on_rails_pro/stream_request.rb (4)
initialize(4-83)initialize(5-14)initialize(85-169)initialize(86-88)spec/pro/react_on_rails_pro/support/mock_stream_helper.rb (2)
clear_stream_mocks(8-10)mock_streaming_response(4-6)spec/pro/react_on_rails_pro/support/mock_block_helper.rb (1)
mock_block(12-21)lib/react_on_rails_pro/concerns/stream.rb (1)
stream_view_containing_react_components(33-46)
eslint.config.ts (1)
react_on_rails_pro/eslint.config.mjs (1)
compat(12-16)
spec/pro/react_on_rails_pro/request_spec.rb (2)
lib/react_on_rails_pro/request.rb (1)
render_code_as_stream(21-34)lib/react_on_rails_pro/stream_request.rb (2)
each_chunk(54-82)each_chunk(92-112)
spec/pro/react_on_rails_pro/stream_decorator_spec.rb (1)
lib/react_on_rails_pro/stream_request.rb (6)
prepend(17-20)transform(23-35)append(38-41)each_chunk(54-82)each_chunk(92-112)rescue(43-46)
spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (2)
lib/react_on_rails_pro/stream_request.rb (4)
initialize(4-83)initialize(5-14)initialize(85-169)initialize(86-88)spec/pro/react_on_rails_pro/support/mock_stream_helper.rb (1)
mock_streaming_response(4-6)
spec/pro/react_on_rails_pro/support/mock_stream_helper.rb (1)
spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (2)
mock_streaming_response(140-150)clear_mocks(135-137)
🪛 ast-grep (0.40.0)
spec/pro/react_on_rails_pro/utils_spec.rb
[warning] 118-119: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: described_class.digest_of_globs(File.join(FixturesHelper.fixtures_dir, "app", "views", "**",
"*.jbuilder"))
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (49)
spec/pro/react_on_rails_pro/fixtures/app/views/comments/show.json.jbuilder (1)
1-1: LGTM!Standard Jbuilder partial rendering pattern, correctly structured for a test fixture. The file placement in
spec/pro/aligns with the PR's objective of restructuring Pro specs to the new directory layout.CLAUDE.md (1)
58-79: Approved: Clear workflow structure for test fix automation.The section provides well-defined workflow steps, commit message guidance, and reasonable exceptions. The reference to
pr-splitting-strategy.mdon line 80 is consistent with the PR objectives. The overall structure and actionable guidance are solid.spec/pro/react_on_rails_pro/fixtures/app/views/comments/index.json.jbuilder (1)
1-2: LGTM!The Jbuilder syntax is correct and uses the standard collection rendering pattern with a partial. The explanatory comment is helpful, and this test fixture appropriately supports the Pro specs that were moved to the new location as part of the Phase 6 restructuring.
react_on_rails_pro/spec/react_on_rails_pro/utils_spec.rb (2)
80-86: LGTM: Mock consolidation improves readability.The refactoring to use
receive_messagesfor setting up multiple stub expectations on the same object is a good improvement. This makes the test setup more concise and maintainable.
1-221: No action needed. The claimed removed tests are still present in the file.The search results show that all the tests mentioned in the AI summary as "removed" are actually still present in
spec/pro/react_on_rails_pro/utils_spec.rb:
rsc_support_enabled?tests at lines 229–248rsc_bundle_js_file_pathtests at lines 251+react_client_manifest_file_pathtests at lines 298+react_server_client_manifest_file_pathtests at lines 379+pro_attribution_commenttests at lines 439+The provided code snippet only displayed lines 1–221, creating the false impression that these tests were removed. The file continues beyond that range with full coverage of all these utility methods.
Likely an incorrect or invalid review comment.
eslint.config.ts (4)
19-19: Verify the removal oftsEslint.config()wrapper is intentional.The configuration was changed from
tsEslint.config([...])to a plain array assignment. ThetsEslint.config()function provides TypeScript type-checking for the ESLint configuration array, ensuring compile-time validation of config structure, rules, and options. Without it, you lose autocomplete and type safety during development, though ESLint will still work at runtime.If type safety is desired, consider reverting to:
-const config = [ +const config = tsEslint.config([ // ... config entries -]; +]);Also applies to: 323-323
64-64: LGTM! Correct spread pattern for FlatCompat.Spreading
compat.extends()is the correct pattern for ESLint flat config, as it returns an array that must be flattened into the parent configuration array.
197-206: LGTM! Good defensive pattern for TypeScript rules.The mapping ensures that strict type-checked rules only apply to TypeScript files by adding a
filespattern when missing. This prevents type-checking rules from being applied to JavaScript files, which is the correct behavior.
289-300: LGTM! Correct pattern for multiple test configurations.Each configuration block (recommended, style, dom) spreads a separate config object with its own rules, all targeting the same test file patterns. This is the correct ESLint flat config pattern, as each entry applies different rule sets to the test files.
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb (3)
5-5: LGTM: RuboCop disable comment removal aligns with updated linting configuration.The removal of the RuboCop disable comment is appropriate given the PR's updates to
.rubocop.ymlthat expand linting scope and adjust exclusions for Pro code paths.
3-3: No issues found — require_relative path resolves correctly.The spec file is correctly located at
spec/pro/react_on_rails_pro/configuration_spec.rbper the PR's restructuring goals. Sincespec/pro/react_on_rails_pro/spec_helper.rbexists in the same directory,require_relative "spec_helper"on line 3 resolves correctly.
6-201: Let me examine the RSC tests that the script found at lines 206-212:Now let me check the actual content after line 201 in the spec file:
Let me check the actual file content around the lines after 201:
Based on verification, I can now generate the final review comment:
The review comment lacks sufficient information to verify RSC test removal concerns.
React Server Components (RSC) configurations have been moved from ReactOnRails.configure to ReactOnRailsPro.configure as part of the v16 architecture refactoring. The script output confirms RSC configuration tests exist at lines 206-212 in this same spec file (beyond the provided snippet).
Since the provided code snippet (lines 6-201) shows only non-RSC configuration tests, and the script confirms RSC tests are present in the file, the original AI summary's claim of "removed entire RSC configuration options test block" appears to refer to tests that may have been reorganized within lines 6-201 specifically, rather than removed from the file entirely.
To properly assess this change:
- Verify the git diff to see what tests within lines 6-201 were actually removed or reorganized
- Confirm whether RSC test coverage is maintained elsewhere in the file or in other spec files
- Ensure RSC configuration validation is still tested given the architecture changes
spec/pro/react_on_rails_pro/simplecov_helper.rb (1)
5-16: LGTM! SimpleCov configuration follows best practices.The implementation correctly:
- Guards SimpleCov initialization behind an environment variable check
- Uses a standard filter pattern to exclude spec files from coverage reports
- Includes clear comments explaining the purpose
The filter logic using
src.filename.include?("/spec/")is a common Ruby pattern that will appropriately exclude all spec files from coverage measurement..github/workflows/lint-js-and-ruby.yml (1)
121-122: LGTM!The build step correctly ensures the react-on-rails package is built before publishing via yalc. The placement is appropriate and consistent with similar changes in other workflow files.
.github/workflows/integration-tests.yml (3)
142-143: LGTM!The build step is correctly placed and ensures the package is ready before publishing to yalc.
189-190: LGTM!The Pro license environment variable is correctly configured using the V2 secret. This aligns with the documentation added in .github/read-me.md.
242-243: LGTM!The build step is correctly placed and consistent with the pattern used throughout the workflow.
.github/read-me.md (1)
108-137: LGTM!The documentation clearly explains the Pro license secret requirement, setup steps, and usage pattern. The example YAML correctly shows the environment variable naming convention.
LICENSE.md (1)
22-27: LGTM!The license scope updates accurately reflect the Phase 6 restructuring, correctly documenting the new sibling layout for Pro components.
lib/react_on_rails_pro/license_public_key.rb (1)
18-18: Good cleanup!Removing the redundant
.strip_heredoccall is appropriate since the squiggly heredoc (<<~) already handles indentation stripping in Ruby.react_on_rails.gemspec (2)
21-27: LGTM!The Pro file exclusions are comprehensive and correctly separate MIT-licensed code from Pro-licensed code, aligning with the Phase 6 restructuring objectives.
52-52: LGTM!Adding
rubygems_mfa_requiredis a good security practice for modern gem publishing.spec/react_on_rails/gemspec_file_inclusion_spec.rb (1)
1-148: LGTM! Excellent test coverage.This comprehensive test suite validates the gemspec file inclusion rules for both MIT and Pro gems, ensuring proper separation of licensed code. The tests are well-organized, with clear expectations and helpful error messages. The dynamic validation of on-disk files (lines 92-104) is particularly robust.
.github/workflows/pro-lint.yml (4)
121-122: Cache path correctly updated for root-level gem structure.The cache path and key now reference the root-level
vendor/bundleandreact_on_rails_pro.gemspec, which correctly aligns with the flattened monorepo layout where the Pro gem is built from the repository root.
137-143: Bundle configuration correctly updated for root-level execution.The working directory is now set to
.(root) and bundler is explicitly configured to usevendor/bundle, which aligns with the cache configuration at lines 121-122. This ensures proper gem installation from the root-level gemspec.
169-171: RuboCop targets correctly updated for restructured Pro paths.The linting now runs from the repository root and targets the new Pro locations (
lib/react_on_rails_proandspec/pro), which correctly reflects the Phase 6 monorepo restructure.
173-175: RBS validation correctly moved to root execution context.The RBS type signature validation now runs from the repository root, aligning with the monorepo structure where both gems are built from the root level.
spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rb (3)
29-49: Mock request handling correctly implemented.The connection mock logic properly:
- Prevents duplicate responses by checking
request.response&.finished?- Validates that all requests have corresponding mocks with a clear error message
- Uses safe navigation to handle nil responses
63-92: Streaming and non-streaming mock handling correctly implemented.The method appropriately handles both streaming and non-streaming response modes:
- Streaming mode delivers chunks via
request.stream.on_chunkcallback- Non-streaming mode collects chunks and writes them to the response body
- Both paths properly finish the response and emit the response event
94-150: Mock lifecycle management correctly implemented.The mock counting, pattern matching, and registration logic are well-designed:
- Finite mocks are decremented and removed when exhausted; infinite mocks persist
- Both exact string and Regexp pattern matching are supported
- Safety check prevents adding mocks after an infinite mock is registered
- Mocked connections bypass real network operations via
open?andinterestsoverridesspec/pro/react_on_rails_pro/support/mock_stream_helper.rb (2)
3-11: Helper module provides clean test interface for mock streaming.The delegation to
HTTPX::Plugins::MockStreamis straightforward and provides a convenient interface for test code.
13-19: RSpec configuration ensures clean mock state between tests.The
beforehook clearing mocks before each test prevents mock pollution and ensures test isolation, which is critical for reliable test execution.spec/pro/react_on_rails_pro/spec_helper.rb (1)
45-120: RSpec configuration appropriately set up for Pro test suite.The configuration includes standard RSpec settings and redirects Rails.logger to stdout for test visibility, which is a common pattern in test suites.
spec/pro/react_on_rails_pro/react_on_rails_pro_spec.rb (1)
1-9: Basic version spec correctly validates Pro module loading.This smoke test ensures
ReactOnRailsPro::VERSIONis properly accessible after the monorepo restructure, confirming the module loads correctly from its new location.spec/pro/react_on_rails_pro/support/caching.rb (1)
3-18: Caching test hooks correctly implement state management.The hooks properly:
- Set up isolated cache stores for tests
- Clear cache state before each test
- Temporarily enable caching during tests
- Restore original
perform_cachingvalue after testsThe direct instance variable access at line 8 is unconventional but acceptable for test setup to ensure clean cache state.
spec/pro/react_on_rails_pro/support/mock_block_helper.rb (1)
3-26: Block mocking helper correctly implemented.The
mock_blockhelper provides a clean way to test methods that accept blocks:
- Creates a double with configurable behavior via the optional block parameter
- The
.blockmethod returns a proc that can be passed with the&operator- The
RSpec/VerifiedDoublesdisable is justified for creating a generic mockThis enables expressive block testing:
expect(mocked_block).to have_received(:call).with(args).spec/pro/react_on_rails_pro/assets_precompile_spec.rb (3)
6-32: Top-level filename/path specs look solidThe tests for
zipped_bundles_filenameandzipped_bundles_filepathcorrectly focus on their dependencies (bundles_cache_key,Rails.root, and the filename) with minimal stubbing and clear expectations. No changes needed.
74-100: remote_bundle_cache_adapter behavior is well coveredThe tests for
remote_bundle_cache_adaptercorrectly enforce that a module must be configured and that a valid adapter is returned from configuration. This matches the intended configuration contract and error surface.
193-222: fetch_and_unzip_cached_bundles control flow is well exercisedThe three examples cover:
- No local cache present and failed remote fetch.
- Local cache present (no remote fetch).
- Propagating the return value of
fetch_bundles.This gives good coverage of the method’s branching behavior; no issues here.
spec/pro/react_on_rails_pro/utils_spec.rb (10)
5-11: LicenseValidator stub keeps tests focused on Utils responsibilitiesThe top-level
beforestub forLicenseValidator.validated_license_data!isolates these tests from license boot-time behavior, which matches the goal of exercisingUtilsin a “licensed” context. This is a reasonable boundary for this spec.
12-107: cache helper specs nicely cover hashed vs non-hashed bundlesThe
bundle_file_name,bundle_hash, andrsc_bundle_hashexamples thoroughly exercise:
- Manifest-resolved filenames with and without hashes.
- MD5-based hashing fallback for unhashed server and RSC bundles.
- Correct use of configuration (
server_bundle_js_file,rsc_bundle_js_file) and Shakapacker lookups.The stubbing strategy keeps the logic deterministic without depending on real assets. No changes needed here.
109-139: digest_of_globs tests are correct; static-analysis “hardcoded secret” is a false positiveBoth examples correctly verify:
- That
Digest::MD5is used across all matching files, with counts changing whenexcluded_dependency_globsis configured.- That the resulting hexdigest is stable for the fixtures.
The long hex string and glob patterns flagged by static analysis are just fixture-based test data, not credentials or passphrases, so there is no security issue to address here.
182-218: mine_type_from_file_name behavior is well coveredThese examples exercise known extensions (
.json,.JSON,.js) and the unknown-extension fallback, matching theRack::Mime.mime_typecontract. This gives good confidence that cache files and manifests will be served with appropriate content types.
220-227: printable_cache_key spec validates nested-key flatteningThe test confirms that nested Enumerable keys are flattened and joined into a single underscored string, matching the implementation’s intent. No issues here.
229-249: rsc_support_enabled? delegation is simple and correctly specifiedThe two contexts cleanly verify that
rsc_support_enabled?just reflectsReactOnRailsPro.configuration.enable_rsc_support, which aligns with the intended RSC validation chain handled elsewhere.
251-296: rsc_bundle_js_file_path caching semantics are well exercisedThe three examples verify:
- Delegation to
ReactOnRails::Utils.bundle_js_file_path.- Caching behavior when not in development.
- Non-caching behavior when
Rails.env.development?is true.The explicit
instance_variable_setsetup/teardown keeps the tests isolated. This looks good.
298-377: react_client_manifest_file_path tests cover both packer/dev-server and caching pathsThe spec:
- Simulates Shakapacker config and dev-server state to verify URL vs filesystem path behavior.
- Separately tests caching vs non-caching based on
Rails.env.development?by stubbingReactOnRails::PackerUtils.asset_uri_from_packer.This gives strong confidence that both dev-server and non-dev environments resolve the manifest location correctly.
379-437: react_server_client_manifest_file_path behavior and error case are well specifiedThe tests validate:
- Correct delegation to
ReactOnRails::Utils.bundle_js_file_path.- Caching when not in development and non-caching in development.
- Raising a clear
ReactOnRailsPro::Errorwhenreact_server_client_manifest_fileis nil.This fully exercises the method’s contract; no changes needed.
439-500: pro_attribution_comment covers all licensing states comprehensivelyThe contexts for:
- Valid licensed,
- Grace period (including 1-day edge case),
- Evaluation license,
- Grace period with 0 days,
provide thorough coverage of the attribution comment formatting. The expectations align with the implementation in
ReactOnRailsPro::Utils.pro_attribution_comment.
| # Using a command name prevents results from getting clobbered by other test | ||
| # suites | ||
| SimpleCov.command_name "gem-tests" |
There was a problem hiding this comment.
🧩 Analysis chain
Consider using a more specific command name to isolate Pro gem coverage.
The command name "gem-tests" is generic. In a monorepo with both core and Pro gems, if the core gem also uses SimpleCov with the same command name, coverage results might merge or overwrite each other. A more specific name like "pro-gem-tests" would better align with the comment's stated intent to prevent clobbering by other test suites.
Apply this diff to use a more specific command name:
- SimpleCov.command_name "gem-tests"
+ SimpleCov.command_name "pro-gem-tests"To verify whether there's a potential naming conflict, run the following script to check for other SimpleCov configurations in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Search for other SimpleCov configurations to check for command_name conflicts
# Search for SimpleCov.command_name usage across the codebase
echo "=== SimpleCov command_name configurations ==="
rg -n "SimpleCov\.command_name" --type ruby
echo -e "\n=== SimpleCov configuration files ==="
rg -n "require ['\"]simplecov['\"]" --type ruby -A 5Length of output: 4599
Change Pro gem SimpleCov command name from "gem-tests" to "pro-gem-tests" to prevent coverage clobbering.
Verification confirms the issue: both the core gem (spec/react_on_rails/simplecov_helper.rb:9) and Pro gem use the identical command name "gem-tests". This will cause SimpleCov to merge or overwrite coverage results when both test suites run, directly contradicting the stated intent in lines 7-8. A more specific command name is required for proper coverage isolation.
Update spec/pro/react_on_rails_pro/simplecov_helper.rb:9:
- SimpleCov.command_name "gem-tests"
+ SimpleCov.command_name "pro-gem-tests"Also update react_on_rails_pro/spec/react_on_rails_pro/simplecov_helper.rb:9 (duplicate file from monorepo restructure):
- SimpleCov.command_name "gem-tests"
+ SimpleCov.command_name "pro-gem-tests"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Using a command name prevents results from getting clobbered by other test | |
| # suites | |
| SimpleCov.command_name "gem-tests" | |
| # Using a command name prevents results from getting clobbered by other test | |
| # suites | |
| SimpleCov.command_name "pro-gem-tests" |
🤖 Prompt for AI Agents
In spec/pro/react_on_rails_pro/simplecov_helper.rb around lines 7 to 9 (and the
duplicate at react_on_rails_pro/spec/react_on_rails_pro/simplecov_helper.rb line
9), the SimpleCov.command_name is set to the generic "gem-tests" which collides
with the core gem; change the command name to a distinct value such as
"pro-gem-tests" in both files to isolate coverage results. Ensure the line
becomes SimpleCov.command_name "pro-gem-tests" in each file so Pro gem coverage
does not clobber core gem results.
| it "returns a StreamDecorator instance" do | ||
| result = described_class.create { mock_response } | ||
| expect(result).to be_a(ReactOnRailsPro::StreamDecorator) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Verify mock_response is defined in the test environment.
The test references mock_response at line 9, but this helper is not defined in the current file or in the visible support files. Ensure this method is properly defined in a support file or as a test helper to prevent test failures.
Run the following script to check if mock_response is defined in the Pro test support files:
🏁 Script executed:
#!/bin/bash
# Search for mock_response definition in Pro spec support files
rg -n "def mock_response" spec/pro/Length of output: 95
🏁 Script executed:
#!/bin/bash
# Search for all uses of mock_response (singular) in the codebase
rg -n "mock_response" spec/pro/ | head -20Length of output: 1331
🏁 Script executed:
#!/bin/bash
# Look at the full context of the test file
cat spec/pro/react_on_rails_pro/stream_request_spec.rbLength of output: 363
🏁 Script executed:
#!/bin/bash
# Check what mock_responses does in mock_stream.rb
sed -n '125,145p' spec/pro/react_on_rails_pro/support/httpx/plugins/mock_stream.rbLength of output: 488
🏁 Script executed:
#!/bin/bash
# Check the spec_helper to see if mock_response is defined
cat spec/pro/react_on_rails_pro/spec_helper.rbLength of output: 5425
🏁 Script executed:
#!/bin/bash
# Check if mock_response is defined in the StreamRequest class
rg -n "def mock_response|mock_response =" spec/pro/react_on_rails_pro/ --type rbLength of output: 26
Define the mock_response method or variable.
The test at line 9 references mock_response, but this is undefined in the test file, spec_helper, or any support files. When the test runs, it will fail with a NameError. Define mock_response in a support file or as a helper method in the spec to provide the expected return value for the block passed to described_class.create.
🤖 Prompt for AI Agents
In spec/pro/react_on_rails_pro/stream_request_spec.rb around lines 8 to 11, the
test references an undefined mock_response which will raise a NameError; define
mock_response as a helper to return an appropriate object for the block (e.g., a
test double or instance_double matching the expected response interface) either
by adding a let(:mock_response) { ... } in this spec or by adding a helper
method in spec/support and requiring it in spec_helper.rb; ensure the provided
mock_response matches what described_class.create expects so the test can yield
it successfully.
Add guard to check if ReactOnRailsPro::Utils is defined before calling it. Add rescue block to handle NameError and LoadError gracefully. This fixes the issue where tests fail with 'uninitialized constant ReactOnRailsPro::Utils' when the Pro gem is available but not fully loaded yet. Fixes failing test: ReactOnRails::Configuration changes the configuration of the gem
…ture In the sibling gem structure, both react_on_rails and react_on_rails_pro gems are loaded from the root Gemfile, making both available via Gem.loaded_specs. This caused the base dummy app to incorrectly detect Pro as installed, leading to: - Package mismatch errors (base dummy has react-on-rails package, not Pro package) - Potential double-engine loading issues Solution: Check Bundler's actual dependencies to see if Pro is explicitly requested in the application's Gemfile, not just if it's available in the load path. This allows both gems to coexist in development while correctly identifying which gem the application is actually using.
The dummy app integration tests were failing with: "ERROR ReactOnRails: You have the Pro gem installed but are using the base 'react-on-rails' package" This occurred because: 1. Tests run with bundle exec from repo root 2. Root Gemfile includes BOTH base and Pro gems (via gemspecs) 3. Dummy app package.json only has base package (intentionally) 4. Version checker correctly detected this mismatch The dummy app is specifically for testing the BASE gem/package, not Pro. In a monorepo development/testing context, it's expected that Pro gem may be available in the parent bundle even when testing the base gem. Solution: Add REACT_ON_RAILS_SKIP_VALIDATION=true to dummy test tasks, matching the existing pattern used for example app tests (line 85). This fix resolves the CI failure in: https://github.com/shakacode/react_on_rails/actions/runs/19616420258/job/56169682395?pr=2108 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Problem: When workspace packages (react-on-rails-pro and react-on-rails-pro-node-renderer) declared `"react-on-rails": "*"` as a dependency, Yarn Classic was incorrectly resolving this to [email protected] from the npm registry instead of using the local workspace package. This caused: 1. node_modules/react-on-rails folders to appear in Pro package directories 2. The installed version (16.1.2) lacked the new exports field entries 3. Webpack builds in generated examples failed with "Module not found" errors when importing from react-on-rails/pageLifecycle, etc. Root Cause: The yarn.lock file had cached a resolution to [email protected] from a previous state when the dependency was `"^16.2.0-beta.12"`. Even after changing back to `"*"`, Yarn kept the old npm registry resolution because 16.1.2 technically matches the `"*"` range. Solution: Added explicit `resolutions` field in root package.json to force Yarn to always use the workspace package via symlink: ```json "resolutions": { "react-on-rails": "link:./packages/react-on-rails" } ``` This ensures that any package in the workspace requesting `react-on-rails` will get a symlink to the local package instead of installing from npm. Verification: - Removed all node_modules and ran `yarn install` - Confirmed packages/react-on-rails-pro/ no longer has node_modules subdirectory - Confirmed yarn.lock now shows `react-on-rails@link:./packages/react-on-rails` - This will fix webpack module resolution in CI example builds 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Master already has react-on-rails@* resolving to 16.1.2 in yarn.lock and still passes CI. The resolutions field was unnecessary and was interfering with test execution. The original webpack module resolution issue appears to be pre-existing or related to other changes in the PR, not a workspace dependency issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - Phase 6: Restructure Ruby Gems to Final Monorepo LayoutOverall AssessmentThis is a well-executed directory restructuring with excellent attention to detail. The PR successfully completes Phase 6 of the monorepo migration by moving Pro Ruby code to be a sibling of the core gem rather than nested. The changes are comprehensive, well-documented, and preserve git history. Strengths1. Comprehensive Path Updates
2. Clear Documentation
3. Proper Git Practices
4. Smart Architecture
CRITICAL Issue FoundLocation: rakelib/release.rake:172 Issue: The Pro version file path still references the old nested structure. The release task will fail when trying to update the Pro version. Current code:
Should be:
Why this matters: The version file has moved from react_on_rails_pro/lib/react_on_rails_pro/version.rb to lib/react_on_rails_pro/version.rb. The release process for Pro gems will break. Impact: High - This will break the release process. Also check line 303 which references pro_gem_root for publishing the gem. Testing RecommendationsBefore merging, please verify:
ConclusionThis is a well-planned and mostly well-executed directory restructuring. The only blocking issue is the incorrect path in release.rake. Once that's fixed, this PR will be in excellent shape. Recommendation: Request changes - fix the release.rake path issue before merging. This follows the guidance in .claude/docs/managing-file-paths.md about verifying all path references after directory structure changes. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rakelib/run_rspec.rake (2)
61-71: Env var handling fordummytask is correct; consider DRY helper.Building
env_vars_arrayand appendingREACT_ON_RAILS_SKIP_VALIDATION=trueplays nicely withrbs_runtime_env_varsandrun_tests_in’s env handling. To reduce duplication withdummy_no_turbolinks, you could extract a small helper (e.g.,build_dummy_rspec_env_vars(*extras)) that includesrbs_runtime_env_varsand joins the extras, and call it from both dummy tasks.Example:
+ def build_dummy_rspec_env_vars(*extras) + env_vars_array = [] + env_vars_array << rbs_runtime_env_vars unless rbs_runtime_env_vars.empty? + env_vars_array.concat(extras) + env_vars_array.join(" ") + end + desc "Runs dummy rspec with turbolinks" task dummy: ["dummy_apps:dummy_app"] do - # Build env vars array for robustness with complex environment variables - env_vars_array = [] - env_vars_array << rbs_runtime_env_vars unless rbs_runtime_env_vars.empty? - # Skip validation since dummy app only has base gem/package but Pro may be available in parent bundle - env_vars_array << "REACT_ON_RAILS_SKIP_VALIDATION=true" - env_vars = env_vars_array.join(" ") + env_vars = build_dummy_rspec_env_vars("REACT_ON_RAILS_SKIP_VALIDATION=true") run_tests_in(spec_dummy_dir, env_vars: env_vars) end
88-94: Skipping validation for example apps is reasonable; verify no example relies on Pro enforcement.Passing
env_vars: "REACT_ON_RAILS_SKIP_VALIDATION=true"to example RSpec runs matches the intent that examples exercise the base gem even when Pro is present in the parent bundle. Please double‑check that none of the shakapacker examples are meant to cover Pro licensing behavior; if they are, those specific tasks should omit the skip flag. Based on learnings, we still want real apps (outside these example tasks) to surface Pro license errors early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
rakelib/run_rspec.rake(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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.
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.
📚 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:
rakelib/run_rspec.rake
📚 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:
rakelib/run_rspec.rake
📚 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:
rakelib/run_rspec.rake
📚 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:
rakelib/run_rspec.rake
📚 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:
rakelib/run_rspec.rake
⏰ 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). (11)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (1)
rakelib/run_rspec.rake (1)
73-85: Aligning skip‑validation across dummy tasks looks good; confirm it stays test‑only.Adding
REACT_ON_RAILS_SKIP_VALIDATION=truehere keepsdummy_no_turbolinksbehavior consistent withdummyand avoids coupling dummy specs to Pro licensing state. Please just confirm that this flag is only honored in test/CI flows and not in real app boot paths, so Pro license checks still fail fast in production. Based on learnings, ensuring license validation runs normally outside these dummy flows is important.
Required by spec/pro/react_on_rails_pro/request_spec.rb for filesystem mocking. The spec uses FakeFS.activate! to mock file operations during testing.
The Pro workflows were failing because of a path confusion issue: - Job-level defaults set working-directory: react_on_rails_pro - Bundle install step explicitly set working-directory: . - This caused gems to install to /vendor/bundle (root) - But cache was configured for react_on_rails_pro/vendor/bundle - Result: gem-release and other development dependencies not found Solution: - Remove job-level defaults.run.working-directory - Add explicit working-directory to each step that needs it - Ensures cache path and install path are aligned - Follows pattern from other workflows (integration-tests.yml) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
PR Review: Phase 6 - Restructure Ruby Gems to Final Monorepo LayoutI've completed a thorough review of this PR. Overall, this is an excellent and well-executed restructuring that completes Phase 6 of the monorepo migration. The code quality is high, and the migration appears to be comprehensive with proper attention to licensing boundaries. ✅ Strengths1. Excellent Architecture & Structure
2. Outstanding Documentation
3. Critical Bug Fixes Identified & ResolvedThe PR description shows excellent debugging work:
4. Thorough CI/CD Updates
5. Test Coverage
|
The pro-gem-tests workflow was failing with: "Could not find gem-release-2.2.2 in locally installed gems (Bundler::GemNotFound)" This occurred because the workflow was missing the `bundle lock --add-platform 'x86_64-linux'` step before installing gems. This is required on Linux CI runners to ensure all platforms are properly included in the lockfile before installation. Added the missing step to match the pattern used in integration-tests.yml (lines that use: bundle lock --add-platform 'x86_64-linux'). This resolves gem resolution errors in CI when running Pro package tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
CRITICAL FIXES for release process: 1. Pro version file path (line 174): - WAS: react_on_rails_pro/lib/react_on_rails_pro/version.rb - NOW: lib/react_on_rails_pro/version.rb - After Phase 6, Pro gem files are at root alongside core gem 2. Pro gem publishing path (line 305): - WAS: publish_gem_with_retry(pro_gem_root, ...) - NOW: publish_gem_with_retry(gem_root, ...) - Pro gemspec is now at repository root 3. Node renderer publishing (line 279-280): - WAS: yarn publish in pro_gem_root directory - NOW: yarn workspace react-on-rails-pro-node-renderer publish - Node renderer is now a workspace package in packages/ 4. Package.json files list (line 191): - WAS: react_on_rails_pro/package.json (doesn't exist) - NOW: packages/react-on-rails-pro-node-renderer/package.json - Matches actual file locations after Phase 6 5. Variable naming clarification (line 173): - Renamed pro_gem_root to pro_dev_dir - Clarifies it's for development infrastructure, not gem publishing - Added comment explaining Phase 6 structure Impact: Without these fixes, the release task would fail when: - Trying to update Pro version file (file not found) - Publishing Pro gem (wrong directory) - Publishing node-renderer (wrong directory) - Updating package.json files (file not found) Testing: Verified paths exist and match actual Phase 6 structure where Pro gem files are at root alongside core gem. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Documentation fixes addressing Phase 6 restructuring feedback: 1. Fix Pro gem path in CONTRIBUTING.md (line 79): - WAS: path: "<React on Rails root>/react_on_rails_pro" - NOW: path: "<React on Rails root>" - After Phase 6, Pro gemspec is at repository root 2. Clarify workspace dependency behavior in testing-build-scripts.md: - Added note explaining yarn.lock behavior with "*" dependencies - Clarified that seeing [email protected] in yarn.lock is normal - Yarn still hoists workspace packages correctly despite cached version Testing completed: ✅ gem build react_on_rails.gemspec - SUCCESS ✅ gem build react_on_rails_pro.gemspec - SUCCESS ✅ rake release[16.2.0.beta.13,true] - SUCCESS (paths all correct) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The previous fix added platform lock but tests still failed with: "Could not find gem-release-2.2.2 in locally installed gems" Root cause: Cache key only included react_on_rails_pro.gemspec, but tests run from root directory where BOTH base and Pro gemspecs are loaded by Bundler. When dependencies change in either gemspec, the cache became stale but wasn't invalidated, causing bundle check to pass while actual gems were missing. Solution: - Changed cache key from react_on_rails_pro.gemspec to Gemfile.lock - This accurately reflects all dependencies loaded when running from root - Bumped cache version from v4 to v5 to force immediate invalidation - Matches caching pattern used in integration-tests.yml Now cache invalidates whenever any gem dependency changes, ensuring bundle install actually runs when needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #2108: Phase 6 Ruby Gems RestructureOverviewThis is a well-executed major refactoring that successfully restructures the monorepo to treat both gems as equal siblings. The changes are comprehensive, methodical, and well-documented. ✅ Strengths1. Excellent Architecture
2. Comprehensive Path Updates
3. Documentation Quality
4. Testing Coverage
|
| Category | Status | Count |
|---|---|---|
| Critical Issues | 🔴 | 1 |
| Medium Issues | 🟡 | 2 |
| Minor Issues | 🟢 | 2 |
| Strengths | ✅ | Multiple |
Verdict: Approve with Minor Fixes
This is excellent work. The one critical issue (brittle relative path) is easy to fix. The medium issues are preventative measures that would improve long-term maintainability.
Required before merge:
- Fix the relative path in
assets_precompile_spec.rb
Recommended before merge:
- Add gemspec file inclusion test
- Clarify RuboCop exclusion comments
Post-merge improvements:
- Consider adding CI validation for gem contents
🎯 Alignment with Project Guidelines
✅ Follows CLAUDE.md monorepo structure guidelines
✅ Preserves git history for moved files
✅ Updates LICENSE.md to reflect new structure
✅ Comprehensive path updates across all config files
✅ Clear documentation of migration impact
✅ Maintains separation between MIT and Pro code
Great job on this refactoring! 🚀
Code Review: Phase 6 Monorepo RestructureI've reviewed this PR and it looks excellent! This is a well-executed architectural refactoring. Here's my detailed feedback: ✅ Strengths1. Excellent Directory StructureThe new layout is much clearer - both gems are now equal siblings instead of parent/child:
This makes the licensing boundaries crystal clear and simplifies mental models. 2. Comprehensive Path UpdatesAll path references were systematically updated:
3. Proper Gem File WhitelistingBoth gemspecs use explicit file whitelisting to ensure clean separation: react_on_rails.gemspec excludes Pro files: f.match(/^react_on_rails_pro/) ||
f.match(%r{^lib/react_on_rails_pro}) ||
f.match(%r{^lib/tasks/(assets_pro|v8_log_processor)\.rake$})react_on_rails_pro.gemspec includes only Pro files: Dir.glob("{lib/react_on_rails_pro.rb,lib/react_on_rails_pro/**/*}")4. LICENSE.md ClarityThe licensing documentation is now extremely clear about what's MIT vs Pro, listing specific directories and files. 5. Git History PreservationAll moves used 6. Smart Rake Task HandlingRenamed
|
The previous fix in PR #2108 added an overly cautious check that prevented Pro gem detection even when properly loaded: return false unless defined?(ReactOnRailsPro::Utils) This caused tests to fail with: "NoMethodError: undefined method \`with_trace' for ReactOnRailsPro::Utils:Module" The issue: After `require "react_on_rails_pro"`, the Utils module IS loaded (confirmed by checking react_on_rails_pro.rb which requires utils.rb), but the check was still failing and returning false prematurely. Solution: Remove the overly cautious Utils check. The existing error handling (rescue NameError, LoadError) is sufficient to catch any constant loading issues. The `require "react_on_rails_pro"` ensures Utils is loaded since react_on_rails_pro.rb explicitly requires it (line 10). This fixes the test failures while maintaining protection against uninitialized constant errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Gemfile.development_dependencies (1)
55-55: Consider pinning the FakeFS version for reproducibility.FakeFS is actively used in the new Pro specs (
spec/pro/react_on_rails_pro/request_spec.rb), where it's required, activated/deactivated, and used for filesystem mocking. However, without a version constraint, the gem will float to the latest version, which may introduce unexpected breaking changes across environments.Apply this diff to pin to a stable version:
- gem "fakefs", require: false + gem "fakefs", "~> 2.5", require: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.claude/docs/testing-build-scripts.md(1 hunks).github/workflows/pro-lint.yml(5 hunks).github/workflows/pro-test-package-and-gem.yml(9 hunks)CONTRIBUTING.md(2 hunks)Gemfile.development_dependencies(1 hunks)lib/react_on_rails/utils.rb(1 hunks)rakelib/release.rake(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/react_on_rails/utils.rb
- .github/workflows/pro-test-package-and-gem.yml
- rakelib/release.rake
- CONTRIBUTING.md
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
.claude/docs/testing-build-scripts.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:
.claude/docs/testing-build-scripts.md.github/workflows/pro-lint.yml
📚 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:
.claude/docs/testing-build-scripts.md.github/workflows/pro-lint.yml
📚 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:
.github/workflows/pro-lint.yml
📚 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:
.github/workflows/pro-lint.yml
📚 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:
.github/workflows/pro-lint.yml
⏰ 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). (11)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- 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: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (3)
.claude/docs/testing-build-scripts.md (1)
140-145: Improved clarity on Yarn Classic workspace hoisting behavior.The terminology shift from "links" to "hoists" more accurately describes how Yarn Classic v1.x resolves workspace dependencies. The added Note clarifies that
react-on-rails@*with a resolved version in yarn.lock is expected and doesn't indicate a problem—the workspace package is hoisted to rootnode_moduleswithout its own subfolder, which can surprise developers unfamiliar with workspace semantics.This documentation enhancement directly supports the monorepo restructuring by clarifying potential confusion points during workspace dependency resolution, especially relevant given the Phase 6 Pro gem sibling layout changes.
.github/workflows/pro-lint.yml (2)
112-143: Verify gem cache and bundler configuration align with new Pro gemspec location.With the gemspec now at repo root and gems cached in
vendor/bundle(also at root), the bundle configuration needs to be consistent across the workflow. Thebundle config set --local path 'vendor/bundle'at line 141 is correct, but verify that the Pro dummy app bundle install (lines 152–156) doesn't conflict with or override this config when running fromreact_on_rails_pro/spec/dummy/.Consider checking:
- Whether
bundle config set --localcreates.bundle/configat repo root and whether the dummy app respects that parent config or creates its own.- Whether the cache keys and paths remain isolated (Pro package gems at
vendor/bundlevs. dummy app gems atreact_on_rails_pro/spec/dummy/vendor/bundle).You can verify the gem installation paths and bundler config behavior by examining the actual cache directories after a successful workflow run or running a local test.
175-176: No issues found with RuboCop path configuration.The
spec/prodirectory structure confirms that lintingspec/prois correct and intentional. The directory containsempty_spec.rbat the root level plus all Pro specs underspec/pro/react_on_rails_pro/. The RuboCop configuration appropriately captures all Pro-related spec files without inadvertently linting unrelated content.
Problem Confirmed: Yarn Classic was installing [email protected] from npm registry into packages/react-on-rails-pro/node_modules/, even though Pro package specifies "react-on-rails": "*" which should use workspace hoisting. This caused webpack module resolution failures in CI: ERROR: Package path ./pageLifecycle is not exported from package /packages/react-on-rails-pro/node_modules/react-on-rails Root Cause: yarn.lock had cached resolution: react-on-rails@*: version "16.1.2" This resolution persisted even after clean installs, causing Yarn to: 1. Install 16.1.2 from npm into Pro package's node_modules 2. The 16.1.2 version lacks the new exports field (./pageLifecycle, ./context, etc.) 3. Webpack fails when Pro package tries to import these subpaths Solution: Add explicit resolutions field to force workspace linking: "resolutions": { "react-on-rails": "link:./packages/react-on-rails" } This tells Yarn to ALWAYS use a symlink to the local workspace package, bypassing the cached npm resolution in yarn.lock. Verification: ✅ Local testing: packages/react-on-rails-pro/ has no node_modules subfolder ✅ yarn.lock now shows: react-on-rails@link:./packages/react-on-rails ✅ This matches the fix attempted earlier but reverted prematurely Previous attempt (commit 34b7a1f) was reverted (commit 6dd869b) based on assumption it caused test failures. However, those failures may have been pre-existing or from other Phase 6 changes. Waiting for CI to confirm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
CI Error: Could not find gem-release-2.2.2 in locally installed gems (Bundler::GemNotFound) Root Cause: Gemfile.lock specified gem-release 2.2.2, but CI environment had a different version installed, causing Bundler to fail when trying to exec rspec. Solution: Run bundle update gem-release to update Gemfile.lock to 2.2.4. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Closing this PR. After review, the file movement complexity is causing too many issues. We're going to explore a simpler approach with two top-level directories instead. See new plan coming soon. |
Summary
Completes Phase 6 of the monorepo migration by restructuring the directory layout so
react_on_rails_prois a sibling structure instead of a nested subdirectory.Status: ✅ Ready for review
Background
Previously, the Pro Ruby gem lived in a confusing nested structure:
This PR restructures it so both gems are equal siblings:
Major Changes
Directory Moves
react_on_rails_pro/lib/react_on_rails_pro/→lib/react_on_rails_pro/react_on_rails_pro/spec/react_on_rails_pro/→spec/pro/react_on_rails_pro/react_on_rails_pro/react_on_rails_pro.gemspec→react_on_rails_pro.gemspec(root)react_on_rails_pro/CHANGELOG.md→CHANGELOG_PRO.md(root)react_on_rails_pro/lib/react_on_rails_pro.rb→lib/react_on_rails_pro.rbreact_on_rails_pro/lib/tasks/→lib/tasks/(assets.rake renamed to assets_pro.rake)Configuration Updates
react_on_rails_pro.gemspecto referencelib/react_on_rails/versionfrom new locationlib/react_on_rails_pro/,spec/pro/, and root gemspecscript/ci-changes-detectorpatterns for new pathsrakelib/release.raketo reference new pathsPath Fixes
Updated require_relative paths in:
spec/pro/react_on_rails_pro/assets_precompile_spec.rbreact_on_rails_pro/spec/dummy/spec/rails_helper.rbWhat Remained
The
react_on_rails_pro/directory still exists with:spec/dummy/- Pro dummy appspec/execjs-compatible-dummy/- Alternative test dummyThis is intentional and properly documented in LICENSE.md.
Benefits
Test Plan
gem build react_on_rails.gemspecandgem build react_on_rails_pro.gemspec)Migration Impact
Users:
Contributors:
bundle installandyarn installafter pullingRelated
docs/MONOREPO_MERGER_PLAN.mdfor full migration planChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
CI / Workflows
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.