Add npm READMEs, Pro quick-start guide, and llms.txt for LLM discoverability#2466
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 comprehensive documentation and READMEs for React on Rails Pro (npm packages and a pro quick-start guide), an llms reference file, minor gem README example tweaks, link updates across pro docs, removal of private-package publishing instructions, and inclusion of README files in two npm package publish lists. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 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 SummaryAdds npm READMEs, Pro quick-start guide, and llms.txt to improve LLM discoverability and correct setup guidance. Key improvements:
Critical issue:
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: fdd60ac |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,106 @@ | |||
| # react-on-rails | |||
There was a problem hiding this comment.
README.md won't be published to npm. The "files" field in package.json only includes lib/** files, which excludes README.md. Add "README.md" to the files array in packages/react-on-rails/package.json:
| # react-on-rails | |
| "files": [ | |
| "README.md", | |
| "lib/**/*.js", |
There was a problem hiding this comment.
Fixed — added "README.md" to the files array in both packages/react-on-rails/package.json and packages/react-on-rails-pro/package.json.
| @@ -0,0 +1,97 @@ | |||
| # react-on-rails-pro | |||
There was a problem hiding this comment.
README.md won't be published to npm. The "files" field in package.json only includes lib/** files. Add "README.md" to the files array in packages/react-on-rails-pro/package.json:
| # react-on-rails-pro | |
| "files": [ | |
| "README.md", | |
| "lib/**/*.js", |
There was a problem hiding this comment.
Fixed in the same commit — see reply above.
PR Review: Add npm READMEs, Pro quick-start guide, and llms.txtOverall this is a well-structured PR that addresses a real problem (LLMs generating incorrect setup code). The content is largely accurate and the motivation is sound. I found a few issues worth addressing before merge. Critical:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ### Node-side (client/node-renderer.js) | ||
|
|
||
| ```js | ||
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
There was a problem hiding this comment.
The generator places node-renderer.js at client/node-renderer.js (as shown in the table above), so __dirname resolves to client/. The correct path to put the cache at the project root (matching the actual template) is '../.node-renderer-bundles', not './.node-renderer-bundles'.
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | |
| serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'), |
There was a problem hiding this comment.
Fixed — path updated to '../.node-renderer-bundles' and added missing require('path').
| **Important:** When using the `react_on_rails_pro` Ruby gem, you **must** use this package (`react-on-rails-pro`) instead of `react-on-rails`. The base gem will reject `react-on-rails` if the Pro gem is detected. | ||
|
|
||
| ## Usage | ||
|
|
There was a problem hiding this comment.
The sentence "The base gem will reject react-on-rails if the Pro gem is detected" is confusing — "the base gem" is ambiguous and it's the Pro gem that enforces this requirement, not the base gem. Suggested rewrite:
| **Important:** When using the `react_on_rails_pro` Ruby gem, you **must** use this package (`react-on-rails-pro`) instead of `react-on-rails`. If the Pro gem detects the base `react-on-rails` npm package at runtime, it will raise an error. |
There was a problem hiding this comment.
Fixed — reworded to: "If the Pro gem detects the base react-on-rails npm package at runtime, it will raise an error."
|
|
||
| - [Node Renderer Basics](https://www.shakacode.com/react-on-rails-pro/docs/node-renderer/basics/) | ||
| - [JavaScript Configuration](https://www.shakacode.com/react-on-rails-pro/docs/node-renderer/js-configuration/) | ||
| - [Rails Configuration](https://www.shakacode.com/react-on-rails-pro/docs/configuration/) |
There was a problem hiding this comment.
setupTracing is imported here but never used in the example. Either add an example usage or remove it from the import to avoid confusing copy-paste.
| - [Rails Configuration](https://www.shakacode.com/react-on-rails-pro/docs/configuration/) | |
| import { addNotifier } from 'react-on-rails-pro-node-renderer/integrations/api'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/react-on-rails-pro/README.md (1)
33-39: RSC example importsRSCRouteandwrapServerComponentRendererbut never uses themThe imports on lines 34–36 include
RSCRouteandwrapServerComponentRenderer, but onlyregisterServerComponentis exercised. Consider either removing the unused imports from the snippet or expanding the example to showRSCRouterendering.✨ Option A – trim to what the example actually demonstrates
-import { RSCRoute } from 'react-on-rails-pro/RSCRoute'; -import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client'; -import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client'; - -// Register a server component for client-side hydration -registerServerComponent({ MyServerComponent }); +import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client'; + +// Register a server component for client-side hydration +registerServerComponent({ MyServerComponent });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro/README.md` around lines 33 - 39, The README snippet imports RSCRoute and wrapServerComponentRenderer but only uses registerServerComponent; either remove the unused imports (delete RSCRoute and wrapServerComponentRenderer from the import lines) or expand the example to demonstrate their usage by showing RSCRoute rendering and invoking wrapServerComponentRenderer (e.g., reference RSCRoute and wrapServerComponentRenderer in the snippet so all imported symbols are exercised alongside registerServerComponent).packages/react-on-rails-pro-node-renderer/README.md (1)
138-144:setupTracingis imported but not used in the error-reporting exampleEither use
setupTracingin the snippet or remove it from the import.✨ Proposed fix
-import { addNotifier, setupTracing } from 'react-on-rails-pro-node-renderer/integrations/api'; +import { addNotifier } from 'react-on-rails-pro-node-renderer/integrations/api';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/README.md` around lines 138 - 144, The example imports setupTracing but never uses it; update the snippet to either use setupTracing or remove it—simplest fix: change the import to only bring addNotifier (remove setupTracing) so the sample shows a used import (refer to addNotifier and setupTracing in the README example).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started/pro-quick-start.md`:
- Line 149: The markdown entry for the Code Splitting link currently points to a
404 URL; locate the line containing the link text "[Code
Splitting](https://www.shakacode.com/react-on-rails-pro/docs/code-splitting/)"
in pro-quick-start.md and either replace the broken URL with the correct working
URL for the Code Splitting docs or remove the entire bullet (the "[Code
Splitting](...)" link) until the target page is restored, ensuring the
surrounding list formatting remains valid.
- Around line 107-115: The example calling reactOnRailsProNodeRenderer uses
path.resolve but never imports path and uses the wrong env var name for
workersCount; add a require('path') at top so path is defined, and change the
workersCount expression from reading process.env.NODE_RENDERER_CONCURRENCY to
process.env.RENDERER_WORKERS_COUNT (fallback to 3 unchanged) so
serverBundleCachePath and workersCount are correct and consistent with the
README.
In `@packages/react-on-rails-pro-node-renderer/README.md`:
- Around line 114-131: Add a brief note above the Advanced example explaining it
uses ESM syntax so users must either use .mjs or set "type":"module" in
package.json or convert the imports to CommonJS; reference the example's symbols
(masterRun, run, configureFastify, cluster) so readers know the note applies to
that snippet.
- Around line 79-81: The README's fenced code block for the Procfile entry lacks
a language specifier which triggers MD040; update the Procfile code block
surrounding the line "node-renderer: node node-renderer.js" to include a
language identifier (for example use "text" or "procfile") so the block becomes
```text ... ``` (or ```procfile ... ```) to silence the markdownlint warning.
---
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/README.md`:
- Around line 138-144: The example imports setupTracing but never uses it;
update the snippet to either use setupTracing or remove it—simplest fix: change
the import to only bring addNotifier (remove setupTracing) so the sample shows a
used import (refer to addNotifier and setupTracing in the README example).
In `@packages/react-on-rails-pro/README.md`:
- Around line 33-39: The README snippet imports RSCRoute and
wrapServerComponentRenderer but only uses registerServerComponent; either remove
the unused imports (delete RSCRoute and wrapServerComponentRenderer from the
import lines) or expand the example to demonstrate their usage by showing
RSCRoute rendering and invoking wrapServerComponentRenderer (e.g., reference
RSCRoute and wrapServerComponentRenderer in the snippet so all imported symbols
are exercised alongside registerServerComponent).
AbanoubGhadban
force-pushed
the
abanoub/improve-llm-discoverability
branch
from
fdd60ac to
ec95e93
Compare
Review SummaryThe goal of this PR (improving LLM discoverability by adding npm READMEs) is well-motivated and the content is generally accurate. However, there is one critical defect that completely undermines the main purpose, plus a few inconsistencies worth fixing before merge. Critical: README files will not be published to npmBoth
Neither Fix: Add Inconsistency:
|
| File | Env var used |
|---|---|
docs/getting-started/pro-quick-start.md |
REACT_RENDERER_URL (matches the generator template) |
packages/react-on-rails-pro-node-renderer/README.md |
RENDERER_URL |
The generator template (lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt) uses REACT_RENDERER_URL, so the node-renderer README should be updated to match.
Clarification needed: workersCount env variable
The node-renderer README config table says the env variable for workersCount is RENDERER_WORKERS_COUNT. But the generator template (client/node-renderer.js) reads NODE_RENDERER_CONCURRENCY and passes it manually as workersCount. These are different things — the renderer supports both (via its internal env and via the JS config object), but someone following the node-renderer README while also using the generator-created file will be confused about which env var to set. The table footnote or a note in the README should clarify this.
| @@ -0,0 +1,106 @@ | |||
| # react-on-rails | |||
There was a problem hiding this comment.
This README will not be included in the npm publish. packages/react-on-rails/package.json has a files field that only matches lib/**/*.js, lib/**/*.cjs, lib/**/*.mjs, and lib/**/*.d.ts — README.md does not match any of these patterns.
Fix: add "README.md" to the files array in packages/react-on-rails/package.json:
| # react-on-rails | |
| # react-on-rails |
(The README content is fine; the change needed is in package.json, not here.)
"files": [
"lib/**/*.js",
"lib/**/*.cjs",
"lib/**/*.mjs",
"lib/**/*.d.ts",
"README.md"
]There was a problem hiding this comment.
Fixed — added "README.md" to files array in packages/react-on-rails/package.json.
| @@ -0,0 +1,97 @@ | |||
| # react-on-rails-pro | |||
There was a problem hiding this comment.
Same issue as the base package: packages/react-on-rails-pro/package.json has:
"files": [
"lib/**/*.js",
"lib/**/*.d.ts"
]README.md is excluded. Add "README.md" to that files array to ensure it gets published to npm.
There was a problem hiding this comment.
Fixed — added "README.md" to files array in packages/react-on-rails-pro/package.json.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/getting-started/pro-quick-start.md (2)
149-149: Broken link (404) confirmed by CI pipeline — already notedThis was flagged in a previous review round and confirmed by the GitHub Actions "Check Markdown Links" pipeline failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/pro-quick-start.md` at line 149, The markdown contains a broken external link for "Code Splitting" pointing to https://www.shakacode.com/react-on-rails-pro/docs/code-splitting/; replace the dead URL with a working target (either the current ShakaCode page for code-splitting or an alternative authoritative resource) or remove the link if no replacement exists, ensuring the link text "Code Splitting" remains accurate; update the link in the pro-quick-start document so the CI "Check Markdown Links" pipeline no longer reports a 404.
107-115:require('path')is missing and env var name is inconsistent — already notedThis was flagged in a previous review round.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/pro-quick-start.md` around lines 107 - 115, Add the missing require('path') at the top and make the environment variable names consistent: import path (e.g., const path = require('path')) and ensure you use the same naming scheme for port and concurrency (either both RENDERER_* or both NODE_RENDERER_*); update the reactOnRailsProNodeRenderer call (reactOnRailsProNodeRenderer, serverBundleCachePath, port, workersCount) to reference the chosen env vars (e.g., process.env.RENDERER_PORT and process.env.RENDERER_CONCURRENCY or process.env.NODE_RENDERER_PORT and process.env.NODE_RENDERER_CONCURRENCY) so names are no longer inconsistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails-pro/README.md`:
- Around line 44-47: Update the unlabeled code fence in the README diagram to
include a language specifier (e.g., change the opening ``` to ```text or
```ascii) so the ASCII-art block in the README.md is labeled and satisfies
MD040; ensure the opening fence is updated and the closing ``` remains present
around the block containing "react-on-rails-pro (this package) └──
react-on-rails (base package, automatically installed as dependency)".
---
Duplicate comments:
In `@docs/getting-started/pro-quick-start.md`:
- Line 149: The markdown contains a broken external link for "Code Splitting"
pointing to https://www.shakacode.com/react-on-rails-pro/docs/code-splitting/;
replace the dead URL with a working target (either the current ShakaCode page
for code-splitting or an alternative authoritative resource) or remove the link
if no replacement exists, ensuring the link text "Code Splitting" remains
accurate; update the link in the pro-quick-start document so the CI "Check
Markdown Links" pipeline no longer reports a 404.
- Around line 107-115: Add the missing require('path') at the top and make the
environment variable names consistent: import path (e.g., const path =
require('path')) and ensure you use the same naming scheme for port and
concurrency (either both RENDERER_* or both NODE_RENDERER_*); update the
reactOnRailsProNodeRenderer call (reactOnRailsProNodeRenderer,
serverBundleCachePath, port, workersCount) to reference the chosen env vars
(e.g., process.env.RENDERER_PORT and process.env.RENDERER_CONCURRENCY or
process.env.NODE_RENDERER_PORT and process.env.NODE_RENDERER_CONCURRENCY) so
names are no longer inconsistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/getting-started/pro-quick-start.mdllms.txtpackages/react-on-rails-pro-node-renderer/README.mdpackages/react-on-rails-pro/README.mdpackages/react-on-rails/README.mdreact_on_rails_pro/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro-node-renderer/README.md
- llms.txt
Review: Documentation Accuracy IssuesGood effort on centralizing the LLM-discoverability content — the motivation is sound and most of the changes are accurate. However, I found several factual inconsistencies across the new docs that could mislead developers more than the current state of affairs. These should be fixed before merging. Critical: Wrong env variable name in node-renderer READMEIn config.renderer_url = ENV["RENDERER_URL"] || "http://localhost:3800"But the actual generator template ( Incorrect
|
| config.renderer_url = ENV["RENDERER_URL"] || "http://localhost:3800" | ||
| config.renderer_password = ENV["RENDERER_PASSWORD"] |
There was a problem hiding this comment.
Wrong env variable name — RENDERER_URL should be REACT_RENDERER_URL. The generator template (lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt) and all other docs (pro-quick-start.md, llms.txt) consistently use REACT_RENDERER_URL. Using RENDERER_URL here will silently fall back to the hardcoded default and the env var will have no effect.
Also note the missing default for renderer_password — the generator template uses ENV.fetch("RENDERER_PASSWORD", "devPassword") for development parity.
| config.renderer_url = ENV["RENDERER_URL"] || "http://localhost:3800" | |
| config.renderer_password = ENV["RENDERER_PASSWORD"] | |
| config.renderer_url = ENV.fetch("REACT_RENDERER_URL", "http://localhost:3800") | |
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") |
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | ||
|
|
||
| reactOnRailsProNodeRenderer({ | ||
| serverBundleCachePath: path.resolve(__dirname, './.node-renderer-bundles'), |
There was a problem hiding this comment.
Wrong relative path. Since the generator places this file at client/node-renderer.js, __dirname is the client/ directory. Using './.node-renderer-bundles' puts the cache inside client/, not the project root. The actual generator template uses '../.node-renderer-bundles' to resolve to the project root.
Also missing the const path = require('path'); import at the top of this snippet.
| serverBundleCachePath: path.resolve(__dirname, './.node-renderer-bundles'), | |
| serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'), |
| ## What the generator configured | ||
|
|
||
| ### Rails-side (config/initializers/react_on_rails_pro.rb) | ||
|
|
||
| ```ruby | ||
| ReactOnRailsPro.configure do |config| | ||
| config.server_renderer = "NodeRenderer" | ||
| config.renderer_url = ENV.fetch("REACT_RENDERER_URL", "http://localhost:3800") | ||
| config.renderer_password = ENV.fetch("RENDERER_PASSWORD", "devPassword") | ||
| config.prerender_caching = true | ||
| end | ||
| ``` |
There was a problem hiding this comment.
The section title "What the generator configured" sets the expectation of accuracy, but the snippet is significantly simplified compared to the actual generated file. The real generator template also includes ssr_timeout, renderer_request_retry_limit, renderer_use_fallback_exec_js, tracing, etc. A developer who looks at their generated file will find it doesn't match.
Consider either:
- Renaming this to "Minimal configuration example" and noting the generator produces a fuller version
- Showing the complete output that matches the actual template
Otherwise developers opening their generated initializer will be confused by options not shown here.
| ```js | ||
| import masterRun from 'react-on-rails-pro-node-renderer/master'; | ||
| import run, { configureFastify } from 'react-on-rails-pro-node-renderer/worker'; | ||
| import cluster from 'cluster'; |
There was a problem hiding this comment.
The Advanced section switches to ESM import syntax without any note about it, while the entire Quick Start and the actual generator template use CommonJS require(). The package doesn't declare "type": "module" in its package.json, so CJS is the default.
Consider adding a note clarifying this is the ESM usage pattern, or converting to require() to be consistent with the Quick Start above.
| | `logHttpLevel` | `RENDERER_LOG_HTTP_LEVEL` | `'error'` | HTTP server log level | | ||
| | `serverBundleCachePath` | `RENDERER_SERVER_BUNDLE_CACHE_PATH` | Auto-detected or `/tmp/...` | Directory for cached server bundles | | ||
| | `supportModules` | `RENDERER_SUPPORT_MODULES` | `false` | Enable Node.js globals in VM context (`Buffer`, `process`, `setTimeout`, etc.) | | ||
| | `workersCount` | `RENDERER_WORKERS_COUNT` | CPU count - 1 | Number of worker processes | |
There was a problem hiding this comment.
The env variable name here (RENDERER_WORKERS_COUNT) doesn't match what the generator template uses. The actual generator template (lib/generators/react_on_rails/templates/pro/base/client/node-renderer.js) reads NODE_RENDERER_CONCURRENCY:
workersCount: env.NODE_RENDERER_CONCURRENCY != null ? Number(env.NODE_RENDERER_CONCURRENCY) : 3,If the package itself reads RENDERER_WORKERS_COUNT internally (separate from the generator config), this needs a clarifying note. If not, this documents a non-functional env var name for the most common use case (following the generator).
…ability LLMs consistently fail to generate correct React on Rails Pro setup code because the npm packages have no READMEs, there's no single quick-start page, and the correct API names are not discoverable from training data. Changes: - Add README.md to all 3 npm packages (react-on-rails, react-on-rails-pro, react-on-rails-pro-node-renderer) with correct API examples, config options, and package relationship documentation - Fix incorrect example in Pro gem README (wrong scoped package name @shakacode-tools/... and deprecated bundlePath config key) - Add docs/getting-started/pro-quick-start.md — a single self-contained guide for setting up a Pro app from scratch using the --pro generator - Add llms.txt at repo root following the llms.txt convention, providing LLMs with correct package relationships, API names, and config keys Addresses #2465 Co-Authored-By: Claude Opus 4.6 <[email protected]>
All React on Rails Pro gems and npm packages are now in the public monorepo. Remove "you will need access to our private repository" notes, update GitHub links from the old shakacode/react_on_rails_pro repo to shakacode/react_on_rails monorepo paths, remove obsolete GitHub Packages publishing instructions from releasing guide, and fix tense in the upgrade guide. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…and npm publish
- Fix broken code-splitting link (404) with correct URL
- Add missing require('path') and fix serverBundleCachePath in quick-start
- Fix RENDERER_URL -> REACT_RENDERER_URL to match generator template
- Add password field and devPassword default in examples
- Add README.md to files array in react-on-rails and react-on-rails-pro
package.json so READMEs are included in npm publish
- Add language specifiers to code blocks (MD040)
- Add ESM note for advanced section in node-renderer README
- Remove unused setupTracing import from error reporting example
- Clarify NODE_RENDERER_CONCURRENCY vs RENDERER_WORKERS_COUNT env vars
- Fix confusing "base gem will reject" sentence
Co-Authored-By: Claude Opus 4.6 <[email protected]>
AbanoubGhadban
force-pushed
the
abanoub/improve-llm-discoverability
branch
from
dd8f302 to
71b1345
Compare
|
|
||
| ### Correct usage: | ||
| ```js | ||
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
There was a problem hiding this comment.
The code example uses path.resolve(__dirname, ...) on line 48 but never imports the path module. Since llms.txt is specifically designed to train LLMs to write correct code, this bug directly undermines the file's purpose.
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | |
| const path = require('path'); | |
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
| ## Step 1: Create a new Rails app | ||
|
|
||
| ```bash | ||
| rails new my-pro-app --skip-javascript --skip-docker |
There was a problem hiding this comment.
The --skip-docker flag was added in Rails 7.1. The prerequisites above state "Rails 7.0+" which would cause rails new to fail with an unknown flag error on 7.0.x.
Either update the prerequisite to "Rails 7.1+" or drop --skip-docker from the command.
| | `logHttpLevel` | `RENDERER_LOG_HTTP_LEVEL` | `'error'` | HTTP server log level | | ||
| | `serverBundleCachePath` | `RENDERER_SERVER_BUNDLE_CACHE_PATH` | Auto-detected or `/tmp/...` | Directory for cached server bundles | | ||
| | `supportModules` | `RENDERER_SUPPORT_MODULES` | `false` | Enable Node.js globals in VM context (`Buffer`, `process`, `setTimeout`, etc.) | | ||
| | `workersCount` | `RENDERER_WORKERS_COUNT` | CPU count - 1 | Number of worker processes. The `--pro` generator uses `NODE_RENDERER_CONCURRENCY` instead. | |
There was a problem hiding this comment.
The footnote in the table cell ("The --pro generator uses NODE_RENDERER_CONCURRENCY instead") is easy to miss and contradicts the example just above it. Users following the quick-start guide will set NODE_RENDERER_CONCURRENCY, then read this README and wonder why the env var name is different.
Consider splitting this into two rows or adding a dedicated "Generator vs. manual setup" note at the top of the configuration section explaining that the generator scaffolds NODE_RENDERER_CONCURRENCY while the canonical env var supported by the package is RENDERER_WORKERS_COUNT.
|
|
||
| ```ruby | ||
| # Append to Gemfile | ||
| gem "shakapacker", "~> 9.5" |
There was a problem hiding this comment.
Pinning to exact minor versions (~> 9.5, ~> 16.3) means this guide will show version errors on every minor release. Quick-start docs tend to stay around longer than they should, so they'll mislead users once a new minor version ships.
Consider using major-only pessimistic constraints (~> 9, ~> 16) or dropping the version constraints entirely and letting bundle add pick the latest.
Review: Add npm READMEs, Pro quick-start guide, and llms.txtThis is a well-motivated and largely well-executed documentation PR. The core idea — adding npm READMEs as the primary LLM training signal, consolidating setup into a single quick-start, and shipping an Issues found1. Bug: missing 2. 3. 4. Minor: tight version pinning in quick-start will go stale (inline comment) Already-confirmed non-issues
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-on-rails-pro/README.md`:
- Around line 33-40: The example imports RSCRoute, registerServerComponent, and
wrapServerComponentRenderer but then references an undefined MyServerComponent;
update the snippet to either import or define MyServerComponent (for example:
import MyServerComponent from './MyServerComponent' or provide a short
function/component declaration) and then call registerServerComponent({
MyServerComponent }); so the example is runnable; ensure the symbol
MyServerComponent is present in the snippet and consistent with
RSCRoute/registerServerComponent usage.
- Around line 58-70: The exports table in the README is missing public export
paths declared in package.json; update the table to include the omitted exports
such as `react-on-rails-pro/ReactOnRails.node` and
`react-on-rails-pro/ServerComponentFetchError` (and any other paths listed in
package.json) so documentation matches actual package exports; locate the README
Exports section (the table currently listing `react-on-rails-pro`,
`react-on-rails-pro/client`, `react-on-rails-pro/RSCRoute`, etc.) and add rows
for each missing public path with a short description referencing the
corresponding symbols (ReactOnRails.node, ServerComponentFetchError).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/contributor-info/releasing.mddocs/getting-started/pro-quick-start.mdllms.txtpackages/react-on-rails-pro-node-renderer/README.mdpackages/react-on-rails-pro/README.mdpackages/react-on-rails-pro/package.jsonpackages/react-on-rails/README.mdpackages/react-on-rails/package.jsonreact_on_rails_pro/README.mdreact_on_rails_pro/docs/code-splitting-loadable-components.mdreact_on_rails_pro/docs/ruby-api.mdreact_on_rails_pro/docs/updating.md
💤 Files with no reviewable changes (1)
- docs/contributor-info/releasing.md
🚧 Files skipped from review as they are similar to previous changes (5)
- react_on_rails_pro/docs/updating.md
- packages/react-on-rails/README.md
- packages/react-on-rails-pro-node-renderer/README.md
- llms.txt
- docs/getting-started/pro-quick-start.md
| ```javascript | ||
| import { RSCRoute } from 'react-on-rails-pro/RSCRoute'; | ||
| import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client'; | ||
| import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client'; | ||
|
|
||
| // Register a server component for client-side hydration | ||
| registerServerComponent({ MyServerComponent }); | ||
| ``` |
There was a problem hiding this comment.
RSC example uses an undefined symbol (MyServerComponent).
Line 39 references MyServerComponent but the snippet never defines/imports it, so the quick-start example is not directly runnable.
🐛 Proposed doc fix
import { RSCRoute } from 'react-on-rails-pro/RSCRoute';
import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client';
import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client';
+import MyServerComponent from './MyServerComponent';
// Register a server component for client-side hydration
registerServerComponent({ MyServerComponent });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```javascript | |
| import { RSCRoute } from 'react-on-rails-pro/RSCRoute'; | |
| import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client'; | |
| import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client'; | |
| // Register a server component for client-side hydration | |
| registerServerComponent({ MyServerComponent }); | |
| ``` | |
| import { RSCRoute } from 'react-on-rails-pro/RSCRoute'; | |
| import registerServerComponent from 'react-on-rails-pro/registerServerComponent/client'; | |
| import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client'; | |
| import MyServerComponent from './MyServerComponent'; | |
| // Register a server component for client-side hydration | |
| registerServerComponent({ MyServerComponent }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro/README.md` around lines 33 - 40, The example
imports RSCRoute, registerServerComponent, and wrapServerComponentRenderer but
then references an undefined MyServerComponent; update the snippet to either
import or define MyServerComponent (for example: import MyServerComponent from
'./MyServerComponent' or provide a short function/component declaration) and
then call registerServerComponent({ MyServerComponent }); so the example is
runnable; ensure the symbol MyServerComponent is present in the snippet and
consistent with RSCRoute/registerServerComponent usage.
| ## Exports | ||
|
|
||
| | Export Path | Description | | ||
| |------------|-------------| | ||
| | `react-on-rails-pro` | Main entry — full ReactOnRails API (same as base + Pro) | | ||
| | `react-on-rails-pro/client` | Client-only build (no SSR utilities) | | ||
| | `react-on-rails-pro/RSCRoute` | React Server Components route component | | ||
| | `react-on-rails-pro/RSCProvider` | RSC provider component | | ||
| | `react-on-rails-pro/registerServerComponent/client` | Client-side server component registration | | ||
| | `react-on-rails-pro/registerServerComponent/server` | Server-side server component registration | | ||
| | `react-on-rails-pro/wrapServerComponentRenderer/client` | Client-side renderer wrapping | | ||
| | `react-on-rails-pro/wrapServerComponentRenderer/server` | Server-side renderer wrapping | | ||
|
|
There was a problem hiding this comment.
Exports table is missing public paths exposed by the package.
The table omits exports that exist in package.json (e.g., react-on-rails-pro/ReactOnRails.node and react-on-rails-pro/ServerComponentFetchError), which weakens the discoverability objective.
🧩 Proposed doc addition
| `react-on-rails-pro/wrapServerComponentRenderer/client` | Client-side renderer wrapping |
| `react-on-rails-pro/wrapServerComponentRenderer/server` | Server-side renderer wrapping |
+| `react-on-rails-pro/ReactOnRails.node` | Node-targeted entry point |
+| `react-on-rails-pro/ServerComponentFetchError` | Error class for server-component fetch failures |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Exports | |
| | Export Path | Description | | |
| |------------|-------------| | |
| | `react-on-rails-pro` | Main entry — full ReactOnRails API (same as base + Pro) | | |
| | `react-on-rails-pro/client` | Client-only build (no SSR utilities) | | |
| | `react-on-rails-pro/RSCRoute` | React Server Components route component | | |
| | `react-on-rails-pro/RSCProvider` | RSC provider component | | |
| | `react-on-rails-pro/registerServerComponent/client` | Client-side server component registration | | |
| | `react-on-rails-pro/registerServerComponent/server` | Server-side server component registration | | |
| | `react-on-rails-pro/wrapServerComponentRenderer/client` | Client-side renderer wrapping | | |
| | `react-on-rails-pro/wrapServerComponentRenderer/server` | Server-side renderer wrapping | | |
| ## Exports | |
| | Export Path | Description | | |
| |------------|-------------| | |
| | `react-on-rails-pro` | Main entry — full ReactOnRails API (same as base + Pro) | | |
| | `react-on-rails-pro/client` | Client-only build (no SSR utilities) | | |
| | `react-on-rails-pro/RSCRoute` | React Server Components route component | | |
| | `react-on-rails-pro/RSCProvider` | RSC provider component | | |
| | `react-on-rails-pro/registerServerComponent/client` | Client-side server component registration | | |
| | `react-on-rails-pro/registerServerComponent/server` | Server-side server component registration | | |
| | `react-on-rails-pro/wrapServerComponentRenderer/client` | Client-side renderer wrapping | | |
| | `react-on-rails-pro/wrapServerComponentRenderer/server` | Server-side renderer wrapping | | |
| | `react-on-rails-pro/ReactOnRails.node` | Node-targeted entry point | | |
| | `react-on-rails-pro/ServerComponentFetchError` | Error class for server-component fetch failures | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro/README.md` around lines 58 - 70, The exports
table in the README is missing public export paths declared in package.json;
update the table to include the omitted exports such as
`react-on-rails-pro/ReactOnRails.node` and
`react-on-rails-pro/ServerComponentFetchError` (and any other paths listed in
package.json) so documentation matches actual package exports; locate the README
Exports section (the table currently listing `react-on-rails-pro`,
`react-on-rails-pro/client`, `react-on-rails-pro/RSCRoute`, etc.) and add rows
for each missing public path with a short description referencing the
corresponding symbols (ReactOnRails.node, ServerComponentFetchError).
- Run prettier on all markdown files to fix formatting - Fix incorrect .js extension to .jsx for loadable-client.imports-hmr link in code-splitting docs (was already wrong in original doc) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Review SummaryThis is a well-structured documentation PR that addresses a clear problem (LLM hallucinations due to poor documentation discoverability). The overall approach — adding npm READMEs, a quick-start guide, and A few issues to address: Bugs in code examples
npm publish gap
Minor
|
|
|
||
| ### Correct usage: | ||
| ```js | ||
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
There was a problem hiding this comment.
Missing const path = require('path'); before this line. path.resolve() is called on line 48 but path is never imported in this example, so copying it verbatim will throw ReferenceError: path is not defined.
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | |
| const path = require('path'); | |
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
| password: process.env.RENDERER_PASSWORD, | ||
|
|
||
| // Number of worker processes (defaults to CPU count - 1) | ||
| workersCount: Number(process.env.RENDERER_WORKERS_COUNT) || 3, |
There was a problem hiding this comment.
This uses RENDERER_WORKERS_COUNT, but the --pro generator template actually uses NODE_RENDERER_CONCURRENCY (with a != null check to allow 0 as a valid value):
workersCount: env.NODE_RENDERER_CONCURRENCY != null ? Number(env.NODE_RENDERER_CONCURRENCY) : 3,Users who follow this Quick Start example will get different env-var behavior than the generator creates. Suggest aligning with the generator here to avoid confusion, with a note that both env vars are supported (per the config table below).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started/pro-quick-start.md`:
- Around line 140-144: Update the RSC "fresh app" generator command to include
the Pro flag: locate the snippet using the command string "rails generate
react_on_rails:install --rsc" and change it to include "--pro" (e.g., "rails
generate react_on_rails:install --rsc --pro") so the Pro quick-start remains
consistent with other guide instructions.
In `@packages/react-on-rails-pro-node-renderer/README.md`:
- Around line 40-42: The README snippet is missing a development fallback for
the Node renderer password causing mismatch with the Rails initializer; update
the Node example where `password: process.env.RENDERER_PASSWORD` is used so it
falls back to "devPassword" (same default used by the Rails
`config.renderer_password`/ENV.fetch("RENDERER_PASSWORD", "devPassword")
pattern), ensuring the example and initializer stay consistent and avoid auth
failures when RENDERER_PASSWORD is not exported.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/getting-started/pro-quick-start.mdpackages/react-on-rails-pro-node-renderer/README.mdpackages/react-on-rails-pro/README.mdpackages/react-on-rails/README.mdreact_on_rails_pro/docs/code-splitting-loadable-components.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-on-rails/README.md
- packages/react-on-rails-pro/README.md
- react_on_rails_pro/docs/code-splitting-loadable-components.md
| Or for a fresh app with RSC from the start: | ||
|
|
||
| ```bash | ||
| rails generate react_on_rails:install --rsc | ||
| ``` |
There was a problem hiding this comment.
RSC “fresh app” command should include Pro flag
This is a Pro quick-start, but the command uses only --rsc. It should include --pro to keep setup consistent with the rest of the guide.
🔧 Suggested fix
-rails generate react_on_rails:install --rsc
+rails generate react_on_rails:install --pro --rsc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/getting-started/pro-quick-start.md` around lines 140 - 144, Update the
RSC "fresh app" generator command to include the Pro flag: locate the snippet
using the command string "rails generate react_on_rails:install --rsc" and
change it to include "--pro" (e.g., "rails generate react_on_rails:install --rsc
--pro") so the Pro quick-start remains consistent with other guide instructions.
| // Password for Rails <-> Node renderer authentication (must match Rails config) | ||
| password: process.env.RENDERER_PASSWORD, | ||
|
|
There was a problem hiding this comment.
Quick Start password defaults are inconsistent and can break auth
Rails defaults renderer_password to "devPassword" (Line 55), but the Node snippet uses process.env.RENDERER_PASSWORD without fallback (Line 41). Following the README without exporting RENDERER_PASSWORD can cause renderer auth failures.
🔧 Suggested fix
reactOnRailsProNodeRenderer({
@@
- password: process.env.RENDERER_PASSWORD,
+ password: process.env.RENDERER_PASSWORD || 'devPassword',Also applies to: 54-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-on-rails-pro-node-renderer/README.md` around lines 40 - 42,
The README snippet is missing a development fallback for the Node renderer
password causing mismatch with the Rails initializer; update the Node example
where `password: process.env.RENDERER_PASSWORD` is used so it falls back to
"devPassword" (same default used by the Rails
`config.renderer_password`/ENV.fetch("RENDERER_PASSWORD", "devPassword")
pattern), ensuring the example and initializer stay consistent and avoid auth
failures when RENDERER_PASSWORD is not exported.
…rc.5 Since commit 97b3d72, react_on_rails_pro explicitly requires react_on_rails, so users only need one gem in their Gemfile. Added a note with the old two-gem approach for older versions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | ||
|
|
||
| reactOnRailsProNodeRenderer({ | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), |
There was a problem hiding this comment.
Bug: path is used but never imported.
path.resolve(...) is called on this line, but const path = require('path'); is missing. This would produce a ReferenceError: path is not defined if an LLM copies the snippet directly.
The fix:
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | |
| const path = require('path'); | |
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | |
| reactOnRailsProNodeRenderer({ | |
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), |
(The path import should be added two lines above, before the require call.)
|
|
||
| reactOnRailsProNodeRenderer({ | ||
| // Directory where the renderer caches uploaded server bundles | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), |
There was a problem hiding this comment.
Path mismatch with the generator template.
The generator places node-renderer.js inside client/, so __dirname resolves to the client/ directory. Using .node-renderer-bundles (no leading ../) puts the cache at client/.node-renderer-bundles, while the generator template uses '../ to put it at the project root:
// react_on_rails/lib/generators/react_on_rails/templates/pro/base/client/node-renderer.js
serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'),This README is the primary reference for users who set up the renderer manually (not via the generator), and the inconsistency will cause confusion when they compare with existing apps generated by --pro.
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | |
| serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'), |
|
|
||
| reactOnRailsProNodeRenderer({ | ||
| bundlePath: path.resolve(__dirname, '../app/assets/webpack'), | ||
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), |
There was a problem hiding this comment.
Same path inconsistency as in the node-renderer package README.
The generator places node-renderer.js under client/, so __dirname is the client/ directory. The generator template uses '../ to resolve the cache relative to the project root:
// generator template (client/node-renderer.js)
serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'),Using .node-renderer-bundles without ../ would place the cache inside client/ instead of at the project root.
| serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'), | |
| serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'), |
Review SummaryThis PR is a valuable documentation improvement — adding npm READMEs, a Pro quick-start guide, and an Bugs1. Missing The Node Renderer API example calls 2. Wrong Both files use: serverBundleCachePath: path.resolve(__dirname, '.node-renderer-bundles'),But the generator template (the authoritative source) uses: serverBundleCachePath: path.resolve(__dirname, '../.node-renderer-bundles'),Since Minor Observations
|
The NormalModuleReplacement example link pointed to a specific commit in the archived react_on_rails_pro repo. Updated to the equivalent file in the monorepo (commonWebpackConfig.js). Co-Authored-By: Claude Opus 4.6 <[email protected]>
PR Review: Add npm READMEs, Pro quick-start guide, and llms.txtThis is a great initiative to improve LLM code generation accuracy for React on Rails Pro. The overall structure and content is solid. I found two factual errors that would directly undermine the PR's goal (LLMs would generate broken code from them), plus one package publishing gap. Critical issues1. The Node Renderer API example uses 2. The client-side Source of truth: Minor issue3. The PR adds What looks good
|
|
|
||
| ### Correct usage: | ||
| ```js | ||
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
There was a problem hiding this comment.
Missing const path = require('path'); — path.resolve() is used on the next lines but path is never declared. This will throw a ReferenceError if an LLM copies this example verbatim.
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); | |
| const path = require('path'); | |
| const { reactOnRailsProNodeRenderer } = require('react-on-rails-pro-node-renderer'); |
| import { wrapServerComponentRenderer } from 'react-on-rails-pro/wrapServerComponentRenderer/client'; | ||
|
|
||
| // Register a server component for client-side hydration | ||
| registerServerComponent({ MyServerComponent }); |
There was a problem hiding this comment.
Wrong call signature. The client registerServerComponent takes rest string arguments, not an object:
// packages/react-on-rails-pro/src/registerServerComponent/client.tsx
const registerServerComponent = (...componentNames: string[]) => { ... }| registerServerComponent({ MyServerComponent }); | |
| registerServerComponent('MyServerComponent'); |
Passing an object ({ MyServerComponent }) will silently register a component named "[object Object]" rather than "MyServerComponent", which is exactly the kind of subtle bug this PR aims to prevent LLMs from generating.
1 participant
Summary
LLMs consistently fail to generate correct React on Rails Pro setup code. After deep analysis (#2465), the root causes are:
@shakacode-tools/scoped name and deprecatedbundlePathThis PR addresses the low-effort, high-impact recommendations from #2465.
Changes
react-on-rails,react-on-rails-pro,react-on-rails-pro-node-renderer) with correct API examples, config option tables, and package relationship docs@shakacode-tools/react-on-rails-pro-node-renderer→react-on-rails-pro-node-renderer, deprecatedbundlePath→serverBundleCachePathdocs/getting-started/pro-quick-start.md— single self-contained guide for Pro setup from scratch using the--progeneratorllms.txtat repo root — follows the llms.txt convention providing LLMs with correct package relationships, API names, and config keysWhat this does NOT include
--pro/--rsc) — already merged in Add --pro and --rsc flags to install generator #2284react_on_rails_progem — open in Auto-install react_on_rails_pro gem when --rsc/--pro flags are used #2439Test plan
packages/react-on-rails-pro-node-renderer/lib/)rails new+rails generate react_on_rails:install --prollms.txtconfig examples match the generator templates inlib/generators/react_on_rails/templates/pro/filesfield excluding them, orfilesfield includesREADME.md)Closes #2465
🤖 Generated with Claude Code
Summary by CodeRabbit