Skip to content

Move Node Renderer entry point to renderer/ directory#3165

Merged
justin808 merged 10 commits intomainfrom
jg/3073-renderer-entry-path
Apr 19, 2026
Merged

Move Node Renderer entry point to renderer/ directory#3165
justin808 merged 10 commits intomainfrom
jg/3073-renderer-entry-path

Conversation

@justin808
Copy link
Copy Markdown
Member

@justin808 justin808 commented Apr 18, 2026

Summary

Establishes renderer/node-renderer.js as the canonical location for the Pro Node Renderer entry point. Previously the path was inconsistent across docs (node-renderer.js, client/node-renderer.js, or react-on-rails-pro-node-renderer.js).

Using a dedicated top-level renderer/ directory also makes it trivial to exclude from production Docker builds that strip JS sources after bundling — the Node Renderer process still needs its entry file at runtime.

Changes

  • Generator (pro_setup.rb, base_generator.rb, demo_page_config.rb, USAGE, initializer template) now creates the entry at renderer/node-renderer.js
  • Template file moved to templates/pro/base/renderer/node-renderer.js
  • Pro spec/dummy — renderer file, Procfile.dev, Procfile.prod, package.json scripts, .controlplane/rails.yml, and benchmarks/bench-node-renderer.rb updated
  • All 13 docs now use renderer/node-renderer.js consistently; the Docker-cleanup rationale is added to the container deployment guide
  • Generator specs updated; 13 renderer-related examples pass

Existing apps are unaffected — the generator skips files that already exist, so upgrades keep their current client/node-renderer.js.

Test plan

  • rubocop passes on all changed Ruby files
  • prettier --check passes on all changed docs + JS
  • Renderer-related generator specs all pass (rspec ... --example "creates node-renderer" --example "adds node-renderer process" --example "does not overwrite existing", 13 examples)
  • Full pro_generator_spec.rb passes (79 examples)
  • Link-check (lychee) passes on all modified docs
  • Review rendered markdown

Fixes #3073


Note

Medium Risk
Moderate risk because it changes the Pro generator’s output paths and Procfile wiring for SSR, which could break dev/prod startup if projects rely on the old client/node-renderer.js location or have customized Procfiles.

Overview
Pro installs now standardize the Node Renderer entrypoint at renderer/node-renderer.js (instead of client/node-renderer.js), including updated generator hints, initializer comments, and a new template under templates/pro/base/renderer/.

The Pro generator adds safer upgrade behavior: it detects an existing legacy client/node-renderer.js, avoids overwriting it, conditionally skips/avoids duplicating Procfile.dev entries, and emits targeted warnings when a Procfile still points at the legacy path.

Docs, dummy app scripts/Procfiles, deployment examples (Docker/K8s/Heroku), benchmarks, and generator specs are updated to reference the new renderer/ path consistently, with added documentation explaining the Docker build rationale.

Reviewed by Cursor Bugbot for commit 20633e7. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Changed
    • Pro generator now places the Node renderer in a dedicated top-level renderer directory, preserves existing legacy renderer files, and adds a configurable server-bundle cache location.
  • Documentation
    • Updated guides, examples, deployment and profiling docs, and sample startup scripts to reference the new renderer location.
  • Tests
    • Added/updated generator tests to cover legacy vs. new renderer presence and Procfile/script handling.

justin808 and others added 2 commits April 17, 2026 17:42
Establishes `renderer/node-renderer.js` as the canonical location for the
Pro Node Renderer entry point. Previously the path was inconsistent across
docs (`node-renderer.js`, `client/node-renderer.js`, or
`react-on-rails-pro-node-renderer.js`).

Using a dedicated top-level `renderer/` directory makes it trivial to
exclude from production Docker builds that strip JS sources after bundling
— the Node Renderer process still needs its entry file at runtime.

Changes:
- Generator (`pro_setup.rb`, `base_generator.rb`, `demo_page_config.rb`,
  USAGE, initializer template) now creates the entry at `renderer/node-renderer.js`
- Template file moved to `templates/pro/base/renderer/node-renderer.js`
- Pro `spec/dummy` renderer file, Procfile.dev, Procfile.prod,
  package.json scripts, .controlplane/rails.yml, and benchmark reference updated
- All 13 docs now use `renderer/node-renderer.js` consistently
- Container deployment doc explains the rationale for the dedicated directory
- Generator specs updated; 13 renderer-related specs pass

Existing apps are unaffected — the generator skips files that already
exist, so upgrades keep their current `client/node-renderer.js`.

Fixes #3073

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous link to main-branch GitHub URL fails link-check on the PR
branch because the file path has changed. Relative link works across all
branches and avoids the chicken-and-egg check on the renamed path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 18, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Change the canonical Node renderer entrypoint to renderer/node-renderer.js across docs, generators, templates, dummy app Procfiles, and related scripts; the Pro generator now creates a top-level renderer/ directory, prefers the new path, and skips Procfile changes when a legacy client/node-renderer.js is detected.

Changes

Cohort / File(s) Summary
Changelog & Benchmarks
CHANGELOG.md, benchmarks/bench-node-renderer.rb, react_on_rails_pro/.controlplane/rails.yml
Documented path change; benchmark loader and control-plane container args updated to reference renderer/node-renderer.js.
Documentation
docs/.../generator-details.md, docs/.../code-splitting.md, docs/.../node-renderer/basics.md, docs/.../node-renderer/container-deployment.md, docs/.../node-renderer/debugging.md, docs/.../node-renderer/heroku.md, docs/.../node-renderer/js-configuration.md, docs/.../deployment/docker-deployment.md, docs/.../migrating/..., docs/pro/.../installation.md, docs/pro/.../js-memory-leaks.md, docs/pro/.../profiling-server-side-rendering-code.md, packages/react-on-rails-pro-node-renderer/README.md
Replaced legacy references (client/node-renderer.js, root node-renderer.js, etc.) with canonical renderer/node-renderer.js in examples, commands, manifests, and notes.
Generators & Templates
react_on_rails/lib/generators/.../base_generator.rb, .../demo_page_config.rb, .../pro/USAGE, .../pro_setup.rb, .../templates/.../react_on_rails_pro.rb.tt, .../templates/pro/base/renderer/node-renderer.js
Generator logic and templates updated to target renderer/node-renderer.js; pro_setup now creates renderer/, copies new template, returns a status to avoid overwriting, detects legacy client/node-renderer.js, and conditionally updates Procfile.dev. Added serverBundleCachePath to template config.
Dummy App & Runtime Scripts
react_on_rails_pro/spec/dummy/Procfile.dev, react_on_rails_pro/spec/dummy/Procfile.prod, react_on_rails_pro/spec/dummy/package.json
Dummy Procfiles and npm scripts updated to run node renderer/node-renderer.js.
Tests / Specs
react_on_rails/spec/.../install_generator_spec.rb, react_on_rails/spec/.../pro_generator_spec.rb, react_on_rails/spec/.../pro_generator_examples.rb, react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
Test expectations adjusted to renderer/node-renderer.js; added integration contexts verifying legacy client/node-renderer.js handling and correct Procfile behavior (avoid duplicates, append when needed).
Repo Packaging / Control Plane
packages/react-on-rails-pro-node-renderer/README.md, react_on_rails_pro/.controlplane/rails.yml
Readme and control-plane startup args updated to use renderer/node-renderer.js.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Developer
  participant Gen as Pro Generator
  participant FS as Filesystem
  participant Proc as Procfile

  rect rgba(100,149,237,0.5)
    Dev->>Gen: run generator (--pro / --force)
    Gen->>FS: check `renderer/node-renderer.js`
    alt renderer exists
      FS-->>Gen: exists -> return (noop)
    else
      Gen->>FS: check `client/node-renderer.js` (legacy)
      alt legacy exists
        FS-->>Gen: legacy found -> log hint
        Gen->>Proc: do NOT add node-renderer entry
      else
        Gen->>FS: create `renderer/` and copy template -> `renderer/node-renderer.js`
        Gen->>Proc: add `node-renderer:` entry to Procfile.dev
      end
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • AbanoubGhadban
  • alexeyr-ci2

Poem

🐰 I hopped through docs and template nests,
Placed the renderer where it safely rests.
renderer/node-renderer.js leads the show,
Old client/ files stay if they must grow.
A tiny hop for code — joy in every test.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Move Node Renderer entry point to renderer/ directory' is clear, concise, and accurately summarizes the primary change: relocating the Node Renderer entry point from client/ to renderer/.
Linked Issues check ✅ Passed The PR fully implements the objectives from issue #3073: establishes renderer/ as the canonical path, updates all 13 documentation files consistently, modifies generator templates and Pro dummy app assets, and preserves existing installs by detecting legacy client/node-renderer.js files.
Out of Scope Changes check ✅ Passed All changes are directly related to the renderer entry point migration: documentation updates, generator modifications, template relocation, spec updates, and example app assets. One minor addition (serverBundleCachePath field in the template) is incidental to the new file location and not out of scope.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg/3073-renderer-entry-path

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

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

@coderabbitai coderabbitai Bot added review-needed documentation docs-cleanup Documentation cleanup or migration P3 Parked priority labels Apr 18, 2026
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 18, 2026

Greptile Summary

This PR establishes renderer/node-renderer.js as the canonical Pro Node Renderer entry point, replacing the inconsistent client/node-renderer.js path across all generators, templates, docs, spec/dummy, and deployment configs. The generator gains legacy-compatibility logic: if client/node-renderer.js already exists it is preserved and Procfile modification is skipped, so existing apps are unaffected on upgrade.

Confidence Score: 5/5

Safe to merge — path rename is contained, legacy path detection is correct, and add_pro_to_procfile already guards against duplicate Procfile entries.

All 30 changed files are consistent path substitutions or the new legacy-detection logic, which is correctly implemented and thoroughly tested (79 generator examples + 3 new legacy-context examples). No P0 or P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
react_on_rails/lib/generators/react_on_rails/pro_setup.rb Core generator logic correctly implements new path, legacy detection, and conditional Procfile update; add_pro_to_procfile already deduplicates on 'node-renderer:' so re-runs are safe.
react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb New context covers all three legacy-detection outcomes (no new file, preserved old file, no Procfile mutation); thorough and well-structured.
react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb Existing 'already exists' spec migrated to the new renderer/ path; existing-file simulation updated accordingly.
react_on_rails/lib/generators/react_on_rails/templates/pro/base/renderer/node-renderer.js Template file renamed from client/ to renderer/; content unchanged (100% similarity).
react_on_rails_pro/spec/dummy/renderer/node-renderer.js Spec-dummy file renamed from client/ to renderer/; content unchanged.
react_on_rails_pro/spec/dummy/Procfile.dev Node-renderer line updated to renderer/node-renderer.js; consistent with new canonical path.
docs/oss/building-features/node-renderer/container-deployment.md All client/node-renderer.js references updated; adds rationale callout explaining why a dedicated renderer/ directory aids Docker cleanup.
react_on_rails_pro/.controlplane/rails.yml Node-renderer command arg updated; also adds a missing trailing newline at end of file.
benchmarks/bench-node-renderer.rb Path updated to renderer/node-renderer.js; function logic unchanged.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([run pro generator]) --> B[create_pro_initializer]
    B --> C[create_node_renderer]
    C --> D{renderer/node-renderer.js already exists?}
    D -- yes --> E[skip, return false]
    D -- no --> F{client/node-renderer.js legacy exists?}
    F -- yes --> G[preserve legacy, return true]
    F -- no --> H[empty_directory renderer/ / copy template / return false]
    E --> I{legacy_renderer_detected?}
    G --> I
    H --> I
    I -- false --> J[add_pro_to_procfile / check for duplicate entry]
    I -- true --> K[skip Procfile update]
    J --> L[update_webpack_config_for_pro]
    K --> L
Loading

Reviews (2): Last reviewed commit: "Guard Procfile update when legacy render..." | Re-trigger Greptile

Comment thread docs/oss/building-features/node-renderer/js-configuration.md
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
Comment thread docs/oss/building-features/node-renderer/js-configuration.md
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

Code Review

Overview

Clean, well-scoped PR. Moving the Node Renderer entrypoint to a dedicated renderer/ directory is a good idea — it makes Docker cleanup straightforward and gives the file a more semantically meaningful home. All 13 docs, the spec/dummy app, generator code, and tests are consistently updated.


Issues

1. Upgrade behavior gap — existing client/node-renderer.js not detected (medium)

The skip check in create_node_renderer only tests for renderer/node-renderer.js. An existing Pro user who re-runs the generator while still on client/node-renderer.js will silently get a second renderer file created at the new path — their customised config is overlooked and they now have two files. The Procfile guard (checks for any node-renderer: entry) prevents a duplicate process entry, but the duplicate file is confusing.

The PR description says "Existing apps are unaffected", which overstates the safety. See inline suggestion on pro_setup.rb:216.

2. Relative link to a file outside the docs tree (minor)

js-configuration.md changed from an absolute GitHub URL to a ../../../../react_on_rails_pro/... relative path. That path leaves the docs directory entirely and points to a .js file, which hosted docs systems (Docusaurus, VitePress, etc.) won't resolve — the link will work on GitHub but 404 on the rendered site. See inline suggestion to restore an absolute URL with the new path.

3. npm package README not updated (minor / out of scope)

packages/react-on-rails-pro-node-renderer/README.md still shows node node-renderer.js (no directory) as the quickstart. A developer following the generator output will have renderer/node-renderer.js and the README will be misleading. Worth a follow-up or an intentional note that the package README shows a project-root convention.


Positives

  • Generator logic is clean: FileUtils.mkdir_p ensures the renderer/ directory is created before copying, and the Procfile guard is already idempotent.
  • serverBundleCachePath uses path.resolve(__dirname, '../.node-renderer-bundles') — this resolves identically from client/ or renderer/, so no cache migration is needed for existing users.
  • Changelog, USAGE, and initializer template comment all updated consistently.
  • Generator specs cover both the new happy-path and the do-not-overwrite case.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/oss/building-features/node-renderer/basics.md (1)

68-81: ⚠️ Potential issue | 🟡 Minor

Convert the launcher example to CommonJS to match the documented approach.

Line 81 instructs users to run node renderer/node-renderer.js, but the snippet uses ESM import syntax with __dirname, which will fail in a default Node.js configuration. Per js-configuration.md, this file should use CommonJS: "The generator uses this filename and CommonJS syntax so the file runs directly with node renderer/node-renderer.js without extra ESM configuration."

📘 Proposed docs fix
-   import path from 'path';
-   import reactOnRailsProNodeRenderer from 'react-on-rails-pro-node-renderer';
+   const path = require('path');
+   const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer');
 
    const config = {
      serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'),
    };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/oss/building-features/node-renderer/basics.md` around lines 68 - 81, The
example launcher uses ESM import syntax which will fail when run with `node
renderer/node-renderer.js`; change the snippet to CommonJS by replacing the ESM
import with require (use require('path') and
require('react-on-rails-pro-node-renderer')), keep the config object with
serverBundleCachePath and __dirname, and invoke
reactOnRailsProNodeRenderer(config) directly (or export nothing); update the
snippet around the symbols reactOnRailsProNodeRenderer, config, and
serverBundleCachePath so it runs with plain `node renderer/node-renderer.js`.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb (1)

17-21: Consider asserting the Procfile command path too.

Since the PR standardizes the runnable entry point, this shared example would catch regressions if Procfile.dev pointed back to node-renderer.js or client/node-renderer.js.

Proposed test strengthening
   it "adds node-renderer process to Procfile.dev" do
     assert_file "Procfile.dev" do |content|
       expect(content).to include("node-renderer:")
       expect(content).to include("RENDERER_PORT=3800")
+      expect(content).to include("node renderer/node-renderer.js")
     end
   end
🤖 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/support/shared_examples/pro_generator_examples.rb`
around lines 17 - 21, Update the shared example in pro_generator_examples.rb
(the "adds node-renderer process to Procfile.dev" test) to also assert that
Procfile.dev uses the standardized runnable entry point (for example contains
the exact command path "bin/node-renderer" or whatever the project-standard
entry is) and to fail if legacy paths like "node-renderer.js" or
"client/node-renderer.js" are present; keep the existing checks for
"node-renderer:" and "RENDERER_PORT=3800" and add assertions inside the same
assert_file block that include the canonical command path and exclude the old
paths.
🤖 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 29: Update the CHANGELOG entry for the Pro generator so the tag is the
standalone prefix "**[Pro]**" and append the PR attribution using the required
format; locate the line containing "Pro generator now creates the Node Renderer
at `renderer/node-renderer.js`" and change its header to start with "**[Pro]**"
and add the PR link and author as `[PR
1818](https://github.com/shakacode/react_on_rails/pull/1818) by
[username](https://github.com/username)` (replace PR number and username with
the actual values).

---

Outside diff comments:
In `@docs/oss/building-features/node-renderer/basics.md`:
- Around line 68-81: The example launcher uses ESM import syntax which will fail
when run with `node renderer/node-renderer.js`; change the snippet to CommonJS
by replacing the ESM import with require (use require('path') and
require('react-on-rails-pro-node-renderer')), keep the config object with
serverBundleCachePath and __dirname, and invoke
reactOnRailsProNodeRenderer(config) directly (or export nothing); update the
snippet around the symbols reactOnRailsProNodeRenderer, config, and
serverBundleCachePath so it runs with plain `node renderer/node-renderer.js`.

---

Nitpick comments:
In
`@react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb`:
- Around line 17-21: Update the shared example in pro_generator_examples.rb (the
"adds node-renderer process to Procfile.dev" test) to also assert that
Procfile.dev uses the standardized runnable entry point (for example contains
the exact command path "bin/node-renderer" or whatever the project-standard
entry is) and to fail if legacy paths like "node-renderer.js" or
"client/node-renderer.js" are present; keep the existing checks for
"node-renderer:" and "RENDERER_PORT=3800" and add assertions inside the same
assert_file block that include the canonical command path and exclude the old
paths.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b050f35b-1cc0-4d9e-9c6c-a02974a90339

📥 Commits

Reviewing files that changed from the base of the PR and between e9e27a2 and dc4ea28.

📒 Files selected for processing (28)
  • CHANGELOG.md
  • benchmarks/bench-node-renderer.rb
  • docs/oss/api-reference/generator-details.md
  • docs/oss/building-features/code-splitting.md
  • docs/oss/building-features/node-renderer/basics.md
  • docs/oss/building-features/node-renderer/container-deployment.md
  • docs/oss/building-features/node-renderer/debugging.md
  • docs/oss/building-features/node-renderer/heroku.md
  • docs/oss/building-features/node-renderer/js-configuration.md
  • docs/oss/deployment/docker-deployment.md
  • docs/oss/migrating/rsc-preparing-app.md
  • docs/oss/migrating/rsc-troubleshooting.md
  • docs/pro/installation.md
  • docs/pro/js-memory-leaks.md
  • docs/pro/profiling-server-side-rendering-code.md
  • react_on_rails/lib/generators/react_on_rails/base_generator.rb
  • react_on_rails/lib/generators/react_on_rails/demo_page_config.rb
  • react_on_rails/lib/generators/react_on_rails/pro/USAGE
  • react_on_rails/lib/generators/react_on_rails/pro_setup.rb
  • react_on_rails/lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt
  • react_on_rails/lib/generators/react_on_rails/templates/pro/base/renderer/node-renderer.js
  • react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
  • react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
  • react_on_rails_pro/.controlplane/rails.yml
  • react_on_rails_pro/spec/dummy/Procfile.dev
  • react_on_rails_pro/spec/dummy/Procfile.prod
  • react_on_rails_pro/spec/dummy/package.json
  • react_on_rails_pro/spec/dummy/renderer/node-renderer.js

Comment thread CHANGELOG.md Outdated
justin808 and others added 2 commits April 17, 2026 18:35
- pro_setup.rb: skip `create_node_renderer` when the legacy
  `client/node-renderer.js` exists, emitting a migration hint so existing
  Pro users don't silently end up with two renderer files after rerunning
  the generator. Added a spec covering the legacy-path case.
- docs/oss/.../js-configuration.md: revert the testing-example link to the
  absolute GitHub URL so hosted docs platforms don't 404 on a relative
  path that leaves the docs tree.
- CHANGELOG.md: split `**[Pro]**` from the entry title and add the PR
  attribution to match the project's changelog guidelines.
- packages/.../react-on-rails-pro-node-renderer/README.md: update the
  quickstart to reference `renderer/node-renderer.js` so the README
  matches the new generator default.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The absolute URL 404s on main until this PR merges, so the pre-push
lychee check rejects it. Keep the relative link which resolves correctly
on GitHub file browsing; hosted docs are served via GitHub, so the
reviewer's concern about Docusaurus-style resolution does not apply.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@justin808
Copy link
Copy Markdown
Member Author

Review response summary

I (Claude Code) addressed the review feedback in two commits on this branch:

54c71c2 – Address PR #3165 review: legacy renderer check, docs/changelog polish

  • react_on_rails/lib/generators/react_on_rails/pro_setup.rb: create_node_renderer now also checks for the legacy client/node-renderer.js and skips with a migration hint, so Pro users rerunning the generator don't end up with two renderer files. Added a spec covering the legacy-path case. (addresses @claude[bot] and @cursor[bot])
  • CHANGELOG.md: split **[Pro]** from the entry title and added the [PR 3165](...) by [justin808](...) attribution per .claude/docs/changelog-guidelines.md. (addresses @coderabbitai[bot])
  • packages/react-on-rails-pro-node-renderer/README.md: updated the quickstart to reference renderer/node-renderer.js, aligning the npm package README with the new generator default. (addresses @claude[bot] general review)

3eaef8c – Revert docs link back to relative path for lychee compatibility

  • Kept docs/oss/building-features/node-renderer/js-configuration.md:61 on a relative link. Tried switching to the absolute GitHub URL per @greptile-apps[bot]/@claude[bot] suggestions, but react_on_rails_pro/spec/dummy/renderer/node-renderer.js doesn't exist on main until this PR merges, so the pre-push lychee check 404s. Docs for this repo are served directly via GitHub file browsing (not Docusaurus/VitePress), so the relative path resolves correctly in the rendered form. Those two threads are intentionally left unresolved pending your call.

Local checks: bundle exec rubocop clean on the changed files; bundle exec rspec spec/react_on_rails/generators/pro_generator_spec.rb – 81 examples, 0 failures.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 18, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of the key files

echo "=== CHANGELOG.md (Unreleased section) ==="
grep -A 20 "## \[Unreleased\]" CHANGELOG.md | head -30

echo ""
echo "=== pro_setup.rb (create_node_renderer method) ==="
grep -A 40 "def create_node_renderer" react_on_rails/lib/generators/react_on_rails/pro_setup.rb

echo ""
echo "=== js-configuration.md around line 61 ==="
sed -n '55,70p' docs/oss/building-features/node-renderer/js-configuration.md

echo ""
echo "=== pro_generator_spec legacy check ==="
grep -A 20 "legacy" react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb | head -40

Length of output: 7237


@justin808 — everything looks good. Here's my assessment of each item:


CHANGELOG.md
The entry is correctly formatted — **[Pro]** is on its own, the PR/author attribution is in place, and the description even mentions the legacy-path skip behavior, which is a nice touch.

create_node_renderer legacy check
The implementation is clean and well-ordered: it checks the new canonical path first, then the legacy client/node-renderer.js, and emits a clear migration hint instead of silently overwriting. The user-facing messaging (emoji indicators, actionable instructions) is a nice UX improvement.

Relative link in js-configuration.md line 61 ✅ — my call: keep the relative path
Your reasoning is correct. react_on_rails_pro/spec/dummy/renderer/node-renderer.js won't exist on main until this PR merges, so an absolute github.com URL would 404 for lychee on every pre-push check until then. Since these docs are served via GitHub file browsing (not a Docusaurus/VitePress pipeline), a relative link resolves correctly in rendered markdown both on this branch and post-merge. The relative path is the right call here; no change needed.


All the review threads have been addressed. The PR looks ready to merge. 🐇


🧠 Learnings used
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 0
File: :0-0
Timestamp: 2026-04-13T02:29:20.920Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/engine.rb` and `react_on_rails/lib/react_on_rails/configuration.rb` (PR `#3094`): Do NOT reset `rendering_strategy` or `js_code_builder` inside `ReactOnRails.configure`. The Pro engine sets these exactly once during `after_initialize`; resetting them on reconfigure would silently drop Pro's NodeStrategy/ProJsCodeBuilder assignments. A proper reconfigure mechanism for these accessors is deferred to Step 3 of the strategy-pattern refactor (`#2905`).

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/commonWebpackConfig.js : Changes to `commonWebpackConfig.js` affect all three webpack bundles (client, server, and RSC) in the Pro dummy app

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/packages/react-on-rails-pro-node-renderer/**/*.test.{ts,tsx,js,jsx} : Node renderer tests should be run with: `pnpm --filter react-on-rails-pro-node-renderer run test`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,tsx,jsx,json,css,md} : Pro Prettier check should be run with: `cd react_on_rails_pro && pnpm run prettier --check .`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/react_on_rails_pro-node-renderer/package.json : The `protocolVersion` in node renderer package.json must match gem expectations

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 2850
File: docs/oss/api-reference/rails-view-rendering-from-inline-javascript.md:0-0
Timestamp: 2026-03-26T07:01:43.653Z
Learning: In `packages/react-on-rails/src/ClientRenderer.ts`, the duplicate-render guard (which skips rendering if the DOM node is already connected/mounted) applies only to the automatic page-load rendering path. The public `ReactOnRails.render(...)` API routes through `packages/react-on-rails/src/base/client.ts` and calls `reactHydrateOrRender(...)` directly, without any deduplication guard. Therefore, repeated calls to the public `ReactOnRails.render(...)` API on the same DOM node (e.g., `#root`) WILL invoke React again — they are NOT deduplicated. Documentation stating this behavior is correct and should not be flagged.

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2284
File: react_on_rails/lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt:10-10
Timestamp: 2026-02-12T20:43:26.843Z
Learning: In React on Rails Pro initializer templates (e.g., `react_on_rails_pro.rb.tt`), the `config.renderer_password` uses `ENV.fetch("RENDERER_PASSWORD", "devPassword")` as a development starter configuration. This pattern is intentional: the node renderer requires explicit production deployment setup, and adding a production-only raise would break generator execution since initializers are loaded during `rails generate`. The pattern is consistent with existing manual setup documentation.

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 0
File: :0-0
Timestamp: 2026-02-22T17:08:48.268Z
Learning: In script/ci-changes-detector, the flags `run_pro_node_renderer_tests` and `run_pro_dummy_tests` written to GITHUB_OUTPUT are informational only (for local JSON mode and future use). The actual CI job gating uses Option A routing: PRO_NODE_RENDERER_CHANGED triggers RUN_PRO_LINT and RUN_PRO_TESTS, which are already declared as workflow outputs. No workflow YAML changes are needed.

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/spec/**/*.rb : Pro integration tests should be run with: `cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/react_on_rails_pro/**/*.rb : Pro gem unit tests should be run with: `cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: The Node renderer is a standalone Fastify HTTP server (separate Node.js process) that can be started locally with: `cd react_on_rails_pro/spec/dummy && pnpm run node-renderer` (port 3800)

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 2464
File: packages/react-on-rails-pro-node-renderer/src/worker.ts:343-393
Timestamp: 2026-02-21T20:46:38.404Z
Learning: In packages/react-on-rails-pro-node-renderer, the `Promise.all` pattern for per-bundle operations (e.g., in `/upload-assets` endpoint and `handleNewBundlesProvided` in handleRenderRequest.ts) is intentional. Copy operations like `copyUploadedAssets` are idempotent and retries are safe, so fast-fail behavior on lock acquisition or copy errors is acceptable. Switching to `Promise.allSettled` to surface partial success would require Ruby-side protocol changes.

@coderabbitai coderabbitai Bot added enhancement P2 Backlog priority labels Apr 18, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb (1)

1226-1255: Consider asserting the migration hint is emitted.

The PR description notes the generator "emits migration hint" when it detects client/node-renderer.js. This spec verifies the two file-system outcomes (renderer not created, legacy preserved) but does not verify the user-facing migration message, which is arguably the most important signal to the developer in this skip path. Adding a third expectation would guard against the hint silently regressing.

♻️ Suggested addition
     it "preserves the legacy client/node-renderer.js" do
       expect(File.read(File.join(destination_root, "client/node-renderer.js"))).to eq(legacy_renderer_content)
     end
+
+    it "emits a migration hint referencing the new renderer path" do
+      expect(GeneratorMessages.messages.join("\n")).to include("renderer/node-renderer.js")
+    end
   end

Adjust the substring to match the exact hint wording used by the generator.

🤖 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/generators/pro_generator_spec.rb` around
lines 1226 - 1255, Add an expectation in this spec block to assert the generator
emitted the user-facing migration hint when it detects the legacy
client/node-renderer.js: after the Dir.chdir { run_generator(["--force"]) }
invocation, assert that the captured generator output (from run_generator)
includes the exact migration hint string the generator prints; locate the spec
context using the existing example (the context block and run_generator call)
and match the substring to the exact wording used by the generator to avoid
false negatives.
🤖 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/spec/react_on_rails/generators/pro_generator_spec.rb`:
- Around line 1226-1255: Add an expectation in this spec block to assert the
generator emitted the user-facing migration hint when it detects the legacy
client/node-renderer.js: after the Dir.chdir { run_generator(["--force"]) }
invocation, assert that the captured generator output (from run_generator)
includes the exact migration hint string the generator prints; locate the spec
context using the existing example (the context block and run_generator call)
and match the substring to the exact wording used by the generator to avoid
false negatives.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 47afa30b-d9df-408d-aa50-e74d69f6bb30

📥 Commits

Reviewing files that changed from the base of the PR and between dc4ea28 and 3eaef8c.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • packages/react-on-rails-pro-node-renderer/README.md
  • react_on_rails/lib/generators/react_on_rails/pro_setup.rb
  • react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb
✅ Files skipped from review due to trivial changes (2)
  • packages/react-on-rails-pro-node-renderer/README.md
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • react_on_rails/lib/generators/react_on_rails/pro_setup.rb

Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
Comment thread react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

Code Review

Overall: Clean, well-scoped PR. The path migration is consistent across generators, docs, specs, and the dummy app, and the backward-compatibility guard for client/node-renderer.js is the right call. One medium bug to fix before merge, plus two smaller items.


Bug (medium) — wrong Procfile.dev entry written for legacy apps

setup_pro calls create_node_renderer and then add_pro_to_procfile unconditionally:

create_node_renderer   # may return early when legacy file detected
add_pro_to_procfile    # still runs

When client/node-renderer.js exists and Procfile.dev does not already contain node-renderer:, add_pro_to_procfile appends:

node-renderer: ... node renderer/node-renderer.js

...pointing to a file that was deliberately not created. The app would fail to start. See inline comment on pro_setup.rb lines 222-226 for suggested fixes.

The legacy-detection test in pro_generator_spec.rb simulates this exact setup (Procfile.dev without node-renderer:) but never asserts the Procfile state, so the bug is invisible to the test suite. See inline comment on lines 1248-1254.


Minor — FileUtils.mkdir_p bypasses Thor dry-run

FileUtils.mkdir_p(File.join(destination_root, "renderer")) executes even when the generator is run with --pretend. Thor's empty_directory "renderer" participates in the generator lifecycle correctly and is a no-op when the directory already exists. See inline comment on line 230.


Looks good

  • Backward-compat guard logic in create_node_renderer is correct for the happy-path upgrade.
  • Template moved (not duplicated), template content unchanged.
  • All 13 doc files updated consistently; Docker rationale note in container-deployment.md is useful context.
  • Spec coverage for renderer/node-renderer.js-already-exists and new-install cases is solid.
  • Benchmark script and dummy app updated in sync.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3eaef8c. Configure here.

Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 18, 2026

size-limit report 📦

Path Size
react-on-rails/client bundled (gzip) 62.76 KB (0%)
react-on-rails/client bundled (gzip) (time) 62.76 KB (0%)
react-on-rails/client bundled (brotli) 53.86 KB (0%)
react-on-rails/client bundled (brotli) (time) 53.86 KB (0%)
react-on-rails-pro/client bundled (gzip) 63.71 KB (0%)
react-on-rails-pro/client bundled (gzip) (time) 63.71 KB (0%)
react-on-rails-pro/client bundled (brotli) 54.63 KB (0%)
react-on-rails-pro/client bundled (brotli) (time) 54.63 KB (0%)
registerServerComponent/client bundled (gzip) 127.53 KB (0%)
registerServerComponent/client bundled (gzip) (time) 127.53 KB (0%)
registerServerComponent/client bundled (brotli) 61.7 KB (0%)
registerServerComponent/client bundled (brotli) (time) 61.7 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) 122.02 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time) 122.02 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) 56.73 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time) 56.73 KB (0%)

…irectory

Addresses PR #3165 review (claude[bot]):

- setup_pro now captures the return value of create_node_renderer.
  When a legacy client/node-renderer.js is present, create_node_renderer
  returns true, and setup_pro skips add_pro_to_procfile. Previously,
  legacy-renderer users could end up with a Procfile.dev entry pointing
  at renderer/node-renderer.js (a file deliberately not created),
  leaving the renderer process unable to start.
- create_node_renderer now uses Thor's empty_directory instead of
  FileUtils.mkdir_p, so it participates in --pretend / verbose output
  like the rest of the generator.
- Added a Procfile.dev assertion to the legacy-renderer spec to lock
  in the correct behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@justin808
Copy link
Copy Markdown
Member Author

Review response summary

I (Claude Code) addressed the follow-up review in commit 7f35de0:

react_on_rails/lib/generators/react_on_rails/pro_setup.rb

  • create_node_renderer now returns true when a legacy client/node-renderer.js is detected and false otherwise. setup_pro captures that return value and skips add_pro_to_procfile when the legacy file is present, so Procfile.dev no longer gets a node-renderer: … node renderer/node-renderer.js entry pointing at a file that was deliberately not created. (addresses @claude[bot] thread on setup_pro/add_pro_to_procfile and the @cursor[bot] duplicate)
  • Swapped FileUtils.mkdir_p(…) for Thor's empty_directory("renderer") so the step participates in --pretend / verbose output like the rest of the generator. (addresses @claude[bot] thread on mkdir_p)

react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb

  • Added a Procfile.dev assertion to the legacy-renderer context: expect(File.read(…"Procfile.dev")).not_to include("node-renderer:") — locks in the fix above. (addresses @claude[bot] missing-assertion thread)

Already-replied threads resolved

  • Relative docs link (both @claude[bot] and @greptile-apps[bot]): keeping relative per replies in 3104510276 / 3104510305 — absolute URL to react_on_rails_pro/spec/dummy/renderer/node-renderer.js 404s on main until this PR merges (breaks pre-push lychee check), and docs are served via GitHub file browsing rather than a Docusaurus/VitePress pipeline.

Verified locally: bundle exec rspec react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb → 82 examples, 0 failures. bundle exec rubocop on the two changed files → clean.

@justin808
Copy link
Copy Markdown
Member Author

Review response summary

I (Claude Code) addressed the three remaining review threads in commit 4e8263473.

react_on_rails/lib/generators/react_on_rails/pro_setup.rb

  • Fixed accidental :): punctuation in the legacy-renderer migration hint (line 231) — now reads ...Docker CMD / command): followed by the example command line. (coderabbitai thread)
  • Tightened the add_pro_to_procfile guard: it now only treats Procfile.dev as "already configured" when it references node renderer/node-renderer.js specifically. If a stale legacy node-renderer: ... node client/node-renderer.js entry is present, the generator prints an explicit warning with the exact line to switch to and skips — avoiding both the old silent-failure scenario and appending a duplicate entry. (claude-bot thread)

react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb

  • Added when renderer/node-renderer.js exists and Procfile.dev has a stale legacy node-renderer entry — asserts exactly one node-renderer: line remains and the stale legacy entry is left in place for the user to edit.
  • Added when Procfile.dev already contains the new renderer/node-renderer.js entry — asserts Procfile.dev is left unchanged (idempotent re-run).

Declined (and resolved as duplicate):

Verification

  • bundle exec rspec spec/react_on_rails/generators/pro_generator_spec.rb → 90 examples, 0 failures.
  • bundle exec rubocop lib/generators/react_on_rails/pro_setup.rb spec/react_on_rails/generators/pro_generator_spec.rb → no offenses.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

Code Review

Overview

This PR cleanly establishes renderer/node-renderer.js as the canonical entry point for the Pro Node Renderer, replacing inconsistent client/ usage across docs, generators, and the spec/dummy app. The Docker-exclusion rationale is a nice addition to the container deployment guide.

Strengths

  • The generator upgrade path is safe: checks for both new and legacy paths before touching anything, and the boolean return from create_node_renderer neatly signals whether to skip the Procfile step.
  • Test coverage for the new upgrade scenarios is thorough (legacy-only, new-only, both, stale Procfile, already-current Procfile — all covered).
  • Switching from FileUtils.mkdir_p to Thor's empty_directory is the right call for generator idiomatic code.

Issues / Suggestions

1. Idempotency check is too narrow (add_pro_to_procfile, line 262)

The check procfile_content.include?("node renderer/node-renderer.js") would miss a Procfile line that uses ./renderer/node-renderer.js (a relative prefix). It would then fall through to the node-renderer: label check and falsely print a "stale entry" warning. In practice the generated line never uses ./, but users who manually edited their Procfile could hit this. A tighter match or regex would be more robust.

2. No Procfile stale-entry check when legacy renderer is detected

When client/node-renderer.js is present, add_pro_to_procfile is skipped entirely. The migration message shown by create_node_renderer does mention updating Procfile.dev, but if the user's Procfile already has a stale node client/node-renderer.js entry, the generator gives no specific warning about that line. A follow-on read-only check of the Procfile would make the guidance more actionable.

3. Minor: relative doc link may break in rendered documentation

js-configuration.md changed the testing-example link from an absolute GitHub URL to a relative path (../../../../react_on_rails_pro/spec/dummy/renderer/node-renderer.js). Works in the repo browser, but depending on how the docs site resolves paths (GitBook, Docusaurus, etc.) this could 404. Might be worth keeping an absolute https://github.com/shakacode/... URL here.

Summary

The core change is solid and the test matrix covers the important upgrade combinations well. Items 1 and 2 are minor edge-case gaps in the generator UX rather than correctness bugs — nothing that would break a fresh install. Item 3 is cosmetic.

Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb Outdated
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
Comment thread docs/oss/building-features/node-renderer/js-configuration.md
- Match renderer command with regex so `node ./renderer/node-renderer.js`
  is recognized as already present (idempotency was too strict).
- When a legacy `client/node-renderer.js` is detected and Procfile.dev
  still launches it, surface a pointed warning pinpointing the stale
  line instead of leaving users to diff the generic migration hint.
- Add specs for both the `./`-prefixed command and the stale legacy
  Procfile entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@justin808
Copy link
Copy Markdown
Member Author

Review response summary

I (Claude Code) addressed the two OPTIONAL items from this review in commit 7f11f1b8d.

react_on_rails/lib/generators/react_on_rails/pro_setup.rb

  • add_pro_to_procfile now matches the renderer command with a regex that tolerates an optional ./ prefix (e.g. node ./renderer/node-renderer.js), so a manually-edited Procfile is still recognized as already configured and no longer trips the "stale entry" fallback. The stale-entry fallback itself is now anchored on ^node-renderer: instead of a bare substring check.
  • When a legacy client/node-renderer.js is detected, create_node_renderer now reads Procfile.dev (if present) and, if it still launches the legacy path, surfaces a pointed GeneratorMessages.add_warning pinpointing the exact line to update rather than leaving users to diff the generic migration hint.

react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb

  • Added a context that seeds Procfile.dev with node ./renderer/node-renderer.js and asserts the file is left unchanged (locks in the widened idempotency check).
  • Added a context where both the legacy renderer file and a stale node client/node-renderer.js Procfile line exist, asserting the new pointed warning is surfaced via GeneratorMessages and the stale line is left untouched.

Item 3 — relative doc link in docs/oss/building-features/node-renderer/js-configuration.md — intentionally not changed. The same concern was raised earlier by greptile (thread, now resolved). We flipped to an absolute GitHub URL in 54c71c210, lychee flagged it in CI, and 3eaef8c2c reverted back to the relative path to keep the link checker green. If Docusaurus/GitBook resolution becomes an actual issue downstream we can revisit with a different link style that both tools accept.

bundle exec rspec react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb passes locally (93 examples, 0 failures).

Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb Outdated
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

Code Review

Overview

Clean, well-scoped PR that establishes renderer/node-renderer.js as the canonical location for the Pro Node Renderer entry point. The backward-compatibility handling (detecting and preserving legacy client/node-renderer.js) is thoughtful, and the 7 new generator spec contexts provide solid coverage of the state matrix.


Potential bugs

Regex matches commented-out Procfile lines (see inline comment on NEW_RENDERER_COMMAND_REGEX)

%r{node\s+\.?/?renderer/node-renderer\.js} has no line-start or non-comment anchor, so a manually commented-out Procfile line like:

# node-renderer: node renderer/node-renderer.js

…would cause the generator to think the entry is already present and skip adding the live entry. The same issue applies to LEGACY_RENDERER_COMMAND_REGEX and the /^node-renderer:/ check in add_pro_to_procfile. A simple fix is to require a non-comment prefix: %r{^[^#\n]*node\s+\.?/?renderer/node-renderer\.js}.


Design / UX concerns

Mixed concern in create_node_renderer (see inline)

The method issues a Procfile.dev warning as a side effect, which is invisible to setup_pro callers. Lifting warn_on_stale_legacy_procfile_entry into setup_pro alongside the unless legacy_renderer_detected guard would make the control flow self-documenting.

Missing guidance for "legacy file exists, no Procfile entry" (see inline)

When client/node-renderer.js is detected and there's no Procfile entry at all, the message says "update any references (e.g. Procfile.dev, …)". There's nothing to update — the user needs to add an entry. The copy should handle that sub-case separately.


Minor observations

  • The serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles') comment added to the template is helpful — __dirname now resolves from renderer/ rather than client/, and the explicit note that ../ lands at the project root prevents a future reader from wondering why the path climbs up.
  • empty_directory("renderer") is a good idiomatic upgrade over FileUtils.mkdir_p — it respects Thor's dry-run mode.
  • Doc link in js-configuration.md was changed from an absolute GitHub URL to a relative path, which is an improvement (won't break across branches). The verbose display text (react_on_rails_pro/spec/dummy/renderer/node-renderer.js) is accurate but a bit heavy — something like spec/dummy/renderer/node-renderer.js would be easier to scan.

Test coverage

The new spec contexts in pro_generator_spec.rb cover the important permutations well. One gap: there's no test for the commented-out Procfile line scenario described above, which would catch the regex anchor bug.


Overall this is in good shape. The regex anchor issue is the one concrete bug worth fixing before merge; the rest are polish.

@justin808
Copy link
Copy Markdown
Member Author

Review sweep completed at 2026-04-18T19:51:39Z.
Scan mode: default cutoff (no prior summary marker existed, so this run reviewed the full PR history through 2026-04-18T19:24:54Z).

Mattered

  • MUST-FIX (addressed): react_on_rails/lib/generators/react_on_rails/pro_setup.rb could treat commented Procfile lines as active and skip adding a live node-renderer entry. Fixed in commit 20633e788 by matching active node-renderer: lines only for both new/legacy paths, plus added regression specs in react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb.
  • Optional feedback (resolved with rationale, no code changes): control-flow separation (create_node_renderer side effect) and legacy migration copy refinement were treated as non-blocking polish in this pass.

Skipped

  • Kept the relative docs link in docs/oss/building-features/node-renderer/js-configuration.md; absolute main URL form can fail pre-merge link checks before this PR lands.
  • Two older inline concerns were already addressed by existing code behavior (./renderer idempotency handling and stale legacy Procfile warning path); replied and resolved.

No follow-up issue was created.

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

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 18, 2026

Code Review

Overview

Good, well-scoped PR. The path standardization is clear and the upgrade safety logic (preserving client/node-renderer.js for existing apps) is a sensible choice. The Procfile detection logic is well-tested across many edge cases.

Issues

1. Documentation link regressiondocs/oss/building-features/node-renderer/js-configuration.md line 61

The link was changed from an absolute GitHub URL to a relative local path (../../../../react_on_rails_pro/spec/dummy/renderer/node-renderer.js). This will break on any rendered documentation site (GitBook, Docusaurus, GitHub Pages) that doesn't resolve cross-directory file-system paths. The original GitHub URL was universally accessible; the relative path only works for developers navigating locally in an editor.

2. Missing edge-case test: both files exist + legacy Procfile entry

The "when both renderer/node-renderer.js and legacy client/node-renderer.js exist" context (line 1493 of pro_generator_spec.rb) only tests a Procfile with no node-renderer: entry. There's no test for when both files exist AND Procfile.dev already points at the legacy client/node-renderer.js.

In that scenario the code path differs from the "legacy only" case:

  • create_node_renderer sees renderer/node-renderer.js first → returns false
  • add_pro_to_procfile is called and issues the shorter "Update it manually" message
  • warn_on_stale_legacy_procfile_entry is never called

The user gets less-specific guidance than if only the legacy file were present. Worth adding a test and deciding whether the more detailed migration hint should surface here too.

3. Public module constants

NEW_RENDERER_COMMAND_REGEX and LEGACY_RENDERER_COMMAND_REGEX are defined as public constants on the ProSetup module (lines 216–217 of pro_setup.rb). They're implementation details used only inside the module. Adding private_constant :NEW_RENDERER_COMMAND_REGEX, :LEGACY_RENDERER_COMMAND_REGEX after the definitions would prevent accidental external access and signal their intended scope.

Positive notes

  • serverBundleCachePath behaviour is unchanged — both client/ and renderer/ are one level deep from the project root, so path.resolve(__dirname, '../.node-renderer-bundles') resolves to the same absolute path. The new comment in the template makes this explicit. Good.
  • Switching from FileUtils.mkdir_p to empty_directory("renderer") is the correct Thor idiom.
  • Regex handling of optional ./ prefix and commented-out Procfile lines is careful and well-tested.
  • The 13 new spec contexts provide solid coverage of the upgrade path matrix.

Comment thread docs/oss/building-features/node-renderer/js-configuration.md
Comment thread react_on_rails/lib/generators/react_on_rails/pro_setup.rb
Comment thread react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rb
@justin808
Copy link
Copy Markdown
Member Author

Follow-up review sweep completed at 2026-04-18T20:21:28Z.
Scan mode: default cutoff, reviewing new feedback after the prior summary checkpoint (2026-04-18T19:51:40Z) through 2026-04-18T19:53:29Z.

Mattered

  • No new must-fix correctness issues were found.
  • Resolved 3 newly opened threads with rationale:
    • docs absolute-vs-relative link suggestion (kept relative for PR-branch link-check compatibility),
    • private_constant suggestion (optional polish),
    • duplicate test-coverage suggestion (scenario already covered by existing stale-legacy-entry context).

Skipped

  • Optional/style-only suggestions were intentionally not changed in code.

No follow-up issue was created.

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

@justin808 justin808 merged commit a5bdaee into main Apr 19, 2026
72 checks passed
@justin808 justin808 deleted the jg/3073-renderer-entry-path branch April 19, 2026 03:26
justin808 added a commit that referenced this pull request Apr 19, 2026
Now that PR #3165 has merged and `react_on_rails_pro/spec/dummy/renderer/node-renderer.js`
exists on `main`, switch the testing-example link in `js-configuration.md` from a
cross-package relative path back to an absolute GitHub URL. The relative path
was a temporary workaround so the lychee link check would pass before the file
existed at its new location; absolute URLs render correctly on the published
documentation site, where relative cross-package links may not resolve.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808 added a commit that referenced this pull request Apr 19, 2026
## Summary

Follow-up to #3165. Now that
`react_on_rails_pro/spec/dummy/renderer/node-renderer.js` exists on
`main`, switches the testing-example link in
`docs/oss/building-features/node-renderer/js-configuration.md` from a
cross-package relative path back to an absolute GitHub URL.

The relative path was a temporary workaround so the lychee link check
would pass before the file existed at its new location. Absolute URLs
render correctly on the published documentation site (Docusaurus /
`reactonrails.com/docs`), where cross-package relative links may not
resolve.

## Test plan

- [x] Lychee pre-commit hook passes on the changed file
- [x] Prettier passes
- [x] `curl
https://github.com/shakacode/react_on_rails/blob/main/react_on_rails_pro/spec/dummy/renderer/node-renderer.js`
returns 200

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk documentation-only change that updates a single link target
with no runtime or API impact.
> 
> **Overview**
> Restores the **testing example** link in
`docs/oss/building-features/node-renderer/js-configuration.md` to an
absolute GitHub URL (instead of a cross-package relative path), so the
link resolves correctly on the published documentation site.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
10da4a9. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Updated a testing example reference link to point to an external
GitHub repository, enhancing documentation accessibility while
preserving the reference material.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808 added a commit that referenced this pull request Apr 23, 2026
…ons-docs

* origin/main:
  chore: remove redundant --rsc-pro install generator flag (#3105)
  ci: warn (don't fail) on Bencher main regression (#3168)
  test: enable RSpec --profile to surface slowest package tests (#3176)
  fix(node-renderer): expose performance in VM context when supportModules (#3158)
  docs: remove stale immediate_hydration references (#3139) (#3159)
  docs: restore absolute URL for node-renderer testing example (#3179)
  Bump Rspack dependencies to v2 (^2.0.0-0) (#3084)
  chore: remove obsolete webpack <5.106.0 pin (#3175)
  Move Node Renderer entry point to renderer/ directory (#3165)
  docs: address RSC pitfalls review follow-ups (#3155) (#3156)
  docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163)

# Conflicts:
#	docs/oss/building-features/node-renderer/js-configuration.md
justin808 added a commit that referenced this pull request Apr 23, 2026
…ging' into jg/3122-rolling-deploy-adapter

* origin/jg/3122-unify-renderer-cache-staging: (39 commits)
  fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190)
  docs: add RSC migration success stories page (#1985) (#3162)
  Fix Bencher reporting permanently broken on pushes to main (#3148)
  docs: add example migrations guide (#3126)
  docs: remove defunct guavapass.com reference (#3199)
  chore: remove redundant --rsc-pro install generator flag (#3105)
  ci: warn (don't fail) on Bencher main regression (#3168)
  test: enable RSpec --profile to surface slowest package tests (#3176)
  fix(node-renderer): expose performance in VM context when supportModules (#3158)
  docs: remove stale immediate_hydration references (#3139) (#3159)
  docs: restore absolute URL for node-renderer testing example (#3179)
  Bump Rspack dependencies to v2 (^2.0.0-0) (#3084)
  chore: remove obsolete webpack <5.106.0 pin (#3175)
  Move Node Renderer entry point to renderer/ directory (#3165)
  docs: address RSC pitfalls review follow-ups (#3155) (#3156)
  docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163)
  Scaffold CI workflow and build scripts for first-run consistency (#3097)
  Add OPTIONAL triage tier and fix recommendations to /address-review (#3161)
  chore: sync Gemfile.lock with term-ansicolor 1.11.3 (#3164)
  Simplify the docs sidebar and Pro landing pages (#3119)
  ...
justin808 added a commit that referenced this pull request Apr 23, 2026
* origin/main:
  fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190)
  docs: add RSC migration success stories page (#1985) (#3162)
  Fix Bencher reporting permanently broken on pushes to main (#3148)
  docs: add example migrations guide (#3126)
  docs: remove defunct guavapass.com reference (#3199)
  chore: remove redundant --rsc-pro install generator flag (#3105)
  ci: warn (don't fail) on Bencher main regression (#3168)
  test: enable RSpec --profile to surface slowest package tests (#3176)
  fix(node-renderer): expose performance in VM context when supportModules (#3158)
  docs: remove stale immediate_hydration references (#3139) (#3159)
  docs: restore absolute URL for node-renderer testing example (#3179)
  Bump Rspack dependencies to v2 (^2.0.0-0) (#3084)
  chore: remove obsolete webpack <5.106.0 pin (#3175)
  Move Node Renderer entry point to renderer/ directory (#3165)
  docs: address RSC pitfalls review follow-ups (#3155) (#3156)

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-cleanup Documentation cleanup or migration documentation enhancement full-ci P2 Backlog priority P3 Parked priority review-needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Establish canonical renderer entry point path across docs

1 participant