Fix first-start bin/dev precompile hook detection for RSC demo#2561
Fix first-start bin/dev precompile hook detection for RSC demo#2561
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTry the Shakapacker runtime config API for the precompile hook during early startup; if unavailable, parse Changes
Sequence DiagramsequenceDiagram
participant BinDev as "bin/dev"
participant PackerUtils as "PackerUtils.extract_precompile_hook"
participant ShakaAPI as "Shakapacker Config API"
participant YAMLFile as "config/shakapacker.yml (ERB + YAML)"
participant EnvFS as "Env / Project Root"
BinDev->>PackerUtils: request precompile hook
PackerUtils->>ShakaAPI: attempt runtime config API
ShakaAPI-->>PackerUtils: returns config or nil/raises
alt API unavailable or returns nil
PackerUtils->>YAMLFile: read & parse (ERB -> YAML), lookup env/default
YAMLFile->>EnvFS: request project root / env context
EnvFS-->>YAMLFile: provide paths / env
YAMLFile-->>PackerUtils: precompile_hook value (if any)
end
PackerUtils->>EnvFS: resolve hook script path (strip interpreter, resolve relative to project root)
EnvFS-->>PackerUtils: resolved script path or nil
PackerUtils-->>BinDev: return resolved hook path or nil
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a first-startup failure in the RSC demo where Key changes:
Issues found:
Confidence Score: 3/5
Last reviewed commit: 884f63b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def self.current_shakapacker_environment | ||
| return Rails.env.to_s if defined?(Rails) && Rails.respond_to?(:env) && !Rails.env.to_s.strip.empty? |
There was a problem hiding this comment.
Rails.env is called twice in the condition without local rescue. If Rails.env itself raises during partial Rails initialization, the exception propagates to the rescue StandardError in extract_precompile_hook_from_yaml, which returns nil and silently discards the entire YAML fallback — defeating the purpose of this fallback strategy.
Consider capturing the value once and rescuing locally:
| def self.current_shakapacker_environment | |
| return Rails.env.to_s if defined?(Rails) && Rails.respond_to?(:env) && !Rails.env.to_s.strip.empty? | |
| if defined?(Rails) && Rails.respond_to?(:env) | |
| env_str = begin | |
| Rails.env.to_s | |
| rescue StandardError | |
| nil | |
| end | |
| return env_str unless env_str.to_s.strip.empty? | |
| end |
| # Try symbol keys first (Shakapacker's internal format), then fall back to string keys | ||
| config_data&.[](:precompile_hook) || config_data&.[]("precompile_hook") |
There was a problem hiding this comment.
The || operator silently drops an explicit false hook value. If Shakapacker's config has precompile_hook: false (explicitly disabled), config_data&.[](:precompile_hook) returns false (falsy), so the || evaluates the right side and returns nil instead of false. This causes an unintended fallback to the YAML path.
The newly added extract_precompile_hook_from_yaml already uses the correct key?-based pattern. Apply the same approach here:
| # Try symbol keys first (Shakapacker's internal format), then fall back to string keys | |
| config_data&.[](:precompile_hook) || config_data&.[]("precompile_hook") | |
| # Try symbol keys first (Shakapacker's internal format), then fall back to string keys | |
| if config_data&.key?(:precompile_hook) | |
| config_data[:precompile_hook] | |
| elsif config_data&.key?("precompile_hook") | |
| config_data["precompile_hook"] | |
| end |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
268-333: Keep the new fallback helpers private.These methods look implementation-only, but
def self...makes them part ofReactOnRails::PackerUtils’ public surface. Marking them private will make future cleanup/refactors much safer.♻️ Proposed change
def self.project_root if defined?(Rails) && Rails.respond_to?(:root) && Rails.root root = Rails.root return root if root.respond_to?(:join) return Pathname.new(root.to_s) end bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) if bundle_gemfile && !bundle_gemfile.strip.empty? gemfile_path = Pathname.new(bundle_gemfile).expand_path return gemfile_path.dirname if gemfile_path.file? end Pathname.new(Dir.pwd) end + + private_class_method :extract_precompile_hook_from_shakapacker_config, + :extract_precompile_hook_from_yaml, + :extract_hash_for_environment, + :current_shakapacker_environment, + :project_root end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/packer_utils.rb` around lines 268 - 333, These helper methods (extract_precompile_hook_from_shakapacker_config, extract_precompile_hook_from_yaml, extract_hash_for_environment, current_shakapacker_environment, project_root) are implementation-only and should not be public; make them private class methods by either grouping them inside a class << self; private; ... end block or by calling private_class_method for each method name after their definitions so they remain usable internally but are not exposed on ReactOnRails::PackerUtils' public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 35: Update the CHANGELOG entry that starts "bin/dev precompile hook
detection before Rails boot" to include the PR and author attribution in the
standard format: append a sentence like "[PR
<PR_NUMBER>](https://github.com/shakacode/react_on_rails/pull/<PR_NUMBER>) by
[<USERNAME>](https://github.com/<USERNAME>)" (keep the existing issue link if
desired); ensure the attribution uses the exact bracketed PR format without a
hash and the author's GitHub username link.
---
Nitpick comments:
In `@react_on_rails/lib/react_on_rails/packer_utils.rb`:
- Around line 268-333: These helper methods
(extract_precompile_hook_from_shakapacker_config,
extract_precompile_hook_from_yaml, extract_hash_for_environment,
current_shakapacker_environment, project_root) are implementation-only and
should not be public; make them private class methods by either grouping them
inside a class << self; private; ... end block or by calling
private_class_method for each method name after their definitions so they remain
usable internally but are not exposed on ReactOnRails::PackerUtils' public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc7b9466-8a64-4bc8-a8a6-a20aaaf0e460
📒 Files selected for processing (3)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| return nil unless config_path.file? | ||
|
|
||
| yaml_content = ERB.new(File.read(config_path)).result | ||
| config_data = YAML.safe_load(yaml_content, aliases: true) || {} |
There was a problem hiding this comment.
YAML parsing missing permitted_classes inconsistent with codebase
Low Severity
The YAML.safe_load call in extract_precompile_hook_from_yaml omits permitted_classes: [Symbol], which the existing parse_shakapacker_yml in generator_helper.rb explicitly includes to handle symbol keys in shakapacker.yml. If a config file contains symbol keys, Psych::DisallowedClass is raised and silently swallowed by the rescue, causing the YAML fallback — the primary detection path in the pre-Rails scenario this PR targets — to return nil and miss the hook entirely.
PR Review: Fix first-start bin/dev precompile hook detectionOverall this is a well-motivated fix with good test coverage. The fallback chain (Shakapacker runtime config to YAML parse) is sound for the stated use-case. A few issues worth addressing: Bug: nil return conflates no-hook-configured with config-load-error extract_precompile_hook_from_shakapacker_config returns nil in two distinct situations: (1) Shakapacker loaded successfully but precompile_hook is not set, and (2) Shakapacker raised an exception rescued to nil. Both cases are indistinguishable, so extract_precompile_hook falls through to YAML parsing even when Shakapacker authoritatively said there is no hook. Low-risk today since YAML and runtime config should agree, but semantically incorrect. A sentinel value (HOOK_LOAD_ERROR = Object.new.freeze, returned from the rescue block) would let the caller distinguish the two cases cleanly. Security note: ERB execution uses the full current binding ERB.new(File.read(config_path)).result is consistent with Shakapacker itself, so acceptable for a trusted config file. Passing binding explicitly (.result(binding)) is a minor defensive improvement that avoids exposing unintended local variables to the template. Minor: CHANGELOG entry is missing the PR number Every other entry in the same section includes both an issue link and a PR link. Please add the PR reference once the number is confirmed. Minor: Rails.env invoked twice in current_shakapacker_environment Rails.env is called twice in the condition guard (once for the empty check, once for the return value). A local variable assignment avoids the double dispatch and is clearer. The regression specs are well-structured and cover the key new paths (pre-Rails YAML fallback, generate_packs_if_needed pattern). The project_root fallback chain (Rails.root to BUNDLE_GEMFILE dirname to Dir.pwd) is pragmatic and correct. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # In bin/dev startup before Rails boots, this can raise (e.g., missing Rails.env), | ||
| # so we rescue and fall back to parsing config/shakapacker.yml directly. | ||
| hook_value = extract_precompile_hook_from_shakapacker_config | ||
| return hook_value unless hook_value.nil? |
There was a problem hiding this comment.
The nil from extract_precompile_hook_from_shakapacker_config is ambiguous here: it means either "Shakapacker config loaded successfully and there is no precompile_hook" or "the config raised an exception and was rescued to nil". Both paths return nil, so the YAML fallback fires even in the success-with-no-hook case.
Consider a sentinel to distinguish the two:
| return hook_value unless hook_value.nil? | |
| return hook_value unless hook_value.nil? |
Could become — using a module-level sentinel constant (HOOK_LOAD_ERROR = Object.new.freeze) and returning it from the rescue block in extract_precompile_hook_from_shakapacker_config:
result = extract_precompile_hook_from_shakapacker_config
return result unless result.equal?(HOOK_LOAD_ERROR) # success path (nil or value)
extract_precompile_hook_from_yaml # only here on config load errorThis keeps the semantics clear: YAML is a fallback for load failures, not a second source of truth layered on top of a working Shakapacker config.
| config_path = project_root.join(SHAKAPACKER_CONFIG_PATH) | ||
| return nil unless config_path.file? | ||
|
|
||
| yaml_content = ERB.new(File.read(config_path)).result |
There was a problem hiding this comment.
Calling .result without an argument evaluates the ERB template with the current method's local binding, exposing any local variables in scope to the template. This is consistent with how Shakapacker evaluates the same file, so it is acceptable for a trusted config. Passing binding explicitly is a minor hardening step:
| yaml_content = ERB.new(File.read(config_path)).result | |
| yaml_content = ERB.new(File.read(config_path)).result(binding) |
| end | ||
|
|
||
| def self.current_shakapacker_environment | ||
| return Rails.env.to_s if defined?(Rails) && Rails.respond_to?(:env) && !Rails.env.to_s.strip.empty? |
There was a problem hiding this comment.
Rails.env is dispatched twice — once in the guard and once for the return value. A local variable avoids the double call and makes the intent clearer:
| return Rails.env.to_s if defined?(Rails) && Rails.respond_to?(:env) && !Rails.env.to_s.strip.empty? | |
| if defined?(Rails) && Rails.respond_to?(:env) | |
| env = Rails.env.to_s | |
| return env unless env.strip.empty? | |
| end |
resolve_hook_script_path required Rails.root to resolve hook script paths, returning nil in bin/dev context where Rails isn't loaded. This caused hook_configured? to return false even when the hook was properly configured, leading to redundant pack generation after the hook already ran. Add project_root helper that falls back to BUNDLE_GEMFILE dirname (set by Bundler during require "bundler/setup") or Dir.pwd when Rails.root is unavailable. Companion to shakacode/shakapacker#963 which fixes Shakapacker::Env to work without Rails. Together these two fixes fully resolve #2438 without the YAML-parsing workaround in #2561. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@justin808 This PR is superseded by shakacode/shakapacker#963 and #2568. The YAML-parsing fallback here is a workaround for a bug that lives in Shakapacker itself. Same class of bug that PR #681 fixed for The fix:
Together they replace the YAML-parsing workaround. |
## Summary - Adds `project_root` helper that resolves the project root without requiring `Rails.root` - Falls back to `BUNDLE_GEMFILE` dirname (reliable — always set after `require "bundler/setup"`) then `Dir.pwd` - `resolve_hook_script_path` now uses `project_root` instead of hard-requiring `Rails.root` Related to #2438 — addresses the hook detection root cause (hook not running from `bin/dev` context), though the exact symptom reported in that issue (pack generation failure) could not be reproduced. ## Problem `resolve_hook_script_path` returned nil whenever `Rails.root` wasn't available (e.g., from `bin/dev` which intentionally skips loading Rails for startup speed). This caused `hook_configured?` to return false even when the precompile hook was correctly configured in `shakapacker.yml`, leading to redundant pack generation after the hook already ran. ## Companion PR This works together with [shakacode/shakapacker#963](shakacode/shakapacker#963), which fixes `Shakapacker::Env#current` to resolve the environment from `RAILS_ENV`/`RACK_ENV` env vars when Rails isn't loaded. Together, these two fixes address the `bin/dev` hook detection issue. With both fixes applied, `bin/dev` output shows the correct flow: ``` 🔧 Running Shakapacker precompile hook... Command: bin/shakapacker-precompile-hook ✅ Precompile hook completed successfully ⏭️ Skipping pack generation (handled by shakapacker precompile hook: bin/shakapacker-precompile-hook) ``` ## Supersedes #2561 PR #2561 worked around the Shakapacker `Env#current` bug by re-implementing YAML config parsing. With the Shakapacker fix in place, that workaround is no longer needed — this PR provides only the minimal `resolve_hook_script_path` fix that was still necessary. ## Test plan - [x] Verified `hook_configured?` returns `true` from non-Rails context (test app with both fixes) - [x] Verified `bin/dev` runs hook and skips redundant pack generation - [x] Verified `BUNDLE_GEMFILE` correctly resolves project root even when `Dir.pwd` differs 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * More robust project-root detection: falls back from Rails to Gemfile directory to current working directory for clearer path resolution across environments. * Improved hook-script detection and reporting, with better handling of self-guard patterns. * **Tests** * Added tests covering project-root detection with Rails present/absent and various Gemfile scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
213-249:⚠️ Potential issue | 🟠 MajorInterpreter-prefixed hook commands still bypass script inspection.
This only reads the hook script when
hook_valueis a bare path. Configs likeruby bin/shakapacker-precompile-hookorbash ./bin/shakapacker-precompile-hookwon't matchGENERATE_PACKS_PATTERN, andresolve_hook_script_paththen treats the whole command as a filename, sogenerate_packs_if_neededis still missed in that common script form.Possible direction
+require "shellwords" ... - script_path = resolve_hook_script_path(hook_value) + script_path = resolve_hook_script_path(hook_value) + if script_path.nil? + argv = Shellwords.split(hook_value.to_s) + hook_arg = argv[1] if %w[ruby bash sh].include?(File.basename(argv.first.to_s)) + script_path = resolve_hook_script_path(hook_arg) if hook_arg + end return false unless script_path && File.exist?(script_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/packer_utils.rb` around lines 213 - 249, The hook detection currently only matches GENERATE_PACKS_PATTERN against the raw hook_value or treats the whole string as a filepath; update hook_contains_generate_packs? to first split interpreter-prefixed commands (e.g., "ruby bin/..." or "bash ./bin/...") and attempt inspection of the actual script file referenced: use a shell-safe split (Shellwords.split or similar) to isolate candidate path tokens (typically the last token or any token containing a "/" or starting with "."), then call resolve_hook_script_path with that token and read/scan the file for GENERATE_PACKS_PATTERN if it exists; keep existing fallback behavior and rescue behavior, and adjust resolve_hook_script_path only if needed to accept Pathname/file? checks for the extracted token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@react_on_rails/lib/react_on_rails/packer_utils.rb`:
- Around line 213-249: The hook detection currently only matches
GENERATE_PACKS_PATTERN against the raw hook_value or treats the whole string as
a filepath; update hook_contains_generate_packs? to first split
interpreter-prefixed commands (e.g., "ruby bin/..." or "bash ./bin/...") and
attempt inspection of the actual script file referenced: use a shell-safe split
(Shellwords.split or similar) to isolate candidate path tokens (typically the
last token or any token containing a "/" or starting with "."), then call
resolve_hook_script_path with that token and read/scan the file for
GENERATE_PACKS_PATTERN if it exists; keep existing fallback behavior and rescue
behavior, and adjust resolve_hook_script_path only if needed to accept
Pathname/file? checks for the extracted token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b3b437b-6628-4b7c-84d0-8bcc4d60064f
📒 Files selected for processing (2)
react_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
justin808
force-pushed
the
jg-codex/fix-bin-dev-precompile-hook-2438
branch
from
3dfa73d to
4152a22
Compare
PR Review: Fix first-start bin/dev precompile hook detectionOverall this is a well-structured fix with good test coverage. The HOOK_LOOKUP_FAILED sentinel cleanly distinguishes an exception from a legitimate nil return, and the fallback-to-YAML strategy is sound. A few issues worth addressing: Bug: hook_script_has_self_guard? not updated to match hook_contains_generate_packs? hook_contains_generate_packs? was updated to also resolve script paths from interpreter-prefixed commands by adding: script_path ||= resolve_hook_script_path(extract_hook_script_argument_from_command(hook_value)). But hook_script_has_self_guard? was NOT updated symmetrically. If a user configures precompile_hook: 'ruby bin/shakapacker-precompile-hook', hook_contains_generate_packs? will correctly detect the hook, but hook_script_has_self_guard? will silently return false. This asymmetry could cause incorrect duplicate-skip behavior. Minor: Redundant File.exist? check in hook_contains_generate_packs? resolve_hook_script_path already validates existence via .file? before returning, so the subsequent File.exist?(script_path) guard in hook_contains_generate_packs? is dead code. Not a bug, but misleading to readers. Minor: ERB binding scope in extract_precompile_hook_from_yaml ERB.new(File.read(config_path)).result(binding) passes the module method binding. Fine for a trusted local config file, but an explicit clean binding would be more hygienic. Missing test coverage for current_shakapacker_environment The new spec only covers the 'Rails.env raises' path. The RACK_ENV fallback and the hardcoded 'development' default are untested. Nitpick: broad rescue in extract_precompile_hook_from_yaml An ERB syntax error in shakapacker.yml would be silently swallowed. Consider emitting a warn under ENV['DEBUG'] to aid debugging. |
| # Check if it's a script file path | ||
| script_path = resolve_hook_script_path(hook_value) | ||
| script_path ||= resolve_hook_script_path(extract_hook_script_argument_from_command(hook_value)) | ||
| return false unless script_path && File.exist?(script_path) |
There was a problem hiding this comment.
Redundant existence check
resolve_hook_script_path only returns a non-nil value when potential_path.file? (or cwd_path.file?) is true, so File.exist?(script_path) is always true here. The guard is dead code and may mislead future readers into thinking that resolve_hook_script_path can return a path to a non-existent file.
| return false unless script_path && File.exist?(script_path) | |
| return false unless script_path |
|
|
||
| # Check if it's a script file path | ||
| script_path = resolve_hook_script_path(hook_value) | ||
| script_path ||= resolve_hook_script_path(extract_hook_script_argument_from_command(hook_value)) |
There was a problem hiding this comment.
This correctly adds interpreter-prefixed command support to hook_contains_generate_packs?, but hook_script_has_self_guard? (below, line ~258) was not updated with the same pattern. A hook like "ruby bin/shakapacker-precompile-hook" will be detected here but the self-guard check will silently return false for the same value, causing an asymmetry in how the two methods interpret the same hook configuration.
| config_path = project_root.join(SHAKAPACKER_CONFIG_PATH) | ||
| return nil unless config_path.file? | ||
|
|
||
| yaml_content = ERB.new(File.read(config_path)).result(binding) |
There was a problem hiding this comment.
ERB.new(...).result(binding) passes the calling method's binding. This works fine for a trusted local config file and is consistent with how Shakapacker itself evaluates its config. That said, if this binding were ever to expose internal module state to a malicious or malformed config file, a clean binding (ERB.new(...).result) would be safer. Low risk in practice, but worth a note.
justin808
force-pushed
the
jg-codex/fix-bin-dev-precompile-hook-2438
branch
from
4152a22 to
de5c746
Compare
Review SummaryThe fix is well-scoped and correctly addresses the pre-Rails startup problem. The new YAML fallback path is solid and the regression specs cover the key scenarios well. A few issues worth addressing: Issues1.
2. Nil-fallback ambiguity in hook_value = extract_precompile_hook_from_shakapacker_config
return hook_value unless hook_value.nil?
extract_precompile_hook_from_yamlIf Shakapacker loads fine but has no 3. Test ENV mutation is fragile Both new specs mutate 4. Minor: redundant existence check in Line 224 calls both 5. Minor: overly defensive guard in
The core fix logic is correct and the specs are thorough. The nil-fallback ambiguity (#2) is the most architecturally interesting concern - everything else is minor polish. |
| config_path = project_root.join(SHAKAPACKER_CONFIG_PATH) | ||
| return nil unless config_path.file? | ||
|
|
||
| yaml_content = ERB.new(File.read(config_path)).result(binding) |
There was a problem hiding this comment.
Passing binding here exposes local variables from extract_precompile_hook_from_yaml (e.g., config_path, yaml_content) to the ERB template. Since shakapacker.yml is a trusted project file this is low risk, but the idiom used elsewhere (including Shakapacker's own implementation) is to pass no argument or use a fresh Object.new.instance_eval { binding }:
| yaml_content = ERB.new(File.read(config_path)).result(binding) | |
| yaml_content = ERB.new(File.read(config_path)).result |
ERB#result with no argument defaults to TOPLEVEL_BINDING, which avoids leaking method locals into the template context.
| # In bin/dev startup before Rails boots, this can raise (e.g., missing Rails.env), | ||
| # so we rescue and fall back to parsing config/shakapacker.yml directly. | ||
| hook_value = extract_precompile_hook_from_shakapacker_config | ||
| return hook_value unless hook_value.nil? |
There was a problem hiding this comment.
The nil-check here conflates two distinct nil cases:
- Shakapacker config raised (pre-Rails) → returns nil → correctly falls back to YAML ✓
- Shakapacker loaded fine, but
precompile_hookkey is simply not set → also returns nil → falls through to YAML unnecessarily
In case 2 the YAML parse produces the same nil, so it is functionally correct, but it means every call where the hook is unconfigured will redundantly parse and ERB-evaluate the YAML file.
Consider tracking whether the runtime call succeeded and only falling back on failure:
def self.extract_precompile_hook
hook_value, loaded = extract_precompile_hook_from_shakapacker_config
return hook_value if loaded
extract_precompile_hook_from_yaml
endThen extract_precompile_hook_from_shakapacker_config returns [value, true] on success and [nil, false] on rescue. This makes the intent explicit and avoids the redundant YAML parse.
| <<: *default | ||
| YAML | ||
|
|
||
| old_bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) |
There was a problem hiding this comment.
Direct ENV[]= mutation is fragile — if an exception is raised before the ensure block (e.g., inside hide_const), the env var is still restored, but the pattern is inconsistent with the rest of the spec file. The existing project_root specs use stub_const("ENV", ENV.to_h.merge(...)), which is scoped to the example automatically:
| old_bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | |
| stub_const("ENV", ENV.to_h.merge("BUNDLE_GEMFILE" => File.join(tmp_dir, "Gemfile"))) |
This removes the need for the old_bundle_gemfile / ensure restoration entirely.
|
|
||
| def self.project_root | ||
| if defined?(Rails) && Rails.respond_to?(:root) && Rails.root | ||
| root = Rails.root |
There was a problem hiding this comment.
Rails.root always returns a Pathname, so root.respond_to?(:join) is always true. The guard is dead code. The original implementation (return Rails.root if ...) was simpler:
| root = Rails.root | |
| if defined?(Rails) && Rails.respond_to?(:root) && Rails.root | |
| return Pathname.new(Rails.root.to_s) | |
| end |
Or just return Rails.root since Pathname already responds to join.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails/lib/react_on_rails/packer_utils.rb`:
- Around line 225-226: The issue: interpreter-prefixed hook commands (e.g.,
"ruby bin/...") skip self-guard resolution because callers pass the raw hook
string into resolve_hook_script_path; update all call sites (including
resolve_hook_script_path usages at the snippet around
resolve_hook_script_path(hook_value) and the warning path in server_manager and
the hook_script_has_self_guard? method) to first try resolving the raw
hook_value and, if nil, extract the script argument via
extract_hook_script_argument_from_command(hook_value) and pass that to
resolve_hook_script_path; ensure hook_script_has_self_guard? and the warning
code use the resolved path (not the original raw command) so guarded hooks
behind interpreter prefixes are detected and file paths are shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 197b96da-c346-40ad-8cba-121e4dce012c
📒 Files selected for processing (3)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
234-240:⚠️ Potential issue | 🟠 MajorInterpreter-prefixed hook commands still bypass script resolution.
resolve_hook_script_pathjoins the full hook string, so values likeruby bin/shakapacker-precompile-hooknever resolve to the underlying file. That leaveshook_contains_generate_packs?,hook_script_has_self_guard?, and the warning path inreact_on_rails/lib/react_on_rails/dev/server_manager.rbaround Line 329 blind to valid file-backed hooks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/packer_utils.rb` around lines 234 - 240, resolve_hook_script_path currently joins the entire hook string so interpreter-prefixed commands like "ruby bin/shakapacker-precompile-hook" never resolve to the actual script file; update resolve_hook_script_path to parse the hook_value (use Shellwords.split or similar) and extract the candidate script path token (e.g., the first non-interpreter/flag argument or the last path-like token), then join that token with project_root and return it if file?; reference resolve_hook_script_path and ensure callers like hook_contains_generate_packs? and hook_script_has_self_guard? will receive a proper Path when an interpreter is present.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/packer_utils_spec.rb (1)
162-185: Add one ERB-backed fallback case.These examples lock down YAML aliases, but
extract_precompile_hook_from_yamlnow runsconfig/shakapacker.ymlthrough ERB first. A minimal<%= ENV.fetch("HOOK_PATH") %>fixture here would keep that advertised branch covered too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/packer_utils_spec.rb` around lines 162 - 185, The new test should also cover the ERB path: modify the shakapacker.yml fixture written in the example to include ERB (e.g. precompile_hook: '<%= ENV.fetch("HOOK_PATH") %>') and set ENV["HOOK_PATH"] to "bin/shakapacker-precompile-hook" before calling described_class.extract_precompile_hook so extract_precompile_hook_from_yaml's ERB processing is exercised; ensure you still stub ::Shakapacker.config to raise and restore any modified ENV (BUNDLE_GEMFILE and HOOK_PATH) in the ensure block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@react_on_rails/lib/react_on_rails/packer_utils.rb`:
- Around line 234-240: resolve_hook_script_path currently joins the entire hook
string so interpreter-prefixed commands like "ruby
bin/shakapacker-precompile-hook" never resolve to the actual script file; update
resolve_hook_script_path to parse the hook_value (use Shellwords.split or
similar) and extract the candidate script path token (e.g., the first
non-interpreter/flag argument or the last path-like token), then join that token
with project_root and return it if file?; reference resolve_hook_script_path and
ensure callers like hook_contains_generate_packs? and
hook_script_has_self_guard? will receive a proper Path when an interpreter is
present.
---
Nitpick comments:
In `@react_on_rails/spec/react_on_rails/packer_utils_spec.rb`:
- Around line 162-185: The new test should also cover the ERB path: modify the
shakapacker.yml fixture written in the example to include ERB (e.g.
precompile_hook: '<%= ENV.fetch("HOOK_PATH") %>') and set ENV["HOOK_PATH"] to
"bin/shakapacker-precompile-hook" before calling
described_class.extract_precompile_hook so extract_precompile_hook_from_yaml's
ERB processing is exercised; ensure you still stub ::Shakapacker.config to raise
and restore any modified ENV (BUNDLE_GEMFILE and HOOK_PATH) in the ensure block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 734482b9-ce6b-4fcc-b233-a4a736e76528
📒 Files selected for processing (3)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
- Guard Rails.env call in current_shakapacker_environment with local rescue to prevent exceptions during partial Rails initialization from discarding the YAML fallback path - Use key?-based lookup instead of || in extract_precompile_hook_from_shakapacker_config to preserve explicit false hook values - Add permitted_classes: [Symbol] to YAML.safe_load for consistency with codebase conventions and to handle symbol keys in shakapacker.yml - Pass explicit binding to ERB.new(...).result for minor hardening - Add PR/author attribution to CHANGELOG entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move interpreter-prefix stripping (ruby/node/bash/sh) into
resolve_hook_script_path so hook_script_has_self_guard? also works
with commands like "ruby bin/shakapacker-precompile-hook"
- Simplify project_root: Rails.root is always a Pathname, so the
respond_to?(:join) guard was dead code
- Remove redundant File.exist? check — resolve_hook_script_path already
guarantees .file? on returned paths
- Revert ERB.new(...).result(binding) back to .result to avoid leaking
method locals into the ERB template context
- Use stub_const("ENV", ...) in spec instead of direct ENV[] mutation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justin808
force-pushed
the
jg-codex/fix-bin-dev-precompile-hook-2438
branch
from
de5c746 to
be3c6a4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
249-260: Limited interpreter detection may missenvwrapper.The interpreter list
%w[ruby node bash sh]handles common cases but won't extract the script from commands likeenv ruby bin/hookor/usr/bin/env ruby bin/hook. While uncommon, shebang-style invocations sometimes useenv.Consider if supporting the
envwrapper is needed for your use cases. If so:♻️ Optional fix to handle env wrapper
def self.extract_script_from_command(command) parts = command.strip.split(/\s+/, 2) return nil unless parts.length == 2 interpreter = File.basename(parts[0]) + # Handle "env ruby bin/hook" -> recursively extract from the rest + return extract_script_from_command(parts[1]) if interpreter == "env" + return parts[1] if %w[ruby node bash sh].include?(interpreter) nil end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/packer_utils.rb` around lines 249 - 260, The extract_script_from_command method only recognizes interpreters in the first token (ruby/node/bash/sh) and thus misses commands wrapped with env (e.g., "env ruby bin/hook" or "/usr/bin/env ruby bin/hook"); update extract_script_from_command to detect when the first token's basename is "env" (or ends with "/env"), then examine the next token as the interpreter and return the following token as the script if that interpreter is in %w[ruby node bash sh]; keep existing behavior for non-env cases and still return nil when the pattern doesn't match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/react_on_rails/packer_utils.rb`:
- Around line 249-260: The extract_script_from_command method only recognizes
interpreters in the first token (ruby/node/bash/sh) and thus misses commands
wrapped with env (e.g., "env ruby bin/hook" or "/usr/bin/env ruby bin/hook");
update extract_script_from_command to detect when the first token's basename is
"env" (or ends with "/env"), then examine the next token as the interpreter and
return the following token as the script if that interpreter is in %w[ruby node
bash sh]; keep existing behavior for non-env cases and still return nil when the
pattern doesn't match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60667e78-e83f-46fa-992c-b4f5587fc746
📒 Files selected for processing (3)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/packer_utils.rbreact_on_rails/spec/react_on_rails/packer_utils_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| # In bin/dev startup before Rails boots, this can raise (e.g., missing Rails.env), | ||
| # so we rescue and fall back to parsing config/shakapacker.yml directly. | ||
| hook_value = extract_precompile_hook_from_shakapacker_config | ||
| return hook_value unless hook_value.nil? |
There was a problem hiding this comment.
The fallback to YAML parsing is triggered whenever extract_precompile_hook_from_shakapacker_config returns nil — including when Shakapacker config is healthy but simply has no precompile_hook configured. In that (common) case, every call hits the filesystem unnecessarily.
Consider distinguishing a "failed to load config" nil from a "config loaded, no hook present" nil, e.g. with a sentinel:
# In extract_precompile_hook_from_shakapacker_config, return a sentinel on success-but-no-hook
HOOK_NOT_CONFIGURED = :hook_not_configured
# Then in extract_precompile_hook:
hook_value = extract_precompile_hook_from_shakapacker_config
return nil if hook_value == HOOK_NOT_CONFIGURED
return hook_value unless hook_value.nil?
extract_precompile_hook_from_yamlAlternatively, have extract_precompile_hook_from_shakapacker_config raise a specific error on failure vs returning nil, so the caller can distinguish the two cases cleanly.
| return gemfile_path.dirname if gemfile_path.file? | ||
| end | ||
| interpreter = File.basename(parts[0]) | ||
| return parts[1] if %w[ruby node bash sh].include?(interpreter) |
There was a problem hiding this comment.
The hard-coded interpreter list only covers ruby, node, bash, and sh. A few common gaps:
zsh— used by macOS userspython/python3— uncommon but valid for hook scriptsenvprefix (e.g.,env ruby bin/hookor/usr/bin/env bash bin/hook) —envitself is the "interpreter", but the actual interpreter is the next token
The env case is particularly worth considering because some CI-generated scripts and Docker images produce commands like env RAILS_ENV=production ruby bin/hook. As written, File.basename("env") → "env" which is not in the list, so the whole hook_value is used as-is for path lookup and the script is never found.
If env is added, the second part would need further splitting to extract the actual script path.
| old_bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | ||
| ENV["BUNDLE_GEMFILE"] = File.join(tmp_dir, "Gemfile") |
There was a problem hiding this comment.
This test directly mutates ENV and restores it in an ensure block, while the earlier test for the same scenario (line ~174) uses stub_const("ENV", ENV.to_h.merge(...)) which is automatically cleaned up by RSpec. Mixing approaches is fragile — if the test fails between ENV["BUNDLE_GEMFILE"] = ... and the ensure, the restore still runs, but stub_const is strictly safer because it restores even across raised exceptions that might bypass ensure in edge cases.
Prefer stub_const for consistency:
| old_bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | |
| ENV["BUNDLE_GEMFILE"] = File.join(tmp_dir, "Gemfile") | |
| stub_const("ENV", ENV.to_h.merge("BUNDLE_GEMFILE" => File.join(tmp_dir, "Gemfile"))) | |
| config_path = project_root.join(SHAKAPACKER_CONFIG_PATH) | ||
| return nil unless config_path.file? | ||
|
|
||
| yaml_content = ERB.new(File.read(config_path)).result |
There was a problem hiding this comment.
ERB evaluation is consistent with how Rails and Shakapacker themselves load this file, so this is acceptable. Worth adding a comment here documenting the intent so future readers aren't alarmed:
| yaml_content = ERB.new(File.read(config_path)).result | |
| # ERB is evaluated to mirror how Shakapacker/Rails itself loads this file (e.g., for | |
| # environment-variable interpolation like <%= ENV["NODE_ENV"] %>). The file is part of | |
| # the application's own config so this is equivalent to what Rails already runs. | |
| yaml_content = ERB.new(File.read(config_path)).result |
|
The approach is sound and the fix addresses a real startup-ordering problem cleanly. The new specs cover the important scenarios. A few things worth addressing before merge. Functional concern: unnecessary YAML I/O on every call extract_precompile_hook falls through to extract_precompile_hook_from_yaml whenever extract_precompile_hook_from_shakapacker_config returns nil. But that method returns nil in two situations: (1) Shakapacker config raised an error (pre-Rails, the case this PR fixes) and (2) Shakapacker config loaded fine but no precompile_hook key is present (the common case). In case 2 every call to shakapacker_precompile_hook_configured? reads and ERB-renders config/shakapacker.yml unnecessarily. A sentinel return value would let the caller distinguish the two. See inline comment. extract_script_from_command interpreter list is narrow Only ruby, node, bash, sh are handled. The env-prefix pattern (env ruby bin/hook) is unhandled — the entire string ends up as a path lookup and the script is never found. See inline comment. Minor
|
Add entries for user-visible changes since v16.4.0.rc.6: - #2539: env-var-driven ports in Procfile templates - #2417: rspack generator config path fix - #2419: precompile hook load-based execution fix - #2577: create-react-on-rails-app validation improvements - #2416: StreamResponse status fallback fix (Pro) - #2566: empty-string license plan mismatch fix (Pro) - Updated #2561 entry to include #2568 contributor credit Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Add changelog entries for 6 user-visible PRs merged since v16.4.0.rc.6 that were missing from `[Unreleased]` - Update existing #2561 entry to include #2568 contributor credit ### New entries added | Section | PR | Description | |---|---|---| | Added | #2539 | Environment-variable-driven ports in Procfile templates | | Fixed | #2417 | Rspack generator config path fix | | Fixed | #2419 | Precompile hook load-based execution fix | | Fixed | #2577 | `create-react-on-rails-app` validation improvements | | Pro Fixed | #2416 | StreamResponse status fallback fix | | Pro Fixed | #2566 | Empty-string license plan mismatch fix | ### Skipped PRs (not user-visible) Docs (#2406, #2414, #2479, #2494, #2518, #2537, #2544), CI/internal (#2533, #2547, #2555, #2557, #2558, #2564), dependabot (#2387, #2541), dev dependencies (#2559, #2569, #2573). ## Test plan - [ ] Verify changelog formatting matches existing entries - [ ] Verify all user-visible PRs since v16.4.0.rc.6 are covered 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only changelog updates with no runtime or build behavior changes. > > **Overview** > Updates `CHANGELOG.md`’s **[Unreleased]** section to include previously missing user-facing entries: Procfile templates now support env-driven ports, several generator/`bin/dev` precompile-hook and rspack-path fixes are documented, and `create-react-on-rails-app` validation improvements are noted. > > Also adds two Pro fix entries (StreamResponse status fallback and license plan empty-string validation) and updates the existing `bin/dev` precompile-hook entry to credit an additional PR/contributor. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e75d2b5. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…2580) ## Summary - Reverts #2561 (squash commit 769d120), which was merged after #2568 already landed on master - #2568 + [shakacode/shakapacker#963](shakacode/shakapacker#963) fix the root cause of #2438 — #2561's YAML-parsing fallback is redundant - CHANGELOG entry updated to reference #2568 only ## What's removed All of #2561's additions on top of #2568: - `extract_precompile_hook_from_yaml` / `extract_precompile_hook_from_shakapacker_config` split - `extract_script_from_command` interpreter-prefix stripping - `extract_hash_for_environment` / `current_shakapacker_environment` helpers - `SHAKAPACKER_CONFIG_PATH` constant, `require "erb"`, `require "yaml"` - `rubocop:disable Metrics/ModuleLength` - `GENERATE_PACKS_PATTERN` expansion for `generate_packs_if_needed` - 72 lines of #2561-specific specs ## What remains (from #2568 + shakapacker#963) - `project_root` helper (Rails.root → BUNDLE_GEMFILE → Dir.pwd fallback) - `resolve_hook_script_path` using `project_root` - `require "pathname"` - 3 project_root specs - Shakapacker `Env#current` guard (separate repo, already merged) ## Why #2568 was designed to supersede #2561 — its PR description says so explicitly. Both were merged, resulting in unnecessary complexity (YAML config re-parsing, ERB evaluation, environment fallback chain) that duplicates what Shakapacker already handles after #963. ## Test plan - `bundle exec rspec react_on_rails/spec/react_on_rails/packer_utils_spec.rb` - `bundle exec rubocop react_on_rails/lib/react_on_rails/packer_utils.rb react_on_rails/spec/react_on_rails/packer_utils_spec.rb` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved bin/dev startup reliability with enhanced hook script path resolution during early startup when Rails.root is unavailable. * Removed YAML configuration dependency, streamlining hook detection. * Added validation checks for hook script existence to prevent runtime errors. * Simplified hook script pattern matching for better clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
bin/devruns before Rails is initialized by falling back to parsingconfig/shakapacker.ymlRails.rootgenerate_packs_if_neededin script-based precompile hooks sobin/devcorrectly skips duplicate pack generationCloses #2438.
Test Plan
bundle exec rspec react_on_rails/spec/react_on_rails/packer_utils_spec.rb react_on_rails/spec/react_on_rails/dev/server_manager_spec.rbbundle exec rubocop react_on_rails/lib/react_on_rails/packer_utils.rb react_on_rails/spec/react_on_rails/packer_utils_spec.rbNote
Medium Risk
Touches dev/startup hook detection and file parsing logic (ERB/YAML) used to decide whether to run pack generation; incorrect detection could skip or duplicate builds, but changes are isolated and well-covered by new specs.
Overview
Fixes
bin/dev/pack generation incorrectly falling back toreact_on_rails:generate_packsduring early startup by makingPackerUtils.extract_precompile_hookresilient when Shakapacker’s runtime config cannot load pre-Rails.Hook detection now falls back to parsing
config/shakapacker.yml(supports ERB + YAML aliases), resolves hook script paths from a project root derived withoutRails.root, and expands script matching to includegenerate_packs_if_needed.Adds regression specs covering the pre-Rails YAML fallback and script-based hook detection, and documents the fix in
CHANGELOG.md.Written by Cursor Bugbot for commit 884f63b. Configure here.
Summary by CodeRabbit
Bug Fixes
bin/devprecompile hook detection at early startup: falls back to parsing Shakapacker config (ERB/YAML, env-aware), recognizes additional hook names, resolves hook script paths without requiring framework root, and avoids unnecessary pack generation on first startup.Tests
Documentation