🚀 React on Rails v15 Generator Overhaul & Developer Experience Enhancement for v16 #1770
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 4 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 (2)
WalkthroughInstalls Shakapacker as a required dependency, replaces Webpacker paths with Shakapacker-only flows, introduces file-system auto-registration for component bundles, adds a modular dev toolchain (ServerManager, ProcessManager, PackGenerator, FileManager) and a simplified bin/dev CLI, and updates generators, templates, CI, rake tasks, and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant IG as InstallGenerator
participant Git as GitUtils
participant Sys as System (Node/PM)
participant Bundler as Bundler
participant Rails as Rails (shakapacker:install)
Dev->>IG: rails g react_on_rails:install
IG->>Git: check uncommitted_changes?
IG->>Sys: detect node + package manager
Sys-->>IG: node & package manager presence
alt shakapacker binaries present
IG-->>Dev: continue (skip install)
else missing shakapacker
IG->>IG: print shakapacker setup banner
IG->>Bundler: run `bundle add shakapacker --strict`
Bundler-->>IG: success/fail
IG->>Rails: `bundle exec rails shakapacker:install`
Rails-->>IG: success -> create .shakapacker_just_installed
end
IG-->>Dev: print helpful installation messages
sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as bin/dev
participant SM as ServerManager
participant PG as PackGenerator
participant PM as ProcessManager
participant OS as System
Dev->>CLI: bin/dev [hmr|static|prod|kill|help]
CLI->>SM: start(mode)/kill_processes/show_help
alt mode == development/hmr
SM->>PG: generate(verbose?)
PG-->>SM: packs generated
SM->>PM: ensure_procfile("Procfile.dev")
SM->>PM: run_with_process_manager("Procfile.dev")
PM->>OS: overmind/foreman start -f Procfile.dev
else mode == static
SM->>PG: generate
SM->>PM: ensure_procfile("Procfile.dev-static-assets")
SM->>PM: run_with_process_manager("Procfile.dev-static-assets")
else mode == production_like
SM->>OS: precompile assets (RAILS_ENV=production)
SM->>PM: ensure_procfile("Procfile.dev-prod-assets")
SM->>PM: run_with_process_manager("Procfile.dev-prod-assets")
else kill/help
SM->>OS: kill processes / print help
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
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 Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/bin/dev (1)
1-3: Require English to replace $? with $CHILD_STATUS (RuboCop fix).Adds the stdlib English module to satisfy Style/SpecialGlobalVars and make status checks clearer.
#!/usr/bin/env ruby # frozen_string_literal: true + +require "English"
🧹 Nitpick comments (8)
lib/generators/react_on_rails/bin/dev (1)
4-8: Make installed? return a boolean and avoid leaking an IO handle.Use array form to avoid a shell, silence output, close the handle via block, and return true/false explicitly.
def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false + IO.popen([process, "-v"], err: File::NULL, out: File::NULL) {} # closes IO and avoids shell + true +rescue Errno::ENOENT + false endspec/react_on_rails/binstubs/dev_spec.rb (1)
79-83: Wrap long expectation lines to satisfy LineLength.Only formatting; behavior unchanged.
- allow_any_instance_of(Kernel).to receive(:system).with("bundle exec rake react_on_rails:generate_packs").and_return(true) + allow_any_instance_of(Kernel).to receive(:system).with( + "bundle exec rake react_on_rails:generate_packs" + ).and_return(true) - allow_any_instance_of(Kernel) - .to receive(:system) - .with("overmind start -f Procfile.dev") + allow_any_instance_of(Kernel) + .to receive(:system) + .with("overmind start -f Procfile.dev") .and_raise(Errno::ENOENT)lib/generators/react_on_rails/install_generator.rb (1)
68-74: Tiny consistency nit: message URL is long; consider wrapping to satisfy LineLength if flagged later.Optional; current code likely passes.
spec/dummy/bin/dev (5)
4-8: Return a boolean from installed? and avoid leaking an IO.IO.popen returns a truthy IO even on nonzero exit and leaves a pipe open. Use system and return a boolean.
-def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false -end +def installed?(process) + system("#{process} -v > /dev/null 2>&1") +rescue Errno::ENOENT + false +end
54-85: Pre-check Procfile existence; don’t rely on Errno::ENOENT.Missing Procfile doesn’t raise Errno::ENOENT; the spawned process exits nonzero. Check files up front and use exit(1), not exit!, to allow at_exit hooks.
def run_static_development @@ - # Generate React on Rails packs first + # Generate React on Rails packs first generate_packs - - if installed? "overmind" + # Ensure required Procfile exists + unless File.exist?("Procfile.dev-static") + warn "ERROR:\nPlease ensure `Procfile.dev-static` exists in your project!" + exit 1 + end + + if installed? "overmind" system "overmind start -f Procfile.dev-static" elsif installed? "foreman" system "foreman start -f Procfile.dev-static" else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG - exit! + exit 1 end -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev-static` exists in your project! - MSG - exit! end
87-97: Mirror Procfile existence check for HMR dev.Same reasoning as static mode.
def run_development(process) - generate_packs - - system "#{process} start -f Procfile.dev" -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev` exists in your project! - MSG - exit! + generate_packs + unless File.exist?("Procfile.dev") + warn "ERROR:\nPlease ensure `Procfile.dev` exists in your project!" + exit 1 + end + system "#{process} start -f Procfile.dev" end
20-52: Optional: exec the long-running Rails server.Using exec hands signals directly to Rails and avoids a wrapper process.
- # Start Rails in production mode - system "RAILS_ENV=production bundle exec rails server -p 3001" + # Start Rails in production mode + exec "RAILS_ENV=production bundle exec rails server -p 3001"
139-151: Consistent exit on missing Overmind/Foreman.Prefer exit 1 over exit! for consistency with other exits and at_exit hooks.
- exit! + exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
-
lib/generators/react_on_rails/bin/dev(1 hunks) -
lib/generators/react_on_rails/install_generator.rb(3 hunks) -
spec/dummy/bin/dev(1 hunks) -
spec/react_on_rails/binstubs/dev_spec.rb(3 hunks) -
spec/react_on_rails/generators/install_generator_spec.rb(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/binstubs/dev_spec.rbspec/react_on_rails/generators/install_generator_spec.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/install_generator.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:
lib/generators/react_on_rails/bin/dev
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (3)
lib/react_on_rails/git_utils.rb (1)
uncommitted_changes?(7-20)lib/react_on_rails/utils.rb (1)
running_on_windows?(139-141)lib/generators/react_on_rails/generator_messages.rb (2)
add_info(17-19)add_error(9-11)
🪛 GitHub Actions: Lint JS and Ruby
spec/react_on_rails/binstubs/dev_spec.rb
[error] 47-47: RSpec/StubbedMock: Prefer allow_any_instance_of over expect_any_instance_of when configuring a response.
[error] 47-47: LineLength: Line is too long. [126/120]
[error] 57-57: RSpec/StubbedMock: Prefer allow_any_instance_of over expect_any_instance_of when configuring a response.
[error] 57-57: LineLength: Line is too long. [126/120]
[error] 79-79: LineLength: Line is too long. [125/120]
lib/generators/react_on_rails/bin/dev
[error] 13-13: Trailing whitespace detected.
[error] 14-14: Style/GuardClause: Use a guard clause (return if $?.success?) instead of wrapping the code inside a conditional expression.
[error] 14-14: Style/SpecialGlobalVars: Prefer
[error] 30-30: Trailing whitespace detected.
[error] 33-33: Trailing whitespace detected.
[error] 37-37: Trailing whitespace detected.
[error] 38-38: Style/SpecialGlobalVars: Prefer
[error] 45-45: Trailing whitespace detected.
[error] 64-64: Trailing whitespace detected.
[error] 67-67: Trailing whitespace detected.
[error] 89-89: Trailing whitespace detected.
[error] 107-107: Trailing whitespace detected.
[error] 114-114: Trailing whitespace detected.
[error] 122-122: Trailing whitespace detected.
[error] 130-130: Trailing whitespace detected.
lib/generators/react_on_rails/install_generator.rb
[error] 105-105: Trailing whitespace detected.
[error] 108-108: LineLength: Line is too long. [134/120]
spec/react_on_rails/generators/install_generator_spec.rb
[error] 137-137: RSpec/ContextWording: Context description should match /^when|^with|^without/.
[error] 139-139: RSpec/VerifiedDoubles: Prefer using verifying doubles over normal doubles.
[error] 145-145: LineLength: Line is too long. [131/120]
[error] 150-150: RSpec/ContextWording: Context description should match /^when|^with|^without/.
[error] 169-169: LineLength: Line is too long. [158/120]
⏰ 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). (3)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: examples (newest)
- GitHub Check: examples (oldest)
🔇 Additional comments (14)
lib/generators/react_on_rails/bin/dev (3)
65-78: Static mode flow looks good.Pack generation precedes process start, and error/help messages for missing Procfile.dev-static are clear.
87-97: HMR dev helper is concise and mirrors static-mode error handling.Looks good after generate_packs fixes.
99-157: CLI dispatch reads well; nice help output and aliases.No functional concerns.
spec/react_on_rails/binstubs/dev_spec.rb (4)
19-23: Test coverage for pack-generation presence is appropriate.Assertions target the expected public API of the script.
25-29: Static-mode expectations are correct.Validates function presence and Procfile usage.
31-36: Production-like mode assertions LGTM.Covers precompile and server port.
38-43: Help command checks are clear.Good coverage for flags and usage output.
lib/generators/react_on_rails/install_generator.rb (3)
28-34: Preflight auto-install hook placement is good.Runs before other generator steps and still prints messages in ensure. LGTM.
65-66: npm prerequisite check aligns with PR goals.Switched from Yarn to npm correctly, keeping Git uncommitted check.
115-120: Gem detection is robust and correctly rescued.No changes needed.
spec/react_on_rails/generators/install_generator_spec.rb (4)
81-83: *Good switch from Yarn to npm on nix.Stubbing
which npmand assertingmissing_npm?false aligns with the PR objective to standardize on npm.
97-99: *Good negative case for npm on nix.Covers the missing-path branch for npm detection.
113-115: Good switch from Yarn to npm on Windows.Stubbing
where npmand assertingmissing_npm?false is correct for Windows.
127-131: Good negative case for npm on Windows.Covers the missing-path branch for Windows detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (2)
57-57: Turning on auto_load_bundle by default — double‑check SSR expectationsWith auto_load_bundle = true, client bundles are auto-required per component. For apps using prerender: true, ensure server-bundle.js includes all SSR components (this setting doesn’t auto-map server entries). Consider a brief inline comment to clarify this or a doc note in the generator output.
Would you like a short initializer comment snippet to clarify client vs. server bundle behavior?
52-52: Replace yarn references with npm in generator template and documentation
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (lines 23–25): update the comment to use
npm run build:test(or equivalent) instead ofyarn- Globally replace stale
yarncommands in docs and example scripts with theirnpmcounterparts (or clearly note both package managers)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/react_on_rails/binstubs/dev_spec.rb
🧰 Additional context used
🧠 Learnings (2)
📚 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:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 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:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
⏰ 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). (6)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
| public_output_path: packs | ||
| cache_path: tmp/shakapacker | ||
| webpack_compile_output: true | ||
| nested_entries: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enabling nested_entries — verify Shakapacker minimum version and update dev comments
Confirm the generator pins/ensures a Shakapacker version that supports nested_entries; otherwise auto-install may fetch an older version. Also, comments later in this file still reference Procfile.dev-static; align with the new bin/dev modes introduced in this PR.
Suggested checks:
🏁 Script executed:
#!/bin/bash
# Ensure generator/specs enforce a compatible Shakapacker version
rg -nC2 'shakapacker' Gemfile* gemspec lib spec
# Find stale Procfile.dev-static references
rg -nC2 'Procfile\.dev-static|dev-static' -SLength of output: 57304
Ensure Shakapacker ≥ 8.2.0 for nested_entries and update template comments
- Add a version check in the install generator (or a new validation) to confirm the installed Shakapacker gem is ≥ 8.2.0 before emitting
nested_entries: true. - In lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (line 21), update the comment to reference the new bin/dev workflow (e.g.
# This is false since we’re running `bin/dev --watch`) instead ofProcfile.dev-static.
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
around line 10 and update comment at line 21: add a version check in the install
generator (or a new validator) to detect the installed Shakapacker gem version
and only emit "nested_entries: true" when the gem is >= 8.2.0 (fail gracefully
or skip setting when older), and change the comment at line 21 to reference the
new bin/dev workflow (for example: "# This is false since we’re running `bin/dev
--watch`") instead of mentioning Procfile.dev-static.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
28-34: Abort the flow if Shakapacker auto-install fails.The generator currently proceeds even when installation fails, which will likely blow up later. Short-circuit on failure.
- ensure_shakapacker_installed + return unless ensure_shakapacker_installed
♻️ Duplicate comments (3)
lib/generators/react_on_rails/bin/dev (2)
114-114: Remove interpolation hack; fix trailing whitespace via formatter instead.Apply:
- #{' '} +Also run your linter/formatter to strip trailing spaces across this file.
14-17: Fix $CHILD_STATUS usage: require 'English' and write failures to stderr.Without
require 'English',$CHILD_STATUSis nil and.success?raises. Also, send failures to stderr.Apply:
+# Add near the top (after frozen_string_literal) +require 'English' @@ - return if $CHILD_STATUS.success? + return if $CHILD_STATUS&.success? @@ - puts "Pack generation failed" + warn "Pack generation failed (exit=#{$CHILD_STATUS&.exitstatus})"lib/generators/react_on_rails/install_generator.rb (1)
101-114: Install via Bundler, add the gem if absent, wrap long error messages (RuboCop), and return a boolean.Running the installer without Bundler risks invoking the wrong Rails; also, if the gem is truly missing,
rails shakapacker:installwon’t exist. Add the gem first, run via Bundler, wrap messages to satisfy line-length, and make the method return true/false so callers can halt. This also addresses prior feedback.- def ensure_shakapacker_installed - return if shakapacker_installed? - - GeneratorMessages.add_info("Shakapacker not detected. Installing Shakapacker...") - - result = system("rails shakapacker:install") - unless result - GeneratorMessages.add_error("Failed to install Shakapacker automatically. " \ - "Please run 'rails shakapacker:install' manually.") - return - end - - GeneratorMessages.add_info("Shakapacker installed successfully!") - end + def ensure_shakapacker_installed + return true if shakapacker_installed? + + GeneratorMessages.add_info("Shakapacker not detected. Installing Shakapacker...") + + added = system("bundle", "add", "shakapacker") + unless added + GeneratorMessages.add_error(<<~MSG.strip) + Failed to add Shakapacker to your Gemfile. + Please run 'bundle add shakapacker' manually and re-run the generator. + MSG + return false + end + + installed = system("bundle", "exec", "rails", "shakapacker:install") + unless installed + GeneratorMessages.add_error(<<~MSG.strip) + Failed to install Shakapacker automatically. + Please run 'bundle exec rails shakapacker:install' manually. + MSG + return false + end + + GeneratorMessages.add_info("Shakapacker installed successfully!") + true + end
🧹 Nitpick comments (7)
lib/generators/react_on_rails/bin/dev (6)
12-12: Prefer execve-style system to avoid shell and improve portability.Apply:
- system "bundle exec rake react_on_rails:generate_packs" + system('bundle', 'exec', 'rake', 'react_on_rails:generate_packs')
35-37: Avoid shell env prefix; pass env via system hash.Apply:
- system "RAILS_ENV=production NODE_ENV=production bundle exec rails assets:precompile" + system({ 'RAILS_ENV' => 'production', 'NODE_ENV' => 'production' }, + 'bundle', 'exec', 'rails', 'assets:precompile')
141-151: Consistent non-bypassing exit; preferexit 1overexit!.Apply:
- exit! + exit 1
153-157: Send unknown-argument errors to stderr.Apply:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" + warn "Unknown argument: #{ARGV[0]}" + warn "Run 'bin/dev help' for usage information"
4-8: Avoid IO.popen leak; return a strict boolean from installed?Apply:
-def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false -end +def installed?(process) + # true if command executed and exited success, false if missing or non-zero + system(process, '-v', out: File::NULL, err: File::NULL) ? true : false +end
140-145: Minor: avoid duplicate checks by caching chosen process.Example (outside selected lines):
process = installed?('overmind') ? 'overmind' : (installed?('foreman') ? 'foreman' : nil) if process run_development(process) else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG exit 1 endlib/generators/react_on_rails/install_generator.rb (1)
116-121: Broaden rescue for gem detection for older RubyGems.Some environments raise Gem::LoadError here. Minor hardening.
- def shakapacker_installed? - Gem::Specification.find_by_name("shakapacker") - true - rescue Gem::MissingSpecError - false - end + def shakapacker_installed? + Gem::Specification.find_by_name("shakapacker") + true + rescue Gem::MissingSpecError, Gem::LoadError + false + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/generators/react_on_rails/bin/dev(1 hunks)lib/generators/react_on_rails/install_generator.rb(3 hunks)spec/react_on_rails/binstubs/dev_spec.rb(2 hunks)spec/react_on_rails/generators/install_generator_spec.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/react_on_rails/binstubs/dev_spec.rb
- spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/install_generator.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:
lib/generators/react_on_rails/bin/dev
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (3)
lib/react_on_rails/git_utils.rb (1)
uncommitted_changes?(7-20)lib/react_on_rails/utils.rb (1)
running_on_windows?(139-141)lib/generators/react_on_rails/generator_messages.rb (2)
add_info(17-19)add_error(9-11)
⏰ 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). (7)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: examples (oldest)
- GitHub Check: build
🔇 Additional comments (2)
lib/generators/react_on_rails/install_generator.rb (2)
65-66: npm prerequisite switch — LGTM.
28-34: No leftover Yarn checks or non-Bundler installer invocations detected.
ihabadham
force-pushed
the
update-generator-for-v15
branch
from
8646f7a to
dbb8b62
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/react_on_rails/binstubs/dev_spec.rb (1)
83-99: Don’t simulate missing Procfile via Errno::ENOENT; stub File.exist? instead.system won’t raise Errno::ENOENT for a missing Procfile. Make the test reflect the intended behavior.
Apply:
- allow_any_instance_of(Kernel) - .to receive(:system) - .with("overmind start -f Procfile.dev") - .and_raise(Errno::ENOENT) - allow_any_instance_of(Kernel).to receive(:exit!) + allow(File).to receive(:exist?).with("Procfile.dev").and_return(false) + allow(Kernel).to receive(:exit!) + allow(Kernel).to receive(:exit)
♻️ Duplicate comments (6)
spec/react_on_rails/binstubs/dev_spec.rb (2)
52-58: Replace expect_any_instance_of with allow + have_received; wrap long command.Avoid RSpec/StubbedMock offense and line length issues. After load, assert the call via have_received.
Apply:
- allow(IO).to receive(:popen).with("overmind -v").and_return("Some truthy result") - expect_any_instance_of(Kernel).to receive(:system).with("overmind start -f Procfile.dev") + allow(IO).to receive(:popen).with("overmind -v").and_return("Some truthy result") + cmd = "overmind start -f Procfile.dev" + allow(Kernel).to receive(:system).with(cmd).and_return(true) @@ - load script_path + load script_path + expect(Kernel).to have_received(:system).with(cmd)
60-67: Same fix for Foreman branch; avoid any_instance_of and assert with have_received.Apply:
- allow(IO).to receive(:popen).with("overmind -v").and_raise(Errno::ENOENT) - allow(IO).to receive(:popen).with("foreman -v").and_return("Some truthy result") - expect_any_instance_of(Kernel).to receive(:system).with("foreman start -f Procfile.dev") + allow(IO).to receive(:popen).with("overmind -v").and_raise(Errno::ENOENT) + allow(IO).to receive(:popen).with("foreman -v").and_return("Some truthy result") + cmd = "foreman start -f Procfile.dev" + allow(Kernel).to receive(:system).with(cmd).and_return(true) @@ - load script_path + load script_path + expect(Kernel).to have_received(:system).with(cmd)lib/generators/react_on_rails/bin/dev (4)
87-97: Same handling for HMR path: precheck Procfile.dev; remove rescue; avoid exit!.Apply:
def run_development(process) generate_packs - system "#{process} start -f Procfile.dev" -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev` exists in your project! - MSG - exit! + unless File.exist?("Procfile.dev") + warn "ERROR:\n Please ensure `Procfile.dev` exists in your project!" + exit 1 + end + system "#{process} start -f Procfile.dev" end
65-78: Precheck Procfile.dev-static; avoid unreachable rescue and exit!Apply:
# Generate React on Rails packs first generate_packs - - if installed? "overmind" + unless File.exist?("Procfile.dev-static") + warn "ERROR:\n Please ensure `Procfile.dev-static` exists in your project!" + exit 1 + end + + if installed? "overmind" system "overmind start -f Procfile.dev-static" elsif installed? "foreman" system "foreman start -f Procfile.dev-static" else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG - exit! + exit 1 end
38-51: Guard on precompile failure and exec Rails for proper signal handling.Use stderr for failures; replace the shell with Rails so Ctrl+C propagates cleanly.
Apply:
- if $CHILD_STATUS.success? - puts "Assets precompiled successfully" - puts "Starting Rails server in production mode..." - puts "" - puts "Press Ctrl+C to stop the server" - puts "To clean up: rm -rf public/packs && bin/dev" - puts "" - - # Start Rails in production mode - system "RAILS_ENV=production bundle exec rails server -p 3001" - else - puts "Asset precompilation failed" - exit 1 - end + unless $CHILD_STATUS&.success? + warn "Asset precompilation failed" + exit 1 + end + + puts "Assets precompiled successfully" + puts "Starting Rails server in production mode..." + puts "" + puts "Press Ctrl+C to stop the server" + puts "To clean up: rm -rf public/packs && bin/dev" + puts "" + + # Replace current process so signals propagate + exec({ "RAILS_ENV" => "production" }, "bundle", "exec", "rails", "server", "-p", "3001")
79-85: Remove rescue Errno::ENOENT block; system won’t raise for missing Procfile.This rescue is never triggered for a missing Procfile; the precheck above handles it. Also avoid exit!.
Apply:
-rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev-static` exists in your project! - MSG - exit! -end +end
🧹 Nitpick comments (6)
spec/react_on_rails/binstubs/dev_spec.rb (3)
23-25: Prefer stubbing Kernel.system directly (avoid any_instance_of).Kernel.system is a module function; stub it directly for clarity and fewer surprises.
Apply:
- allow_any_instance_of(Kernel).to receive(:system) - .with("bundle exec rake react_on_rails:generate_packs").and_return(true) + allow(Kernel).to receive(:system) + .with("bundle exec rake react_on_rails:generate_packs") + .and_return(true)
68-81: Also stub Kernel.exit in addition to exit! to future‑proof tests.The script should use exit(1) rather than exit!; stubbing both avoids brittle coupling.
Apply:
- allow_any_instance_of(Kernel).to receive(:exit!) + allow(Kernel).to receive(:exit!) + allow(Kernel).to receive(:exit)
46-50: Avoid string-matching implementation details for help; assert behavior.Readability and robustness: execute with ARGV=["help"] and assert the usage output instead of checking the exact conditional string.
Example:
- script_content = File.read(script_path) - expect(script_content).to include('ARGV[0] == "help" || ARGV[0] == "--help" || ARGV[0] == "-h"') - expect(script_content).to include("Usage: bin/dev [command]") + stub_const("ARGV", ["help"]) + expect { load script_path }.to output(/Usage: bin\/dev \[command\]/).to_stdout_from_any_processlib/generators/react_on_rails/bin/dev (3)
4-8: Avoid IO.popen leak; use system for presence check.IO.popen without a block leaks an IO handle. A boolean-returning system call is simpler and safer.
Apply:
def installed?(process) - IO.popen "#{process} -v" + system("#{process} -v > /dev/null 2>&1") rescue Errno::ENOENT false end
154-156: Send unknown-argument errors to stderr.User-facing error should go to stderr; keep non-zero exit.
Apply:
- puts "Unknown argument: #{ARGV[0]}" - puts "Run 'bin/dev help' for usage information" + warn "Unknown argument: #{ARGV[0]}" + warn "Run 'bin/dev help' for usage information"
100-105: Minor: simplify help flag check.Cleaner, less duplication.
Apply:
-elsif ARGV[0] == "help" || ARGV[0] == "--help" || ARGV[0] == "-h" +elsif %w[help --help -h].include?(ARGV[0])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/generators/react_on_rails/bin/dev(1 hunks)lib/generators/react_on_rails/install_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)spec/dummy/bin/dev(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(2 hunks)spec/react_on_rails/generators/install_generator_spec.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
- lib/generators/react_on_rails/install_generator.rb
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- spec/dummy/bin/dev
- spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/binstubs/dev_spec.rb
🧠 Learnings (2)
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
PR: shakacode/react_on_rails#1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
ihabadham
force-pushed
the
update-generator-for-v15
branch
from
a61deeb to
243eb9f
Compare
Code Review: Update generator for v15🎉 Overall AssessmentThis PR successfully modernizes the React on Rails generator for v15 by migrating from Yarn to npm and adding comprehensive development workflow improvements. The changes are well-implemented and thoroughly tested. ✅ Code Quality & Best PracticesStrong Points:
Ruby Code Quality:
📈 Performance ConsiderationsImprovements:
Potential Concerns:
🔒 Security ConsiderationsGood Security Practices:
Recommendations:
🏗️ Architectural ImprovementsExcellent Design Decisions:
🧪 Test Coverage AssessmentComprehensive Testing:
🐛 Potential Issues
📝 Documentation & User ExperienceExcellent UX:
🎯 Recommendations
🏁 ConclusionThis is a high-quality PR that significantly improves the developer experience while maintaining backward compatibility. The migration from Yarn to npm is well-executed, the new development workflow is comprehensive, and the test coverage is excellent. Ready to merge after addressing the minor file path handling suggestion. 🏷️ Files Reviewed
Great work on this modernization effort! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/install_generator.rb (1)
30-34: Don’t proceed if Shakapacker auto-install fails (early return bug).ensure_shakapacker_installed returns from itself, but run_generators continues. Gate subsequent steps on a truthy result.
Apply:
- if installation_prerequisites_met? || options.ignore_warnings? - ensure_shakapacker_installed + if installation_prerequisites_met? || options.ignore_warnings? + return unless ensure_shakapacker_installed invoke_generators add_bin_scripts add_post_install_message
♻️ Duplicate comments (5)
lib/generators/react_on_rails/install_generator.rb (3)
101-114: Run installer via Bundler, return a boolean, and wrap long error (RuboCop).Also aligns with previous feedback.
- def ensure_shakapacker_installed - return if shakapacker_installed? - - GeneratorMessages.add_info("Shakapacker not detected. Installing Shakapacker...") - - result = system("rails shakapacker:install") - unless result - GeneratorMessages.add_error("Failed to install Shakapacker automatically. " \ - "Please run 'rails shakapacker:install' manually.") - return - end - - GeneratorMessages.add_info("Shakapacker installed successfully!") - end + def ensure_shakapacker_installed + return true if shakapacker_installed? + + GeneratorMessages.add_info("Shakapacker not detected. Installing Shakapacker...") + + result = system("bundle", "exec", "rails", "shakapacker:install") + unless result + GeneratorMessages.add_error(<<~MSG.strip) + Failed to install Shakapacker automatically. + Please run 'bundle exec rails shakapacker:install' manually. + MSG + return false + end + + GeneratorMessages.add_info("Shakapacker installed successfully!") + true + endOptional: after successful install, update package.json’s packageManager field to reflect the chosen manager/version.
68-74: If you keep npm checks, replace brittle backtick/blank? with a cross-platform CLI probe; otherwise delete this method.Backticks + .blank? mis-detect on Windows (error text is non-empty). Either remove this method (preferred, see above) or switch to a robust helper.
Option A (preferred): delete this method entirely.
- def missing_npm? - return false unless ReactOnRails::Utils.running_on_windows? ? `where npm`.blank? : `which npm`.blank? - - error = "npm is required. Please install it before continuing. https://docs.npmjs.com/downloading-and-installing-node-js-and-npm" - GeneratorMessages.add_error(error) - true - endOption B: adopt a helper (outside this hunk) and use it here:
# private def cli_exists?(cmd) if ReactOnRails::Utils.running_on_windows? system("where", cmd, out: File::NULL, err: File::NULL) else system("command", "-v", cmd, out: File::NULL, err: File::NULL) end end def missing_npm? return false if cli_exists?("npm") GeneratorMessages.add_error("npm is required. Please install it before continuing. "\ "https://docs.npmjs.com/downloading-and-installing-node-js-and-npm") true endMirror the same approach for missing_node? for consistency.
65-65: Remove explicit npm prerequisite; defer to Shakapacker for package-manager handling.Per maintainer guidance, avoid enforcing npm/yarn here; let Shakapacker’s installer handle it.
- !(missing_node? || missing_npm? || ReactOnRails::GitUtils.uncommitted_changes?(GeneratorMessages)) + !(missing_node? || ReactOnRails::GitUtils.uncommitted_changes?(GeneratorMessages))spec/react_on_rails/generators/install_generator_spec.rb (1)
81-83: Align CLI detection tests with new strategy (no backticks).If you remove npm checks, drop these examples. If you keep them, stub a cli_exists? helper rather than backticks.
Example adjustments:
- allow(install_generator).to receive(:`).with("which npm").and_return("/path/to/bin") - expect(install_generator.send(:missing_npm?)).to be false + allow(install_generator).to receive(:cli_exists?).with("npm").and_return(true) + expect(install_generator.send(:missing_npm?)).to be falseand for “missing”:
- allow(install_generator).to receive(:`).with("which npm").and_return("") + allow(install_generator).to receive(:cli_exists?).with("npm").and_return(false)Windows variants:
- allow(install_generator).to receive(:`).with("where npm").and_return("/path/to/bin") + allow(install_generator).to receive(:cli_exists?).with("npm").and_return(true)Also applies to: 97-99, 113-115, 127-131
spec/react_on_rails/binstubs/dev_spec.rb (1)
55-56: Prefer allow_any_instance_of for stubbing Kernel.system (RuboCop).Make these non-strict stubs; keep assertions via other expectations if needed.
- expect_any_instance_of(Kernel).to receive(:system).with("overmind start -f Procfile.dev") + allow_any_instance_of(Kernel).to receive(:system).with("overmind start -f Procfile.dev").and_return(true) ... - expect_any_instance_of(Kernel).to receive(:system).with("foreman start -f Procfile.dev") + allow_any_instance_of(Kernel).to receive(:system).with("foreman start -f Procfile.dev").and_return(true)Also applies to: 64-65
🧹 Nitpick comments (2)
spec/react_on_rails/generators/install_generator_spec.rb (1)
137-151: Minor: tighten missing gem stub and shorten lines.Use a compact error instance to satisfy LineLength cops.
- allow(Gem::Specification).to receive(:find_by_name).with("shakapacker").and_raise(Gem::MissingSpecError.new( - "gem", "spec" - )) + error = Gem::MissingSpecError.new("shakapacker", ">= 0") + allow(Gem::Specification).to receive(:find_by_name).with("shakapacker").and_raise(error)spec/react_on_rails/binstubs/dev_spec.rb (1)
6-17: Silencing output: avoid leaking FDs and global state.Use around(:each) with StringIO to scope redirection and close handles.
around(:each) do |ex| orig_out, orig_err = $stdout, $stderr $stdout = StringIO.new $stderr = StringIO.new ex.run ensure $stdout = orig_out $stderr = orig_err end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/generators/react_on_rails/install_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)spec/dummy/bin/dev(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(1 hunks)spec/react_on_rails/generators/install_generator_spec.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- spec/dummy/bin/dev
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/generators/install_generator_spec.rbspec/react_on_rails/binstubs/dev_spec.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/install_generator.rb
🧠 Learnings (2)
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
PR: shakacode/react_on_rails#1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (3)
lib/react_on_rails/git_utils.rb (1)
uncommitted_changes?(7-20)lib/react_on_rails/utils.rb (1)
running_on_windows?(139-141)lib/generators/react_on_rails/generator_messages.rb (2)
add_info(17-19)add_error(9-11)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (newest)
🔇 Additional comments (2)
lib/generators/react_on_rails/install_generator.rb (1)
116-121: LGTM on gem presence check.Clear, fast check with proper rescue.
spec/react_on_rails/generators/install_generator_spec.rb (1)
160-176: Do not update spec’s system invocation
The implementation still callssystem("rails shakapacker:install"), and the spec’s expectations correctly match that.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/react_on_rails/generators/install_generator_spec.rb (2)
121-126: Initialize Gem::MissingSpecError with realistic args and simplify formatting.Use the actual gem name and a minimal requirement; also avoid the awkward multi-line constructor for readability.
- specify "when Shakapacker gem is not installed" do - allow(Gem::Specification).to receive(:find_by_name).with("shakapacker").and_raise(Gem::MissingSpecError.new( - "gem", "spec" - )) - expect(install_generator.send(:shakapacker_installed?)).to be false - end + specify "when Shakapacker gem is not installed" do + error = Gem::MissingSpecError.new("shakapacker", ">= 0") + allow(Gem::Specification) + .to receive(:find_by_name).with("shakapacker").and_raise(error) + expect(install_generator.send(:shakapacker_installed?)).to be false + end
144-151: Prefer heredoc for the long error message to satisfy cops and improve readability.This keeps lines short and mirrors the prior cop suggestion.
- expect(GeneratorMessages).to receive(:add_error) - .with("Failed to install Shakapacker automatically. " \ - "Please run 'rails shakapacker:install' manually.") + msg = <<~MSG.strip + Failed to install Shakapacker automatically. Please run 'rails shakapacker:install' manually. + MSG + expect(GeneratorMessages).to receive(:add_error).with(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/generators/react_on_rails/install_generator.rb(3 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/install_generator.rb
🧰 Additional context used
📓 Path-based instructions (1)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/generators/install_generator_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). (6)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (4)
spec/react_on_rails/generators/install_generator_spec.rb (4)
113-120: Good use of verifying doubles and positive-path coverage.The installed path uses instance_double correctly and keeps expectations tight. Looks good.
129-135: Already-installed branch is precise.Asserting no system call keeps behavior crisp; this is the right constraint.
136-142: Success path assertions read well.Messages and command stubbing match expectations and remain focused.
129-153: Implementation matches specs Verified thatensure_shakapacker_installeduses the exact"rails shakapacker:install"command and the same informational and error messages asserted in the spec.
ihabadham
force-pushed
the
update-generator-for-v15
branch
from
724b02b to
d5ff477
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
lib/generators/react_on_rails/bin/dev (5)
12-20: Harden pack generation: nil-safe $CHILD_STATUS and stderr output.Avoid NoMethodError when system fails; send failures to stderr.
system "bundle exec rake react_on_rails:generate_packs" - return if $CHILD_STATUS.success? + return if $CHILD_STATUS&.success? - puts "❌ Pack generation failed" + warn "❌ Pack generation failed" exit 1 end
36-53: Guard on precompile failure; exec Rails so signals propagate.Use a guard clause with nil‑safe check; write errors to stderr; replace system with exec.
- if $CHILD_STATUS.success? - puts "✅ Assets precompiled successfully" - puts "🚀 Starting Rails server in production mode..." - puts "" - puts "Press Ctrl+C to stop the server" - puts "To clean up: rm -rf public/packs && bin/dev" - puts "" - - # Start Rails in production mode - system "RAILS_ENV=production bundle exec rails server -p 3001" - else - puts "❌ Asset precompilation failed" - exit 1 - end + unless $CHILD_STATUS&.success? + warn "❌ Asset precompilation failed" + exit 1 + end + + puts "✅ Assets precompiled successfully" + puts "🚀 Starting Rails server in production mode..." + puts "" + puts "Press Ctrl+C to stop the server" + puts "To clean up: rm -rf public/packs && bin/dev" + puts "" + + # Replace current process so signals propagate properly + exec({ 'RAILS_ENV' => 'production' }, 'bundle', 'exec', 'rails', 'server', '-p', '3001')
70-87: Don’t rely on Errno::ENOENT for missing Procfile; precheck and exit cleanly.Also avoid exit!.
- if installed? "overmind" + unless File.exist?('Procfile.dev-static-assets') + warn "ERROR:\n Please ensure `Procfile.dev-static-assets` exists in your project!" + exit 1 + end + + if installed?('overmind') system "overmind start -f Procfile.dev-static-assets" - elsif installed? "foreman" + elsif installed?('foreman') system "foreman start -f Procfile.dev-static-assets" else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG - exit! + exit 1 end -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev-static-assets` exists in your project! - MSG - exit! -end +end
89-99: Same Procfile handling for HMR path; remove unreachable rescue and exit cleanly.- system "#{process} start -f Procfile.dev" -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev` exists in your project!" - MSG - exit! -end + unless File.exist?('Procfile.dev') + warn "ERROR:\n Please ensure `Procfile.dev` exists in your project!" + exit 1 + end + system "#{process} start -f Procfile.dev" +end
141-153: Use exit(1) instead of exit!.exit! skips ensure/at_exit.
- exit! + exit 1spec/dummy/bin/dev (5)
141-153: Use exit(1) instead of exit!.- exit! + exit 1
12-20: Harden pack generation: nil-safe $CHILD_STATUS and stderr output.Mirror fixes from lib/generators variant.
- return if $CHILD_STATUS.success? + return if $CHILD_STATUS&.success? - - puts "❌ Pack generation failed" + warn "❌ Pack generation failed" exit 1
36-53: Guard on precompile failure; exec Rails so signals propagate.- if $CHILD_STATUS.success? + unless $CHILD_STATUS&.success? + warn "❌ Asset precompilation failed" + exit 1 + end + + puts "✅ Assets precompiled successfully" + puts "🚀 Starting Rails server in production mode..." + puts "" + puts "Press Ctrl+C to stop the server" + puts "To clean up: rm -rf public/packs && bin/dev" + puts "" + + # Replace current process so signals propagate properly + exec({ 'RAILS_ENV' => 'production' }, 'bundle', 'exec', 'rails', 'server', '-p', '3001') - puts "✅ Assets precompiled successfully" - puts "🚀 Starting Rails server in production mode..." - puts "" - puts "Press Ctrl+C to stop the server" - puts "To clean up: rm -rf public/packs && bin/dev" - puts "" - - # Start Rails in production mode - system "RAILS_ENV=production bundle exec rails server -p 3001" - else - puts "❌ Asset precompilation failed" - exit 1 - end
70-87: Same Procfile.dev-static-assets precheck; avoid exit! and unreachable rescue.- if installed? "overmind" + unless File.exist?('Procfile.dev-static-assets') + warn "ERROR:\n Please ensure `Procfile.dev-static-assets` exists in your project!" + exit 1 + end + + if installed?('overmind') system "overmind start -f Procfile.dev-static-assets" - elsif installed? "foreman" + elsif installed?('foreman') system "foreman start -f Procfile.dev-static-assets" else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG - exit! + exit 1 end -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev-static-assets` exists in your project! - MSG - exit! -end +end
89-99: Same Procfile.dev precheck; remove rescue and use exit(1).- system "#{process} start -f Procfile.dev" -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev` exists in your project! - MSG - exit! -end + unless File.exist?('Procfile.dev') + warn "ERROR:\n Please ensure `Procfile.dev` exists in your project!" + exit 1 + end + system "#{process} start -f Procfile.dev" +end
🧹 Nitpick comments (3)
lib/generators/react_on_rails/bin/dev (1)
6-10: Return a boolean from installed? and avoid leaking an IO pipe.system gives a clean true/false and no open handle.
-def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false -end +def installed?(process) + system("#{process} -v >/dev/null 2>&1") +endlib/generators/react_on_rails/templates/base/base/Procfile.dev-static (1)
9-9: Reconsider cleaning packs on every watch start.rm -rf public/packs/* each time can be slow and surprising; consider relying on incremental builds and cleaning only when needed (e.g., before production precompile), or document the rationale.
lib/generators/react_on_rails/templates/base/base/Procfile.dev (1)
3-5: Prefer bin/rails for consistency with other Procfiles.Keeps Bundler context consistent with static/prod variants.
-rails: bundle exec rails s -p 3000 +rails: bin/rails s -p 3000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/bin/dev(2 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-static(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assets(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt(0 hunks)spec/dummy/bin/dev(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(1 hunks)spec/react_on_rails/support/shared_examples/base_generator_examples.rb(1 hunks)
💤 Files with no reviewable changes (1)
- lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt
✅ Files skipped from review due to trivial changes (2)
- lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets
- spec/react_on_rails/support/shared_examples/base_generator_examples.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/react_on_rails/binstubs/dev_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/base_generator.rb
🧠 Learnings (2)
📚 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:
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assetslib/generators/react_on_rails/bin/devlib/generators/react_on_rails/templates/base/base/Procfile.dev-staticlib/generators/react_on_rails/templates/base/base/Procfile.devlib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assetslib/generators/react_on_rails/bin/devlib/generators/react_on_rails/templates/base/base/Procfile.dev-staticlib/generators/react_on_rails/base_generator.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). (7)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: claude-review
- GitHub Check: examples (newest)
🔇 Additional comments (3)
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assets (1)
1-2: LGTM: minimal, clear static assets Procfile.Matches the new static flow and shakapacker watch.
lib/generators/react_on_rails/base_generator.rb (1)
32-37: LGTM: generator now copies new Procfiles (static/prod-assets).Aligns with bin/dev static/prod flows.
spec/dummy/bin/dev (1)
6-10: Boolean installed? implementation (no open IO).[ suggest_optional_refactor ]
-def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false -end +def installed?(process) + system("#{process} -v >/dev/null 2>&1") +end
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
spec/react_on_rails/generators/install_generator_spec.rb (2)
136-142: Break long expectations to satisfy LineLength.Split the chained expectation across lines; same behavior, cleaner formatting.
- expect(GeneratorMessages).to receive(:add_info).with("Shakapacker not detected. Installing Shakapacker...") - expect(GeneratorMessages).to receive(:add_info).with("Shakapacker installed successfully!") + expect(GeneratorMessages) + .to receive(:add_info).with("Shakapacker not detected. Installing Shakapacker...") + expect(GeneratorMessages) + .to receive(:add_info).with("Shakapacker installed successfully!")
147-151: Prefer heredoc over backslash string continuation.Removes awkward
\continuation and passes RuboCop LineLength.- expect(GeneratorMessages).to receive(:add_error) - .with("Failed to install Shakapacker automatically. " \ - "Please run 'rails shakapacker:install' manually.") + expect(GeneratorMessages) + .to receive(:add_error) + .with(<<~MSG.strip) + Failed to install Shakapacker automatically. + Please run 'rails shakapacker:install' manually. + MSG
🧹 Nitpick comments (5)
spec/react_on_rails/generators/install_generator_spec.rb (1)
121-126: Tighten MissingSpecError stubbing and reduce noise.Use a local error with realistic args to simplify formatting and intent.
- specify "when Shakapacker gem is not installed" do - allow(Gem::Specification).to receive(:find_by_name).with("shakapacker").and_raise(Gem::MissingSpecError.new( - "gem", "spec" - )) - expect(install_generator.send(:shakapacker_installed?)).to be false - end + specify "when Shakapacker gem is not installed" do + error = Gem::MissingSpecError.new("shakapacker", ">= 0") + allow(Gem::Specification) + .to receive(:find_by_name).with("shakapacker").and_raise(error) + expect(install_generator.send(:shakapacker_installed?)).to be false + endspec/dummy/bin/dev (4)
6-10: Makeinstalled?return a boolean and avoid leaking an IO
IO.popenreturns an IO (truthy) and isn’t closed; switch to a shell-builtin lookup for a clean boolean.-def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false -end +def installed?(tool) + system("command -v #{tool} >/dev/null 2>&1") +end
79-79: Useexit 1instead ofexit!
exit!skipsat_exithandlers and is unnecessary here. Standardize onexit 1for error paths.- exit! + exit 1Also applies to: 86-86, 98-98, 152-152
116-116: Remove stray interpolation line in help output
#{' '}prints two spaces on a separate line; drop it.- #{' '}
49-49: Optional: Replacesystemwithexecinspec/dummy/bin/devfor cleaner signal propagation
No test stubs referencesystemin this script, so updating won’t break any expectations. For example:- system "RAILS_ENV=production bundle exec rails server -p 3001" + exec "RAILS_ENV=production bundle exec rails server -p 3001"Apply the same swap for the other
systeminvocations in this file (e.g. the Overmind/Foreman commands around lines 71–74 and line 92).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/bin/dev(2 hunks)lib/generators/react_on_rails/install_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-static(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assets(1 hunks)lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt(0 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)spec/dummy/bin/dev(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(1 hunks)spec/react_on_rails/generators/install_generator_spec.rb(1 hunks)spec/react_on_rails/support/shared_examples/base_generator_examples.rb(1 hunks)
💤 Files with no reviewable changes (1)
- lib/generators/react_on_rails/templates/base/base/Procfile.dev.tt
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/generators/react_on_rails/templates/base/base/Procfile.dev
- lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets
- lib/generators/react_on_rails/base_generator.rb
- lib/generators/react_on_rails/bin/dev
- lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assets
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- spec/react_on_rails/support/shared_examples/base_generator_examples.rb
- lib/generators/react_on_rails/install_generator.rb
- lib/generators/react_on_rails/templates/base/base/Procfile.dev-static
- spec/react_on_rails/binstubs/dev_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/generators/install_generator_spec.rb
🔇 Additional comments (3)
spec/react_on_rails/generators/install_generator_spec.rb (3)
110-113: Good grouping for Shakapacker checks.Clear, scoped context and local subject via
let(:install_generator)reads well.
115-118: Nice switch to verifying doubles.
instance_doubleforGem::SpecificationandGem::Versionaligns with RSpec/VerifiedDoubles.
136-138: No change needed: spec stubs the exact system call used by the generator — the implementation invokessystem("rails shakapacker:install"), which matches the spec stub.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/dummy/Procfile.dev-prod-assets (4)
1-3: Document usage and precompile prerequisite.Make it explicit how to run this Procfile and that Shakapacker packs must be precompiled for “production assets” mode.
# Procfile for development with production assets # Uses production-optimized, precompiled assets with development environment -# Uncomment additional processes as needed for your app +# Usage: overmind/foreman/hivemind start -f Procfile.dev-prod-assets +# Prereq: ensure Shakapacker packs are precompiled: +# RAILS_ENV=development NODE_ENV=production bin/rails shakapacker:compile +# (The repo's bin/dev wrapper may automate this.) +# Uncomment additional processes as needed for your app
1-1: Trim trailing whitespace in the header.-# Procfile for development with production assets +# Procfile for development with production assets
9-9: Add a newline at EOF.Some tools/linters expect a trailing newline.
- +
5-5: Make port/bind configurable via ENV defaults
Replace in spec/dummy/Procfile.dev-prod-assets (line 5):- rails: bundle exec rails s -p 3001 + rails: bundle exec rails s -p ${PORT:-3001} -b ${BIND:-127.0.0.1}(Optional) Mirror this change in the corresponding template at lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets and other Procfile.dev* templates for consistency; no bin/dev references to this file were found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spec/dummy/Procfile.dev-prod-assets(1 hunks)spec/dummy/Procfile.dev-static-assets(1 hunks)spec/dummy/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- spec/dummy/README.md
- spec/dummy/Procfile.dev-static-assets
⏰ 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). (7)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: examples (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (1)
spec/dummy/Procfile.dev-prod-assets (1)
1-8: LGTM on adding a minimal, production-assets Procfile.This complements the new bin/dev modes and keeps the dummy setup straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/generators/react_on_rails/templates/base/base/config/webpack/test.js.tt (1)
3-3: Add semicolons for consistency with other env configs.Development/production templates use semicolons; test should match.
Apply within the changed lines:
-const generateWebpackConfigs = require('./generateWebpackConfigs') +const generateWebpackConfigs = require('./generateWebpackConfigs'); -module.exports = generateWebpackConfigs(testOnly) +module.exports = generateWebpackConfigs(testOnly);Additionally (outside the changed lines), consider ending the function assignment with a semicolon:
const testOnly = (_clientWebpackConfig, _serverWebpackConfig) => { // place any code here that is for test only };Also applies to: 9-9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt(2 hunks)lib/generators/react_on_rails/templates/base/base/config/webpack/production.js.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/webpack/test.js.tt(1 hunks)
🧰 Additional context used
🧠 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:
lib/generators/react_on_rails/templates/base/base/config/webpack/test.js.ttlib/generators/react_on_rails/templates/base/base/config/webpack/production.js.ttlib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 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:
lib/generators/react_on_rails/templates/base/base/config/webpack/production.js.ttlib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/webpack/production.js.tt
⏰ 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). (3)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: build
🔇 Additional comments (2)
lib/generators/react_on_rails/templates/base/base/config/webpack/production.js.tt (1)
3-3: Multi-config export LGTM —generateWebpackConfigs.js.ttexists and is referenced in test, development, and production configs.lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt (1)
5-5: Multi-config export LGTM; React Refresh plugin included
@pmmmwh/react-refresh-webpack-plugin and react-refresh are already added as devDependencies in base_generator.rb and present in spec/dummy/package.json.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (2)
11-12: Gatenested_entriesby Shakapacker version or annotate minimum supported versionIf an older Shakapacker is present, this key may be ignored or cause confusion. Either enforce a minimum gem version in the generator or annotate the requirement inline.
Apply this diff to document the requirement:
# Enable nested entry points for auto-registration + # Requires Shakapacker >= 8.2.0 nested_entries: trueRun to verify constraints across the repo:
#!/bin/bash set -euo pipefail echo "== Gem requirement lines ==" rg -nC2 -S '\bshakapacker\b' Gemfile* *.gemspec lib || true echo "== Locked versions (if any) ==" rg -n -S 'shakapacker' -g '!**/node_modules/**' -g '!vendor/**' **/Gemfile.lock || true
23-23: Update stale Procfile reference to new bin/dev workflowComment still points to Procfile.dev-static; align with bin/dev modes introduced in this PR.
Apply this diff:
- # This is false since we're running `bin/shakapacker -w` in Procfile.dev-static + # This is false since `bin/dev` runs Shakapacker in watch mode (e.g., `bin/dev --watch`)
🧹 Nitpick comments (3)
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (1)
10-10: Trim trailing whitespace (yamllint error)Remove trailing spaces on the blank line.
Apply this diff:
- +lib/generators/react_on_rails/base_generator.rb (2)
32-37: Be cautious overwriting Procfiles/layout in existing apps.Consider skipping copy if the target file exists to avoid clobbering user customizations.
- base_files.each { |file| copy_file("#{base_path}#{file}", file) } + base_files.each do |file| + copy_file("#{base_path}#{file}", file) unless File.exist?(file) + end
117-131: Good addition to .gitignore; also handle missing .gitignore.If
.gitignoreis absent, we can create it so the rule is always applied.- return unless File.exist?(gitignore_path) + unless File.exist?(gitignore_path) + create_file ".gitignore", "" + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/generators/react_on_rails/base_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/HelloWorld.module.css(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/HelloWorldServer.js(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx(1 hunks)lib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erb(1 hunks)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/HelloWorld.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/react_no_redux_generator.rblib/generators/react_on_rails/base_generator.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:
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.jslib/generators/react_on_rails/react_no_redux_generator.rblib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.jslib/generators/react_on_rails/react_no_redux_generator.rblib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erblib/generators/react_on_rails/base_generator.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js
🧬 Code graph analysis (3)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/HelloWorldServer.js (1)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx (1)
HelloWorld(5-20)
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
create_appropriate_templates(35-45)
lib/generators/react_on_rails/base_generator.rb (2)
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
copy_base_files(13-17)lib/generators/react_on_rails/react_with_redux_generator.rb (1)
copy_base_files(16-20)
🪛 YAMLlint (1.37.1)
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: claude-review
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build
🔇 Additional comments (9)
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (1)
30-30: Switch toserver: httplooks correct for modern webpack-dev-serverMatches current dev-server options and replaces legacy
https: false.lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js (1)
3-3: Path update to src/HelloWorld looks correct.Import aligns with the new src/HelloWorld layout and default export. No further action.
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/HelloWorldServer.js (1)
1-5: Minimal SSR entry LGTM.Re-exporting the component for SSR is fine. Keep browser-only APIs out of the component to avoid server crashes.
lib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erb (1)
7-9: No change required — Shakapacker zero-arg pack helpers are supported and recommended for runtime per-component injection.Shakapacker v7+ accepts stylesheet_pack_tag/javascript_pack_tag with no arguments (they render packs queued via append_stylesheet_pack_tag/append_javascript_pack_tag, or nothing if none queued); React on Rails’ file-system/component-registry workflow recommends the append_* + zero-arg layout call pattern.
lib/generators/react_on_rails/react_no_redux_generator.rb (2)
25-25: Comment clarity LGTM.Message accurately reflects auto-registration behavior.
15-16: Install flow verified: BaseGenerator runs before react_no_redux
InstallGenerator.invoke_generators calls "react_on_rails:base" before "react_on_rails:react_no_redux" (lib/generators/react_on_rails/install_generator.rb); BaseGenerator.copy_js_bundle_files provides HelloWorldServer.js and HelloWorld.module.css, so react_no_redux only copying the ror component JSX is correct.lib/generators/react_on_rails/base_generator.rb (3)
25-27: Creating auto-registration dirs — LGTM.Matches the new
src/HelloWorld/ror_componentsstructure.
47-49: Updated JS asset paths — LGTM.Server bundle and HelloWorld assets match the new layout.
63-63: Confirm: generateWebpackConfigs.js is used by environment configs; webpack.config.js appears listed but isn't the runtime entry—clarify or remove.
- Evidence: development.js.tt, production.js.tt, and test.js.tt all require('./generateWebpackConfigs') and export its result; base_generator.rb includes both paths which may confuse maintainers. Recommend keeping generateWebpackConfigs.js (used) and either:
- remove/stop templating webpack.config.js if unused, or
- update base_generator.rb comment to explain webpack.config.js is intentionally present and how it relates to generateWebpackConfigs.js.
...act_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx
Outdated
Show resolved
Hide resolved
Comprehensive Code Review - PR #1770: Update generator for v15🟢 Overall AssessmentThis is a substantial and well-structured PR that modernizes the React on Rails generator with auto-registration capabilities and improved developer experience. The changes are generally well-implemented with good architectural decisions. ✅ StrengthsCode Quality & Best Practices
Developer Experience Improvements
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
22-31: SSR auto-registration import rewrite verified. Confirmed lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.jsx containsimport * as style from './HelloWorld.module.css';— the gsub will match. Optional: replace the string-rewrite with a dedicated ror_components template to avoid brittle behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/generators/react_on_rails/react_with_redux_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(2 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.module.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧠 Learnings (2)
📚 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:
lib/generators/react_on_rails/react_with_redux_generator.rb
📚 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:
lib/generators/react_on_rails/react_with_redux_generator.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). (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (2)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
18-20: Including HelloWorld.module.css with the component is right.Ensures CSS Modules are available for both Redux and SSR paths.
58-60: ```shell
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Git branch: $(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo 'no git repo')"
echo
echo "== Show target file (if present) =="
if [ -f lib/generators/react_on_rails/react_with_redux_generator.rb ]; then
sed -n '1,240p' lib/generators/react_on_rails/react_with_redux_generator.rb
else
echo "lib/generators/react_on_rails/react_with_redux_generator.rb not found"
fi
echo
echo "== Search for 'yarn' occurrences (rg, git grep, grep fallbacks) =="
if command -v rg >/dev/null 2>&1; then
echo "-- rg (no-config, -uuu to disable ignores) --"
rg --no-config -uuu -n '\byarn\b' . || true
else
echo "rg not available"
fi
echo
echo "-- git grep --"
git grep -n --no-color -e '\byarn' || true
echo
echo "-- git ls-files | xargs grep -- (search tracked files) --"
git ls-files | xargs -r grep -nH -e '\byarn' || true
echo
echo "-- find + grep fallback (skip common build/vendor dirs) --"
find . -type f
-not -path './node_modules/' -not -path './dist/' -not -path './build/' -not -path './.git/'
-exec grep -nH --line-number '\byarn\b' {} + || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/generators/react_on_rails/generator_messages.rb (1)
61-62: Minor copy edit for clarity.
Use “server rendering” phrasing.- - To server render, change this line app/views/hello_world/index.html.erb to + - To enable server rendering, change app/views/hello_world/index.html.erb to:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/generators/react_on_rails/generator_messages.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/react_with_redux_generator.rb
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/generator_messages.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:
lib/generators/react_on_rails/generator_messages.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). (7)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (oldest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
🔇 Additional comments (1)
lib/generators/react_on_rails/generator_messages.rb (1)
64-64: Rename to HelloWorld aligns with the new templates. LGTM.
Matches the component rename across the generator assets and SSR entry points.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (1)
59-61: Blocking: switch generator from Yarn to npm and remove remaining Yarn artifactsReplace the Yarn invocation in the generator and remove/update lingering Yarn files/refs.
- def add_redux_yarn_dependencies - run "yarn add redux react-redux" + def add_redux_yarn_dependencies + run "npm install redux react-redux" endOptional cleanup (rename for clarity):
- def add_redux_yarn_dependencies + def add_redux_dependencies run "npm install redux react-redux" endActionable locations found during verification:
- lib/generators/react_on_rails/react_with_redux_generator.rb — apply diff above.
- spec/dummy/client/README.md — contains
yarn upgrade/yarn add(lines reported: 39, 56, 58) — update to npm commands or remove.- yarn.lock (repo root) and spec/dummy/yarn.lock — remove or replace with npm lockfile (package-lock.json) as appropriate.
🧹 Nitpick comments (7)
spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb (1)
5-6: Good: asserting absence of the manual pack for non-Redux.Matches the auto-registration goal. Consider tightening the spec by asserting the auto-reg files exist so failures are more actionable.
it "creates appropriate templates" do # No manual bundle for non-Redux (auto-registration only) assert_no_file("app/javascript/packs/hello-world-bundle.js") + assert_file("app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx") + assert_file("app/javascript/src/HelloWorld/HelloWorld.module.css")lib/generators/react_on_rails/react_with_redux_generator.rb (1)
22-27: Potentially unused duplicate CSS copy under src/.You copy HelloWorld.module.css to both components/ and src/. Ensure the auto-registered HelloWorld.jsx actually imports the src CSS; otherwise this duplicate becomes dead weight.
If unused, drop the extra copy:
- copy_file("#{base_js_path}/app/javascript/bundles/HelloWorld/components/HelloWorld.module.css", - "app/javascript/src/HelloWorld/HelloWorld.module.css") + # CSS under src/ is only needed if referenced by the auto-registered component.spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb (2)
8-15: Strengthen the spec to cover auto-registration outputs and path rewrites.Add assertions for the new auto-reg component, CSS, and the rewritten import paths to the store and container. This guards against future template drift.
it "creates appropriate templates" do assert_file("app/javascript/packs/hello-world-bundle.js") do |contents| expect(contents).to match("import HelloWorld from '../bundles/HelloWorld/startup/HelloWorldApp';") end assert_file("app/views/hello_world/index.html.erb") do |contents| expect(contents).to match(/"HelloWorld"/) end + # Auto-registration artifacts for Redux-connected component + assert_file("app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx") do |contents| + expect(contents).to include("../../../bundles/HelloWorld/store/helloWorldStore") + expect(contents).to include("../../../bundles/HelloWorld/containers/HelloWorldContainer") + end + assert_file("app/javascript/src/HelloWorld/HelloWorld.module.css") end
4-6: Optional: assert all created Redux directories.Generator creates containers and startup; include them for completeness.
- it "creates redux directories" do - %w[actions constants reducers store].each { |dir| assert_directory("app/javascript/bundles/HelloWorld/#{dir}") } - end + it "creates redux directories" do + %w[actions constants containers reducers store startup].each do |dir| + assert_directory("app/javascript/bundles/HelloWorld/#{dir}") + end + endspec/react_on_rails/support/shared_examples/base_generator_examples.rb (3)
14-16: Tighten the directory checks for auto-registration.Also assert the parent dir to catch partial generation failures.
# Auto-registration structure is always created + assert_directory "app/javascript/src/HelloWorld" assert_directory "app/javascript/src/HelloWorld/ror_components"
22-24: Ensure Procfiles reflect the npm switch and include bin/dev.Existence checks won’t catch stale Yarn commands; verify contents and assert bin/dev is generated.
- Procfile.dev-static-assets - Procfile.dev-prod-assets].each { |file| assert_file(file) } + Procfile.dev-static-assets + Procfile.dev-prod-assets + bin/dev].each { |file| assert_file(file) } + + # Procfiles should use npm (not yarn) + assert_file("Procfile.dev") { |c| expect(c).to include("npm"); expect(c).not_to match(/\byarn\b/) } + assert_file("Procfile.dev-static") { |c| expect(c).to include("npm") } + assert_file("Procfile.dev-static-assets") { |c| expect(c).to include("npm") } + assert_file("Procfile.dev-prod-assets") { |c| expect(c).to include("npm") }
26-27: Validate the auto-registered component exports.Add a quick content assertion to ensure default export for auto-registration.
# Auto-registration component is always created - assert_file "app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx" + assert_file "app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx" + assert_file("app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx", /export default .*HelloWorld/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/generators/react_on_rails/react_with_redux_generator.rb(2 hunks)spec/react_on_rails/support/shared_examples/base_generator_examples.rb(1 hunks)spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb(1 hunks)spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rbspec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rbspec/react_on_rails/support/shared_examples/base_generator_examples.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/react_with_redux_generator.rb
🧠 Learnings (2)
📚 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/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rbspec/react_on_rails/support/shared_examples/base_generator_examples.rb
📚 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/support/shared_examples/react_with_redux_generator_examples.rbspec/react_on_rails/support/shared_examples/base_generator_examples.rblib/generators/react_on_rails/react_with_redux_generator.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). (7)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: build
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (4)
lib/generators/react_on_rails/react_with_redux_generator.rb (2)
51-56: LGTM: switch to registering HelloWorld while keeping the startup path.Aligns with tests expecting import HelloWorld from ../bundles/.../HelloWorldApp and "HelloWorld" in the view.
28-31: Add guard + generator spec to lock rewritten import paths
- Keep the gsub rewrites in lib/generators/react_on_rails/react_with_redux_generator.rb (around lines 28–31) but add a generator spec that runs the generator and asserts the generated file contains these exact imports: '../../../bundles/HelloWorld/store/helloWorldStore' and '../../../bundles/HelloWorld/containers/HelloWorldContainer' (example: spec/generators/react_with_redux_generator_spec.rb).
- Verification note: my automated check could not find app/javascript/src/HelloWorld/ror_components/HelloWorld.jsx in the repo; confirm the actual generated path (app-react16 → app conversions may alter paths) and point the spec at the correct output file.
spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb (2)
10-10: LGTM: import alias updated to HelloWorld.Matches the generator change to register HelloWorld while keeping the same startup path.
13-13: LGTM: view expectation now matches "HelloWorld".Consistent with the new registration name.
| @@ -0,0 +1,2 @@ | |||
| web: bin/rails server -p 3000 | |||
There was a problem hiding this comment.
Why you added this file instead of Procfile.dev-static file? Also, Procfile.dev-static is not used anywhere now, should we delete it if it's not needed?
There was a problem hiding this comment.
Why you added this file instead of Procfile.dev-static file?
it's for clarity, pulled from this example repo: https://github.com/shakacode/react_on_rails-demo-v15-ssr-auto-registration-bundle-splitting
Procfile.dev-static is not used anywhere now, should we delete it if it's not needed?
it's still used here: https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/adapt_for_older_shakapacker_generator.rb
but i think we should use the new one there and delete old one yes
There was a problem hiding this comment.
Should we, could we remove https://github.com/shakacode/react_on_rails/blob/master/lib/generators/react_on_rails/adapt_for_older_shakapacker_generator.rb and upgrading Shakapacker can be required?
There was a problem hiding this comment.
@justin808 That would be a breaking change and theoretically require a new major version, no? Maybe we can get away with it.
PR Review: Update generator for v15🟢 Positive ChangesArchitecture & Modernization:
Code Quality:
🟡 Areas for ImprovementTest Coverage:
Error Handling:
Documentation:
🔧 Technical Recommendations
it "creates proper auto-registration structure" do
run_generator
expect(File).to exist("app/javascript/src/HelloWorld/ror_components/")
expect(File).to exist("app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx")
end
def update_gitignore_for_auto_registration
return unless File.exist?(gitignore_path)
gitignore_content = File.read(gitignore_path)
if gitignore_content.include?("**/generated/**")
say "Gitignore already contains generated files pattern", :blue
return
end
# ... rest of method
end
🚀 Performance & SecurityPerformance: ✅ Auto-registration should improve build times by eliminating manual bundle management 📋 Final AssessmentThis is a solid modernization PR that significantly improves the developer experience. The auto-registration system and improved Shakapacker integration are valuable improvements. Recommendation: ✅ Approve with minor suggestions The changes align well with modern React on Rails patterns and the test coverage for core functionality is adequate. Consider adding the suggested integration tests in a follow-up PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/generators/react_on_rails/base_generator.rb (2)
30-41: Avoid double slash in template path
base_pathalready ends with/;template("#{base_path}/#{file}.tt", ...)adds a redundant slash. Not harmful, but easy to tidy.Apply:
- base_path = "base/base/" + base_path = "base/base/" ... - template("#{base_path}/#{file}.tt", file, { packer_type: ReactOnRails::PackerUtils.packer_type }) + template("#{base_path}#{file}.tt", file, { packer_type: ReactOnRails::PackerUtils.packer_type })
116-131: Gitignore entry is sensible; consider narrowing the pattern (optional)
**/generated/**may hide unrelated directories named “generated.” Optionally scope to JS assets.Apply:
- # Generated React on Rails packs - **/generated/** + # Generated React on Rails packs + app/javascript/generated/**lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx (1)
1-5: SSR note: ensure server webpack handles CSS modulesThis server entry re-exports the client component, which imports CSS modules. Confirm the server webpack config stubs/handles CSS to avoid SSR runtime errors. If not, consider a server-specific wrapper that avoids style imports.
spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb (1)
17-25: Optionally assert auto-registration copies existSince the Redux generator now also copies to
src/HelloWorld/ror_components, add asserts to lock that in.Apply:
it "copies base redux files" do %w[app/javascript/bundles/HelloWorld/actions/helloWorldActionCreators.js app/javascript/bundles/HelloWorld/containers/HelloWorldContainer.js app/javascript/bundles/HelloWorld/constants/helloWorldConstants.js app/javascript/bundles/HelloWorld/reducers/helloWorldReducer.js app/javascript/bundles/HelloWorld/store/helloWorldStore.js startup/HelloWorldApp.client.jsx startup/HelloWorldApp.server.jsx].each do |file| copy_file("#{base_hello_world_path}/#{file}", "app/javascript/bundles/HelloWorld/#{file}") end end + + it "copies Redux-connected auto-registration files" do + %w[ + app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx + app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx + app/javascript/src/HelloWorld/HelloWorld.module.css + ].each { |file| assert_file(file) } + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/generators/react_on_rails/base_generator.rb(4 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb(1 hunks)lib/generators/react_on_rails/react_with_redux_generator.rb(3 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx(1 hunks)lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.jsx(1 hunks)spec/react_on_rails/support/shared_examples/base_generator_examples.rb(1 hunks)spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/generators/react_on_rails/react_no_redux_generator.rb
- spec/react_on_rails/support/shared_examples/base_generator_examples.rb
- lib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.js
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/react_with_redux_generator.rb
🧠 Learnings (3)
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.jsxlib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
📚 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/support/shared_examples/react_with_redux_generator_examples.rblib/generators/react_on_rails/base_generator.rblib/generators/react_on_rails/react_with_redux_generator.rb
📚 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:
lib/generators/react_on_rails/base_generator.rb
🧬 Code graph analysis (3)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.jsx (1)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (1)
HelloWorld(5-20)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx (1)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (1)
HelloWorld(5-20)
lib/generators/react_on_rails/base_generator.rb (2)
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
copy_base_files(13-18)lib/generators/react_on_rails/react_with_redux_generator.rb (1)
copy_base_files(16-35)
⏰ 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). (8)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build
- GitHub Check: examples (oldest)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (newest)
🔇 Additional comments (12)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.jsx (1)
1-5: LGTM: simple server re-export is fine for SSRRe-exporting the client component keeps parity and matches the generator flow.
lib/generators/react_on_rails/base_generator.rb (4)
25-27: Good addition: create auto-registration directoriesThis ensures the ror_components target exists before copies.
33-36: Confirm Procfile names align with bin/dev orchestrationVerify that your new
bin/devexpects exactly these names and that the docs mention each mode.
47-49: LGTM: relocate CSS module under src/HelloWorldMatches the new auto-registration structure.
55-63: Webpack config set looks broad—ensure no duplication/conflictYou’re templating both legacy individual configs and
generateWebpackConfigs.js. Please confirm they’re intended to coexist and that templates exist for all listed files.spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb (1)
10-10: LGTM: tests updated to new import and component nameExpectations match the generator changes and new client/server split.
Also applies to: 13-13, 23-24
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (1)
1-26: LGTM: simple, typed props and CSS module usageComponent structure and import paths look correct for the new layout.
lib/generators/react_on_rails/react_with_redux_generator.rb (5)
18-20: LGTM: include JSX and CSS under bundlesKeeps Redux example self-contained while sharing styles with auto-registration.
22-29: LGTM: copy to auto-registration structureMatches the new ror_components layout.
55-57: LGTM: updated registration config for HelloWorldImport path and component name align with test expectations.
31-34: Verify gsub targets exist in the generated HelloWorld.client.jsxSandbox couldn't find app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx. Run the generator in a tmp app and confirm the generated file contains the original imports "../store/helloWorldStore" and "../containers/HelloWorldContainer" so the gsub replacements succeed; add a spec asserting the generated file contains "../../../bundles/HelloWorld/store/helloWorldStore" and "../../../bundles/HelloWorld/containers/HelloWorldContainer".
Verification command:
rg -n --line-number "../store/helloWorldStore|../containers/HelloWorldContainer|bundles/HelloWorld/(store|containers)" app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
3-10: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
git rev-parse --show-toplevel || true
git rev-parse --abbrev-ref HEAD || trueecho
echo "Root listing (first 120 lines):"
ls -la | sed -n '1,120p' || trueecho
echo "Searching repo (including hidden/ignored files) for 'yarn' occurrences..."
rg --hidden --no-ignore -n -S "yarn add|run yarn|\byarn\b" || trueecho
echo "Looking for react_with_redux_generator.rb file..."
found=$(find . -type f -iname 'react_with_redux_generator.rb' -print -quit || true)
if [ -n "$found" ]; then
echo "Found file: $found"
sed -n '1,240p' "$found" || true
else
echo "File not found. Searching by class name 'ReactWithReduxGenerator'..."
rg -n --hidden --no-ignore "class ReactWithReduxGenerator" || true
fiecho
echo "Searching for generator helper and package-manager abstractions..."
rg -n --hidden --no-ignore "generator_helper|GeneratorHelper|package_json.manager|package_manager|PackageManager" || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Code Review for PR #1770: Update generator for v15I've thoroughly reviewed the changes in this PR that modernize the React on Rails generator for v15. Here's my comprehensive feedback: ✅ Strengths
🔍 Issues and Suggestions1. Inconsistent Procfile NamingThe PR introduces multiple Procfile variants with inconsistent naming:
Recommendation: Standardize on one naming convention. I suggest:
2. Potential Race Condition in Pack GenerationThe Recommendation: Add a verification step to ensure required packs exist after generation. 3. Security Consideration: Production Mode on Port 3001Running production mode on port 3001 without explicit warnings about exposed secrets/credentials could be risky if developers have production credentials in their environment. Recommendation: Add a warning message when starting production-like mode about ensuring no production secrets are in the environment. 4. Performance: Asset PrecompilationThe production-like mode runs full asset precompilation each time, which can be slow. Consider checking if assets are already compiled and offering a --force flag to recompile. 5. Code Duplication in bin/devThe error messages for missing Overmind/Foreman are duplicated in multiple places (lines 75-79, 148-152). Recommendation: Extract to a helper method: def ensure_process_manager_installed
return if installed?("overmind") || installed?("foreman")
warn <<~MSG
NOTICE:
For this script to run, you need either 'overmind' or 'foreman' installed...
MSG
exit!
end6. Missing Documentation UpdatesThe PR checklist indicates documentation updates are pending. Key areas that need updating:
🐛 Potential Bugs
📋 Additional Recommendations
✅ Overall AssessmentThis is a solid PR that brings important modernizations to React on Rails. The core functionality is well-implemented with good test coverage. The main areas for improvement are around consistency in naming, documentation, and some minor code organization improvements. Once the documentation is updated and the Procfile naming is standardized, this will be ready to merge. Great work on modernizing the developer experience! 🚀 This review was performed focusing on code quality, best practices, performance, security, and test coverage as outlined in the project's CLAUDE.md guidelines. |
Add comprehensive documentation to CLAUDE.md about Rails Engine specifics: - Automatic rake task loading from lib/tasks/ (avoid duplicate execution) - Engine initializers and hooks (config.to_prepare, initializer blocks) - Engine vs application code contexts - Testing strategies (dummy app vs unit tests) - Common pitfalls and best practices Create RAKE_TASK_DUPLICATE_ANALYSIS.md analyzing PR #1770: - Why explicit rake_tasks block was added during Generator Overhaul - How it caused 2x slower builds for 2 months - Lessons learned about massive PRs and subtle performance bugs - Best practices for Rails::Engine rake task loading This documentation prevents future bugs like duplicate task execution and helps developers understand Rails Engine development nuances. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add comprehensive tests to verify Rails::Engine properly loads rake tasks from lib/tasks/ without needing explicit rake_tasks block: 1. Verifies engine.rb does NOT have a rake_tasks block - This prevents duplicate task execution (PR #1770 bug) - Ensures we rely on Rails::Engine automatic loading 2. Verifies all expected task files exist in lib/tasks/ - assets.rake, generate_packs.rake, locale.rake, doctor.rake - Confirms files are in the standard location for automatic loading Update .rubocop.yml to exclude engine_spec.rb from ModuleLength check as the comprehensive test suite requires many examples to properly validate Rails::Engine behavior. These tests catch the duplicate loading bug that occurred in PR #1770 and was fixed in PR #2052. See RAKE_TASK_DUPLICATE_ANALYSIS.md for complete historical context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
… rake task loading (#2067) ## Summary Adds comprehensive documentation about Rails Engine development and unit tests to prevent the duplicate rake task bug that occurred in PR #1770 and was fixed in PR #2052. ## Changes ### 1. Documentation (CLAUDE.md) Added new section "Rails Engine Development Nuances" covering: - **Automatic Rake Task Loading**: Rails::Engine automatically loads `lib/tasks/*.rake` files - no `rake_tasks` block needed - **Engine Initializers and Hooks**: Proper use of `config.to_prepare` and `initializer` blocks - **Engine vs Application Code**: Different contexts and what code runs where - **Testing Engines**: Dummy app vs unit tests - **Common Pitfalls**: Path resolution, autoloading, configuration ### 2. Analysis Document (RAKE_TASK_DUPLICATE_ANALYSIS.md) Complete analysis of why PR #1770 added the problematic `rake_tasks` block: - Context of the massive 97-file Generator Overhaul - Why explicit loading was added (ensuring task availability for new file-system auto-registration) - How it caused 2x slower builds for 2 months - Lessons for code reviews, testing, and documentation ### 3. Unit Tests (spec/react_on_rails/engine_spec.rb) Two new tests to catch this bug: - Verifies `engine.rb` does NOT have a `rake_tasks` block - Verifies all task files exist in the standard `lib/tasks/` location Updated `.rubocop.yml` to exclude engine_spec.rb from ModuleLength check. ## Why This Matters The Rails Engine nuances documented here are **not well documented** in general Rails guides, leading to bugs like: - PR #1770: Duplicate rake task execution causing 2x slower builds - Confusion about where code runs (engine vs host app) - Generator failures in different app configurations ## Testing - ✅ Unit tests pass and verify no `rake_tasks` block exists - ✅ RuboCop passes with exclusion for comprehensive test module - ✅ All files end with newlines ## References - **Bug introduced**: PR #1770 - "Generator Overhaul & Developer Experience Enhancement" - **Bug fixed**: PR #2052 - "Fix duplicate rake task execution by removing explicit task loading" - **Historical analysis**: See `RAKE_TASK_DUPLICATE_ANALYSIS.md` - **Rails Engine Guide**: https://guides.rubyonrails.org/engines.html 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <[email protected]>
🚀 React on Rails v15 Generator Overhaul & Developer Experience Enhancement
🎯 Major Improvements
⚡ Generator Architecture Redesign
HelloWorld.jsxapproachror_componentsdirectory structure with clear migration paths🎨 Enhanced Developer Experience
bin/dev help(wrongly reverted in bd4210b)/hello_world)🛠️ Technical Refinements
📚 Documentation & Guidance
💡 Comprehensive TODO Roadmap
🎓 Inline Developer Education
🔧 Problem Solving
❌ Issues Resolved
✅ Quality Improvements
🎉 Impact
This overhaul transforms the React on Rails v15 generator from a complex, error-prone setup into a streamlined, educational, and visually appealing developer experience. New users get a working foundation
immediately, while experienced developers receive clear guidance for advanced patterns.
The enhanced
bin/dev helpcommand now serves as both a functional tool and an educational resource, making React on Rails more accessible to developers at all skill levels.This change is