refactor: unify renderer cache staging with mode: :copy / :symlink

[justin808]

Summary

Collapses ReactOnRailsPro::PreSeedRendererCache (copy) and ReactOnRailsPro::PrepareNodeRenderBundles (symlink) into a single entry point: PreSeedRendererCache.call(mode: :copy | :symlink). Both modes produce the same `//.js` layout — only the file operation differs.

Stacked on #3124 (PR A). Target base is `jg/3122-preseed-renderer-cache`; will be rebased onto `master` once PR A merges. Refs #3122.

Changes

• Unified API: `PreSeedRendererCache.call(mode: :copy)` for Docker/image builds (default); `(mode: :symlink)` for same-filesystem workflows (local dev, CI, Heroku-style same-dyno deploys, bundle-caching restores). Unknown modes → `ArgumentError`.
• Non-dev env-var guard (copy mode only): When neither `RENDERER_SERVER_BUNDLE_CACHE_PATH` nor `RENDERER_BUNDLE_PATH` is set in a non-dev/test environment, `mode: :copy` raises a clear error. Motivation: the Node renderer's default cache-path resolution can differ from the Ruby side (falling back to `/tmp` when `cwd` is outside the app tree), so silent fallback is a misconfig footgun that pre-seeds bundles into a directory the renderer never reads.
• Rake task: `react_on_rails_pro:pre_seed_renderer_cache` accepts `MODE=copy` (default) or `MODE=symlink`.
• Deprecations (soft): `PrepareNodeRenderBundles` and the `pre_stage_bundle_for_node_renderer` rake task remain as thin shims emitting once-per-process deprecation warnings; both delegate to the unified API with `mode: :symlink`. No behavior change for existing users.
• Auto-invocation: `AssetsPrecompile.call` now calls `PreSeedRendererCache.call(mode: :symlink)` after precompile (previously `PrepareNodeRenderBundles.call` — same effect).
• Doctor check: `react_on_rails:doctor` scans common deploy-script locations (`Procfile*`, `Dockerfile`, `bin/deploy`, `bin/release`, `bin/docker-entrypoint`) for references to the deprecated task and surfaces a migration warning.
• Docs: `docs/pro/node-renderer.md` updated to describe both modes, the non-dev env-var requirement for copy, and the deprecation story.
• CHANGELOG: new `Changed` entry.

Why this shape

• The two classes shared ~90% of their logic after feat: align renderer cache staging with bundle-hash layout #3124 (both use `RendererCacheHelpers` for bundle validation, asset collection, and required-RSC-asset path construction). Keeping them separate forced readers to understand two entry points with parallel semantics.
• Single class with `mode:` keeps the public surface obvious: the user picks copy vs symlink based on whether bundles need to survive Docker layer boundaries. Everything else is shared.
• Soft deprecation avoids breaking users with `Procfile`/`Dockerfile`/`bin/` entries pinned to the old names. The doctor check gives them a clear migration nudge.

Test plan

• 26 dummy specs pass for both `pre_seed_renderer_cache_spec.rb` and `prepare_node_renderer_bundles_spec.rb` (+6 new: mode validation, symlink mode via unified API, raise-in-non-dev guard, symlink mode opt-out from guard, env-var bypass for guard, deprecation warning on the shim).
• 2 new doctor specs for `check_deprecated_renderer_cache_task`: Procfile-reference warning + no-match silence.
• 163 total doctor specs pass (1 pre-existing unrelated failure in `auto_fix_versions`, not introduced by this PR).
• `bundle exec rubocop` clean on all changed files.
• CI (will verify after push).

Migration path for users

• Existing `Procfile` / `Dockerfile` / `bin/*` entries that call `pre_stage_bundle_for_node_renderer` keep working unchanged — they emit a one-time deprecation warning and delegate to `MODE=symlink`.
• Doctor surfaces affected files with migration guidance.
• No changes needed for users relying on `AssetsPrecompile.call` auto-invocation (same behavior).
• Docker builds that were using the old task name should migrate to `MODE=copy` (the default) for image-baked caches.

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches Pro deployment/SSR renderer-cache staging behavior and introduces new env-var enforcement in MODE=copy, which could break misconfigured non-dev environments; changes are mitigated by deprecation shims and added doctor warnings/tests.

Overview
Unifies Node Renderer cache staging in Pro by making ReactOnRailsPro::PreSeedRendererCache.call(mode: :copy | :symlink) the single entry point; both modes produce the same <cache>/<bundleHash>/<bundleHash>.js layout and the rake task react_on_rails_pro:pre_seed_renderer_cache now accepts MODE=copy (default) or MODE=symlink.

Deprecates legacy paths: react_on_rails_pro:pre_stage_bundle_for_node_renderer and ReactOnRailsPro::PrepareNodeRenderBundles now warn and delegate to mode: :symlink, while assets:precompile auto-stages via mode: :symlink.

Adds safety/migration checks: MODE=copy now raises in non-dev/test when neither RENDERER_SERVER_BUNDLE_CACHE_PATH nor RENDERER_BUNDLE_PATH is set, and react_on_rails:doctor scans common deploy scripts for the deprecated task and suggests the correct replacement.

Docs/CHANGELOG are updated for the new modes and deprecations; specs are expanded for the new behavior, and the link checker ignore list adds guavapass.com.

^{Reviewed by Cursor Bugbot for commit f511867. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0f52bcf8-e303-4099-ac80-b4ccc754f42b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/3122-unify-renderer-cache-staging

---

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]

Code Review

Overall: Clean, well-motivated refactor. The unified API is simpler to reason about than two parallel classes sharing 90% of their logic, the deprecation shims preserve backward compatibility correctly, and the env-var guard for copy mode is a genuine foot-gun protection. The test coverage is solid. A few issues worth addressing before merge:

---

Correctness / Edge Cases

No atomicity on partial failure. If a copy or symlink fails midway through staging (e.g. disk full after the first bundle file is written), the cache directory is left in a partially-staged state. The renderer could pick up an incomplete cache directory on startup. This was true for the previous classes too, so it's not a regression — but now that both modes share this code path it's worth a note. Wrapping the staging loop in a begin/rescue that tears down the partially-written cache_dir on failure would make this more robust.

make_relative_symlink realpath can raise unexpectedly. See inline comment on line 130. Short-circuited in the happy path, but a dangling-symlink webpack output would surface as an Errno::ENOENT with no diagnostic context.

---

Test Coverage Gap

The mode: :symlink spec only exercises the bundle file, not stage_assets. The else branch in stage_assets (which calls make_relative_symlink for each configured asset) is untested. See inline comment on line 62 of the spec.

---

Minor / Nits

• reset_deprecation_warned! is public API — test helper exposed without guard. Inline comment on prepare_node_renderer_bundles.rb:28.
• Error message in enforce_cache_dir_env_var! doesn't mention MODE=symlink as an alternative when the env var is absent. Inline comment on pre_seed_renderer_cache.rb:62.
• File.read without size limit in the doctor check. Low risk for a developer tool, but simple to guard. Inline comment on doctor.rb:2749.
• Arbitrary symbol from ENV["MODE"] in the rake task. Caught immediately by VALID_MODES, but a pre-validation abort message would be more user-friendly. Inline comment on assets.rake:9.
• Spec ensure env var leak (minor, outer after covers it). Inline comment on spec line 83.

---

Documentation / Changelog

Both look good. The Dockerfile snippet now correctly places ENV RENDERER_SERVER_BUNDLE_CACHE_PATH before the RUN rake line, and the deprecation story is clearly communicated.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e97260dc38

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Greptile Summary

This PR collapses PreSeedRendererCache (copy) and PrepareNodeRenderBundles (symlink) into a single PreSeedRendererCache.call(mode: :copy | :symlink) entry point, adds MODE= support to the rake task, introduces a non-dev env-var guard for copy mode, wires the doctor to flag deprecated task usage, and soft-deprecates the old class and task as thin shims.

All three prior review concerns (MODE case-normalization, @deprecation_warned mutex, and ENV cleanup in ensure) are addressed in this revision. Remaining findings are all P2.

Confidence Score: 5/5

Safe to merge; no P0/P1 issues found, and all prior review feedback has been addressed.

All three previous review concerns (mutex for deprecation guard, MODE env-var case normalization, ENV cleanup in ensure) are resolved. The remaining comments are P2 style/test-coverage suggestions: a missing Dockerfile branch test in the doctor spec, a misleading rescue message in make_relative_symlink, and a silent large-file skip in the doctor check. None affect correctness or runtime behavior.

No files require special attention.

Important Files Changed

Filename	Overview
react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb	New unified entry point for renderer cache staging; copy/symlink modes are cleanly separated; realpath canonicalization and mode validation are solid; outer rescue in make_relative_symlink can produce a misleading error message (minor).
react_on_rails_pro/lib/react_on_rails_pro/prepare_node_renderer_bundles.rb	Correctly thinned to a mutex-guarded deprecation shim that delegates to PreSeedRendererCache; once-per-process guarantee is now thread-safe.
react_on_rails_pro/lib/tasks/assets.rake	MODE env var is now lowercased and defaulted before validation; deprecated task emits a clear warning and delegates to the unified API.
react_on_rails/lib/react_on_rails/doctor.rb	New check_deprecated_renderer_cache_task scans known deploy-script locations; size guard silently skips > 1MB files (minor UX gap); Dockerfile vs Procfile suggestion branching is correct.
react_on_rails_pro/lib/react_on_rails_pro/assets_precompile.rb	Auto-invocation correctly switches from PrepareNodeRenderBundles to PreSeedRendererCache(mode: :symlink); PreSeedRendererCache is loaded via the gem's top-level require chain.
react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb	Good coverage for mode validation, symlink/copy behavior, env-var guard, and ENV cleanup in ensure blocks; Dockerfile migration-suggestion path lacks a dedicated test.
react_on_rails_pro/spec/dummy/spec/prepare_node_renderer_bundles_spec.rb	Correctly exercises the deprecation warning and delegates to the unified API; proper cleanup via reset_deprecation_warned! in after hooks.
react_on_rails/spec/lib/react_on_rails/doctor_spec.rb	New check_deprecated_renderer_cache_task specs use Dir.chdir+mktmpdir correctly; only the Procfile path is tested, Dockerfile suggestion branch is untested.
CHANGELOG.md	Clear 'Changed' entry documents the API unification, mode semantics, non-dev guard, and shim deprecation.
docs/pro/node-renderer.md	Documentation updated to describe both staging modes, the non-dev env-var requirement, and the deprecation migration path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[["rake pre_seed_renderer_cache\nMODE=copy|symlink"]] --> B{Normalize & validate MODE}
    B -- invalid --> Z1[abort with valid modes list]
    B -- :copy / :symlink --> C[PreSeedRendererCache.call mode:]

    D[["rake pre_stage_bundle_for_node_renderer\n(deprecated)"]] -->|warn + delegate| C

    E[[PrepareNodeRenderBundles.call\n deprecated shim]] -->|mutex-guarded warn + delegate| C

    F[[AssetsPrecompile.call]] -->|mode: :symlink\nif node_renderer?| C

    C --> G{enforce_cache_dir_env_var!}
    G -- mode==:copy AND no env var AND not dev/test --> Z2[raise ReactOnRailsPro::Error]
    G -- passes --> H[resolve_cache_dir]

    H --> I[RendererCacheHelpers.bundle_sources]
    I --> J{for each bundle src + hash}

    J --> K[stage_bundle src → bundle_dir/hash.js]
    K -- :copy --> K1[FileUtils.cp]
    K -- :symlink --> K2[make_relative_symlink\nrealpath both sides]

    J --> L[stage_assets → bundle_dir]
    L -- :copy --> L1[FileUtils.cp each asset]
    L -- :symlink --> L2[make_relative_symlink each asset]
    L -- missing required RSC asset --> Z3[raise ReactOnRailsPro::Error]
    L -- missing optional asset --> L3[warn to stderr]

    style Z1 fill:#f88,stroke:#c00
    style Z2 fill:#f88,stroke:#c00
    style Z3 fill:#f88,stroke:#c00

Loading

_{Reviews (2): Last reviewed commit: "fix: address PR #3167 review feedback (r..." | Re-trigger Greptile}

[justin808]

Addressed review feedback in 14f4a45

Fixed (8 items):

1. DISCUSS TODO for first version #1 — per-file migration command — doctor now branches the suggestion: Dockerfile matches recommend the bare `rake react_on_rails_pro:pre_seed_renderer_cache` (MODE=copy is the default); Procfile/bin matches recommend `MODE=symlink`.
2. Make work with turbolinks and better helper #2 — escape hatch in copy-mode error — the env-var enforcement error now points CI users at `MODE=symlink` as an alternative when they don't need an immutable artifact.
3. Add linting and CI scripts #3 — realpath ENOENT guard — `make_relative_symlink` rescues `Errno::ENOENT` into a clear `ReactOnRailsPro::Error` so dangling symlinks / mid-rotate webpack output surface with actionable text.
4. Add integration tests #4 — private_class_method on reset_deprecation_warned! — specs invoke via `send`; prevents accidental reset from production code.
5. Add Unit tests #5 + Support configuration options #9 — validate + downcase `MODE` — rake task downcases the env var, validates against `PreSeedRendererCache::VALID_MODES` before `to_sym`, and aborts with a listing of valid modes on unknown values. Accepts `MODE=Copy` / `MODE=COPY`.
6. Clean up app/startup/clientApp.jsx and app/startup/serverApp.jsx #7 — symlink + assets spec — new test in `pre_seed_renderer_cache_spec.rb` exercises asset symlinking via the unified API (previously only covered via the deprecation-shim spec).
7. Port console polyfill from react-rails #8 — ensure block cleanup — env-var-bypass test uses a local `tmpdir` variable so `FileUtils.rm_rf` + `ENV.delete` run from `ensure` even if `Dir.mktmpdir` or an intermediate assertion raises.
8. Detailed Setup and Usage Docs #6 skipped — size guard on `File.read` in the doctor scan. The scanned files (Procfile, Dockerfile, bin/*) are tiny in practice and checked once per doctor invocation; adding a size cap would be dead defensive code. Reconsider if we ever see a real-world issue.

Declined with rationale (1 item):

9. Support generator to build a template wepack setup #10 — @deprecation_warned thread-safety — this code path runs only from rake tasks and `AssetsPrecompile.call` (single-threaded contexts), never from request handling. A race would produce at most a harmless duplicate warning, not a correctness issue. Wrapping in a Mutex adds locking ceremony for code that never contends. Consistent with the PR A precedent for `@renderer_bundle_path_deprecation_warned`.

27 dummy specs pass (was 26, +1 new symlink-assets test); 163 doctor specs pass (same pre-existing unrelated failure in `auto_fix_versions`); rubocop clean.

I (Claude Code) addressed the feedback on behalf of @justin808.

[claude]

Code Review

Overall: solid refactor with good test coverage and a clean deprecation story. One genuine bug in the doctor check; the rest are minor concerns.

What this does well

• Unification is clean: collapsing copy/symlink into a single mode: parameter makes the public API obvious and eliminates ~40 lines of duplicated logic from PrepareNodeRenderBundles.
• Soft deprecation is correct: shims are thin, emit once-per-process warnings, delegate immediately, and the doctor check provides a clear migration nudge.
• enforce_cache_dir_env_var! is the right call: silently pre-seeding bundles into a directory the renderer never reads is exactly the kind of footgun that causes hours of debugging. Failing fast in non-dev/test copy mode is the right trade-off.
• make_relative_symlink error handling: catching Errno::ENOENT and re-raising as ReactOnRailsPro::Error with a human-readable message is good practice. The realpath canonicalization for macOS /var to /private/var is a known gotcha.
• Test coverage: the 6 new spec cases cover the important paths (mode validation, symlink mode, env-var guard raise/bypass, symlink opt-out from guard). Doctor specs using Dir.chdir into a tmpdir is the right isolation approach.

Issues

1. Bug: doctor path scan uses Dir.pwd, not Rails.root (see inline comment on doctor.rb:2744)

File.exist?(path) and File.read(path) resolve the bare strings ("Procfile", "Dockerfile", etc.) relative to Dir.pwd. If rake react_on_rails:doctor is run from a subdirectory, all files will appear not to exist and the deprecation warning is silently swallowed. Fix: use Rails.root.join(path).

2. .present? edge case (see inline on pre_seed_renderer_cache.rb:59)

ENV["..."].present? treats a whitespace-only env var as "not set", which fires the error even though the variable is technically set. Unlikely in practice, but worth noting alongside the implicit Active Support dependency.

3. rescue Errno::ENOENT message blames only source (see inline on pre_seed_renderer_cache.rb:140)

The rescue message says "could not resolve real path for symlink source" but the rescue covers both source_path.realpath and destination_dir.realpath. On a permissions failure, the message would be misleading.

4. @deprecation_warned not thread-safe (see inline on prepare_node_renderer_bundles.rb:17)

Minor - worst case is a duplicate deprecation warning. Acceptable for a shim.

Minor observations

• The rake task validates MODE before PreSeedRendererCache.call validates mode:, creating two different error messages for the same bad input. Not a bug, but slight duplication.
• RENDERER_CACHE_DEPLOY_SCRIPT_PATHS could use private_constant if you want to prevent it from leaking into the public module API.
• The changelog "Changed" entry could link to this PR number for traceability.

Generated with Claude Code

[justin808]

Review feedback addressed

All 11 review threads addressed and resolved. Summary:

Addressed in new commit 1f9e79681

• pre_seed_renderer_cache.rb — make_relative_symlink now rescues Errno::ENOENT from Pathname#realpath and surfaces a clear ReactOnRailsPro::Error ("Cannot resolve real path for …") so dangling symlinks / mid-stage webpack rotations no longer leak a raw system error. (@claude[bot] inline on line 130)
• prepare_node_renderer_bundles.rb — class-level @deprecation_mutex wraps the check-then-set on @deprecation_warned so concurrent callers (multi-worker Puma boots of the shim) see exactly one warning per process. (@greptile-apps[bot])
• doctor.rb — check_deprecated_renderer_cache_task now guards File.read behind File.size < 1 MiB so unusually large deploy scripts are skipped defensively. (@claude[bot])

Already addressed in earlier commit 14f4a45ae (prior review round, verified applied on current tip)

• doctor.rb — migration-command suggestion is tailored per matched file: Dockerfile* gets rake react_on_rails_pro:pre_seed_renderer_cache (copy-mode default), Procfile* / bin/* scripts get MODE=symlink. Avoids the Dockerfile-as-symlinks footgun. (@chatgpt-codex-connector[bot])
• pre_seed_renderer_cache.rb — copy-mode env-var error now includes a MODE=symlink escape hatch line. (@claude[bot])
• prepare_node_renderer_bundles.rb — reset_deprecation_warned! is private_class_method; spec reaches it via .send(...). (@claude[bot])
• assets.rake — validates the raw MODE string against VALID_MODES (aborts with a friendly message) before to_sym, and downcases for case-insensitive input. Covers both @claude[bot]'s "arbitrary symbol" concern and @greptile-apps[bot]'s "MODE=Copy should work" concern.
• pre_seed_renderer_cache_spec.rb — added a mode: :symlink example that exercises stage_assets (asserts each asset is a symlink with correct realpath). The copy-mode env-var spec's ensure now deletes RENDERER_SERVER_BUNDLE_CACHE_PATH alongside the tmpdir cleanup.

Skipped

• @greptile-apps[bot] comment on spec:83-88 was a duplicate of @claude[bot]'s comment on the same lines; replied and resolved.

All 11 threads now resolved. Lint + Pro cache specs green locally. (I — Claude Code — posted this on Justin's behalf.)

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f9e79681f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review

Overall: Clean, well-structured refactoring. The unified mode: API is the right shape, the deprecation path is safe, and the doctor check is a nice DX touch. A few items to address.

Bug: make_relative_symlink outer rescue has a misleading message

The inner begin/rescue (lines 139-145 of pre_seed_renderer_cache.rb) correctly converts Errno::ENOENT from Pathname.new(source).realpath into a ReactOnRailsPro::Error, which escapes the outer rescue. However, destination_dir.realpath on line 146 sits outside the inner block. If it raises Errno::ENOENT (e.g., race condition where another process removes the dir between mkdir_p and realpath), the method-level outer rescue catches it and blames the symlink source — which is wrong. The outer rescue message should account for this, or destination_dir.realpath should be wrapped in its own rescue.

Style: Constants placed between methods in doctor.rb

DEPRECATED_RENDERER_CACHE_TASK, RENDERER_CACHE_DEPLOY_SCRIPT_PATHS, and RENDERER_CACHE_DEPLOY_SCRIPT_MAX_BYTES appear between method definitions (~lines 2730-2741). Valid Ruby, but breaks the convention used throughout doctor.rb where constants sit near the top of the class.

Good patterns worth keeping

• Thread-safe one-per-process warning in PrepareNodeRenderBundles (@deprecation_mutex + @deprecation_warned) is exactly right for Puma multi-worker boot scenarios.
• ENV[...].present? in enforce_cache_dir_env_var! correctly rejects empty-string env vars (not just nil) — a common footgun with plain ENV checks.
• Private reset_deprecation_warned! test helper (accessed via send) keeps spec examples isolated without exposing the reset to production code.
• Guard clause ordering in enforce_cache_dir_env_var! (mode → env-var → Rails.env) is the right early-return precedence.

Minor

• The rake task validates MODE before calling PreSeedRendererCache.call, which also validates mode: internally. Intentional double-validation for nicer UX at the rake boundary, but the ArgumentError inside the Ruby method is unreachable via the rake path — just worth knowing.
• RENDERER_CACHE_DEPLOY_SCRIPT_MAX_BYTES = 1_048_576 with >= silently skips files of exactly 1 MiB. Consider > or add a comment that the conservative boundary is intentional.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Unprotected symlink retry can raise EEXIST in race
  • I wrapped the retry-path symlink creation in an Errno::EEXIST rescue that treats a concurrently recreated matching symlink as success and re-raises only true conflicts.

Or push these changes by commenting:

@cursor push 9b777e6fce

Preview (9b777e6fce)

diff --git a/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb b/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
--- a/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
+++ b/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
@@ -187,7 +187,17 @@
       end
 
       FileUtils.rm_f(destination)
-      File.symlink(relative_source_path, destination)
+      begin
+        File.symlink(relative_source_path, destination)
+      rescue Errno::EEXIST
+        if File.symlink?(destination) && File.readlink(destination) == relative_source_path.to_s
+          puts "[ReactOnRailsPro] Symlink already present at #{destination} " \
+               "(concurrent creator won the replacement race); leaving existing link."
+          return
+        end
+
+        raise
+      end
       puts "[ReactOnRailsPro] #{log_prefix}: #{relative_source_path} -> #{destination} " \
            "(replaced stale symlink)"
     end

diff --git a/react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb b/react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb
--- a/react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb
+++ b/react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb
@@ -119,6 +119,31 @@
       expect(File.realpath(dest_file)).to eq(server_bundle_path)
     end
 
+    it "treats EEXIST during replacement as success when the concurrent link matches" do
+      stale_source = "stale-server-bundle.js"
+      create_stale_link = true
+      simulate_second_race = true
+      allow(File).to receive(:symlink).and_wrap_original do |original, source, destination|
+        if create_stale_link
+          create_stale_link = false
+          original.call(stale_source, destination)
+          raise Errno::EEXIST
+        end
+
+        if simulate_second_race
+          simulate_second_race = false
+          original.call(source, destination)
+          raise Errno::EEXIST
+        end
+
+        original.call(source, destination)
+      end
+
+      expect { described_class.call(mode: :symlink) }.not_to raise_error
+      dest_file = File.join(bundle_dir, "#{bundle_hash}.js")
+      expect(File.realpath(dest_file)).to eq(server_bundle_path)
+    end
+
     it "logs mode-accurate prefixes (Pre-staged / Symlinked) instead of copy-oriented wording" do
       FileUtils.cp(fixture_path, path_in_webpack_folder(asset_filename))
       FileUtils.cp(fixture_path2, path_in_webpack_folder(asset_filename2))

_{You can send follow-ups to the cloud agent here.}

[claude]

Code Review

Overview

This PR cleanly unifies two separate cache-staging classes (PreSeedRendererCache and PrepareNodeRenderBundles) into a single entry point with a mode: keyword argument. The approach is sound — same on-disk layout, only the file operation differs. The deprecation strategy (shim + doctor check + once-per-process warning) is well thought out and backward-compatible.

What's well done

• Validation: ArgumentError on unknown mode, ReactOnRailsPro::Error for misconfigured envs — correct error types at the right layer.
• ENV var guard in copy mode: The enforce_cache_dir_env_var! guard with .strip.empty? (avoiding ActiveSupport dependency) is a nice touch and addresses a real misconfig footgun.
• Thread safety in PrepareNodeRenderBundles: The Mutex-guarded one-time deprecation warning is correct for multi-worker Puma/Unicorn scenarios.
• Symlink robustness: Canonicalizing both sides with realpath before computing the relative path is the right fix for macOS /var → /private/var aliasing.
• Test coverage: The boundary condition specs (exact size gate, concurrent matching symlink, concurrent mismatched symlink, filesystem error in doctor) are thorough and clearly written.
• Doctor check: Rails.root.join (not Dir.pwd) ensures the scan works regardless of working directory — good attention to detail.

Issues & suggestions

Bug: unhandled Errno::EEXIST in replace_existing_symlink (line 190)

The replace_existing_symlink method handles a stale-link case with rm_f + File.symlink, but the second File.symlink call is not rescue-guarded. In a tight concurrent scenario — another process creates the same destination between the rm_f at line 189 and the File.symlink at line 190 — the call raises Errno::EEXIST and propagates uncaught. The matching-symlink check at line 183 only runs before the rm_f, not after it; a concurrent creator between those two lines would result in an unhandled exception. Since the first EEXIST path is already handled, the second one should be too (swallow it if the resulting link is correct, or raise with context if it isn't).

Minor: rm_f runs before source realpath is verified (line 147 vs 157)

FileUtils.rm_f(destination) deletes the destination before the source realpath call confirms the source still exists. If the source was removed between the File.exist? guard in stage_assets and make_relative_symlink's realpath, the destination is left absent. The PR comment notes the renderer's 410→retry recovers, so this is low severity, but reversing the order (verify source path first, then rm_f) would avoid the gap entirely.

Nit: substring match in doctor check can fire on commented-out lines

full_path.read.include?(DEPRECATED_RENDERER_CACHE_TASK) will trigger on Procfile/Dockerfile lines that are commented out (# was: pre_stage_bundle_for_node_renderer). The PR acknowledges this ("worst case is a benign migration nudge"), and I agree it's acceptable — just worth a note in the CHANGELOG or docs so users know why the warning fires on already-migrated files.

Nit: MODE env var default inconsistency between rake task and auto-invocation

rake react_on_rails_pro:pre_seed_renderer_cache defaults MODE to "copy", but AssetsPrecompile.call always uses mode: :symlink. This is the correct behavior, but the rake task description ("MODE=copy (default; Docker/image builds)") could make users think assets:precompile uses copy mode too. The docs cover this, but a one-liner in the rake task desc like "(assets:precompile uses symlink automatically)" would prevent confusion.

Test coverage gaps

• No spec covering the second Errno::EEXIST in replace_existing_symlink (the rm_f + concurrent re-create scenario).
• Doctor specs cover Procfile and Dockerfile.production but not bin/deploy, bin/release, or bin/docker-entrypoint paths — worth adding at least one bin/ case to confirm the MODE=symlink suggestion path.

Summary

Solid refactor with good backward-compatibility planning. The one actionable fix is the unhandled Errno::EEXIST in replace_existing_symlink; the rest are minor. The overall structure is clean and the test suite is well above average quality for this kind of file-system-heavy code.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Inline symlink check duplicates matching_symlink? helper
  • I replaced the duplicated inline symlink comparison with matching_symlink? in replace_existing_symlink so both call sites use the shared helper.

Or push these changes by commenting:

@cursor push d4b3b905ce

Preview (d4b3b905ce)

diff --git a/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb b/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
--- a/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
+++ b/react_on_rails_pro/lib/react_on_rails_pro/pre_seed_renderer_cache.rb
@@ -181,7 +181,7 @@
     private_class_method :make_relative_symlink
 
     def self.replace_existing_symlink(destination, relative_source_path, log_prefix)
-      if File.symlink?(destination) && File.readlink(destination) == relative_source_path.to_s
+      if matching_symlink?(destination, relative_source_path)
         puts "[ReactOnRailsPro] Symlink already present at #{destination} " \
              "(concurrent creator won the race); leaving existing link."
         return

_{You can send follow-ups to the cloud agent here.}

^{Reviewed by Cursor Bugbot for commit 25826d0. Configure here.}

[justin808]

Mattered:

• Addressed renderer-cache staging review feedback through c1db2a7, acd4bbd, 25826d0, and 95b271c: deploy-script scan coverage, normalized MODE reporting, env guard ordering, mode-aware symlink logs, exact-size doctor scan behavior, stale symlink replacement on EEXIST, symlink source resolution before destination removal, second-EEXIST handling in stale-symlink replacement, shared matching_symlink? usage, changelog attribution, Dockerfile/runtime migration comments, deprecated env-var migration note, and the doctor rescue-branch spec.
• Replied to and resolved all unresolved refactor: unify renderer cache staging with mode: :copy / :symlink #3167 review threads.

Skipped:

• Did not restore the blob/main GitHub URL for react_on_rails_pro/spec/dummy/client/node-renderer.js because that URL is currently a live 404 until this stack lands; the docs now use repository path text instead of a stale pinned URL.

Future full-PR scans should start after this comment unless you say check all reviews.

[justin808]

Mattered:

• Addressed the selected docs-link decision in b4cf875 by restoring the testing-example link to the evergreen blob/main URL for the current react_on_rails_pro/spec/dummy/renderer/node-renderer.js path.
• Items 1 and 3-7 were already present on the fast-forwarded PR head before b4cf875: stale symlink replacement on EEXIST plus specs, changelog PR attribution, simplified tmpdir cleanup, Dockerfile/runtime migration rationale, deprecated env-var note, and hoisted action_description.
• Removed one stale RuboCop disable in Pro test support after the CI-aligned Pro RuboCop command flagged it.
• Replied on the updated docs-link thread and on skipped/duplicate threads; all selected review threads remain resolved.

Skipped:

• #3128152115: duplicate of the later P1 symlink-race thread; replied with the existing EEXIST target-verification fix.
• #3128192168: duplicate of the docs-link request; replied that b4cf875 restored the blob/main URL.
• #3128191869: no further change needed in this pass; current branch already stubs Pathname#read directly, and Ruby 3.4.8 also verifies the original File.read stub was not silently missing.

Validation:

• cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/pre_seed_renderer_cache_spec.rb — 25 examples, 0 failures.
• pnpm exec prettier --check docs/oss/building-features/node-renderer/js-configuration.md — passed.
• cd react_on_rails && bundle exec rubocop — passed.
• cd react_on_rails_pro && bundle exec rubocop --ignore-parent-exclusion — passed after the stale-disable cleanup.
• Pre-commit and pre-push hooks passed, including changed-file RuboCop, Prettier, and Lychee checks.

Run context: this action used the selected items from the latest triage after the previous summary checkpoint at 2026-04-23T04:12:40Z. Future full-PR scans should start after this comment unless check all reviews is requested.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0752feb936

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[justin808]

Mattered:

• Completed the selected review fixes through b4cf8754a, 0752feb93, and f511867a7.
• Restored the node-renderer testing example link to the evergreen blob/main URL for the current react_on_rails_pro/spec/dummy/renderer/node-renderer.js path.
• Kept the symlink EEXIST target-verification/replacement work, changelog PR attribution, tmpdir cleanup simplification, Docker/runtime rationale, deprecated env-var note, and hoisted action_description fixes from the fast-forwarded PR head.
• Reworked the Pro mock_block helper to use a real spy object instead of a plain RSpec double, fixing the GitHub pro-lint-js-and-ruby failure from the prior push.
• Fixed the new Codex P2 directory-scan thread in f511867a7: deprecated renderer-cache task scanning now requires Pathname#file? before size/read, so directories such as bin/deploy are skipped while real files continue to be scanned.

Review follow-up:

• Replied to skipped/duplicate threads from the previous pass.
• Replied to and resolved the new Codex directory-scan thread: refactor: unify renderer cache staging with mode: :copy / :symlink #3167 (comment)
• Ran the requested follow-up review scan more than 10 minutes after the final push; no new unresolved review threads, issue comments, or bot reviews appeared after this checkpoint window.

Validation:

• cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/pre_seed_renderer_cache_spec.rb — 25 examples, 0 failures.
• cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/stream_spec.rb spec/react_on_rails_pro/request_spec.rb spec/react_on_rails_pro/stream_decorator_spec.rb — 60 examples, 0 failures.
• cd react_on_rails && bundle exec rspec spec/lib/react_on_rails/doctor_spec.rb:2471 — 7 examples, 0 failures.
• cd react_on_rails && bundle exec rubocop — passed.
• cd react_on_rails_pro && bundle exec rubocop --ignore-parent-exclusion — passed.
• pnpm exec prettier --check docs/oss/building-features/node-renderer/js-configuration.md — passed.
• Pre-commit and pre-push hooks passed, including branch Ruby RuboCop and Lychee markdown-link checks.

CI as of this checkpoint:

• All actionable GitHub checks have passed except the Claude review check, which remains red because it is a review-status check.
• rspec-package-tests (3.4, latest, generators) is still pending in GitHub after an extended wait; GitHub reports it in the Run rspec tests step with no failure log available yet.

Future full-PR scans should start after this comment unless check all reviews is requested.

[justin808]

CI update after the previous checkpoint:

• rspec-package-tests (3.4, latest, generators) finished successfully at 2026-04-23T06:06:13Z.
• All actionable GitHub checks are now green on f511867a7.
• The only red check is claude-review; no unresolved review threads remain, and the post-push review scan found no new actionable feedback.

Future full-PR scans should start after this comment unless check all reviews is requested.