Move React/Shakapacker version compatibility to generator smoke tests

[justin808]

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:

• Update spec/dummy to always use latest React 19 and Shakapacker 9.4.0
• Add minimum version example apps (basic-minimum, basic-server-rendering-minimum) that test React 18.0.0 and Shakapacker 8.2.0
• Simplify script/convert to only handle Node.js tooling compatibility (removed React/Shakapacker version modifications)
• Update CI workflows to run appropriate examples per dependency level:
  • Latest CI: runs run_rspec:shakapacker_examples_latest (basic, redux, etc.)
  • Minimum CI: runs run_rspec:shakapacker_examples_minimum (basic-minimum, etc.)

Benefits

• Clearer separation: spec/dummy tests latest versions, generators test compatibility matrix
• Simpler CI configuration: spec/dummy integration tests no longer need version conversions
• Better reflects real-world usage: users running the generators get their compatibility tested

Test plan

• Verify rake tasks are created correctly for new example types
• Verify run_rspec:shakapacker_examples_latest includes only non-minimum examples
• Verify run_rspec:shakapacker_examples_minimum includes only minimum examples
• RuboCop passes
• CI runs generator tests with correct examples per dependency level

Closes #2123

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added minimum version compatibility testing to verify support for minimum required React and Shakapacker versions

• Tests

  • Updated React to v19 and upgraded related dependencies
  • Enhanced CI/testing infrastructure with separate test suites for latest and minimum dependency versions
  • Removed deprecated chromedriver-helper

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

Walkthrough

The PR implements minimum-version compatibility testing for React 18.0.0 and Shakapacker 8.2.0 in separate generator example apps, keeping spec/dummy on latest versions. New Rake tasks and configurations enable running test suites separately for latest and minimum-version examples, with support for unbundled dependency resolution in generated apps.

Changes

Cohort / File(s)	Change Summary
Workflow CI step restructuring .github/workflows/examples.yml, .github/workflows/integration-tests.yml	Split single "Main CI" job into conditional steps for latest and minimum dependency versions; renamed conversion step to clarify Node compatibility focus.
Example infrastructure react_on_rails/rakelib/example_type.rb, react_on_rails/rakelib/examples_config.yml	Added minimum-version constants and configuration support; introduced minimum_versions attribute to ExampleType; defined new minimum-version test configurations (basic-minimum, basic-server-rendering-minimum).
Rake task enhancements react_on_rails/rakelib/run_rspec.rake	Added latest_examples and minimum_examples helper methods; introduced shakapacker_examples_latest and shakapacker_examples_minimum Rake tasks; extended run_tests_in with unbundled execution option.
Generator pack workflow react_on_rails/rakelib/shakapacker_examples.rake	Added apply_minimum_versions helper to pin React, react-dom, webpack-assets-manifest, and Shakapacker to minimum supported versions in generated apps; integrated unbundled execution for minimum-version examples.
Conversion script refocus script/convert	Removed version-forcing logic for React and Shakapacker; retained trailing-comma cleanup; refocused on Pro package test configuration (RSC test skip on React 18).
Root package.json updates package.json	Updated React and react-dom from 18.0.0 to ^19.0.0 in devDependencies.
Spec dummy dependencies react_on_rails/spec/dummy/package.json	Updated React, react-dom, react-redux, redux, redux-thunk to latest versions; replaced react-helmet with @dr.pogodin/react-helmet; removed @types/react-helmet.
Redux thunk import refactoring react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx, react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx, react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx, react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx	Updated redux-thunk imports from default to named thunk export across all Redux store initialization files.
React Helmet migration react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx, react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx, react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx, react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.client.jsx, react_on_rails/spec/dummy/client/app/startup/ReactHelmetAppBroken.server.jsx	Migrated from react-helmet to @dr.pogodin/react-helmet; wrapped components with HelmetProvider; updated server rendering to use helmetContext for data extraction.
Test generator updates react_on_rails/lib/generators/react_on_rails/dev_tests_generator.rb, react_on_rails/spec/react_on_rails/generators/dev_tests_generator_spec.rb	Removed deprecated chromedriver-helper gem from test dependencies; added deprecation context noting modern selenium-webdriver handles driver management.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Workflow
    participant Latest as Latest Examples
    participant Minimum as Minimum Examples
    
    CI->>Latest: Run shakapacker_examples_latest
    Latest->>Latest: Load examples with minimum_versions=false
    Latest->>Latest: Run RSpec with bundled deps
    Latest-->>CI: ✓ Tests pass (React 19, Shakapacker latest)
    
    CI->>Minimum: Run shakapacker_examples_minimum
    Minimum->>Minimum: Load examples with minimum_versions=true
    Minimum->>Minimum: apply_minimum_versions() in generated app
    Minimum->>Minimum: Pin React 18.0.0, Shakapacker 8.2.0
    Minimum->>Minimum: Run RSpec with unbundled deps
    Minimum-->>CI: ✓ Tests pass (React 18, Shakapacker 8.2.0)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Specific areas requiring extra attention:
  • react_on_rails/rakelib/shakapacker_examples.rake — the apply_minimum_versions method's package.json and Gemfile manipulation logic; verify regex patterns and error handling for edge cases
  • react_on_rails/rakelib/run_rspec.rake — unbundled execution flow and task dependencies; ensure correct filtering and invocation order
  • script/convert — verify the removal of version replacements doesn't inadvertently break CI for other use cases
  • Redux-thunk and react-helmet migration across multiple files — confirm all imports and usages are consistently updated

Possibly related PRs

• Restructure monorepo with two top-level product directories #2114 — Original issue requesting this change; code review comments proposing minimum-version testing in generator smoke tests rather than spec/dummy
• Apply PR 2039 changes: modernize generator config and workflow #2040 — Modifies the same rakelib/shakapacker_examples.rake and generator command execution flow; adds complementary pack generation enhancements
• Update React to v19 #1677 — Related dependency update; upgrades React and react-dom to ^19.0.0 in package.json files

Suggested reviewers

• AbanoubGhadban
• Judahmeek
• alexeyr-ci

Poem

🐰 Hop-hop, the versions now split clean,
Latest and minimum, a testing dream!
Shakapacker eight, React eighteen bold,
Generator smoke tests, a story retold! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Out of Scope Changes check	❓ Inconclusive	The PR contains changes beyond version compatibility testing: React 19 compatibility updates (redux-thunk imports, react-helmet migration) and extensive Yarn-to-PNPM migration across workflows appear in scope only if part of the objectives.	Clarify whether React 19 compatibility changes and Yarn-to-PNPM migration are intentional scope or should be in a separate PR for cleaner review and release notes.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly summarizes the main change: moving React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests, which is the core objective of this PR.
Linked Issues check	✅ Passed	The PR addresses all coding requirements from issue #2123: new minimum-version example apps added [examples_config.yml], rake tasks for grouping examples implemented [run_rspec.rake], apply_minimum_versions helper added [shakapacker_examples.rake], and CI workflows updated to run different example sets per dependency level.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #2125: Move React/Shakapacker version compatibility to generator smoke tests

✅ Overall Assessment

This 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.

---

🎯 Strengths

1. Clear Separation of Concerns

• spec/dummy now focuses on latest versions only (React 19, Shakapacker 9.4.0)
• Generator examples test the compatibility matrix (minimum versions via basic-minimum and basic-server-rendering-minimum)
• This separation makes it easier to understand what each test suite validates

2. Cleaner CI Configuration

• Simplified CI workflows with explicit tasks: run_rspec:shakapacker_examples_latest vs run_rspec:shakapacker_examples_minimum
• Matrix-based testing now properly separated by dependency level
• Updated step descriptions accurately reflect what's being tested

3. Better Real-World Representation

• Users running generators will have their actual compatibility tested
• Minimum version examples (basic-minimum) directly validate the advertised minimum support

4. Code Quality

• Constants defined at class level (MINIMUM_REACT_VERSION, MINIMUM_SHAKAPACKER_VERSION)
• Single source of truth for minimum versions in ExampleType
• Follows Ruby conventions and project style guidelines

---

⚠️ Potential Issues & Recommendations

1. CRITICAL: Missing Test Coverage for New Rake Tasks

Issue: The new rake tasks (shakapacker_examples_latest, shakapacker_examples_minimum) and helper methods (latest_examples, minimum_examples) have no test coverage.

Recommendation: Add RSpec tests similar to react_on_rails/spec/react_on_rails/rake_tasks_spec.rb:

# react_on_rails/spec/react_on_rails/example_rake_tasks_spec.rb
require 'rake'

RSpec.describe 'Example Rake Tasks' do
  before do
    load File.expand_path('../../rakelib/run_rspec.rake', __dir__)
  end

  describe 'rake run_rspec:shakapacker_examples_latest' do
    it 'is defined' do
      expect(Rake::Task.task_defined?('run_rspec:shakapacker_examples_latest')).to be true
    end

    it 'only includes non-minimum examples' do
      # Test that the task dependencies don't include minimum examples
    end
  end

  describe 'rake run_rspec:shakapacker_examples_minimum' do
    it 'is defined' do
      expect(Rake::Task.task_defined?('run_rspec:shakapacker_examples_minimum')).to be true
    end

    it 'only includes minimum version examples' do
      # Test that the task only runs minimum examples
    end
  end
end

2. Potential Logic Issue: Helper Method Placement

Issue: The helper methods latest_examples and minimum_examples are defined at line 94-100 in run_rspec.rake, but they're only used in task definitions on the same lines. This works in Ruby but could be confusing.

Location: react_on_rails/rakelib/run_rspec.rake:94-100

Current Code:

# 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

Recommendation: These are used at the top-level of the rake file, so they're fine as-is. However, consider adding a comment clarifying they're module-level helpers:

# Module-level helper methods for filtering example types by version support
def latest_examples
  # ...
end

3. Security: JSON Parsing Without Error Handling

Issue: apply_minimum_versions parses JSON without catching potential parse errors.

Location: react_on_rails/rakelib/shakapacker_examples.rake:24

Current Code:

package_json = JSON.parse(File.read(package_json_path))

Recommendation: Add error handling:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  begin
    package_json = JSON.parse(File.read(package_json_path))
  rescue JSON::ParserError => e
    puts "  ERROR: Failed to parse package.json: #{e.message}"
    raise
  end
  
  # ... rest of the method
end

4. Edge Case: Missing Dependencies Hash

Issue: The code checks for package_json["dependencies"] but doesn't verify it exists before trying to update it.

Location: react_on_rails/rakelib/shakapacker_examples.rake:26-30

Current Code:

# Update React versions to minimum supported
if package_json["dependencies"]
  package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
  package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION
end

Issue: This is actually fine - the if check protects against nil. But there's a potential issue if dependencies doesn't contain react/react-dom (they might be in devDependencies).

Recommendation: Log if React is not found:

if package_json["dependencies"]
  if package_json["dependencies"].key?("react")
    package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
    package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION
  else
    puts "  WARNING: React not found in dependencies, checking devDependencies..."
  end
end

5. Documentation: Update CLAUDE.md

Issue: The changes to script/convert significantly change its purpose, but CLAUDE.md isn't updated to reflect this.

Location: CLAUDE.md mentions the conversion script in CI context

Recommendation: Update CLAUDE.md to clarify:

• script/convert now only handles Node.js tooling compatibility
• React/Shakapacker version testing is done via generator examples
• Document the new rake tasks in the "Development Commands" section

6. Performance: Redundant File.exist? Check

Issue: Minor optimization opportunity in apply_minimum_versions.

Current:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  package_json = JSON.parse(File.read(package_json_path))
  # ...
end

Better:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  content = File.read(package_json_path)
  package_json = JSON.parse(content)
  # ...
end

This is very minor and the current code is fine.

---

🔍 Testing Considerations

Manual Testing Checklist (from CLAUDE.md requirements)

Before merging, verify:

1. ✅ RuboCop passes - Listed as complete in test plan
2. ⚠️ CI verification needed - Test plan shows "CI runs generator tests with correct examples per dependency level" as incomplete
3. ❓ Local testing - Per CLAUDE.md critical requirements:

# Test rake task definitions
cd react_on_rails
bundle exec rake -T | grep shakapacker_examples

# Verify latest examples task
bundle exec rake run_rspec:shakapacker_examples_latest --dry-run

# Verify minimum examples task  
bundle exec rake run_rspec:shakapacker_examples_minimum --dry-run

# Test that examples actually generate with correct versions
bundle exec rake shakapacker_examples:gen_basic-minimum
cat gen-examples/basic-minimum/package.json | grep '"react"'
# Should show: "react": "18.0.0"

Integration Testing

CRITICAL PER CLAUDE.md: Before pushing, test the full CI workflow locally:

# Simulate latest CI
bin/ci-switch-config latest
cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_latest

# Simulate minimum CI
bin/ci-switch-config minimum
cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples_minimum

---

📝 Code Style & Best Practices

Adherence to Project Guidelines

✅ Follows CLAUDE.md:

• Uses proper Rake task naming conventions
• Includes descriptive task descriptions
• Follows Ruby style guidelines
• Constants defined at appropriate scope

✅ Follows Rails Engine best practices:

• Rake tasks in lib/tasks/ (actually rakelib/, which is fine for development tasks)
• Proper task dependency management

---

🔒 Security Assessment

Low Risk: This PR primarily refactors test infrastructure. No user-facing code changes.

• ✅ No SQL injection vectors
• ✅ No XSS vulnerabilities
• ✅ No command injection (JSON parsing is safe)
• ⚠️ Minor: JSON parse error handling could be more robust (see recommendation Add linting and CI scripts #3)

---

📊 Performance Considerations

Minimal Impact:

• JSON parsing overhead is negligible (one-time during example generation)
• Rake task filtering is O(n) where n = number of examples (currently 6, minimal)
• No runtime performance impact on end users

---

🎓 Suggestions for Improvement

1. Add Validation for Minimum Versions

Ensure the minimum versions are actually compatible:

# In example_type.rb
def validate_minimum_versions\!
  return unless minimum_versions
  
  # Could add validation that these versions are installable
  # or at least that they're in semver format
  unless MINIMUM_REACT_VERSION.match?(/\d+\.\d+\.\d+/)
    raise "Invalid minimum React version: #{MINIMUM_REACT_VERSION}"
  end
end

2. Make Minimum Versions Configurable

Consider allowing override via environment variables:

MINIMUM_REACT_VERSION = ENV.fetch('MIN_REACT_VERSION', '18.0.0')
MINIMUM_SHAKAPACKER_VERSION = ENV.fetch('MIN_SHAKAPACKER_VERSION', '8.2.0')

This would make it easier to test different minimum versions without code changes.

---

✨ Final Verdict

Recommendation: APPROVE with minor improvements

This PR successfully achieves its goals and is well-implemented. The suggestions above are mostly nice-to-haves rather than blockers.

Before Merging:

1. ✅ Verify CI passes (esp. the new example tasks)
2. 🔧 Consider adding test coverage for new rake tasks
3. 📝 Update CLAUDE.md to document new rake tasks
4. 🛡️ Add JSON parse error handling (optional but recommended)

Changelog Entry Needed?

Per CLAUDE.md: This is user-visible if developers use the rake tasks directly, but it's primarily a testing infrastructure change. Suggest:

#### Improved
- [PR 2125](https://github.com/shakacode/react_on_rails/pull/2125) by [justin808](https://github.com/justin808): Improved version compatibility testing by moving React 18 and Shakapacker 8.2.0 tests to dedicated generator examples (`basic-minimum`, `basic-server-rendering-minimum`)

Great work! 🚀

[claude]

Code Review - PR #2125

Overview

This 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

1. Clear Separation of Concerns: Moving version compatibility testing to generator examples makes logical sense - spec/dummy tests latest versions while generators test the compatibility matrix.

2. Improved CI Configuration: The split between run_rspec:shakapacker_examples_latest and run_rspec:shakapacker_examples_minimum is clean and maintainable.

3. Good Code Organization: The apply_minimum_versions helper function in shakapacker_examples.rake is well-structured with proper JSON handling.

4. Proper Constants: Using MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION constants is good practice.

5. Simplified script/convert: Removing React/Shakapacker version logic from this script makes it more focused.

---

⚠️ Issues & Concerns

1. CRITICAL: Missing minimum_versions attribute accessor

# react_on_rails/rakelib/example_type.rb - LINE 16
attr_reader :packer_type, :name, :generator_options

The PR adds @minimum_versions in the initializer (line 18 in the diff), but DOES NOT add it to attr_reader. This will cause:

• run_rspec.rake:97 - .reject(&:minimum_versions) to fail with NoMethodError
• run_rspec.rake:101 - .select(&:minimum_versions) to fail with NoMethodError

Fix Required:

attr_reader :packer_type, :name, :generator_options, :minimum_versions

2. Missing minimum_versions? predicate method

Ruby convention uses ? suffix for boolean predicates. The code uses &:minimum_versions which works but is less idiomatic.

Recommendation:

def minimum_versions?
  @minimum_versions
end

Then use: .reject(&:minimum_versions?) and .select(&:minimum_versions?)

3. Missing newline at EOF in run_rspec.rake

Per CLAUDE.md: "ALWAYS ensure files end with a newline character"

The new code at lines 93-110 doesn't show a trailing newline in the diff. Verify this locally:

bundle exec rubocop react_on_rails/rakelib/run_rspec.rake

4. script/convert is outdated

The script/convert file shown in your review still contains the OLD logic (lines 29-61 with React/Shakapacker version changes). The diff shows these should be REMOVED.

Verify:

# The script/convert file should look like the PR diff, not the Read output
cat script/convert | grep -A 5 "minimum supported React"

If the old code is still there, the file wasn't properly updated.

---

🔍 Code Quality Observations

1. Good: JSON pretty generation with newline

File.write(package_json_path, "\#{JSON.pretty_generate(package_json)}\n")

This ensures proper formatting and trailing newlines. ✅

2. Good: Informative output

The apply_minimum_versions method provides clear feedback about what versions were applied. ✅

3. Good: Proper error handling in gsub_file_content

The script/convert has proper file existence checks (though this is existing code). ✅

4. Consider: Extract constants to shared location

MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION are defined in example_type.rb but used in shakapacker_examples.rake. While this works, consider if these should live in a shared config file for better discoverability.

---

🧪 Testing Recommendations

Before merging, MANDATORY LOCAL TESTING:

# 1. Verify rake tasks are defined correctly
rake -T | grep shakapacker_examples

# Expected output:
# rake run_rspec:shakapacker_examples
# rake run_rspec:shakapacker_examples_latest
# rake run_rspec:shakapacker_examples_minimum

# 2. Verify minimum examples are detected
cd react_on_rails && bundle exec rake -T | grep minimum

# 3. Test that minimum version application works
rake shakapacker_examples:gen_basic-minimum
cat gen-examples/basic-minimum/package.json | grep '"react"'
# Should show: "react": "18.0.0"

# 4. Verify linting passes
bundle exec rubocop

# 5. Test the actual rake tasks run
rake run_rspec:shakapacker_examples_latest
rake run_rspec:shakapacker_examples_minimum

---

📋 Checklist Before Merge

• Fix missing attr_reader :minimum_versions in example_type.rb:16
• Verify script/convert has the NEW simplified version (not the old one shown in Read output)
• Run bundle exec rubocop and fix ALL violations
• Test locally: rake run_rspec:shakapacker_examples_latest
• Test locally: rake run_rspec:shakapacker_examples_minimum
• Verify CI runs with correct examples per dependency level
• Ensure all files end with newlines

---

🎯 Security & Performance

✅ No security concerns identified
✅ No performance regressions expected
✅ Proper JSON parsing with error handling

---

📝 Documentation

The PR description is excellent and clearly explains the rationale. The inline comments in script/convert properly explain the new scope.

Suggested addition: Update CLAUDE.md or testing docs to mention the new *_minimum example types and their purpose.

---

Summary

This is a well-thought-out refactoring with clear benefits. The main blocker is the missing attr_reader :minimum_versions which will cause immediate runtime failures. Fix that, verify the script/convert file is updated correctly, run rubocop, and this should be good to merge!

🤖 Review generated with Claude Code

[claude]

Code Review - PR #2125

I'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

1. Clear Separation of Concerns: Excellent design decision to have spec/dummy test latest versions while generator smoke tests verify compatibility matrix. This better reflects real-world usage patterns.

2. Clean Implementation: The minimum_versions flag and filtering helpers (latest_examples/minimum_examples) provide a clear, extensible pattern for categorizing test examples.

3. Well-Documented: The script/convert file now has clear comments explaining its purpose and scope after these changes.

4. DRY Constants: Centralizing version numbers in ExampleType::MINIMUM_REACT_VERSION and ExampleType::MINIMUM_SHAKAPACKER_VERSION is good practice.

🔍 Observations & Suggestions

1. Testing Coverage

The PR description mentions:

• Verify rake tasks are created correctly for new example types
• Verify run_rspec:shakapacker_examples_latest includes only non-minimum examples
• Verify run_rspec:shakapacker_examples_minimum includes only minimum examples
• CI runs generator tests with correct examples per dependency level ⚠️

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_minimum

2. Potential Edge Case in apply_minimum_versions

In react_on_rails/rakelib/shakapacker_examples.rake:20-43:

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

Issue: If a generated example doesn't have dependencies section (unlikely but possible), React versions won't be set, but Shakapacker might still be updated. This could cause inconsistent state.

Suggestion: Add validation or logging:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  package_json = JSON.parse(File.read(package_json_path))
  
  # Validate expected structure
  unless package_json["dependencies"]
    puts "  WARNING: No dependencies section in package.json, skipping React version update"
    return
  end

  # Rest of the method...
end

3. Task Dependencies

In react_on_rails/rakelib/run_rspec.rake:103-104:

task shakapacker_examples_latest: latest_examples.map(&:gen_task_name) do
  latest_examples.each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
end

Good: Dependencies are properly set up - the task depends on generation tasks, then invokes the spec tasks. This ensures examples are generated before tests run.

4. Documentation Gap

The new minimum_versions parameter in ExampleType class is not documented in any README or developer guide.

Suggestion: Consider adding a section to CLAUDE.md or developer documentation explaining:

• How to add new minimum version test examples
• What the minimum version examples test
• Why we maintain separate minimum/latest example sets

5. Cleanup of script/convert

Good simplification! The file went from 52 lines to 53 lines, but removed complex React/Shakapacker version management, leaving only Node.js tooling compatibility. The comments clearly explain the new scope.

Question: Are there plans to eventually remove this file entirely, or will it always be needed for Node.js tooling compatibility on minimum versions?

🔒 Security Considerations

No security concerns. The changes are limited to CI/test infrastructure and don't affect production code.

⚡ Performance Considerations

Positive Impact: By separating minimum version tests into dedicated examples, CI can:

• Run only latest examples on PRs (faster feedback)
• Run full matrix only on master/workflow_dispatch
• Better parallelization potential

Note: Adding 2 new generator examples (basic-minimum, basic-server-rendering-minimum) will increase CI time slightly when running the full matrix, but this is acceptable for comprehensive compatibility testing.

📝 Code Quality

• Ruby Style: Code follows project conventions and appears RuboCop-compliant
• Naming: Clear, descriptive names (minimum_versions, apply_minimum_versions, shakapacker_examples_latest)
• YAML Config: Clean structure in examples_config.yml with helpful comments

🐛 Potential Issues

None identified. The implementation looks solid.

✅ Final Recommendation

APPROVE with minor suggestions above. The refactoring achieves its stated goals:

• ✅ Clearer separation: spec/dummy tests latest, generators test compatibility matrix
• ✅ Simpler CI configuration: conditional task execution based on dependency level
• ✅ Better reflects real-world usage: users running generators get compatibility tested

Consider the suggestions above, particularly:

1. Validate CI runs correctly with new tasks
2. Add robustness checks to apply_minimum_versions
3. Document the minimum version pattern for future maintainers

Great work on this refactoring! 🎉

[claude]

Code Review - PR #2125

Summary

This 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

1. Clear separation of concerns: spec/dummy now focuses on latest versions while generator examples test the compatibility matrix
2. Improved CI organization: The split between shakapacker_examples_latest and shakapacker_examples_minimum makes the test matrix more explicit
3. Good code organization: The minimum_versions flag on ExampleType is a clean way to categorize examples
4. Simplified script/convert: Removing React/Shakapacker version logic from this script reduces complexity

---

🔍 Issues & Concerns

1. CRITICAL: Missing attr_reader and minimum_versions? method

In react_on_rails/rakelib/example_type.rb, the PR diff shows:

attr_reader :packer_type, :name, :generator_options, :minimum_versions

But the current file only has:

attr_reader :packer_type, :name, :generator_options

The :minimum_versions attr_reader is missing! This means:

• run_rspec.rake calls to .minimum_versions will fail with NoMethodError
• The filtering methods latest_examples and minimum_examples won't work

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
end

2. Missing run_rspec.rake changes

The PR diff shows new rake tasks should be added to run_rspec.rake:

• shakapacker_examples_latest task
• shakapacker_examples_minimum task
• Helper methods latest_examples and minimum_examples

These changes are not in the current file. Without these:

• CI will fail because run_rspec:shakapacker_examples_latest doesn't exist
• CI will fail because run_rspec:shakapacker_examples_minimum doesn't exist

Required fix: Add the rake tasks as shown in the PR diff.

3. Missing shakapacker_examples.rake changes

The PR diff shows a new apply_minimum_versions method should be added to shakapacker_examples.rake with:

• JSON parsing logic
• Package.json modification to set React 18.0.0 and Shakapacker 8.2.0
• Call to this method when example_type.minimum_versions is true

This code is not present in the current file.

Required fix: Add the apply_minimum_versions method and the conditional call.

4. Missing examples_config.yml entries

The current examples_config.yml doesn't include the minimum version examples:

# Missing from current file:
- name: basic-minimum
  generator_options: ''
  minimum_versions: true
- name: basic-server-rendering-minimum
  generator_options: --example-server-rendering
  minimum_versions: true

Without these entries, no minimum version examples will be generated.

---

🐛 Potential Bugs

5. Predicate method naming inconsistency

In run_rspec.rake, the PR uses:

def latest_examples
  ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
end

def minimum_examples
  ExampleType.all[:shakapacker_examples].select(&:minimum_versions)
end

This calls .minimum_versions as a predicate. In Ruby, predicates typically end with ?. The code should either:

• Use &:minimum_versions? (requires adding a minimum_versions? method to ExampleType)
• Or use a block: .reject { |e| e.minimum_versions }

The symbol-to-proc syntax &:minimum_versions will work if you have the attr_reader, but using minimum_versions? would be more idiomatic.

6. JSON formatting in apply_minimum_versions

The 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 Quality

7. Constants placement is good

The 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 simplification

The removal of React/Shakapacker logic from script/convert is excellent. The script is now more focused on Node.js tooling compatibility. The comments added are helpful.

---

⚡ Performance Considerations

No performance concerns. The changes are primarily organizational and don't introduce performance overhead.

---

🔒 Security Concerns

No security issues identified. The changes involve:

• Test infrastructure configuration
• Package version specifications
• Rake task definitions

All are low-risk from a security perspective.

---

🧪 Test Coverage

Missing verification:

According to the PR description test plan:

• Verify rake tasks are created correctly for new example types
• Verify run_rspec:shakapacker_examples_latest includes only non-minimum examples
• Verify run_rspec:shakapacker_examples_minimum includes only minimum examples
• RuboCop passes
• CI runs generator tests with correct examples per dependency level ⚠️

The last item is unchecked, but it's the most critical one!

Before merging, you should:

1. Apply all the missing code changes identified above
2. Test locally:

   # Verify rake tasks exist
   rake -T | grep shakapacker_examples
   
   # Should show:
   # rake run_rspec:shakapacker_examples_latest
   # rake run_rspec:shakapacker_examples_minimum
   
   # Generate and test one minimum example
   rake shakapacker_examples:gen_basic-minimum
   cd gen-examples/basic-minimum
   cat package.json | grep react  # Should show 18.0.0
   cat package.json | grep shakapacker  # Should show 8.2.0

3. Push and verify CI passes on BOTH matrix levels (latest and minimum)

---

🎯 Recommendations

Must fix before merge:

1. ✅ Add minimum_versions to attr_reader in example_type.rb
2. ✅ Add the new rake tasks to run_rspec.rake
3. ✅ Add the apply_minimum_versions method to shakapacker_examples.rake
4. ✅ Add minimum example entries to examples_config.yml
5. ✅ Add minimum_versions? predicate method for cleaner Ruby code

Nice to have:

• Consider adding a comment in examples_config.yml explaining the minimum_versions flag
• Document the minimum supported versions in a README or changelog entry

---

📋 Checklist for Author

Before marking as ready to merge:

• All code changes from the PR diff are actually present in the files
• bundle exec rubocop passes
• Local testing: rake shakapacker_examples:gen_basic-minimum works
• Local testing: generated package.json has correct versions
• CI passes for both latest and minimum dependency levels
• Trailing newlines on all files (per CLAUDE.md requirements)

---

💡 Overall Assessment

Architecture: 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. 🚀

[claude]

Code Review - PR #2125

Great work on separating version compatibility testing from spec/dummy! This is a cleaner architecture. Here's my detailed review:

✅ Strengths

1. Clear separation of concerns: Moving version compatibility to generator smoke tests is the right architectural decision
2. Good documentation: The updated comments in script/convert clearly explain the new scope
3. Maintainable approach: Centralizing minimum version constants in ExampleType class makes updates easier
4. Proper CI integration: Splitting rake tasks for latest/minimum versions is clean

---

🔍 Issues & Suggestions

1. Missing minimum_versions accessor in example_type.rb (CRITICAL)

The diff shows you're adding:

attr_reader :packer_type, :name, :generator_options, :minimum_versions
def initialize(packer_type: nil, name: nil, generator_options: nil, minimum_versions: false)
  @minimum_versions = minimum_versions
  # ...
end

But the current file I see doesn't have this. Verify the attr_reader includes minimum_versions - otherwise the helper methods in run_rspec.rake will fail:

def minimum_examples
  ExampleType.all[:shakapacker_examples].select(&:minimum_versions)  # ← needs accessor
end

---

2. Potential RuboCop violation in run_rspec.rake

The helper methods are defined at the top level of the namespace without clear scope:

namespace :run_rspec do
  # Helper methods for filtering examples
  def latest_examples
    ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
  end
  # ...
end

Suggestion: Move these to the ReactOnRails::TaskHelpers module for better organization:

module ReactOnRails
  module TaskHelpers
    def latest_examples
      ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
    end

    def minimum_examples
      ExampleType.all[:shakapacker_examples].select(&:minimum_versions)
    end
  end
end

---

3. Missing error handling in apply_minimum_versions

The method reads/writes JSON but doesn't handle parse errors:

def apply_minimum_versions(dir)
  package_json = JSON.parse(File.read(package_json_path))  # ← no error handling
  # ...
end

Suggestion: Add basic error handling:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  begin
    package_json = JSON.parse(File.read(package_json_path))
  rescue JSON::ParserError => e
    puts "  Warning: Failed to parse package.json: #{e.message}"
    return
  end
  # ... rest of method
end

---

4. Verify Shakapacker 9.4.0 compatibility

The diff shows updating spec/dummy to Shakapacker 9.4.0, but I don't see this version explicitly in the diff. Did you mean to include a Gemfile change? Current best practice per CLAUDE.md is to test build scripts after dependency changes.

Recommendation:

cd react_on_rails/spec/dummy
bundle install
pnpm install --frozen-lockfile  # Verify clean install works
pnpm run build                   # Test build

---

5. Test coverage gaps

The test plan shows unchecked items. Before merging, ensure:

• Run bundle exec rubocop - MANDATORY per CLAUDE.md
• Verify rake run_rspec:shakapacker_examples_latest works locally
• Verify rake run_rspec:shakapacker_examples_minimum works locally
• Test that minimum examples actually use React 18/Shakapacker 8.2.0 (check generated package.json)
• Verify CI runs correctly (don't rely on CI to catch issues - test locally first per CLAUDE.md)

---

🛡️ Security & Performance

• ✅ No security concerns
• ✅ Performance impact is minimal (one-time JSON operations during example generation)

---

📋 Before Merging Checklist

Per CLAUDE.md requirements:

1. CRITICAL: Run bundle exec rubocop and fix ALL violations
2. Ensure all files end with newline character
3. Test locally - don't wait for CI:

   rake run_rspec:shakapacker_examples_latest
   rake run_rspec:shakapacker_examples_minimum

4. Verify generated example apps have correct versions in their package.json
5. Update CHANGELOG.md if this is user-visible (seems internal, so probably skip)

---

🎯 Overall Assessment

This is a solid architectural improvement with good separation of concerns. The main concerns are:

1. Verify the minimum_versions accessor is present
2. Add error handling to JSON parsing
3. Test locally before merging (per CLAUDE.md critical requirements)

Nice work! Let me know if you'd like help addressing any of these points.

---

Review generated by Claude Code

[claude]

Code Review - PR #2125

Overall 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

1. Clear architectural improvement: Moving version compatibility testing to generator smoke tests is the right approach - it tests what users actually run.

2. Good constant management: MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION are centralized in ExampleType class, making version updates easier.

3. Simplified CI logic: The separation between run_rspec:shakapacker_examples_latest and run_rspec:shakapacker_examples_minimum is clean and easy to understand.

4. Proper error handling: The apply_minimum_versions method includes JSON parsing error handling with proper re-raising.

5. Good documentation: Comments in script/convert clearly explain the new scope of the script.

---

🔍 Issues & Concerns

1. Potential Race Condition in Rake Task (CRITICAL)

Location: react_on_rails/rakelib/run_rspec.rake:94-100

def latest_examples
  ExampleType.all[:shakapacker_examples].reject(&:minimum_versions)
end

def minimum_examples
  ExampleType.all[:shakapacker_examples].select(&:minimum_versions)
end

Problem: These helper methods are defined at the top level of the run_rspec namespace and called during task definition time (line 103, 108). If ExampleType.all isn't fully populated when these tasks are defined, you'll get empty task dependencies.

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 }
end

Or ensure examples_config.yml is loaded before task definition.

---

2. Silent Failure Risk in apply_minimum_versions (MEDIUM)

Location: react_on_rails/rakelib/shakapacker_examples.rake:32-42

if package_json["dependencies"]
  package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
  package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION
end

Problems:

• If dependencies is nil, react/react-dom versions won't be updated (silent failure)
• If shakapacker is missing from both dependencies and devDependencies, it's silently skipped
• No validation that the updates actually occurred

Recommendation:

unless package_json["dependencies"]
  raise "No dependencies found in package.json at #{dir}"
end

package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION

shakapacker_updated = false
if package_json["devDependencies"]&.key?("shakapacker")
  package_json["devDependencies"]["shakapacker"] = ExampleType::MINIMUM_SHAKAPACKER_VERSION
  shakapacker_updated = true
elsif package_json["dependencies"]&.key?("shakapacker")
  package_json["dependencies"]["shakapacker"] = ExampleType::MINIMUM_SHAKAPACKER_VERSION
  shakapacker_updated = true
end

raise "Shakapacker not found in package.json at #{dir}" unless shakapacker_updated

---

3. Missing Test Coverage (MEDIUM)

Problem: No tests verify:

• The new rake tasks (shakapacker_examples_latest, shakapacker_examples_minimum) exist and work correctly
• apply_minimum_versions correctly updates package.json
• The helper methods filter examples correctly

Recommendation: Add tests in a new spec/rake/shakapacker_examples_rake_spec.rb:

RSpec.describe "shakapacker_examples rake tasks" do
  describe "run_rspec:shakapacker_examples_latest" do
    it "only includes non-minimum examples" do
      # Test implementation
    end
  end

  describe "run_rspec:shakapacker_examples_minimum" do
    it "only includes minimum version examples" do
      # Test implementation
    end
  end
end

---

4. Hardcoded Version Check (LOW)

Location: script/convert:40

Comment mentions "Skip RSC tests on React 18 since RSC requires React 19" but there's no actual version check - it just assumes React 18 when running minimum CI.

Recommendation: If this assumption could break (e.g., if minimum React version is updated to 19), add a version check:

# Only skip RSC tests if React version is < 19
react_version = package_json.dig("dependencies", "react")
if react_version && Gem::Version.new(react_version.gsub(/[^\d.]/, "")) < Gem::Version.new("19.0.0")
  # Skip RSC tests
end

---

🎯 Best Practices Followed

• ✅ Follows project structure guidelines (no top-level analysis docs created)
• ✅ Good separation of concerns
• ✅ YAML configuration for data-driven example generation
• ✅ Clear naming conventions (-minimum suffix)
• ✅ Updated CI workflows to match new rake task structure

---

📋 Pre-Merge Checklist

Based on CLAUDE.md requirements:

• Run bundle exec rubocop - MANDATORY before commit
• Verify all files end with newline - MANDATORY
• Test locally: Run rake run_rspec:shakapacker_examples_latest and rake run_rspec:shakapacker_examples_minimum
• Verify task list: Run rake -T | grep shakapacker to confirm tasks are created
• Test minimum version generation: Manually verify that basic-minimum example has correct package.json versions
• CI verification: Ensure both Latest and Minimum CI jobs run the correct rake tasks

---

🚀 Performance Considerations

No significant performance impact. The filtering operations are O(n) on a small set of examples (currently 6 total).

---

🔒 Security Concerns

None identified. The apply_minimum_versions method:

• Only modifies generated example apps (not user code)
• Uses proper JSON parsing with error handling
• No injection risks

---

💡 Suggestions for Future Work

1. Consider adding a validation step that verifies generated minimum examples actually have the expected versions
2. Could extract version update logic into a reusable helper if this pattern is needed elsewhere
3. Consider adding a CI check that fails if minimum versions are updated without updating both MINIMUM_* constants

---

Verdict

Approve with recommendations. The critical issue (#1) should be addressed to ensure task dependencies work correctly. Issues #2 and #3 are nice-to-haves that would improve robustness and test coverage.

The architectural direction is excellent - this is a cleaner approach than the previous version conversion script.

[claude]

Code Review - PR #2125

Summary

This 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.

---

✅ Strengths

1. Clear Separation of Concerns

The PR correctly separates:

• spec/dummy: Tests latest versions (React 19, Shakapacker 9.4.0)
• Generator examples: Test compatibility matrix via new -minimum examples

This makes the testing strategy much clearer and easier to maintain.

2. Good Architectural Design

• Adding minimum_versions flag to ExampleType class is clean and extensible
• Constants for minimum versions (MINIMUM_REACT_VERSION, MINIMUM_SHAKAPACKER_VERSION) centralize version management
• Helper methods latest_examples and minimum_examples provide clean separation in rake tasks

3. Simplified CI Logic

• Removing React/Shakapacker conversion logic from script/convert simplifies the CI pipeline
• The script/convert now focuses solely on Node.js/tooling compatibility
• CI workflows correctly route to shakapacker_examples_latest vs shakapacker_examples_minimum

4. Proper Version Updates

• Root package.json and spec/dummy/package.json both updated to React 19
• Lockfiles updated appropriately

---

🔍 Observations & Questions

1. Task Invocation Pattern (Minor)

In run_rspec.rake:90:

task shakapacker_examples: "shakapacker_examples:gen_all" do
  ExampleType.all[:shakapacker_examples].each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
end

This runs ALL examples (both latest and minimum). Is this intentional? The new tasks shakapacker_examples_latest and shakapacker_examples_minimum filter correctly, but the original shakapacker_examples task still runs everything.

Recommendation: Consider whether shakapacker_examples should:

• Run all examples (current behavior)
• Only run latest examples (delegate to shakapacker_examples_latest)
• Be deprecated in favor of the specific tasks

2. Dependency Generation Order

In shakapacker_examples.rake:78-82:

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 apply_minimum_versions runs AFTER the generator but BEFORE npm install. This is correct - it modifies package.json before installing dependencies. ✅

3. Error Handling in JSON Parsing

The apply_minimum_versions method includes proper error handling:

rescue JSON::ParserError => e
  puts "  ERROR: Failed to parse package.json in #{dir}: #{e.message}"
  raise
end

Good defensive programming! ✅

4. Workflow Step Names

The workflow step names were updated from:

• "run conversion script to support shakapacker v6" (misleading - we're on Shakapacker 8+)

To:

• "Run conversion script for older Node compatibility" (accurate)

This is a good clarification! ✅

---

🔒 Security & Performance

• No security concerns - changes are purely CI/testing infrastructure
• Performance impact: Adding two new example apps will increase generator test time, but this is expected and acceptable for comprehensive compatibility testing
• No breaking changes for users

---

📋 Test Coverage

Based on the PR description, the following are marked as complete:

• ✅ Rake tasks created correctly
• ✅ run_rspec:shakapacker_examples_latest filters correctly
• ✅ run_rspec:shakapacker_examples_minimum filters correctly
• ✅ RuboCop passes
• ⏳ CI verification pending

Recommendation: Once CI passes, verify that:

1. The basic-minimum example actually installs React 18.0.0 and Shakapacker 8.2.0
2. The basic-server-rendering-minimum example works with those minimum versions
3. Check the CI logs to confirm the correct rake tasks are being invoked per matrix

---

🎯 Overall Assessment

LGTM with minor clarification needed on the shakapacker_examples task behavior.

This is a well-structured PR that:

• ✅ Follows the project's architecture patterns
• ✅ Maintains backward compatibility
• ✅ Improves code clarity and maintainability
• ✅ Properly updates documentation (CI step names)
• ✅ Includes proper error handling

Recommendation

Once CI passes and the question about shakapacker_examples task is clarified, this PR is ready to merge.

---

🤖 Generated with Claude Code

[claude]

Code Review - PR #2125

✅ Overall Assessment

This 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.

---

Strengths

1. Excellent Separation of Concerns ✨

• Latest testing: spec/dummy now exclusively uses React 19 and Shakapacker 9.4.0
• Compatibility testing: New minimum version examples (basic-minimum, basic-server-rendering-minimum) test React 18.0.0 and Shakapacker 8.2.0
• This mirrors real-world usage patterns better than the previous script/convert approach

2. Clean Implementation

• example_type.rb: Adding minimum_versions boolean attribute is simple and clear
• examples_config.yml: Declarative configuration makes it easy to add more compatibility tests in the future
• run_rspec.rake: Helper methods (latest_examples, minimum_examples) are concise and follow Ruby best practices
• shakapacker_examples.rake: The apply_minimum_versions method is well-structured with proper error handling

3. Improved CI Organization

• Workflows now explicitly run appropriate examples per dependency level
• run_rspec:shakapacker_examples_latest vs run_rspec:shakapacker_examples_minimum - crystal clear intent
• Reduces complexity by removing need for version conversions in script/convert

4. Good Documentation

• Updated comments in script/convert explain the new responsibility (Node.js tooling only)
• Workflow step names updated to reflect actual purpose

---

Issues & Recommendations

⚠️ CRITICAL: Missing Implementation in example_type.rb

Location: react_on_rails/rakelib/example_type.rb:16-18

The PR diff shows you're adding minimum_versions parameter and constants to ExampleType, but the current file in the repo does NOT have these changes yet:

# Current state (from Read tool):
attr_reader :packer_type, :name, :generator_options

def initialize(packer_type: nil, name: nil, generator_options: nil)
  @packer_type = packer_type
  @name = name
  @generator_options = generator_options
  self.class.all[packer_type.to_sym] << self
end

Expected state (from PR diff):

# Minimum supported versions for compatibility testing
MINIMUM_REACT_VERSION = "18.0.0"
MINIMUM_SHAKAPACKER_VERSION = "8.2.0"

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

Impact: Without this change:

• ❌ The new rake tasks will fail with NoMethodError: undefined method 'minimum_versions'
• ❌ apply_minimum_versions won't be able to access the constants

Action Required: Verify this file is included in your PR commit. It appears the diff shows it but the repo state doesn't reflect it yet.

---

🔧 Minor Issues

1. Error Handling in apply_minimum_versions Could Be More Robust

Location: shakapacker_examples.rake:18-48

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)  # Silent return if missing
  
  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  # Good! Re-raises the error
  end

Issue: If package.json doesn't exist, the method silently returns. This could hide configuration issues.

Recommendation:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  unless File.exist?(package_json_path)
    puts "  WARNING: package.json not found in #{dir}, skipping version updates"
    return
  end
  # ... rest of method
end

This makes the silent skip explicit and helps with debugging.

2. Potential Edge Case: What If Neither dependencies nor devDependencies Has Shakapacker?

Location: shakapacker_examples.rake:37-40

# Update Shakapacker to minimum supported version
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

Issue: If Shakapacker is missing from both, the method silently skips it. For a minimum version test, this might indicate a problem.

Recommendation: Add a warning:

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
else
  puts "  WARNING: shakapacker not found in dependencies or devDependencies"
end

3. Testing: Verify Rake Tasks Are Created Correctly

Your test plan has checkboxes for:

• Verify rake tasks are created correctly for new example types
• Verify run_rspec:shakapacker_examples_latest includes only non-minimum examples
• Verify run_rspec:shakapacker_examples_minimum includes only minimum examples

Recommendation: Before merging, manually verify:

cd react_on_rails
bundle exec rake -T | grep shakapacker_examples

# Expected output should include:
# rake run_rspec:shakapacker_examples_latest
# rake run_rspec:shakapacker_examples_minimum
# rake shakapacker_examples:gen_basic-minimum
# rake shakapacker_examples:gen_basic-server-rendering-minimum

---

💡 Enhancement Opportunities (Future Work)

1. Document Minimum Versions in CLAUDE.md

The minimum versions are now defined as constants but aren't documented in the main docs. Consider adding:

## Supported Versions

React on Rails maintains compatibility testing for:
- **React**: 18.0.0+ (minimum), 19.x (latest)
- **Shakapacker**: 8.2.0+ (minimum), 9.4.0+ (latest)
- **Ruby**: 3.2+ (minimum), 3.4+ (latest)
- **Node**: 20+ (minimum), 22+ (latest)

See `react_on_rails/rakelib/example_type.rb` for the source of truth.

2. Consider Adding More Compatibility Matrix Examples

Now that you have the infrastructure, you could easily add:

• redux-minimum (tests Redux with React 18)
• server-rendering-minimum-shakapacker-8 (specific Shakapacker edge cases)

The declarative config makes this trivial.

3. RuboCop: Disable Metrics/MethodLength for apply_minimum_versions?

The method is 31 lines but very linear. If RuboCop complains about method length, consider:

def apply_minimum_versions(dir) # rubocop:disable Metrics/MethodLength
  # ... method body
end

Or break into smaller methods:

def apply_minimum_versions(dir)
  package_json = load_package_json(dir)
  return unless package_json
  
  update_react_versions(package_json)
  update_shakapacker_version(package_json)
  save_package_json(dir, package_json)
end

---

Security Considerations

✅ No security concerns identified:

• No user input is directly executed
• All file operations use safe Ruby methods
• JSON parsing has proper error handling
• Regex replacements are bounded and safe

---

Performance Considerations

✅ No significant performance impact:

• File operations are limited to generator tasks (not runtime)
• JSON parsing is only done once per example generation
• CI jobs are now more focused (run fewer unnecessary tests)

Potential improvement: The new approach actually improves CI performance by avoiding unnecessary version conversions during script/convert.

---

Test Coverage

✅ CI shows all tests passing, but consider:

1. Manual verification needed (per CLAUDE.md critical requirements):

   • Run bundle exec rubocop locally
   • Test generator with minimum versions:

     cd gen-examples/basic-minimum
     cat package.json | grep -E '"(react|shakapacker)"'
     # Should show React 18.0.0 and Shakapacker 8.2.0

2. Integration test: Verify the minimum examples actually work:

   bundle exec rake run_rspec:shakapacker_examples_minimum

---

Changelog Compliance

⚠️ This PR should NOT update CHANGELOG.md (per CLAUDE.md guidelines):

Do NOT add entries for: linting, formatting, refactoring, tests, or documentation fixes

This is an internal test infrastructure improvement, not a user-visible change. ✅ Correctly excluded.

---

Final Verdict

✅ APPROVE with Minor Recommendations

Strengths:

• Well-architected refactoring
• Improves test clarity and maintainability
• All CI checks passing
• Follows project conventions

Before Merging:

1. ✅ Verify example_type.rb changes are committed (see Critical issue above)
2. ⚠️ Consider adding warnings for edge cases in apply_minimum_versions
3. 📋 Manually verify rake tasks exist: rake -T | grep minimum
4. 🧪 Run bundle exec rubocop locally (per CLAUDE.md requirement)

Optional Future Work:

• Document minimum versions in CLAUDE.md
• Add more granular compatibility examples as needed

Great work on improving the test architecture! 🎉

---

Review completed using Claude Code 🤖

[claude]

Code Review for PR #2125

Summary

This 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

1. Clear separation of concerns: Latest versions tested in spec/dummy, minimum versions tested via generator examples
2. Good code organization: New minimum_versions flag cleanly separates example types
3. Smart implementation: The apply_minimum_versions function modifies package.json after generation, avoiding complex generator changes
4. CI improvements: Separate rake tasks (shakapacker_examples_latest vs shakapacker_examples_minimum) make CI intent explicit
5. Simplified script/convert: Removing React/Shakapacker version logic from this script improves maintainability

🔍 Issues Found

1. CRITICAL: Missing Implementation in example_type.rb

The PR diff shows these additions to example_type.rb:

# Minimum supported versions for compatibility testing
MINIMUM_REACT_VERSION = "18.0.0"
MINIMUM_SHAKAPACKER_VERSION = "8.2.0"

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  # NEW LINE
  self.class.all[packer_type.to_sym] << self
end

However, the current file at react_on_rails/rakelib/example_type.rb:16-23 does NOT include:

• The minimum_versions attribute reader
• The minimum_versions parameter in initialize
• The @minimum_versions instance variable assignment

This will cause failures when the rake tasks call .minimum_versions on ExampleType objects.

2. Missing Rake Task Implementations in run_rspec.rake

The PR diff shows additions of:

# 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

desc "Runs Rspec for latest version example apps only (excludes minimum version tests)"
task shakapacker_examples_latest: latest_examples.map(&:gen_task_name) do
  latest_examples.each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
end

desc "Runs Rspec for minimum version example apps only (React 18, Shakapacker 8.2.0)"
task shakapacker_examples_minimum: minimum_examples.map(&:gen_task_name) do
  minimum_examples.each { |example_type| Rake::Task[example_type.rspec_task_name].invoke }
end

However, the current file at react_on_rails/rakelib/run_rspec.rake does NOT contain these additions after line 92.

3. CI Workflow Not Updated

The .github/workflows/examples.yml file still shows:

- name: Main CI
  run: cd react_on_rails && bundle exec rake run_rspec:shakapacker_examples

But the PR diff shows this should be split into:

- 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_minimum

4. script/convert Still Modifies React Versions

Lines 45-49 in script/convert still modify React versions in package.json files:

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",')

According to the PR description, these should be removed since React version compatibility is now handled by generator examples. The diff shows these lines should be removed.

🐛 Potential Bugs

5. Error Handling in apply_minimum_versions

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  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  # Re-raises the exception, failing the rake task
  end

Issue: The raise will cause the entire rake task to fail. Consider if this is the desired behavior or if it should be more graceful.

Recommendation: This is actually good defensive programming - if package.json is malformed, the build should fail. Keep this.

6. Potential Race Condition with npm install

In shakapacker_examples.rake, after calling apply_minimum_versions, the code runs:

apply_minimum_versions(example_type.dir) if example_type.minimum_versions
sh_in_dir(example_type.dir, "npm install")

Issue: There's no package manager lock file regeneration between modifying package.json and running npm install. This could potentially use stale lock files.

Recommendation: Add a comment explaining this is intentional, or delete/regenerate lock files:

if example_type.minimum_versions
  apply_minimum_versions(example_type.dir)
  # Delete lock file to force regeneration with new versions
  lock_file = File.join(example_type.dir, "yarn.lock")
  File.delete(lock_file) if File.exist?(lock_file)
end

📋 Minor Issues

7. Inconsistent Package Manager References

The comment says "npm install" but based on CLAUDE.md, the project uses pnpm:

sh_in_dir(example_type.dir, "npm install")  # Should this be pnpm?

Check if generator examples use npm or pnpm.

8. Magic Numbers in Constants

The minimum versions are hardcoded:

MINIMUM_REACT_VERSION = "18.0.0"
MINIMUM_SHAKAPACKER_VERSION = "8.2.0"

Recommendation: Add a comment explaining why these are the minimum versions (e.g., "React 18: First version with automatic JSX transform", "Shakapacker 8.2.0: First version with async script loading support").

9. Unused move Method in script/convert

Lines 20-24 define a move method that is never used. Should be removed.

🧪 Testing Recommendations

Before merging:

1. Test the new rake tasks locally:

   # Should only run non-minimum examples
   bundle exec rake run_rspec:shakapacker_examples_latest
   
   # Should only run minimum examples  
   bundle exec rake run_rspec:shakapacker_examples_minimum

2. Verify minimum version examples are generated correctly:

   bundle exec rake shakapacker_examples:gen_basic-minimum
   cat gen-examples/basic-minimum/package.json | grep '"react"'
   # Should show: "react": "18.0.0"

3. Test CI workflow changes:

   • Push to PR and verify CI uses correct rake task for each dependency level
   • Check that latest CI runs shakapacker_examples_latest
   • Check that minimum CI runs shakapacker_examples_minimum

🎯 Action Items

Must fix before merge:

1. ✅ Ensure example_type.rb includes minimum_versions attribute (appears missing from current file)
2. ✅ Ensure run_rspec.rake includes the new helper methods and tasks (appears missing)
3. ✅ Verify .github/workflows/examples.yml uses conditional rake tasks
4. ✅ Remove React version modifications from script/convert (lines 45-49)
5. ✅ Remove unused move method from script/convert

Nice to have:
6. Add documentation comments for minimum version constants
7. Consider lock file handling in apply_minimum_versions
8. Verify correct package manager (npm vs pnpm) for example generation

📝 CLAUDE.md Compliance

• ✅ Follows project structure guidelines (using /analysis for docs would be good for future investigation docs)
• ⚠️ CRITICAL: Must run bundle exec rubocop before committing
• ⚠️ CRITICAL: Ensure all files end with newline character
• ✅ Changes are user-visible, so CHANGELOG.md update is appropriate (already in test plan)
• ✅ Uses appropriate testing strategy

---

Overall Assessment: Strong architectural approach, but appears to have incomplete file changes. The diff shows additions that don't appear in the current files. Please verify all changes from the diff are actually present in the branch before merging.

🤖 Generated with Claude Code

[justin808]

Review Comments Addressed

Thank you for the thorough reviews! All the issues mentioned have been addressed in the commits on this branch:

✅ All Critical Issues Resolved

1. attr_reader :minimum_versions - Already present in example_type.rb:20

2. minimum_versions? predicate method - Already present in example_type.rb:23-25

3. JSON parse error handling in apply_minimum_versions - Added in commit e5b89d3:

   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
   end

4. Rake tasks shakapacker_examples_latest and shakapacker_examples_minimum - Already present in run_rspec.rake:102-110

5. Helper methods latest_examples and minimum_examples - Already present in run_rspec.rake:94-100

6. examples_config.yml entries - Already includes basic-minimum and basic-server-rendering-minimum with minimum_versions: true

7. Trailing newlines - All files end with proper newlines

Files Ready for Review

• react_on_rails/rakelib/example_type.rb - Contains constants, attr_reader, and predicate method
• react_on_rails/rakelib/run_rspec.rake - Contains new tasks and helper methods
• react_on_rails/rakelib/shakapacker_examples.rake - Contains apply_minimum_versions with error handling
• react_on_rails/rakelib/examples_config.yml - Contains minimum version example entries

All RuboCop violations have been fixed and the code follows project conventions.

[claude]

Code Review: Move React/Shakapacker version compatibility to generator smoke tests

Overall 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

• Clear separation of concerns: spec/dummy now focuses on latest versions, while generator examples test the compatibility matrix
• Good use of Ruby conventions: The minimum_versions? predicate method follows Ruby naming conventions properly (line 23 in example_type.rb)
• Constants for version numbers: MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION are well-placed and easily maintainable

✅ Helper Methods

def latest_examples
  ExampleType.all[:shakapacker_examples].reject(&:minimum_versions?)
end

def minimum_examples
  ExampleType.all[:shakapacker_examples].select(&:minimum_versions?)
end

These filtering methods (lines 94-100 in run_rspec.rake) are clean and readable.

💡 Minor Suggestion - Method Location

The helper methods latest_examples and minimum_examples are defined in the run_rspec.rake namespace at lines 94-100, but they're only used within that namespace. Consider either:

1. Making them private if they're not needed elsewhere
2. Or documenting that they're intentionally kept public for potential future use

---

Potential Issues

⚠️ JSON Parsing Error Handling

In shakapacker_examples.rake lines 24-29:

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
end

Concern: The error handling is good, but consider if raise is the right approach. This will halt the entire rake task if any example has a malformed package.json.

Suggestion: Consider whether you want to:

• Continue with a warning (for more resilient test runs)
• Or fail fast (current behavior - probably correct for CI)

If fail-fast is intentional (which seems right for CI), this is fine as-is.

🔍 Edge Case: Missing Dependencies

In apply_minimum_versions (lines 32-42), the code checks if React exists in dependencies:

if package_json["dependencies"]
  package_json["dependencies"]["react"] = ExampleType::MINIMUM_REACT_VERSION
  package_json["dependencies"]["react-dom"] = ExampleType::MINIMUM_REACT_VERSION
end

Question: What happens if the generated app doesn't have React in dependencies at all? Should this be a hard error, or should it create the dependencies section?

Current behavior: Silently skips if dependencies section is missing - this might mask configuration issues.

---

Performance Considerations

✅ Efficient Filtering

The use of reject(&:minimum_versions?) and select(&:minimum_versions?) is efficient for small arrays (typical number of examples).

✅ Lazy Evaluation

The rake task dependencies ensure examples are generated only when needed, which is good for incremental testing.

---

Security Concerns

✅ No Security Issues Identified

• File operations use safe paths
• No user input is directly interpolated into shell commands
• JSON parsing has error handling

---

Test Coverage

✅ Good Manual Test Plan

The PR description shows good testing discipline:

• Verify rake tasks are created correctly for new example types
• Verify run_rspec:shakapacker_examples_latest includes only non-minimum examples
• Verify run_rspec:shakapacker_examples_minimum includes only minimum examples
• RuboCop passes
• CI runs generator tests with correct examples per dependency level

💡 Suggestion: Add Unit Tests

Consider adding unit tests for:

1. The apply_minimum_versions method behavior
2. The filtering logic (latest_examples / minimum_examples)
3. Edge cases (missing dependencies, malformed package.json)

These could live in spec/react_on_rails/rakelib/ if that pattern exists.

---

CI/CD Considerations

✅ Workflow Changes Look Good

The changes to .github/workflows/examples.yml properly split the tasks:

- 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_minimum

✅ Clear Comment Updates

Changed from "run conversion script to support shakapacker v6" to "Run conversion script for older Node compatibility" - this accurately reflects the new purpose.

---

script/convert Simplification

✅ Excellent Cleanup

The removal of React/Shakapacker version manipulation from script/convert is a major improvement:

• Before: 34 deletions of complex version switching logic
• After: 10 additions of clear documentation

The new comment block at the top clearly explains the script's purpose.

⚠️ One Concern: Gemfile Modification Removed

I notice this line was removed from script/convert:

gsub_file_content("../react_on_rails/Gemfile.development_dependencies", /gem "shakapacker", "[^"]*"/, 'gem "shakapacker", "8.2.0"')

Question: How does the minimum CI now get Shakapacker 8.2.0 in the dummy app? Is it:

1. Only through generated examples (which get it via apply_minimum_versions)?
2. Or does spec/dummy need separate handling?

Clarification needed: If spec/dummy integration tests now always run on latest versions (per your PR description), this is fine. But ensure spec/dummy integration tests aren't being run in minimum CI, or they'll test the wrong versions.

---

Documentation

✅ PR Description is Excellent

Clear explanation of:

• What changed and why
• Benefits of the approach
• Test plan

💡 Consider Adding Comments

Consider adding a comment in examples_config.yml explaining the minimum_versions flag:

# Minimum version compatibility tests - tests React 18 and Shakapacker 8.2.0
# When minimum_versions: true, apply_minimum_versions() will downgrade package.json
- name: basic-minimum
  generator_options: ''
  minimum_versions: true

This helps future maintainers understand the mechanism.

---

Suggestions Summary

1. Clarify: Ensure spec/dummy doesn't run in minimum CI (or document if it should)
2. Consider: Adding unit tests for the new apply_minimum_versions logic
3. Optional: Make helper methods private or document their public nature
4. Optional: Add inline comments in examples_config.yml explaining minimum_versions flag

---

Conclusion

Recommendation: ✅ APPROVE with minor clarifications

This is solid refactoring work that improves code organization and maintainability. The separation of concerns is clear, the implementation is clean, and the Ruby code follows best practices.

The only outstanding question is ensuring spec/dummy integration tests are properly handled in the minimum CI workflow. Once that's confirmed/documented, this is ready to merge.

Great work! 🎉

---

Review conducted following CLAUDE.md guidelines and React on Rails best practices.

[coderabbitai]

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/react and @types/react-dom to 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.json and align with the correct versions already used in spec/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/Gemfile to /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 reject and select with the minimum_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

📥 Commits

Reviewing files that changed from the base of the PR and between b50a74d and e8442f8.

⛔ Files ignored due to path filters (2)

• react_on_rails/spec/dummy/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
• yarn.lock is 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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• react_on_rails/rakelib/example_type.rb
• react_on_rails/rakelib/run_rspec.rake
• script/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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml

package.json

📄 CodeRabbit inference engine (CLAUDE.md)

Never run npm commands - 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 rubocop and 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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• script/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.yml
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• react_on_rails/rakelib/example_type.rb
• react_on_rails/rakelib/run_rspec.rake
• script/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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/example_type.rb
• script/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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• react_on_rails/rakelib/example_type.rb
• script/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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• script/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.json
• react_on_rails/spec/dummy/package.json
• react_on_rails/rakelib/examples_config.yml
• react_on_rails/rakelib/run_rspec.rake
• script/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.json
• react_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.yml
• react_on_rails/rakelib/examples_config.yml
• .github/workflows/examples.yml
• script/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.json
• react_on_rails/rakelib/shakapacker_examples.rake
• script/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.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• react_on_rails/rakelib/example_type.rb
• 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:

• react_on_rails/rakelib/examples_config.yml
• script/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.rake
• script/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.rake
• script/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: false maintains backward compatibility

script/convert (1)

40-51: LGTM!

Appropriately skips RSC-related tests when running on React 18, as RSC requires React 19. The exit 0 no-op for test:rsc is 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 new apply_minimum_versions method that parses and writes package.json files.

---

82-83: LGTM!

The conditional invocation of apply_minimum_versions correctly applies minimum version constraints only to examples marked with minimum_versions: true.

---

19-48: All dependencies verified and confirmed in place.

The verification confirms:

• minimum_versions? predicate is properly defined in ExampleType (line 23 in example_type.rb)
• Constants MINIMUM_REACT_VERSION ("18.0.0") and MINIMUM_SHAKAPACKER_VERSION ("8.2.0") are defined (lines 17-18 in example_type.rb)
• The apply_minimum_versions method correctly uses these constants to update package.json files

The 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-level values 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: true flag 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.

[claude]

Code Review

This 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

1. Clear separation of concerns: Moving version compatibility to generator tests makes the test architecture more intuitive - spec/dummy tests the latest versions, generators test the compatibility matrix.

2. Good error handling: The apply_minimum_versions function properly handles JSON parsing errors with rescue blocks and informative error messages.

3. Proper Ruby conventions: The minimum_versions? predicate method follows Ruby naming conventions for boolean checks.

4. Well-documented: The script/convert file has excellent inline documentation explaining its narrowed scope.

5. Clean CI integration: The workflow changes are minimal and clear - conditionally running different rake tasks based on dependency level.

🔍 Potential Issues

1. Critical: Missing RuboCop validation

Per CLAUDE.md requirements, you MUST run bundle exec rubocop before committing. The diff shows:

def apply_minimum_versions(dir) # rubocop:disable Metrics/CyclomaticComplexity

This suggests the method may have complexity issues. Consider:

• Breaking down the method into smaller helper functions
• Or, if the disable is justified, document WHY the complexity is necessary

2. Error handling incompleteness

In apply_minimum_versions:

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
end

3. Missing test coverage validation

Per CLAUDE.md's "CRITICAL - LOCAL TESTING REQUIREMENTS", before claiming tests are fixed:

• Have you run bundle exec rake run_rspec:shakapacker_examples_latest locally?
• Have you run bundle exec rake run_rspec:shakapacker_examples_minimum locally?
• Have the minimum version examples been generated and tested successfully?

The test plan shows checkboxes for rake task verification but not for actual test execution.

4. Potential race condition in CI

In .github/workflows/examples.yml:

- 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_minimum

Both 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 Improvement

1. Add version verification

Consider 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
end

Call this after apply_minimum_versions and npm install to ensure versions are actually what we expect.

2. Document the script/convert changes

The 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 visibility

The constants MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION are defined in ExampleType but used in a rake task. Consider:

• Moving them to a shared configuration file
• Or documenting that they're the single source of truth for minimum versions

⚠️ Action Items Before Merge

1. MANDATORY: Run bundle exec rubocop and fix all violations
2. MANDATORY: Ensure trailing newlines in all files
3. Test locally with both rake tasks:

   bundle exec rake run_rspec:shakapacker_examples_latest
   bundle exec rake run_rspec:shakapacker_examples_minimum

4. Verify that minimum version examples actually install React 18.0.0 and Shakapacker 8.2.0
5. Consider adding the version verification helper mentioned above

📊 Test Coverage

The PR test plan should verify:

• ✅ RuboCop passes (per CLAUDE.md requirement)
• ✅ Latest examples run with React 19 and Shakapacker 9.4.0
• ✅ Minimum examples run with React 18.0.0 and Shakapacker 8.2.0
• ✅ Generated package.json files have correct versions after apply_minimum_versions
• ✅ CI runs correct example sets based on dependency-level matrix

🎯 Summary

This is a solid architectural improvement. The main concerns are:

1. Ensuring RuboCop compliance (MANDATORY per CLAUDE.md)
2. Adding error handling for file write operations
3. Verifying local testing was done for both rake tasks
4. Optional but recommended: add version verification after generation

Great work on simplifying the CI configuration and making version compatibility testing more explicit! 🚀

[justin808]

Addressing Review Comment #3573928031

Thank 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 Addressed

1. Clarification: spec/dummy in minimum CI

Good 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 (run_rspec:shakapacker_examples_minimum), not the spec/dummy integration tests. This is intentional - we test the latest user experience in spec/dummy, while generator examples test minimum version compatibility.

2. JSON Parsing Error Handling

The apply_minimum_versions method already has proper error handling (added in commit e5b89d3):

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
end

Fail-fast behavior is correct for CI - we want to know immediately if generated examples have malformed package.json.

3. Documentation Added

Added comment in examples_config.yml explaining the minimum_versions flag:

# Minimum version compatibility tests - tests React 18 and Shakapacker 8.2.0
- name: basic-minimum
  generator_options: ''
  minimum_versions: true

---

🚀 React 19 Compatibility Updates

I've also updated the spec/dummy app to be fully compatible with React 19, addressing peer dependency issues:

Dependencies Updated in package.json:

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:

1. 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

2. react-helmet migration: Updated to @dr.pogodin/react-helmet which is the React 19 compatible fork:

   • Uses HelmetProvider with context prop for SSR (thread-safe)
   • Updated ReactHelmetApp.server.jsx and ReactHelmetAppBroken.server.jsx with new SSR pattern
   • Updated ReactHelmet.jsx component

Verification:

• ✅ yarn install completes without peer dependency warnings (only unrelated babel-eslint warning)
• ✅ yarn run build:test completes 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.

[claude]

Code Review - PR #2125

Thank you for this well-structured refactoring! This PR successfully moves React/Shakapacker version compatibility testing from spec/dummy to generator smoke tests.

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: Using minimum_versions flag on ExampleType is clean and extensible
• Smart use of existing infrastructure: Leverages the example generator system rather than complex CI logic
• Clear naming convention: *-minimum suffix makes backwards compatibility testing obvious
• Simplified conversion script: Removing React/Shakapacker version logic from script/convert reduces complexity

Implementation Quality

• Well-documented constants: MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION centralize version requirements
• Ruby conventions followed: Using minimum_versions? predicate method (with ? suffix) is idiomatic
• Proper error handling: JSON parsing has rescue block with helpful error message
• Clean CI workflow changes: Conditional job execution based on dependency level is straightforward

---

🔍 Suggestions & Questions

1. RuboCop Disable - Is It Necessary?

def apply_minimum_versions(dir) # rubocop:disable Metrics/CyclomaticComplexity

This 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 Formatting

File.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 Enhancement

Consider handling Errno::ENOENT separately for clearer diagnostics:

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}"
  raise

4. CI Workflow Edge Case

What if matrix.dependency-level is neither 'latest' nor 'minimum'? Both CI steps would be skipped silently. Consider adding validation to fail fast on unexpected values.

5. Test Coverage

Are there tests verifying:

• The minimum_versions? predicate correctly filters examples?
• The new rake tasks shakapacker_examples_latest and shakapacker_examples_minimum work?
• apply_minimum_versions correctly updates package.json?

6. Documentation

The comment in examples_config.yml is good. Consider expanding slightly to explain what minimum_versions: true does (triggers apply_minimum_versions() during generation).

---

🧪 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

---

🏁 Verdict

Overall: ✅ 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

[claude]

Code Review - PR #2125

Overview

This 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.

---

✅ Strengths

1. Clear Separation of Concerns

• Dummy app now focuses exclusively on testing latest versions (React 19, Shakapacker 9.4.0)
• Generator smoke tests handle backward compatibility (React 18, Shakapacker 8.2.0)
• This separation makes the test strategy more maintainable and easier to understand

2. Excellent Code Quality

• Ruby predicate method naming follows conventions (minimum_versions?)
• Constants MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION are well-placed in ExampleType class
• JSON parsing includes proper error handling with rescue clause
• Clear comments explaining the version update logic

3. CI Integration

• Conditional task execution (shakapacker_examples_latest vs shakapacker_examples_minimum) is clean
• Workflow naming updates clarify purpose ("Run conversion script for older Node compatibility")
• Uses appropriate matrix strategy for testing dependency levels

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?)
end

These are readable and leverage Ruby's functional collection methods appropriately.

---

🔍 Areas for Consideration

1. Missing Cleanup in script/convert (Critical)

The script/convert file still contains the old React/Shakapacker version modification logic (lines 30-31, 45-49 in the current file). According to the PR description, this logic should now be removed since version compatibility is handled by generator smoke tests.

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, script/convert should only handle Node.js tooling compatibility, not React/Shakapacker versions. The diff shows these lines being removed in the PR, but I want to confirm this is intentional and complete.

2. Potential Complexity in apply_minimum_versions Method

The method has reasonable complexity (RuboCop disabled with Metrics/CyclomaticComplexity), but consider:

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
  # ...
end

Suggestions:

• Could extract a helper method update_dependency(package_json, dep_name, version) to reduce duplication
• Consider what happens if React isn't in dependencies (should it be added, or is this a validation error?)
• The method currently modifies both react and react-dom but doesn't check if they exist first

3. Test Coverage Gaps

The test plan in the PR description shows:

• Verify rake tasks are created correctly ✅
• Verify filtering works ✅
• RuboCop passes ✅
• CI runs generator tests with correct examples per dependency level ⚠️

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:

• A comment in examples_config.yml explaining the minimum_versions flag
• Update to project documentation explaining the new testing strategy
• Inline comment in run_rspec.rake explaining why we have separate tasks

---

🛡️ Security & Performance

Security: ✅ 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 Recommendations

Before merging, verify:

1. Run locally:

   bundle exec rake run_rspec:shakapacker_examples_latest
   bundle exec rake run_rspec:shakapacker_examples_minimum

2. Check generated package.json files:

   cat gen-examples/basic-minimum/package.json | grep -E '(react|shakapacker)'
   cat gen-examples/basic/package.json | grep -E '(react|shakapacker)'

3. Verify script/convert no longer modifies React/Shakapacker versions (if that was the intent)

---

📋 Minor Suggestions

1. File: react_on_rails/rakelib/run_rspec.rake

   • The helper methods latest_examples and minimum_examples are defined at the module level but only used within the run_rspec namespace. Consider moving them inside the namespace or making them private.

2. File: react_on_rails/rakelib/example_type.rb

   • The minimum_versions attribute is added to initialize but not to the attr_reader line. Consider adding it for consistency:

     attr_reader :packer_type, :name, :generator_options, :minimum_versions

3. File: react_on_rails/rakelib/shakapacker_examples.rake

   • The success message could be more informative:

     puts "  Updated package.json with minimum versions (React: #{ExampleType::MINIMUM_REACT_VERSION}, Shakapacker: #{ExampleType::MINIMUM_SHAKAPACKER_VERSION})"

---

🎯 Recommendation

Approve with minor revisions

This is a solid architectural improvement that makes the codebase more maintainable. The only blocking concern is confirming that script/convert has been properly updated to remove the old React/Shakapacker version modification logic (based on the diff, it looks correct, but please verify).

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

[claude]

Code Review

Overall 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 💪

1. Clear separation of concerns: spec/dummy now consistently tests latest versions, while generator examples test the compatibility matrix
2. Well-documented changes: The script/convert file has excellent comments explaining the new scope
3. Clean architecture: The minimum_versions flag in ExampleType is properly implemented with a Ruby-idiomatic predicate method
4. Good naming: shakapacker_examples_latest and shakapacker_examples_minimum are clear and descriptive
5. CI workflow logic: Conditional workflow steps are correctly implemented

---

Code Quality Issues 🔍

1. Missing Implementation in example_type.rb ⚠️

Looking at the diff, I see that example_type.rb should have these changes:

+      # Minimum supported versions for compatibility testing
+      MINIMUM_REACT_VERSION = "18.0.0"
+      MINIMUM_SHAKAPACKER_VERSION = "8.2.0"
+
+      attr_reader :packer_type, :name, :generator_options, :minimum_versions
+
+      # Ruby convention: predicate method with ? suffix for boolean checks
+      def minimum_versions?
+        minimum_versions
+      end
+
+      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

However, when I read the actual file at react_on_rails/rakelib/example_type.rb:16-18, I only see:

attr_reader :packer_type, :name, :generator_options

def initialize(packer_type: nil, name: nil, generator_options: nil)

This suggests the changes may not have been applied yet, or there's a mismatch. Could you verify that all changes in the diff have been committed?

2. Missing apply_minimum_versions method in shakapacker_examples.rake ⚠️

The diff shows a new apply_minimum_versions method being added to shakapacker_examples.rake starting at line 18:

+  # Updates package.json to use minimum supported versions for compatibility testing
+  def apply_minimum_versions(dir)
+    # ... implementation
+  end

And it should be called after generator commands:

+      # Apply minimum versions for compatibility testing examples
+      apply_minimum_versions(example_type.dir) if example_type.minimum_versions

However, when I read the actual file, I don't see this method. Please verify this has been committed.

3. Missing JSON require

The diff adds:

+require "json"

Confirm this is present in shakapacker_examples.rake.

4. Potential Issue: Error Handling in apply_minimum_versions

From the diff, the apply_minimum_versions method includes:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)

  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
  end

Suggestion: The error message is good, but consider whether raise is the right behavior. Should this fail the entire rake task, or should it skip this example with a warning? Current behavior will halt all example generation if one package.json is malformed.

Recommendation: Document this behavior or add a comment explaining why we want to fail-fast on JSON parse errors.

5. RuboCop Disable Comment

The diff shows:

+  def apply_minimum_versions(dir) # rubocop:disable Metrics/CyclomaticComplexity

While the disable is necessary, consider if the method could be simplified. The method does multiple things:

• Reads package.json
• Modifies React versions
• Modifies Shakapacker version (checking both devDependencies and dependencies)
• Writes back

Suggestion: Consider extracting version update logic into smaller helper methods to reduce complexity:

def apply_minimum_versions(dir)
  package_json_path = File.join(dir, "package.json")
  return unless File.exist?(package_json_path)
  
  package_json = parse_package_json(package_json_path)
  update_react_versions(package_json)
  update_shakapacker_version(package_json)
  write_package_json(package_json_path, package_json)
  
  log_version_updates
end

This would eliminate the RuboCop disable and make the code more maintainable.

---

Testing Considerations 🧪

1. Missing Test Coverage

The PR adds significant new functionality but doesn't include tests for:

• apply_minimum_versions method behavior
• Correct filtering of examples by minimum_versions?
• Correct rake task generation for latest vs minimum

Recommendation: Add RSpec tests in spec/react_on_rails/rakelib/ to verify:

describe ReactOnRails::TaskHelpers::ExampleType do
  describe '#minimum_versions?' do
    it 'returns true when minimum_versions is set' do
      example = described_class.new(
        packer_type: 'test',
        name: 'test',
        generator_options: '',
        minimum_versions: true
      )
      expect(example.minimum_versions?).to be true
    end
  end
end

2. Manual Testing Checklist (from PR description)

The PR mentions:

• CI runs generator tests with correct examples per dependency level

This is still unchecked. Has this been verified manually? The CI is passing, but explicit verification would be good.

---

Security Considerations 🔒

No security concerns identified. The changes:

• Only modify test/example code
• Don't introduce any external dependencies
• Don't handle user input
• Don't modify production code paths

---

Performance Considerations ⚡

Positive impact:

• Separating latest vs minimum tests allows CI to run more efficiently
• No longer need to convert spec/dummy between versions
• Each CI job tests exactly what it needs to test

Potential concern:

• apply_minimum_versions reads and writes package.json during example generation
• This adds I/O overhead, but it's during test setup so impact is minimal
• Consider caching the parsed JSON if this method is called multiple times per example

---

Best Practices Alignment 📋

✅ Follows CLAUDE.md guidelines:

• Clear naming conventions
• Proper documentation in script/convert
• RuboCop compliance (though with some disables)
• Good separation of concerns

⚠️ Could improve:

• Add RSpec tests for new functionality
• Consider refactoring to avoid RuboCop disable
• Add more inline comments for complex logic in apply_minimum_versions

---

Recommendations Summary

Must Address:

1. ✅ Verify all changes from diff are actually committed (especially in example_type.rb and shakapacker_examples.rake)

Should Consider:
2. Add RSpec tests for new functionality
3. Refactor apply_minimum_versions to reduce complexity
4. Document the fail-fast behavior on JSON parse errors
5. Manually verify the unchecked test plan item

Nice to Have:
6. Add inline comments explaining the Shakapacker version check logic (why check both dependencies and devDependencies)
7. Consider extracting constants for package.json keys ("dependencies", "devDependencies", etc.)

---

Conclusion

This is a solid PR with a clear improvement to the testing strategy. The main concerns are:

1. Confirming all code changes are actually committed
2. Adding test coverage for the new functionality

Once these are addressed, this will be ready to merge. Great work on improving the test organization! 🎉

[claude]

Code Review - PR #2125: Move React/Shakapacker version compatibility to generator smoke tests

Overview

This PR successfully refactors version compatibility testing from spec/dummy to generator smoke tests. The approach is clean and well-architected.

---

Strengths

1. Clean Architecture

• Excellent separation of concerns: Latest versions in spec/dummy, minimum versions in generator examples
• Constants in the right place: MINIMUM_REACT_VERSION and MINIMUM_SHAKAPACKER_VERSION in ExampleType class
• Ruby idioms followed: Added minimum_versions? predicate method - proper Ruby convention

2. Smart Implementation

• apply_minimum_versions method is elegant - modifies package.json after generation
• JSON error handling added in patch 3/4 - good defensive programming
• Conditional CI execution properly routes latest vs minimum examples to correct workflows

3. Simplified Conversion Script

• Great cleanup of script/convert - removed React/Shakapacker version logic
• Clearer purpose now: only handles Node.js tooling compatibility

---

Issues and Recommendations

CRITICAL: Missing Test Coverage

Problem: 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 Testing

Issue: From examples.yml lines 175-179 - conditional execution based on dependency-level

Recommendation per CLAUDE.md:

1. Test locally BEFORE CI using bin/ci-switch-config
2. "NEVER claim a test is fixed without running it locally first"
3. Your PR description shows unchecked box for CI verification - You MUST verify locally before merging

---

MEDIUM: Potential Race Condition

Issue 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 Verification

From CLAUDE.md: ALWAYS run bundle exec rubocop before commit

Recommendation: Run locally before merge

---

Security and Performance

Security: No concerns

• JSON parsing has error handling
• File operations use proper paths
• No user input processed

Performance: Acceptable

• apply_minimum_versions runs once per example generation
• CI conditional execution properly optimizes PR vs master runs

---

Testing Checklist

Per CLAUDE.md "15-Minute Rule": spend 15 minutes testing locally before pushing

Before merging, verify:

1. Rake tasks are defined correctly
2. Filtering works (latest vs minimum examples)
3. Test with minimum config using bin/ci-switch-config
4. RuboCop passes on rakelib/
5. apply_minimum_versions creates correct package.json

---

Recommendations Summary

Before Merge (Required)

1. Run local testing checklist
2. Run bundle exec rubocop
3. Verify CI passes with both latest and minimum configs
4. Add tests for rake tasks (or create follow-up issue)

Future Improvements (Optional)

1. Extract apply_minimum_versions to separate module if it grows
2. Add integration test verifying generated examples work with minimum versions
3. Document minimum version testing strategy in CLAUDE.md

---

Conclusion

Overall: 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

• Fix: Add tests for rake task filtering
• Verify: Run local testing checklist
• Ensure: RuboCop passes

Great work on the refactoring! The separation of concerns is much cleaner now.

[claude]

Code Review: PR #2125

Great work on this refactor! This is a well-thought-out improvement that simplifies the testing strategy. Here's my detailed feedback:

✅ Strengths

1. Clear Separation of Concerns

   • Excellent decision to move version compatibility testing from spec/dummy to generator smoke tests
   • spec/dummy now focuses on latest versions, while generators test the compatibility matrix
   • This better reflects real-world usage where users run generators with specific versions

2. Clean Implementation

   • The minimum_versions flag on ExampleType is a simple, extensible design
   • Good use of Ruby idioms with the minimum_versions? predicate method
   • The apply_minimum_versions method is well-documented and includes error handling

3. CI Improvements

   • Conditional task execution (run_rspec:shakapacker_examples_latest vs _minimum) is cleaner than the previous approach
   • Removed unnecessary React/Shakapacker version modifications from script/convert

4. React 19 Migration

   • Correct dependency updates: react-redux ^9.2.0, redux ^5.0.1, redux-thunk ^3.1.0
   • Good choice switching to @dr.pogodin/react-helmet (thread-safe React 19 fork)
   • Proper SSR implementation with HelmetProvider and context

🔍 Potential Issues & Suggestions

1. Missing @types/react-helmet Removal ⚠️

In react_on_rails/spec/dummy/package.json, the old @types/react-helmet should be removed since @dr.pogodin/react-helmet has built-in TypeScript types.

 "devDependencies": {
   ...
-  "@types/react-helmet": "^6.1.5",

Impact: Minor - may cause type conflicts if old types are still present

2. Error Handling in apply_minimum_versions 💡

The JSON parse error handling is good, but consider logging to stderr and adding context:

rescue JSON::ParserError => e
  warn "  ERROR: Failed to parse package.json in #{dir}: #{e.message}"
  raise
end

Using warn instead of puts ensures error messages go to stderr.

3. Constants Should Be Frozen 🔒

The version strings should be frozen to prevent accidental mutation:

MINIMUM_REACT_VERSION = "18.0.0".freeze
MINIMUM_SHAKAPACKER_VERSION = "8.2.0".freeze

Impact: Low - prevents accidental mutation in Ruby

4. Testing Recommendations 🧪

Before merging, please verify:

1. Rake task generation:

   bundle exec rake -T | grep shakapacker_examples
   # Should show: run_rspec:shakapacker_examples_latest and _minimum

2. Example filtering:

   # In Rails console or test script:
   require './react_on_rails/rakelib/example_type.rb'
   # Verify that minimum examples are correctly identified

3. Package version application:
   Generate a minimum example and verify its package.json has React 18.0.0 and Shakapacker 8.2.0

4. Full CI run:
   Ensure both latest and minimum CI workflows complete successfully

5. Documentation Clarity 📝

The PR description mentions that spec/dummy uses "latest React 19 and Shakapacker 9.4.0" but react_on_rails/spec/dummy/package.json uses ^19.0.0 (caret range). Consider if you want to pin to 19.0.0 exactly for consistency with the minimum versions, or document that latest uses caret ranges.

🎯 Performance & Security

• ✅ No performance concerns
• ✅ No security issues
• ✅ Proper use of JSON parsing with error handling
• ✅ Good separation between development and runtime dependencies

📊 Test Coverage

The test plan in the PR description is comprehensive. All items marked complete except CI verification, which is appropriate for a pre-merge checklist.

🔧 Code Quality

• ✅ Follows Ruby conventions (predicate methods, keyword arguments)
• ✅ Good use of constants for magic strings
• ✅ Clear comments explaining the purpose
• ✅ Consistent with project coding standards

📋 Final Recommendations

Before merging:

1. Remove @types/react-helmet from devDependencies
2. Freeze version constants
3. Change puts to warn for error logging
4. Verify CI passes for both latest and minimum workflows

Nice-to-have (can be follow-up):

• Consider adding a test that validates apply_minimum_versions correctly updates package.json
• Document the new testing strategy in the project's testing documentation

✨ Overall Assessment

This is a solid refactor that improves the testing architecture. The code is clean, well-documented, and addresses a real pain point with the previous approach. With the minor fixes above, this is ready to merge.

Rating: 8.5/10 - Great work! 🚀

---

Review generated with Claude Code following CLAUDE.md guidelines

[coderabbitai]

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 helmet being undefined. If helmet exists but helmet.title is 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 helmet or helmet.title might be undefined.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8442f8 and dbc7a32.

⛔ Files ignored due to path filters (2)

• react_on_rails/spec/dummy/Gemfile.lock is excluded by !**/*.lock
• react_on_rails/spec/dummy/yarn.lock is 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.jsx
• react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx
• react_on_rails/spec/dummy/package.json
• react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx
• react_on_rails/spec/dummy/package.json
• react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx
• react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx
• react_on_rails/spec/dummy/package.json
• react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx
• react_on_rails/spec/dummy/client/app/stores/SharedReduxStore.jsx
• react_on_rails/spec/dummy/package.json
• react_on_rails/spec/dummy/client/app/components/ReactHelmet.jsx
• react_on_rails/spec/dummy/client/app/startup/ReduxApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_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.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.server.jsx
• react_on_rails/spec/dummy/client/app/startup/ReactHelmetApp.client.jsx
• react_on_rails/spec/dummy/package.json
• react_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.jsx
• react_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-helmet is 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-helmet for 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-helmet SSR. This replaces the old Helmet.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-helmet to @dr.pogodin/react-helmet is 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 HelmetProvider from @dr.pogodin/react-helmet is 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.

[coderabbitai]

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 deps and dev_deps is 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 like MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION in ExampleType.

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

📥 Commits

Reviewing files that changed from the base of the PR and between dbc7a32 and 40e9642.

📒 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 rubocop and 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 new apply_minimum_versions method.

---

107-112: LGTM: Clean integration of minimum version pinning.

The conditional application of apply_minimum_versions is correctly placed after generator commands and before npm install. Re-running bundle_install_in after Gemfile modification ensures dependencies are properly updated. The comment clearly explains the rationale.

[justin808]

CI Status Update

✅ Fixed

• Integration Tests: Passing - React 19 compatibility changes working correctly
• Lint JS and Ruby: Passing

🔧 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 react_on_rails/vendor/bundle instead of its own bundle. This causes a version mismatch error:

**ERROR** Shakapacker: Shakapacker gem and node package versions do not match
Detected: 8.2.0
     gem: 9.4.0

This is happening because:

1. The generated app's Gemfile correctly pins shakapacker 8.2.0
2. bundle install correctly installs 8.2.0 in the generated app
3. But when running bundle exec rake in the generated app, Rails loads the Shakapacker railtie from the parent workspace's vendor/bundle (which has 9.4.0)

Potential solutions:

1. Use BUNDLE_GEMFILE explicitly when running rake in the generated app
2. Add BUNDLE_PATH isolation for the generated app
3. Use SHAKAPACKER_SKIP_VERSION_CHECK if available (need to verify this env var exists)

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.

[coderabbitai]

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 to MINIMUM_SHAKAPACKER_VERSION. Extracting it as MINIMUM_WEBPACK_ASSETS_MANIFEST_VERSION in ExampleType would 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

📥 Commits

Reviewing files that changed from the base of the PR and between 40e9642 and ef3377f.

📒 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 rubocop and 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_in after 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_GEMFILE ensures 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.

[justin808]

PR Split Notice

This 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

• react-redux 9.x, redux 5.x, redux-thunk 3.x
• @dr.pogodin/react-helmet (React 19 compatible fork)
• Integration Tests ✅, Lint ✅

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:

• Generator tests failing due to bundler isolation issues when testing with Shakapacker 8.2.0 + React 18
• This is an infrastructure challenge that needs more debugging

Recommendation: Merge PR #2127 first, then continue work on this PR for the minimum version testing.

[claude]

Code Review: PR #2125

Overall Assessment

This 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 Issues

1. PR Scope Too Large
This PR combines TWO major infrastructure changes: version compatibility testing refactor + package manager migration. This makes it harder to isolate failures and violates pr-splitting-strategy.md guidance.

2. Missing Type Definitions for React 19
Root package.json needs @types/react and @types/react-dom updated to v19 to match React 19 runtime. Current v18 types will cause TypeScript build failures.

Code Quality - Excellent Design

✅ Clean separation of concerns with ExampleType class
✅ Robust error handling in apply_minimum_versions
✅ Well-structured CI workflow with conditional tasks
✅ Follows Ruby conventions (minimum_versions? predicate)

Areas for Improvement

1. Gemfile regex in shakapacker_examples.rake:56-67 may not handle multi-line declarations
2. Bundle isolation only for minimum versions adds complexity - document why
3. Script convert fallback logic incomplete - only handles Gemfile paths

Testing Requirements (CRITICAL)

Per CLAUDE.md and testing-build-scripts.md, MUST test locally before merge:

• Clean install: rm -rf node_modules pnpm-lock.yaml && pnpm install --frozen-lockfile
• Build scripts: pnpm run build && pnpm run prepack && pnpm run yalc:publish
• Both rake test suites (latest and minimum)
• Full linting

PR description shows incomplete testing. Do not merge until ALL CI passes.

Must Fix Before Merge

1. Update @types/react and @types/react-dom to ^19.0.0 in root package.json
2. Verify ALL CI checks pass
3. Test pnpm install --frozen-lockfile in fresh environment

Should Consider

4. Add CHANGELOG.md entry (significant architectural change)
5. Document Gemfile regex behavior or make more robust
6. Update CONTRIBUTING.md with minimum version testing guide
7. Consider uniform bundle isolation approach

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

[coderabbitai]

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 role

This section still describes script/convert as 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/convert purpose, 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_dir is built from gem_root, not pro_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/ and react_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_root instead:

-  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.json declares "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 correct

Using pnpm --filter <workspace> run type-check for 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 flow

Declaring packageManager: [email protected] and using pnpm run link-source plus yalc add --link react-on-rails makes 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 fallback

The header comment and current replacements correctly narrow script/convert to:

• Dropping Node‑incompatible dev deps from the root package.json, and
• Adjusting the Pro test:non-rsc script 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_content helper still contains a Gemfile‑specific fallback:

old_path = path.sub(%r{/react_on_rails/Gemfile}, "/Gemfile")

but the script now only operates on ../package.json and ../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 consistency

The “Mandatory Local Testing Checklist” still shows:

rm -rf node_modules && yarn install --frozen-lockfile
yarn run build
yarn run yalc:publish
...
yarn run lint

while the rest of this file now standardizes on pnpm and explicitly says to avoid npm commands.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ef3377f and 34fa151.

⛔ Files ignored due to path filters (6)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
• react_on_rails/spec/dummy/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
• react_on_rails_pro/spec/dummy/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
• react_on_rails_pro/spec/execjs-compatible-dummy/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
• react_on_rails_pro/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
• yarn.lock is 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.tsx
• bin/lefthook/eslint-lint
• CONTRIBUTING.md
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• package-scripts.yml
• bin/lefthook/prettier-format
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• pnpm-workspace.yaml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• packages/react-on-rails-pro/package.json
• knip.ts
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• react_on_rails/spec/dummy/package.json
• packages/react-on-rails-pro/tests/jest.setup.js
• package.json

**/*.{rb,rake}

📄 CodeRabbit inference engine (CLAUDE.md)

ALWAYS run bundle exec rubocop and fix ALL violations before every commit/push

Files:

• react_on_rails/rakelib/dummy_apps.rake
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• react_on_rails/rakelib/shakapacker_examples.rake
• react_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.tsx
• bin/lefthook/eslint-lint
• CONTRIBUTING.md
• .gitignore
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• package-scripts.yml
• bin/lefthook/prettier-format
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• .claude/docs/testing-build-scripts.md
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• .github/workflows/examples.yml
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• .github/workflows/pro-integration-tests.yml
• react_on_rails/spec/dummy/package.json
• packages/react-on-rails-pro/tests/jest.setup.js
• package.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.tsx
• CONTRIBUTING.md
• packages/react-on-rails-pro-node-renderer/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• CLAUDE.md
• packages/react-on-rails-pro/package.json
• knip.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-lint
• CONTRIBUTING.md
• .gitignore
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• package-scripts.yml
• .github/workflows/pro-test-package-and-gem.yml
• bin/lefthook/prettier-format
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• .claude/docs/testing-build-scripts.md
• react_on_rails/rakelib/lint.rake
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• .github/workflows/pro-lint.yml
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• .github/workflows/examples.yml
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• .github/workflows/pro-integration-tests.yml
• react_on_rails/spec/dummy/package.json
• packages/react-on-rails-pro/tests/jest.setup.js
• package.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-lint
• CONTRIBUTING.md
• .gitignore
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• package-scripts.yml
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• .claude/docs/testing-build-scripts.md
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• .github/workflows/pro-lint.yml
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• react_on_rails/spec/dummy/package.json
• packages/react-on-rails-pro/tests/jest.setup.js
• package.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-lint
• CONTRIBUTING.md
• bin/lefthook/prettier-format
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• .claude/docs/testing-build-scripts.md
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• CLAUDE.md
• react_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-lint
• CONTRIBUTING.md
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• .github/workflows/pro-test-package-and-gem.yml
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• .github/workflows/package-js-tests.yml
• .claude/docs/testing-build-scripts.md
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/node_package.rake
• .github/workflows/integration-tests.yml
• CLAUDE.md
• .github/workflows/pro-lint.yml
• packages/react-on-rails-pro/package.json
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• .github/workflows/examples.yml
• react_on_rails/rakelib/release.rake
• react_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-lint
• CONTRIBUTING.md
• .github/workflows/pro-lint.yml
• CHANGELOG.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-lint
• CONTRIBUTING.md
• .gitignore
• bin/lefthook/prettier-format
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• script/convert
• CHANGELOG.md
• react_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-lint
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• packages/react-on-rails-pro/package.json
• script/convert
• CHANGELOG.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.md
• packages/react-on-rails/package.json
• .github/workflows/pro-test-package-and-gem.yml
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• react_on_rails/rakelib/dummy_apps.rake
• .claude/docs/testing-build-scripts.md
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• .github/workflows/pro-lint.yml
• packages/react-on-rails-pro/package.json
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• CHANGELOG.md
• .github/workflows/pro-integration-tests.yml
• react_on_rails/spec/dummy/package.json
• package.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
• .gitignore
• packages/react-on-rails/package.json
• packages/react-on-rails-pro-node-renderer/package.json
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• react_on_rails/rakelib/node_package.rake
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.md
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• react_on_rails/spec/dummy/package.json
• package.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
• .gitignore
• packages/react-on-rails-pro-node-renderer/package.json
• react_on_rails_pro/spec/execjs-compatible-dummy/package.json
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• react_on_rails/rakelib/dummy_apps.rake
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/shakapacker_examples.rake
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• CHANGELOG.md
• react_on_rails/rakelib/release.rake
• react_on_rails/spec/dummy/package.json
• packages/react-on-rails-pro/tests/jest.setup.js
• package.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.md
• packages/react-on-rails-pro/scripts/check-react-version.cjs
• CHANGELOG.md
• react_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.md
• packages/react-on-rails-pro/package.json
• CHANGELOG.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.yml
• react_on_rails/rakelib/dummy_apps.rake
• .github/workflows/integration-tests.yml
• CLAUDE.md
• .github/workflows/pro-lint.yml
• react_on_rails_pro/spec/dummy/package.json
• .github/workflows/pro-integration-tests.yml
• packages/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.yml
• react_on_rails/rakelib/dummy_apps.rake
• react_on_rails/rakelib/lint.rake
• .github/workflows/integration-tests.yml
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.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.yml
• react_on_rails/rakelib/dummy_apps.rake
• .github/workflows/integration-tests.yml
• CLAUDE.md
• .github/workflows/pro-lint.yml
• .github/workflows/pro-integration-tests.yml
• packages/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.md
• packages/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.md
• react_on_rails/rakelib/lint.rake
• react_on_rails/rakelib/shakapacker_examples.rake
• CLAUDE.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.md
• bin/lefthook/prettier-format
• react_on_rails/rakelib/lint.rake
• CLAUDE.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
• .gitignore
• packages/react-on-rails-pro-node-renderer/package.json
• .claude/docs/testing-build-scripts.md
• pnpm-workspace.yaml
• .github/workflows/integration-tests.yml
• CLAUDE.md
• .github/workflows/pro-lint.yml
• packages/react-on-rails-pro/package.json
• knip.ts
• .github/workflows/lint-js-and-ruby.yml
• react_on_rails_pro/spec/dummy/package.json
• script/convert
• package.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.md
• react_on_rails/rakelib/lint.rake
• CLAUDE.md
• .github/workflows/lint-js-and-ruby.yml
• 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: 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:

• .gitignore
• knip.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.yml
• script/convert
• CHANGELOG.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.yml
• CLAUDE.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.cjs
• CHANGELOG.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.md
• CLAUDE.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.md
• CHANGELOG.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.md
• packages/react-on-rails-pro/package.json
• knip.ts
• react_on_rails/spec/dummy/package.json
• packages/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.json
• knip.ts
• CHANGELOG.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.ts file 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 static import statements 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.js already 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 for Readable.toWeb(). The .nvmrc specifies 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.js configures setupFiles: ['<rootDir>/tests/jest.setup.js'], meaning this file runs synchronously during jest initialization
• The test utils.test.js successfully verifies the mock works: it asserts expect(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 ensures window is 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 layout

Ignoring /Gemfile.lock at the root while documenting that the real lock lives under react_on_rails/ keeps the VCS state clean and avoids confusion between locks.

pnpm-workspace.yaml (1)

1-2: Workspace scope definition is sane

Declaring 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 reasonable

Switching 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 tooling

Using pnpm run clean && tsc for build, a dedicated type-check script with tsc --noEmit, and pnpm run build in prepublishOnly fits 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 pnpm

Updating 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 pnpm

Updating 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 consistent

The 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 correct

The script correctly treats React <19 as “skip RSC tests” (exit 0) and React ≥19 as “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‑formed

The 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 correct

Switching to pnpm publish for:

• packages/react-on-rails
• packages/react-on-rails-pro
• react_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 clear

The updated Essential Commands, linting/formatting, build, and changelog sections consistently use pnpm and explicitly forbid npm for 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 sets

The workflow now:

• Sets up pnpm with a cached store and prints its version.
• Installs Node deps via pnpm install (frozen for latest, non‑frozen for minimum) and uses pnpm yalc:publish plus pnpm add -g yalc for the local package flow.
• Runs script/convert only for the minimum dependency level, which matches the script’s new “minimum toolchain” responsibility.
• Routes example specs through the new tasks:
  • run_rspec:shakapacker_examples_latest for latest deps,
  • run_rspec:shakapacker_examples_minimum for 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 workflows

The updated doc:

• Emphasizes clean pnpm install --frozen-lockfile, pnpm run build, pnpm run prepack, and pnpm run yalc:publish as mandatory checks after changing package.json or build configs.
• Calls out workspace-specific behavior and the need to use workspace:* with pnpm, plus verification via pnpm -r list and 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 prettier to pnpm exec prettier is 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 eslint to pnpm exec eslint is 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 run instead 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 network flag 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-workspace flag 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.yaml is 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.yaml for 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-lockfile ensures deterministic, reproducible installs in CI. The --ignore-workspace flag 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 --filter command 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:

1. 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.0 to match React and ensure proper type checking.

2. 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.yaml hash key. This is consistent with the pnpm migration across other workflow files.

---

123-130: LGTM! pnpm commands correctly replace Yarn equivalents.

The conditional --frozen-lockfile logic 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-workspace for dummy app isolation.

The --ignore-workspace flag 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 exec or pnpm run as appropriate. The pnpm start command 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-workspace flag for the dummy app and pnpm exec for 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_versions method correctly:

• Handles JSON parsing errors with informative output and re-raises to fail CI fast
• Updates both dependencies and devDependencies for 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_GEMFILE and BUNDLE_FROZEN=false environment 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-workspace with clear documentation.

The comment explaining why --ignore-workspace is 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.yaml correctly 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-lockfile for latest runs to ensure reproducible builds
• Uses --no-frozen-lockfile for minimum runs (older Node/Ruby) to test compatibility

The --ignore-workspace flag 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-matrix job 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.log to pnpm-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 run commands:

• 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 [email protected] 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.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 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 -5

Repository: 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.ts

Repository: 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 2

Repository: 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 2

Repository: 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.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

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.

Suggested change

	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.