Add self-guard to precompile hook for HMR reliability #2388
Conversation
The detection logic was looking for `react_on_rails:generate_packs` rake task pattern, but the generator template uses `generate_packs_if_stale` Ruby method directly. This mismatch caused the detection to always return false for apps created with the generator. Updated the regex pattern to match both valid approaches: - The rake task: `react_on_rails:generate_packs` - The Ruby method: `generate_packs_if_stale` Fixes #2279 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Address review feedback: - Add \b word boundaries to GENERATE_PACKS_PATTERN to prevent false positives on substrings like 'my_generate_packs_if_stale_helper' - Add test for direct command containing generate_packs_if_stale - Add tests for script file reading path (both patterns) - Add test for script file not existing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The method can return nil when Shakapacker is not defined or on error, but the RBS signature declared it as returning String. Updated to String? to match the actual implementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
In HMR mode with SSR, Shakapacker spawns two webpack processes that each trigger the precompile_hook, causing duplicate execution. The existing SHAKAPACKER_SKIP_PRECOMPILE_HOOK env var only works on Shakapacker >= 9.4.0. This adds a self-guard (`exit 0 if ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"]`) directly in the hook script, making it version-independent. The version warning is now smarter with three cases: script+guard (silent), script without guard (suggests adding the line), and direct command hooks (suggests upgrading Shakapacker or switching to script-based hook). Also fixes the dummy app hook which was a no-op due to load not triggering the shared file's __FILE__ == $PROGRAM_NAME block. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Greptile OverviewGreptile SummaryThis PR improves Shakapacker precompile hook reliability in SSR+HMR setups by adding a self-guard ( On the Ruby side, Tests are expanded to cover the new hook extraction and self-guard detection behaviors, and docs/upgrading guidance is updated to recommend script-based hooks with self-guards for SSR+HMR across Shakapacker versions. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant Dev as bin/dev (ReactOnRails::Dev::ServerManager)
participant PU as ReactOnRails::PackerUtils
participant SC as Shakapacker.config
participant WH as Webpack process (client/server)
participant HS as Hook script (bin/shakapacker-precompile-hook)
Dev->>PU: shakapacker_version_requirement_met?("9.0.0")
Dev->>PU: shakapacker_version_requirement_met?("9.4.0")
alt Shakapacker < 9.4.0 and precompile_hook configured
Dev->>PU: shakapacker_precompile_hook_value()
PU->>SC: extract precompile_hook (public API if available)
Dev->>PU: hook_script_has_self_guard?(hook_value)
alt Hook is script path and has self-guard
Dev-->>Dev: No warning
else Hook is script path but missing guard
Dev-->>Dev: Print actionable warning to add self-guard line
else Hook is direct command
Dev-->>Dev: Print version warning and recommend upgrade or script-based hook
end
end
par Two webpack processes in SSR+HMR
WH->>HS: execute precompile_hook
WH->>HS: execute precompile_hook
end
alt ENV[SHAKAPACKER_SKIP_PRECOMPILE_HOOK] == "true"
HS-->>WH: exit 0 (self-guard prevents duplicate work)
else
HS->>HS: run precompile tasks (e.g., generate_packs_if_stale)
end
|
Code reviewFound 1 issue to address: CHANGELOG.md (line 33)Missing PR attribution for the changelog entry. According to AGENTS.md line 168, the required format is:
Current (line 33): Should end with: The same issue applies to line 34 (the "Dummy App Precompile Hook" entry should also end with the same PR attribution). |
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Caution Review failedThe pull request is closed. WalkthroughImplements robust detection for Shakapacker precompile hooks, adds a self-guard to precompile hook scripts to avoid duplicate HMR-triggered runs, extends packer utility helpers and RBS signatures, updates server warning logic and generator templates, and adds documentation and tests for the new behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant WebpackA as Webpack Dev (proc A)
participant WebpackB as Webpack Dev (proc B)
participant HookScript as bin/shakapacker-precompile-hook
participant RailsApp as Rails (react_on_rails)
WebpackA->>HookScript: invoke precompile hook
HookScript->>HookScript: read SHAKAPACKER_SKIP_PRECOMPILE_HOOK
alt skip == "true"
HookScript-->>WebpackA: exit (skip)
else
HookScript->>RailsApp: load shared hook file
HookScript->>RailsApp: call build_rescript_if_needed / generate_packs_if_needed
HookScript-->>WebpackA: exit
end
WebpackB->>HookScript: concurrent invoke
HookScript->>HookScript: read SHAKAPACKER_SKIP_PRECOMPILE_HOOK
alt skip == "true"
HookScript-->>WebpackB: exit (skip duplicate)
else
HookScript->>RailsApp: (may be skipped by guard) run tasks
HookScript-->>WebpackB: exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…manager detection 1. Add -r pathname to rbs validate command so the Pathname type used in packer_utils.rbs can be resolved during validation. 2. Fix precompile hook to detect the correct package manager from package.json's packageManager field instead of blindly preferring yarn. In CI, global yarn exists but the project uses pnpm, causing yarn to reject the packageManager: "[email protected]" field. Co-Authored-By: Claude Opus 4.6 <[email protected]>
![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
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 34: The CHANGELOG.md entry about the dummy app precompile hook
(referencing spec/dummy/bin/shakapacker-precompile-hook and PR 2388) is a
tests-only/internal change and should not appear in the user-facing changelog;
either remove this bullet entirely or relocate it under a non-user-facing
section such as "Developer (Contributors Only)" or "Internal/Testing" so that
CHANGELOG.md only contains user-visible changes (features, bug fixes, breaking
changes, deprecations, performance improvements).
🧹 Nitpick comments (2)
react_on_rails/lib/react_on_rails/packer_utils.rb (1)
207-216: RedundantFile.exist?check afterresolve_hook_script_path.
resolve_hook_script_pathalready returnsnilunlesspotential_path.file?, so theFile.exist?(script_path)check on line 212 is redundant. Consider simplifying to justreturn false unless script_path.♻️ Proposed simplification
# Check if it's a script file path script_path = resolve_hook_script_path(hook_value) - return false unless script_path && File.exist?(script_path) + return false unless script_pathreact_on_rails/lib/react_on_rails/dev/server_manager.rb (1)
322-341: Minor:resolve_hook_script_pathcalled twice for the same hook value.
hook_script_has_self_guard?(line 326) internally callsresolve_hook_script_path, and then line 329 calls it again. Consider extracting the script path once and passing it through, or accepting the minor redundancy for readability.
CHANGELOG.md
Outdated
|
|
||
| - **Precompile Hook Detection**: Fixed `shakapacker_precompile_hook_configured?` always returning `false` for apps created with the React on Rails generator. The detection logic only matched the rake task pattern (`react_on_rails:generate_packs`) but the generator template uses the Ruby method (`generate_packs_if_stale`). Now correctly detects both patterns, including resolving script file contents. [PR 2282](https://github.com/shakacode/react_on_rails/pull/2282) by [ihabadham](https://github.com/ihabadham). | ||
| - **Precompile Hook Self-Guard for HMR**: Added self-guard to the generator template's `bin/shakapacker-precompile-hook` to prevent duplicate execution in HMR mode where two webpack processes (client dev-server + server watcher) each trigger the hook. The script now exits early when `SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true` is set by `bin/dev`, regardless of Shakapacker version. The version warning is now smarter: it only warns for hooks that lack the self-guard or use direct commands. **Existing users**: add `exit 0 if ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] == "true"` near the top of your `bin/shakapacker-precompile-hook` script. [PR 2388](https://github.com/shakacode/react_on_rails/pull/2388) by [justin808](https://github.com/justin808). | ||
| - **Dummy App Precompile Hook**: Fixed `spec/dummy/bin/shakapacker-precompile-hook` which was a no-op due to `load` not triggering the shared file's `__FILE__ == $PROGRAM_NAME` block. The hook now explicitly invokes the shared functions after loading. [PR 2388](https://github.com/shakacode/react_on_rails/pull/2388) by [justin808](https://github.com/justin808). |
There was a problem hiding this comment.
Dummy app fix may not warrant a user-facing changelog entry.
The spec/dummy precompile hook fix is an internal test infrastructure change, not a user-visible bug fix. Per coding guidelines, the CHANGELOG should include user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements) and exclude tests or refactoring. Consider moving this to a "Developer (Contributors Only)" section or removing it.
As per coding guidelines, "Update /CHANGELOG.md for user-visible changes only (features, bug fixes, breaking changes, deprecations, performance improvements) — exclude linting, formatting, refactoring, tests, or doc fixes."
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 34, The CHANGELOG.md entry about the dummy app
precompile hook (referencing spec/dummy/bin/shakapacker-precompile-hook and PR
2388) is a tests-only/internal change and should not appear in the user-facing
changelog; either remove this bullet entirely or relocate it under a
non-user-facing section such as "Developer (Contributors Only)" or
"Internal/Testing" so that CHANGELOG.md only contains user-visible changes
(features, bug fixes, breaking changes, deprecations, performance improvements).
The "Dummy App Precompile Hook" entry was a tests-only/internal change (fixing spec/dummy/bin/shakapacker-precompile-hook) and should not appear in the user-facing changelog. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Avoid false positives when checking for self-guard logic by requiring an actual early-exit statement. Add coverage for public/fallback precompile hook extraction and guard detection edge cases. Also make the Pro dummy hook invoke the shared helper methods after load so behavior matches the OSS dummy hook.
Align process-manager and i18n docs with script self-guard behavior, clarify when warnings are shown, and add explicit upgrade guidance for existing SSR+HMR apps including version and CI recommendations.
- Add extract_precompile_hook and hook_contains_generate_packs? to packer_utils.rbs for type-checking completeness - Fix misleading test description to accurately reflect what the test validates (no exit/return guard, not just comments/strings) Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 2m 8s —— View job Code Review Complete ✅Todo List
Review SummaryThis is a well-implemented PR that solves a real problem (duplicate precompile hook execution in HMR mode with SSR). The implementation is thorough, well-tested, and backwards compatible. CI is mostly passing (7 in progress, 4 passed, 0 failed). ✅ Strengths
🔍 Issues Found1. CHANGELOG: Missing PR attribution (Already identified by previous reviewers)Location: Per Both changelog entries (lines 33 and the removed line 34 about dummy app) need PR attribution at the end. Status: ✅ RESOLVED - Line 34 (dummy app entry) was removed in commit 2. Minor code redundancy in
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| puts Rainbow(" Recommendations:").cyan | ||
| puts Rainbow(" 1. Upgrade to Shakapacker 9.4.0 or later:").cyan | ||
| puts Rainbow(" bundle update shakapacker").cyan.bold | ||
| puts Rainbow(" 2. Or switch to a script-based hook with a self-guard.").cyan | ||
| puts Rainbow(" See: https://www.shakacode.com/react-on-rails/docs/building-features/i18n").cyan |
There was a problem hiding this comment.
Incorrect docs link
This warning message links to the i18n docs (/docs/building-features/i18n), but the recommendation here is about migrating precompile_hook to a script-based hook with a self-guard. That link will send users to an unrelated page, so they won’t find the migration steps referenced by the warning. Please update it to point at the precompile hook guidance (e.g., the process-managers section that documents script-based hooks/self-guard).
The warning for direct-command hooks linked to the i18n docs page instead of the process-managers page that documents script-based hooks and the self-guard pattern. Co-Authored-By: Claude Opus 4.6 <[email protected]>
2 participants
Summary
exit 0 if ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] == "true") to the generator template'sbin/shakapacker-precompile-hook, preventing duplicate execution in HMR mode (two webpack processes) regardless of Shakapacker versionloadnot triggering__FILE__ == $PROGRAM_NAMEextract_precompile_hookto prefer Shakapacker's public API when availablehook_script_has_self_guard?detection method with full test coverageBuilds on #2282 (hook detection fix).
Problem
In HMR mode with SSR, Shakapacker spawns two webpack processes (
wp-clientandwp-server), each triggering theprecompile_hook. The existingSHAKAPACKER_SKIP_PRECOMPILE_HOOKenv var set bybin/devonly works on Shakapacker >= 9.4.0.Migration for existing users
Add this line near the top of your
bin/shakapacker-precompile-hook:New installations get this automatically. The version warning will detect missing self-guards and suggest the fix.
Test plan
bundle exec rspec spec/react_on_rails/packer_utils_spec.rb(28 examples, 0 failures)bundle exec rspec spec/react_on_rails/dev/server_manager_spec.rb(50 examples, 0 failures)bundle exec rubocopon all changed files (0 offenses)SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true bin/shakapacker-precompile-hookexits immediately🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests