Fix doctor false positives and stabilize default RSC starter#2478
Fix doctor false positives and stabilize default RSC starter#2478
Conversation
WalkthroughThis pull request removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 makes two targeted improvements:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rails react_on_rails:doctor] --> B[check_react_on_rails_npm_package]
B --> C{react_on_rails_npm_package_details}
C -->|react-on-rails-pro found| D["✅ Pro package detected"]
C -->|react-on-rails found| E["✅ OSS package detected"]
C -->|neither found| F["⚠️ Warning with both options"]
A --> G[check_react_dependencies]
G --> H{required_react_dependencies}
H --> I[using_babel_transpiler?]
I --> J{detected_javascript_transpiler}
J -->|shakapacker.yml exists| K[parsed_shakapacker_config]
K --> L{javascript_transpiler value}
L -->|babel / nil| M["Include @babel/preset-react check"]
L -->|swc| N["Skip @babel/preset-react check"]
J -->|no config file| M
A --> O[check_package_version_sync]
O --> C
Last reviewed commit: e1bf0b7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| end | ||
|
|
||
| def react_on_rails_npm_package_details(package_json) | ||
| all_deps = package_json["dependencies"]&.merge(package_json["devDependencies"] || {}) || {} |
There was a problem hiding this comment.
Pre-existing: nil dependencies drops devDependencies
If package_json["dependencies"] is nil (e.g., a project that only has devDependencies), the safe-navigation &.merge(...) returns nil, and nil || {} yields an empty hash — silently dropping everything in devDependencies. A package listed only in devDependencies would not be found.
This is a pre-existing pattern used throughout the file (7 occurrences), so not a regression from this PR, but worth noting since this method is now the single source of truth for Pro/OSS detection. A safer alternative would be:
all_deps = (package_json["dependencies"] || {}).merge(package_json["devDependencies"] || {})
![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.
| end | ||
|
|
||
| def react_on_rails_npm_package_details(package_json) | ||
| all_deps = package_json["dependencies"]&.merge(package_json["devDependencies"] || {}) || {} |
There was a problem hiding this comment.
Merge pattern loses devDependencies when dependencies key absent
Medium Severity
The react_on_rails_npm_package_details method uses package_json["dependencies"]&.merge(package_json["devDependencies"] || {}) || {} to combine deps. When a package.json has no "dependencies" key at all (only "devDependencies"), the safe navigation operator returns nil, and nil || {} discards all devDependencies. This is a regression from the old code which used independent dig calls for each section, correctly finding packages in either location regardless of whether the other key existed.
Review: Fix doctor false positives and stabilize default RSC starterOverall this is a solid, well-scoped fix. The logic is correct and the test coverage is good. A few things worth discussing:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| raw_content = File.read(shakapacker_config_path) | ||
| rendered_content = ERB.new(raw_content).result | ||
| YAML.safe_load(rendered_content, aliases: true) || {} | ||
| rescue StandardError |
There was a problem hiding this comment.
The broad rescue StandardError here will silently swallow meaningful errors. If shakapacker.yml contains invalid ERB (e.g., <%= undefined_method %>), the ERB::SyntaxError is caught, nil is returned, and using_babel_transpiler? falls back to true — causing a spurious @babel/preset-react warning with no indication of why. Consider rescuing specific, expected exceptions instead:
| rescue StandardError | |
| rescue Errno::ENOENT, Errno::EACCES, Psych::SyntaxError, YAML::SyntaxError | |
| nil | |
| rescue ERB::SyntaxError, SyntaxError | |
| # ERB evaluation failed — fall back to requiring babel but don't crash doctor | |
| nil |
Or at minimum log the error via warn so developers can diagnose the issue.
| deps | ||
| end | ||
|
|
||
| def react_on_rails_npm_package_details(package_json) |
There was a problem hiding this comment.
merge here gives devDependencies precedence over dependencies when the same package key exists in both. The original code checked dependencies first (higher precedence). To restore that behavior:
| def react_on_rails_npm_package_details(package_json) | |
| all_deps = (package_json["devDependencies"] || {}).merge(package_json["dependencies"] || {}) |
This is a minor edge case (unlikely anyone has the same package in both sections), but the intent should be explicit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/system_checker.rb (1)
488-497: Consider the safety ofERB.new(raw_content).resultin edge cases.
ERB#resultwithout an explicit binding uses the top-level binding. Ifshakapacker.ymlcontains ERB tags referencing Rails helpers (e.g.,Rails.root), this will fail before Rails is fully loaded. Therescue StandardErrorcovers it gracefully, but it silently swallows the error — a debug-level log could help users diagnose why the transpiler wasn't detected.That said, for a diagnostic tool this is an acceptable trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/lib/react_on_rails/system_checker.rb` around lines 488 - 497, In parsed_shakapacker_config, ERB.new(raw_content).result can raise errors when shakapacker.yml references top-level bindings; update the rescue in parsed_shakapacker_config to log the caught exception at debug level (e.g., process or Rails.logger.debug) including the error message and backtrace along with a short context string like "failed to parse config/shakapacker.yml"; keep returning nil after logging so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/react_on_rails/system_checker.rb`:
- Around line 488-497: In parsed_shakapacker_config, ERB.new(raw_content).result
can raise errors when shakapacker.yml references top-level bindings; update the
rescue in parsed_shakapacker_config to log the caught exception at debug level
(e.g., process or Rails.logger.debug) including the error message and backtrace
along with a short context string like "failed to parse config/shakapacker.yml";
keep returning nil after logging so behavior is unchanged.
justin808
added
codex
1 participant
Summary
Addresses two RSC follow-ups:
Fixes
rails react_on_rails:doctorfalse positives for Pro + SWC setups:react-on-rails-proas a valid npm package for React on Rails checks.react-on-railsorreact-on-rails-pro).@babel/preset-reactwhen Shakapackerjavascript_transpilerisbabel(skips that warning forswc).Stabilizes generated
--rscstarter output:HelloServersample server-only (removes defaultLikeButtonusage) to avoid fragile client-hook demo behavior in initial scaffold.Closes #2431
Closes #2432
Testing
mise exec ruby@3.3 -- bundle exec rspec react_on_rails/spec/lib/react_on_rails/system_checker_spec.rbNPM_CONFIG_CACHE=/tmp/npm-cache-ror-2431-2432 mise exec ruby@3.3 -- bundle exec rspec react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb:785 react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb:873mise exec ruby@3.3 -- bundle exec rubocop react_on_rails/lib/react_on_rails/system_checker.rb react_on_rails/spec/lib/react_on_rails/system_checker_spec.rb react_on_rails/spec/react_on_rails/generators/install_generator_spec.rbNote
Medium Risk
Touches the
SystemChecker/doctor logic and adds YAML+ERB config parsing, which can affect setup diagnostics across environments; the RSC starter change is low impact but alters generated output expectations.Overview
Improves
rails react_on_rails:doctorchecks to avoid false positives by recognizingreact-on-rails-proas a valid npm package, making gem/npm version-sync messages package-aware, and only requiring@babel/preset-reactwhen Shakapacker is configured to use thebabeltranspiler (notswc, withshakapacker.ymlparsed via ERB+YAML).Stabilizes the
--rscscaffold by removing the defaultLikeButtonusage from the generatedHelloServer(JS + TS) and updating the demo view copy accordingly, plus adds generator specs to ensure the starter remains server-only by default.Written by Cursor Bugbot for commit e1bf0b7. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Documentation