🚀 Enhance bin/dev with configurable routes and improved production asset handling #1790
Conversation
…handling ## Major Improvements ### 🎯 Configurable Route Support - Add `--route` parameter to bin/dev for custom URL display - Default to root URL (/) instead of hardcoded /hello_world - Generators pass specific routes (hello_world) when creating demo apps - Clean abstraction in ReactOnRails::Dev::ServerManager ### 🛠️ Enhanced Error Handling - Comprehensive error messages for `bin/dev prod` failures - Specific guidance for missing secret_key_base, database issues, dependencies - Suggest `bin/dev static` as alternative for development with production-like assets - Color-coded, actionable error messages with specific commands ### 🏗️ Architecture Cleanup - Remove duplicate bin/dev files - single source of truth in templates/ - Abstract command-line logic into ServerManager.run_from_command_line() - Eliminate complex route detection - simple generator-based approach - Update install generator to copy directly from templates ### 🔧 Configuration Fixes - Add nil-safe operators for PackerUtils.packer_type calls - Improve handling when Shakapacker is not available - Better deprecation warnings with fallback values ## Benefits - ✅ Cleaner development workflow with configurable URLs - ✅ Better error messages guide users to solutions - ✅ Reduced code duplication and maintenance overhead - ✅ More robust configuration handling - ✅ Preserved backwards compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
## Improvement - Use Open3.capture3 to capture both stdout and stderr from asset precompilation - Display actual error output when bin/dev prod fails - Provide contextual fixes based on error content analysis - Smart detection of specific issues (secret_key_base, database, webpack, gems) ## Benefits - ✅ Users see the actual error message instead of generic failure - ✅ Contextual guidance based on error type - ✅ Better debugging experience for production asset compilation - ✅ No more hidden error output 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…nment ## New Feature - Add `--rails-env` option to `bin/dev prod` for custom Rails environment during assets:precompile - Default: NODE_ENV=production, RAILS_ENV=development (avoids credentials complexity) - Option: `--rails-env=production` for full production Rails environment - Clear documentation that option only affects assets:precompile, not server processes ## Enhanced Documentation - Comprehensive help text with examples and environment explanations - Clear runtime messaging about environment configuration - Separation of asset precompilation vs server process environments - Guidance on when to use each option ## Usage Examples ```bash bin/dev prod # Default: production webpack, development Rails bin/dev prod --rails-env=production # Full production environment bin/dev prod --route=dashboard # Custom route in URLs ``` ## Benefits - ✅ Production webpack optimizations without production Rails complexity by default - ✅ Option for full production environment when needed (testing credentials, etc.) - ✅ Clear documentation prevents confusion about scope - ✅ Better error handling with environment-specific guidance 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR centralizes bin/dev CLI handling into ServerManager.run_from_command_line, adds route and rails_env support across dev server modes, adjusts generator messages to include a route, updates generators to use template-based bin scripts, revises configuration messages, removes an old bin/dev, updates specs accordingly, and makes multiple documentation formatting edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Bin as bin/dev (template)
participant SM as ReactOnRails::Dev::ServerManager
participant PM as Proc Manager
participant Rails as Rails App
participant WP as Packer (e.g., Shakapacker)
participant OS as Open3/System
Dev->>Bin: run bin/dev [args: --mode, --route, --rails-env, ...]
Bin->>SM: run_from_command_line(ARGV)
alt parse args
SM->>SM: determine mode (development/static/production_like)\nextract route, rails_env, verbosity
end
alt production_like
SM->>OS: precompile assets (NODE_ENV=production,\nRAILS_ENV override optional)
OS-->>SM: stdout, stderr, status
alt success
SM->>PM: start processes (Procfile.dev-production-like)
SM->>SM: print_server_info(..., route)
PM-->>Dev: servers running at http://localhost:<port>[/<route>]
else failure
SM->>Dev: print combined output and hints
end
else static
SM->>PM: start processes (Procfile.dev-static-assets)
SM->>SM: print_procfile_info(..., route)
else development (HMR)
SM->>PM: start processes (Procfile.dev)
SM->>SM: print_server_info(..., route)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
Enhances the bin/dev script with configurable routes, improved error handling, and flexible asset precompilation environment management while refactoring PackerUtils to assume shakapacker support.
- Refactored PackerUtils to assume shakapacker support instead of runtime detection
- Added configurable route display and Rails environment options to ServerManager
- Enhanced error handling with detailed output capture and contextual debugging guidance
Reviewed Changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/react_on_rails/utils_spec.rb | Removed test for deleted using_shakapacker_const? method |
| lib/react_on_rails/packer_utils.rb | Simplified by hardcoding shakapacker support and removing runtime detection |
| lib/react_on_rails/dev/server_manager.rb | Added command-line argument parsing, configurable routes, and enhanced error handling |
| lib/react_on_rails/configuration.rb | Added nil-safe operators for PackerUtils method calls |
| lib/generators/react_on_rails/react_with_redux_generator.rb | Updated generator message to include route parameter |
| lib/generators/react_on_rails/install_generator.rb | Modified to pass route information to installation messages |
| lib/generators/react_on_rails/generator_messages.rb | Added route parameter to installation helper messages |
| docs/javascript/troubleshooting-build-errors.md | Fixed markdown table formatting |
| docs/guides/upgrading-react-on-rails.md | Fixed markdown table formatting |
| docs/contributor-info/coding-agents-guide.md | Fixed markdown table formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| packer_name = ReactOnRails::PackerUtils.packer_type&.upcase_first | ||
|
|
||
| Rails.logger.warn "Error configuring config/initializers/react_on_rails. Define neither the generated_assets_dirs nor " \ | ||
| "the generated_assets_dir when using #{ReactOnRails::PackerUtils.packer_type.upcase_first}. This is defined by " \ | ||
| "the generated_assets_dir when using #{packer_name}. This is defined by " \ | ||
| "public_output_path specified in #{ReactOnRails::PackerUtils.packer_type}.yml = #{packer_public_output_path}." |
There was a problem hiding this comment.
The code uses nil-safe operator on line 287 but then calls ReactOnRails::PackerUtils.packer_type without nil-safety on line 291. Since packer_type now always returns 'shakapacker', the nil-safe operator is unnecessary and the inconsistency should be resolved.
🔍 Code Review for PR #1790: Enhance bin/dev with configurable routes and improved production asset handlingThank you for this comprehensive enhancement to the ✅ Strengths
🎯 Code Quality Observations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
lib/react_on_rails/packer_utils.rb (3)
13-18: Guardrequire "shakapacker"to surface a clear error if the gem is missing.As written, a missing gem raises a
LoadError/NameErrorin various call sites. Raise the project’s explicit error instead.Apply this diff:
def self.packer - return nil unless using_packer? - - require "shakapacker" - ::Shakapacker + return nil unless using_packer? + begin + require "shakapacker" + ::Shakapacker + rescue LoadError + raise_shakapacker_not_installed + end end
37-44: Nil safety forshakapacker_version_as_array.If
shakapacker_versionisnil,.matchwill raise. Defensive check avoids surprising crashes in partial setups.Apply this diff:
def self.shakapacker_version_as_array return @shakapacker_version_as_array if defined?(@shakapacker_version_as_array) - match = shakapacker_version.match(ReactOnRails::VersionChecker::VERSION_PARTS_REGEX) + version = shakapacker_version + return nil unless version + match = version.match(ReactOnRails::VersionChecker::VERSION_PARTS_REGEX)
86-88: Optional: usepacker.configfor consistency.
precompile?directly references::Shakapacker. Usingpacker.configkeeps access patterns uniform and benefits from therequireguard above.Apply this diff:
- ::Shakapacker.config.shakapacker_precompile? + packer.config.shakapacker_precompile?docs/javascript/troubleshooting-build-errors.md (2)
18-24: Add language to fenced code block (markdownlint MD040).Specify a language for the error log block.
Apply this diff:
-``` +```text Cannot read properties of undefined (reading 'module') TypeError: Cannot read properties of undefined (reading 'module') at ProvidedDependencyTemplate.apply--- `137-149`: **Add language to fenced code block (markdownlint MD040).** This is a log snippet; mark it as text. Apply this diff: ```diff -``` +```text MONGODB | Error checking localhost:27017: Connection refused</blockquote></details> <details> <summary>docs/contributor-info/coding-agents-guide.md (1)</summary><blockquote> `107-113`: **Fragile sed command; prefer Bundler or cross‑platform sed.** The current sed can break quoting and differs between GNU/BSD. Suggest either Bundler or a safer sed. Use Bundler (preferred): ```bash bundle add react_on_rails --version="~> 16.0" --strictOr cross‑platform sed with backup:
sed -i.bak -E 's/(react_on_rails",[[:space:]]*")~>[[:space:]]*14\.0/\1~> 16.0/' Gemfilelib/react_on_rails/configuration.rb (1)
178-183: Capitalize “Shakapacker” consistently in warning text.Minor copy nit for consistency with the next line.
Apply this diff:
- ReactOnRails: Your current version of shakapacker \ + ReactOnRails: Your current version of Shakapacker \lib/react_on_rails/dev/server_manager.rb (3)
295-296: Remove duplicate require.
require "open3"is already at the top; the second require is unnecessary.- require "open3"
396-404: Normalize route to avoid double slashes and odd input.If a user passes
--route=/dashboard, URLs becomehttp://localhost:3000//dashboard. Strip leading slashes.- url = route ? "http://localhost:#{port}/#{route}" : "http://localhost:#{port}" + safe_route = route.to_s.sub(%r{^/+}, "") + url = safe_route.empty? ? "http://localhost:#{port}" : "http://localhost:#{port}/#{safe_route}"
406-422: Normalize route in procfile box, too.Same normalization should be applied here for consistency.
- url = route ? "http://localhost:#{port}/#{route}" : "http://localhost:#{port}" + safe_route = route.to_s.sub(%r{^/+}, "") + url = safe_route.empty? ? "http://localhost:#{port}" : "http://localhost:#{port}/#{safe_route}"lib/generators/react_on_rails/generator_messages.rb (1)
65-66: Normalize route when rendering URL.Avoid double slashes if a caller passes
/hello_world.- 3. Visit: #{Rainbow(route ? "http://localhost:3000/#{route}" : 'http://localhost:3000').cyan.underline} + 3. Visit: #{Rainbow(route && !route.empty? ? "http://localhost:3000/#{route.sub(%r{^/+}, '')}" : 'http://localhost:3000').cyan.underline}lib/generators/react_on_rails/install_generator.rb (2)
139-151: Harden path resolution; avoid chdir; chmod only regular files.Use File.expand_path/File.join for portability, skip directories when chmod’ing, and drop Dir.chdir. Cleaner and RuboCop-friendly.
- # Copy bin scripts from templates - template_bin_path = "#{__dir__}/templates/base/base/bin" - directory template_bin_path, "bin" + # Copy bin scripts from templates + template_bin_path = File.expand_path("templates/base/base/bin", __dir__) + directory template_bin_path, "bin" - files_to_copy = [] - Dir.chdir(template_bin_path) do - files_to_copy.concat(Dir.glob("*")) - end - files_to_become_executable = files_to_copy.map { |filename| "bin/#{filename}" } + files_to_copy = Dir.children(template_bin_path) + .select { |f| File.file?(File.join(template_bin_path, f)) } + files_to_become_executable = files_to_copy.map { |filename| File.join("bin", filename) } File.chmod(0o755, *files_to_become_executable)
154-161: Don’t hardcode the route; centralize and tidy call formatting.Avoid drift if base generator changes. Centralize the default route and clean up multiline call indentation.
- # Determine what route will be created by the generator - route = "hello_world" # This is the hardcoded route from base_generator.rb + # Determine what route will be created by the generator + route = default_install_route component_name = options.redux? ? "HelloWorldApp" : "HelloWorld" - GeneratorMessages.add_info(GeneratorMessages.helpful_message_after_installation( - component_name: component_name, - route: route - )) + GeneratorMessages.add_info( + GeneratorMessages.helpful_message_after_installation( + component_name: component_name, + route: route + ) + )Add this helper (private section):
def default_install_route # TODO: replace with a shared constant from the base generator if/when exposed "hello_world" end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/contributor-info/coding-agents-guide.md(6 hunks)docs/guides/upgrading-react-on-rails.md(1 hunks)docs/javascript/troubleshooting-build-errors.md(7 hunks)lib/generators/react_on_rails/bin/dev(0 hunks)lib/generators/react_on_rails/generator_messages.rb(2 hunks)lib/generators/react_on_rails/install_generator.rb(2 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/bin/dev(1 hunks)lib/react_on_rails/configuration.rb(3 hunks)lib/react_on_rails/dev/server_manager.rb(7 hunks)lib/react_on_rails/packer_utils.rb(2 hunks)spec/dummy/bin/dev(1 hunks)spec/react_on_rails/utils_spec.rb(0 hunks)
💤 Files with no reviewable changes (2)
- spec/react_on_rails/utils_spec.rb
- lib/generators/react_on_rails/bin/dev
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/generator_messages.rblib/react_on_rails/configuration.rblib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rblib/react_on_rails/dev/server_manager.rblib/react_on_rails/packer_utils.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/generator_messages.rblib/react_on_rails/configuration.rblib/generators/react_on_rails/react_with_redux_generator.rblib/generators/react_on_rails/install_generator.rblib/react_on_rails/dev/server_manager.rblib/react_on_rails/packer_utils.rb
🧠 Learnings (3)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
spec/dummy/bin/devdocs/guides/upgrading-react-on-rails.mdlib/generators/react_on_rails/templates/base/base/bin/devdocs/javascript/troubleshooting-build-errors.mddocs/contributor-info/coding-agents-guide.md
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
spec/dummy/bin/dev
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
docs/javascript/troubleshooting-build-errors.mddocs/contributor-info/coding-agents-guide.md
🧬 Code graph analysis (4)
lib/react_on_rails/configuration.rb (1)
lib/react_on_rails/packer_utils.rb (1)
packer_type(9-11)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
lib/generators/react_on_rails/generator_messages.rb (1)
helpful_message_after_installation(41-82)
lib/generators/react_on_rails/install_generator.rb (1)
lib/generators/react_on_rails/generator_messages.rb (2)
add_info(17-19)helpful_message_after_installation(41-82)
lib/react_on_rails/dev/server_manager.rb (2)
lib/react_on_rails/dev/process_manager.rb (2)
ensure_procfile(14-22)run_with_process_manager(24-45)lib/react_on_rails/dev/pack_generator.rb (2)
generate(7-25)generate(9-23)
🪛 Brakeman (7.0.2)
lib/react_on_rails/dev/server_manager.rb
[medium] 296-296: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🪛 markdownlint-cli2 (0.17.2)
docs/javascript/troubleshooting-build-errors.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.4, latest)
🔇 Additional comments (12)
docs/guides/upgrading-react-on-rails.md (1)
91-91: LGTM: spacing/readability tweak only.No content change; improves formatting after the “Solutions:” line.
lib/react_on_rails/packer_utils.rb (1)
6-11: Intentional hard‑require of Shakapacker looks fine; confirm deprecation posture.Assuming v16+ fully requires Shakapacker, always‑true
using_packer?and fixedpacker_typeare acceptable. Ensure docs and gemspec/message paths consistently reflect this hard requirement.docs/javascript/troubleshooting-build-errors.md (1)
67-69: Docs examples and matrix updates look good.ProvidePlugin examples/readability tweaks and the compatibility matrix align with the repo’s direction.
Also applies to: 78-80, 174-178, 198-201, 280-281
docs/contributor-info/coding-agents-guide.md (2)
18-23: LGTM: table header formatting.Clearer table delineation; no semantic changes.
251-256: Upgrade commands are clear.Explicit npm/gem version bumps for agents are helpful.
lib/react_on_rails/configuration.rb (2)
287-291: Message clarity improvement looks good.Using
packer_namefor human‑readable output andpacker_typefor paths is clearer.
336-347: Improved conflict message reads well.Clear steps linking config keys, YAML path, and default bin task. No logic changes.
lib/react_on_rails/dev/server_manager.rb (1)
11-21: Centralized start dispatch looks good.The new signature and propagation of
route/rails_envare clear and backwards-compatible.lib/generators/react_on_rails/react_with_redux_generator.rb (1)
71-72: Pass-through of route is correct.Supplying
route: "hello_world"keeps messages consistent with the generated route.lib/generators/react_on_rails/generator_messages.rb (1)
41-41: Signature change LGTM.Optional
route:parameter reads cleanly and keeps defaults stable.lib/generators/react_on_rails/templates/base/base/bin/dev (1)
31-31: Delegation to ServerManager is correct.Centralizing CLI parsing/behavior in
ServerManager.run_from_command_line(ARGV)simplifies maintenance.spec/dummy/bin/dev (1)
1-1: No action required — spec/dummy/bin/dev is a symlink.
It points to ../../../lib/generators/react_on_rails/templates/base/base/bin/dev, so the missing-shebang concern does not apply.
| def run_from_command_line(args = ARGV) | ||
| require "optparse" | ||
|
|
||
| options = { route: nil, rails_env: nil } | ||
|
|
||
| OptionParser.new do |opts| | ||
| opts.banner = "Usage: dev [command] [options]" | ||
|
|
||
| opts.on("--route ROUTE", "Specify the route to display in URLs (default: root)") do |route| | ||
| options[:route] = route | ||
| end | ||
|
|
||
| opts.on("--rails-env ENV", "Override RAILS_ENV for assets:precompile step only (prod mode only)") do |env| | ||
| options[:rails_env] = env | ||
| end | ||
|
|
||
| opts.on("-h", "--help", "Prints this help") do | ||
| show_help | ||
| exit | ||
| end | ||
| end.parse!(args) |
There was a problem hiding this comment.
Wire up --verbose and align usage banner.
Help mentions --verbose, -v, but the flag isn’t parsed and we hardcode verbose: false when dispatching. Also, the banner shows dev while other help shows bin/dev.
- options = { route: nil, rails_env: nil }
+ options = { route: nil, rails_env: nil, verbose: false }
@@
- opts.banner = "Usage: dev [command] [options]"
+ opts.banner = "Usage: bin/dev [command] [options]"
@@
opts.on("--rails-env ENV", "Override RAILS_ENV for assets:precompile step only (prod mode only)") do |env|
options[:rails_env] = env
end
+
+ opts.on("-v", "--verbose", "Enable verbose output for pack generation") do
+ options[:verbose] = true
+ end
@@
- when "production-assets", "prod"
- start(:production_like, nil, verbose: false, route: options[:route], rails_env: options[:rails_env])
+ when "production-assets", "prod"
+ start(:production_like, nil, verbose: options[:verbose], route: options[:route], rails_env: options[:rails_env])
when "static"
- start(:static, "Procfile.dev-static-assets", verbose: false, route: options[:route])
+ start(:static, "Procfile.dev-static-assets", verbose: options[:verbose], route: options[:route])
when "kill"
kill_processes
when "help", "--help", "-h"
show_help
when "hmr", nil
- start(:development, "Procfile.dev", verbose: false, route: options[:route])
+ start(:development, "Procfile.dev", verbose: options[:verbose], route: options[:route])Also applies to: 145-156, 169-187, 189-203
Code Review for PR #1790: Enhance bin/dev with configurable routes✅ Overall AssessmentExcellent enhancement to the development tooling with thoughtful implementation. The PR successfully improves developer experience while maintaining backward compatibility. The architecture refactoring is well-executed. 🎯 Strengths1. Architecture & Design ⭐
2. Developer Experience ⭐
3. Documentation ⭐
🔍 Code Quality Observations1. Rubocop Disables (Minor)Multiple rubocop disable comments for Metrics/AbcSize and Metrics/CyclomaticComplexity in lib/react_on_rails/dev/server_manager.rb. While understandable for complex methods, consider extracting error handling logic into separate methods for better maintainability. 2. Safe Navigation Usage ✅Good use of safe navigation operator in configuration.rb:288 and configuration.rb:336 for handling potentially nil values from PackerUtils. 3. Environment Variable Handling ✅Smart approach to NODE_ENV/RAILS_ENV separation, allowing production webpack bundles without production Rails complexity. 🐛 Potential Issues1. Error Content Parsing (Low Risk)In server_manager.rb:330-340, the error detection logic uses simple string matching. Consider using more specific regex patterns to avoid false positives when detecting specific error types. 2. Process Cleanup (Medium Risk)The kill_processes method could benefit from handling scenarios where processes might be stuck, perhaps with a force option using SIGKILL as fallback. ⚡ Performance Considerations1. Open3 Usage ✅Good choice using Open3.capture3 for subprocess management - provides proper stdout/stderr separation. 2. String ConcatenationMinor optimization opportunity in error message building - consider using arrays and join for better memory efficiency when building large multi-line strings. 🔒 Security Considerations1. Command Injection ✅No security issues found. User input is properly handled through OptionParser, and commands are constructed safely. 2. Credential Exposure ✅Good practice warning users about production Rails environment requiring credentials. ✅ Test CoverageThe PR indicates all test scenarios have been covered:
Consider adding automated tests for:
💡 Suggestions for Improvement
📝 Minor Style/Documentation Notes
✨ ConclusionThis is a high-quality PR that significantly improves the developer experience. The code is well-structured, thoroughly documented, and maintains backward compatibility. The architectural improvements (removing duplicate files, centralizing logic) are particularly commendable. Recommendation: Approve with minor suggestions ✅ The only actionable items are minor refactoring opportunities to reduce method complexity. The PR is ready to merge as-is, with these suggestions being optional improvements for future iterations. Great work on this enhancement! The attention to developer experience and the comprehensive error handling are particularly impressive. 🎉 |
Code Review: bin/dev Enhancement PR #1790I've reviewed this PR and found it to be a significant improvement to the development server management. Here are my findings: Strengths
Security Concerns (High Priority)
Code Quality Suggestions
Test CoverageGood comprehensive test plan. Consider adding tests for malformed route parameters and special characters in rails_env. RecommendationApprove with changes - This is high-quality work that will improve DX significantly. The security issues should be addressed before merging. Great work on the architecture improvements and enhanced error handling! |
🔍 Code ReviewThank you for this comprehensive enhancement to the bin/dev script! The improved production asset handling and configurable routing are excellent additions. Here is my detailed review: ✅ Strengths1. Excellent Architecture Improvements
2. Developer Experience Enhancements
3. Backward Compatibility
🎯 Code Quality & Best Practices1. Good Use of Ruby Idioms
2. Documentation Improvements
|
Code Review for PR #1790: Enhance bin/dev with configurable routes and improved production asset handling🎯 Overall AssessmentThis PR significantly improves the developer experience with the bin/dev script. The enhancements are well-structured and add valuable functionality. However, there are several areas that need attention before merging. ✅ Strengths
🔍 Code Quality Issues1. Complexity in run_production_like method (lib/react_on_rails/dev/server_manager.rb:281-374)
2. Nil-safety inconsistency (lib/react_on_rails/configuration.rb:287-288, 336-337)Using &. operator inconsistently - sometimes with packer_type&.upcase_first, sometimes without. Should be consistent throughout. 3. Magic numbers (lib/react_on_rails/dev/server_manager.rb)Port numbers (3000, 3001) are hardcoded. Consider making these configurable or at least constants.
|
Code Review: bin/dev Enhancement PR #1790I've reviewed this PR and found it to be a well-structured enhancement. The refactoring improves maintainability, adds useful features like configurable routes and improved error handling with Open3.capture3, and maintains backwards compatibility. Strengths:
Suggestions:
Overall: High-quality PR that meaningfully improves developer experience. Approve with minor suggestions. |
- Restore proper PackerUtils.using_packer? logic from master branch - Fix precompile? method to handle missing shakapacker properly - Update bin/dev tests to match new simplified template structure - Update server_manager_spec to expect Open3.capture3 instead of system 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Code Review - PR #1790Thank you for this comprehensive enhancement to the bin/dev script! I have reviewed the changes and have feedback on code quality, potential issues, performance, security, and test coverage. ✅ Strengths
🎯 Code Quality & Best Practices1. Missing --rails-env Option HandlingThe PR description mentions --rails-env=production option, but the implementation in run_from_command_line does not fully handle it in some of the commits. The latest commit appears to address this. 2. Hardcoded Shakapacker ReferencesIn lib/react_on_rails/configuration.rb:178, there is a hardcoded "shakapacker" string that should use the dynamic packer name for consistency. 🐛 Potential Bugs1. Error Output Not DisplayedIn run_production_like, using Open3.capture3 but not showing the stderr output to users when precompilation fails. This makes debugging harder. Consider displaying stderr when status.success? is false. 2. Missing Test UpdatesSome tests were updated to use run_from_command_line but comprehensive tests for the new options are missing. ⚡ Performance Considerations
🔒 Security ConcernsNo significant security issues found. The command construction is safe and does not use user input directly in shell commands. 🧪 Test CoverageMissing test scenarios for:
📋 Documentation NotesThe markdown formatting fixes in the documentation are good improvements for readability. 🎬 SummaryThis is a well-thought-out enhancement that significantly improves the developer experience. The main areas that could use attention are:
The architectural improvements and enhanced error handling are particularly valuable contributions. Great work on consolidating the bin/dev logic and making it more maintainable! 🚀 |
- Use instance_double instead of double for Process::Status - Break long line for readability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Enhance bin/dev with configurable routes and improved production asset handlingGreat work on this enhancement! The implementation provides valuable improvements to the development workflow. Here's my comprehensive review: ✅ Strengths
🔍 Areas for Consideration1. Potential Bug: Nil-safe Operator UsageIn packer_name = ReactOnRails::PackerUtils.packer_type&.upcase_firstThe nil-safe operator is good, but if packer_name = ReactOnRails::PackerUtils.packer_type&.upcase_first || "Packer"2. Error Handling Edge CaseIn begin
stdout, stderr, status = Open3.capture3(command)
rescue Interrupt
puts "\n⚠️ Asset precompilation interrupted by user"
exit 1
end3. Performance ConsiderationThe error content analysis in lines 331-345 performs multiple string searches on potentially large output. Consider combining into a single pass or using a case statement with regex for better performance. 4. Documentation FormattingWhile the markdown formatting improvements in the docs are good, some code blocks lost their language specifiers (e.g., 🔒 Security Considerations
🧪 Test CoverageThe test updates look appropriate, but consider adding specific tests for:
💡 Suggestions
🎯 Overall AssessmentThis is a well-thought-out enhancement that significantly improves the development experience. The code quality is high, with good separation of concerns and helpful user feedback. The backwards compatibility is maintained perfectly. Recommendation: Approve with minor suggestions ✅ The PR is ready to merge after addressing the nil-safe operator fallback issue. The other suggestions are optional improvements that could be addressed in follow-up PRs. Excellent work on making the development workflow more flexible and user-friendly! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
spec/react_on_rails/dev/server_manager_spec.rb (2)
57-59: Prefer Open3 with explicit env hash and argv array; assert RAILS_ENV default explicitlyUsing a shell string is brittle (platform/env quoting) and doesn’t verify the intended default of RAILS_ENV=development. Recommend passing env/argv to Open3 and asserting the explicit RAILS_ENV to make the test resilient and aligned with the PR’s intent.
- command = "NODE_ENV=production bundle exec rails assets:precompile" - status_double = instance_double(Process::Status, success?: true) - expect(Open3).to receive(:capture3).with(command).and_return(["output", "", status_double]) + env = { "NODE_ENV" => "production", "RAILS_ENV" => "development" } + args = %w[bundle exec rails assets:precompile] + status_double = instance_double(Process::Status, success?: true) + expect(Open3).to receive(:capture3).with(env, *args).and_return(["output", "", status_double])If the implementation intentionally omits RAILS_ENV, please confirm Rails’ default for assets:precompile across supported versions to avoid surprises.
9-19: Close the File::NULL IOs to avoid leaking FDsYou swap $stdout/$stderr to new IOs but never close them. Close the opened IOs in teardown to prevent descriptor leaks in long test runs.
before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") + @original_stderr = $stderr + @original_stdout = $stdout + @null_stderr = File.open(File::NULL, "w") + @null_stdout = File.open(File::NULL, "w") + $stderr = @null_stderr + $stdout = @null_stdout end after(:all) do + @null_stderr&.close + @null_stdout&.close $stderr = @original_stderr $stdout = @original_stdout endspec/react_on_rails/binstubs/dev_spec.rb (3)
50-57: Add a case to verify ARGV is forwarded unchanged (with real args)Current test forwards empty ARGV. Add a concrete-args example to prevent regressions when parsing flags like --route and --rails-env.
it "with ReactOnRails::Dev loaded, delegates to ServerManager" do setup_script_execution_for_tool_tests allow(ReactOnRails::Dev::ServerManager).to receive(:run_from_command_line) # Mock the require to succeed allow_any_instance_of(Kernel).to receive(:require).with("bundler/setup").and_return(true) allow_any_instance_of(Kernel).to receive(:require).with("react_on_rails/dev").and_return(true) expect(ReactOnRails::Dev::ServerManager).to receive(:run_from_command_line).with(ARGV) load script_path end + + it "forwards CLI args to ServerManager unchanged" do + setup_script_execution_for_tool_tests + args = %w[--route=/hello --rails-env=production] + stub_const("ARGV", args) + allow_any_instance_of(Kernel).to receive(:require).and_return(true) + expect(ReactOnRails::Dev::ServerManager).to receive(:run_from_command_line).with(args) + load script_path + end
21-26: Minor: avoid allow_any_instance_of(Kernel).RuboCop RSpec discourages any_instance stubs. Prefer stubbing the specific calls invoked by the script or isolate via a wrapper.
9-19: Unify stdout/stderr suppression style across specs and close IOsThis file uses local vars vs. instance vars in server_manager_spec. Consider consistent pattern and close the opened IOs in teardown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/react_on_rails/binstubs/dev_spec.rb(2 hunks)spec/react_on_rails/dev/server_manager_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
spec/react_on_rails/dev/server_manager_spec.rbspec/react_on_rails/binstubs/dev_spec.rb
🧠 Learnings (1)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: examples (3.4, latest)
🔇 Additional comments (2)
spec/react_on_rails/binstubs/dev_spec.rb (2)
6-6: Template path matches generator source; LGTMPath aligns with install_generator’s template_bin_path. Nothing else to change here.
43-46: Good: assert delegation symbolically rather than mode branchingThe content check for run_from_command_line keeps the bin script thin and future‑proof.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 14 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def run_from_command_line(args = ARGV) | ||
| require "optparse" | ||
|
|
||
| options = { route: nil, rails_env: nil } | ||
|
|
||
| OptionParser.new do |opts| | ||
| opts.banner = "Usage: dev [command] [options]" | ||
|
|
||
| opts.on("--route ROUTE", "Specify the route to display in URLs (default: root)") do |route| | ||
| options[:route] = route | ||
| end | ||
|
|
||
| opts.on("--rails-env ENV", "Override RAILS_ENV for assets:precompile step only (prod mode only)") do |env| | ||
| options[:rails_env] = env | ||
| end | ||
|
|
||
| opts.on("-h", "--help", "Prints this help") do | ||
| show_help | ||
| exit | ||
| end | ||
| end.parse!(args) |
There was a problem hiding this comment.
[nitpick] The OptionParser instance is created but not assigned to a variable. While this works, storing it in a variable would make the code more readable and easier to test.
| puts Rainbow("💡 Common fixes:").yellow.bold | ||
|
|
||
| # Provide specific guidance based on error content | ||
| error_content = "#{stderr} #{stdout}".downcase |
There was a problem hiding this comment.
String concatenation with interpolation creates unnecessary intermediate strings. Consider using [stderr, stdout].join(' ').downcase or check each stream separately to avoid creating the combined string.
| error_content = "#{stderr} #{stdout}".downcase | |
| error_content = [stderr, stdout].map(&:to_s).join(' ').downcase |
| if error_content.include?("database") || error_content.include?("relation") || | ||
| error_content.include?("table") |
There was a problem hiding this comment.
[nitpick] Multiple string checks could be simplified using a single regular expression: error_content.match?(/database|relation|table/) which would be more efficient and maintainable.
| if error_content.include?("database") || error_content.include?("relation") || | |
| error_content.include?("table") | |
| if error_content.match?(/database|relation|table/) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Security improvements: - Replace string interpolation with env hash and argv array for Open3.capture3 - Add rails_env validation with allowlist pattern (/^[a-z0-9_]+$/i) - Update error handling to use safe command display - Add tests for rails_env validation and custom environment usage - Prevent arbitrary shell command execution via --rails-env parameter 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Code Review for PR #1790: Enhanced bin/dev with Configurable Routes and Production Asset Handling✅ Overall AssessmentThis PR represents a significant improvement to the development workflow with thoughtful architecture changes. The enhancements are well-designed, backwards compatible, and provide valuable developer experience improvements. 🌟 Key Strengths
🎯 Code Quality Highlights
|
|
Code Review for PR 1790: See detailed feedback below |
Code Review - Part 1: Strengths
|
Code Review - Part 2: Issues and SuggestionsIssues Found:
Suggestions:
Testing Commands:
Overall: Well-designed enhancement. Fix the truncated error message and add test coverage before merging. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 14 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Validate and sanitize rails_env to prevent shell injection | ||
| if rails_env | ||
| unless rails_env.match?(/\A[a-z0-9_]+\z/i) | ||
| puts "❌ Invalid rails_env: '#{rails_env}'. Must contain only letters, numbers, and underscores." |
There was a problem hiding this comment.
The regex validation for rails_env is case-insensitive but the error message only mentions lowercase letters. Either make the regex case-sensitive (/\A[a-z0-9_]+\z/) or update the error message to mention 'letters (case-insensitive)'.
| puts "❌ Invalid rails_env: '#{rails_env}'. Must contain only letters, numbers, and underscores." | |
| puts "❌ Invalid rails_env: '#{rails_env}'. Must contain only letters (case-insensitive), numbers, and underscores." |
| puts Rainbow("💡 Common fixes:").yellow.bold | ||
|
|
||
| # Provide specific guidance based on error content | ||
| error_content = "#{stderr} #{stdout}".downcase |
There was a problem hiding this comment.
Consider using string interpolation more efficiently here: error_content = [stderr, stdout].join(' ').downcase to avoid potential issues if either stderr or stdout is nil.
| error_content = "#{stderr} #{stdout}".downcase | |
| error_content = [stderr, stdout].compact.join(' ').downcase |
Summary
Major enhancement to
bin/devscript with configurable routes, improved error handling, and flexible asset precompilation environment management.🎯 Key Features
--routeparameter for custom URL display (defaults to root)NODE_ENV=production+RAILS_ENV=developmentby default--rails-env=productionfor full production testing🔧 Technical Improvements
ReactOnRails::Dev::ServerManager.run_from_command_line()💡 Usage Examples
🎁 Benefits
🔄 Backwards Compatibility
All existing
bin/devcommands work exactly as before. New options are additive and optional.Test Plan
bin/devdefault behavior (unchanged)bin/dev prodwith new default environment (NODE_ENV=production, RAILS_ENV=development)bin/dev prod --rails-env=productionoption--routeparameter functionality🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Documentation
Refactor