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 [email protected]`
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 [email protected]` 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) <[email protected]>
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) <[email protected]>
|
@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 [email protected]` 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) <[email protected]>
…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) <[email protected]>
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:
|
…le.dev change The "Quick start: debugging with the full stack running" section previously said `bin/dev` runs the renderer with `--inspect` enabled. That was true when `Procfile.dev` carried `--inspect`, but PR #3200 dropped it from the base `node-renderer:` entry. Rewrite the paragraph to tell the user how to restart the renderer with `--inspect` (via `pnpm run node-renderer:debug` or by temporarily editing `Procfile.dev`) before opening `chrome://inspect`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Address-review summaryScan scope: final-gate review run by Claude Code; surfaced one issue not previously addressed by automated reviewers (cutoff 2026-04-30T08:56:31Z). Branch: TriageMUST-FIX (1): addressed.
NotesLocal checks on Final-gate verdict: merge-ready once CI re-runs green on this commit. All earlier fixes (sockPort Next default cutoff for full-PR scans: this comment. — I (Claude Code) |
|
Overall this is a well-scoped and well-documented set of fixes. All three root causes are real, the fixes are correct, and the new documentation is genuinely useful. One functional concern: build-watch deletes lib/ on startup The react-on-rails-pro-node-renderer build-watch script is pnpm run clean && tsc --watch, which removes the entire lib/ directory before the first compile completes. If a contributor uncomments the node-renderer-build line in Procfile.dev, the renderer will hit a module-not-found error until the watcher emits its first build. The existing comment warns about a race but understates the severity — the lib is actively deleted, not just slow to appear. The inline comment on Procfile.dev has a suggested stronger warning. Minor issues
Looks good
Generated with Claude Code |
Adds a brief comment near the `parseInt(devServer.port, 10) || 3035` fallback warning that port `0` (OS-assigned) would also be replaced by 3035. Shakapacker never uses `0` as a dev server port, so this is a documentation note for future readers, not a behavioral fix. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@claude[bot] re: Code Review summary — both items addressed:
— Claude Code (acting on Justin's behalf) |
Address-review summaryScan scope: new comments since previous address-review summary (cutoff 2026-04-30T08:56:31Z). Branch: TriageMUST-FIX (1) — addressed.
OPTIONAL (1) — addressed.
DISCUSS (0): none. SKIPPED (0): none. ResultBoth new items resolved. CI is the only remaining gate. |
Code ReviewOverall: Solid, focused PR that fixes three real developer-experience breakages. The changes are conservative, well-scoped, and come with thorough documentation. Approach is correct throughout. Bug fixes
Ruby 3.5+ stdlib gems npm script reorganizationRenaming The DocumentationThe new Minor observations
🤖 Generated with Claude Code |
- Add benchmark/logger/ostruct to execjs-compatible-dummy Gemfile so it also avoids `cannot load such file` errors on Ruby 3.5+ (the dummy has a standalone Gemfile that does not eval_gemfile the shared loader). - Procfile.dev: strengthen the build-watch warning to call out that `pnpm run clean && tsc --watch` deletes `lib/` on startup, so the node-renderer process will fail with module-not-found until the first compile completes. - Gemfile.development_dependencies: correct the comment to reflect that on Ruby 3.5+ the missing stdlib gems produce hard load errors, not warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review feedback addressedI (Claude Code) just addressed all 6 unresolved review threads in commit eb01670. Code/doc changes (3)
Acknowledged without code changes (3)
All 6 threads resolved. |
Review: fix(pro-dummy): make manual node-renderer validation reliableOverall: LGTM with minor suggestions. Three well-scoped, independently-motivated fixes for reproducible breakages. The test plan is thorough, and the validation checklist is a valuable addition to the agent docs. What's good
Suggestions (minor)1. Webpack comment: port-0 caveat adds noise The five-line comment block in both webpack 2.
🤖 Generated with Claude Code |
| // bin/dev sets SHAKAPACKER_DEV_SERVER_PORT as a string, which Shakapacker | ||
| // surfaces unchanged on devServer.port. The plugin schema requires a number. | ||
| // `|| 3035` falls back to Shakapacker's default if devServer.port is missing, | ||
| // so a misconfiguration surfaces as a wrong port rather than silent NaN. | ||
| // Note: port `0` (OS-assigned) would also fall back to 3035, but Shakapacker | ||
| // does not use `0` as a dev server port — do not copy this pattern where `0` is valid. | ||
| sockPort: parseInt(devServer.port, 10) || 3035, |
There was a problem hiding this comment.
The first three lines of this comment are necessary and worth keeping — the string-env-var / plugin-schema mismatch is genuinely non-obvious. The port-0 note (lines 18–19) reads as a "beware if you copy this elsewhere" callout, which is unusual for inline code comments and adds length without clarifying the current code.
| // bin/dev sets SHAKAPACKER_DEV_SERVER_PORT as a string, which Shakapacker | |
| // surfaces unchanged on devServer.port. The plugin schema requires a number. | |
| // `|| 3035` falls back to Shakapacker's default if devServer.port is missing, | |
| // so a misconfiguration surfaces as a wrong port rather than silent NaN. | |
| // Note: port `0` (OS-assigned) would also fall back to 3035, but Shakapacker | |
| // does not use `0` as a dev server port — do not copy this pattern where `0` is valid. | |
| sockPort: parseInt(devServer.port, 10) || 3035, | |
| // bin/dev exports SHAKAPACKER_DEV_SERVER_PORT as a string; Shakapacker surfaces | |
| // it unchanged on devServer.port, but the plugin schema requires a number. | |
| // Fallback to 3035 (Shakapacker default) when the env var is absent/empty. | |
| sockPort: parseInt(devServer.port, 10) || 3035, |
Same suggestion applies to the identical block in execjs-compatible-dummy/config/webpack/development.js.
| # errors" and then restart the `node-renderer` process before trusting renderer output. | ||
| # node-renderer-build: pnpm --filter react-on-rails-pro-node-renderer run build-watch |
There was a problem hiding this comment.
Worth adding a one-liner here so the warning is visible at the point where a developer would decide to use node-renderer:fresh:
| # errors" and then restart the `node-renderer` process before trusting renderer output. | |
| # node-renderer-build: pnpm --filter react-on-rails-pro-node-renderer run build-watch | |
| # errors" and then restart the `node-renderer` process before trusting renderer output. | |
| # If you plan to use `pnpm run node-renderer:fresh` instead, stop this process first | |
| # (or comment out the line below) to avoid an EADDRINUSE conflict on port 3800. | |
| # node-renderer-build: pnpm --filter react-on-rails-pro-node-renderer run build-watch |
…rn/npm-security-dd6aeadc3f * origin/main: (34 commits) fix(pro-dummy): make manual node-renderer validation reliable (#3200) [codex] Add Markdown Prettier CI check (#3242) Add /stress-test Claude Code command for adversarial QA (#3207) Document examples catalog and naming plan (#3191) test(dummy): enable StrictMode in OSS and Pro dummies (#3206) ci: exclude bot-blocking URLs from lychee link check (#3214) Remove stale Coveralls integration (#3204) docs: normalize external GitHub repo slugs in links and generators (#3198) docs: add Example Migrations page (#3125) (#3197) docs: warn about react_component helper collision with react-rails (#3143) (#3160) docs: legacy Webpacker and migration-fit guidance (#3138) (#3157) 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) ...
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
Medium Risk
Medium risk because it changes dev startup orchestration (
Procfile.dev/scripts), webpack dev-server config, and Ruby dependency sets/lockfiles, which can break local workflows if misconfigured.Overview
Improves Pro dummy app manual validation of the node renderer by adding a dedicated checklist (
.claude/docs/validating-node-renderer-changes.md), linking it from existing agent docs, and documenting the built-lib/consumption/rebuild requirement inreact_on_rails_pro/CLAUDE.mdand the dummyProcfile.dev.Fixes
bin/devstartup breakages by coercing the React Refresh overlaysockPortto a number in both dummy webpack development configs, and by explicitly adding stdlib-removed gems (benchmark,logger,ostruct) to Pro development/test Gemfiles with corresponding lockfile updates.Updates renderer run/debug ergonomics by renaming/adding dummy
package.jsonscripts (node-renderer:debug,node-renderer:fresh,node-renderer:build-watch) and aligning the OSS debugging docs with the new workflow; also removes--inspect/debug logging defaults from theProcfile.devnode-renderercommand.Reviewed by Cursor Bugbot for commit eb01670. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Bug Fixes
Chores