No actionable comments were generated in the recent review. 🎉

Walkthrough

Adds Pathname support and a project_root resolver (prefers Rails.root, then Bundler's Gemfile dir, then Dir.pwd), updates hook path resolution to use project_root and short-circuit blank inputs, and introduces public helpers project_root, hook_script_has_self_guard?, and shakapacker_precompile_hook_value. Tests for project_root added.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I tap each root with twitchy nose and ear,
Rails, Gemfile, or cwd — I sniff them clear.
I guard the hooks and find the script's way,
A tiny hop to tidy up the day. 🥕

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

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

Review

The fix is correct and well-scoped. resolve_hook_script_path previously short-circuited entirely when Rails was not loaded, so the fallback chain in project_root is exactly the right approach.

Minor nits (inline):

• Pathname.new(Rails.root) is redundant — Rails.root is already a Pathname.
• ENV.fetch("BUNDLE_GEMFILE", nil) can be simplified to ENV["BUNDLE_GEMFILE"].

More significant gap — test coverage:
The new project_root method has two fallback branches (BUNDLE_GEMFILE and Dir.pwd) that are not exercised by any spec. The existing tests for the script-file path scenario all stub Rails.root, so only the first branch is implicitly covered. A describe ".project_root" block covering all three branches would prevent silent regressions and document the intended priority order.

One edge case to be aware of:
BUNDLE_GEMFILE can be set to a relative path (e.g. Gemfile with no directory component). expand_path handles that correctly relative to Dir.pwd, so the current code is safe — but it is worth a comment since the behaviour is non-obvious.

Overall the approach is solid and the dependency on the companion shakapacker PR is clearly documented.

Greptile Summary

This PR fixes resolve_hook_script_path to work in non-Rails contexts (e.g., bin/dev) by extracting a new project_root helper that falls back from Rails.root → BUNDLE_GEMFILE dirname → Dir.pwd. The approach is sound and solves the described bin/dev hook-detection problem, but there is one logic-level regression introduced by unnecessarily wrapping Rails.root in Pathname.new().

Key changes:

• New project_root method with a three-tier fallback for resolving the project directory without requiring Rails.
• resolve_hook_script_path now uses project_root instead of bailing out early when Rails is absent.
• require "pathname" added to make Pathname explicit.

Issues found:

• Pathname.new(Rails.root) on line 238 is redundant (Rails.root is already a Pathname) and will break existing specs that mock Rails.root as an instance_double(Pathname) — calling Pathname.new on that double triggers an unstubbed to_path call, raising RSpec::Mocks::MockExpectationError in the test suite.
• No automated tests cover the new BUNDLE_GEMFILE or Dir.pwd fallback branches of project_root.

Confidence Score: 3/5

• The runtime fix is correct and minimal, but Pathname.new(Rails.root) will cause existing spec failures and the new fallback logic is untested.
• The overall design is correct and the fallback chain is sensible. The score is reduced because the Pathname.new(Rails.root) wrapper is expected to break existing RSpec tests (the double's to_path method is called but never stubbed), and no new tests cover the critical BUNDLE_GEMFILE/Dir.pwd code paths that are the primary motivation of the PR.
• react_on_rails/lib/react_on_rails/packer_utils.rb — specifically the project_root method and its interaction with the existing test doubles in packer_utils_spec.rb.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["resolve_hook_script_path(hook_value)"] --> B{hook_value blank?}
    B -- yes --> C[return nil]
    B -- no --> D["project_root.join(hook_value.strip)"]
    D --> E["project_root()"]
    E --> F{Rails defined\n& Rails.root truthy?}
    F -- yes --> G["Pathname.new(Rails.root) ⚠️\n(redundant wrap — Rails.root\nalready returns a Pathname)"]
    F -- no --> H{BUNDLE_GEMFILE\nenv var set & non-blank?}
    H -- yes --> I["Pathname.new(BUNDLE_GEMFILE).expand_path"]
    I --> J{File exists?}
    J -- yes --> K[return gemfile_path.dirname]
    J -- no --> L["Pathname.new(Dir.pwd)"]
    H -- no --> L
    G --> M[potential_path]
    K --> M
    L --> M
    M --> N{potential_path.file?}
    N -- yes --> O[return potential_path]
    N -- no --> C

_{Last reviewed commit: f9f0115}

Review Summary

The fix is conceptually correct and well-motivated — using BUNDLE_GEMFILE as a fallback for Rails.root is a sound approach since Bundler guarantees it is set after require "bundler/setup". The project_root implementation logic is clean. A few issues need attention before merging:

Critical: Test stubs do not match the implementation

All three new tests under "when Rails is not defined" stub ENV.[] via allow(ENV).to receive(:[]), but the implementation calls ENV.fetch("BUNDLE_GEMFILE", nil). In MRI Ruby, ENV.fetch and ENV.[] are separate methods — RSpec stubs on [] do not intercept fetch. In a real CI run, BUNDLE_GEMFILE is always set by Bundler, so:

• The "valid file" test will use the real Gemfile path instead of __FILE__, producing a wrong expected_dir.
• The "not set" and "non-existent file" tests will find the real, valid BUNDLE_GEMFILE, so project_root returns the Gemfile directory instead of Dir.pwd — both assertions fail.

The simplest fix is to change the implementation to use ENV["BUNDLE_GEMFILE"] (functionally identical; nil default is the same), which matches the stub style already used throughout the spec. Alternatively, change all three stubs to allow(ENV).to receive(:fetch).with("BUNDLE_GEMFILE", nil). See inline comments for suggested diffs.

Missing higher-level test coverage

The new specs test the project_root helper directly, but there is no spec for resolve_hook_script_path exercising the non-Rails path. A test that verifies "resolves a hook path relative to BUNDLE_GEMFILE dirname when Rails is not loaded" would give more confidence that the actual public-facing fix works end-to-end and would survive a future refactor of project_root.

project_root visibility

project_root is only called internally by resolve_hook_script_path. Leaving it as a public class method makes it an accidental part of the module's API surface. Consider private_class_method :project_root to signal it is an internal helper and to hint that behaviour should be tested through resolve_hook_script_path, not the helper directly.

PR Review: Fix resolve_hook_script_path to work without Rails.root

Overall this is a well-motivated and correctly-scoped fix. The fallback chain (Rails.root → BUNDLE_GEMFILE → Dir.pwd) is a sound approach. A few things worth addressing:

1. Missing test: Rails.root returns nil/falsy

The project_root guard handles the case where Rails.root is nil (e.g., during engine initialization before Rails is fully booted). However, there is no spec for this — the test suite only covers Rails being fully defined with a valid root, or Rails being absent entirely. A spec where Rails.root returns nil would prevent regressions.

2. resolve_hook_script_path behavior change is undocumented

The old comment (Hook value might be a script path relative to Rails root) has been removed without replacement. A short note explaining the project_root fallback chain would help future readers understand why Rails is no longer required.

3. project_root visibility

project_root is a module-level self. method and becomes part of the public API. If it is only intended as an internal helper, consider private_class_method :project_root to prevent unintended external use.

Positive notes

• The fallback chain is correct and well-ordered.
• Tests cover the three main branches of project_root.
• expand_path on BUNDLE_GEMFILE is the right defensive move.
• Keeping the change minimal (no YAML re-implementation) is the right call given the companion Shakapacker fix.

Review

The approach is sound — removing the hard Rails.root dependency from resolve_hook_script_path via a dedicated project_root helper with a sensible fallback chain is the right fix. The companion Shakapacker PR makes this complete.

Minor issues:

1. project_root visibility (see inline): It's an internal helper and could be marked private_class_method to avoid leaking it into the public API surface.

2. Dir.pwd fallback is silent (see inline): If the fallback fires with the wrong directory, resolve_hook_script_path silently returns nil and the hook is treated as unconfigured. A warning log would make this much easier to diagnose.

3. Missing integration test for the core fix (see inline): project_root is well-tested in isolation, but there's no spec for resolve_hook_script_path in a non-Rails context — which is the exact scenario this PR is designed to fix. Adding that test would ensure the fix stays covered under refactoring.

Otherwise the implementation is clean, the commit history tells a clear story, and the test structure for project_root is thorough.