Improve fresh Pro install docs and generator defaults#2494
Improve fresh Pro install docs and generator defaults#2494
Conversation
Move vmCreationPromises cleanup from a try/finally inside the async IIFE to a .finally() on the promise chain after vmCreationPromises.set(). The IIFE's finally block runs synchronously before set() when code throws before the first await, leaving a rejected promise permanently stuck in the map and preventing retries for that bundle path. Add test verifying buildVM recovers after failure for a nonexistent bundle. Fixes #2483 Co-Authored-By: Claude Opus 4.6 <[email protected]>
WalkthroughDocumentation guides, generator logic, and development tooling are updated to improve Rails installation instructions with environment-variable-driven port configuration, SSR path setup, and placeholder-based version management. VM promise cleanup is refactored for concurrent safety, and corresponding recovery tests are added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 SummaryThis PR improves the fresh Pro install experience by addressing friction discovered during real-world testing of Key improvements:
Technical notes:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: e72bebd |
PR Review: Improve fresh Pro install docs and generator defaultsOverall this is a solid improvement — replacing hardcoded version numbers with explicit placeholders, adding the clean-git prerequisite, and auto-configuring private_output_path reduce a real pain point for new installers. The vm.ts concurrency fix is also correct and well-explained. A few issues worth addressing: configure_private_output_path_in_shakapacker bypasses Thor generator DSLThe new method uses File.read / File.write directly (lines 377, 398), while the adjacent configure_precompile_hook_in_shakapacker and configure_rspack_in_shakapacker use Thor's gsub_file for writes. Using raw File.write bypasses --pretend mode (dry-run), --force behavior, and the generator's change logging, creating an inconsistency. Consider splitting the two-step logic into separate gsub_file + insert_into_file calls, matching the pattern used by the other configure methods in the same file. Already-configured regex misses null / empty YAML valuesThe guard at line 380 (return if content.match?(/^\sprivate_output_path:\s\S+/)) won't match private_output_path: (empty value) or private_output_path: ~ (YAML null). If a user previously set one of those values, the generator will attempt to insert a second private_output_path key, producing invalid (ambiguous) YAML. The check should also cover the null/no-value form, e.g. content.match?(/^\s*private_output_path:/). Recovery test in vm.test.ts passes trivially when VM already existsThe new test at line 545 doesn't call resetVM() before running. If serverBundlePath is already present in vmContexts from an earlier test in the suite, buildVM(serverBundlePath) hits the early-return (line 189 – 'return Promise.resolve(true)') and the hasVMContextForBundle assertion passes without exercising the recovery path. Adding a resetVM() at the top of the test would make the coverage meaningful. Minor: Procfile.dev-static-assets uses a different process nameThe other two Procfiles use 'rails:' as the process name; Procfile.dev-static-assets uses 'web:'. That inconsistency predates this PR but is worth a follow-up since it affects how process names appear in overmind/foreman output. |
size-limit report 📦
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| return if updated_content == content | ||
|
|
||
| File.write(shakapacker_config_path, updated_content) |
There was a problem hiding this comment.
Using File.write here bypasses Thor's generator DSL. configure_precompile_hook_in_shakapacker (line 363) and configure_rspack_in_shakapacker (line 333) both use gsub_file for their writes, which respects --pretend (dry-run), --force, and logs the change in generator output.
Consider restructuring as two separate gsub_file / insert_into_file calls:
# Step 1: try to uncomment the placeholder line
gsub_file shakapacker_config_path,
/^(\s*)#\s*private_output_path:\s*.*$/,
"\\1private_output_path: ssr-generated"
# Step 2: if the placeholder wasn't there, insert after public_output_path
unless File.read(shakapacker_config_path).match?(/^\s*private_output_path: ssr-generated/)
insert_into_file shakapacker_config_path,
"\\1private_output_path: ssr-generated\n",
after: /^(\s*)public_output_path:.*\n/
endThis keeps the logic consistent with the rest of the generator.
| content = File.read(shakapacker_config_path) | ||
|
|
||
| # Already configured? Do nothing. | ||
| return if content.match?(/^\s*private_output_path:\s*\S+/) |
There was a problem hiding this comment.
The \S+ quantifier requires at least one non-whitespace character after the colon, so this guard won't fire for private_output_path: (empty) or private_output_path: ~ (YAML null). A user who previously added the key with a null value would get a duplicate key inserted by the fallback branch, producing ambiguous YAML.
Suggest broadening the guard to cover any existing key (regardless of value):
return if content.match?(/^\s*private_output_path:/)If you need to distinguish "set to a real value" from "set to null / empty", handle that case explicitly rather than silently re-inserting.
| expect(vmContext1).toBe(vmContext2); | ||
| }); | ||
|
|
||
| test('buildVM recovers after failure for nonexistent bundle', async () => { |
There was a problem hiding this comment.
This test doesn't call resetVM() before running. If serverBundlePath is already present in vmContexts from a prior test in the suite, buildVM(serverBundlePath) returns immediately via the early-exit branch (return Promise.resolve(true) on line ~192) and the hasVMContextForBundle assertion passes trivially — without actually verifying that the cleanup after failure enabled a fresh build.
Adding resetVM() at the start ensures the test exercises the actual recovery path:
test('buildVM recovers after failure for nonexistent bundle', async () => {
resetVM(); // start with a clean slate
const config = getConfig();
config.supportModules = true;
config.stubTimers = false;
const nonExistentPath = path.resolve(__dirname, './tmp/nonexistent-bundle.js');
// First call should fail because the bundle file does not exist
await expect(buildVM(nonExistentPath)).rejects.toThrow();
// After failure, the vmCreationPromises map should be cleaned up,
// allowing a retry with the correct path to succeed
await buildVM(serverBundlePath);
expect(hasVMContextForBundle(serverBundlePath)).toBeTruthy();
});
| // Remove from the map after the promise settles. Using .finally() on the | ||
| // stored promise guarantees this runs after set(), unlike a try/finally | ||
| // inside the IIFE which can run synchronously before set() on sync throws. | ||
| void vmCreationPromise |
There was a problem hiding this comment.
The .catch(() => {}).finally(...) chain is the right pattern here — the .catch is necessary to prevent an unhandled rejection on the void-ed chain (since .finally() on a rejected promise propagates the rejection through the tail). The comment could be a touch more explicit about this:
// Suppress the rejection on this internal chain so it doesn't surface as an
// unhandled rejection when the outer `void` discards the tail promise.
// The original `vmCreationPromise` is still returned to callers and will
// reject normally.
void vmCreationPromise
.catch(() => {})
.finally(() => {
vmCreationPromises.delete(filePath);
});No functional change needed — just a suggestion for clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/getting-started/installation-into-an-existing-rails-app.md (1)
24-29: Non-descriptive link text "here".The link on line 26 uses "here" which isn't descriptive per accessibility best practices. Consider using descriptive text.
📝 Suggested improvement
-2. Run the following command to install Shakapacker with React. Note, if you are using an older version of - Rails than 5.1, you'll need to install Webpacker with React per the instructions - [here](https://github.com/rails/webpacker). +2. Run the following command to install Shakapacker with React. Note, if you are using an older version of + Rails than 5.1, you'll need to install Webpacker with React per the + [Webpacker documentation](https://github.com/rails/webpacker).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/installation-into-an-existing-rails-app.md` around lines 24 - 29, Replace the non-descriptive link text "here" with a descriptive label so it’s accessible; in the markdown near the command example (the paragraph mentioning older Rails than 5.1 and the link whose text is "here"), change the link text to something meaningful like "Webpacker README on GitHub" or "Rails Webpacker repository" while keeping the same URL, so the link reads e.g. [Webpacker README on GitHub](https://github.com/rails/webpacker).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/getting-started/installation-into-an-existing-rails-app.md`:
- Around line 24-29: Replace the non-descriptive link text "here" with a
descriptive label so it’s accessible; in the markdown near the command example
(the paragraph mentioning older Rails than 5.1 and the link whose text is
"here"), change the link text to something meaningful like "Webpacker README on
GitHub" or "Rails Webpacker repository" while keeping the same URL, so the link
reads e.g. [Webpacker README on GitHub](https://github.com/rails/webpacker).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/getting-started/installation-into-an-existing-rails-app.mdpackages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.tsreact_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.devreact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assetsreact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assetsreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails_pro/docs/installation.md
![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.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
| # Procfile for development | ||
| # You can run these commands in separate shells | ||
| rails: bundle exec rails s -p 3000 | ||
| rails: bundle exec rails s -p ${PORT:-3000} |
There was a problem hiding this comment.
Procfile PORT default never used via process managers
Medium Severity
The ${PORT:-3000} default is effectively dead code when running through bin/dev. Both foreman and overmind assign their own PORT to each child process (defaulting to base 5000), so the :-3000 fallback is never reached. This changes the default Rails port from 3000 to 5000. The bin/dev Ruby script doesn't set ENV["PORT"] before invoking the process manager (unlike the standard Rails 7.1+ bin/dev pattern of export PORT="${PORT:-3000}"), so all downstream references to port 3000 — docs step 7, procfile_port, and print_server_info — become incorrect.
Additional Locations (2)
1 participant
Summary
private_output_path: ssr-generatedinconfig/shakapacker.ymlfor Shakapacker 9+ during generator setupPORToverrides in generated Procfiles (Procfile.dev,Procfile.dev-static-assets,Procfile.dev-prod-assets)private_output_pathbehaviorWhy
A real fresh-install run for
16.4.0.rc.5exposed avoidable friction:shakapacker.ymlPORT=...overrides ineffectiveTest Plan
mise exec [email protected] -- bundle exec rubocop react_on_rails/lib/generators/react_on_rails/base_generator.rb react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbmise exec [email protected] -- bundle exec rspec react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb --tag type:generatorNote
Medium Risk
Medium risk because it changes generator behavior that edits
config/shakapacker.ymland modifies node-renderer VM build concurrency cleanup; both can affect SSR build/runtime if edge cases are missed. Documentation updates are low risk, but the generator and VM changes warrant review/testing across Shakapacker versions and failure paths.Overview
Improves install docs to use version placeholders (including pre-release formatting), consistently recommend
bundle exec, call out the generator’s clean-git requirement, and document a quick post-install validation +PORT=...overrides.Updates the Rails generator to auto-configure
private_output_path: ssr-generatedinconfig/shakapacker.ymlfor Shakapacker 9+ (keeping SSR bundle paths aligned) and changes generated Procfiles to honor thePORTenvironment variable.Hardens the Pro node renderer’s
buildVMconcurrency tracking by ensuringvmCreationPromisescleanup happens via.finally()on the stored promise, and adds a regression test to confirm recovery after a failed VM build (e.g., missing bundle file).Written by Cursor Bugbot for commit e72bebd. Configure here.
Summary by CodeRabbit
Documentation
bundle execcommands and version placeholders for clearer setup instructions.New Features
Bug Fixes
Tests