fix(pro-dummy): make manual node-renderer validation reliable#3200
fix(pro-dummy): make manual node-renderer validation reliable#3200
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new manual validation checklist for Pro node-renderer changes, updates CLAUDE indexes and dummy app scripts/Procfile, coerces webpack devServer port for the React Refresh overlay, and adds three development gems to mitigate Ruby 3.5+ warnings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes three independent breakages in the Pro dummy app that could cause correct renderer fixes to appear broken or silently validate stale artifacts: a Confidence Score: 5/5Safe to merge — all three fixes are correct and well-scoped to dev tooling with no production code impact. All changes are confined to dummy app configs, Gemfiles, package.json scripts, and documentation. The sockPort Number() coercion is correct (env-vars are always strings, Number() is the idiomatic coercion), the Ruby gem additions match the pattern already in the open-source Gemfile, and the new scripts use && so a build failure prevents a stale start. No P0 or P1 findings. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["bin/dev starts"] --> B["SHAKAPACKER_DEV_SERVER_PORT set as string"]
B --> C["Shakapacker forwards to devServer.port unchanged"]
C --> D{"sockPort type?"}
D -->|"Before: string '3035'"| E["❌ overlay.sockPort should be a number"]
D -->|"After: Number(devServer.port) = 3035"| F["✅ Plugin schema satisfied"]
G["Ruby 3.5+"] --> H{"ostruct/logger/benchmark"}
H -->|"Before: missing"| I["❌ cannot load such file -- ostruct"]
H -->|"After: explicit gem declarations"| J["✅ bundle install resolves cleanly"]
K["Edit src/**"] --> L{"Rebuild lib/?"}
L -->|"Before: no rebuild step"| M["❌ Stale lib/ silently consumed"]
L -->|"After: node-renderer:fresh or build-watch"| N["✅ Fresh lib/ validated"]
Reviews (1): Last reviewed commit: "fix(pro-dummy): make manual node-rendere..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
react_on_rails_pro/spec/dummy/package.json (1)
108-110: Scripts look good; naming is consistent with other colon-namespaced entries.
node-renderer:freshandnode-renderer:build-watchcorrectly delegate to the package'sbuild/build-watchscripts (seepackages/react-on-rails-pro-node-renderer/package.json), and the port/entry-point match the existingnode-rendererscript. Nit (optional): the existingnode-renderer-debuguses hyphen-style while the new entries use colon-style — if you want a fully consistent namespace, consider also aliasingnode-renderer:debug. Not worth a follow-up on its own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/package.json` around lines 108 - 110, Add a colon-namespaced alias for the existing node-renderer-debug script to keep script naming consistent: update package.json scripts to include a new "node-renderer:debug" entry that points to the same command as "node-renderer-debug" (alongside the existing "node-renderer", "node-renderer:fresh", and "node-renderer:build-watch" entries) so both hyphen- and colon-style invocations work.react_on_rails_pro/spec/dummy/Procfile.dev (1)
14-19: Useful inline documentation; consider clarifying the foreman ordering caveat.The comment and optional
node-renderer-buildline are a good ergonomic improvement. One small gotcha worth calling out (optionally): if a reviewer uncomments thenode-renderer-buildwatcher line, foreman/overmind starts processes concurrently, so on a first run thenode-rendererprocess may start before the initialbuild-watchhas emittedlib/. A brief note like "on first use, wait for the initialbuild-watchcompile before the renderer will pick up changes; restart thenode-rendererprocess after saves" would save reviewers a debugging cycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails_pro/spec/dummy/Procfile.dev` around lines 14 - 19, Add a short clarifying sentence to the existing comment explaining the foreman/overmind concurrency caveat: mention that if someone uncomments node-renderer-build (the pnpm --filter react-on-rails-pro-node-renderer run build-watch watcher), processes start concurrently so the node-renderer (renderer/node-renderer.js) may start before the initial build outputs lib/, and advise to wait for the first build-watch compile (or restart the node-renderer process) on first use to ensure the renderer picks up changes..claude/docs/validating-node-renderer-changes.md (1)
100-106:lib/vssrc/directory mtime comparison is unreliable.
ls -la <dir>/shows the directory's own mtime, which only updates when entries are added/removed/renamed — editing a file insrc/does not bump thesrc/directory mtime, andtscrewriting files inlib/may or may not bumplib/'s. A reviewer can easily see matching-looking timestamps and draw the wrong conclusion either direction. Prefer a per-file check, e.g.:Proposed doc tweak
-- [ ] `lib/` mtime is newer than the `src/` file you changed (compare -- `ls -la packages/react-on-rails-pro-node-renderer/lib/` against -- `ls -la packages/react-on-rails-pro-node-renderer/src/`) +- [ ] The built `lib/` file corresponding to your edit is newer than the `src/` file, e.g. + `find packages/react-on-rails-pro-node-renderer/lib -newer packages/react-on-rails-pro-node-renderer/src/<changed-file>.ts | head` + should list the rebuilt output, or compare `stat -c '%Y %n'` on the specific files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/docs/validating-node-renderer-changes.md around lines 100 - 106, Update the checklist in .claude/docs/validating-node-renderer-changes.md to stop recommending directory mtime comparisons and instead instruct reviewers to verify per-file mtimes for the compiled output vs the source (i.e., compare the specific file in packages/react-on-rails-pro-node-renderer/lib/ to its counterpart in packages/react-on-rails-pro-node-renderer/src/), advising use of a file-level timestamp/stat check or equivalent file listing per file to ensure the lib artifact is newer than the src file; keep the watcher/rebuild and Procfile restart reminders but replace the directory-level guidance with this per-file verification wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/docs/validating-node-renderer-changes.md:
- Around line 24-33: Update the sentence that currently reads "The Pro dummy app
requires Ruby 3.3.x" and remove or replace the hard `mise use ruby@3.3.7`
directive: instead state the verified/supported Ruby range (e.g., "Ruby
3.3.x–3.4.x verified; 3.5+ should work due to explicit ostruct/logger/benchmark
entries in the Pro Gemfile") or drop the specific `mise` command while keeping
the bundle install instructions; ensure you reference the explicit gems
`ostruct`, `logger`, and `benchmark` so the note explains why newer Rubies are
supported.
---
Nitpick comments:
In @.claude/docs/validating-node-renderer-changes.md:
- Around line 100-106: Update the checklist in
.claude/docs/validating-node-renderer-changes.md to stop recommending directory
mtime comparisons and instead instruct reviewers to verify per-file mtimes for
the compiled output vs the source (i.e., compare the specific file in
packages/react-on-rails-pro-node-renderer/lib/ to its counterpart in
packages/react-on-rails-pro-node-renderer/src/), advising use of a file-level
timestamp/stat check or equivalent file listing per file to ensure the lib
artifact is newer than the src file; keep the watcher/rebuild and Procfile
restart reminders but replace the directory-level guidance with this per-file
verification wording.
In `@react_on_rails_pro/spec/dummy/package.json`:
- Around line 108-110: Add a colon-namespaced alias for the existing
node-renderer-debug script to keep script naming consistent: update package.json
scripts to include a new "node-renderer:debug" entry that points to the same
command as "node-renderer-debug" (alongside the existing "node-renderer",
"node-renderer:fresh", and "node-renderer:build-watch" entries) so both hyphen-
and colon-style invocations work.
In `@react_on_rails_pro/spec/dummy/Procfile.dev`:
- Around line 14-19: Add a short clarifying sentence to the existing comment
explaining the foreman/overmind concurrency caveat: mention that if someone
uncomments node-renderer-build (the pnpm --filter
react-on-rails-pro-node-renderer run build-watch watcher), processes start
concurrently so the node-renderer (renderer/node-renderer.js) may start before
the initial build outputs lib/, and advise to wait for the first build-watch
compile (or restart the node-renderer process) on first use to ensure the
renderer picks up changes.
🪄 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: d6c5c81b-62c1-49b3-8002-5b43f23fc7d1
⛔ Files ignored due to path filters (2)
react_on_rails_pro/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.claude/docs/manual-dev-environment-testing.md.claude/docs/validating-node-renderer-changes.mdCLAUDE.mdreact_on_rails_pro/CLAUDE.mdreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/spec/dummy/Procfile.devreact_on_rails_pro/spec/dummy/config/webpack/development.jsreact_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/config/webpack/development.js
Code ReviewThree clean, independent fixes. The root causes are correctly identified and the solutions are minimal and targeted. A few observations below. What's done well
Minor issues
VerdictThe fixes are correct and solve real problems. The two minor issues above are non-blocking; the rest of this is good to merge once they're addressed or acknowledged. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Applies reviewer suggestions against the manual validation workflow so the guide matches the actual supported Ruby range, the freshness check is accurate, and the convenience scripts mirror the Procfile invocation. - docs: broaden Ruby guidance (3.3.x baseline, 3.4.8 verified, 3.5+ via explicit ostruct/logger/benchmark gems); move `mise shell ruby@3.3.7` under an "Optional fallback" block so 3.4.x users see the right path. - docs: replace unreliable directory-level `ls -la` mtime comparison with a per-file `find ... -newer ...` check, and update the report template to match. - Procfile.dev: note foreman/overmind concurrency caveat when uncommenting `node-renderer-build` (wait for first build-watch compile on first run). - package.json: add `node-renderer:debug` colon alias for naming consistency; align `node-renderer:fresh` flags with the Procfile.dev entry (`RENDERER_LOG_LEVEL=debug`, `--inspect`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address-review summaryScan scope: full PR history (no prior address-review summary — this comment becomes the next default cutoff). Branch: Mattered
Optional
Skipped
Next default scan starts after this comment. Say |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@react_on_rails_pro/spec/dummy/package.json`:
- Line 108: The package script "node-renderer:debug" currently invokes "ndb"
which is not a declared dependency; change it to use the built-in inspector like
the existing "node-renderer:fresh" script by replacing the "ndb
renderer/node-renderer.js" invocation with a "node --inspect" style invocation
so the script runs without requiring global ndb (update the
"node-renderer:debug" script string accordingly, matching the pattern used by
"node-renderer:fresh").
🪄 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: 67b0c734-9868-4e92-b9ac-f38132965410
📒 Files selected for processing (3)
.claude/docs/validating-node-renderer-changes.mdreact_on_rails_pro/spec/dummy/Procfile.devreact_on_rails_pro/spec/dummy/package.json
✅ Files skipped from review due to trivial changes (2)
- react_on_rails_pro/spec/dummy/Procfile.dev
- .claude/docs/validating-node-renderer-changes.md
Code ReviewSummary: Three targeted fixes for the Pro dummy app dev environment — ✅ What looks good
Issues1. Orphaned
"node-renderer-debug": "RENDERER_PORT=3800 ndb renderer/node-renderer.js", // old, still present
"node-renderer:debug": "RENDERER_PORT=3800 ndb renderer/node-renderer.js", // new, added by this PRThe old hyphen-style name should be removed to avoid confusion. See inline comment. 2.
3. The execjs-compatible-dummy pins VerdictThe core fixes are correct and the documentation improvements are valuable. The orphaned |
Apply optional cleanups from CodeRabbit and claude[bot] review: - Replace `ndb` with `node --inspect` in `node-renderer:debug` so the script works without a global `ndb` install. Drop the duplicated hyphen-style `node-renderer-debug` script and update the OSS debugging doc to point at the colon-style name. - Use `parseInt(devServer.port, 10)` instead of `Number(devServer.port)` in both Pro dummy and ExecJS dummy webpack development configs to make the base-10 numeric-parse intent explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@claude[bot] re: Code Review summary: the three issues raised in this summary were addressed inline as the threads they referenced:
|
Address-review summaryScan scope: new comments since previous address-review summary (cutoff: 2026-04-23T07:32:40Z). Branch: Mattered (Optional improvements addressed)
Skipped (rationale reply posted)
Next default scan starts after this comment. Say |
Review: PR #3200 — fix(pro-dummy): make manual node-renderer validation reliableThis PR fixes three independent developer experience breakages cleanly. The scope is tight, the test plan is explicit, and the documentation additions are well-structured. A couple of minor items below. ✅ Strengths
Minor IssuesSee inline comments for specifics. Summary:
No concerns
|
|
@claude[bot] re: Code Review summary — both minor items addressed:
Replied inline on both threads and resolved them. — I (Claude Code) |
Address-review summaryScan scope: new comments since previous address-review summary (2026-04-29T06:34:55Z). Branch: TriageMUST-FIX (0): none. OPTIONAL (2): both addressed.
SKIPPED (1): rationale reply posted.
NotesBoth inline threads ( Side-note in inline reply 1: the reviewer's claim that — I (Claude Code) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
node-renderer:freshscript missing--inspectflag- Added the missing
--inspectflag sonode-renderer:freshmatches the debug Procfile renderer command.
- Added the missing
Or push these changes by commenting:
@cursor push e87c695364
Preview (e87c695364)
diff --git a/react_on_rails_pro/spec/dummy/package.json b/react_on_rails_pro/spec/dummy/package.json
--- a/react_on_rails_pro/spec/dummy/package.json
+++ b/react_on_rails_pro/spec/dummy/package.json
@@ -106,7 +106,7 @@
"build:clean": "rm -rf public/webpack && rm -rf ssr-generated || true",
"node-renderer:debug": "RENDERER_LOG_LEVEL=debug RENDERER_PORT=3800 node --inspect renderer/node-renderer.js",
"node-renderer": "RENDERER_PORT=3800 node renderer/node-renderer.js",
- "node-renderer:fresh": "pnpm --filter react-on-rails-pro-node-renderer run build && RENDERER_LOG_LEVEL=debug RENDERER_PORT=3800 node renderer/node-renderer.js",
+ "node-renderer:fresh": "pnpm --filter react-on-rails-pro-node-renderer run build && RENDERER_LOG_LEVEL=debug RENDERER_PORT=3800 node --inspect renderer/node-renderer.js",
"node-renderer:build-watch": "pnpm --filter react-on-rails-pro-node-renderer run build-watch"
},
"license": "UNLICENSED",You can send follow-ups to the cloud agent here.
|
Code review posted inline. Summary: all three fixes are correct and appropriately scoped. Two minor points: (1) parseInt(devServer.port, 10) silently returns NaN on undefined/empty input -- a guard like || 3035 would surface misconfiguration more loudly; (2) node-renderer:fresh hardcodes RENDERER_LOG_LEVEL=debug while the base script does not -- a brief comment clarifying this is intentional would help. Nothing blocks merging. |
|
@claude[bot] re: Code Review summary — both points were addressed inline as the threads they map to:
— I (Claude Code) |
Address-review summaryScan scope: new comments since previous address-review summary (cutoff 2026-04-29T07:08:43Z). Branch: TriageMUST-FIX (0): none. OPTIONAL (3): all addressed.
SKIPPED (2): rationale replies posted.
NotesAll three previously-unresolved review threads ( Local checks: prettier and eslint pass on both webpack configs. Next default scan starts after this comment. Say — I (Claude Code) |
Applies reviewer suggestions against the manual validation workflow so the guide matches the actual supported Ruby range, the freshness check is accurate, and the convenience scripts mirror the Procfile invocation. - docs: broaden Ruby guidance (3.3.x baseline, 3.4.8 verified, 3.5+ via explicit ostruct/logger/benchmark gems); move `mise shell ruby@3.3.7` under an "Optional fallback" block so 3.4.x users see the right path. - docs: replace unreliable directory-level `ls -la` mtime comparison with a per-file `find ... -newer ...` check, and update the report template to match. - Procfile.dev: note foreman/overmind concurrency caveat when uncommenting `node-renderer-build` (wait for first build-watch compile on first run). - package.json: add `node-renderer:debug` colon alias for naming consistency; align `node-renderer:fresh` flags with the Procfile.dev entry (`RENDERER_LOG_LEVEL=debug`, `--inspect`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply optional cleanups from CodeRabbit and claude[bot] review: - Replace `ndb` with `node --inspect` in `node-renderer:debug` so the script works without a global `ndb` install. Drop the duplicated hyphen-style `node-renderer-debug` script and update the OSS debugging doc to point at the colon-style name. - Use `parseInt(devServer.port, 10)` instead of `Number(devServer.port)` in both Pro dummy and ExecJS dummy webpack development configs to make the base-10 numeric-parse intent explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg-conductor/3177-fix
branch
from
9f37426 to
805b242
Compare
Code ReviewOverall: solid PR that fixes three verified, reproducible bugs. The changes are targeted, low-risk, and well-documented. A few observations below.
|
|
@claude[bot] re: Code Review summary — primarily duplicate signal of the inline comment on Other points in the summary are confirmations of prior fixes ( |
Address-review summaryScan scope: new comments since previous address-review summary (cutoff 2026-04-29T08:14:31Z). Branch: TriageMUST-FIX (1): addressed.
SKIPPED (1): rationale reply posted.
NotesThe doc update in Local checks: prettier and lefthook hooks are green on Next default scan starts after this comment. Say — I (Claude Code) |
There was a problem hiding this comment.
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: Procfile.dev node-renderer uses debug flags instead of base
- Aligned Procfile.dev node-renderer with the base node-renderer script by removing debug logging and --inspect.
Or push these changes by commenting:
@cursor push 5ca87ef817
Preview (5ca87ef817)
diff --git a/react_on_rails_pro/spec/dummy/Procfile.dev b/react_on_rails_pro/spec/dummy/Procfile.dev
--- a/react_on_rails_pro/spec/dummy/Procfile.dev
+++ b/react_on_rails_pro/spec/dummy/Procfile.dev
@@ -18,4 +18,4 @@
# Foreman/overmind starts processes concurrently, so on first use wait for the initial
# build-watch compile or restart `node-renderer` before trusting renderer output.
# node-renderer-build: pnpm --filter react-on-rails-pro-node-renderer run build-watch
-node-renderer: RENDERER_LOG_LEVEL=debug RENDERER_PORT=3800 node --inspect renderer/node-renderer.js
+node-renderer: RENDERER_PORT=3800 node renderer/node-renderer.jsYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 230e9ee. Configure here.
Address-review summaryScan scope: new comments since previous address-review summary (cutoff 2026-04-30T08:48:19Z). Branch: TriageDISCUSS (1) → addressed (option 1a): code tightened to match PR description.
NotesLocal checks: lefthook pre-commit ( Next default scan starts after this comment. Say — I (Claude Code) |
Addresses the follow-up issues surfaced during review of #3158, where validating a renderer fix locally required repo-specific workarounds. Three independent breakages, one shared cause: the dummy app could make a correct fix look broken or silently validate stale build artifacts. 1. `bin/dev` failed with `overlay.sockPort should be a number` because `bin/dev` exports `SHAKAPACKER_DEV_SERVER_PORT` as a string and Shakapacker passes it through unchanged on `devServer.port`. The Pro dummy's webpack development config forwarded that string straight to `@pmmmwh/react-refresh-webpack-plugin`, whose schema requires a number. Coerce with `Number()` (applied to both Pro dummy and execjs dummy). 2. Rails precompile hook failed with `cannot load such file -- ostruct` on Ruby 3.5+ because the Pro Gemfile was missing the explicit `ostruct`, `logger`, `benchmark` declarations the open-source Gemfile already had. 3. The dummy consumes the *built* `react-on-rails-pro-node-renderer/lib/`, not the TypeScript sources, so renderer fixes weren't exercised until the package was rebuilt. Add `node-renderer:fresh` and `node-renderer:build-watch` scripts, document the rebuild requirement inline in `Procfile.dev`, expand the Pro CLAUDE.md, and add a dedicated `validating-node-renderer-changes.md` checklist for reviewers. Fixes #3177 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Applies reviewer suggestions against the manual validation workflow so the guide matches the actual supported Ruby range, the freshness check is accurate, and the convenience scripts mirror the Procfile invocation. - docs: broaden Ruby guidance (3.3.x baseline, 3.4.8 verified, 3.5+ via explicit ostruct/logger/benchmark gems); move `mise shell ruby@3.3.7` under an "Optional fallback" block so 3.4.x users see the right path. - docs: replace unreliable directory-level `ls -la` mtime comparison with a per-file `find ... -newer ...` check, and update the report template to match. - Procfile.dev: note foreman/overmind concurrency caveat when uncommenting `node-renderer-build` (wait for first build-watch compile on first run). - package.json: add `node-renderer:debug` colon alias for naming consistency; align `node-renderer:fresh` flags with the Procfile.dev entry (`RENDERER_LOG_LEVEL=debug`, `--inspect`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply optional cleanups from CodeRabbit and claude[bot] review: - Replace `ndb` with `node --inspect` in `node-renderer:debug` so the script works without a global `ndb` install. Drop the duplicated hyphen-style `node-renderer-debug` script and update the OSS debugging doc to point at the colon-style name. - Use `parseInt(devServer.port, 10)` instead of `Number(devServer.port)` in both Pro dummy and ExecJS dummy webpack development configs to make the base-10 numeric-parse intent explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The :fresh script's purpose is rebuild + start; debug-port binding already lives in :debug. Removing --inspect keeps the script's contract clean and avoids unexpected debugger-port listening on validation runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`parseInt(undefined, 10)` returns NaN, which would be silently passed to ReactRefreshWebpackPlugin if devServer.port was ever absent. Falling back to Shakapacker's default port (3035) surfaces a misconfiguration as a wrong port rather than silent NaN. Applied to both the Pro dummy and the execjs-compatible dummy webpack configs for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…node-renderer Align the Procfile.dev `node-renderer:` line with the base `node-renderer` npm script so it matches the PR description. Previously it was byte-for-byte identical to `node-renderer:debug`, which carried `--inspect` overhead and opened port 9229 on every `bin/dev` run. Use `node-renderer:debug` when debugger access is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg-conductor/3177-fix
branch
from
9d85c31 to
a3f6edb
Compare
Code ReviewOverall this is a well-scoped, low-risk PR that correctly addresses three independent breakages in the Pro dummy app. The fixes are solid and the added documentation is genuinely useful. The Good
Issues FoundStale documentation:
|
| ```bash | ||
| cd react_on_rails_pro/spec/dummy | ||
| pnpm run node-renderer-debug | ||
| pnpm run node-renderer:debug |
There was a problem hiding this comment.
This rename is correct, but the Quick start section above (line 17) still says:
"the node renderer is already listening on port 3800 with
--inspectenabled"
That claim was true when Procfile.dev had node --inspect, but this PR removes --inspect from the node-renderer: Procfile entry. A developer following the Quick start guide will open chrome://inspect and find nothing to connect to.
Suggested fix for that paragraph (not in this diff, so flagging here):
If you already have the dummy app running via `bin/dev` (which uses `Procfile.dev`), the node renderer is listening on port 3800. To attach a debugger you first need to restart it with `--inspect` — either stop the process and run `pnpm run node-renderer:debug`, or start `bin/dev` after temporarily adding `--inspect` to the `node-renderer:` entry in `Procfile.dev`.
1 participant
Summary
Addresses three independent breakages in the Pro dummy app that surfaced during review of #3158, where validating a renderer fix locally required repo-specific workarounds. Each one could cause a correct fix to look broken or silently validate stale build artifacts.
bin/devoverlay.sockPort should be a number—bin/devexportsSHAKAPACKER_DEV_SERVER_PORTas a string, Shakapacker forwards it unchanged ondevServer.port, and the Pro dummy passed it straight to@pmmmwh/react-refresh-webpack-plugin, whose schema requires a number. Coerced withparseInt(devServer.port, 10) || 3035in both Pro dummy and execjs dummy webpack configs (the fallback surfaces a misconfiguration as a wrong port instead of silentNaN).cannot load such file -- ostruct— Pro Gemfile was missing the explicitostruct/logger/benchmarkdeclarations that the open-source Gemfile already had for Ruby 3.5+ compatibility. Added them.react-on-rails-pro-node-renderer/lib/, not the TS source. Addednode-renderer:fresh(rebuild + start) andnode-renderer:build-watchscripts, documented the rebuild requirement inline inProcfile.dev, expandedreact_on_rails_pro/CLAUDE.md, and added a dedicated.claude/docs/validating-node-renderer-changes.mdchecklist for reviewers.Notes for reviewers
node-renderer:freshintentionally hardcodesRENDERER_LOG_LEVEL=debug. The script exists for one-shot manual validation runs, where verbose logging is valuable; the basenode-rendererscript (used byProcfile.dev/bin/dev) deliberately stays at the default level. JSON has no comments, so this callout lives here instead of inline.node-renderer:freshdoes not include--inspect. It's a clean rebuild + start;node-renderer:debugis the dedicated debug-mode entry point. Keeping the two contracts orthogonal is intentional.Fixes #3177
Test plan
new ReactRefreshWebpackPlugin({ overlay: { sockPort: '3035' } })throws the exact error message from the issue (options.overlay.sockPort should be a number);sockPort: 3035succeeds.parseInt(devServer.port, 10) || 3035correctly converts string-typed env-var input to a number, and falls back to3035(Shakapacker's default) on undefined/empty input.bundle installresolves cleanly with the addedostruct/logger/benchmarkgems on Ruby 3.4.8.require 'jbuilder'; require 'ostruct'; require 'logger'; require 'benchmark'succeeds.node --checkpasses on both updated webpack configs..claude/docs/validating-node-renderer-changes.mdagainstbin/devto confirm theoverlay.sockPortandostructerrors no longer occur on a fresh checkout.🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to dev tooling/config and documentation; main risk is minor local workflow or dependency resolution regressions in the Pro dummy environment.
Overview
Improves the Pro dummy app’s manual dev/SSR validation reliability by ensuring the React Refresh overlay gets a numeric
sockPort(coercingdevServer.portviaparseInt(... ) || 3035) in both dummy webpack configs, preventingbin/devstartup failures.Updates Pro development dependencies for Ruby 3.5+ stdlib default-gem compatibility by explicitly adding
benchmark,logger, andostruct(and refreshing lockfiles) to avoidcannot load such file -- ostructduring precompile.Reduces stale node-renderer builds during local testing by adding
node-renderer:fresh/node-renderer:build-watchscripts, simplifying theProcfile.devrenderer command, and expanding documentation (new.claude/docs/validating-node-renderer-changes.md, plus cross-links and updated guidance inCLAUDE.md,react_on_rails_pro/CLAUDE.md, and OSS debugging docs).Reviewed by Cursor Bugbot for commit a3f6edb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Bug Fixes
Chores