-
-
Notifications
You must be signed in to change notification settings - Fork 638
π React on Rails v15 Generator Overhaul & Developer Experience Enhancement for v16 #1770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.rb
spec/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 npm
and 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 npm
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
yarn
commands in docs and example scripts with theirnpm
counterparts (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π‘ 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' -S
Length 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_STATUS
is 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:install
wonβ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 1
overexit!
.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.
8646f7a
to
dbb8b62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.rb
spec/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_installed
uses the exact"rails shakapacker:install"
command and the same informational and error messages asserted in the spec.
724b02b
to
d5ff477
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-assets
lib/generators/react_on_rails/bin/dev
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static
lib/generators/react_on_rails/templates/base/base/Procfile.dev
lib/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-assets
lib/generators/react_on_rails/bin/dev
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static
lib/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.popen
returns 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 1
instead ofexit!
exit!
skipsat_exit
handlers and is unnecessary here. Standardize onexit 1
for 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: Replacesystem
withexec
inspec/dummy/bin/dev
for cleaner signal propagation
No test stubs referencesystem
in 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
system
invocations 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_double
forGem::Specification
andGem::Version
aligns 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.tt
lib/generators/react_on_rails/templates/base/base/config/webpack/production.js.tt
lib/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.tt
lib/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.tt
exists 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
β»οΈ Duplicate comments (2)
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (2)
11-12
: Gatenested_entries
by 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: true
Run 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
.gitignore
is 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.rb
lib/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.js
lib/generators/react_on_rails/react_no_redux_generator.rb
lib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erb
lib/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.js
lib/generators/react_on_rails/react_no_redux_generator.rb
lib/generators/react_on_rails/templates/base/base/app/views/layouts/hello_world.html.erb
lib/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: http
looks 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_components
structure.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.rb
spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb
spec/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.rb
spec/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.rb
spec/react_on_rails/support/shared_examples/base_generator_examples.rb
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). (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_path
already 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.rb
lib/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.jsx
lib/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.rb
lib/generators/react_on_rails/base_generator.rb
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/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/dev
expects 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!
end 6. 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. |
Code Review: Breaking Changes for v16 πThank you for this comprehensive PR introducing breaking changes for v16! This is a significant modernization effort that improves the developer experience. Here's my detailed review: β Strengths & Best PracticesArchitecture & Design
Code Quality
Developer Experience
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
π§Ή Nitpick comments (5)
CONTRIBUTING.md (5)
285-286
: Fix baseline reset command (missing tag).The βclean reset to baseline stateβ should actually reset to the tag you just created.
Apply this diff:
-git clean -fd && git reset --hard && git clean -fd +git clean -fd && git reset --hard generator_testing_base && git clean -fd
302-303
: Prefer bin/rails for consistency and correct gem context.Use bin/rails (or consistently use bundle exec) for Rails tasks.
Apply this diff:
-rails generate react_on_rails:install +bin/rails generate react_on_rails:install-bundle exec rails shakapacker:install +bin/rails shakapacker:install-rails generate react_on_rails:install +bin/rails generate react_on_rails:installAlso applies to: 317-318, 323-324
313-315
: Avoid Gemfile duplication; use Bundler to add shakapacker.Appending with echo can duplicate entries. Use bundle add.
Apply this diff:
-echo 'gem "shakapacker", "~> 8.0"' >> Gemfile -bundle install +bundle add shakapacker -v "~> 8.0"
292-292
: Fix MD036: use headings, not bold, for section titles.Converts emphasis-as-heading to actual headings.
Apply this diff:
-**Scenario A: No Shakapacker installed (fresh Rails app)** +##### Scenario A: No Shakapacker installed (fresh Rails app)-**Scenario B: Shakapacker already installed** +##### Scenario B: Shakapacker already installed-**Example: Testing commits 81c66fa and bc69dcd0** +##### Example: Testing commits 81c66fa and bc69dcd0Also applies to: 307-307, 344-344
391-399
: Clarify yalc usage (global vs npx) for consistency.Earlier we require global yalc; here we use npx. Add a note to avoid confusion.
Apply this diff:
npx yalc publish @@ npx yalc add react-on-rails npm install + +# Note: You can either install yalc globally (preferred for dev speed) +# or use `npx yalc` everywhere. Be consistent across steps.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
CONTRIBUTING.md
(1 hunks)
π§° Additional context used
π§ Learnings (1)
π Common learnings
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.
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
πͺ markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
292-292: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
307-307: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
344-344: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
π Additional comments (2)
CONTRIBUTING.md (2)
371-376
: Solid, actionable acceptance criteria.Clear expected outcomes for generator validation. No changes requested.
441-444
: Verifybin/dev kill
exists or update CONTRIBUTING.md.
CONTRIBUTING.md (lines 441β444) referencesbin/dev kill
β confirm there is abin/dev
script with akill
subcommand; if not, implement it or update the doc to the repository's supported stop procedure.
```bash | ||
# Navigate to the main react_on_rails directory | ||
cd react_on_rails/ | ||
|
||
# Run Prettier for JavaScript/TypeScript formatting | ||
yarn run format | ||
|
||
# Run ESLint for JavaScript/TypeScript linting | ||
yarn run lint | ||
|
||
# Run RuboCop for Ruby linting and formatting | ||
rake lint:rubocop | ||
|
||
# Or run all linters together | ||
rake lint | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align pre-commit commands with npm migration.
This section still uses Yarn; switch to npm.
Apply this diff:
-# Run Prettier for JavaScript/TypeScript formatting
-yarn run format
+# Run Prettier for JavaScript/TypeScript formatting
+npm run format
-# Run ESLint for JavaScript/TypeScript linting
-yarn run lint
+# Run ESLint for JavaScript/TypeScript linting
+npm run lint
π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```bash | |
# Navigate to the main react_on_rails directory | |
cd react_on_rails/ | |
# Run Prettier for JavaScript/TypeScript formatting | |
yarn run format | |
# Run ESLint for JavaScript/TypeScript linting | |
yarn run lint | |
# Run RuboCop for Ruby linting and formatting | |
rake lint:rubocop | |
# Or run all linters together | |
rake lint | |
``` | |
# Navigate to the main react_on_rails directory | |
cd react_on_rails/ | |
# Run Prettier for JavaScript/TypeScript formatting | |
npm run format | |
# Run ESLint for JavaScript/TypeScript linting | |
npm run lint | |
# Run RuboCop for Ruby linting and formatting | |
rake lint:rubocop | |
# Or run all linters together | |
rake lint |
π€ Prompt for AI Agents
In CONTRIBUTING.md around lines 456 to 471, update the pre-commit command
examples to use npm instead of yarn: replace any "yarn run format" with "npm run
format", "yarn run lint" with "npm run lint", and change the surrounding text
that references Yarn to reference npm; keep the rake lint and rake lint:rubocop
lines unchanged and ensure the code block reflects the new npm commands.
β¦tructure - Replace single HelloWorld.jsx with client/server split: - HelloWorld.client.jsx: Main component implementation - HelloWorld.server.jsx: Server-side wrapper importing from client - Update react_no_redux_generator.rb to copy both client and server files - This matches the proper_app_structure expected by React on Rails v15 - Enables proper SSR with separate client/server rendering paths Fixes generator structure to match test expectations in base_generator_examples.rb π€ Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Code Review for PR #1770: Breaking Changes for v16Overall AssessmentThis PR successfully removes Webpacker support and modernizes the toolchain for React on Rails v16. The changes are well-structured and align with current ecosystem standards. β Strengths1. Improved CI/CD Configuration
2. Enhanced Generator Functionality
3. Developer Experience Improvements
π Areas of Concern1. Security ConsiderationsThe generator uses system() calls in several places (lines 181, 205, 216, 223). While these commands are safe as implemented, ensure input validation if any user-provided values are ever passed to these system calls in future updates. 2. Backward CompatibilityThe PR correctly marks these as breaking changes. Users will need to:
Consider adding a migration guide to help users transition smoothly. 3. Test CoverageGood test coverage for the install generator. However, consider adding tests for:
π Minor Suggestions
π― Recommendations
Verdict: APPROVED with minor suggestions βThis is a well-executed modernization of the React on Rails toolchain. The removal of Webpacker support is timely, and the improvements to developer experience are valuable. Address the minor issues mentioned above, particularly the marker file cleanup and consider adding a migration guide. Great work on modernizing the project while maintaining good test coverage and developer experience! π |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
β»οΈ Duplicate comments (5)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (1)
2-2
: CSS Modules import is correct for namedExports=true.Acknowledging prior learning: this repoβs webpack config uses CSS Modules namedExports;
import * as style
is the right pattern.lib/generators/react_on_rails/generator_messages.rb (2)
65-65
: Fix sample URL to an existing route.The dummy app doesnβt expose /hello_world; use an existing route or add one.
For consistency with current dummy routes:
- 3. Visit: #{Rainbow('http://localhost:3000/hello_world').cyan.underline} + 3. Visit: #{Rainbow('http://localhost:3000/client_side_hello_world').cyan.underline}(Alternatively: /server_side_hello_world.)
68-71
: Pack helper snippet needs a pack name (copy/paste will fail).Both helpers require at least one entrypoint name.
Apply:
- <%= javascript_pack_tag %> - <%= stylesheet_pack_tag %> + <%= javascript_pack_tag 'application' %> + <%= stylesheet_pack_tag 'application' %>lib/generators/react_on_rails/react_no_redux_generator.rb (1)
25-26
: Config reduced to component_name β LGTMMatches the autoβregistration approach and prior fixes removing app_relative_path.
CONTRIBUTING.md (1)
478-495
: Align pre-commit commands with npm migration.Replace Yarn invocations with npm. This also addresses an earlier review asking for the same change.
# Run Prettier for JavaScript/TypeScript formatting -yarn run format +npm run format # Run ESLint for JavaScript/TypeScript linting -yarn run lint +npm run lint
π§Ή Nitpick comments (14)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (3)
4-6
: Avoid uncontrolledβcontrolled input warning; set a default string.If
props.name
is undefined, the input starts uncontrolled and later becomes controlled. Default it to ''.Apply this diff:
-const HelloWorld = (props) => { - const [name, setName] = useState(props.name); +const HelloWorld = ({ name: initialName = '' }) => { + const [name, setName] = useState(initialName);Also applies to: 14-14
1-1
: Optional: useId to ensure unique label/field association.Prevents ID collisions when multiple HelloWorlds render on a page.
Apply this diff:
-import React, { useState } from 'react'; +import React, { useId, useState } from 'react'; @@ - return ( + const inputId = useId(); + return ( @@ - <label className={style.bright} htmlFor="name"> + <label className={style.bright} htmlFor={inputId}> Say hello to: - <input id="name" type="text" value={name} onChange={(e) => setName(e.target.value)} /> + <input id={inputId} type="text" value={name} onChange={(e) => setName(e.target.value)} />Also applies to: 12-15
21-21
: Prettier: add trailing newline.Satisfy prettier/prettier βInsert ββ.
-export default HelloWorld; +export default HelloWorld; +lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx (2)
1-5
: Oneβline reβexport is cleaner.Functionally identical and terser.
-import HelloWorld from './HelloWorld.client'; -// This could be specialized for server rendering -// For example, if using React Router, we'd have the SSR setup here. - -export default HelloWorld; +export { default } from './HelloWorld.client';
5-5
: Prettier: add trailing newline.Satisfy prettier/prettier βInsert ββ.
-export default HelloWorld; +export default HelloWorld; +lib/generators/react_on_rails/generator_messages.rb (4)
56-58
: Improve install command per detected tool and lockfile.Use the optimal command for each package manager (prefer frozen/ci when lockfiles exist).
Apply:
- 1. Install dependencies: - #{Rainbow("bundle && #{package_manager} install").cyan} + 1. Install dependencies: + #{Rainbow("bundle && #{package_install_command(package_manager)}").cyan}Add helper (see addition near detect_package_manager):
+ def package_install_command(pm) + case pm + when "npm" + File.exist?("package-lock.json") ? "npm ci" : "npm install" + when "yarn" + "yarn install --frozen-lockfile" + when "pnpm" + "pnpm install --frozen-lockfile" + else + "npm install" + end + end
123-130
: Use a more portable command check for process managers.
which
isnβt guaranteed; prefercommand -v
for POSIX shells.Apply:
- if system("which overmind > /dev/null 2>&1") + if system("command -v overmind > /dev/null 2>&1") "overmind" - elsif system("which foreman > /dev/null 2>&1") + elsif system("command -v foreman > /dev/null 2>&1") "foreman" end
131-146
: Avoid sentinel file for installation status.Relying on
.shakapacker_just_installed
can leave repo artifacts; prefer passing state from the installer.Consider:
helpful_message_after_installation(shakapacker_just_installed: true)
βbuild_shakapacker_status_section(just_installed: ...)
and remove the file check.
148-157
: Package manager detection vs. βnpm-firstβ objectiveβconfirm intent.Current logic prefers Yarn if
yarn.lock
exists. If v15 aims to default to npm, adjust to prefer npm when multiple lockfiles exist (or always prefer npm).Option A (prefer npm when both exist):
- if File.exist?("yarn.lock") - "yarn" - elsif File.exist?("pnpm-lock.yaml") + if File.exist?("pnpm-lock.yaml") "pnpm" + elsif File.exist?("yarn.lock") && !File.exist?("package-lock.json") + "yarn" else # Default to npm (Shakapacker 8.x default) - covers package-lock.json and no lockfile "npm" endOption B (always npm-first): return "npm" unless explicitly running in a pnpm template.
CONTRIBUTING.md (5)
82-92
: Switch yalc workflow snippet to npm and add code-fence language.Align with this PRβs npm migration and fix MD040 by specifying the language.
-``` +```bash cd <top dir> # Will send the updates to other folders - MUST DO THIS AFTER ANY CHANGES yalc push cd spec/dummy # Will update from yalc -yarn +npm install
94-106
: Clarify post-yalc install step for npm users.The sample output mentions yarn; add an npm note to avoid confusion.
Don't forget you may need to run yarn after adding packages with yalc to install/update dependencies/bin scripts.Example: Testing NPM changes with the dummy app
+Note: If you use npm instead of Yarn, run
npm install
after adding packages with yalc.
294-329
: Use headings instead of bold text for section titles (MD036).Convert emphasized lines to proper headings.
-**Scenario A: No Shakapacker installed (fresh Rails app)** +##### Scenario A: No Shakapacker installed (fresh Rails app) @@ -**Scenario B: Shakapacker already installed** +##### Scenario B: Shakapacker already installed
346-350
: Use a heading for the βExampleβ label (MD036).-**Example: Testing commits 81c66fa and bc69dcd0** +##### Example: Testing commits 81c66fa and bc69dcd0
410-420
: Update versioned artifact name to v16 (or parameterize).This PR is for v16; avoid v15 artifacts in contributor docs.
-npm pack # Creates react-on-rails-15.0.0.tgz +npm pack # Creates react-on-rails-16.0.0.tgz @@ -npm install ../path/to/react_on_rails/react-on-rails-15.0.0.tgz +npm install ../path/to/react_on_rails/react-on-rails-16.0.0.tgzAlternatively, use a placeholder:
- react-on-rails-[VERSION].tgz
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (5)
CONTRIBUTING.md
(2 hunks)lib/generators/react_on_rails/generator_messages.rb
(1 hunks)lib/generators/react_on_rails/react_no_redux_generator.rb
(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)
π§° Additional context used
π Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
π CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/react_no_redux_generator.rb
lib/generators/react_on_rails/generator_messages.rb
lib/**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/react_no_redux_generator.rb
lib/generators/react_on_rails/generator_messages.rb
π§ Learnings (6)
π 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/src/HelloWorld/ror_components/HelloWorld.client.jsx
lib/generators/react_on_rails/react_no_redux_generator.rb
π Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
π 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/src/HelloWorld/ror_components/HelloWorld.client.jsx
lib/generators/react_on_rails/react_no_redux_generator.rb
π Learning: 2025-01-23T18:19:25.261Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: node_package/src/buildConsoleReplay.ts:12-12
Timestamp: 2025-01-23T18:19:25.261Z
Learning: When reviewing imports, carefully distinguish between default exports and named exports, as they represent different usage patterns. The presence of an import from a file doesn't necessarily mean all exported members are being used.
Applied to files:
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
π 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/src/HelloWorld/ror_components/HelloWorld.client.jsx
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
π 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/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
𧬠Code graph analysis (4)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (2)
lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/components/HelloWorld.jsx (1)
HelloWorld
(4-18)lib/generators/react_on_rails/templates/base/base/app/javascript/bundles/HelloWorld/components/HelloWorld.jsx (2)
HelloWorld
(4-19)name
(5-5)
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
(4-19)
lib/generators/react_on_rails/react_no_redux_generator.rb (1)
lib/generators/react_on_rails/react_with_redux_generator.rb (3)
hide_namespace
(7-74)copy_base_files
(20-36)create_appropriate_templates
(51-60)
lib/generators/react_on_rails/generator_messages.rb (1)
lib/react_on_rails/packs_generator.rb (1)
component_name
(340-344)
πͺ ESLint
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
[error] 21-21: Insert β
(prettier/prettier)
lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
[error] 5-5: Insert β
(prettier/prettier)
πͺ markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
309-309: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
346-346: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
π Additional comments (6)
lib/generators/react_on_rails/generator_messages.rb (1)
72-74
: Good: message now uses the configurable component name.This eliminates lingering βHelloWorldAppβ references in the banner.
Please confirm all templates/specs register and reference the same name (e.g., βHelloWorldβ) for SSR and client entry points.
lib/generators/react_on_rails/react_no_redux_generator.rb (3)
28-30
: Autoβregistration workflow: viewβonly template β LGTMNo manual bundle generation is appropriate here.
10-10
: Noβop whitespace changeNothing to do.
16-19
: CSS location is correct; optional nit to rename variableBoth templates import './HelloWorld.module.css' as a sibling (lib/generators/react_on_rails/templates/base/base/app/javascript/bundles/HelloWorld/components/HelloWorld.jsx and lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx), so the ror_components CSS placement is intentional and safe. Optional: rename base_js_path β base_path in lib/generators/react_on_rails/react_no_redux_generator.rb for consistency with create_appropriate_templates.
Likely an incorrect or invalid review comment.
CONTRIBUTING.md (2)
315-317
: Confirm required Shakapacker version and reflect it here.AI summary indicates β>= 8.2β. This snippet pins
~> 8.0
. Please align with the actual minimum.-echo 'gem "shakapacker", "~> 8.0"' >> Gemfile +echo 'gem "shakapacker", ">= 8.2"' >> GemfileIf the gemspec/enforced requirement differs, update this example accordingly.
439-442
: Verify bin/dev subcommands.Please confirm that bin/dev supports βstaticβ and βprodβ subcommands as documented, and add a brief βbin/dev helpβ snippet if available.
β¦tices - Add RuboCop disable/enable for ABC complexity in colorized help methods - Auto-fix ESLint/Prettier formatting in generator template files - Add comprehensive "Best Practices for AI Coding Agents" section to CONTRIBUTING.md - Include mandatory linting workflow to prevent CI failures - Document common AI agent mistakes and auto-fix commands All linters now pass locally before commit. π€ Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
β»οΈ Duplicate comments (3)
CONTRIBUTING.md (1)
484-495
: Align pre-commit examples with npm migration.Switch remaining Yarn commands to npm.
# Run Prettier for JavaScript/TypeScript formatting -yarn run format +npm run format # Run ESLint for JavaScript/TypeScript linting -yarn run lint +npm run lintlib/react_on_rails/dev/server_manager.rb (2)
214-226
: Avoid shell invocation for precompile; capture output and status robustly.Use Open3 with an env hash to improve signal handling and avoid shell parsing. This was raised earlier; still unresolved.
Apply:
- puts "π¨ Precompiling assets..." - success = system "RAILS_ENV=production NODE_ENV=production bundle exec rails assets:precompile" - - if success + puts "π¨ Precompiling assets..." + env = { "RAILS_ENV" => "production", "NODE_ENV" => "production" } + output, status = Open3.capture2e(env, "bundle", "exec", "rails", "assets:precompile") + puts output unless output.empty? + + if status.success?
26-33
: Don't shortβcircuit cleanup when processes are killed.Using
||
preventscleanup_socket_files
from running if any process was found. Always run both, then OR their results.Apply:
- killed_any = kill_running_processes || cleanup_socket_files + killed = kill_running_processes + cleaned = cleanup_socket_files + killed_any = killed || cleaned
π§Ή Nitpick comments (7)
CONTRIBUTING.md (5)
84-92
: Use npm (post-migration) and add a code-fence language.Replace Yarn call and annotate the fence to satisfy MD040.
-``` +```bash cd <top dir> # Will send the updates to other folders - MUST DO THIS AFTER ANY CHANGES yalc push cd spec/dummy # Will update from yalc -yarn +npm install -``` +```
294-301
: Convert emphasized lines to proper headings (markdownlint MD036).Use headings instead of bolded lines for section titles.
-**Scenario A: No Shakapacker installed (fresh Rails app)** +#### Scenario A: No Shakapacker installed (fresh Rails app) -**Scenario B: Shakapacker already installed** +#### Scenario B: Shakapacker already installed -**Example: Testing commits 81c66fa and bc69dcd0** +#### Example: Testing commits 81c66fa and bc69dcd0Also applies to: 309-328, 346-351
410-420
: Generalize npm pack filename to avoid hardcoding versions.Prevents docs drift each release and works cross-version.
```bash # In react_on_rails directory npm run build -npm pack # Creates react-on-rails-15.0.0.tgz +npm pack # Creates react-on-rails-<version>.tgz # In test app directory -npm install ../path/to/react_on_rails/react-on-rails-15.0.0.tgz +# Use the generated tarball; wildcard handles version +npm install ../path/to/react_on_rails/react-on-rails-*.tgz--- `94-95`: **Prefer a heading over emphasized text (readability + MD036).** ```diff -**β οΈ Common Mistake**: Forgetting to run `yalc push` after making changes to React on Rails source code will result in test apps not receiving updates, making it appear that your changes have no effect. +#### β οΈ Common Mistake +Forgetting to run `yalc push` after making changes to React on Rails source code will result in test apps not receiving updates, making it appear that your changes have no effect.
515-517
: Replace Yarn-specific eslint invocations with npm-compatible commands.Use npx to call local eslint without assuming script names.
-# If violations exist, auto-fix them first -yarn run eslint . --fix # Auto-fix JS/TS formatting +# If violations exist, auto-fix them first +npx eslint . --fix # Auto-fix JS/TS formatting bundle exec rubocop -A # Auto-fix Ruby violations-# If any violations, auto-fix immediately -yarn run eslint . --fix +# If any violations, auto-fix immediately +npx eslint . --fix bundle exec rubocop -A-# Verify everything passes -rake lint +# Verify everything passes +rake lintAlso applies to: 521-527, 529-530
lib/react_on_rails/dev/server_manager.rb (2)
70-76
: Terminate more reliably; escalate if TERM doesnβt exit.Optionally wait briefly and send KILL if the process remains alive. Improves dev ergonomics for hung processes.
Apply:
def terminate_processes(pids) pids.each do |pid| Process.kill("TERM", pid) rescue StandardError nil end + # Best-effort escalation + pids.each do |pid| + begin + deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + 3 + while Process.kill(0, pid) # still alive + break if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline + sleep 0.1 + end + rescue Errno::ESRCH + next # gone + end + begin + Process.kill("KILL", pid) + rescue StandardError + nil + end + end end
295-301
: Guard against negative padding in box rendering.Longer lines can make
padding
negative and raiseArgumentError
for string multiplication.Apply:
- padding = box_width - visual_length - 2 - line + "#{' ' * padding}β" + padding = [box_width - visual_length - 2, 0].max + line + (' ' * padding) + 'β'
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (4)
CONTRIBUTING.md
(2 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/react_on_rails/dev/server_manager.rb
(1 hunks)
π§ Files skipped from review as they are similar to previous changes (2)
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
π§° Additional context used
π Path-based instructions (2)
**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/react_on_rails/dev/server_manager.rb
lib/**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/react_on_rails/dev/server_manager.rb
𧬠Code graph analysis (1)
lib/react_on_rails/dev/server_manager.rb (2)
lib/react_on_rails/dev/process_manager.rb (2)
ensure_procfile
(14-22)run_with_process_manager
(24-45)lib/react_on_rails/dev/pack_generator.rb (2)
generate
(7-25)generate
(9-23)
πͺ markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
309-309: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
346-346: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
β° 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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
π Additional comments (3)
CONTRIBUTING.md (1)
273-341
: Manual generator test plan reads well and aligns with PR goals.Clear, reproducible workflow covering both Shakapacker scenarios and commit-by-commit validation. Nice job.
Please confirm the βbin/devβ expectations here match the actual Procfile templates and ServerManager defaults for this PR branch (ports, mode names).
lib/react_on_rails/dev/server_manager.rb (2)
62-68
: Good hardening: no shell and safe PID filtering.Using
Open3.capture2
witherr: File::NULL
and rejectingProcess.pid
avoids shell injection and selfβmatches. LGTM.
35-45
: Verify process patterns reflect v15+/Shakapacker flows.Ensure these cover your actual dev stack (e.g., Overmind/Foreman, shakapacker serve/watch). If webpack-dev-server is only spawned via Shakapacker, pattern overlap is fine; otherwise, consider adding/adjusting patterns as needed.
- Lower Shakapacker requirement from ~> 8.0 to >= 6.0 for broader compatibility - Add intelligent version warning in generator output for Shakapacker < 8.0 - Create unified autofix command (rake autofix) combining eslint --fix, prettier --write, and rubocop -A - Update AI agent best practices documentation with simplified workflow - Add prettier --write to linting workflow for complete formatting coverage The version strategy maintains backward compatibility while encouraging upgrades through clear messaging and actionable guidance. π€ Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
β»οΈ Duplicate comments (3)
lib/generators/react_on_rails/generator_messages.rb (2)
65-65
: Fix sample URL to an existing route in the dummy/templates.Prior feedback noted
/hello_world
doesnβt exist by default; use an actual route (e.g.,client_side_hello_world
).Apply:
- 3. Visit: #{Rainbow('http://localhost:3000/hello_world').cyan.underline} + 3. Visit: #{Rainbow('http://localhost:3000/client_side_hello_world').cyan.underline}
68-71
: Helpers need a pack name; current snippet will error.
javascript_pack_tag
/stylesheet_pack_tag
require a pack (e.g., "application"). This was previously flagged.Apply:
- <%= javascript_pack_tag %> - <%= stylesheet_pack_tag %> + <%= javascript_pack_tag "application" %> + <%= stylesheet_pack_tag "application" %>CONTRIBUTING.md (1)
493-508
: Align pre-commit commands with npm-first migration.Replace Yarn commands with npm equivalents for consistency with this PR.
Apply:
-# Run Prettier for JavaScript/TypeScript formatting -yarn run format +# Run Prettier for JavaScript/TypeScript formatting +npm run format @@ -# Run ESLint for JavaScript/TypeScript linting -yarn run lint +# Run ESLint for JavaScript/TypeScript linting +npm run lint
π§Ή Nitpick comments (5)
react_on_rails.gemspec (1)
34-34
: Add an upper bound to the Shakapacker dependency to avoid future major-breakage.Unbounded
>= 6.0
can pull a future breaking major. Recommend pinning to the supported majors (the banner recommends 8+).Apply:
- s.add_dependency "shakapacker", ">= 6.0" + s.add_dependency "shakapacker", ">= 8.0", "< 10"lib/generators/react_on_rails/generator_messages.rb (3)
123-129
: Use a portable command check (macOS/Linux/Windows shells).
which
isnβt universal;command -v
is more portable.Apply:
- if system("which overmind > /dev/null 2>&1") + if system("command -v overmind > /dev/null 2>&1") "overmind" - elsif system("which foreman > /dev/null 2>&1") + elsif system("command -v foreman > /dev/null 2>&1") "foreman"
151-181
: Robust version parsing for prereleases and non-3-part versions.Current regex misses prereleases (e.g.,
8.0.0.beta.1
). PreferGem::Version
.Apply:
- gemfile_lock_content = File.read("Gemfile.lock") - shakapacker_match = gemfile_lock_content.match(/shakapacker \((\d+\.\d+\.\d+)\)/) + gemfile_lock_content = File.read("Gemfile.lock") + shakapacker_match = gemfile_lock_content.match(/shakapacker \(([^)]+)\)/) @@ - version = shakapacker_match[1] - major_version = version.split(".").first.to_i + require "rubygems/version" + version_str = shakapacker_match[1] + major_version = Gem::Version.new(version_str).segments.first.to_i
183-193
: Optional: Prefer npm-first messaging to match the PR objective.Even if
yarn.lock
exists, consider defaulting tonpm
in banner instructions (still supporting Yarn detection elsewhere) to reinforce βnpm-first, Yarn not required.βrakelib/lint.rake (1)
28-34
: Consider making RuboCop auto-correct level configurable (unsafe -A vs safe -a).
-A
can introduce behavior changes. Provide an env-controlled flag for safety in CI/local.Apply this diff:
- sh_in_dir(gem_root, "bundle exec rubocop -A") + sh_in_dir(gem_root, "bundle exec rubocop #{ENV.fetch('RUBOCOP_AUTO_CORRECT_FLAG', '-A')}")Then CI can run with
RUBOCOP_AUTO_CORRECT_FLAG=-a
when desired.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
β Files ignored due to path filters (1)
Gemfile.lock
is excluded by!**/*.lock
π Files selected for processing (7)
CHANGELOG.md
(1 hunks)CONTRIBUTING.md
(2 hunks)TODO.md
(1 hunks)lib/generators/react_on_rails/generator_messages.rb
(1 hunks)package-scripts.yml
(1 hunks)rakelib/lint.rake
(1 hunks)react_on_rails.gemspec
(1 hunks)
π§ Files skipped from review as they are similar to previous changes (2)
- TODO.md
- CHANGELOG.md
π§° Additional context used
π Path-based instructions (2)
**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/generators/react_on_rails/generator_messages.rb
lib/**/*.rb
π CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/generators/react_on_rails/generator_messages.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:
react_on_rails.gemspec
π Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to **/*.rb : Use RuboCop for all Ruby code
Applied to files:
rakelib/lint.rake
𧬠Code graph analysis (1)
lib/generators/react_on_rails/generator_messages.rb (1)
lib/react_on_rails/packs_generator.rb (1)
component_name
(340-344)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
π Additional comments (4)
lib/generators/react_on_rails/generator_messages.rb (1)
41-46
: LGTM: Clear, structured post-install banner with dynamic sections.Nice touch detecting package/process managers and surfacing next steps.
rakelib/lint.rake (2)
40-41
: Nice top-level alias.The
autofix
alias is convenient. Once the Yarn β npm change above lands, this will be npmβfriendly endβtoβend.Please add a short note in CONTRIBUTING/README about
rake autofix
andnps autofix
so newcomers find it quickly. Want me to draft that?
28-34
: Replace Yarn invocations in lint tasks with npm exec; confirm repo-wide strategy
- Replace yarn calls in rakelib/lint.rake (eslint + autofix). Apply this diff:
- sh_in_dir(gem_root, "yarn run eslint . --fix") - sh_in_dir(gem_root, "yarn run prettier --write .") + sh_in_dir(gem_root, "npm exec -- eslint . --fix") + sh_in_dir(gem_root, "npm exec -- prettier --write .")
- Also update the eslint task in the same file:
- task :eslint do - sh_in_dir(gem_root, "yarn run eslint --version", "yarn run eslint .") - end + task :eslint do + sh_in_dir(gem_root, "npm exec -- eslint --version", "npm exec -- eslint .") + end
- Repo-wide follow-up: the repository still contains many Yarn references (package.json scripts, yarn.lock, spec/dummy/, node_package/scripts/, rakelib/node_package.rake, .github/workflows/*, docs/CONTRIBUTING.md, etc.). Decide whether the goal is to fully remove Yarn (update CI, scripts, docs, and example apps) or only to avoid blocking devs without Yarn; act accordingly and re-run a repo-wide search for '\byarn(\s|$)' after changes.
package-scripts.yml (1)
41-44
: Autofix delegates to rake β confirm CI provides Ruby/Bundler or use bundle execpackage-scripts.yml contains
autofix
withscript: rake autofix
(lines 41β44). If CI environments lack Ruby/Bundler shims, change tobundle exec rake autofix
or ensure Ruby+Bundler are installed; otherwise no change needed.
CHANGELOG.md
Outdated
|
||
#### Removed (Breaking Changes) | ||
|
||
- **Webpacker support completely removed**. Shakapacker 8.2+ is now required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= 6
CONTRIBUTING.md
Outdated
|
||
```bash | ||
# Navigate to test app directory (replace with your test app) | ||
cd ~/shakacode/react-on-rails/react_on_rails-test-apps/react-on-rails-tutorial-v15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use something like
{project_dir}/react_on_rails
(this repo)
{project_dir}/test-app
(where you will test the generator)
# Navigate to test app directory (replace with your test app) | ||
cd ~/shakacode/react-on-rails/react_on_rails-test-apps/react-on-rails-tutorial-v15 | ||
|
||
# Create a clean baseline tag for testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to first
-
Rails new with right params for things like no-javascript etc for simple app
-
For the test app:
- Need to indicate the relative path in the Gemfile react_on_rails
- Need to use the yalc add react-on-rails command
CONTRIBUTING.md
Outdated
git clean -fd && git reset --hard generator_testing_base && git clean -fd | ||
|
||
# Ensure Shakapacker is in Gemfile | ||
echo 'gem "shakapacker", "~> 8.0"' >> Gemfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, use bundle add strict
CONTRIBUTING.md
Outdated
bundle install | ||
|
||
# Run generator - should detect existing Shakapacker | ||
rails generate react_on_rails:install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always either bundle exec or ./bin/rails and be consistent
CONTRIBUTING.md
Outdated
|
||
This systematic approach ensures generator changes work correctly whether Shakapacker is pre-installed or needs to be installed by the generator. | ||
|
||
#### Testing Specific Generator Commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this section
CONTRIBUTING.md
Outdated
- React components render and are interactive | ||
- Development server starts successfully with `bin/dev` | ||
- Console output shows expected messages and minimal warnings | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove up to here
# Or run all linters together | ||
rake lint | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rake autofix
will fix all
CONTRIBUTING.md
Outdated
|
||
```bash | ||
# MANDATORY: Run linters after code changes | ||
rake lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just autofix?
- Change index.html.erb template from prerender: false to prerender: true - Demonstrates SSR capabilities out of the box for new React on Rails apps - Provides better initial experience with improved SEO and performance - Users can still disable by changing to prerender: false if needed π€ Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt (1)
2-2
: SSR wiring verified β optional optβout recommendedServer/client components, server-bundle initializer, webpack server-bundle generation, Shakapacker config, and specs referencing prerender: true are present β no blocking issues.
Relevant locations:
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx
- lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx
- lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (config.server_bundle_js_file = "server-bundle.js")
- lib/generators/react_on_rails/templates/base/base/config/webpack/{generateWebpackConfigs.js.tt, serverWebpackConfig.js.tt, clientWebpackConfig.js.tt}
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
- spec/react_on_rails/generators/dev_tests_generator_spec.rb and spec/react_on_rails/support/shared_examples/base_generator_examples.rb
Add an ENV/config toggle to opt out of prerendering in minimal setups (optional).
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt
(1 hunks)
π§° 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/app/views/hello_world/index.html.erb.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/app/views/hello_world/index.html.erb.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). (10)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
π Additional comments (1)
lib/generators/react_on_rails/templates/base/base/app/views/hello_world/index.html.erb.tt (1)
2-2
: SSR default in HelloWorld view β OKCorrect helper usage and proper
<%%= ... %>
escaping for a template-of-a-template. The generated view should be<%= react_component("HelloWorld", props: @hello_world_props, prerender: true) %>
.
π React on Rails v15 Generator Overhaul & Developer Experience Enhancement
π― Major Improvements
β‘ Generator Architecture Redesign
HelloWorld.jsx
approachror_components
directory 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 help
command 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β