Move React/Shakapacker version compatibility to generator smoke tests #2125
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 12 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCI and generator tooling split example testing into "latest" and "minimum/pinned" flows; example generators and rake tasks now support per-example React/Shakapacker pinning, apply package/Gemfile edits, run installs/generation in the generated app context (optionally unbundled), then run tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Workflow
participant Repo as Repository
participant Rake as Rake Tasks
participant Generator as Example Generator
participant Shell as Shell / Bundler / npm
participant App as Generated Example App
CI->>Repo: Trigger examples workflow with dependency-level
alt dependency-level == latest
CI->>Rake: invoke shakapacker_examples_latest
Rake->>Generator: select examples (not pinned)
Generator->>Shell: generate example app (bundled context)
Shell->>App: bundle install
Shell->>App: npm install
Shell->>App: run rspec (bundled)
App-->>CI: return test results
else dependency-level == minimum
CI->>Rake: invoke shakapacker_examples_pinned
Rake->>Generator: select pinned/react-version examples
Generator->>Generator: apply_react_version (patch package.json & Gemfile)
Generator->>Shell: bundle install (app)
Shell->>App: npm install --legacy-peer-deps
Generator->>Shell: run generate_packs via unbundled execution
Generator->>Shell: run rspec via unbundled execution
App-->>CI: return test results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 - PR #2125: Move React/Shakapacker version compatibility to generator smoke tests✅ Overall AssessmentThis is a well-structured refactoring that improves the testing strategy by separating version compatibility testing from integration tests. The approach is sound and aligns with the original issue #2123 objectives. 🎯 Strengths1. Clear Separation of Concerns
2. Cleaner CI Configuration
3. Better Real-World Representation
4. Code Quality
|
Code Review - PR #2125OverviewThis PR successfully refactors React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. The approach is clean and improves the separation of concerns. Overall, this is well-structured with good benefits. ✅ Strengths
|
Code Review - PR #2125I've reviewed the changes for moving React/Shakapacker version compatibility testing to generator smoke tests. Overall, this is a well-structured refactoring that improves the separation of concerns. Here's my detailed feedback: ✅ Strengths
🔍 Observations & Suggestions1. Testing CoverageThe PR description mentions:
Suggestion: Before merging, verify the CI workflow runs as expected. Consider testing locally with: # Simulate the CI environment
cd react_on_rails
bundle exec rake run_rspec:shakapacker_examples_latest
bundle exec rake run_rspec:shakapacker_examples_minimum2. Potential Edge Case in
|
Code Review - PR #2125SummaryThis PR successfully moves React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. The overall approach is sound and aligns well with the project's testing strategy. ✅ Strengths
🔍 Issues & Concerns1. CRITICAL: Missing attr_reader and minimum_versions? methodIn attr_reader :packer_type, :name, :generator_options, :minimum_versionsBut the current file only has: attr_reader :packer_type, :name, :generator_optionsThe
Required fix: attr_reader :packer_type, :name, :generator_options, :minimum_versions
def initialize(packer_type: nil, name: nil, generator_options: nil, minimum_versions: false)
@packer_type = packer_type
@name = name
@generator_options = generator_options
@minimum_versions = minimum_versions
self.class.all[packer_type.to_sym] << self
end
def minimum_versions?
@minimum_versions
end2. Missing run_rspec.rake changesThe PR diff shows new rake tasks should be added to
These changes are not in the current file. Without these:
Required fix: Add the rake tasks as shown in the PR diff. 3. Missing shakapacker_examples.rake changesThe PR diff shows a new
This code is not present in the current file. Required fix: Add the 4. Missing examples_config.yml entriesThe current # Missing from current file:
- name: basic-minimum
generator_options: ''
minimum_versions: true
- name: basic-server-rendering-minimum
generator_options: --example-server-rendering
minimum_versions: trueWithout these entries, no minimum version examples will be generated. 🐛 Potential Bugs5. Predicate method naming inconsistencyIn def latest_examples
ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
end
def minimum_examples
ExampleType.all[:shakapacker_examples].select(&:minimum_versions)
endThis calls
The symbol-to-proc syntax 6. JSON formatting in apply_minimum_versionsThe PR adds: File.write(package_json_path, "\#{JSON.pretty_generate(package_json)}\n")Good: This preserves consistent formatting with a trailing newline (as required by CLAUDE.md). 📝 Code Quality7. Constants placement is goodThe minimum version constants are well-placed: MINIMUM_REACT_VERSION = "18.0.0"
MINIMUM_SHAKAPACKER_VERSION = "8.2.0"These make it easy to update supported versions in the future. 8. Script/convert simplificationThe removal of React/Shakapacker logic from ⚡ Performance ConsiderationsNo performance concerns. The changes are primarily organizational and don't introduce performance overhead. 🔒 Security ConcernsNo security issues identified. The changes involve:
All are low-risk from a security perspective. 🧪 Test CoverageMissing verification:According to the PR description test plan:
The last item is unchecked, but it's the most critical one! Before merging, you should:
🎯 RecommendationsMust fix before merge:
Nice to have:
📋 Checklist for AuthorBefore marking as ready to merge:
💡 Overall AssessmentArchitecture: Excellent - The separation of concerns is much cleaner. Implementation: Incomplete - Several critical code changes are missing. This appears to be an issue with the PR branch not containing all the changes shown in the diff, or the diff was generated from a different state than what's currently in the branch. Recommendation: Request changes - The PR cannot be merged until all the missing code is added and tested locally. Great work on the design! Just need to ensure all the implementation is present. 🚀 |
Code Review - PR #2125Great work on separating version compatibility testing from spec/dummy! This is a cleaner architecture. Here's my detailed review: ✅ Strengths
🔍 Issues & Suggestions1. Missing
|
Code Review - PR #2125Overall Assessment: This is a well-structured refactoring that improves the separation of concerns between spec/dummy integration tests and generator smoke tests. The approach is cleaner and more maintainable. ✅ ✅ Strengths
🔍 Issues & Concerns1. Potential Race Condition in Rake Task (CRITICAL)Location: def latest_examples
ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
end
def minimum_examples
ExampleType.all[:shakapacker_examples].select(&:minimum_versions)
endProblem: These helper methods are defined at the top level of the Recommendation: Move the filtering logic into the task bodies or use lambdas: desc "Runs Rspec for latest version example apps only"
task shakapacker_examples_latest: "shakapacker_examples:gen_all" do
latest_examples = ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
latest_examples.each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
endOr ensure 2. Silent Failure Risk in
|
Code Review - PR #2125SummaryThis PR successfully moves React and Shakapacker version compatibility testing from spec/dummy to generator smoke tests. The approach is sound and aligns well with the suggested improvements from PR #2114. ✅ Strengths1. Clear Separation of ConcernsThe PR correctly separates:
This makes the testing strategy much clearer and easier to maintain. 2. Good Architectural Design
3. Simplified CI Logic
4. Proper Version Updates
🔍 Observations & Questions1. Task Invocation Pattern (Minor)In task shakapacker_examples: "shakapacker_examples:gen_all" do
ExampleType.all[:shakapacker_examples].each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
endThis runs ALL examples (both latest and minimum). Is this intentional? The new tasks Recommendation: Consider whether
2. Dependency Generation OrderIn sh_in_dir(example_type.dir, generator_commands)
# Apply minimum versions for compatibility testing examples
apply_minimum_versions(example_type.dir) if example_type.minimum_versions
sh_in_dir(example_type.dir, "npm install")The 3. Error Handling in JSON ParsingThe rescue JSON::ParserError => e
puts " ERROR: Failed to parse package.json in #{dir}: #{e.message}"
raise
endGood defensive programming! ✅ 4. Workflow Step NamesThe workflow step names were updated from:
To:
This is a good clarification! ✅ 🔒 Security & Performance
📋 Test CoverageBased on the PR description, the following are marked as complete:
Recommendation: Once CI passes, verify that:
🎯 Overall AssessmentLGTM with minor clarification needed on the This is a well-structured PR that:
RecommendationOnce CI passes and the question about 🤖 Generated with Claude Code |
Code Review - PR #2125✅ Overall AssessmentThis is a well-designed refactoring that improves the test architecture by separating concerns: spec/dummy now tests latest versions, while generators test the compatibility matrix. All CI checks are passing. Strengths1. Excellent Separation of Concerns ✨
2. Clean Implementation
3. Improved CI Organization
4. Good Documentation
Issues & Recommendations
|
Code Review for PR #2125SummaryThis PR successfully refactors version compatibility testing from spec/dummy to dedicated generator smoke test examples. The approach is clean and follows the project's architecture well. ✅ Strengths
🔍 Issues Found1. CRITICAL: Missing Implementation in
|
Review Comments AddressedThank you for the thorough reviews! All the issues mentioned have been addressed in the commits on this branch: ✅ All Critical Issues Resolved
Files Ready for Review
All RuboCop violations have been fixed and the code follows project conventions. |
Code Review: Move React/Shakapacker version compatibility to generator smoke testsOverall Assessment✅ Strong refactoring - This PR successfully separates concerns by moving version compatibility testing from spec/dummy to generator smoke tests. The approach is cleaner and more maintainable. Code Quality & Best Practices✅ Excellent Structure
✅ Helper Methodsdef latest_examples
ExampleType.all[:shakapacker_examples].reject(&:minimum_versions?)
end
def minimum_examples
ExampleType.all[:shakapacker_examples].select(&:minimum_versions?)
endThese filtering methods (lines 94-100 in run_rspec.rake) are clean and readable. 💡 Minor Suggestion - Method LocationThe helper methods
Potential Issues
|
![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)
package.json (1)
31-32: Update@types/reactand@types/react-domto v19 to match React 19 runtime.TypeScript types must be updated when upgrading to React 19. Install @types/react@^19.0.0 and @types/react-dom@^19.0.0. React 19 includes significant TypeScript type changes, with removed types and cleanups that require matching type definitions.
Mismatched versions (v18 types with v19 runtime) prevent builds and cause errors like "It looks like you're trying to use TypeScript but do not have the required package(s) installed." Update lines 31-32 in
package.jsonand align with the correct versions already used inspec/dummy/package.json.
🧹 Nitpick comments (2)
script/convert (1)
15-21: Fallback path logic only handles Gemfile, not package.json.The fallback from
/react_on_rails/Gemfileto/Gemfile(line 19) doesn't apply to the package.json paths used throughout this script. If backward compatibility with the old repository layout is needed for package.json files, consider generalizing the pattern:- old_path = path.sub(%r{/react_on_rails/Gemfile}, "/Gemfile") + # Try removing react_on_rails/ prefix for old layout compatibility + old_path = path.sub(%r{/react_on_rails/}, "/")Alternatively, if the Gemfile fallback is no longer needed, the entire block could be removed to simplify the code.
react_on_rails/rakelib/run_rspec.rake (1)
93-100: Consider moving helper methods earlier in the namespace for better organization.The helper methods work correctly, using
rejectandselectwith theminimum_versions?predicate to filter examples. However, for better readability, consider moving these helpers to the top of the namespace block (after line 12) before they're used.namespace :run_rspec do include ReactOnRails::TaskHelpers + + # Helper methods for filtering examples + def latest_examples + ExampleType.all[:shakapacker_examples].reject(&:minimum_versions?) + end + + def minimum_examples + ExampleType.all[:shakapacker_examples].select(&:minimum_versions?) + end # Loads data from examples_config.yml and instantiates corresponding ExampleType objects examples_config_file = File.expand_path("examples_config.yml", __dir__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
react_on_rails/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.github/workflows/examples.yml(2 hunks).github/workflows/integration-tests.yml(2 hunks)package.json(1 hunks)react_on_rails/rakelib/example_type.rb(1 hunks)react_on_rails/rakelib/examples_config.yml(1 hunks)react_on_rails/rakelib/run_rspec.rake(1 hunks)react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)react_on_rails/spec/dummy/package.json(1 hunks)script/convert(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: CRITICAL: Ensure all files end with a newline character
When making changes to directory structure or file paths, run comprehensive grep searches to find all affected files before committing:grep -r 'old/path' . --exclude-dir=node_modules --exclude-dir=.git
Files:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/run_rspec.rakescript/convert
**/*.{js,jsx,ts,tsx,json,yaml,yml,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER manually format code - let Prettier and RuboCop handle ALL formatting automatically via
rake autofix
Files:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.yml
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Never run
npmcommands - only use equivalent Yarn Classic commands
Files:
package.json
.github/workflows/*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying CI workflows, test all changes locally first via manual execution of equivalent commands before pushing - CI iteration is expensive (10-30 min/cycle)
Files:
.github/workflows/integration-tests.yml.github/workflows/examples.yml
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
CRITICAL: Always run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/example_type.rb
🧠 Learnings (18)
📓 Common learnings
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.
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to react_on_rails_pro/**/*.{js,jsx,ts,tsx,json,yaml,yml,css,scss} : The react_on_rails_pro directory has its own Prettier and ESLint configuration - CI lints both directories separately
Applied to files:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to packages/react-on-rails/src/**/*.{ts,tsx} : TypeScript files in the npm package must be compiled to JavaScript in `packages/react-on-rails/lib/` via `yarn run build`
Applied to files:
package.json.github/workflows/integration-tests.ymlreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/run_rspec.rakescript/convert
📚 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:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/example_type.rbscript/convert
📚 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:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/example_type.rbscript/convert
📚 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:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to spec/react_on_rails/**/*.rb : Ruby unit tests should be located in `spec/react_on_rails/` and tested via `rake run_rspec:gem`
Applied to files:
package.jsonreact_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/run_rspec.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Use `yalc` for local development when testing React on Rails with external applications - this allows local package linking without publishing to npm
Applied to files:
package.jsonreact_on_rails/spec/dummy/package.json
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: CRITICAL: For infrastructure/config changes affecting directory structure, build configs, or CI workflows, perform comprehensive local testing before pushing including: grep for affected files, test build pipeline, run relevant specs, and run full lint check
Applied to files:
.github/workflows/integration-tests.ymlreact_on_rails/rakelib/examples_config.yml.github/workflows/examples.ymlscript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to .github/workflows/*.yml : When modifying CI workflows, test all changes locally first via manual execution of equivalent commands before pushing - CI iteration is expensive (10-30 min/cycle)
Applied to files:
.github/workflows/integration-tests.yml.github/workflows/examples.yml
📚 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:
react_on_rails/spec/dummy/package.jsonreact_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to lib/react_on_rails/**/*.rb : When creating new Ruby files in `lib/react_on_rails/`, create corresponding RBS signature file in `sig/react_on_rails/` and add to Steepfile for type checking
Applied to files:
react_on_rails/rakelib/examples_config.ymlreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/example_type.rbscript/convert
📚 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/rakelib/examples_config.ymlscript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to lib/generators/react_on_rails/**/*.rb : Rails generators run in host app context and should not assume specific project structure (e.g., `app/javascript/` may not exist) - use `Rails.root` for path resolution
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,yaml,yml,css,scss} : NEVER manually format code - let Prettier and RuboCop handle ALL formatting automatically via `rake autofix`
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: React on Rails is a Rails Engine - automatically loads all `.rake` files from `lib/tasks/` directory without explicit loading via `rake_tasks` block to avoid duplicate task execution
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to lib/tasks/*.rake : Rake tasks in lib/tasks/ are automatically loaded by Rails::Engine - verify tasks don't cause duplicate execution during `rake assets:precompile`
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T04:54:35.286Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T04:54:35.286Z
Learning: Applies to spec/dummy/**/*.rb : Integration tests in the dummy app should be located in `spec/dummy/` and tested via `rake run_rspec:dummy`
Applied to files:
react_on_rails/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). (7)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- 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 (11)
react_on_rails/rakelib/example_type.rb (1)
16-31: LGTM!Clean implementation following Ruby conventions:
- Constants are appropriately scoped within the class
- Predicate method
minimum_versions?follows Ruby idioms- Default parameter
minimum_versions: falsemaintains backward compatibilityscript/convert (1)
40-51: LGTM!Appropriately skips RSC-related tests when running on React 18, as RSC requires React 19. The
exit 0no-op fortest:rscis a clean approach..github/workflows/integration-tests.yml (2)
135-137: LGTM!Step name accurately reflects the updated purpose of
script/convert(Node.js tooling compatibility rather than Shakapacker v6 support).
214-216: Consistent rename with the other workflow step.react_on_rails/rakelib/shakapacker_examples.rake (3)
11-11: LGTM!The
require "json"statement is necessary for the newapply_minimum_versionsmethod that parses and writes package.json files.
82-83: LGTM!The conditional invocation of
apply_minimum_versionscorrectly applies minimum version constraints only to examples marked withminimum_versions: true.
19-48: All dependencies verified and confirmed in place.The verification confirms:
minimum_versions?predicate is properly defined inExampleType(line 23 in example_type.rb)- Constants
MINIMUM_REACT_VERSION("18.0.0") andMINIMUM_SHAKAPACKER_VERSION("8.2.0") are defined (lines 17-18 in example_type.rb)- The
apply_minimum_versionsmethod correctly uses these constants to update package.json filesThe method logic is sound with appropriate error handling and safe navigation checks.
.github/workflows/examples.yml (2)
133-133: LGTM!The step name update accurately reflects the script's broader purpose of handling Node.js tooling compatibility.
175-180: Conditional CI workflow changes verified and correct.The conditional steps properly split testing based on
matrix.dependency-level. Verification confirms:
- Both rake tasks exist in
react_on_rails/rakelib/run_rspec.rake(lines 103, 108)- Matrix configuration properly defines
dependency-levelvalues as 'latest' and 'minimum' (line 81)- Conditional syntax correctly matches matrix values
- Environment setup responds appropriately to dependency-level (BUNDLE_FROZEN, line 105)
react_on_rails/rakelib/examples_config.yml (1)
10-16: LGTM!The new minimum-version test configurations are well-structured and clearly documented. The
minimum_versions: trueflag properly integrates with the new testing infrastructure to verify compatibility with React 18.0.0 and Shakapacker 8.2.0.react_on_rails/rakelib/run_rspec.rake (1)
102-110: LGTM!The new rake tasks properly separate latest and minimum version testing. The task structure is consistent with existing patterns, correctly setting up dependencies on generation tasks and invoking RSpec for the appropriate example subsets.
Code ReviewThis is a well-structured PR that successfully moves React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. The approach is clean and maintainable. Here's my detailed feedback: ✅ Strengths
🔍 Potential Issues1. Critical: Missing RuboCop validationPer CLAUDE.md requirements, you MUST run def apply_minimum_versions(dir) # rubocop:disable Metrics/CyclomaticComplexityThis suggests the method may have complexity issues. Consider:
2. Error handling incompletenessIn File.write(package_json_path, "\#{JSON.pretty_generate(package_json)}\n")This should have error handling for write failures: begin
File.write(package_json_path, "\#{JSON.pretty_generate(package_json)}\n")
puts " Updated package.json with minimum versions:"
# ...
rescue Errno::EACCES, Errno::ENOENT => e
puts " ERROR: Failed to write package.json: \#{e.message}"
raise
end3. Missing test coverage validationPer CLAUDE.md's "CRITICAL - LOCAL TESTING REQUIREMENTS", before claiming tests are fixed:
The test plan shows checkboxes for rake task verification but not for actual test execution. 4. Potential race condition in CIIn - name: Main CI - Latest version examples
if: matrix.dependency-level == 'latest'
run: cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_latest
- name: Main CI - Minimum version examples
if: matrix.dependency-level == 'minimum'
run: cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_minimumBoth steps run unconditionally based on matrix values. This is correct, but consider adding a validation step to ensure the examples were generated with the correct versions before running tests. 💡 Suggestions for Improvement1. Add version verificationConsider adding a verification step in the rake task: def verify_minimum_versions(dir)
package_json = JSON.parse(File.read(File.join(dir, "package.json")))
react_version = package_json.dig("dependencies", "react")
shakapacker_version = package_json.dig("devDependencies", "shakapacker") ||
package_json.dig("dependencies", "shakapacker")
unless react_version == MINIMUM_REACT_VERSION && shakapacker_version == MINIMUM_SHAKAPACKER_VERSION
raise "Version mismatch in \#{dir}: React=\#{react_version}, Shakapacker=\#{shakapacker_version}"
end
endCall this after 2. Document the script/convert changesThe script/convert file removes a lot of code. Consider adding a comment or git commit message explaining that React/Shakapacker conversion is now handled by generator examples, not by script/convert. 3. Constants visibilityThe constants
|
Addressing Review Comment #3573928031Thank you for the detailed review! I've addressed the concerns and also made critical React 19 compatibility updates to the spec/dummy app. ✅ Review Concerns Addressed1. Clarification: spec/dummy in minimum CIGood catch! You're right to ask about this. The spec/dummy integration tests should only run with latest versions (React 19, Shakapacker 9.4.0). The minimum CI workflow only runs the generator example tasks ( 2. JSON Parsing Error HandlingThe begin
package_json = JSON.parse(File.read(package_json_path))
rescue JSON::ParserError => e
puts " ERROR: Failed to parse package.json in \#{dir}: \#{e.message}"
raise
endFail-fast behavior is correct for CI - we want to know immediately if generated examples have malformed package.json. 3. Documentation AddedAdded comment in # Minimum version compatibility tests - tests React 18 and Shakapacker 8.2.0
- name: basic-minimum
generator_options: ''
minimum_versions: true🚀 React 19 Compatibility UpdatesI've also updated the spec/dummy app to be fully compatible with React 19, addressing peer dependency issues: Dependencies Updated in
|
| Package | Old Version | New Version | Reason |
|---|---|---|---|
react-redux |
^8.0.2 | ^9.2.0 | React 19 support |
redux |
^4.0.1 | ^5.0.1 | Required by react-redux 9.x |
redux-thunk |
^2.2.0 | ^3.1.0 | Required by redux 5.x |
react-helmet |
^6.1.0 | Replaced with @dr.pogodin/react-helmet@^3.0.4 |
Thread-safe React 19 fork |
Code Changes:
-
redux-thunk import changes: Updated from default export to named export:
import middleware from 'redux-thunk'→import { thunk } from 'redux-thunk'- Files updated:
SharedReduxStore.jsx,ReduxApp.server.jsx,ReduxApp.client.jsx,app-react16/ReduxApp.client.jsx
-
react-helmet migration: Updated to
@dr.pogodin/react-helmetwhich is the React 19 compatible fork:- Uses
HelmetProviderwithcontextprop for SSR (thread-safe) - Updated
ReactHelmetApp.server.jsxandReactHelmetAppBroken.server.jsxwith new SSR pattern - Updated
ReactHelmet.jsxcomponent
- Uses
Verification:
- ✅
yarn installcompletes without peer dependency warnings (only unrelated babel-eslint warning) - ✅
yarn run build:testcompletes successfully - ✅ RuboCop passes with no offenses
Source
The react-helmet migration follows the official Dr. Pogodin React Helmet documentation for server-side rendering with context-based data capture.
Code Review - PR #2125Thank you for this well-structured refactoring! This PR successfully moves React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. ✅ StrengthsArchitecture & Design
Implementation Quality
🔍 Suggestions & Questions1. RuboCop Disable - Is It Necessary?def apply_minimum_versions(dir) # rubocop:disable Metrics/CyclomaticComplexityThis method appears relatively simple (early return, JSON parse, 2-3 conditionals). Have you verified this disable is actually needed? Try removing it and running RuboCop. 2. JSON FormattingFile.write(package_json_path, "\#{JSON.pretty_generate(package_json)}\n")Good that you're adding the trailing newline! Note that this will reformat the entire package.json (key ordering, spacing). Consider documenting this behavior in a comment if it's intentional. 3. Error Handling EnhancementConsider handling rescue JSON::ParserError => e
puts " ERROR: Invalid JSON in \#{package_json_path}: \#{e.message}"
raise
rescue Errno::ENOENT
puts " ERROR: package.json not found at \#{package_json_path}"
raise4. CI Workflow Edge CaseWhat if 5. Test CoverageAre there tests verifying:
6. DocumentationThe comment in 🧪 Testing Recommendations# Verify rake tasks exist
bundle exec rake -T | grep shakapacker_examples
# Test latest examples
bundle exec rake shakapacker_examples:gen_basic
cat gen-examples/basic/package.json | grep '"react"' # Should be ^19.0.0
# Test minimum examples
bundle exec rake shakapacker_examples:gen_basic-minimum
cat gen-examples/basic-minimum/package.json | grep '"react"' # Should be 18.0.0
# Verify filtering
bundle exec rake run_rspec:shakapacker_examples_latest
bundle exec rake run_rspec:shakapacker_examples_minimum🏁 VerdictOverall: ✅ Approve This is a well-executed refactoring. The architectural approach is sound, implementation is clean, and CI is passing. The suggestions above are minor improvements, not blockers. Great work! 🚀 This approach is much more maintainable than modifying spec/dummy on the fly. Review generated by Claude Code following CLAUDE.md conventions |
Code Review - PR #2125OverviewThis PR successfully moves React and Shakapacker version compatibility testing from the dummy app to dedicated generator smoke tests. The approach is well-architected and aligns with best practices for testing compatibility matrices. ✅ Strengths1. Clear Separation of Concerns
2. Excellent Code Quality
3. CI Integration
4. Helper Methods Are Well Structured def latest_examples
ExampleType.all[:shakapacker_examples].reject(&:minimum_versions?)
end
def minimum_examples
ExampleType.all[:shakapacker_examples].select(&:minimum_versions?)
endThese are readable and leverage Ruby's functional collection methods appropriately. 🔍 Areas for Consideration1. Missing Cleanup in The Current state: # Lines 30-31 - Should these be removed?
gsub_file_content("../react_on_rails/Gemfile.development_dependencies", /gem "shakapacker", "[^"]*"/, 'gem "shakapacker", "8.2.0"')
gsub_file_content("../react_on_rails/spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "8.2.0",')
# Lines 45-49 - Should these be removed?
gsub_file_content("../package.json", /"react": "[^"]*",/, '"react": "18.0.0",')
gsub_file_content("../package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",')
gsub_file_content("../react_on_rails/spec/dummy/package.json", /"react": "[^"]*",/, '"react": "18.0.0",')
gsub_file_content("../react_on_rails/spec/dummy/package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",')Expected: Based on the PR description, 2. Potential Complexity in The method has reasonable complexity (RuboCop disabled with def apply_minimum_versions(dir)
# ...
if package_json["dependencies"]
package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION
end
if package_json["devDependencies"]&.key?("shakapacker")
package_json["devDependencies"]["shakapacker"] = ExampleType::MINIMUM_SHAKAPACKER_VERSION
elsif package_json["dependencies"]&.key?("shakapacker")
package_json["dependencies"]["shakapacker"] = ExampleType::MINIMUM_SHAKAPACKER_VERSION
end
# ...
endSuggestions:
3. Test Coverage Gaps The test plan in the PR description shows:
Question: Will you be monitoring the CI run to ensure the minimum version examples actually run and pass? This is critical validation that the approach works end-to-end. 4. Documentation Consider adding:
🛡️ Security & PerformanceSecurity: ✅ No security concerns. The JSON parsing includes error handling. Performance: ✅ The filtering operations are performed on small arrays (handful of example types), so performance impact is negligible. 🧪 Testing RecommendationsBefore merging, verify:
📋 Minor Suggestions
🎯 RecommendationApprove with minor revisions This is a solid architectural improvement that makes the codebase more maintainable. The only blocking concern is confirming that All other suggestions are optional refinements. The core logic is sound, well-tested, and follows Ruby best practices. Great work on this refactoring! 🚀 Related: Closes #2123 |
Code ReviewOverall Assessment ✅This is a well-structured PR that successfully moves React/Shakapacker version compatibility testing from spec/dummy to the generator smoke tests. The approach is sound and the implementation is clean. CI is passing successfully. Strengths 💪
Code Quality Issues 🔍1. Missing Implementation in
|
Code Review - PR #2125: Move React/Shakapacker version compatibility to generator smoke testsOverviewThis PR successfully refactors version compatibility testing from spec/dummy to generator smoke tests. The approach is clean and well-architected. Strengths1. Clean Architecture
2. Smart Implementation
3. Simplified Conversion Script
Issues and RecommendationsCRITICAL: Missing Test CoverageProblem: No tests verify the new rake tasks work correctly. Recommendation: Add tests to verify latest_examples and minimum_examples filtering. Why critical: The PR checklist shows "Verify rake tasks are created correctly" but this should be automated, not manual. MEDIUM: CI Workflow TestingIssue: From examples.yml lines 175-179 - conditional execution based on dependency-level Recommendation per CLAUDE.md:
MEDIUM: Potential Race ConditionIssue in shakapacker_examples.rake:56-58 - Modifies package.json AFTER generation but BEFORE npm install Recommendation: Add a comment explaining the ordering to prevent future refactoring issues LOW: Missing RuboCop VerificationFrom CLAUDE.md: ALWAYS run bundle exec rubocop before commit Recommendation: Run locally before merge Security and PerformanceSecurity: No concerns
Performance: Acceptable
Testing ChecklistPer CLAUDE.md "15-Minute Rule": spend 15 minutes testing locally before pushing Before merging, verify:
Recommendations SummaryBefore Merge (Required)
Future Improvements (Optional)
ConclusionOverall: Strong PR with good architecture. Main concern is lack of automated tests for new rake tasks and missing local verification before CI runs. Recommendation: APPROVE with conditions
Great work on the refactoring! The separation of concerns is much cleaner now. |
Code Review: PR #2125Great work on this refactor! This is a well-thought-out improvement that simplifies the testing strategy. Here's my detailed feedback: ✅ Strengths
🔍 Potential Issues & Suggestions1. Missing
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
react_on_rails/spec/dummy/package.json (1)
21-28: LGTM — React 19 ecosystem dependencies updated correctly.All dependencies now support React 19:
- react-redux upgraded to ^9.2.0 (adds React 19 peer dependency support)
- @dr.pogodin/react-helmet ^3.0.4 replaces react-helmet (React 19 compatible)
- redux ^5.0.1 and redux-thunk ^3.1.0 are current stable versions
This addresses the compatibility concerns raised in the previous review.
🧹 Nitpick comments (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx (1)
31-31: Use optional chaining for safer property access.The current fallback only guards against
helmetbeing undefined. Ifhelmetexists buthelmet.titleis undefined,helmet.title.toString()would throw a TypeError.Apply this diff for more defensive access:
- title: helmet ? helmet.title.toString() : '', + title: helmet?.title?.toString() || '',This ensures the code gracefully handles cases where either
helmetorhelmet.titlemight be undefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
react_on_rails/spec/dummy/Gemfile.lockis excluded by!**/*.lockreact_on_rails/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx(2 hunks)react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx(2 hunks)react_on_rails/spec/dummy/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxreact_on_rails/spec/dummy/package.jsonreact_on_rails/spec/dummy/client/app/components/ReactHelmet.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
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.
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.
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxreact_on_rails/spec/dummy/package.jsonreact_on_rails/spec/dummy/client/app/components/ReactHelmet.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxreact_on_rails/spec/dummy/client/app/components/ReactHelmet.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxreact_on_rails/spec/dummy/package.jsonreact_on_rails/spec/dummy/client/app/components/ReactHelmet.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsxreact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxreact_on_rails/spec/dummy/package.jsonreact_on_rails/spec/dummy/client/app/components/ReactHelmet.jsxreact_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/package.jsonreact_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/spec/dummy/package.json
📚 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:
react_on_rails/spec/dummy/package.json
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/spec/dummy/package.json
🧬 Code graph analysis (8)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx (1)
props(10-14)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (3)
react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (3)
store(37-37)combinedReducer(32-32)combinedProps(33-33)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (3)
store(31-31)combinedReducer(26-26)combinedProps(27-27)react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (1)
combinedReducer(13-13)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx (5)
helmetContext(21-21)helmetContext(27-27)componentHtml(22-26)props(20-34)renderedHtml(29-32)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx (1)
props(10-14)
react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (2)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (3)
store(32-32)combinedReducer(27-27)combinedProps(28-28)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (3)
store(31-31)combinedReducer(26-26)combinedProps(27-27)
react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (3)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (1)
combinedReducer(27-27)react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (1)
combinedReducer(32-32)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (1)
combinedReducer(26-26)
react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (3)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (3)
store(32-32)combinedReducer(27-27)combinedProps(28-28)react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (3)
store(37-37)combinedReducer(32-32)combinedProps(33-33)react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (1)
combinedReducer(13-13)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx (2)
packages/react-on-rails/src/ReactDOMServer.cts (1)
renderToString(4-4)react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx (1)
ReactHelmet(8-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- 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)
🔇 Additional comments (11)
react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (1)
8-8: LGTM — redux-thunk 3.x migration is correct.The switch from default import to named
{ thunk }export aligns with redux-thunk 3.x API. The middleware usage at line 37 is updated consistently.Also applies to: 37-37
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (1)
8-8: LGTM — Consistent redux-thunk 3.x migration in React 16 variant.The named
{ thunk }import and middleware usage align with the main app/ variant and redux-thunk 3.x API.Also applies to: 32-32
react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (1)
9-9: LGTM — Server-side redux-thunk migration is correct.The named import and curried middleware application pattern are both valid for redux-thunk 3.x and consistent with the client-side changes.
Also applies to: 31-31
react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx (1)
2-2: LGTM — Helmet migration with clear documentation.The switch to
@dr.pogodin/react-helmetis correct for React 19 compatibility. The added comments clearly document the HelmetProvider requirement for both client and server rendering.Also applies to: 5-7
react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (1)
2-2: LGTM — Shared store migration is consistent.The redux-thunk 3.x named export and middleware usage are correct and align with other Redux store configurations in the codebase.
Also applies to: 15-15
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx (1)
3-3: LGTM — Client-side HelmetProvider integration is correct.The HelmetProvider wrapper is required by
@dr.pogodin/react-helmetfor both client and server rendering. The implementation and documentation are correct.Also applies to: 9-14
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx (1)
4-4: LGTM — Server-side helmet rendering migrated correctly.The context-based HelmetProvider pattern is the proper approach for
@dr.pogodin/react-helmetSSR. This replaces the oldHelmet.renderStatic()global approach with thread-safe per-request context extraction.Also applies to: 19-39
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx (2)
3-4: LGTM: React 19-compatible helmet library migration.The migration from
react-helmetto@dr.pogodin/react-helmetis correct. This fork provides React 19 compatibility as documented in the PR objectives.
9-14: LGTM: HelmetProvider wrapper correctly applied.The HelmetProvider wrapper is correctly implemented for client-side rendering, consistent with the pattern in ReactHelmetApp.client.jsx. The explanatory comment on line 9 is helpful.
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx (2)
6-6: LGTM: Server-side helmet library migration.The import of
HelmetProviderfrom@dr.pogodin/react-helmetis correct for React 19 compatibility.
21-27: LGTM: Correct server-side HelmetProvider pattern.The helmetContext pattern correctly implements the @dr.pogodin/react-helmet server-side rendering approach. The context object is populated during rendering and then extracted for use in the returned HTML structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
19-73: Good implementation with JSON error handling; consider extracting webpack-assets-manifest version to a constant.The method correctly handles JSON parsing errors (re-raising with context for CI), updates both package.json and Gemfile, and provides clear output. The logic for checking both
depsanddev_depsis thorough.However, the webpack-assets-manifest version
"^5.0.6"is hardcoded on lines 40 and 44, while other versions use named constants (MINIMUM_REACT_VERSION,MINIMUM_SHAKAPACKER_VERSION). For consistency and easier maintenance, consider extracting this to a constant likeMINIMUM_WEBPACK_ASSETS_MANIFEST_VERSIONinExampleType.Example:
# In example_type.rb MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION = "^5.0.6"Then update lines 40 and 44:
- deps["webpack-assets-manifest"] = "^5.0.6" if deps.key?("webpack-assets-manifest") + deps["webpack-assets-manifest"] = ExampleType::MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION if deps.key?("webpack-assets-manifest")- dev_deps["webpack-assets-manifest"] = "^5.0.6" if dev_deps&.key?("webpack-assets-manifest") + dev_deps["webpack-assets-manifest"] = ExampleType::MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION if dev_deps&.key?("webpack-assets-manifest")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/shakapacker_examples.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/rakelib/shakapacker_examples.rake
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
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.
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 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:
react_on_rails/rakelib/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/rakelib/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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/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). (4)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
🔇 Additional comments (2)
react_on_rails/rakelib/shakapacker_examples.rake (2)
11-11: LGTM: JSON library required for new functionality.The addition of
require "json"is necessary to support JSON parsing and generation in the newapply_minimum_versionsmethod.
107-112: LGTM: Clean integration of minimum version pinning.The conditional application of
apply_minimum_versionsis correctly placed after generator commands and before npm install. Re-runningbundle_install_inafter Gemfile modification ensures dependencies are properly updated. The comment clearly explains the rationale.
CI Status Update✅ Fixed
🔧 In Progress: Generator tests (minimum version)The minimum version generator tests are encountering a Bundler isolation issue where the generated example app is loading Shakapacker from the parent This is happening because:
Potential solutions:
The Integration Tests pass which validates the core React 19 compatibility changes. The Generator test issue is a CI environment isolation problem, not a code issue. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
40-40: Consider extracting webpack-assets-manifest version as a constant.The hardcoded
"^5.0.6"appears twice and is coupled toMINIMUM_SHAKAPACKER_VERSION. Extracting it asMINIMUM_WEBPACK_ASSETS_MANIFEST_VERSIONinExampleTypewould improve maintainability and consistency with the other minimum version constants.Example:
# In example_type.rb MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION = "^5.0.6" # In this file, lines 40 and 44: deps["webpack-assets-manifest"] = ExampleType::MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION if deps.key?("webpack-assets-manifest")Also applies to: 44-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/shakapacker_examples.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/rakelib/shakapacker_examples.rake
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
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.
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.
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 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:
react_on_rails/rakelib/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/rakelib/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/rakelib/shakapacker_examples.rake
📚 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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.778Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.778Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 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/rakelib/shakapacker_examples.rake
🔇 Additional comments (4)
react_on_rails/rakelib/shakapacker_examples.rake (4)
11-11: LGTM – JSON library addition.The JSON library require is necessary for the package.json manipulation in
apply_minimum_versions.
19-73: Well-implemented minimum version updates with proper error handling.The method correctly:
- Handles JSON parse errors by logging context and re-raising to fail fast in CI
- Updates React, Shakapacker, and webpack-assets-manifest versions in package.json
- Pins Shakapacker in Gemfile using regex replacement (appropriate for generated apps with standard gem declarations)
- Adds trailing newline when writing package.json
107-112: LGTM – Correct integration of minimum version updates.The logic correctly applies minimum versions when the flag is set and re-runs
bundle_install_inafter modifying the Gemfile. The placement after generator commands and before npm install is appropriate.
116-120: Excellent fix for the Bundler isolation issue.Setting
BUNDLE_GEMFILEensures the rake task uses the generated app's Gemfile and bundle, preventing version mismatches where the parent workspace's gems (e.g., Shakapacker 9.4.0) could be loaded instead of the generated app's pinned versions (e.g., Shakapacker 8.2.0). This directly resolves the CI failure described in the PR objectives.
PR Split NoticeThis PR has been split into two smaller, focused PRs: PR 1: React 19 Compatibility ✅ (Ready for merge)PR #2127 - Updates spec/dummy for React 19 compatibility
PR 2: Minimum Version Testing Infrastructure (This PR)This PR will be rebased/updated to contain only the minimum version testing infrastructure changes once PR #2127 is merged. Current blockers:
Recommendation: Merge PR #2127 first, then continue work on this PR for the minimum version testing. |
Code Review: PR #2125Overall AssessmentThis is a well-architected refactoring with significant scope. The PR successfully moves version compatibility testing from spec/dummy to generator smoke tests while also migrating from Yarn to pnpm. Critical Issues1. PR Scope Too Large 2. Missing Type Definitions for React 19 Code Quality - Excellent Design✅ Clean separation of concerns with ExampleType class Areas for Improvement
Testing Requirements (CRITICAL)Per CLAUDE.md and testing-build-scripts.md, MUST test locally before merge:
PR description shows incomplete testing. Do not merge until ALL CI passes. Must Fix Before Merge
Should Consider
Final Recommendation✅ Approve with required changes - This is solid engineering work with clear benefits (better separation of concerns, maintainable CI). Main risk is combined scope making diagnosis harder if issues arise. Fix the React 19 type definitions and verify complete CI pass before merging. Review follows CLAUDE.md, testing-build-scripts.md, avoiding-ci-failure-cycles.md, and pr-splitting-strategy.md |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
CONTRIBUTING.md (1)
239-247: Update script/convert docs to match its new roleThis section still describes
script/convertas switching the development environment between Shakapacker v6 and v8, but the script now only tweaks Node/tooling compatibility and Pro test behavior (React/Shakapacker version coverage moved to generator smoke tests). That can confuse contributors who expect it to toggle Shakapacker versions.Consider either:
- Rewording this section to describe the current
script/convertpurpose, and/or- Pointing to the generator smoke-test examples (minimum vs latest) as the place where Shakapacker/React version compatibility is exercised.
react_on_rails/rakelib/release.rake (1)
210-215: Verify Pro dummy app path when updating Gemfile.lock
pro_dummy_app_diris built fromgem_root, notpro_gem_root:pro_dummy_app_dir = File.join(gem_root, "react_on_rails_pro", "spec", "dummy") unbundled_sh_in_dir(pro_dummy_app_dir, "bundle install#{bundle_quiet_flag}") if Dir.exist?(pro_dummy_app_dir)Given the monorepo now has
react_on_rails/andreact_on_rails_pro/as top‑level siblings, this may miss the actual Pro dummy app (react_on_rails_pro/spec/dummy), leaving its Gemfile.lock stale during releases.Consider basing this path on
pro_gem_rootinstead:- pro_dummy_app_dir = File.join(gem_root, "react_on_rails_pro", "spec", "dummy") + pro_dummy_app_dir = File.join(pro_gem_root, "spec", "dummy")and keep the
Dir.exist?guard.react_on_rails_pro/spec/dummy/package.json (1)
50-62: Based on my verification, the original review comment correctly identifies the React version inconsistency but the Redux compatibility concern is unfounded. Let me provide the final rewritten comment:
Inconsistent React version declarations between root and dummy packages.
Version inconsistency detected:
- Root
package.jsondeclares"react": "^19.0.0"(allows minor/patch updates)- This file declares
"react": "19.0.0"(exact version)Based on learnings, version consistency should be maintained across package.json files to avoid potential conflicts.
Apply this consistency fix:
- "react": "19.0.0", - "react-dom": "19.0.0", + "react": "^19.0.0", + "react-dom": "^19.0.0",Note: redux-thunk 3.1.0 is compatible with Redux 5.0.1 and React 19, so no compatibility changes are needed there.
🧹 Nitpick comments (4)
package-scripts.yml (1)
18-18: pnpm-based TypeScript check looks correctUsing
pnpm --filter <workspace> run type-checkfor each of the three packages is consistent with the new pnpm workspace setup and keeps checks scoped per package.react_on_rails_pro/spec/execjs-compatible-dummy/package.json (1)
2-2: ExecJS dummy wired into pnpm/yalc flowDeclaring
packageManager: pnpm@9.14.2and usingpnpm run link-sourceplusyalc add --link react-on-railsmakes this dummy app consistent with the rest of the pnpm-based workflow and keeps it sourcing the local package.Also applies to: 42-43
script/convert (1)
4-26: Script scope matches new “minimum toolchain” role; consider simplifying path fallbackThe header comment and current replacements correctly narrow
script/convertto:
- Dropping Node‑incompatible dev deps from the root
package.json, and- Adjusting the Pro
test:non-rscscript to skip RSC/streaming tests under minimum React/Node.That matches the intent to move React/Shakapacker version compatibility into generator smoke tests.
The
gsub_file_contenthelper still contains a Gemfile‑specific fallback:old_path = path.sub(%r{/react_on_rails/Gemfile}, "/Gemfile")but the script now only operates on
../package.jsonand../packages/.../package.json, so this fallback is effectively dead code and slightly contradicts the “old vs new structure” comment.Optional: either remove the Gemfile‑specific fallback, or generalize it (or the comment) to match the current usage. Behavior today is correct either way.
CLAUDE.md (1)
96-113: Consider updating legacy yarn-based checklist to pnpm for consistencyThe “Mandatory Local Testing Checklist” still shows:
rm -rf node_modules && yarn install --frozen-lockfile yarn run build yarn run yalc:publish ... yarn run lintwhile the rest of this file now standardizes on pnpm and explicitly says to avoid
npmcommands.For consistency with current tooling (and to avoid mixed signals to AI agents), consider updating this snippet to the pnpm equivalents (and, if desired, mentioning yarn only as an alternative for external consumers, not for this repo’s workflows).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreact_on_rails/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/execjs-compatible-dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (63)
.claude/commands/update-changelog.md(0 hunks).claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md(0 hunks).claude/docs/analysis/PHASE_5_COMPLETION_NOTES.md(0 hunks).claude/docs/analysis/claude-md-improvements.md(0 hunks).claude/docs/analysis/github-issue-1765-status-update.md(0 hunks).claude/docs/analysis/github-issue-1765-update.md(0 hunks).claude/docs/avoiding-ci-failure-cycles.md(0 hunks).claude/docs/pr-splitting-strategy.md(0 hunks).claude/docs/testing-build-scripts.md(7 hunks).github/workflows/examples.yml(3 hunks).github/workflows/gem-tests.yml(0 hunks).github/workflows/integration-tests.yml(5 hunks).github/workflows/lint-js-and-ruby.yml(2 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/playwright.yml(2 hunks).github/workflows/pro-integration-tests.yml(13 hunks).github/workflows/pro-lint.yml(4 hunks).github/workflows/pro-test-package-and-gem.yml(5 hunks).gitignore(1 hunks).npmrc(1 hunks).prettierignore(1 hunks)CHANGELOG.md(1 hunks)CLAUDE.md(8 hunks)CODING_AGENTS.md(0 hunks)CONTRIBUTING.md(16 hunks)TODO.md(0 hunks)bin/lefthook/eslint-lint(2 hunks)bin/lefthook/prettier-format(2 hunks)docs/CI_OPTIMIZATION.md(0 hunks)docs/DIRECTORY_LICENSING.md(0 hunks)docs/LICENSING_FAQ.md(0 hunks)docs/MONOREPO_MERGER_PLAN.md(0 hunks)docs/building-features/i18n.md(0 hunks)docs/building-features/streaming-server-rendering.md(0 hunks)docs/contributor-info/releasing.md(0 hunks)docs/guides/testing-configuration.md(0 hunks)docs/migrating/migrating-from-react-rails.md(0 hunks)docs/planning/docs-improvement/01-problem-analysis.md(0 hunks)docs/planning/docs-improvement/02-pr-1813-comparison.md(0 hunks)docs/planning/docs-improvement/04-ia-redesign-plan.md(0 hunks)docs/planning/docs-improvement/ia-redesign-live.md(0 hunks)docs/upgrading/release-notes/16.0.0.md(0 hunks)docs/upgrading/upgrading-react-on-rails.md(0 hunks)knip.ts(3 hunks)package-scripts.yml(1 hunks)package.json(3 hunks)packages/react-on-rails-pro-node-renderer/package.json(3 hunks)packages/react-on-rails-pro/package.json(2 hunks)packages/react-on-rails-pro/scripts/check-react-version.cjs(1 hunks)packages/react-on-rails-pro/tests/jest.setup.js(1 hunks)packages/react-on-rails/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)react_on_rails/rakelib/dummy_apps.rake(1 hunks)react_on_rails/rakelib/lint.rake(3 hunks)react_on_rails/rakelib/node_package.rake(1 hunks)react_on_rails/rakelib/release.rake(1 hunks)react_on_rails/rakelib/run_rspec.rake(2 hunks)react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)react_on_rails/spec/dummy/package.json(3 hunks)react_on_rails_pro/spec/dummy/client/app/components/LazyApolloGraphQL.tsx(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(2 hunks)script/convert(2 hunks)
💤 Files with no reviewable changes (26)
- .github/workflows/gem-tests.yml
- docs/guides/testing-configuration.md
- .claude/docs/avoiding-ci-failure-cycles.md
- docs/building-features/streaming-server-rendering.md
- .claude/docs/analysis/claude-md-improvements.md
- .claude/docs/analysis/PHASE_5_COMPLETION_NOTES.md
- docs/CI_OPTIMIZATION.md
- CODING_AGENTS.md
- docs/planning/docs-improvement/01-problem-analysis.md
- docs/upgrading/upgrading-react-on-rails.md
- docs/LICENSING_FAQ.md
- docs/building-features/i18n.md
- docs/planning/docs-improvement/04-ia-redesign-plan.md
- docs/planning/docs-improvement/ia-redesign-live.md
- docs/planning/docs-improvement/02-pr-1813-comparison.md
- docs/contributor-info/releasing.md
- docs/MONOREPO_MERGER_PLAN.md
- docs/DIRECTORY_LICENSING.md
- .claude/docs/analysis/github-issue-1765-status-update.md
- .claude/commands/update-changelog.md
- TODO.md
- .claude/docs/pr-splitting-strategy.md
- docs/migrating/migrating-from-react-rails.md
- .claude/docs/analysis/github-issue-1765-update.md
- docs/upgrading/release-notes/16.0.0.md
- .claude/docs/analysis/MONOREPO_MIGRATION_ANALYSIS.md
✅ Files skipped from review due to trivial changes (2)
- .npmrc
- .prettierignore
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/rakelib/run_rspec.rake
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails_pro/spec/dummy/client/app/components/LazyApolloGraphQL.tsxbin/lefthook/eslint-lintCONTRIBUTING.mdpackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonpackage-scripts.ymlbin/lefthook/prettier-formatreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rakereact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rakepnpm-workspace.yamlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.tsreact_on_rails_pro/spec/dummy/package.jsonscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rakereact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.jspackage.json
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/dummy_apps.rakereact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rakereact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/release.rake
🧠 Learnings (34)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
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: 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.
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails_pro/spec/dummy/client/app/components/LazyApolloGraphQL.tsxbin/lefthook/eslint-lintCONTRIBUTING.md.gitignorepackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonpackage-scripts.ymlbin/lefthook/prettier-formatreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rake.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rakereact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.json.github/workflows/examples.ymlscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rake.github/workflows/pro-integration-tests.ymlreact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.jspackage.json
📚 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:
react_on_rails_pro/spec/dummy/client/app/components/LazyApolloGraphQL.tsxCONTRIBUTING.mdpackages/react-on-rails-pro-node-renderer/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsCLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.ts
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
bin/lefthook/eslint-lintCONTRIBUTING.md.gitignorepackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonpackage-scripts.yml.github/workflows/pro-test-package-and-gem.ymlbin/lefthook/prettier-formatreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rake.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rake.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.md.github/workflows/pro-lint.ymlpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.json.github/workflows/examples.ymlscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rake.github/workflows/pro-integration-tests.ymlreact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.jspackage.json
📚 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:
bin/lefthook/eslint-lintCONTRIBUTING.md.gitignorepackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonpackage-scripts.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rake.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rake.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rake.github/workflows/pro-lint.ymlpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.jsonscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rakereact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.jspackage.json
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT manually format code during conflict resolution; always use `rake autofix` or `pnpm run format` for automated formatting
Applied to files:
bin/lefthook/eslint-lintCONTRIBUTING.mdbin/lefthook/prettier-formatreact_on_rails_pro/spec/execjs-compatible-dummy/package.json.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rakeCLAUDE.mdreact_on_rails/rakelib/release.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER use `npm` commands; only use equivalent `pnpm` commands for JavaScript package management
Applied to files:
bin/lefthook/eslint-lintCONTRIBUTING.mdpackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.json.github/workflows/pro-test-package-and-gem.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.json.github/workflows/package-js-tests.yml.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/node_package.rake.github/workflows/integration-tests.ymlCLAUDE.md.github/workflows/pro-lint.ymlpackages/react-on-rails-pro/package.json.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.json.github/workflows/examples.ymlreact_on_rails/rakelib/release.rakereact_on_rails/spec/dummy/package.json
📚 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:
bin/lefthook/eslint-lintCONTRIBUTING.md.github/workflows/pro-lint.ymlCHANGELOG.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
bin/lefthook/eslint-lintCONTRIBUTING.md.gitignorebin/lefthook/prettier-formatpackages/react-on-rails-pro/scripts/check-react-version.cjs.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.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:
bin/lefthook/eslint-lintpackages/react-on-rails-pro/scripts/check-react-version.cjspackages/react-on-rails-pro/package.jsonscript/convertCHANGELOG.md
📚 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:
CONTRIBUTING.mdpackages/react-on-rails/package.json.github/workflows/pro-test-package-and-gem.ymlreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails/rakelib/dummy_apps.rake.claude/docs/testing-build-scripts.md.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.md.github/workflows/pro-lint.ymlpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/spec/dummy/package.jsonscript/convertCHANGELOG.md.github/workflows/pro-integration-tests.ymlreact_on_rails/spec/dummy/package.jsonpackage.json
📚 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:
CONTRIBUTING.md.gitignorepackages/react-on-rails/package.jsonpackages/react-on-rails-pro-node-renderer/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rakereact_on_rails/rakelib/node_package.rake.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.jsonscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rakereact_on_rails/spec/dummy/package.jsonpackage.json
📚 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:
CONTRIBUTING.md.gitignorepackages/react-on-rails-pro-node-renderer/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.jsonpackages/react-on-rails-pro/scripts/check-react-version.cjsreact_on_rails/rakelib/dummy_apps.rakereact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/shakapacker_examples.rakepackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.jsonscript/convertCHANGELOG.mdreact_on_rails/rakelib/release.rakereact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.jspackage.json
📚 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:
CONTRIBUTING.mdpackages/react-on-rails-pro/scripts/check-react-version.cjsCHANGELOG.mdreact_on_rails/rakelib/release.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:
CONTRIBUTING.mdpackages/react-on-rails-pro/package.jsonCHANGELOG.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
CONTRIBUTING.md.github/workflows/pro-test-package-and-gem.yml.github/workflows/playwright.ymlreact_on_rails/rakelib/dummy_apps.rake.github/workflows/integration-tests.ymlCLAUDE.md.github/workflows/pro-lint.ymlreact_on_rails_pro/spec/dummy/package.json.github/workflows/pro-integration-tests.ymlpackages/react-on-rails-pro/tests/jest.setup.js
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
CONTRIBUTING.md.github/workflows/playwright.ymlreact_on_rails/rakelib/dummy_apps.rakereact_on_rails/rakelib/lint.rake.github/workflows/integration-tests.ymlreact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.md.github/workflows/pro-lint.yml.github/workflows/pro-integration-tests.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Generate test data in Playwright tests using `appFactories` function with factory_bot syntax (e.g., `appFactories([['create', 'user', { name: 'Test User' }]])`) rather than manual database manipulation
Applied to files:
CONTRIBUTING.md.github/workflows/pro-test-package-and-gem.ymlreact_on_rails/rakelib/dummy_apps.rake.github/workflows/integration-tests.ymlCLAUDE.md.github/workflows/pro-lint.yml.github/workflows/pro-integration-tests.ymlpackages/react-on-rails-pro/tests/jest.setup.js
📚 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:
CONTRIBUTING.mdpackages/react-on-rails-pro/tests/jest.setup.js
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
CONTRIBUTING.md.gitignore.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakereact_on_rails/rakelib/shakapacker_examples.rakeCLAUDE.md.github/workflows/pro-lint.yml.github/workflows/lint-js-and-ruby.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: ALWAYS let Prettier and RuboCop handle ALL formatting - never manually format
Applied to files:
CONTRIBUTING.mdbin/lefthook/prettier-formatreact_on_rails/rakelib/lint.rakeCLAUDE.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
CONTRIBUTING.md.gitignorepackages/react-on-rails-pro-node-renderer/package.json.claude/docs/testing-build-scripts.mdpnpm-workspace.yaml.github/workflows/integration-tests.ymlCLAUDE.md.github/workflows/pro-lint.ymlpackages/react-on-rails-pro/package.jsonknip.ts.github/workflows/lint-js-and-ruby.ymlreact_on_rails_pro/spec/dummy/package.jsonscript/convertpackage.json
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Resolve merge conflicts by: resolving logical conflicts first, verifying file paths with grep, testing affected scripts, auto-fixing formatting with `rake autofix`, and testing critical scripts if build configs changed
Applied to files:
CONTRIBUTING.md.claude/docs/testing-build-scripts.mdreact_on_rails/rakelib/lint.rakeCLAUDE.md.github/workflows/lint-js-and-ruby.ymlscript/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
.gitignoreknip.ts
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for Pro-only features, fixes, and changes affecting Pro packages
Applied to files:
.github/workflows/pro-test-package-and-gem.yml.github/workflows/pro-lint.ymlscript/convertCHANGELOG.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : In Playwright E2E tests, use `app('clean')` in `beforeEach` blocks to ensure clean database state between tests
Applied to files:
.github/workflows/playwright.ymlCLAUDE.md.github/workflows/pro-integration-tests.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 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:
packages/react-on-rails-pro/scripts/check-react-version.cjsCHANGELOG.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: For infrastructure/config changes affecting directory structure, file paths, build configurations, or CI workflows, conduct comprehensive local testing before pushing (the '15-Minute Rule')
Applied to files:
.claude/docs/testing-build-scripts.mdCLAUDE.md.github/workflows/examples.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER push without running full lint check first
Applied to files:
.claude/docs/testing-build-scripts.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG*.md : Use changelog format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` (no hash in PR number)
Applied to files:
CLAUDE.mdCHANGELOG.md
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
CLAUDE.mdpackages/react-on-rails-pro/package.jsonknip.tsreact_on_rails/spec/dummy/package.jsonpackages/react-on-rails-pro/tests/jest.setup.js
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails-pro/package.jsonknip.tsCHANGELOG.md
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
script/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT add changelog entries for linting, formatting, refactoring, tests, or documentation fixes
Applied to files:
CHANGELOG.md
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~22-~22: “React” is a proper noun and needs to be capitalized.
Context: ...sites Note for users: End users of react_on_rails can continue using their prefe...
(A_GOOGLE)
[uncategorized] ~22-~22: “React” is a proper noun and needs to be capitalized.
Context: ...low are for contributors working on the react_on_rails codebase itself. - [Yalc](htt...
(A_GOOGLE)
[style] ~310-~310: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...figs, then you need to restart Foreman. If you change the JS code for react-on-rai...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
.claude/docs/testing-build-scripts.md
114-114: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (56)
knip.ts (3)
41-45: LGTM!The new ignoreDependencies entries are well-documented and correctly identify tooling that Knip cannot auto-detect (nps via YAML config, validation tools used in scripts).
179-182: LGTM!The updated comments more accurately describe how these dependencies are used (webpack for runtime polyfills, CSS extraction for MiniCssExtractPlugin).
201-201: No action required—the file correctly ends with a trailing newline.The
knip.tsfile already complies with the coding guideline requirement to end with a newline character. No changes are needed.packages/react-on-rails-pro/tests/jest.setup.js (4)
1-4: LGTM—static imports improve ES module compliance.The migration from runtime
require()calls to staticimportstatements at the top of the file follows ES module conventions and improves clarity.
74-74: File ends with newline — no issue.The file
packages/react-on-rails-pro/tests/jest.setup.jsalready ends with a newline character, meeting the coding guideline requirement. No action needed.
34-34: Node.js version compatibility is satisfied; Readable.toWeb() is supported in the project's minimum supported Node.js version.The project's minimum supported Node.js version is 20 (per CI matrix in
package-js-tests.yml), which exceeds the 17.0.0 requirement forReadable.toWeb(). The.nvmrcspecifies 22.12.0 and CI workflows test against Node.js 22 and 20. No version compatibility issues exist.
24-39: Perfect. Now I have all the information I need. Let me verify one final detail about the jest mock hoisting in ESM:
<function_calls>
jest.mock hoisting ES modules @jest/globals setupFiles conditional
</function_calls>Based on my comprehensive verification, I can now provide the rewritten review comment:
The jest.mock in jest.setup.js is correctly placed and functioning as intended; no changes needed.
The jest.mock at lines 24-39 is in a setupFiles configuration file that runs during jest initialization (before any tests), not during test execution. While jest.mock is typically at the top level, calling it within a conditional at module load time in a setup file is a valid pattern in ESM with @jest/globals.
Evidence:
jest.config.jsconfiguressetupFiles: ['<rootDir>/tests/jest.setup.js'], meaning this file runs synchronously during jest initialization- The test
utils.test.jssuccessfully verifies the mock works: it assertsexpect(body).toBeInstanceOf(ReadableStream), confirming the fetch response body is correctly converted from Node's Readable stream to Web ReadableStream- The
testEnvironment: 'jsdom'in the base config ensureswindowis defined when needed, making the conditional check valid- No test failures are reported in CI
The conditional
if (typeof window !== 'undefined')is appropriate here for environment detection at module load time, and the mock is properly registered before any imports that depend on it..gitignore (1)
1-2: Root Gemfile.lock ignore is consistent with monorepo layoutIgnoring
/Gemfile.lockat the root while documenting that the real lock lives underreact_on_rails/keeps the VCS state clean and avoids confusion between locks.pnpm-workspace.yaml (1)
1-2: Workspace scope definition is saneDeclaring
packages/*as the workspace root matches the typical monorepo layout here and should work well with the pnpm filters used elsewhere.react_on_rails_pro/spec/dummy/client/app/components/LazyApolloGraphQL.tsx (1)
80-82: Structured error rendering is reasonableSwitching to
JSON.stringify(updateError)should surface more detail than.toString()in this dummy component and is fine for debugging-oriented UI.packages/react-on-rails/package.json (1)
8-15: Build and type-check scripts align with pnpm toolingUsing
pnpm run clean && tscfor build, a dedicatedtype-checkscript withtsc --noEmit, andpnpm run buildinprepublishOnlyfits the pnpm migration and keeps package scripts straightforward.react_on_rails/rakelib/node_package.rake (1)
10-10: Node package build task correctly migrated to pnpmUpdating the task to
sh "pnpm run build && pnpm yalc:publish"removes the last yarn dependency and matches the pnpm-only tooling direction.react_on_rails/rakelib/lint.rake (1)
20-21: Lint tasks correctly switched to pnpmUpdating eslint, prettier, and stylelint invocations to
pnpm run …aligns the Ruby rake lint tasks with the pnpm-based JS tooling and keeps all linting routed through the workspace scripts.Also applies to: 30-31, 40-40
CONTRIBUTING.md (1)
22-38: PNPM contributor guidance and setup commands look consistentThe new note clearly distinguishes end‑user package manager freedom from pnpm‑only contributor workflows, and the updated setup command (
bundle && pnpm install && rake …) matches the rest of the repo’s pnpm migration. No issues here.Also applies to: 31-34
packages/react-on-rails-pro/scripts/check-react-version.cjs (1)
1-19: React-version gate for RSC tests is correctThe script correctly treats React
<19as “skip RSC tests” (exit 0) and React≥19as “run RSC tests” (exit 1 so the surrounding|| jest …chain executes). This aligns with the test:rsc auto‑detection behavior described elsewhere.CHANGELOG.md (1)
34-35: PNPM migration changelog entry is well‑formedThe new Unreleased “Package Manager Migration to pnpm (Contributors Only)” entry follows the project’s changelog format and accurately scopes the change to contributors. Looks good.
react_on_rails/rakelib/release.rake (1)
259-278: PNPM-based NPM publishing flow looks correctSwitching to
pnpm publishfor:
packages/react-on-railspackages/react-on-rails-proreact_on_rails_pro(node renderer, with--no-git-checks)reuses the
npm_publish_args(registry + OTP when hitting npmjs.org) and keeps Verdaccio support. This matches the repository’s pnpm-only policy and the new unified release behavior.CLAUDE.md (1)
135-158: PNPM-centric development guidance is consistent and clearThe updated Essential Commands, linting/formatting, build, and changelog sections consistently use
pnpmand explicitly forbidnpmfor repo work, which matches the project’s tooling direction. The commands (pnpm install,pnpm run test,pnpm run lint,pnpm run format.listDifferent,pnpm run build,pnpm run type-check) look coherent with the rest of the monorepo.Also applies to: 279-317
.github/workflows/examples.yml (1)
119-188: Examples workflow correctly migrates to PNPM and splits latest/minimum example setsThe workflow now:
- Sets up pnpm with a cached store and prints its version.
- Installs Node deps via
pnpm install(frozen forlatest, non‑frozen forminimum) and usespnpm yalc:publishpluspnpm add -g yalcfor the local package flow.- Runs
script/convertonly for theminimumdependency level, which matches the script’s new “minimum toolchain” responsibility.- Routes example specs through the new tasks:
run_rspec:shakapacker_examples_latestfor latest deps,run_rspec:shakapacker_examples_minimumfor minimum deps.This is consistent with the CI story described in the PR and the rest of the pnpm migration.
Also applies to: 151-158
.claude/docs/testing-build-scripts.md (1)
7-59: Build-script testing guidance now matches PNPM-based workflowsThe updated doc:
- Emphasizes clean
pnpm install --frozen-lockfile,pnpm run build,pnpm run prepack, andpnpm run yalc:publishas mandatory checks after changing package.json or build configs.- Calls out workspace-specific behavior and the need to use
workspace:*with pnpm, plus verification viapnpm -r listand a fresh install.- Grounded real-world examples (path issues, workspace protocol migration) in PNPM terms.
This aligns well with the current tooling and should help prevent future silent build/publish regressions.
Also applies to: 41-59, 113-159
bin/lefthook/prettier-format (1)
26-26: LGTM! Correct pnpm migration.The migration from
yarn run prettiertopnpm exec prettieris correct and consistent with the broader pnpm adoption across the codebase.Also applies to: 41-41
bin/lefthook/eslint-lint (1)
34-34: LGTM! Correct pnpm migration.The migration from
yarn run eslinttopnpm exec eslintis correct and maintains proper separation of root and Pro directory linting.Also applies to: 51-51
packages/react-on-rails-pro-node-renderer/package.json (2)
50-50: LGTM! Correct workspace dependency syntax.The change to
"react-on-rails": "workspace:*"is the correct pnpm syntax for monorepo workspace dependencies, replacing Yarn's plain"*"syntax.
73-82: LGTM! Scripts properly migrated to pnpm.All script commands have been correctly updated to use
pnpm runinstead of yarn equivalents. The removal of the resolutions block is also correct, as pnpm uses overrides (defined in the root package.json).react_on_rails/rakelib/dummy_apps.rake (1)
8-12: LGTM! Correct pnpm migration in Rake task.The task has been properly renamed and the command updated. Note that the
--mutex networkflag was correctly removed as pnpm has built-in lock file management.Per coding guidelines, ensure this file has been checked with RuboCop:
#!/bin/bash bundle exec rubocop react_on_rails/rakelib/dummy_apps.rake.github/workflows/pro-lint.yml (2)
158-162: LGTM! Correct use of --ignore-workspace flag.The
--ignore-workspaceflag is correctly used for dummy app installations to prevent pnpm from treating them as part of the parent workspace. This ensures proper dependency isolation for test scenarios.
100-115: pnpm-lock.yaml exists and is properly committed—no action needed.The verification confirms
pnpm-lock.yamlis present in the repository root, tracked in git, and the cache key configuration at line 111 of the workflow will function correctly. The lock file exists and is committed, so the cache mechanism will work as intended..github/workflows/pro-test-package-and-gem.yml (3)
102-115: Verify pnpm-lock.yaml exists for cache keys.Both jobs in this workflow use
pnpm-lock.yamlfor cache keys. Ensure this file is committed to the repository for proper caching.Same verification as pro-lint.yml applies - see comment on that file.
Also applies to: 207-220
135-140: LGTM! Correct pnpm installation with frozen lockfile.The use of
pnpm install --frozen-lockfileensures deterministic, reproducible installs in CI. The--ignore-workspaceflag for dummy apps correctly isolates their dependencies.Also applies to: 249-250
253-254: LGTM! Proper use of pnpm filter for monorepo workspace.The
pnpm --filtercommand correctly targets the specific package in the monorepo workspace for testing.package.json (1)
58-59: Update @types/react and @types/react-dom to React 19 versions; function component propTypes in spec/dummy will be silently ignored.The upgrade to React 19 has been partially applied. While the main dependencies are updated, there are two compatibility concerns:
TypeScript Types Mismatch: @types/react and @types/react-dom are still pinned to v18 (lines 31-32), not v19. These should be updated to
^19.0.0to match React and ensure proper type checking.Function Component propTypes: Multiple function components in spec/dummy use propTypes (HelloWorldProps.jsx, HelloWorldHooks.jsx, RailsContext.jsx, HelloWorldContainer.jsx, etc.), which React 19 silently ignores. In React 19, propTypes checks are removed entirely. Class component propTypes (static propTypes) work fine, but function component propTypes will no longer provide runtime validation.
Actions needed:
- Update @types/react and @types/react-dom to ^19.0.0
- Consider removing propTypes from function components in spec/dummy or migrating to the standalone prop-types package with runtime validation
- The production code appropriately uses TypeScript instead of propTypes
.github/workflows/package-js-tests.yml (2)
101-112: LGTM! pnpm setup and caching is correctly configured.The pnpm setup follows the recommended pattern: action setup, store path retrieval, and cache configuration with
pnpm-lock.yamlhash key. This is consistent with the pnpm migration across other workflow files.
123-130: LGTM! pnpm commands correctly replace Yarn equivalents.The conditional
--frozen-lockfilelogic correctly allows lockfile updates for Node 20 (minimum version testing via conversion script) while enforcing frozen lockfile for Node 22 (latest). Global yalc installation via pnpm aligns with the project's package management standards..github/workflows/lint-js-and-ruby.yml (3)
101-112: LGTM! pnpm setup follows the established pattern.The pnpm setup and caching configuration is consistent with other workflow files in this PR.
136-139: Good use of--ignore-workspacefor dummy app isolation.The
--ignore-workspaceflag correctly prevents pnpm from treating the dummy app as part of the parent workspace, which is essential since it has its own dependencies and uses yalc links. The explanatory comment is helpful.
167-185: LGTM! Linting commands correctly migrated to pnpm.All linting tools are correctly invoked via
pnpm execorpnpm runas appropriate. Thepnpm startcommand for format.listDifferent correctly uses the nps (package-scripts) runner..github/workflows/playwright.yml (2)
56-69: LGTM! pnpm setup and caching correctly configured.The pnpm setup follows the established pattern used consistently across the PR's workflow files.
71-100: LGTM! Playwright workflow correctly migrated to pnpm.All dependency installation and test execution commands are correctly using pnpm equivalents. The
--ignore-workspaceflag for the dummy app andpnpm execfor Playwright browser installation are appropriate.react_on_rails/rakelib/shakapacker_examples.rake (2)
21-73: Well-structured minimum version application with proper error handling.The
apply_minimum_versionsmethod correctly:
- Handles JSON parsing errors with informative output and re-raises to fail CI fast
- Updates both
dependenciesanddevDependenciesfor React, Shakapacker, and webpack-assets-manifest- Updates Gemfile with exact Shakapacker version pin
- Provides clear output of the applied versions
The RuboCop disables for complexity metrics are acceptable given this is a configuration-focused method.
117-127: Good solution for Bundler isolation in minimum-version examples.The
BUNDLE_GEMFILEandBUNDLE_FROZEN=falseenvironment variables correctly address the CI issue mentioned in PR comments where generated apps were loading the wrong Shakapacker version from the parent workspace's vendor/bundle. This ensures the generated app uses its own Gemfile with the pinned minimum Shakapacker version..github/workflows/pro-integration-tests.yml (5)
101-115: LGTM! pnpm setup consistent across Pro integration workflow.The pnpm setup pattern is correctly replicated across all three jobs in this workflow (build-dummy-app-webpack-test-bundles, rspec-dummy-app-node-renderer, dummy-app-node-renderer-e2e-tests).
138-141: Good use of--ignore-workspacewith clear documentation.The comment explaining why
--ignore-workspaceis needed for the Pro dummy app is helpful for maintainability.
351-356: LGTM! Artifact naming correctly reflects pnpm migration.The debug log artifact is appropriately renamed from yarn to pnpm context.
501-513: Note the 5x timeout increase for E2E tests.The Rails server wait timeout is 300s here (E2E tests job) vs 60s in the rspec-dummy-app-node-renderer job (line 301). This difference is likely intentional since E2E tests may have heavier initialization, but verify this is the expected behavior.
515-519: LGTM! Playwright commands correctly migrated to pnpm.The Playwright installation and E2E test execution correctly use pnpm commands.
.github/workflows/integration-tests.yml (5)
122-133: PNPM setup is correctly configured and duplicated appropriately.The PNPM setup steps (pnpm/action-setup, store path retrieval, cache configuration) are properly duplicated across both jobs. This is standard practice in GitHub Actions since jobs run independently. The cache key glob pattern
**/pnpm-lock.yamlcorrectly targets the monorepo root lockfile.Also applies to: 211-222
147-149: Conditional frozen-lockfile logic supports both reproducible and compatibility testing.The conditional
${{ matrix.dependency-level == 'latest' && '--frozen-lockfile' || '--no-frozen-lockfile' }}correctly:
- Uses
--frozen-lockfilefor latest runs to ensure reproducible builds- Uses
--no-frozen-lockfilefor minimum runs (older Node/Ruby) to test compatibilityThe
--ignore-workspaceflag for dummy app installs (lines 154–157, 260–263) is correct, as the dummy app has its own dependency scope and should not inherit workspace constraints.Also applies to: 252-255, 260-263
143-145: Conversion script scope aligns with PR objectives.The conversion script is correctly run only when
dependency-level == 'minimum'(for older Ruby/Node compatibility). Per PR objectives, the script now handles only Node.js tooling compatibility and no longer modifies React/Shakapacker versions. The spec/dummy app maintains React 19 and Shakapacker 9.4.0 regardless of matrix level, which is correct.Also applies to: 232-234
80-85: Matrix setup correctly tests latest and minimum Ruby/Node versions.The
setup-integration-matrixjob correctly generates two matrix configurations:
- Latest: Ruby 3.4, Node 22 (for fast PR feedback)
- Minimum: Ruby 3.2, Node 20 (full CI, master, or workflow_dispatch only)
This aligns with the PR objectives: spec/dummy always uses React 19/Shakapacker 9.4.0, but the matrix ensures compatibility across supported Ruby/Node versions.
328-329: Verify file ends with a newline character.The artifact name correctly changed from
yarn-debug.logtopnpm-debug.log. However, ensure the file ends with a newline after the closing braces on line 329, per coding guidelines.react_on_rails/spec/dummy/package.json (3)
21-28: React 19 dependencies are correctly updated.The upgrade from React 18 to ^19.0.0 includes critical dependency updates:
- react-helmet replaced with @dr.pogodin/react-helmet@^3.0.4 (React 19 compatible)
- react-redux upgraded to ^9.2.0 (adds React 19 peer dependency support)
- redux upgraded to ^5.0.1 and redux-thunk to ^3.1.0
Per past review comments, these changes were already implemented and verified in commits db1c0fd to dbc7a32. No further action needed.
69-86: PNPM scripts correctly migrated from Yarn.All build, test, and utility scripts properly use
pnpm runcommands:
- preinstall, link-source, lint, format delegate via pnpm
- test:js invokes jest directly (pnpm automatically includes node_modules/.bin in PATH)
- build commands correctly chain pnpm and bin/shakapacker
The packageManager field (line 89) ensures the team enforces pnpm@9.14.2 usage.
1-89: Verify trailing newline at end of file.Ensure this file ends with a newline character after the closing brace, per coding guidelines.
packages/react-on-rails-pro/package.json (3)
58-58: Workspace dependency resolution is correctly configured.The change from
"react-on-rails": "*"to"react-on-rails": "workspace:*"correctly uses PNPM's workspace protocol. This ensures the pro package depends on the local react-on-rails package in the monorepo, matching the PNPM workspace setup referenced in pnpm-workspace.yaml.
7-15: PNPM scripts and TypeScript compilation are correctly configured.All scripts properly delegate to pnpm and tsc:
- build and build-watch clean and compile with tsc
- test chains test:non-rsc and test:rsc via pnpm
- test:rsc conditionally runs jest with NODE_CONDITIONS=react-server (gates RSC tests)
- prepublishOnly ensures build runs before publishing
1-82: Verify trailing newline at end of file.Ensure this file ends with a newline character after the closing brace, per coding guidelines.
| // Export disableHttp2 for test utilities | ||
| 'src/worker.ts!', |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
# First, locate and examine the knip.ts file
fd -t f "knip.ts" --exec cat -n {} \; | head -100
# Then check for the package.json in react-on-rails-pro-node-renderer
fd -t f "package.json" | grep -i "react-on-rails-pro-node-renderer" | head -5Repository: shakacode/react_on_rails
<shell_result>
Repository: shakacode/react_on_rails
Exit code: 0
stdout:
1 import type { KnipConfig } from 'knip';
2
3 const config: KnipConfig = {
4 // ! at the end means files are used in production
5 workspaces: {
6 // Root workspace - manages the monorepo and global tooling
7 '.': {
8 entry: ['eslint.config.ts', 'jest.config.base.js'],
9 project: ['*.{js,mjs,ts}'],
10 ignoreBinaries: [
11 // Has to be installed globally
12 'yalc',
13 // Pro package binaries used in Pro workflows
14 'playwright',
15 'e2e-test',
16 ],
17 ignore: ['react_on_rails_pro/**', 'react_on_rails/vendor/**'],
18 ignoreDependencies: [
19 // Required for TypeScript compilation, but we don't depend on Turbolinks itself.
20 '@types/turbolinks',
21 // The Knip ESLint plugin fails to detect these are transitively required by a config,
22 // though we don't actually use its rules anywhere.
23 '@babel/eslint-parser',
24 '@babel/preset-react',
25 'eslint-config-shakacode',
26 'eslint-plugin-jsx-a11y',
27 'eslint-plugin-react',
28 'eslint-plugin-react-hooks',
29 // These are used as transitive dependencies and missing from package.json
30 '@eslint/eslintrc',
31 '@eslint/js',
32 // used by Jest
33 'jsdom',
34 'jest-junit',
35 // This is an optional peer dependency because users without RSC don't need it
36 // but Knip doesn't like such dependencies to be referenced directly in code
37 'react-on-rails-rsc',
38 // SWC transpiler dependencies used by Shakapacker in dummy apps
39 '@swc/core',
40 'swc-loader',
41 // Used via nps (package-scripts.yml) which knip doesn't fully analyze
42 'nps',
43 // Used for package validation but not directly imported
44 '@arethetypeswrong/cli',
45 'publint',
46 ],
47 },
48
49 // React on Rails core package workspace
50 'packages/react-on-rails': {
51 entry: [
52 'src/ReactOnRails.full.ts!',
53 'src/ReactOnRails.client.ts!',
54 'src/base/full.rsc.ts!',
55 'src/context.ts!',
56 ],
57 project: ['src/**/*.[jt]s{x,}!', 'tests/**/*.[jt]s{x,}', '!lib/**'],
58 ignore: [
59 // Jest setup and test utilities - not detected by Jest plugin in workspace setup
60 'tests/jest.setup.js',
61 // Build output directories that should be ignored
62 'lib/**',
63 ],
64 },
65
66 // React on Rails Pro Node Renderer package workspace
67 'packages/react-on-rails-pro-node-renderer': {
68 entry: [
69 'src/ReactOnRailsProNodeRenderer.ts!',
70 'src/default-node-renderer.ts!',
71 'src/integrations/*.ts!',
72 // Export disableHttp2 for test utilities
73 'src/worker.ts!',
74 ],
75 project: ['src/**/*.[jt]s{x,}!', 'tests/**/*.[jt]s{x,}', '!lib/**'],
76 ignore: [
77 // Build output directories that should be ignored
78 'lib/**',
79 // Test fixtures
80 'tests/fixtures/**',
81 // Test helper utilities
82 'tests/helper.ts',
83 'tests/httpRequestUtils.ts',
84 ],
85 ignoreDependencies: [
86 // Optional dependencies used in integrations
87 '@honeybadger-io/js',
88 '@sentry/*',
89 // Jest reporter used in CI
90 'jest-junit',
91 ],
92 },
93
94 // React on Rails Pro package workspace
95 'packages/react-on-rails-pro': {
96 entry: [
97 'src/ReactOnRails.node.ts!',
98 'src/ReactOnRails.full.ts!',
99 'src/ReactOnRails.client.ts!',
100 'src/ReactOnRailsRSC.ts!',
packages/react-on-rails-pro-node-renderer/package.json
</shell_result>
Script executed:
# Check the package.json to see what's exported
cat packages/react-on-rails-pro-node-renderer/package.json | jq '.exports'
# Read worker.ts to see what disableHttp2 is
cat packages/react-on-rails-pro-node-renderer/src/worker.tsRepository: shakacode/react_on_rails
<shell_result>
Repository: shakacode/react_on_rails
Exit code: 0
stdout:
{
".": {
"types": "./lib/ReactOnRailsProNodeRenderer.d.ts",
"default": "./lib/ReactOnRailsProNodeRenderer.js"
},
"./integrations/*": {
"types": "./lib/integrations/*.d.ts",
"default": "./lib/integrations/*.js"
},
"./package.json": "./package.json"
}
/**
* Entry point for worker process that handles requests.
* @module worker
*/
import path from 'path';
import cluster from 'cluster';
import { mkdir } from 'fs/promises';
import fastify from 'fastify';
import fastifyFormbody from '@fastify/formbody';
import fastifyMultipart from '@fastify/multipart';
import log, { sharedLoggerOptions } from './shared/log.js';
import packageJson from './shared/packageJson.js';
import { buildConfig, Config, getConfig } from './shared/configBuilder.js';
import fileExistsAsync from './shared/fileExistsAsync.js';
import type { FastifyInstance, FastifyReply, FastifyRequest } from './worker/types.js';
import checkProtocolVersion from './worker/checkProtocolVersionHandler.js';
import authenticate from './worker/authHandler.js';
import { handleRenderRequest, type ProvidedNewBundle } from './worker/handleRenderRequest.js';
import handleGracefulShutdown from './worker/handleGracefulShutdown.js';
import {
errorResponseResult,
formatExceptionMessage,
copyUploadedAssets,
ResponseResult,
workerIdLabel,
saveMultipartFile,
Asset,
getAssetPath,
getBundleDirectory,
deleteUploadedAssets,
} from './shared/utils.js';
import * as errorReporter from './shared/errorReporter.js';
import { lock, unlock } from './shared/locks.js';
import { startSsrRequestOptions, trace } from './shared/tracing.js';
// Uncomment the below for testing timeouts:
// import { delay } from './shared/utils.js';
//
// function getRandomInt(max) {
// return Math.floor(Math.random() * Math.floor(max));
// }
declare module '@fastify/multipart' {
interface MultipartFile {
// We save all uploaded files and store this value
value: Asset;
}
}
export type FastifyConfigFunction = (app: FastifyInstance) => void;
const fastifyConfigFunctions: FastifyConfigFunction[] = [];
/**
* Configures Fastify instance before starting the server.
* @param configFunction The configuring function. Normally it will be something like `(app) => { app.register(...); }`
* or `(app) => { app.addHook(...); }` to report data from Fastify to an external service.
* Note that we call `await app.ready()` in our code, so you don't need to `await` the results.
*/
export function configureFastify(configFunction: FastifyConfigFunction) {
fastifyConfigFunctions.push(configFunction);
}
function setHeaders(headers: ResponseResult['headers'], res: FastifyReply) {
// eslint-disable-next-line @typescript-eslint/no-misused-promises -- fixing it with `void` just violates no-void
Object.entries(headers).forEach(([key, header]) => res.header(key, header));
}
const setResponse = async (result: ResponseResult, res: FastifyReply) => {
const { status, data, headers, stream } = result;
if (status !== 200 && status !== 410) {
log.info({ msg: 'Sending non-200, non-410 data back', data });
}
setHeaders(headers, res);
res.status(status);
if (stream) {
await res.send(stream);
} else {
res.send(data);
}
};
const isAsset = (value: unknown): value is Asset => (value as { type?: string }).type === 'asset';
function assertAsset(value: unknown, key: string): asserts value is Asset {
if (!isAsset(value)) {
throw new Error(`React On Rails Error: Expected an asset for key: ${key}`);
}
}
// Remove after this issue is resolved: https://github.com/fastify/light-my-request/issues/315
let useHttp2 = true;
// Call before any test using `app.inject()`
export const disableHttp2 = () => {
useHttp2 = false;
};
type WithBodyArrayField<T, K extends string> = T & { [P in K | `${K}[]`]?: string | string[] };
const extractBodyArrayField = <Key extends string>(
body: WithBodyArrayField<Record<string, unknown>, Key>,
key: Key,
): string[] | undefined => {
const value = body[key] ?? body[`${key}[]`];
if (Array.isArray(value)) {
return value;
}
if (typeof value === 'string' && value.length > 0) {
return [value];
}
return undefined;
};
export default function run(config: Partial<Config>) {
// Store config in app state. From now it can be loaded by any module using
// getConfig():
buildConfig(config);
const { serverBundleCachePath, logHttpLevel, port, fastifyServerOptions, workersCount } = getConfig();
const app = fastify({
http2: useHttp2 as true,
bodyLimit: 104857600, // 100 MB
logger:
logHttpLevel !== 'silent' ? { name: 'RORP HTTP', level: logHttpLevel, ...sharedLoggerOptions } : false,
...fastifyServerOptions,
});
handleGracefulShutdown(app);
// We shouldn't have unhandled errors here, but just in case
app.addHook('onError', (req, res, err, done) => {
// Not errorReporter.error so that integrations can decide how to log the errors.
app.log.error({ msg: 'Unhandled Fastify error', err, req, res });
done();
});
// 10 MB limit for code including props
const fieldSizeLimit = 1024 * 1024 * 10;
// Supports application/x-www-form-urlencoded
void app.register(fastifyFormbody);
// Supports multipart/form-data
void app.register(fastifyMultipart, {
attachFieldsToBody: 'keyValues',
limits: {
fieldSize: fieldSizeLimit,
// For bundles and assets
fileSize: Infinity,
},
onFile: async (part) => {
const destinationPath = path.join(serverBundleCachePath, 'uploads', part.filename);
// TODO: inline here
await saveMultipartFile(part, destinationPath);
// eslint-disable-next-line no-param-reassign
part.value = {
filename: part.filename,
savedFilePath: destinationPath,
type: 'asset',
};
},
});
const isProtocolVersionMatch = async (req: FastifyRequest, res: FastifyReply) => {
// Check protocol version
const protocolVersionCheckingResult = checkProtocolVersion(req);
if (typeof protocolVersionCheckingResult === 'object') {
await setResponse(protocolVersionCheckingResult, res);
return false;
}
return true;
};
const isAuthenticated = async (req: FastifyRequest, res: FastifyReply) => {
// Authenticate Ruby client
const authResult = authenticate(req);
if (typeof authResult === 'object') {
await setResponse(authResult, res);
return false;
}
return true;
};
const requestPrechecks = async (req: FastifyRequest, res: FastifyReply) => {
if (!(await isProtocolVersionMatch(req, res))) {
return false;
}
if (!(await isAuthenticated(req, res))) {
return false;
}
return true;
};
// See https://github.com/shakacode/react_on_rails_pro/issues/119 for why
// the digest is part of the request URL. Yes, it's not used here, but the
// server logs might show it to distinguish different requests.
app.post<{
Body: WithBodyArrayField<
{
renderingRequest: string;
},
'dependencyBundleTimestamps'
>;
// Can't infer from the route like Express can
Params: { bundleTimestamp: string; renderRequestDigest: string };
}>('/bundles/:bundleTimestamp/render/:renderRequestDigest', async (req, res) => {
if (!(await requestPrechecks(req, res))) {
return;
}
// DO NOT REMOVE (REQUIRED FOR TIMEOUT TESTING)
// if(TESTING_TIMEOUTS && getRandomInt(2) === 1) {
// console.log(
// 'ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ');
// console.log(`Sleeping, to test timeouts`);
// console.log(
// 'ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ');
//
// await delay(100000);
// }
const { renderingRequest } = req.body;
const { bundleTimestamp } = req.params;
const providedNewBundles: ProvidedNewBundle[] = [];
const assetsToCopy: Asset[] = [];
Object.entries(req.body).forEach(([key, value]) => {
if (key === 'bundle') {
assertAsset(value, key);
providedNewBundles.push({ timestamp: bundleTimestamp, bundle: value });
} else if (key.startsWith('bundle_')) {
assertAsset(value, key);
providedNewBundles.push({ timestamp: key.replace('bundle_', ''), bundle: value });
} else if (isAsset(value)) {
assetsToCopy.push(value);
}
});
try {
const dependencyBundleTimestamps = extractBodyArrayField(req.body, 'dependencyBundleTimestamps');
await trace(async (context) => {
try {
const result = await handleRenderRequest({
renderingRequest,
bundleTimestamp,
dependencyBundleTimestamps,
providedNewBundles,
assetsToCopy,
});
await setResponse(result, res);
} catch (err) {
const exceptionMessage = formatExceptionMessage(
renderingRequest,
err,
'UNHANDLED error in handleRenderRequest',
);
errorReporter.message(exceptionMessage, context);
await setResponse(errorResponseResult(exceptionMessage), res);
}
}, startSsrRequestOptions({ renderingRequest }));
} catch (theErr) {
const exceptionMessage = formatExceptionMessage(renderingRequest, theErr);
errorReporter.message(`Unhandled top level error: ${exceptionMessage}`);
await setResponse(errorResponseResult(exceptionMessage), res);
}
});
// There can be additional files that might be required at the runtime.
// Since the remote renderer doesn't contain any assets, they must be uploaded manually.
app.post<{
Body: WithBodyArrayField<Record<string, Asset>, 'targetBundles'>;
}>('/upload-assets', async (req, res) => {
if (!(await requestPrechecks(req, res))) {
return;
}
let lockAcquired = false;
let lockfileName: string | undefined;
const assets: Asset[] = Object.values(req.body).filter(isAsset);
// Handle targetBundles as either a string or an array
const targetBundles = extractBodyArrayField(req.body, 'targetBundles');
if (!targetBundles || targetBundles.length === 0) {
const errorMsg = 'No targetBundles provided. As of protocol version 2.0.0, targetBundles is required.';
log.error(errorMsg);
await setResponse(errorResponseResult(errorMsg), res);
return;
}
const assetsDescription = JSON.stringify(assets.map((asset) => asset.filename));
const taskDescription = `Uploading files ${assetsDescription} to bundle directories: ${targetBundles.join(', ')}`;
try {
const { lockfileName: name, wasLockAcquired, errorMessage } = await lock('transferring-assets');
lockfileName = name;
lockAcquired = wasLockAcquired;
if (!wasLockAcquired) {
const msg = formatExceptionMessage(
taskDescription,
errorMessage,
`Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`,
);
await setResponse(errorResponseResult(msg), res);
} else {
log.info(taskDescription);
try {
// Prepare all directories first
const directoryPromises = targetBundles.map(async (bundleTimestamp) => {
const bundleDirectory = getBundleDirectory(bundleTimestamp);
// Check if bundle directory exists, create if not
if (!(await fileExistsAsync(bundleDirectory))) {
log.info(`Creating bundle directory: ${bundleDirectory}`);
await mkdir(bundleDirectory, { recursive: true });
}
return bundleDirectory;
});
const bundleDirectories = await Promise.all(directoryPromises);
// Copy assets to each bundle directory
const assetCopyPromises = bundleDirectories.map(async (bundleDirectory) => {
await copyUploadedAssets(assets, bundleDirectory);
log.info(`Copied assets to bundle directory: ${bundleDirectory}`);
});
await Promise.all(assetCopyPromises);
// Delete assets from uploads directory
await deleteUploadedAssets(assets);
await setResponse(
{
status: 200,
headers: {},
},
res,
);
} catch (err) {
const msg = 'ERROR when trying to copy assets';
const message = `${msg}. ${err}. Task: ${taskDescription}`;
log.error({
msg,
err,
task: taskDescription,
});
await setResponse(errorResponseResult(message), res);
}
}
} finally {
if (lockAcquired) {
try {
if (lockfileName) {
await unlock(lockfileName);
}
} catch (error) {
log.warn({
msg: `Error unlocking ${lockfileName} from worker ${workerIdLabel()}`,
err: error,
task: taskDescription,
});
}
}
}
});
// Checks if file exist
app.post<{
Querystring: { filename: string };
Body: WithBodyArrayField<Record<string, unknown>, 'targetBundles'>;
}>('/asset-exists', async (req, res) => {
if (!(await isAuthenticated(req, res))) {
return;
}
const { filename } = req.query;
if (!filename) {
const message = `ERROR: filename param not provided to GET /asset-exists`;
log.info(message);
await setResponse(errorResponseResult(message), res);
return;
}
// Handle targetBundles as either a string or an array
const targetBundles = extractBodyArrayField(req.body, 'targetBundles');
if (!targetBundles || targetBundles.length === 0) {
const errorMsg = 'No targetBundles provided. As of protocol version 2.0.0, targetBundles is required.';
log.error(errorMsg);
await setResponse(errorResponseResult(errorMsg), res);
return;
}
// Check if the asset exists in each of the target bundles
const results = await Promise.all(
targetBundles.map(async (bundleHash) => {
const assetPath = getAssetPath(bundleHash, filename);
const exists = await fileExistsAsync(assetPath);
if (exists) {
log.info(`/asset-exists Uploaded asset DOES exist in bundle ${bundleHash}: ${assetPath}`);
} else {
log.info(`/asset-exists Uploaded asset DOES NOT exist in bundle ${bundleHash}: ${assetPath}`);
}
return { bundleHash, exists };
}),
);
// Asset exists if it exists in all target bundles
const allExist = results.every((result) => result.exists);
await setResponse({ status: 200, data: { exists: allExist, results }, headers: {} }, res);
});
app.get('/info', (_req, res) => {
res.send({
node_version: process.version,
renderer_version: packageJson.version,
});
});
// In tests we will run worker in master thread, so we need to ensure server
// will not listen:
// we are extracting worker from cluster to avoid false TS error
const { worker } = cluster;
if (workersCount === 0 || cluster.isWorker) {
app.listen({ port }, () => {
const workerName = worker ? `worker #${worker.id}` : 'master (single-process)';
log.info(`Node renderer ${workerName} listening on port ${port}!`);
});
}
fastifyConfigFunctions.forEach((configFunction) => {
configFunction(app);
});
return app;
}
</shell_result>
Script executed:
# Search for disableHttp2 usage in the codebase
rg -n "disableHttp2" --type ts --type tsx -C 2Repository: shakacode/react_on_rails
<shell_result>
Repository: shakacode/react_on_rails
Exit code: 2
stderr:
unrecognized file type: tsx
</shell_result>
Script executed:
# Search for disableHttp2 usage in the codebase (correct file types)
rg -n "disableHttp2" --type ts -C 2Repository: shakacode/react_on_rails
<shell_result>
Repository: shakacode/react_on_rails
Exit code: 0
stdout:
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-5-// eslint-disable-next-line import/no-relative-packages
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-6-import packageJson from '../package.json';
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts:7:import worker, { disableHttp2 } from '../src/worker';
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-8-import {
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-9- BUNDLE_TIMESTAMP,
--
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-30-const railsEnv = 'test';
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-31-
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts:32:disableHttp2();
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-33-
packages/react-on-rails-pro-node-renderer/tests/worker.test.ts-34-describe('worker', () => {
--
packages/react-on-rails-pro-node-renderer/src/worker.ts-94-
packages/react-on-rails-pro-node-renderer/src/worker.ts-95-// Call before any test using `app.inject()`
packages/react-on-rails-pro-node-renderer/src/worker.ts:96:export const disableHttp2 = () => {
packages/react-on-rails-pro-node-renderer/src/worker.ts-97- useHttp2 = false;
packages/react-on-rails-pro-node-renderer/src/worker.ts-98-};
--
knip.ts-70- 'src/default-node-renderer.ts!',
knip.ts-71- 'src/integrations/*.ts!',
knip.ts:72: // Export disableHttp2 for test utilities
knip.ts-73- 'src/worker.ts!',
knip.ts-74- ],
</shell_result>
Remove the ! production marker from src/worker.ts.
The disableHttp2 export is test-only (called at module level in tests/worker.test.ts to configure Fastify for testing via app.inject()), not a production entry point. The file itself is not exported in package.json, so it should not be marked with ! (production marker). Either remove the ! suffix to treat it as a dev/test-only entry, or remove the entry entirely and let Knip detect it via test imports.
🤖 Prompt for AI Agents
In knip.ts around lines 72 to 73, the production marker '!' is incorrectly
applied to 'src/worker.ts!' making a test-only file appear as a production
entry; remove the trailing '!' from that entry (change 'src/worker.ts!' to
'src/worker.ts') or delete the entry entirely so Knip treats it as a
dev/test-only file discovered via test imports.
| bundle_install_in(example_type.dir) | ||
| end | ||
|
|
||
| sh_in_dir(example_type.dir, "npm install") |
There was a problem hiding this comment.
Use pnpm install instead of npm install.
Based on learnings, this repository should NEVER use npm commands; only pnpm equivalents should be used for JavaScript package management.
- sh_in_dir(example_type.dir, "npm install")
+ sh_in_dir(example_type.dir, "pnpm install")📝 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.
| sh_in_dir(example_type.dir, "npm install") | |
| sh_in_dir(example_type.dir, "pnpm install") |
🤖 Prompt for AI Agents
In react_on_rails/rakelib/shakapacker_examples.rake around line 114, replace the
call that runs "npm install" with the pnpm equivalent; update the sh_in_dir
invocation to execute "pnpm install" so the project uses pnpm for package
installation consistently across scripts. Ensure the command string is exactly
"pnpm install" and leave any surrounding helpers/arguments unchanged.
The lockfile was generated with React 18.0.0 but package.json specifies ^19.0.0. This caused CI failures with --frozen-lockfile. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use unbundled_sh_in_dir to properly isolate the bundler environment when running generate_packs in generated example apps. The previous approach with BUNDLE_GEMFILE env var didn't work because bundler still used cached gem paths from the parent rake context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add unbundled option to run_tests_in to use unbundled_sh_in_dir for minimum version example tests. This ensures the example app's Gemfile and gem versions are used (e.g., Shakapacker 8.2.0) instead of the parent workspace's bundle context (which has Shakapacker 9.4.0). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
chromedriver-helper was deprecated in March 2019 and is incompatible with modern selenium-webdriver (4.x). The gem tries to set Selenium::WebDriver::Chrome.driver_path which no longer exists. Modern selenium-webdriver uses webdriver-manager internally for driver management. GitHub Actions and most CI environments have Chrome and ChromeDriver pre-installed, so no driver helper gem is needed. This fixes the minimum version example tests which were failing with: "undefined method `driver_path=' for Selenium::WebDriver::Chrome:Module" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update @types/react and @types/react-dom to v19 to match React 19 runtime - Fix Gemfile regex to handle multi-line gem declarations - Document why bundle isolation is only needed for minimum version examples - Improve script/convert fallback logic to handle all file path patterns with warning when file not found 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1. Refactor apply_minimum_versions into focused helper methods: - update_react_dependencies: Updates React and React-DOM versions - update_shakapacker_dependency: Updates Shakapacker in deps or devDeps - update_package_json_versions: Parses JSON and coordinates updates - update_gemfile_versions: Updates Gemfile with pinned version - apply_minimum_versions: Orchestrates both file updates This eliminates all RuboCop metric disables and improves testability. 2. Improve error message to include full package.json path instead of just the directory, making debugging easier. 3. Fix task description symmetry - both latest and minimum version task descriptions now specify exact versions for clarity: - latest: "React 19, Shakapacker 9.4.0" - minimum: "React 18, Shakapacker 8.2.0" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
React 19's type definitions removed the deprecated hydrate, render, and unmountComponentAtNode APIs. Added a LegacyReactDOM interface to provide types for these methods that still exist at runtime for React 16/17 compatibility. Also removes unnecessary eslint-disable directives and fixes ReactElement type parameter warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Ruby 3.4 changed how hash-to-keyword argument conversion works. Changed from passing a hash as a positional argument to explicit keyword arguments to ensure compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
jg/version-compat-smoke
branch
from
5a754f9 to
7288eef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
129-129: Usepnpm installinstead ofnpm install.Based on coding guidelines, this repository should NEVER use
npmcommands; onlypnpmequivalents should be used for JavaScript package management.Apply this diff:
- sh_in_dir(example_type.dir, "npm install") + sh_in_dir(example_type.dir, "pnpm install")
🧹 Nitpick comments (3)
react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (1)
2-15: Namedthunkimport and middleware wiring look correct; consider more idiomaticcreateStorecallUsing a named import for
thunkmatches redux-thunk v3+ where the default export was removed andthunkis now the primary named export, so this change is functionally correct as long as the dummy app is indeed on redux-thunk 3.x. (github.com)As a small readability tweak (optional), you could switch to the more common 3‑argument
createStoreform:- return applyMiddleware(thunk)(createStore)(combinedReducer, newProps); + return createStore(combinedReducer, newProps, applyMiddleware(thunk));Behavior should be equivalent, but the intent is a bit clearer to most Redux readers.
Please confirm that
react_on_rails/spec/dummy/package.jsonis pinned to redux-thunk 3.x so this named import is valid across all environments (including CI and any Storybook/demo setups).react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx (1)
4-40: SSR HelmetProvider + context usage looks solid; consider slightly more defensive title extractionThe switch to
@dr.pogodin/react-helmetwith:
- a per-request
helmetContext = {}passed to<HelmetProvider context={helmetContext}>, and- post‑render extraction via
const { helmet } = helmetContextis the right pattern for thread‑safe SSR and aligns with the “broken” example you added for test coverage.
One small optional hardening: if the library API or usage ever changes so that
helmetis set buthelmet.titleis missing,helmet.title.toString()would throw. You could make this a bit more defensive without changing behavior in the happy path:- const renderedHtml = { - componentHtml, - title: helmet ? helmet.title.toString() : '', - }; + const renderedHtml = { + componentHtml, + title: helmet && helmet.title ? helmet.title.toString() : '', + };Not a blocker, just a safeguard in case of future changes to the Helmet usage or configuration.
react_on_rails/rakelib/shakapacker_examples.rake (1)
60-76: Gemfile regex may be fragile with unusual formatting.The regex pattern
/gem ['"]shakapacker['"][^\n]*(?:\n\s+[^g\n][^\n]*)*$/mattempts to handle multi-line gem declarations but uses[^g\n]to avoid matching the next gem line. This could break if a continuation line starts with 'g' but isn't a gem declaration, or if the Gemfile has unusual formatting.However, since this operates on generated example Gemfiles with predictable structure, it's likely safe in practice. Consider adding a comment documenting the expected Gemfile format or using a more robust parsing approach (e.g., Bundler::Definition) if issues arise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.github/workflows/examples.yml(2 hunks).github/workflows/integration-tests.yml(2 hunks)package.json(2 hunks)packages/react-on-rails/src/ClientRenderer.ts(0 hunks)packages/react-on-rails/src/reactApis.cts(3 hunks)packages/react-on-rails/src/types/index.ts(1 hunks)react_on_rails/lib/generators/react_on_rails/dev_tests_generator.rb(1 hunks)react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb(1 hunks)react_on_rails/rakelib/example_type.rb(1 hunks)react_on_rails/rakelib/examples_config.yml(1 hunks)react_on_rails/rakelib/run_rspec.rake(4 hunks)react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx(2 hunks)react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx(1 hunks)react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx(2 hunks)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx(2 hunks)react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx(2 hunks)react_on_rails/spec/dummy/package.json(1 hunks)react_on_rails/spec/react_on_rails/generators/dev_tests_generator_spec.rb(1 hunks)script/convert(2 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails/src/ClientRenderer.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
- react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
- .github/workflows/examples.yml
- script/convert
- react_on_rails/rakelib/example_type.rb
- .github/workflows/integration-tests.yml
- react_on_rails/spec/dummy/package.json
- react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
- react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
- package.json
- react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx
- react_on_rails/rakelib/examples_config.yml
- react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx
- react_on_rails/spec/react_on_rails/generators/dev_tests_generator_spec.rb
- react_on_rails/lib/generators/react_on_rails/dev_tests_generator.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/run_rspec.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxpackages/react-on-rails/src/types/index.tsreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxpackages/react-on-rails/src/reactApis.ctsreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/rakelib/run_rspec.rake
🧠 Learnings (25)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbpackages/react-on-rails/src/types/index.tsreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbpackages/react-on-rails/src/types/index.ts
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: In TypeScript files, use RBS (Ruby Signature) for static type checking with Steep
Applied to files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.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:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.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/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/run_rspec.rake
📚 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/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rbreact_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxpackages/react-on-rails/src/types/index.tsreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxpackages/react-on-rails/src/reactApis.ctsreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxpackages/react-on-rails/src/types/index.tsreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsxpackages/react-on-rails/src/reactApis.ctsreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsxreact_on_rails/rakelib/run_rspec.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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxpackages/react-on-rails/src/reactApis.ctsreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsxpackages/react-on-rails/src/reactApis.ctsreact_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
📚 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/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
📚 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:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER use `npm` commands; only use equivalent `pnpm` commands for JavaScript package management
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT manually format code during conflict resolution; always use `rake autofix` or `pnpm run format` for automated formatting
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/rakelib/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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/tasks/*.rake : Rails::Engine rake tasks should NOT be explicitly loaded via `rake_tasks` block; instead, place them in `lib/tasks/*.rake` where Rails::Engine automatically loads them
Applied to files:
react_on_rails/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:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
react_on_rails/rakelib/run_rspec.rake
🧬 Code graph analysis (5)
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)
configuration(9-38)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx (4)
helmetContext(21-21)helmetContext(27-27)componentHtml(22-26)props(20-34)
react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx (3)
react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx (1)
combinedReducer(27-27)react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx (1)
combinedReducer(32-32)react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx (1)
combinedReducer(26-26)
packages/react-on-rails/src/reactApis.cts (1)
packages/react-on-rails/src/types/index.ts (1)
RenderReturnType(267-267)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx (1)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx (1)
props(10-14)
⏰ 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). (12)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- 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: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: check-bundle-size
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (17)
react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx (1)
3-14: Client-side HelmetProvider wrapping is correct and consistent with SSR setupImporting
HelmetProviderfrom@dr.pogodin/react-helmetand wrapping<ReactHelmet {...props} />:export default (props) => ( <HelmetProvider> <ReactHelmet {...props} /> </HelmetProvider> );matches the library’s expected usage and keeps client behavior aligned with the new server implementation that also uses
HelmetProvider. No issues from my side here.packages/react-on-rails/src/types/index.ts (1)
128-130: Ensure type changes are compatible with React 18.0.0+ and React 19 before merging.This consistency improvement aligns types 128 and 130 with line 140's existing use of non-generic
ReactElement. However, verify that the unparameterizedReactElementtype works correctly across both the minimum supported React version (18.0.0) and React 19, particularly given React 19's changes to default type parameters forReactElement["props"]. Since these are return types and not directly accessed for props, the practical impact is minimal, but confirmation with TypeScript compilation is recommended.packages/react-on-rails/src/reactApis.cts (5)
7-13: LGTM!The interface correctly defines the legacy React DOM APIs removed from @types/react-dom@19. The types match the expected signatures for React 16/17 compatibility.
40-42: LGTM!The type cast is necessary to access legacy APIs that exist at runtime but were removed from @types/react-dom@19. The double cast pattern through
unknownis the correct approach for this type of assertion.
44-47: LGTM!The updated implementation correctly uses
legacyReactDOM.hydratefor React 16/17 compatibility. The ESLint comment clarification is helpful.
49-57: LGTM!The updated implementation correctly uses
legacyReactDOM.renderfor React 16/17 compatibility while maintaining the proper return type.
7-62: Verify React 16/17 support is intentional.The PR objectives state the minimum supported React version is 18.0.0, but this code maintains runtime compatibility with React 16/17 through the legacy API paths (when
supportsRootApiis false).Please confirm whether this is intentional—for example, if the library still supports React 16/17 at runtime while only testing against 18+ in CI.
react_on_rails/lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
12-15: LGTM! Clean refactor to keyword arguments.The conversion from hash-based options to direct keyword arguments is idiomatic Ruby and improves readability. The behavior remains unchanged.
react_on_rails/rakelib/run_rspec.rake (4)
85-89: LGTM! Correct use of unbundled mode for minimum-version examples.The conditional unbundled execution ensures minimum-version examples use their own Gemfile versions rather than inheriting the parent workspace's bundle context. This aligns with the PR's objective to test minimum supported dependencies in isolation.
97-114: LGTM! Well-structured example filtering and task definitions.The helper methods cleanly separate latest and minimum version examples, and the new rake tasks properly declare dependencies and provide clear descriptions. This enables CI to run the appropriate example suites based on dependency levels.
164-174: LGTM! Comprehensive documentation of the unbundled option.The documentation clearly explains the rationale for Bundler isolation in minimum-version examples and helps future maintainers understand when and why to use the unbundled flag.
180-196: LGTM! Clean implementation of unbundled execution.The conditional logic correctly switches between bundled and unbundled execution contexts. The command string works in both modes since
bundle execwill use the appropriate Gemfile based on the execution context set by the helper methods.react_on_rails/rakelib/shakapacker_examples.rake (5)
11-11: LGTM! Required for JSON parsing functionality.The
jsonrequire is necessary for the newupdate_package_json_versionshelper that parses and writes package.json files.
19-58: LGTM! Well-structured package.json update helpers.The helpers correctly pin minimum versions for React, React-DOM, and Shakapacker. The JSON parsing includes proper error handling that logs context before re-raising, which will help diagnose malformed package.json files in CI. The webpack-assets-manifest version (^5.0.6) correctly matches Shakapacker 8.2.0's requirements, and the logic appropriately checks both dependencies and devDependencies.
78-86: LGTM! Clear orchestration and informative output.The
apply_minimum_versionshelper cleanly coordinates the package.json and Gemfile updates and provides useful output for debugging the version pinning.
119-127: LGTM! Correct sequencing of version updates and bundle installs.The flow properly handles the multi-stage installation: first installing generator-added gems, then conditionally applying minimum versions and reinstalling. The comments clearly explain why each bundle install is necessary.
131-134: LGTM! Correct use of unbundled mode for pack generation.Using
unbundled_sh_in_dirensures the generated app's Gemfile and gem versions are used rather than the parent workspace's bundle, which is essential for minimum-version examples that have different gem versions pinned.
The yalc-linked react-on-rails package was causing npm to install React 19 as a transitive dependency, even when package.json specified React 18.0.0. Added npm overrides to force React 18 versions for minimum version compatibility testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Added react_version parameter to ExampleType for specifying React versions - Added REACT_VERSIONS constant mapping major versions to specific versions - Updated examples_config.yml with React 17 and React 18 test configurations - Renamed test examples from *-minimum to *-react18 and *-react17 for clarity - Added new rake tasks: shakapacker_examples_react17, shakapacker_examples_react18, and shakapacker_examples_pinned - Updated CI workflow to run all pinned version tests (React 17 and 18) - Added npm overrides to force specific React versions in generated apps - Use --legacy-peer-deps for all pinned React version examples This ensures the react-on-rails package works correctly with: - React 19 (latest, uses Root API) - React 18 (uses Root API) - React 17 (uses legacy render/hydrate API) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
136-139: Usepnpminstead ofnpmfor package installation.Based on learnings, this repository should NEVER use
npmcommands; onlypnpmequivalents should be used.Note: pnpm handles peer dependencies differently than npm. Instead of
--legacy-peer-deps, you may need to:
- Use
pnpm install --strict-peer-dependencies=false, or- Configure
.npmrcwithstrict-peer-dependencies=falseApply this diff:
- npm_install_cmd = example_type.minimum_versions? ? "npm install --legacy-peer-deps" : "npm install" + npm_install_cmd = example_type.minimum_versions? ? "pnpm install --strict-peer-dependencies=false" : "pnpm install"
🧹 Nitpick comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
67-83: Consider documenting regex limitations or using a more robust approach.The regex
/gem ['"]shakapacker['"][^\n]*(?:\n\s+[^g\n][^\n]*)*$/massumes continuation lines won't start with 'g' to avoid matching the next gem. This could fail if there's a comment or content starting with 'g' between declarations.Given the controlled context of generated example apps, this is likely acceptable. However, consider either:
- Adding a comment explaining this assumption
- Using a more specific pattern (e.g., matching until the next line that starts with a word character at column 0:
/gem ['"]shakapacker['"][^\n]*(?:\n\s+[^\n]*)*(?=\n\w|$)/m)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/shakapacker_examples.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/rakelib/shakapacker_examples.rake
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 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:
react_on_rails/rakelib/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/rakelib/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/rakelib/shakapacker_examples.rake
📚 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/rakelib/shakapacker_examples.rake
📚 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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER use `npm` commands; only use equivalent `pnpm` commands for JavaScript package management
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT manually format code during conflict resolution; always use `rake autofix` or `pnpm run format` for automated formatting
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/rakelib/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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
react_on_rails/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). (12)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- 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: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: check-bundle-size
- GitHub Check: claude-review
🔇 Additional comments (4)
react_on_rails/rakelib/shakapacker_examples.rake (4)
11-11: LGTM!The
require "json"addition is necessary for the JSON parsing and generation functionality in the new helper methods.
19-93: LGTM! Well-structured version pinning helpers.The helper methods correctly implement minimum version pinning:
update_react_dependenciesappropriately pins React and related packagesupdate_shakapacker_dependencychecks devDependencies first (correct priority)update_package_json_versionsincludes npm overrides to prevent transitive dependencies from pulling newer React versions- JSON parse error handling correctly re-raises to fail fast in CI
- Explicit newline on line 64 ensures proper file formatting
140-144: LGTM! Correct use of unbundled context.Using
unbundled_sh_in_dirfor thegenerate_packstask ensures the generated app's Gemfile and gem versions are used instead of the parent workspace's bundle context. This is essential to avoid version mismatches when testing minimum versions.
1-156: Verify RuboCop compliance for this rake file.This file must be verified with
bundle exec rubocopbefore commit per coding guidelines. Ensure all RuboCop violations are resolved.
Code Review for PR #2125This PR successfully refactors React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. Overall, the implementation is well-structured with good separation of concerns. Here are my findings: ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
134-137: Usepnpm installinstead ofnpm install.Per coding guidelines, this repository should use
pnpmfor JavaScript package management. The--legacy-peer-depsflag also works with pnpm.- npm_install_cmd = example_type.pinned_react_version? ? "npm install --legacy-peer-deps" : "npm install" + npm_install_cmd = example_type.pinned_react_version? ? "pnpm install --legacy-peer-deps" : "pnpm install" sh_in_dir(example_type.dir, npm_install_cmd)
🧹 Nitpick comments (1)
packages/react-on-rails/src/reactApis.cts (1)
7-13: LegacyReactDOM shim correctly restores removed ReactDOM typesThe
LegacyReactDOMinterface pluslegacyReactDOMcast is a reasonable, tightly-scoped way to bridge the removedhydrate/render/unmountComponentAtNodetypings while still using the runtime APIs only whensupportsRootApiis false. If this also fixes a user-visible TypeScript break with React 19 typings, consider adding a brief note to/CHANGELOG.mdso downstream users can discover it. Based on learnings, this kind of runtime-facing behavior change is typically documented.Also applies to: 40-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/examples.yml(2 hunks)packages/react-on-rails/src/reactApis.cts(3 hunks)react_on_rails/rakelib/example_type.rb(1 hunks)react_on_rails/rakelib/examples_config.yml(2 hunks)react_on_rails/rakelib/run_rspec.rake(4 hunks)react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/shakapacker_examples.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/rakelib/run_rspec.rakepackages/react-on-rails/src/reactApis.ctsreact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
🧠 Learnings (23)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/rakelib/run_rspec.rakepackages/react-on-rails/src/reactApis.ctsreact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
📚 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/rakelib/run_rspec.rakepackages/react-on-rails/src/reactApis.ctsreact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.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:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/example_type.rbreact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/example_type.rb
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Resolve merge conflicts by: resolving logical conflicts first, verifying file paths with grep, testing affected scripts, auto-fixing formatting with `rake autofix`, and testing critical scripts if build configs changed
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 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/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.rakereact_on_rails/rakelib/examples_config.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/tasks/*.rake : Rails::Engine rake tasks should NOT be explicitly loaded via `rake_tasks` block; instead, place them in `lib/tasks/*.rake` where Rails::Engine automatically loads them
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
react_on_rails/rakelib/run_rspec.rakereact_on_rails/rakelib/shakapacker_examples.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:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
react_on_rails/rakelib/run_rspec.rake
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/src/reactApis.cts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails/src/reactApis.cts
📚 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:
react_on_rails/rakelib/shakapacker_examples.rake
📚 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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER use `npm` commands; only use equivalent `pnpm` commands for JavaScript package management
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT manually format code during conflict resolution; always use `rake autofix` or `pnpm run format` for automated formatting
Applied to files:
react_on_rails/rakelib/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/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
🧬 Code graph analysis (1)
packages/react-on-rails/src/reactApis.cts (1)
packages/react-on-rails/src/types/index.ts (1)
RenderReturnType(267-267)
⏰ 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). (12)
- GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
- GitHub Check: dummy-app-integration-tests (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: 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: check-bundle-size
- GitHub Check: claude-review
🔇 Additional comments (23)
.github/workflows/examples.yml (4)
188-188: Verify rake task name: "pinned" vs "minimum"The workflow calls
run_rspec:shakapacker_examples_pinned, but the PR objectives (and related rake files from the summary) referencerun_rspec:shakapacker_examples_minimum. Confirm whether this task should be namedminimuminstead.
140-142: Good: Conversion script scoped to minimum dependency levelThe conversion script is correctly gated to run only for
minimumdependency level, aligning with the PR objective to run it only for older Node compatibility testing.
67-85: Good: Matrix setup supports dependency-level varianceThe matrix correctly:
- Runs full matrix (both
latestandminimum) on master, workflow_dispatch, full-ci label, or force_run- Runs only
latestfor regular PRs (fast feedback)This aligns with the PR objectives to test the full compatibility matrix in comprehensive runs while keeping PR feedback fast.
183-188: Good: Conditional execution of appropriate rake tasksThe split of Main CI into two conditional steps based on dependency level is logically sound. Each dependency level now runs its corresponding rake task—assuming the task names are corrected per the earlier comment.
react_on_rails/rakelib/example_type.rb (4)
17-25: LGTM!Well-documented version constants with appropriate use of
freezefor immutability. Thenilfor React 19 to indicate "latest" is a clean pattern.
29-39: LGTM!Clean implementation with good fallback behavior in
react_version_stringthat allows custom version strings not in the predefined mapping.
41-44: LGTM!The legacy method provides backward compatibility while the codebase transitions to the new
react_versionparameter. The string comparison is consistent with howreact_versionis set ininitialize.
46-54: LGTM!Good backward-compatible design that prioritizes the explicit
react_versionparameter while still supporting the legacyminimum_versionsflag.react_on_rails/rakelib/examples_config.yml (2)
1-10: LGTM!Clear documentation comment indicating the latest version expectations for the default examples.
11-26: LGTM!Well-structured compatibility test configurations with clear comments distinguishing React 18 (Root API) from React 17 (legacy render/hydrate API). The decision to limit pinned versions to basic and server-rendering variants (excluding redux) is a reasonable trade-off for CI efficiency.
react_on_rails/rakelib/shakapacker_examples.rake (6)
19-25: LGTM!Clean helper method with appropriate nil guard.
27-34: LGTM!Correct precedence logic checking
devDependenciesbeforedependencies, with safe navigation to handle nil cases.
36-63: LGTM!Well-implemented with proper JSON error handling that logs context before re-raising. The
overridesaddition is crucial for preventing transitive dependency issues with yalc-linked packages.
65-81: LGTM with a note.The regex handles common Gemfile patterns well. The
[^g\n]exclusion in the continuation pattern prevents matching subsequentgemlines but could theoretically fail if a continuation line legitimately starts with 'g'. Given this is used in controlled generator contexts, the risk is minimal.
83-91: LGTM!Good orchestration with helpful logging for CI visibility.
138-143: LGTM!Good use of
unbundled_sh_in_dirto ensure the generated app's Gemfile context is used, avoiding the version mismatch issues mentioned in the PR objectives.react_on_rails/rakelib/run_rspec.rake (5)
81-90: LGTM!Good integration of the
unbundledoption to ensure pinned version examples use their own Gemfile context.
97-112: LGTM!Clean filtering helpers with consistent logic that aligns with the
ExampleTypeimplementation and YAML configuration.
114-138: LGTM!Well-structured tasks with clear descriptions. The
shakapacker_examples_minimumlegacy alias provides backward compatibility while the codebase transitions to the more granular version-specific tasks.
188-198: LGTM!Excellent documentation explaining both the option and the rationale for bundler isolation. This will help maintainers understand the design decisions.
199-220: LGTM!Clean implementation with backward-compatible default. The conditional dispatch to
unbundled_sh_in_diris appropriate. Verified thatunbundled_sh_in_diris defined intask_helpers.rbat line 32.packages/react-on-rails/src/reactApis.cts (2)
44-47: Hydrate/render branching between root and legacy APIs looks soundThe
reactHydrateandreactRenderimplementations correctly delegate tohydrateRoot/createRootwhensupportsRootApiis true and to the legacyhydrate/renderwhen false, with argument order normalized through the wrapper. The non‑null assertion onreactDomClientis safe given the module‑level initialization guarded bysupportsRootApi.Also applies to: 56-56
59-63: Unmount helper now has a consistent, well‑typed signatureExporting
unmountComponentAtNodeas(container: Element) => booleanand making the root‑API branch a typed no‑op resolves the earlier inconsistent union of function signatures while keeping the legacy path wired tolegacyReactDOM.unmountComponentAtNode. This should simplify call sites and typings without changing runtime behavior in the root path.
When downgrading from Shakapacker 9.x to 8.2.x, the bin/shakapacker binstub needs to be regenerated as the format may differ between major versions. Without this, the binstub fails immediately when running webpack. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update the pinned version examples step name from "React 17 and 18" to "minimum: React 18.0.0, Shakapacker 8.2.0" for accuracy. This makes the workflow label correctly document the actual minimum supported versions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The shakapacker:binstubs rake task loads the Rails environment, which validates that the gem and npm package versions match. Previously, npm install ran AFTER the binstubs task, causing version mismatch errors when testing minimum supported versions (gem 8.2.0 vs npm 9.4.0). Reorder operations for pinned React version examples: 1. Update package.json with shakapacker 8.2.0 2. Run bundle install (gem 8.2.0) 3. Run npm install (npm 8.2.0) - MOVED BEFORE binstubs 4. Run shakapacker:binstubs (now versions match) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add runtime validation when using legacy ReactDOM APIs (React < 18) to catch mismatched React versions early with clear error messages. This addresses the type safety concern where `as unknown as LegacyReactDOM` could silently fail at runtime if React structure changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Manual Testing ResultsAddressed code review feedback and manually tested: Changes Made
Manual Tests Performed
Note: The |
Code Review for PR #2125Overall AssessmentThis is a well-architected refactoring that successfully separates concerns between spec/dummy (latest versions) and generator smoke tests (compatibility matrix). The approach is cleaner and more maintainable than the previous version-switching mechanism. ✅ Strengths1. Excellent Separation of Concerns
2. Type Safety Improvements in
|
Quote step name containing colon to prevent YAML parser from interpreting "minimum:" as a mapping key. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: PR #2125 - Move React/Shakapacker version compatibility to generator smoke testsI've reviewed this PR and it looks excellent overall. This is a well-architected refactoring that properly separates concerns between spec/dummy (testing latest versions) and generator smoke tests (testing version compatibility matrix). ✅ Strengths1. Architecture & Design
2. TypeScript Type Safety (reactApis.cts)The React 19 type compatibility handling is excellent:
// Validate legacy APIs exist at runtime when needed (React < 18)
if (!supportsRootApi) {
if (typeof legacyReactDOM.hydrate !== 'function') {
throw new Error('React legacy hydrate API not available. Expected React 16/17.');
}
// ...
}This is best practice for handling breaking type changes in dependencies. 3. Generator Configuration (examples_config.yml)Well-structured:
4. Package Version ManagementThe
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Excellent separation of concerns |
| Code Quality | ⭐⭐⭐⭐ | Very good, minor cleanup needed |
| Type Safety | ⭐⭐⭐⭐⭐ | Excellent React 19 compatibility handling |
| Testing | Local verification required before merge | |
| Documentation | ⭐⭐⭐ | Good, but naming inconsistency |
✅ Approval Status: CONDITIONAL
This PR can be merged AFTER:
- ✅ Completing local testing checklist (mark checkboxes in PR description)
⚠️ Resolving "minimum" vs "pinned" naming inconsistency- ✅ Verifying ESLint passes with removed disable comment
- ℹ️ (Optional) Cleaning up commented code in script/convert
Great work on this refactoring! The architecture is sound and the type safety handling is excellent. Just needs those final verification steps before merging.
🤖 Generated with Claude Code
Clean up historical comments that were kept for reference but are no longer relevant. The remaining comment explains why test:rsc script doesn't need modification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use warn() instead of puts() and flush stderr before exit!(1) to ensure error messages are visible in CI logs. The previous implementation may have been buffering output that wasn't flushed before the hard exit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #2125This is a well-structured refactoring that moves React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests. The approach is sound and the implementation is thorough. Here's my detailed feedback: ✅ Strengths1. Clear Architectural Improvement
2. Well-Designed AbstractionThe
3. Comprehensive Dependency UpdatesReact 19 migration in spec/dummy is properly handled:
4. Excellent Error Handling
begin
package_json = JSON.parse(File.read(package_json_path))
rescue JSON::ParserError => e
puts " ERROR: Failed to parse #{package_json_path}: #{e.message}"
raise
endGood practice to catch and re-raise with context. 5. CI Workflow LogicMatrix setup is smart:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react_on_rails/rakelib/shakapacker_examples.rake (1)
137-137: Usepnpm installinstead ofnpm install.Lines 137 and 142 use
npm install, but based on learnings, this repository should NEVER usenpmcommands; onlypnpmequivalents should be used for JavaScript package management.Apply this diff:
- sh_in_dir(example_type.dir, "npm install --legacy-peer-deps") + sh_in_dir(example_type.dir, "pnpm install --legacy-peer-deps")- sh_in_dir(example_type.dir, "npm install") + sh_in_dir(example_type.dir, "pnpm install")Also applies to: 142-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/examples.yml(2 hunks)packages/react-on-rails/src/reactApis.cts(3 hunks)react_on_rails/lib/react_on_rails/utils.rb(1 hunks)react_on_rails/rakelib/shakapacker_examples.rake(2 hunks)script/convert(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rb,rake}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS run
bundle exec rubocopand fix ALL violations before every commit/push
Files:
react_on_rails/lib/react_on_rails/utils.rbreact_on_rails/rakelib/shakapacker_examples.rake
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
react_on_rails/lib/react_on_rails/utils.rbreact_on_rails/rakelib/shakapacker_examples.rakepackages/react-on-rails/src/reactApis.ctsscript/convert
🧠 Learnings (26)
📓 Common learnings
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
📚 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:
react_on_rails/lib/react_on_rails/utils.rb
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
react_on_rails/lib/react_on_rails/utils.rb.github/workflows/examples.ymlreact_on_rails/rakelib/shakapacker_examples.rakepackages/react-on-rails/src/reactApis.ctsscript/convert
📚 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/lib/react_on_rails/utils.rbreact_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG*.md : Use changelog format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` (no hash in PR number)
Applied to files:
.github/workflows/examples.ymlreact_on_rails/rakelib/shakapacker_examples.rake
📚 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:
.github/workflows/examples.ymlreact_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Distinguish hypothetical fixes from confirmed fixes in commit messages and PR descriptions
Applied to files:
.github/workflows/examples.yml
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 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/rakelib/shakapacker_examples.rakepackages/react-on-rails/src/reactApis.ctsscript/convert
📚 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/rakelib/shakapacker_examples.rakescript/convert
📚 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/rakelib/shakapacker_examples.rakescript/convert
📚 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/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: NEVER use `npm` commands; only use equivalent `pnpm` commands for JavaScript package management
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Do NOT manually format code during conflict resolution; always use `rake autofix` or `pnpm run format` for automated formatting
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rakescript/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to spec/dummy/e2e/playwright/app_commands/**/*.rb : Add custom Playwright app commands in `e2e/playwright/app_commands/` directory as Ruby files using CypressOnRails SmartFactoryWrapper pattern
Applied to files:
react_on_rails/rakelib/shakapacker_examples.rake
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
packages/react-on-rails/src/reactApis.cts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails/src/reactApis.cts
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG_PRO.md : Update `/CHANGELOG_PRO.md` for Pro-only features, fixes, and changes affecting Pro packages
Applied to files:
script/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Resolve merge conflicts by: resolving logical conflicts first, verifying file paths with grep, testing affected scripts, auto-fixing formatting with `rake autofix`, and testing critical scripts if build configs changed
Applied to files:
script/convert
📚 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/convert
📚 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:
script/convert
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
script/convert
🧬 Code graph analysis (1)
packages/react-on-rails/src/reactApis.cts (1)
packages/react-on-rails/src/types/index.ts (1)
RenderReturnType(267-267)
⏰ 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.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: check-bundle-size
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (14)
react_on_rails/lib/react_on_rails/utils.rb (1)
101-106: LGTM! Improved error visibility for CI contexts.The change from
putstowarncorrectly routes failure messages to STDERR, which is the standard channel for error output. Adding$stderr.flushbeforeexit!(1)is essential sinceexit!bypasses normal cleanup and could otherwise lose buffered output. This aligns well with the PR's broader CI improvements.packages/react-on-rails/src/reactApis.cts (6)
7-13: Well-structured interface for legacy API compatibility.The
LegacyReactDOMinterface cleanly encapsulates the removed type definitions and keeps them internal. Good documentation explaining why this exists.
40-42: Appropriate use of type assertion for runtime-only APIs.The double cast pattern is correct here since the legacy APIs exist at runtime but aren't in the type definitions.
44-55: Good defensive validation for legacy API availability.Runtime checks at module load time ensure early failure with clear error messages if there's a React version mismatch. This prevents obscure runtime errors later.
57-70: Clean integration of typed legacy API access.The switch to
legacyReactDOMfor bothhydrateandrendercalls provides type safety while maintaining the same runtime behavior. The ESLint disable comment is well-justified.
72-76: Signature inconsistency resolved.The function now has a consistent signature
(container: Element) => booleanacross both branches. The underscore prefix properly signals the intentionally unused parameter.
84-84: No action required. The file already ends with a newline character and complies with coding guidelines..github/workflows/examples.yml (1)
140-142: ✅ Conversion script step naming is accurate.The rename from "run conversion script to support shakapacker v6" to "Run conversion script for older Node compatibility" appropriately reflects the simplified role of
script/convertper PR #2123 objectives.script/convert (3)
4-10: Documentation update clearly explains the new approach.The updated comments accurately reflect the shift in responsibility: this script now handles only Node.js/tooling compatibility and Pro package adjustments, while React/Shakapacker version compatibility testing has moved to generator smoke tests.
18-38: Path fallback logic provides good resilience across repository structures.The alternate path resolution allows the script to work across different monorepo organizations by trying multiple path patterns before failing. The approach is reasonable and includes appropriate warning when no path is found.
57-65: Pro package test configuration aligns with React 18 RSC limitations.The changes correctly configure the test suite to skip RSC-related tests when React 18 is in use, since RSC requires React 19. The automatic detection mentioned in the comment reduces the need for manual overrides.
react_on_rails/rakelib/shakapacker_examples.rake (3)
19-63: Version pinning helpers are well-structured with proper error handling.The helper methods
update_react_dependencies,update_shakapacker_dependency, andupdate_package_json_for_react_versionprovide clear separation of concerns. The JSON parsing includes appropriate error handling (lines 40-45) and the output formatting includes a trailing newline (line 62) as required by coding guidelines.
127-148: Conditional version pinning logic properly isolates compatibility testing.The generation flow correctly applies version-specific configurations for minimum-version examples while keeping latest-version examples unchanged. The use of
unbundled_sh_in_diron line 148 ensures the generated app's Gemfile context is used rather than the parent workspace's bundle, which is essential for proper version isolation.
65-81: The regex pattern is robust and handles various Gemfile formats correctly.The pattern
/gem ['"]shakapacker['"][^\n]*(?:\n\s+[^g\n][^\n]*)*$/msuccessfully matches both single-line and multi-linegem 'shakapacker'declarations while avoiding subsequent gem lines. Testing confirms it correctly handles multi-line declarations, comments between gems, and group blocks without matching beyond the intended gem declaration. The[^g\n]constraint is effective for this use case and does not introduce fragility.
| - name: Main CI - Latest version examples | ||
| if: matrix.dependency-level == 'latest' | ||
| run: cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_latest | ||
| - name: "Main CI - Pinned version examples (minimum: React 18.0.0, Shakapacker 8.2.0)" | ||
| if: matrix.dependency-level == 'minimum' | ||
| run: cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_pinned |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find react_on_rails -name "run_rspec.rake" -o -name "*rspec*.rake" | head -20Repository: shakacode/react_on_rails
Length of output: 105
🏁 Script executed:
rg -A2 'desc.*shakapacker_examples' --type rubyRepository: shakacode/react_on_rails
Length of output: 50
🏁 Script executed:
rg -n 'task.*:shakapacker_examples' --type rubyRepository: shakacode/react_on_rails
Length of output: 50
🏁 Script executed:
cat -n react_on_rails/rakelib/run_rspec.rake | head -150Repository: shakacode/react_on_rails
Length of output: 7221
🏁 Script executed:
rg -i 'shakapacker_examples' react_on_rails/rakelib/run_rspec.rake -B2 -A2Repository: shakacode/react_on_rails
Length of output: 2659
Change rake task name on line 188 to match the step's minimum version specification.
Line 188 uses run_rspec:shakapacker_examples_pinned, but the step's description and condition (matrix.dependency-level == 'minimum') indicate it should run only minimum-supported version examples (React 18.0.0, Shakapacker 8.2.0). The correct task is run_rspec:shakapacker_examples_minimum. The shakapacker_examples_pinned task runs all pinned versions (both React 17 and 18), which is inconsistent with the "minimum" condition and label.
🤖 Prompt for AI Agents
.github/workflows/examples.yml around lines 183 to 188: the "minimum" matrix job
currently runs the rake task run_rspec:shakapacker_examples_pinned which
executes all pinned versions; change the task name to
run_rspec:shakapacker_examples_minimum so the step runs only the
minimum-supported examples (React 18.0.0, Shakapacker 8.2.0) to match the step
label and condition.
Shakapacker 8.2.0 requires babel-loader to be explicitly installed when babel is configured in the webpack setup. In 9.x, this requirement was relaxed or the package structure changed, so the dependency isn't needed. When downgrading to 8.2.0 for minimum version testing, we need to add babel-loader to devDependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
This PR moves React and Shakapacker version compatibility testing from spec/dummy to the generator smoke tests, as suggested in PR #2114 review.
Key changes:
basic-minimum,basic-server-rendering-minimum) that test React 18.0.0 and Shakapacker 8.2.0script/convertto only handle Node.js tooling compatibility (removed React/Shakapacker version modifications)run_rspec:shakapacker_examples_latest(basic, redux, etc.)run_rspec:shakapacker_examples_minimum(basic-minimum, etc.)Benefits
Test plan
run_rspec:shakapacker_examples_latestincludes only non-minimum examplesrun_rspec:shakapacker_examples_minimumincludes only minimum examplesManual Testing Checklist
Before merging, verify the following locally:
Closes #2123
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Dependencies
Chores
✏️ Tip: You can customize this high-level summary in your review settings.