Skip to content

Improve SWC compiler detection and default dependencies#2135

Merged
justin808 merged 7 commits intomasterfrom
jg/swc-detection-fix
Dec 10, 2025
Merged

Improve SWC compiler detection and default dependencies#2135
justin808 merged 7 commits intomasterfrom
jg/swc-detection-fix

Conversation

@justin808
Copy link
Copy Markdown
Member

@justin808 justin808 commented Nov 26, 2025

Summary

  • Add using_swc? helper method to detect SWC configuration in shakapacker.yml
  • Automatically install SWC dependencies (@swc/core, swc-loader) when SWC is configured
  • Update spec/dummy to include SWC packages

Key Changes

1. Enhanced using_swc? Helper

  • Parses shakapacker.yml for javascript_transpiler setting
  • Returns true when SWC is explicitly configured
  • Returns true for Shakapacker 9.3.0+ when not specified (SWC is the default)
  • Returns true for fresh installations (SWC is recommended)

2. Generator Updates

  • Add SWC_DEPENDENCIES constant with @swc/core and swc-loader
  • Install SWC deps automatically when using_swc? returns true
  • Add to devDependencies since they're build-time tools

3. spec/dummy Updates

  • Add @swc/core and swc-loader to devDependencies
  • Matches the javascript_transpiler: swc setting already in shakapacker.yml

Testing

  • Added comprehensive RSpec tests for using_swc? helper
  • All existing tests pass
  • RuboCop checks pass

Test Plan

  • Test generator with Shakapacker 9.3.0+
  • Verify SWC detection logic
  • Test fallback to Babel when explicitly configured

Closes #1956

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Automatic SWC transpiler detection using Shakapacker config and version heuristics
    • Conditional installation of SWC dependencies during project setup when applicable
    • Resilient config parsing with clear precedence and fallback behavior
  • Tests

    • Expanded test coverage for SWC detection, dependency handling, and edge/error scenarios

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 26, 2025

Walkthrough

Adds SWC detection to the generator by parsing shakapacker.yml and falling back to a Shakapacker version check, and wires conditional installation of SWC packages (@swc/core, swc-loader) into the JS dependency manager with tests and spec/dummy package.json updates.

Changes

Cohort / File(s) Change Summary
SWC Detection & Configuration
react_on_rails/lib/generators/react_on_rails/generator_helper.rb
Adds using_swc? (memoized) plus detect_swc_configuration, parse_shakapacker_yml, and shakapacker_version_9_3_or_higher? helpers; parses config/shakapacker.yml, prefers explicit config, falls back to version-based default, and adds resilient YAML parsing and error handling.
SWC Dependency Installation
react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
Adds SWC_DEPENDENCIES = %w[@swc/core swc-loader] and private add_swc_dependencies; add_js_dependencies now conditionally calls add_swc_dependencies when using_swc? is true; includes error handling and manual-install guidance.
Test Suite & Specs
react_on_rails/spec/react_on_rails/generators/generator_helper_spec.rb, react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb, react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb
New/expanded tests covering using_swc? across config scenarios (explicit swc/babel, missing config, parse errors, version boundaries) and integration checks that SWC deps are added conditionally; test helpers updated to allow toggling using_swc?.
Development / Dummy Package
react_on_rails/spec/dummy/package.json
Adds @swc/core and swc-loader entries to package.json for spec/dummy.

Sequence Diagram(s)

sequenceDiagram
  participant Generator as Generator (install generator)
  participant Helper as GeneratorHelper
  participant FS as File System (config/shakapacker.yml)
  participant PackerUtils as ReactOnRails::PackerUtils
  participant JSDeps as JsDependencyManager

  Generator->>Helper: call using_swc?
  Helper->>FS: read config/shakapacker.yml (if exists)
  alt explicit javascript_transpiler present
    FS-->>Helper: returns "swc" or "babel"
    Helper-->>Generator: returns based on explicit value
  else no explicit config
    Helper->>PackerUtils: shakapacker_version_requirement_met?(9.3.0)
    PackerUtils-->>Helper: true / false / error
    Helper-->>Generator: decides default (SWC if >=9.3.0 or fresh install)
  end

  Generator->>JSDeps: add_js_dependencies
  alt using_swc? == true
    JSDeps->>JSDeps: add_swc_dependencies (install SWC deps)
    JSDeps-->>Generator: success / warn with manual instructions
  else
    JSDeps->>JSDeps: add_babel_or_existing_deps
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay special attention to generator_helper.rb for precedence (explicit config vs. version fallback), memoization reset behavior, and resilient YAML parsing across Psych versions.
  • Verify js_dependency_manager.rb matches existing dependency-install patterns and error messaging consistency.
  • Review new specs for reliable setup/teardown and correct stubbing of PackerUtils version checks.

Possibly related issues

  • #1956: Directly aligns — enhances SWC detection, parses shakapacker.yml, and defaults SWC for Shakapacker 9.3.0+.
  • #2198: Matches objective to migrate spec/dummy to SWC and add SWC deps in package.json.

Possibly related PRs

  • #1852: Related change-set around transpiler selection and generator config wiring (overlaps on transpiler selection logic).
  • #2051: Related JS dependency management and SWC defaulting changes.
  • #1932: Similar SWC adoption changes (dependencies and shakapacker.yml handling).

Suggested reviewers

  • alexeyr-ci
  • Judahmeek

Poem

🐰✨ I hopped through configs, sniffed version and file,
If Shakapacker's new, I chose SWC with a smile.
I bundled the loaders, left notes if things break,
Tests hopped along—now builds wake and partake! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title concisely describes the main change: improving SWC detection and establishing SWC as default dependencies, which aligns with the primary objective of the changeset.
Linked Issues check ✅ Passed The PR implements core requirements from #1956: enhances using_swc? to parse shakapacker.yml with SWC fallback logic for 9.3.0+, adds SWC dependencies to generator, and includes comprehensive tests, though documentation and generated template updates are not covered.
Out of Scope Changes check ✅ Passed All changes are directly related to SWC detection and dependency management requirements from #1956; test dependencies in spec/dummy align with implementation validation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jg/swc-detection-fix

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

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

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Nov 26, 2025

Code Review: PR #2135 - Improve SWC compiler detection and default dependencies

Summary

This PR adds intelligent SWC (Speedy Web Compiler) detection and automatic dependency installation for the React on Rails generator. Overall, the implementation is well-designed and production-ready with excellent test coverage and error handling.


✅ Strengths

1. Excellent Error Handling & Graceful Degradation

The code follows a defensive programming approach that aligns well with the project's error handling philosophy:

def parse_shakapacker_yml(path)
  require "yaml"
  YAML.load_file(path, aliases: true)
rescue ArgumentError
  # Older Psych versions don't support the aliases parameter
  YAML.load_file(path)
rescue StandardError
  # If we can't parse the file, return empty config
  {}
end
  • ✅ Handles both old and new Psych versions
  • ✅ Returns empty config instead of raising exceptions
  • ✅ Prevents generator crashes from malformed YAML files

2. Smart Default Behavior

The detection logic makes sensible assumptions:

  • Explicitly configured transpilers take precedence (babel vs swc)
  • Defaults to SWC for Shakapacker 9.3.0+ (matches upstream default)
  • Assumes latest version for fresh installs (optimistic but safe)
  • Falls back gracefully when version detection fails

3. Comprehensive Test Coverage

The specs cover all edge cases:

 Explicit SWC configuration
 Explicit Babel configuration  
 No transpiler specified (version-based default)
 Missing shakapacker.yml
 Malformed YAML parsing errors

4. Proper Memoization

def using_swc?
  return @using_swc if defined?(@using_swc)
  @using_swc = detect_swc_configuration
end

Uses defined?(@using_swc) instead of just checking truthiness, which correctly handles false values.

5. Clear Documentation

Excellent inline documentation explaining:

  • What the method does
  • Detection logic flow
  • Why certain defaults are chosen
  • Performance benefits ("20x faster than Babel")

🔍 Potential Issues & Suggestions

1. Missing Version Check Implementation ⚠️

The code calls shakapacker_version_9_3_or_higher? but I notice it's a new method. Let me verify the implementation:

def shakapacker_version_9_3_or_higher?
  return true unless defined?(ReactOnRails::PackerUtils)
  
  ReactOnRails::PackerUtils.shakapacker_version_requirement_met?("9.3.0")
rescue StandardError
  # If we can't determine version, assume latest (which uses SWC)
  true
end

Good: This follows the same pattern as the existing shakapacker_version_9_or_higher? method.

Concern: The method returns true when ReactOnRails::PackerUtils is not defined. During generator runtime, this might not be defined yet.

Recommendation: Verify this works correctly during fresh installations where Shakapacker hasn't been installed yet. The optimistic default is probably correct, but worth confirming.

2. Dependency Installation Ordering 💡

In js_dependency_manager.rb:

def add_js_dependencies
  add_react_on_rails_package
  add_react_dependencies
  add_css_dependencies
  add_rspack_dependencies if respond_to?(:options) && options&.rspack?
  add_swc_dependencies if using_swc?  # ← Called here
  add_dev_dependencies
end

Question: Should SWC dependencies be added earlier in the sequence, perhaps before CSS dependencies? Build tools typically need to be installed before other dependencies that might use them.

Recommendation: Consider moving add_swc_dependencies right after add_react_on_rails_package or at least before add_css_dependencies.

3. YAML.load_file Security 🔒

YAML.load_file(path, aliases: true)

Current: Uses YAML.load_file which can execute arbitrary Ruby code in older YAML versions.

Recommendation: Consider using YAML.safe_load_file for better security:

def parse_shakapacker_yml(path)
  require "yaml"
  YAML.safe_load_file(path, aliases: true, permitted_classes: [Symbol])
rescue ArgumentError
  YAML.safe_load_file(path, permitted_classes: [Symbol])
rescue StandardError
  {}
end

However: Since this is reading the user's own shakapacker.yml config file (not external input), the security risk is minimal. But it's worth considering for defense-in-depth.

4. Missing Test: Version Boundary 📝

The tests cover the logic well, but there's no test for the version boundary:

Suggestion: Add a test that mocks ReactOnRails::PackerUtils.shakapacker_version_requirement_met? to return both true and false for version 9.3.0.

Example:

context "when Shakapacker version is 9.2.0 (before SWC default)" do
  before do
    allow(ReactOnRails::PackerUtils).to receive(:shakapacker_version_requirement_met?)
      .with("9.3.0").and_return(false)
  end

  it "returns false when no transpiler is specified" do
    File.write(shakapacker_yml_path, "default: {}\n")
    expect(using_swc?).to be false
  end
end

5. Potential Race Condition with Memoization 🧵

The memoization uses instance variables:

def using_swc?
  return @using_swc if defined?(@using_swc)
  @using_swc = detect_swc_configuration
end

Concern: If shakapacker.yml changes during generator execution, the memoized value won't update.

Reality Check: This is probably fine since generators run quickly and config files shouldn't change mid-execution.

Optional Enhancement: Add a comment explaining this caching behavior.


🎯 Code Quality Assessment

Category Rating Notes
Error Handling ⭐⭐⭐⭐⭐ Excellent graceful degradation
Test Coverage ⭐⭐⭐⭐½ Comprehensive, missing version boundary test
Documentation ⭐⭐⭐⭐⭐ Clear comments and inline docs
Code Style ⭐⭐⭐⭐⭐ Follows project conventions
Security ⭐⭐⭐⭐☆ Minor YAML.load concern (low risk)
Performance ⭐⭐⭐⭐⭐ Proper memoization

🚀 Testing Recommendations

Before merging, please verify:

  1. Fresh Installation Test:

    # In a new Rails app without Shakapacker
    rails new test_app
    cd test_app
    bundle add react_on_rails --git=<your-branch>
    rails generate react_on_rails:install
    # Verify SWC dependencies are installed
    grep -E '@swc/core|swc-loader' package.json
  2. Babel Override Test:

    # Edit config/shakapacker.yml BEFORE running generator
    # Set javascript_transpiler: babel
    rails generate react_on_rails:install
    # Verify SWC dependencies are NOT installed
    grep -E '@swc/core|swc-loader' package.json  # Should be empty
  3. Existing Installation Test:

    # In an app with existing Shakapacker 9.3.0+
    rails generate react_on_rails:install
    # Verify SWC deps are added

📋 Checklist for Author

  • Manual testing completed per recommendations above
  • CI passes (RuboCop, RSpec)
  • Tested with both fresh and existing installations
  • Verified SWC deps are correctly installed/skipped based on config
  • Consider adding version boundary test (optional)
  • Consider YAML.safe_load_file (optional security hardening)

✅ Final Recommendation

APPROVE with minor suggestions.

This is a well-implemented feature that:

  • Solves a real problem (manual SWC dependency installation)
  • Has excellent error handling
  • Includes comprehensive tests
  • Follows project conventions
  • Makes sensible default assumptions

The suggestions above are minor improvements, not blocking issues. The code is production-ready as-is.

Great work! 🎉


Reviewed by: Claude Code
Date: 2025-11-26

@justin808
Copy link
Copy Markdown
Member Author

/run-skipped-ci

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 9, 2025

🚀 Full CI Mode Enabled

Triggered all workflows for full CI coverage

Note: Added the full-ci label to this PR. All future commits will run the full CI suite (including minimum dependency tests) until the label is removed.

To disable full CI mode, use the /stop-run-skipped-ci command.

View progress in the Actions tab.

@claude claude Bot mentioned this pull request Dec 9, 2025
3 tasks
justin808 and others added 5 commits December 8, 2025 19:59
Add using_swc? helper method to detect SWC configuration:
- Parses shakapacker.yml for javascript_transpiler setting
- Returns true when SWC is explicitly configured
- Returns true for Shakapacker 9.3.0+ when not specified (SWC default)
- Returns true for fresh installations (SWC is recommended)

Add SWC dependency installation to generator:
- Add SWC_DEPENDENCIES constant (@swc/core, swc-loader)
- Install SWC deps automatically when using_swc? returns true
- Add to devDependencies since they're build-time tools

Update spec/dummy to include SWC packages:
- Add @swc/core and swc-loader to devDependencies
- Matches the javascript_transpiler: swc setting in shakapacker.yml

Add comprehensive tests for using_swc? helper:
- Test explicit swc config returns true
- Test explicit babel config returns false
- Test missing config defaults to SWC for 9.3.0+
- Test missing file defaults to SWC
- Test invalid YAML defaults to SWC

Closes #1956

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
The JsDependencyManager tests create a mock test class that includes
the module. After adding the using_swc? call to add_js_dependencies,
the test class needs to provide this method (which comes from
GeneratorHelper in actual generator classes).

Changes:
- Add using_swc? method stub to test class (defaults to true)
- Add using_swc setter for test control
- Add test for SWC_DEPENDENCIES constant

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Changes based on code review feedback:

1. Security: Use YAML.safe_load_file instead of YAML.load_file
   - Defense-in-depth even though it's the user's own config
   - Added fallback chain for older Psych versions

2. Documentation: Add caching behavior note to using_swc?
   - Explains memoization is per-generator-instance
   - Notes config changes during execution won't be reflected

3. Tests: Add version boundary tests for Shakapacker 9.3.0
   - Test SWC default when version is 9.3.0+
   - Test Babel default when version is below 9.3.0
   - Test graceful fallback when version check fails

4. Tests: Add generator test verifying SWC dependency installation
   - Confirms @swc/core and swc-loader in devDependencies
   - Mirrors existing rspack dependency test pattern

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Remove `result-encoding: string` from the github-script step.
With this setting, returning an object produces "[object Object]"
instead of proper JSON, causing fromJSON() to fail with
"Unexpected character 'o'".

Without result-encoding, github-script returns proper JSON by default.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Tests for using_swc? and SWC dependency installation were failing
in the "minimum" CI environment (Shakapacker 8.2.0) because they
expected SWC to be the default, but SWC is only default for
Shakapacker 9.3.0+.

Changes:
- Stub ReactOnRails::PackerUtils in generator_helper_spec.rb tests
  that check default SWC behavior (fresh install, missing config,
  parse errors) to simulate Shakapacker 9.3.0+
- Update install_generator_spec.rb to check actual Shakapacker
  version and expect appropriate transpiler dependencies

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@justin808 justin808 force-pushed the jg/swc-detection-fix branch from 457ee41 to d02af07 Compare December 9, 2025 05:59
@justin808
Copy link
Copy Markdown
Member Author

@claude review this PR.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Dec 10, 2025

Claude finished @justin808's task in 2m 10s —— View job


Code Review for PR #2135

This PR adds intelligent SWC (Speedy Web Compiler) detection and automatic dependency installation. The implementation is production-ready with excellent error handling, comprehensive test coverage, and well-thought-out design.


✅ Strengths

1. Outstanding Error Handling & Graceful Degradation

The code demonstrates defense-in-depth error handling that prevents generator crashes:

YAML Parsing (generator_helper.rb:169-186)

def parse_shakapacker_yml(path)
  # Uses safe_load_file for security (defense-in-depth)
  YAML.safe_load_file(path, permitted_classes: [Symbol], aliases: true)
rescue ArgumentError
  # Handles older Psych versions gracefully
  begin
    YAML.safe_load_file(path, permitted_classes: [Symbol])
  rescue ArgumentError
    # Very old Psych - fall back to safe_load
    YAML.safe_load(File.read(path), permitted_classes: [Symbol])
  end
rescue StandardError
  # Returns empty config instead of crashing
  {}
end

Key improvements since my November review:

  • Security enhancement: Now uses YAML.safe_load_file instead of YAML.load_file (addressed my security suggestion)
  • Triple-layer fallback: Handles new Psych → old Psych → very old Psych gracefully
  • Never crashes: Returns {} on any error, preventing generator failures

2. Smart Version-Based Defaults

The detection logic makes excellent assumptions (generator_helper.rb:143-167):

  1. Explicit config wins: If javascript_transpiler is set, use that value
  2. Version-aware defaults: Shakapacker 9.3.0+ defaults to SWC
  3. Optimistic for fresh installs: Assumes latest version when Shakapacker isn't installed yet
  4. Graceful degradation: Returns true if version check fails (optimistic but safe)

This aligns perfectly with the "pit of success" design philosophy - new users get the best defaults.

3. Comprehensive Test Coverage

The test suite (generator_helper_spec.rb:113-254) covers all edge cases:

  • ✅ Explicit SWC configuration
  • ✅ Explicit Babel configuration
  • ✅ No transpiler specified (version-based default)
  • ✅ Missing shakapacker.yml
  • ✅ Malformed YAML (parse errors)
  • NEW: Version boundary scenarios (9.3.0+ vs <9.3.0)
  • NEW: PackerUtils error handling during version checks

Excellent improvement: The version boundary tests (lines 203-253) address my November suggestion to test the 9.3.0 cutoff explicitly.

4. Proper Memoization Pattern

def using_swc?
  return @using_swc if defined?(@using_swc)
  @using_swc = detect_swc_configuration
end

Uses defined?(@using_swc) correctly - handles both nil and false values properly, unlike a simple ||= operator.

5. Excellent Documentation

  • Clear inline comments explaining detection logic
  • Performance benefits noted ("20x faster than Babel")
  • Rationale for defaults documented (generator_helper.rb:107-112)
  • Caching behavior explained (generator_helper.rb:140-142)

🎯 Minor Observations (Not Blocking)

1. Workflow File Change

The removal of result-encoding: string in .github/workflows/detect-invalid-ci-commands.yml appears unrelated to SWC detection.

Question: Was this a bug fix discovered during development? If so, worth noting in the PR description.

Impact: None - this is a valid fix (GitHub Actions defaults to string encoding anyway).

2. Integration Test Design

The integration test (install_generator_spec.rb:16-32) uses runtime version checking:

swc_is_default = ReactOnRails::PackerUtils.shakapacker_version_requirement_met?("9.3.0")

if swc_is_default
  expect(package_json["devDependencies"]).to include("@swc/core")
else
  expect(package_json["devDependencies"]).not_to include("@swc/core")
end

Observation: This test's behavior depends on which Shakapacker version is installed in the CI environment. It's smart (adapts to environment) but could be confusing if CI runs with different Shakapacker versions.

Recommendation: Consider adding a comment explaining this is intentional:

# This test verifies the generator adapts to the Shakapacker version in the current environment
swc_is_default = ReactOnRails::PackerUtils.shakapacker_version_requirement_met?("9.3.0")

3. Dependency Installation Ordering

In js_dependency_manager.rb:122-132:

def add_js_dependencies
  add_react_on_rails_package
  add_react_dependencies
  add_css_dependencies
  add_rspack_dependencies if respond_to?(:options) && options&.rspack?
  add_swc_dependencies if using_swc?
  add_dev_dependencies
end

Observation: SWC dependencies are added near the end, after CSS dependencies. Build tools are typically installed first.

Impact: Minimal - package managers install all dependencies together anyway. But for consistency with typical dependency ordering conventions, consider moving add_swc_dependencies earlier (e.g., right after add_react_on_rails_package).

Verdict: Not blocking - current order works fine.


🔍 Detailed Code Analysis

Security: YAML Parsing ✅

Previous concern (my November review): Used YAML.load_file which can execute arbitrary Ruby code.

Current implementation: Uses YAML.safe_load_file with explicit permitted_classes: [Symbol].

Verdict: ✅ EXCELLENT - Addressed my security suggestion comprehensively.

Error Handling Philosophy ✅

The entire module follows the "graceful degradation" pattern documented in js_dependency_manager.rb:32-45:

All dependency addition methods use a graceful degradation approach:

  • Methods return false on failure instead of raising exceptions
  • Failures trigger user-facing warnings via GeneratorMessages
  • This ensures the generator ALWAYS completes successfully

Verdict: ✅ PRODUCTION-READY - Prevents generator crashes, guides users to fix issues manually.

Test Quality ✅

All tests use proper setup/teardown:

before do
  remove_instance_variable(:@using_swc) if instance_variable_defined?(:@using_swc)
  FileUtils.mkdir_p(File.join(destination_root, "config"))
end

after do
  FileUtils.rm_rf(File.join(destination_root, "config"))
end

Verdict: ✅ CLEAN - No test pollution, proper isolation.


📊 Code Quality Assessment

Category Rating Notes
Error Handling ⭐⭐⭐⭐⭐ Triple-layer fallback, graceful degradation
Security ⭐⭐⭐⭐⭐ Uses safe_load_file, explicit permitted classes
Test Coverage ⭐⭐⭐⭐⭐ Comprehensive, includes edge cases & version boundaries
Documentation ⭐⭐⭐⭐⭐ Clear rationale, inline comments, design decisions explained
Code Style ⭐⭐⭐⭐⭐ Follows project conventions, RuboCop compliant
Performance ⭐⭐⭐⭐⭐ Proper memoization, efficient YAML parsing

Overall: ⭐⭐⭐⭐⭐ PRODUCTION-READY


🚀 Improvements Since November Review

  1. Security: Switched from YAML.load_file to YAML.safe_load_file
  2. Test Coverage: Added version boundary tests (9.3.0+ vs <9.3.0)
  3. Error Handling: Added PackerUtils exception handling tests
  4. Robustness: Triple-layer Psych version fallback

These address all suggestions from my previous review. Excellent iteration!


📋 Final Recommendation

✅ APPROVE

This PR is ready to merge. The code is:

  • Well-tested with comprehensive edge case coverage
  • Secure (uses safe_load_file)
  • Robust (graceful error handling throughout)
  • Well-documented (clear rationale for design decisions)
  • Production-ready (no blocking issues)

The minor observations above are optional improvements, not blocking issues.


🎉 Highlights

What I particularly like:

  1. Defense in depth: Triple-layer YAML parsing fallback
  2. User-friendly: Warns users about failures instead of crashing
  3. Smart defaults: Optimistic assumptions that lead to best practices
  4. Test quality: Version boundary tests ensure correctness across Shakapacker versions
  5. Security: Uses safe_load_file even though risk is minimal (defense in depth)

Great work addressing the review feedback and iterating on the implementation! 🚀


Reviewed by: Claude
Review Date: 2025-12-10

justin808 and others added 2 commits December 9, 2025 16:31
The actions/github-script@v7 action needs explicit result-encoding: string
to ensure proper JSON parsing in subsequent workflow steps.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Explains that the test intentionally adapts to the current environment's
Shakapacker version, since CI runs with both minimum (8.x) and latest (9.x).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1)

16-34: Well-structured test for conditional SWC dependency installation.

The test correctly validates that SWC dependencies are installed conditionally based on Shakapacker version. The inline comments explaining CI behavior with both Shakapacker 8.x and 9.x are helpful.

Minor consistency suggestion: Line 31 only checks for absence of @swc/core but not swc-loader. For symmetry with the positive case (lines 26-27), consider also asserting swc-loader absence:

          # For older Shakapacker versions, SWC is NOT installed by default
          # (Babel is the default, but we don't install Babel deps since Shakapacker handles it)
          expect(package_json["devDependencies"]).not_to include("@swc/core")
+         expect(package_json["devDependencies"]).not_to include("swc-loader")
react_on_rails/lib/generators/react_on_rails/generator_helper.rb (1)

188-197: Consider adding memoization for consistency.

This method mirrors shakapacker_version_9_or_higher? but lacks the memoization pattern. While the result is effectively cached at the using_swc? level, adding memoization here would:

  1. Improve consistency with shakapacker_version_9_or_higher?
  2. Protect against future callers who might invoke this method directly
+  # Check if Shakapacker 9.3.0 or higher is available
+  # This version made SWC the default JavaScript transpiler
   def shakapacker_version_9_3_or_higher?
+    return @shakapacker_version_9_3_or_higher if defined?(@shakapacker_version_9_3_or_higher)
+
+    @shakapacker_version_9_3_or_higher = begin
-    return true unless defined?(ReactOnRails::PackerUtils)
+      return true unless defined?(ReactOnRails::PackerUtils)
 
-    ReactOnRails::PackerUtils.shakapacker_version_requirement_met?("9.3.0")
+      ReactOnRails::PackerUtils.shakapacker_version_requirement_met?("9.3.0")
+    rescue StandardError
+      true
+    end
-  rescue StandardError
-    # If we can't determine version, assume latest (which uses SWC)
-    true
   end
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7f23d7 and 4e72069.

📒 Files selected for processing (6)
  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb (1 hunks)
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb (3 hunks)
  • react_on_rails/spec/dummy/package.json (2 hunks)
  • react_on_rails/spec/react_on_rails/generators/generator_helper_spec.rb (1 hunks)
  • react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (1 hunks)
  • react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • react_on_rails/spec/react_on_rails/generators/generator_helper_spec.rb
  • react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb
🧰 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/spec/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
**/*

📄 CodeRabbit inference engine (CLAUDE.md)

ALWAYS ensure files end with a newline character before committing/pushing

Files:

  • react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
🧠 Learnings (11)
📚 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/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
📚 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/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/spec/dummy/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: 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/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately

Applied to files:

  • react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/spec/dummy/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: Applies to spec/dummy/e2e/playwright/**/*.spec.js : Use Rails helpers (`appFactories`, `appEval`) in Playwright tests instead of UI-based setup to leverage factory_bot and arbitrary Ruby code execution

Applied to files:

  • react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.

Applied to files:

  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing

Applied to files:

  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.

Applied to files:

  • react_on_rails/lib/generators/react_on_rails/generator_helper.rb
  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

  • react_on_rails/spec/dummy/package.json
  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing

Applied to files:

  • react_on_rails/spec/dummy/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:

  • react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb
🧬 Code graph analysis (2)
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb (2)
react_on_rails/lib/generators/react_on_rails/generator_helper.rb (1)
  • package_json (7-20)
react_on_rails/spec/react_on_rails/generators/generator_helper_spec.rb (6)
  • shakapacker_version_requirement_met? (160-162)
  • shakapacker_version_requirement_met? (176-178)
  • shakapacker_version_requirement_met? (192-194)
  • shakapacker_version_requirement_met? (214-216)
  • shakapacker_version_requirement_met? (228-231)
  • shakapacker_version_requirement_met? (243-245)
react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb (1)
react_on_rails/lib/generators/react_on_rails/generator_helper.rb (1)
  • using_swc? (143-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
  • GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: examples (3.2, minimum)
  • GitHub Check: examples (3.4, latest)
  • GitHub Check: build
  • GitHub Check: claude-review
🔇 Additional comments (7)
react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb (3)

102-107: LGTM! SWC_DEPENDENCIES constant properly defined.

The constant is correctly frozen and contains the essential SWC packages. Placing this alongside other dependency constants maintains good organization.


244-262: LGTM! add_swc_dependencies follows established patterns.

The method is consistent with other add_*_dependencies methods in the module. The error handling and user-friendly warning messages with manual installation instructions follow the module's "graceful degradation" philosophy documented in the header comments.


128-129: LGTM! SWC dependency integration is correctly placed.

The conditional installation after Rspack dependencies and before dev dependencies follows a logical order. The using_swc? method is provided by GeneratorHelper which must be included by any class using this module.

react_on_rails/lib/generators/react_on_rails/generator_helper.rb (3)

143-147: LGTM! Well-documented public API for SWC detection.

The using_swc? method follows the same memoization pattern as shakapacker_version_9_or_higher? and includes comprehensive documentation explaining the detection logic and caching behavior.


151-167: LGTM! Detection logic correctly prioritizes explicit configuration.

The precedence is well-designed:

  1. Explicit javascript_transpiler: swc → true
  2. Explicit javascript_transpiler: babel (or other) → false
  3. No explicit config + Shakapacker 9.3.0+ → true (SWC is default)
  4. Fresh install → delegates to version check

This aligns with the PR objective of making SWC the default for Shakapacker 9.3.0+ while respecting explicit Babel configuration.


169-186: Robust YAML parsing with appropriate version fallbacks.

The cascading fallback strategy handles different Psych versions well:

  1. Full-featured safe_load_file with aliases support
  2. Without aliases: for older Psych
  3. safe_load with File.read for very old Psych

The permitted_classes: [Symbol] is appropriate since shakapacker.yml may use symbol keys, and aliases: true supports Rails-style YAML anchors (&default, *default).

react_on_rails/spec/dummy/package.json (1)

39-39: Fix version inconsistency and add trailing newline.

The @swc/core version in spec/dummy/package.json (^1.7.0) does not match the root package.json (^1.15.0). Per the learnings, maintain version consistency across package.json files in the codebase. Additionally, the file is missing a trailing newline character, which violates the coding guidelines requiring all files to end with a newline.

⛔ Skipped due to 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.
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 **/* : ALWAYS ensure files end with a newline character before committing/pushing

@justin808 justin808 merged commit c59bcf4 into master Dec 10, 2025
24 checks passed
@justin808 justin808 deleted the jg/swc-detection-fix branch December 10, 2025 02:59
justin808 added a commit that referenced this pull request Dec 11, 2025
Coalesce all prior beta versions into a single 16.2.0.beta.19 section. Add entries for:
- SWC compiler detection improvements (#2135)
- TypeScript processPromise return type fix (#2204)
- connection_pool 3.0+ compatibility fix (#2125, addressing #2185)

Update version headers and comparison links to reflect beta.19.
justin808 added a commit that referenced this pull request Dec 11, 2025
Coalesce all prior beta versions into a single 16.2.0.beta.19 section. Add entries for:
- SWC compiler detection improvements (#2135)
- TypeScript processPromise return type fix (#2204)
- connection_pool 3.0+ compatibility fix (#2125, addressing #2185)

Update version headers and comparison links to reflect beta.19.

Also update release script to mention /update-changelog Claude Code command
as the preferred option for updating changelogs after a release.
justin808 added a commit that referenced this pull request Dec 11, 2025
Coalesce all prior beta versions into a single 16.2.0.beta.19 section. Add entries for:
- SWC compiler detection improvements (#2135)
- TypeScript processPromise return type fix (#2204)
- connection_pool 3.0+ compatibility fix (#2125, addressing #2185)

Update version headers and comparison links to reflect beta.19.

Also update release script to mention /update-changelog Claude Code command
as the preferred option for updating changelogs after a release.
justin808 added a commit that referenced this pull request Dec 11, 2025
### Summary

Consolidate all prior beta versions into a single v16.2.0.beta.19
changelog section. This reflects the packages published to npm and
RubyGems:
- react-on-rails 16.2.0-beta.19
- react-on-rails-pro 16.2.0-beta.19  
- react_on_rails 16.2.0.beta.19
- react_on_rails_pro 16.2.0.beta.19

Added new changelog entries for recent fixes since beta.13:
- SWC compiler detection improvements (PR #2135)
- TypeScript processPromise return type fix (PR #2204)
- connection_pool 3.0+ compatibility fix (PR #2125, addressing issue
#2185)

Also updated the release script's "Next steps" message to mention the
`/update-changelog` Claude Code command as the preferred option for
updating changelogs after releases.

### Pull Request checklist

- [x] Update CHANGELOG file

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

## Release Notes

* **New Features**
* Automatic SWC package detection and installation; SWC becomes default
for Shakapacker 9.3.0+ with automatic package installation.

* **Bug Fixes**
  * Resolved TypeScript compilation issue.
  * Fixed compatibility with connection_pool 3.0+.

* **Chores**
  * Version updated to 16.2.0.beta.19.
  * Changelog references updated.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve SWC compiler detection and configuration for Shakapacker 9.3.0+

1 participant