Fix load-based precompile hook execution path

[justin808]

Summary

• Add a shared run_precompile_tasks entrypoint in shakapacker_precompile_hook_shared.rb
• Update both dummy hook wrappers to call run_precompile_tasks after load, instead of relying on __FILE__ == $PROGRAM_NAME
• Add a regression spec to verify the load-based entrypoint executes both precompile steps

Closes #2195

Test plan

• Proposed fix (UNTESTED in this environment)
• Intended: bundle exec rspec react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb
• Blocked by local Ruby version (2.6.10) vs project requirement (>= 3.0.0)

Summary by CodeRabbit

• Tests

  • Added test coverage to verify consolidated precompile tasks are invoked correctly.

• Refactor

  • Unified precompilation steps into a single, consolidated execution entry point for simpler, more reliable precompile behavior.

[coderabbitai]

Walkthrough

Replace the conditional inline execution in the shared Shakapacker precompile hook with a public run_precompile_tasks method, and update the bin callers to invoke this method so the build and pack-generation steps run when the shared file is loaded.

Changes

Cohort / File(s)	Summary
Shared precompile hook react_on_rails/spec/support/shakapacker_precompile_hook_shared.rb, react_on_rails_pro/spec/support/shakapacker_precompile_hook_shared.rb	Add run_precompile_tasks that calls build_rescript_if_needed and generate_packs_if_needed; move inline guarded execution into that method.
Bin callers (precompile hooks) react_on_rails/spec/dummy/bin/shakapacker-precompile-hook, react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook	Replace direct calls/guarded reliance with an explicit run_precompile_tasks invocation.
Tests react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb	New spec asserting run_precompile_tasks invokes both build_rescript_if_needed and generate_packs_if_needed.
Manifest Gemfile	Minor dependency/version edits (4 additions, 5 removals across diff).

Sequence Diagram(s)

sequenceDiagram
    participant HookCaller as Bin script
    participant Shared as shakapacker_precompile_hook_shared.rb
    participant Rescript as build_rescript_if_needed
    participant Packs as generate_packs_if_needed

    HookCaller->>Shared: load shared hook
    HookCaller->>Shared: run_precompile_tasks()
    Shared->>Rescript: build_rescript_if_needed()
    Rescript-->>Shared: build complete
    Shared->>Packs: generate_packs_if_needed()
    Packs-->>Shared: generation complete
    Shared-->>HookCaller: return

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I loaded the hook and gave it a poke,
No more false guards, no invisible yoke.
One call to run, two tasks hum along,
Precompile dances, singing a build-time song. 🎶

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: fixing the execution path of the load-based precompile hook by introducing a run_precompile_tasks method.
Linked Issues check	✅ Passed	The PR implements option C from issue #2195: exposing a run_precompile_tasks method and invoking it from wrappers after load, matching the chosen approach and resolving the $PROGRAM_NAME execution issue.
Out of Scope Changes check	✅ Passed	All changes directly address the precompile hook execution issue: refactoring shared hook API, updating both wrapper scripts, and adding regression tests. No unrelated changes detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into master

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch codex/fix-2195-precompile-hook-execution

---

No actionable comments were generated in the recent review. 🎉

---

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

[greptile-apps]

Greptile Summary

Refactors the shared precompile hook to expose a run_precompile_tasks entrypoint method, replacing the previous pattern where hook wrappers called build_rescript_if_needed and generate_packs_if_needed individually after load. Both dummy hook wrappers (OSS and Pro) are updated to call the new single method. A regression spec is added, though it was not executed by the author.

• The core change in shakapacker_precompile_hook_shared.rb is clean: wrapping two calls in a named method and calling it from the __FILE__ == $PROGRAM_NAME guard preserves backward compatibility while providing a cleaner API for load-based callers
• Both hook wrappers are updated identically — no functional change, just using run_precompile_tasks instead of two separate calls
• The new spec was not run by the PR author (blocked by local Ruby version mismatch). It should be verified in CI before merging
• The spec uses load in a before block which leaks methods onto Object globally for the test suite — consider isolating with a module wrapper
• Note: the previous commit (c1c5de2's parent) already fixed the original issue (Precompile hook doesn't run build steps due to $PROGRAM_NAME check #2195) by explicitly calling the functions after load. This PR is a follow-up refactor to consolidate those calls into a single run_precompile_tasks method

Confidence Score: 3/5

• Low-risk refactor of test infrastructure, but the new spec was not verified and has test isolation concerns
• The core refactor (wrapping two calls in a method) is trivially correct. Both hook wrappers are updated consistently. However, the new regression spec was explicitly not run by the author, has potential global pollution from load, and provides limited regression value. The changes only affect test dummy infrastructure, not production code, which limits the blast radius.
• Pay close attention to react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb — untested and has global method pollution concerns

Important Files Changed

Filename	Overview
react_on_rails/spec/support/shakapacker_precompile_hook_shared.rb	Wraps existing build_rescript_if_needed and generate_packs_if_needed calls into a new run_precompile_tasks method, and calls it from the __FILE__ == $PROGRAM_NAME guard. Clean refactor with no functional change.
react_on_rails/spec/dummy/bin/shakapacker-precompile-hook	Replaces two explicit function calls (build_rescript_if_needed, generate_packs_if_needed) with a single run_precompile_tasks call. Functionally equivalent, cleaner API surface.
react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook	Mirror of the OSS dummy hook change — replaces two explicit calls with single run_precompile_tasks call. Identical refactor.
react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb	New regression spec for run_precompile_tasks. Has global pollution concern from load in before block, and was not executed by the PR author. Test value is limited — only asserts delegation, not the actual load-based execution path.

Flowchart

flowchart TD
    A["bin/shakapacker-precompile-hook<br/>(OSS or Pro dummy)"] -->|"check ENV guard"| B{SHAKAPACKER_SKIP_PRECOMPILE_HOOK?}
    B -->|"true"| C["exit 0"]
    B -->|"false"| D["load shakapacker_precompile_hook_shared.rb"]
    D --> E["call run_precompile_tasks()"]
    E --> F["build_rescript_if_needed()"]
    E --> G["generate_packs_if_needed()"]
    
    H["Direct execution<br/>(__FILE__ == $PROGRAM_NAME)"] --> E

    style A fill:#4a90d9,color:#fff
    style E fill:#2ecc71,color:#fff
    style H fill:#e67e22,color:#fff

Loading

_{Last reviewed commit: 7b8eaef}

[greptile-apps]

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Global method pollution from load in before block

Using load in the before block defines run_precompile_tasks, build_rescript_if_needed, generate_packs_if_needed, find_rails_root, and detect_package_manager as private instance methods on Object. These persist for the entire test suite run and could interfere with other specs.

Consider wrapping the load in a clean module to isolate these methods:

Suggested change

	before do
	load File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__)
	end
	it "exposes run_precompile_tasks for load-based callers" do
	allow(self).to receive(:build_rescript_if_needed)
	allow(self).to receive(:generate_packs_if_needed)
	run_precompile_tasks
	expect(self).to have_received(:build_rescript_if_needed)
	expect(self).to have_received(:generate_packs_if_needed)
	end
	let(:hook_module) do
	Module.new.tap { |mod| mod.module_eval(File.read(File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__))) }
	end
	it "exposes run_precompile_tasks for load-based callers" do
	allow(hook_module).to receive(:build_rescript_if_needed)
	allow(hook_module).to receive(:generate_packs_if_needed)
	hook_module.run_precompile_tasks
	expect(hook_module).to have_received(:build_rescript_if_needed)
	expect(hook_module).to have_received(:generate_packs_if_needed)
	end

Alternatively, at minimum, the before block should use load shared_path, true (the second argument wraps the code in an anonymous module) to prevent global leakage:

before do
  load File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__), true
end

Note: using load path, true would wrap the methods in an anonymous module, making them inaccessible from the test. So the Module.new approach or a different isolation strategy would be needed if this is a concern.

[greptile-apps]

Test was not run per PR description

The PR description states this test is "UNTESTED in this environment" due to a Ruby version mismatch. Per AGENTS.md policy: "Never claim a test is 'fixed' without running it locally first." The test should be verified in CI or by another contributor before merging.

Additionally, the test only asserts that run_precompile_tasks delegates to the two sub-methods — something trivially obvious from the 3-line implementation. It doesn't test the actual load-based execution path from the hook wrappers (i.e., that load shared_hook followed by run_precompile_tasks works end-to-end). A more meaningful test would invoke the dummy bin/shakapacker-precompile-hook script in a subprocess and verify the expected output or side effects.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}