Add lockfile version resolution for exact version checking

[justin808]

Summary

This PR applies the same approach used in shakacode/shakapacker#170 to React on Rails for keeping versions in sync by resolving exact package versions from lockfiles.

Key Changes

• Lockfile parsing support: Added methods to parse both yarn.lock (v1) and package-lock.json (v1, v2, v3) formats
• Version resolution priority:
  1. Try yarn.lock first if available
  2. Fall back to package-lock.json if available
  3. Use package.json version as final fallback
• Improved accuracy: Uses exact resolved versions from lockfiles instead of semver ranges for version mismatch detection
• Both packages supported: Works with both react-on-rails and react-on-rails-pro packages

Implementation Details

The NodePackageVersion class now:

• Accepts yarn_lock and package_lock paths in addition to package_json
• Implements version_from_yarn_lock method for parsing Yarn lockfiles
• Implements version_from_package_lock method for parsing npm lockfiles (supports both v1 and v2/v3 formats)
• Uses a resolve_version method to check lockfiles before falling back to package.json

Test Coverage

Added comprehensive test coverage including:

• ✅ Yarn.lock version resolution for standard package
• ✅ Package-lock.json version resolution for standard package
• ✅ Yarn.lock version resolution for Pro package
• ✅ Package-lock.json version resolution for Pro package
• ✅ Lockfile preference (yarn.lock over package-lock.json)
• ✅ Fallback to package.json when no lockfile exists
• ✅ Exact version resolution when lockfile matches package.json

All existing tests continue to pass.

Benefits

This change makes version checking more accurate by using the exact resolved version from lockfiles rather than semver ranges, which helps catch version mismatches earlier and more reliably.

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved package version detection to prioritize resolved versions from dependency lockfiles (yarn.lock and package-lock.json) for more reliable tracking, with automatic fallback to package manifest when lockfiles are unavailable or unreadable.

• Tests

  • Added comprehensive test fixtures and coverage for lockfile-based version resolution across multiple scenarios.

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 57 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3e889c1 and 893bd3e.

⛔ Files ignored due to path filters (5)

• spec/react_on_rails/fixtures/malformed_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/pro_semver_caret_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/semver_caret_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/semver_exact_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/similar_packages_yarn.lock is excluded by !**/*.lock

📒 Files selected for processing (9)

• lib/react_on_rails/version_checker.rb (4 hunks)
• spec/react_on_rails/fixtures/malformed_package-lock.txt (1 hunks)
• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_caret_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_caret_package-lock_v1.json (1 hunks)
• spec/react_on_rails/fixtures/semver_exact_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_exact_package.json (1 hunks)
• spec/react_on_rails/fixtures/similar_packages_package.json (1 hunks)
• spec/react_on_rails/version_checker_spec.rb (1 hunks)

Walkthrough

The NodePackageVersion class is extended to track yarn.lock and package-lock.json alongside package.json. The constructor now accepts paths to both lockfiles, and version resolution logic is updated to prioritize reading versions from lockfiles over package.json when available, with a fallback chain and parsing support for multiple lockfile formats.

Changes

Cohort / File(s)	Summary
Main Implementation lib/react_on_rails/version_checker.rb	Extended NodePackageVersion constructor to accept yarn_lock and package_lock paths as optional parameters; added attribute readers for both lockfiles; introduced resolve_version, version_from_yarn_lock, and version_from_package_lock private methods to parse versions with precedence: yarn.lock → package-lock.json → package.json; updated raw method to delegate version resolution.
Package-lock.json Test Fixtures spec/react_on_rails/fixtures/semver_caret_package-lock.json, spec/react_on_rails/fixtures/semver_exact_package-lock.json, spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json	Added three complete package-lock.json fixture files (lockfileVersion 2) for testing version resolution across different semver ranges and pro-package scenarios.
Package.json Test Fixture spec/react_on_rails/fixtures/semver_exact_package.json	Added package.json fixture pinning react-on-rails to exact version 16.1.1 for fallback testing scenarios.
Test Coverage spec/react_on_rails/version_checker_spec.rb	Introduced comprehensive test contexts validating lockfile-based version resolution, precedence rules (yarn.lock priority), fallback behavior when lockfiles are absent, and pro-package scenarios.

Sequence Diagram

sequenceDiagram
    participant Test as Test/Caller
    participant NV as NodePackageVersion
    participant YL as yarn.lock
    participant PL as package-lock.json
    participant PJ as package.json

    Test->>NV: resolve_version(package_json_version, "react-on-rails")
    
    alt yarn.lock provided
        NV->>YL: version_from_yarn_lock("react-on-rails")
        YL-->>NV: version (if found)
    else yarn.lock absent
        Note over NV: Skip yarn.lock
    end
    
    alt version not resolved and package-lock.json provided
        NV->>PL: version_from_package_lock("react-on-rails")
        PL-->>NV: version (if found)
    else package-lock.json absent or no match
        Note over NV: Skip package-lock.json
    end
    
    alt version still not resolved
        NV->>PJ: Fall back to package_json_version
        PJ-->>NV: version from package.json
    end
    
    NV-->>Test: Final resolved version

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Focus areas:
  • Logic density in resolve_version, version_from_yarn_lock, and version_from_package_lock methods—verify correct parsing of both lockfile formats and edge cases
  • Constructor signature change—confirm all callers pass the new parameters correctly and backward compatibility considerations
  • Precedence and fallback chain—ensure prioritization logic (yarn.lock → package-lock.json → package.json) works as intended
  • Test fixture correctness—validate lockfile structure matches npm/yarn specification

Possibly related PRs

• Unify release scripts and add strict version validation #1881: Modifies the same VersionChecker::NodePackageVersion behavior for version determination and class public interface.
• Fix Version Checker's hard-coded path for package.json #1657: Alters lib/react_on_rails/version_checker.rb constructor and version-resolution logic for the same class.
• Fix invalid version checker warnings and add pre-release comparison #1742: Changes method signatures and version parsing behavior in lib/react_on_rails/version_checker.rb.

Suggested reviewers

• Judahmeek
• AbanoubGhadban
• alexeyr-ci2

Poem

🐰 A rabbit's verse on lockfiles new:
Two lockfiles join the version queue,
Yarn first, then npm, no less—
Fallback to package, confess!
Locked and loaded, precedence true.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Add lockfile version resolution for exact version checking" accurately and directly describes the primary change in this changeset. The core modification extends NodePackageVersion to parse and prioritize versions from yarn.lock and package-lock.json files rather than relying solely on package.json, which aligns precisely with the stated objective of resolving "exact package versions from lockfiles so version checks use resolved versions instead of semver ranges." The title is concise at eight words, uses clear and specific terminology, and contains no vague language, emojis, or noise. A teammate scanning the git history would immediately understand that this PR implements lockfile-based version resolution.

---

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]

PR Review: Add lockfile version resolution for exact version checking

Summary

This PR adds lockfile parsing to resolve exact package versions for more accurate version mismatch detection. The implementation follows the same pattern used in shakacode/shakapacker#170.

Code Quality & Best Practices ✅

Strengths:

• Clean separation of concerns: The resolve_version method clearly encapsulates the fallback logic
• Good documentation: Inline comments explain the lockfile formats and parsing logic
• Consistent with existing patterns: Matches the codebase style and conventions
• Proper use of memoization: The @raw instance variable is correctly memoized in the raw method

Suggestions:

1. Error handling in version_from_package_lock (line 396):

   return parsed["packages"][node_modules_key]["version"] if parsed["packages"][node_modules_key]

   This could raise a NoMethodError if parsed["packages"][node_modules_key] exists but doesn't have a version key. Consider:

   package_data = parsed["packages"][node_modules_key]
   return package_data["version"] if package_data && package_data["version"]

2. Regex escaping consistency: In version_from_yarn_lock (line 362), you correctly use Regexp.escape(package_name), which is good practice for security and correctness.

3. File I/O optimization: The version_from_yarn_lock method reads the lockfile line-by-line, which is efficient for large files. However, version_from_package_lock loads the entire JSON into memory. For very large lockfiles, consider if this could be an issue (though typically it's fine).

Potential Bugs & Issues ⚠️

1. Edge case: Multiple package entries in yarn.lock (line 375):

   break if line.strip.empty? || (line[0] != " " && line[0] != "\t")

   This assumes the block ends with a blank line OR a non-indented line. However, if there are malformed lockfiles or unusual formatting, this might break early. The logic seems sound, but consider adding a test case for a yarn.lock with multiple package entries to ensure it picks the first one correctly.

2. Path construction (lines 227, 231):

   Rails.root.join(ReactOnRails.configuration.node_modules_location, "..", "yarn.lock")

   Using ".." is fragile. If node_modules_location is customized to be at the root or has a complex path, this could break. Consider using File.dirname or documenting the assumption that node_modules_location is always inside a parent directory containing the lockfiles:

   lockfile_dir = File.dirname(ReactOnRails.configuration.node_modules_location)
   Rails.root.join(lockfile_dir, "yarn.lock")

   Or even better, make the path resolution more explicit:

   File.join(Rails.root, ReactOnRails.configuration.node_modules_location, "..", "yarn.lock")

3. Missing fixture: The test references fixtures/semver_caret_package.json (line 433) but I only see it in the yarn.lock and package-lock.json tests. Ensure this file exists or the tests will fail.

Performance Considerations 🚀

Positive aspects:

• Lazy evaluation: Lockfile parsing only happens when raw is called, and results are memoized
• Early returns: The resolve_version method returns as soon as it finds a version, avoiding unnecessary I/O
• Line-by-line parsing for yarn.lock: Efficient for large files, stops reading as soon as the version is found

Potential improvements:

• Caching lockfile data: If NodePackageVersion#raw is called multiple times across different instances (e.g., for different packages), each instance will re-read the lockfiles. Consider caching the parsed lockfile data at the class level if this becomes a bottleneck.

Security Concerns 🔒

Overall: Low risk

1. File path traversal: The lockfile paths are constructed from configuration and don't include user input, so path traversal isn't a concern.

2. JSON parsing: Uses Ruby's built-in JSON parser which is safe. The rescue JSON::ParserError properly handles malformed files.

3. Regex injection: Properly using Regexp.escape(package_name) prevents regex injection (line 362) ✅

4. File existence checks: All file operations check for existence before reading, preventing errors ✅

Test Coverage 📊

Excellent coverage! The tests cover:

• ✅ Yarn.lock resolution (standard package)
• ✅ Package-lock.json resolution (standard package)
• ✅ Yarn.lock resolution (Pro package)
• ✅ Package-lock.json resolution (Pro package)
• ✅ Lockfile preference (yarn.lock over package-lock.json)
• ✅ Fallback to package.json when no lockfile exists
• ✅ Exact version resolution

Suggested additional tests:

1. Malformed lockfiles: Test behavior when lockfiles are corrupt or have invalid JSON
2. Missing version keys: Test when lockfile has the package but missing the version field
3. npm v1 format: Add a test specifically for npm lockfile v1 format (not just v2/v3)
4. Non-existent lockfile paths: Ensure graceful handling when lockfile path doesn't exist (though this might already be covered)
5. Edge case: Multiple entries for the same package in yarn.lock (different version ranges)

Repository Conventions 📋

Per CLAUDE.md:

• ✅ Linting: Ensure you run bundle exec rubocop before pushing (required!)
• ✅ File endings: All files should end with a newline (check the fixture files)
• ⚠️ Formatting: Run rake autofix to ensure Prettier/RuboCop formatting is correct

Before merging:

bundle exec rubocop  # MANDATORY - must pass with zero offenses
rake autofix         # Format all files

Overall Assessment ⭐

Score: 8.5/10

This is a solid PR with clear benefits, good test coverage, and well-structured code. The implementation is straightforward and follows best practices. The main areas for improvement are:

1. Minor edge case handling in path construction and error handling
2. Adding a few more test cases for edge scenarios
3. Ensuring all formatting checks pass before merge

Recommendation: Approve with minor suggestions

The suggested improvements are not blockers but would make the code more robust. Great work! 🎉

---

Review generated with assistance from Claude Code

[claude]

PR Review: Add lockfile version resolution for exact version checking

Overall Assessment

This is a well-implemented PR that improves version checking accuracy by resolving exact versions from lockfiles. The implementation is solid, follows Ruby best practices, and includes comprehensive test coverage.

---

Strengths

1. Excellent Problem Solving: The PR directly addresses the issue of semver ranges causing inaccurate version mismatch detection by using exact resolved versions from lockfiles.

2. Comprehensive Test Coverage: All major scenarios are covered (yarn.lock, package-lock.json, both lockfiles, fallback). Tests for both standard and Pro packages. Edge cases handled (missing lockfiles, preference order).

3. Smart Fallback Chain: The priority order is well thought out: yarn.lock then package-lock.json then package.json

4. Good Code Structure: New methods are well-separated and focused on single responsibilities.

5. Backward Compatibility: Existing tests pass, and the change gracefully falls back when lockfiles do not exist.

---

Potential Issues and Suggestions

1. Missing File Read Error Handling (lib/react_on_rails/version_checker.rb:356-357)

The version_from_yarn_lock method uses File.foreach without error handling. If the file becomes unreadable mid-read, it could raise an exception. Consider wrapping file operations in a rescue block.

2. Regex Pattern Escaping (lib/react_on_rails/version_checker.rb:363)

Good use of Regexp.escape for package names. The pattern handles standard and quoted formats well. Consider testing with a lockfile that has multiple version specifiers to ensure robustness.

3. JSON Parse Error Handling (lib/react_on_rails/version_checker.rb:388-401)

Good error handling for JSON::ParserError. Consider adding optional debug logging when JSON parsing fails to aid troubleshooting.

4. Missing Package Lock V1 Format Test

The test fixtures include v2 format package-lock.json (with both packages and dependencies sections), but there is no test specifically for v1-only format. Add a test fixture with lockfileVersion 1 that only has dependencies to ensure v1 fallback works correctly.

5. RuboCop Compliance

The PR adds rubocop:disable Metrics/CyclomaticComplexity comments. Per CLAUDE.md, before merging verify that bundle exec rubocop passes or that the complexity disables are justified per project standards.

---

Security Considerations

• File Access: Good - Uses File.exist? checks before reading
• Path Traversal: Good - Paths are constructed using Rails.root.join, not user input
• Regex Injection: Good - Uses Regexp.escape for package names
• JSON Parsing: Good - Handles parse errors gracefully

---

Test Coverage Assessment

Excellent coverage overall with tests for standard package with yarn.lock, standard package with package-lock.json, Pro package with both lockfiles, lockfile preference order, fallback to package.json, and exact version matching.

Missing edge cases (nice-to-have): Package-lock.json v1 format only, malformed lockfiles, lockfile with package present but version missing, yarn.lock with multiple version specifiers for same package.

---

Performance Considerations

Impact is minimal - file reads are cached by raw memoization. Adds 1-2 additional file existence checks and potentially 1 file read per version check. Version checking typically happens once at Rails initialization. Acceptable performance impact.

---

Recommendations Summary

Before Merge:

1. Run bundle exec rubocop to ensure compliance
2. Verify all new fixture files end with newline per CLAUDE.md
3. Consider adding error handling for file read operations
4. Add test for package-lock.json v1 format

Future Enhancements (optional):

1. Add debug logging for version resolution path taken
2. Consider caching lockfile parsing results if version checking happens multiple times

---

Final Verdict

APPROVE with minor suggestions

This PR is ready to merge after running the mandatory bundle exec rubocop check. The code quality is high, test coverage is comprehensive, and the implementation follows best practices. The suggestions above are mostly nice-to-haves that would make the code more robust but are not blockers.

Great work on improving version checking accuracy! This will help catch version mismatches more reliably.

[claude]

Code Review: Lockfile Version Resolution

I've reviewed PR #1898 and here's my detailed feedback:

✅ Strengths

1. Code Quality & Best Practices

• Clean implementation following Ruby conventions
• Good use of class methods for path resolution
• Proper RuboCop compliance with explicit disables for complexity metrics
• Backward compatible with optional lockfile parameters
• Clear priority hierarchy: yarn.lock → package-lock.json → package.json

2. Test Coverage

• Excellent comprehensive test suite covering both standard and Pro packages
• Both yarn.lock and package-lock.json formats
• Lockfile preference ordering and fallback scenarios
• Test fixtures are well-structured and realistic

3. Implementation Design

• Smart fallback chain provides robustness
• JSON parsing error handling prevents crashes
• File existence checks avoid unnecessary I/O errors

⚠️ Potential Issues

1. Yarn Lock Parsing - Edge Case Vulnerability

The regex at version_checker.rb:365 could match unintended packages:

• Looking for react-on-rails could match react-on-rails-pro
• Looking for foo could match foo-bar

Recommendation: Make the pattern more strict to ensure exact package name matching.

2. Missing Test Cases

• Malformed lockfile handling
• package-lock.json v1 format (code supports it but no test fixture)
• Scoped packages (e.g., @scope/package-name)

🎯 Overall Assessment

Rating: 8.5/10 - Solid implementation that achieves its goal effectively.

✅ Recommended Actions Before Merge

1. Fix regex pattern in version_from_yarn_lock to prevent prefix matching
2. Add test case for package-lock.json v1 format
3. Run full test suite: rake
4. Run RuboCop: bundle exec rubocop

Great work on this improvement!

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7825688 and 3e889c1.

⛔ Files ignored due to path filters (3)

• spec/react_on_rails/fixtures/pro_semver_caret_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/semver_caret_yarn.lock is excluded by !**/*.lock
• spec/react_on_rails/fixtures/semver_exact_yarn.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• lib/react_on_rails/version_checker.rb (4 hunks)
• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_caret_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_exact_package-lock.json (1 hunks)
• spec/react_on_rails/fixtures/semver_exact_package.json (1 hunks)
• spec/react_on_rails/version_checker_spec.rb (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Prettier is the sole authority for formatting all non-Ruby files; never manually format them

Files:

• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json
• spec/react_on_rails/fixtures/semver_exact_package.json
• spec/react_on_rails/fixtures/semver_caret_package-lock.json
• spec/react_on_rails/fixtures/semver_exact_package-lock.json

{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}

📄 CodeRabbit inference engine (CLAUDE.md)

{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files

Files:

• spec/react_on_rails/version_checker_spec.rb
• lib/react_on_rails/version_checker.rb

🧠 Learnings (12)

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

• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json
• spec/react_on_rails/version_checker_spec.rb
• spec/react_on_rails/fixtures/semver_exact_package.json
• spec/react_on_rails/fixtures/semver_caret_package-lock.json
• lib/react_on_rails/version_checker.rb
• spec/react_on_rails/fixtures/semver_exact_package-lock.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:

• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json
• spec/react_on_rails/fixtures/semver_exact_package.json
• spec/react_on_rails/fixtures/semver_caret_package-lock.json
• lib/react_on_rails/version_checker.rb
• spec/react_on_rails/fixtures/semver_exact_package-lock.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:

• spec/react_on_rails/fixtures/pro_semver_caret_package-lock.json
• spec/react_on_rails/fixtures/semver_exact_package.json
• spec/react_on_rails/fixtures/semver_caret_package-lock.json
• lib/react_on_rails/version_checker.rb
• spec/react_on_rails/fixtures/semver_exact_package-lock.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:

• spec/react_on_rails/version_checker_spec.rb
• spec/react_on_rails/fixtures/semver_exact_package.json
• lib/react_on_rails/version_checker.rb

📚 Learning: 2025-09-29T14:45:42.687Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.

Applied to files:

• spec/react_on_rails/version_checker_spec.rb

📚 Learning: 2025-09-16T08:01:11.146Z

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

Applied to files:

• spec/react_on_rails/fixtures/semver_exact_package.json
• spec/react_on_rails/fixtures/semver_caret_package-lock.json
• spec/react_on_rails/fixtures/semver_exact_package-lock.json

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

• spec/react_on_rails/fixtures/semver_exact_package.json

📚 Learning: 2024-10-08T20:53:47.076Z

Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

• spec/react_on_rails/fixtures/semver_exact_package.json

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

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

• spec/react_on_rails/fixtures/semver_exact_package.json

📚 Learning: 2025-02-13T16:50:26.861Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.

Applied to files:

• spec/react_on_rails/fixtures/semver_exact_package.json

🧬 Code graph analysis (2)

spec/react_on_rails/version_checker_spec.rb (1)

lib/react_on_rails/version_checker.rb (1)

• raw (243-271)

lib/react_on_rails/version_checker.rb (1)

lib/react_on_rails/configuration.rb (2)

• configuration (14-57)
• initialize (75-132)

🪛 ast-grep (0.39.6)

lib/react_on_rails/version_checker.rb

[warning] 223-223: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json")
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html

(hardcoded-secret-rsa-passphrase-ruby)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: build
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: lint-js-and-ruby
• GitHub Check: rspec-package-tests (3.2, latest)
• GitHub Check: rspec-package-tests (3.2, minimum)
• GitHub Check: rspec-package-tests (3.4, minimum)
• GitHub Check: rspec-package-tests (3.4, latest)
• GitHub Check: claude-review

🔇 Additional comments (1)

spec/react_on_rails/fixtures/semver_caret_package-lock.json (1)

1-49: Verify Prettier formatting compliance locally.

The fixture file's JSON structure is valid and appears well-formatted (consistent 2-space indentation, proper alignment). However, Prettier is not available in the sandbox environment to verify compliance with coding guidelines. Please run the following locally to ensure the file meets formatting standards:

prettier --check spec/react_on_rails/fixtures/semver_caret_package-lock.json

If formatting issues are detected, run:

prettier --write spec/react_on_rails/fixtures/semver_caret_package-lock.json

[claude]

PR Review: Lockfile Version Resolution

Overall, this is a well-implemented and valuable enhancement that improves version checking accuracy. The code quality is high, tests are comprehensive, and the approach follows established patterns.

---

✅ Strengths

Code Quality

• Clean implementation: The lockfile parsing logic is well-structured and easy to follow
• Follows existing patterns: Mirrors feat: attempt to resolve exact npm package version from lockfiles for constraint checking shakapacker#170 for consistency
• Good separation of concerns: Each parsing method handles one lockfile format
• Proper error handling: Gracefully falls back to package.json on JSON parse errors

Test Coverage

• Comprehensive tests: 7 new test cases covering all major scenarios
• Good fixture data: Realistic lockfile examples for both yarn.lock and package-lock.json
• Edge cases covered: Tests for missing lockfiles, preference ordering, and both package types

Architecture

• Priority ordering is logical: yarn.lock → package-lock.json → package.json
• Supports both packages: Works for react-on-rails and react-on-rails-pro
• Non-breaking change: Falls back gracefully when lockfiles don't exist

---

🔍 Potential Issues

1. Critical: Missing Newline at EOF

Location: lib/react_on_rails/version_checker.rb:448

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

Action: Add newline at end of file before merging.

2. Edge Case: Multiple yarn.lock Entries

Location: lib/react_on_rails/version_checker.rb:358-383

The method returns the FIRST match found. Yarn.lock can have multiple entries for the same package with different specifiers. This is probably fine for well-formed lockfiles, but consider documenting this assumption.

3. Silent Lockfile Parse Failures

Location: lib/react_on_rails/version_checker.rb:387-410

JSON parse errors are silently caught. Consider adding debug logging:

rescue JSON::ParserError => e
  Rails.logger.debug("Failed to parse package-lock.json: #{e.message}")
  nil
end

4. Lockfile Path Assumptions

Location: lib/react_on_rails/version_checker.rb:227-234

The code assumes lockfiles are in the parent directory of node_modules_location. This works for standard configs but consider adding a comment explaining this assumption.

---

📊 Test Coverage

Excellent coverage for core functionality:

• ✅ Yarn.lock parsing (both packages)
• ✅ Package-lock.json parsing (both packages)
• ✅ Lockfile priority handling
• ✅ Fallback to package.json
• ✅ Exact version matching

Minor gaps:

• ❌ Malformed lockfile handling
• ❌ Package-lock.json v1 format

---

🔒 Security & Performance

Security: No concerns. File paths from config, not user input.

Performance: No issues. Lockfile parsing only happens at boot, memoization prevents redundancy.

---

📝 Recommendations

Must Fix Before Merge:

1. Add newline at EOF in lib/react_on_rails/version_checker.rb
2. Run bundle exec rubocop and fix all violations per CLAUDE.md

Nice to Have:

3. Add debug logging for version resolution
4. Document lockfile format assumptions
5. Add comments explaining first-match behavior for yarn.lock

---

🎯 Final Verdict

Status: Approve with minor changes ✨

This PR significantly improves version checking accuracy. The implementation is clean, well-tested, and backward compatible. Once the trailing newline is added and RuboCop passes, this is ready to merge.

Great work on applying the shakapacker pattern here!

---

Review per CLAUDE.md: code quality, best practices, bugs, performance, security, test coverage

[claude]

I have reviewed PR #1898 and provided detailed feedback. Here is my assessment:

Strengths:

• Well-designed implementation with sensible priority order (yarn.lock → package-lock.json → package.json)
• Comprehensive test coverage including both lockfile formats, both packages, and edge cases
• Backward compatibility with graceful fallback
• Proper error handling for JSON parse errors and missing files

Code Quality Issues & Suggestions:

1. Package Name Matching (line 269): The regex pattern could be more explicit to ensure exact package name matching. Consider adding a closing quote in the pattern.

2. Code Duplication (lines 255-266): The version resolution logic is duplicated for both packages. Could be refactored using array iteration.

3. Type Inconsistency (lines 227-239): yarn_lock_path and package_lock_path return strings while package_json_path returns Pathname. Recommend keeping consistency.

4. Missing Edge Case Test: Consider adding a test for when lockfile exists but does not contain the package entry.

Security & Performance:

• No security concerns identified
• Performance impact is minimal (only runs at initialization)

Overall Assessment:
This is a well-implemented PR that significantly improves version checking accuracy. The code is clean, well-tested, and follows Ruby best practices. The suggestions above are minor optimizations rather than critical issues.

Recommendation: Approve with minor suggestions

The implementation correctly handles the complexity of lockfile parsing while maintaining backward compatibility and good error handling.

Generated with Claude Code

[claude]

Code Review - PR #1898: Add lockfile version resolution for exact version checking

I've reviewed this PR and found it to be well-implemented with comprehensive test coverage. Here's my detailed analysis:

---

✅ Strengths

1. Code Quality & Best Practices

• Clean implementation: The lockfile parsing logic is well-structured and follows Ruby conventions
• Proper separation of concerns: Each parsing method handles one format
• Good error handling: JSON parsing errors are caught gracefully with fallback to package.json
• RuboCop compliance: Appropriate use of disable comments for complexity metrics
• Comprehensive documentation: Inline comments explain the parsing patterns and edge cases

2. Test Coverage

• Excellent test coverage with 11 new test scenarios covering:
  • Yarn.lock and package-lock.json parsing (v1, v2, v3 formats)
  • Both standard and Pro packages
  • Malformed lockfiles with proper fallbacks
  • Edge case: Similar package names (react-on-rails vs react-on-rails-pro)
  • Lockfile preference order (yarn.lock → package-lock.json → package.json)

3. Security & Reliability

• No security concerns: Only reads lockfiles, no execution or eval
• Graceful degradation: Always falls back to package.json if lockfiles are missing/malformed
• Pattern matching safety: Uses Regexp.escape to prevent regex injection

---

🔍 Potential Issues & Suggestions

1. Regex Pattern (Minor - Low Priority)

Location: lib/react_on_rails/version_checker.rb:335-336

The pattern /^"?#{Regexp.escape(package_name)}@/ could be more robust with a negative lookahead to ensure no additional word characters follow. However, your test fixtures demonstrate it works correctly with similar package names.

2. Performance (Very Minor)

The version_from_yarn_lock method reads line-by-line which is O(n). This is negligible for 99% of projects and the current approach is simpler and more maintainable than alternatives.

3. Edge Case: Scoped Packages

Have you tested with scoped packages like @shakacode/react-on-rails? The Regexp.escape should handle @ and / correctly, but worth verifying if you plan to support scoped packages.

4. Minor Code Duplication

The yarn_lock_path and package_lock_path methods have similar logic. Could be extracted to a helper method, but current code is clear and duplication is minimal.

---

🎯 Performance Considerations

• File I/O: Acceptable - version checks happen once at startup
• Memory: Yarn.lock uses line-by-line streaming (minimal footprint). Package-lock.json requires full JSON parse but this is a one-time operation
• Memoization: @raw prevents re-parsing ✅

---

📋 CLAUDE.md Compliance

✅ Formatting: RuboCop disable comments appropriately used
✅ Testing: Comprehensive coverage added
✅ Documentation: Well-commented code
✅ Changelog: This is a user-visible improvement - worthy of changelog entry

Note: Verify bundle exec rubocop passes with zero offenses before merging.

---

🔒 Security Assessment

✅ No security concerns identified

• Only reads files, doesn't execute code
• Uses Regexp.escape to prevent injection
• Graceful error handling prevents crashes

---

📊 Overall Assessment

This is a high-quality PR that improves version detection accuracy. The implementation is:

• ✅ Well-tested with comprehensive coverage
• ✅ Properly error-handled with fallbacks
• ✅ Performance-appropriate for the use case
• ✅ Secure and safe
• ✅ Follows project conventions

Recommendation: APPROVE ✅

Minor suggestions above are optional enhancements that don't block merge.

---

📝 Pre-Merge Checklist

• bundle exec rubocop passes with zero offenses
• All tests pass: rake run_rspec
• Consider adding changelog entry per CLAUDE.md guidelines

---

Great work on this enhancement! The lockfile-based version resolution will make version mismatch detection much more reliable.

[claude]

Code Review: Add lockfile version resolution for exact version checking

I've completed a thorough review of PR #1898. Overall, this is a well-implemented feature with excellent test coverage. Here's my detailed feedback:

✅ Strengths

1. Excellent Test Coverage: Comprehensive tests covering:

   • yarn.lock and package-lock.json (v1, v2/v3) parsing
   • Both standard and Pro package variants
   • Malformed lockfile handling
   • Fallback scenarios
   • Edge case with similar package names (react-on-rails vs react-on-rails-pro)

2. Clean Implementation: The code is well-structured with clear separation of concerns:

   • resolve_version for orchestration
   • Separate parsers for each lockfile format
   • Graceful fallbacks at every level

3. Good Documentation: Inline comments explain the regex patterns and lockfile format handling clearly

4. Backward Compatibility: Maintains existing API while adding new functionality

🔍 Code Quality Issues

1. Potential Bug: Lockfile Path Inconsistency (lib/react_on_rails/version_checker.rb:227-238)

The lockfile path methods return a String while package_json_path returns a Pathname:

def self.package_json_path
  Rails.root.join(ReactOnRails.configuration.node_modules_location, "package.json")
end

def self.yarn_lock_path
  base_dir = ReactOnRails.configuration.node_modules_location.presence || ""
  Rails.root.join(base_dir, "yarn.lock").to_s  # ⚠️ Returns String
end

Issue: Inconsistent return types and potential path joining error when node_modules_location is empty.

Suggested Fix:

def self.yarn_lock_path
  base_dir = ReactOnRails.configuration.node_modules_location.presence || "."
  Rails.root.join(base_dir).join("yarn.lock")
end

This ensures:

• Consistent Pathname return type
• Proper path joining behavior
• . explicitly represents current directory

2. Inefficient File Reading (lib/react_on_rails/version_checker.rb:401)

The version_from_package_lock method reads the entire lockfile into memory:

parsed = JSON.parse(File.read(package_lock))

Performance Concern: Package lockfiles can be very large (10MB+), and this happens on every version check.

Suggested Optimization: Consider adding memoization or streaming for large files, though this may be premature optimization depending on usage frequency.

3. Regex Edge Case in yarn.lock Parser (lib/react_on_rails/version_checker.rb:386)

break if line.strip.empty? || (line[0] != " " && line[0] != "\t")

Issue: Accessing line[0] without checking if line is empty could theoretically cause issues with malformed files, though unlikely.

Suggested Improvement:

break if line.strip.empty? || (!line.empty? && line[0] != " " && line[0] != "\t")

4. Missing Test Case: No test for when lockfile exists but package is missing from it

Test Gap: What happens if:

• yarn.lock exists
• But react-on-rails is not listed in it (e.g., newly added dependency before lockfile update)

Current behavior would fall back to package.json, which is correct, but an explicit test would document this behavior.

🛡️ Security Considerations

No Critical Issues - The implementation safely handles:

• ✅ JSON parsing errors
• ✅ Missing files
• ✅ Malformed lockfiles
• ✅ Regex injection (uses Regexp.escape)

⚡ Performance Considerations

1. File I/O: Multiple File.exist? checks and file reads. Consider caching lockfile existence if version checking happens frequently during request processing.

2. Lockfile Parsing: Both parsers are reasonably efficient:

   • yarn.lock: Line-by-line streaming (good)
   • package-lock.json: Full file read (acceptable for typical sizes)

📋 Style & Best Practices

1. RuboCop Compliance: Code properly uses rubocop:disable comments for legitimate complexity
2. Naming: Clear, descriptive method names
3. Error Handling: Graceful degradation with fallbacks
4. Comments: Good documentation of regex patterns and format handling

🎯 Recommendations

Before Merge:

1. Fix the lockfile path inconsistency (Issue TODO for first version #1 above)
2. Add test case for package missing from lockfile
3. Consider adding a comment explaining why lockfiles are read fresh each time (or add caching if appropriate)

Optional Enhancements:

1. Add logging when falling back from lockfile to package.json (for debugging)
2. Consider extracting lockfile parsing into separate classes for better testability

📝 Minor Suggestions

1. The PR description is excellent and comprehensive
2. All test fixtures have proper trailing newlines ✅
3. Code follows repository conventions from CLAUDE.md ✅

Summary

This is a high-quality PR that significantly improves version detection accuracy. The implementation is solid, test coverage is excellent, and the changes are well-documented. After addressing the path inconsistency issue, this should be ready to merge.

Recommendation: Approve with minor fixes requested

Great work on maintaining backward compatibility while adding this useful feature! 🚀

[claude]

Pull Request Review: Lockfile Version Resolution

Overall, this is a well-implemented and thoroughly tested PR that significantly improves version checking accuracy.

✅ Strengths

1. Excellent Test Coverage - comprehensive tests for yarn.lock and package-lock.json (v1, v2, v3)
2. Robust Error Handling - graceful fallback to package.json when lockfiles are malformed
3. Good Code Organization - clear separation of concerns with dedicated parsing methods
4. Smart Package Name Matching - correctly handles similar package names (react-on-rails vs react-on-rails-pro)

⚠️ Critical Issue

Build Method Signature Mismatch (lib/react_on_rails/version_checker.rb:218)

The build method still uses the old signature but the constructor now expects 3 parameters. This needs to be fixed:

def self.build
  new(package_json_path, yarn_lock_path, package_lock_path)
end

🛡️ Security

All security concerns addressed properly:

• Input validation with Regexp.escape
• Path traversal prevention using Rails.root.join
• JSON parsing errors caught gracefully
• No code execution risks

🚀 Performance

Good performance characteristics:

• Caching with @raw memoization
• Early returns when version found
• Sensible lockfile priority (yarn.lock before package-lock.json)

📝 Suggestions

1. Add debug logging to show which lockfile was used for version resolution
2. Consider extracting YarnLockParser and PackageLockParser classes (not blocking)
3. Document supported lockfile formats in comments

🎯 Final Recommendation

Status: ✅ Approve with Minor Fix Required

Before merging:

1. ⚠️ Fix self.build method to pass all parameters
2. ✅ Run full test suite (rake)
3. ✅ Ensure RuboCop passes (bundle exec rubocop)

Great work on the comprehensive test coverage and clear documentation! 🎉

---

Reviewed by: Claude Code
Review Date: 2025-11-03

[claude]

Code Review - PR #1898

Great work on implementing lockfile version resolution! This is a solid enhancement that improves version checking accuracy. Here's my comprehensive review:

✅ Strengths

1. Excellent Test Coverage

• Comprehensive test suite covering all major scenarios
• Tests for both yarn.lock and package-lock.json (v1, v2, v3)
• Edge cases well covered: malformed lockfiles, missing lockfiles, preference ordering
• Both standard and Pro package variants tested

2. Good Fallback Strategy

• Clear priority: yarn.lock → package-lock.json → package.json
• Graceful degradation when lockfiles are malformed or missing
• Maintains backward compatibility

3. Implementation Quality

• Clean separation of concerns with dedicated parsing methods
• Proper error handling with JSON::ParserError rescue
• Good use of memoization and early returns

🔍 Potential Issues & Suggestions

1. CRITICAL: Missing attr_reader update

The diff shows attr_reader :package_json on line 216, but it's missing the new instance variables. Without adding yarn_lock and package_lock to the attr_reader, the instance variables won't be accessible, causing the lockfile parsing methods to fail silently.

Should be:
attr_reader :package_json, :yarn_lock, :package_lock

2. CRITICAL: .build method needs updating

The NodePackageVersion.build method still only passes package_json_path, but the initialize method now expects three parameters. This will cause the lockfile parameters to always be nil.

Current (line 218-219):
def self.build
new(package_json_path)
end

Should be:
def self.build
new(package_json_path, yarn_lock_path, package_lock_path)
end

3. Edge case: Lockfile path handling

The yarn_lock_path and package_lock_path methods always return a path string, but if node_modules_location is misconfigured, you could get invalid paths. Consider returning nil if the file doesn't exist to make the intent clearer.

4. Comment clarity (lines 227-229)

The comment says "Lockfiles are in the same directory as package.json" but the code actually puts them in the parent directory of node_modules/package.json. The behavior is correct, but the comment could be clearer about this relationship.

5. Regex pattern for scoped packages

The pattern in version_from_yarn_lock uses Regexp.escape which is good, but doesn't explicitly handle scoped packages like @scope/package. Not an issue currently, but worth noting for future package naming.

📋 Code Style & Conventions

Well done:

• Proper RuboCop disable/enable comments for complexity
• Follows project Ruby style guide
• Clear variable naming and method organization

Minor suggestion:
Both lockfile parsing methods are at the cyclomatic complexity threshold. Consider extracting some logic into helper methods to improve readability.

🔒 Security Considerations

• ✅ No user input directly used in file paths
• ✅ JSON parsing properly rescued with fallback
• ✅ No shell command injection risks
• ✅ File existence checks before reading

🎯 Testing Recommendations

Additional test cases to consider:

1. Test when ReactOnRails.configuration.node_modules_location is empty/nil
2. Test with scoped package names
3. Integration test verifying full flow from VersionChecker.build
4. Test similar packages scenario with package-lock.json

✅ Checklist Before Merge

• Fix missing attr_reader declaration
• Update .build method to pass lockfile paths
• Run bundle exec rubocop and ensure zero violations
• Verify all tests pass
• Consider edge case handling suggestions

🎉 Overall Assessment

This is a high-quality PR with excellent test coverage and clean implementation. The two critical issues (attr_reader and .build method) appear to be oversights that need to be fixed before merge. Once corrected, this will be a valuable enhancement to the version checking system.

Recommendation: Request changes for the two critical fixes, then approve.

[claude]

Code Review for PR #1898

Thank you for this PR! This is a solid implementation that brings improved accuracy to version checking. I've reviewed the changes and have the following feedback:

✅ Strengths

1. Well-tested: Comprehensive test coverage including edge cases (malformed lockfiles, v1/v2/v3 package-lock formats, similar package names, fallback scenarios)
2. Backward compatible: Falls back gracefully to package.json when lockfiles are missing or malformed
3. Clear intent: Resolves exact versions from lockfiles instead of semver ranges, improving version mismatch detection
4. Good documentation: Clear comments explaining regex patterns and format differences

🔍 Code Quality Observations

1. Regex Pattern Security (lib/react_on_rails/version_checker.rb:342)

The regex pattern uses Regexp.escape(package_name) which is good, but consider this edge case:

if line.match?(/^"?#{Regexp.escape(package_name)}@/)

The pattern allows optional quotes but doesn't ensure they're balanced. A malformed lockfile with unbalanced quotes could theoretically match incorrectly. However, this is extremely unlikely in practice since yarn generates these files, and the malformed lockfile tests confirm graceful degradation.

Recommendation: Consider adding a test case for unbalanced quotes in yarn.lock to ensure robustness.

2. File I/O Performance (lib/react_on_rails/version_checker.rb:365, 395)

The version_from_yarn_lock method uses File.foreach which is efficient, but version_from_package_lock reads the entire file into memory with File.read(package_lock).

For large monorepos, package-lock.json files can be 10MB+. Consider:

• Using streaming JSON parsing for very large files (though this may be overkill for most use cases)
• Caching the parsed result if raw is called multiple times

Current impact: Likely minimal for most projects. Modern Ruby handles JSON parsing efficiently.

3. Method Complexity (lib/react_on_rails/version_checker.rb:340-390)

Both parsing methods have Metrics/CyclomaticComplexity warnings disabled. While the complexity is justified by handling multiple formats, consider:

# Extract format-specific logic into separate methods
def version_from_package_lock(package_name)
  return nil unless package_lock && File.exist?(package_lock)
  
  begin
    parsed = JSON.parse(File.read(package_lock))
    version_from_packages_format(parsed, package_name) ||
      version_from_dependencies_format(parsed, package_name)
  rescue JSON::ParserError
    nil
  end
end

private

def version_from_packages_format(parsed, package_name)
  parsed.dig("packages", "node_modules/#{package_name}", "version")
end

def version_from_dependencies_format(parsed, package_name)
  parsed.dig("dependencies", package_name, "version")
end

This would:

• Reduce cyclomatic complexity
• Make testing easier (can unit test each format separately)
• Improve readability

4. Error Handling (lib/react_on_rails/version_checker.rb:415)

The JSON::ParserError rescue is good, but silently returns nil. Consider logging for debugging:

rescue JSON::ParserError => e
  Rails.logger.debug("Failed to parse package-lock.json: #{e.message}")
  nil
end

This would help users troubleshoot lockfile issues without breaking functionality.

5. Class Length Warning (lib/react_on_rails/version_checker.rb:215, 460)

The NodePackageVersion class grew from ~150 to ~245 lines. While the ClassLength cop is disabled, consider:

• Extracting lockfile parsing logic into a separate LockfileParser module or class
• This would improve single responsibility and testability

Example structure:

module LockfileParser
  class YarnLock
    def self.parse_version(path, package_name)
      # ... yarn.lock parsing logic
    end
  end
  
  class PackageLock
    def self.parse_version(path, package_name)
      # ... package-lock.json parsing logic
    end
  end
end

🐛 Potential Issues

1. Race Condition Potential (lib/react_on_rails/version_checker.rb:343, 372)

Multiple File.exist? checks followed by File.read/File.foreach have a TOCTOU (time-of-check-time-of-use) window, though the risk is minimal:

if yarn_lock && File.exist?(yarn_lock)  # Check
  lockfile_version = version_from_yarn_lock(package_name)  # Use
end

Recommendation: Use rescue blocks instead:

def version_from_yarn_lock(package_name)
  return nil unless yarn_lock
  
  File.foreach(yarn_lock) do |line|
    # ... parsing logic
  end
rescue Errno::ENOENT
  nil
end

2. Lockfile Location Assumption (lib/react_on_rails/version_checker.rb:227-238)

The code assumes lockfiles are always in the same directory as package.json. While this is standard, some monorepo setups might have:

• Workspace-specific lockfiles in subdirectories
• Lockfiles in parent directories (yarn workspaces)

Current impact: Should work for 99% of projects. Edge cases might need configuration options.

🎯 Best Practices

1. ✅ DRY principle: Good - the resolve_version method avoids duplication
2. ✅ Fail gracefully: Excellent fallback chain (yarn.lock → package-lock.json → package.json)
3. ✅ Test coverage: Comprehensive edge case testing
4. ⚠️ Single Responsibility: NodePackageVersion class handles too many concerns (consider refactoring)

🔒 Security Considerations

1. ✅ Input validation: Regexp.escape prevents regex injection
2. ✅ Error handling: No unhandled exceptions that could crash the app
3. ✅ File access: Only reads files, no writes or command execution
4. ⚠️ Path traversal: Consider validating that lockfile paths don't escape the Rails root (though Rails.root.join should prevent this)

⚡ Performance Considerations

1. Caching: The @raw instance variable caches the result, which is good
2. File I/O: Minimal impact - only reads small portions of lockfiles in most cases
3. Parsing efficiency: Yarn.lock parsing is line-by-line (good), JSON parsing loads full file (acceptable for most cases)

📝 Testing Notes

1. ✅ All major scenarios covered
2. ✅ Edge cases tested (malformed files, missing files, similar names)
3. ✅ Both package formats tested (react-on-rails and react-on-rails-pro)
4. 💡 Consider adding performance tests for large lockfiles (10MB+)

✅ MANDATORY Pre-Merge Checklist

Before merging, ensure:

• bundle exec rubocop runs with zero offenses
• All test files end with newline characters
• Run full test suite: rake

🎉 Recommendation

APPROVE with minor suggestions

This is a well-implemented feature that significantly improves version checking accuracy. The suggestions above are mostly polish and future-proofing rather than critical issues. The code is production-ready as-is, but would benefit from the refactoring suggestions for long-term maintainability.

Great work! 🚀

[justin808]

@claude review this PR.

[claude]

Claude finished @justin808's task —— View job

---

Code Review: PR #1898 - Lockfile Version Resolution

I've completed a comprehensive review of this PR. Overall, this is a well-implemented enhancement that significantly improves version checking accuracy. The code is clean, well-tested, and follows established patterns from shakacode/shakapacker#170.

---

✅ Strengths

1. Excellent Implementation Quality

• Clear fallback chain: yarn.lock → package-lock.json → package.json is logical and well-documented
• Smart edge case handling: Correctly handles local paths/URLs by skipping lockfile resolution (lines 343-345)
• Good separation of concerns: Each parsing method handles one lockfile format clearly
• Proper error handling: JSON parse errors are caught gracefully with fallback to package.json (line 430-432)

2. Comprehensive Test Coverage

The test suite is exemplary with 11 new test contexts covering:

• ✅ Yarn.lock and package-lock.json parsing (v1, v2/v3 formats)
• ✅ Both standard and Pro packages
• ✅ Malformed lockfiles with proper fallbacks
• ✅ Critical edge case: Similar package names (react-on-rails vs react-on-rails-pro)
• ✅ Lockfile preference order validation
• ✅ Fallback scenarios when lockfiles don't exist

3. Security & Robustness

• ✅ Uses Regexp.escape(package_name) to prevent regex injection (line 386)
• ✅ Graceful degradation when lockfiles are missing or malformed
• ✅ No code execution or shell command risks
• ✅ File existence checks before reading (lines 348, 354, 379, 411)

---

🔍 Issues & Suggestions

1. Critical: Regex Pattern Could Match Similar Package Names

Location: lib/react_on_rails/version_checker.rb:386

The regex pattern /^"?#{Regexp.escape(package_name)}@/ could potentially match packages with similar prefixes:

if line.match?(/^"?#{Regexp.escape(package_name)}@/)

Issue: While the test at line 444-453 validates the "similar packages" scenario works correctly, the pattern relies on the @ character being present immediately after the package name. In yarn.lock v1, this is standard, but it's worth noting this assumption.

Analysis: After reviewing the test fixtures, I can confirm this works correctly because:

• The test similar_packages_yarn.lock shows both react-on-rails-pro@^16.1.1: and react-on-rails@^1.2.3:
• The pattern correctly matches only the exact package name due to the @ delimiter
• The test verifies it returns "16.1.1" for the Pro package, not "1.2.3"

Verdict: ✅ This is actually correct. The implementation is safe.

2. Code Quality: Method Complexity

Location: lib/react_on_rails/version_checker.rb:342, 378, 410

Three methods have cyclomatic complexity warnings that are disabled:

• resolve_version (line 342)
• version_from_yarn_lock (line 378)
• version_from_package_lock (line 410)

Suggestion: While the RuboCop disables are justified (the complexity is inherent to handling multiple formats), consider extracting the v1 vs v2/v3 package-lock logic:

def version_from_package_lock(package_name)
  return nil unless package_lock && File.exist?(package_lock)
  
  begin
    parsed = JSON.parse(File.read(package_lock))
    version_from_packages_format(parsed, package_name) ||
      version_from_dependencies_format(parsed, package_name)
  rescue JSON::ParserError
    nil
  end
end

private

def version_from_packages_format(parsed, package_name)
  # v2/v3 format
  parsed.dig("packages", "node_modules/#{package_name}", "version")
end

def version_from_dependencies_format(parsed, package_name)
  # v1 format
  parsed.dig("dependencies", package_name, "version")
end

Priority: Low - not blocking, but would improve maintainability.

3. Minor: Inconsistent Nil Checking

Location: lib/react_on_rails/version_checker.rb:420-421

package_data = parsed["packages"][node_modules_key]
return package_data["version"] if package_data&.key?("version")

Issue: Uses safe navigation (&.) but then directly accesses ["version"] which could theoretically raise if package_data has a truthy "version" key but the value is not hash-like.

Suggestion: More defensive:

return package_data["version"] if package_data.is_a?(Hash) && package_data.key?("version")

Priority: Very Low - edge case is unlikely in real lockfiles.

4. Documentation: Comment Accuracy

Location: lib/react_on_rails/version_checker.rb:227-231

# Lockfiles are in the same directory as package.json
# If node_modules_location is empty, use Rails.root
base_dir = ReactOnRails.configuration.node_modules_location.presence || ""
Rails.root.join(base_dir, "yarn.lock").to_s

Analysis: This is correct! When node_modules_location is empty string, Rails.root.join("", "yarn.lock") correctly resolves to Rails.root/yarn.lock. The comment accurately describes the behavior.

---

🎯 Performance Analysis

Performance Impact: ✅ Minimal and appropriate

1. Caching: The @raw instance variable provides memoization (line 248)
2. Early returns: Methods return as soon as version is found (lines 350, 356)
3. Lazy evaluation: Lockfile parsing only happens when raw is called
4. File I/O efficiency:
   • Yarn.lock uses line-by-line streaming (efficient for large files)
   • Package-lock.json loads full JSON (acceptable for typical lockfile sizes)

Recommendation: ✅ Current performance characteristics are excellent for this use case.

---

📋 CLAUDE.md Compliance Check

Pre-merge Requirements:

1. RuboCop compliance: ⚠️ MUST RUN bundle exec rubocop before merge

   • The code has rubocop:disable comments which are appropriate
   • Must verify zero offenses overall

2. File endings: ⚠️ MUST CHECK all files end with newline character

   • This is a common CI failure point

3. Formatting: Run rake autofix to ensure Prettier/RuboCop formatting

4. Testing: ✅ Comprehensive test coverage added

---

🔒 Security Assessment

Overall Security: ✅ Excellent - No concerns

• ✅ No user input in file paths (all from configuration)
• ✅ Regex injection prevented with Regexp.escape
• ✅ JSON parsing errors handled gracefully
• ✅ No code execution or shell commands
• ✅ File operations are read-only

---

📊 Test Coverage Assessment

Rating: ⭐⭐⭐⭐⭐ Exceptional

Coverage includes:

• All major lockfile formats (yarn v1, npm v1/v2/v3)
• Both package types (standard and Pro)
• Edge cases (malformed files, missing files, similar names)
• Preference ordering validation
• Integration scenarios

Minor Enhancement Opportunities:

1. Could add test for scoped packages (e.g., @shakacode/react-on-rails) - though not currently used
2. Could add test for extremely large lockfiles - though unlikely to be an issue

Verdict: ✅ Test coverage is production-ready as-is.

---

🎯 Final Assessment

Overall Score: 9/10 - Excellent Implementation

Recommendation: ✅ APPROVE (after pre-merge checks)

Before Merging:

MANDATORY (per CLAUDE.md):

1. ✅ Run bundle exec rubocop - MUST pass with zero offenses
2. ✅ Verify all files end with newline character
3. ✅ Run rake autofix to ensure formatting is correct
4. ✅ Run full test suite: rake

OPTIONAL (nice-to-have):

1. Consider extracting package-lock v1/v2 logic into separate methods (low priority)
2. Consider more defensive nil checking on line 420-421 (very low priority)

---

💡 Summary

This PR achieves its goal of improving version checking accuracy by resolving exact versions from lockfiles. The implementation is:

• ✅ Well-architected with clear fallback logic
• ✅ Thoroughly tested with excellent edge case coverage
• ✅ Secure with proper input validation
• ✅ Performant with appropriate caching and lazy evaluation
• ✅ Maintainable with clear comments and structure

The approach follows the proven pattern from shakacode/shakapacker#170, which gives additional confidence in the design.

Great work on this enhancement! 🎉

---