Skip to content

Conversation

ihabadham
Copy link
Contributor

@ihabadham ihabadham commented Sep 9, 2025

πŸš€ React on Rails v15 Generator Overhaul & Developer Experience Enhancement

🎯 Major Improvements

⚑ Generator Architecture Redesign

  • Simplified component structure: Replaced complex client/server split with unified HelloWorld.jsx approach
  • Smart documentation: Added inline guidance for when client/server separation is needed
  • Better file organization: Improved ror_components directory structure with clear migration paths
  • Eliminated bad imports: Fixed server components incorrectly importing from client files

🎨 Enhanced Developer Experience

  • Restored colorized output: Re-added Rainbow-colored URLs and visual hierarchy to bin/dev help (wrongly reverted in bd4210b)
  • Improved messaging: Better generator success messages with proper endpoint URLs (/hello_world)
  • Visual polish: Enhanced help command with emojis, colors, and better formatting
  • Process manager detection: Smart detection of Overmind/Foreman with tailored recommendations

πŸ› οΈ Technical Refinements

  • Babel configuration cleanup: Resolved duplicate preset conflicts common in yalc development
  • Yalc integration: Improved local development workflow with proper package linking
  • Linting compliance: Fixed ESLint/Prettier formatting and RuboCop style violations
  • Template consolidation: Streamlined generator templates for better maintainability

πŸ“š Documentation & Guidance

πŸ’‘ Comprehensive TODO Roadmap

  • Created detailed improvement plan covering advanced patterns, TypeScript support, and real-world examples
  • Documented migration paths from Webpacker to Shakapacker
  • Outlined future generator flags for different architectural patterns

πŸŽ“ Inline Developer Education

  • Added clear comments explaining when to use client/server split patterns
  • Provided examples for React Router, styled-components, and other libraries requiring separation
  • Included migration guidance from simple to advanced architectures

πŸ”§ Problem Solving

❌ Issues Resolved

  • Generator regression: Fixed components not rendering despite correct file structure
  • Import conflicts: Eliminated circular dependencies in generated templates
  • Configuration conflicts: Addressed Babel preset duplication issues
  • Visual regression: Restored accidentally removed colorization and formatting

βœ… Quality Improvements

  • Consistent code style: Applied ESLint auto-fixes and RuboCop corrections
  • Better error messaging: Improved guidance for common development issues
  • Process management: Enhanced development server startup with better visual feedback

πŸŽ‰ 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 Reviewable

Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 37917e5 and a05c43f.

πŸ“’ Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • CONTRIBUTING.md (3 hunks)

Walkthrough

Installs 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

Cohort / File(s) Summary
Install generator & Shakapacker integration
lib/generators/react_on_rails/install_generator.rb, react_on_rails.gemspec, lib/react_on_rails/packer_utils.rb, lib/generators/react_on_rails/generator_helper.rb, lib/react_on_rails/git_utils.rb, lib/react_on_rails/version_syntax_converter.rb, lib/react_on_rails/engine.rb
Make shakapacker a required runtime dependency; add robust detection/ensure_install flow (ensure_shakapacker_installed, Gemfile/lock/Bundler/runtime checks), replace yarn-only checks with package-manager detection, add Node version checks, lazy-load package_json and add add_npm_dependencies, and load engine rake tasks.
Base & React generators + templates
lib/generators/react_on_rails/base_generator.rb, lib/generators/react_on_rails/react_no_redux_generator.rb, lib/generators/react_on_rails/react_with_redux_generator.rb, lib/generators/react_on_rails/templates/base/base/..., lib/generators/react_on_rails/templates/redux/base/...
Switch to file-system auto-registration under app/javascript/src/.../ror_components; remove generated registration pack template; add client/server entrypoints and CSS module templates; update webpack/babel template entrypoints to generateWebpackConfigs; change JS dependency installation flow and add install_js_dependencies/update_gitignore_for_auto_registration.
Dev CLI & tooling
lib/generators/react_on_rails/bin/dev, lib/generators/react_on_rails/templates/base/base/bin/dev, lib/react_on_rails/dev.rb, lib/react_on_rails/dev/*
Add ReactOnRails::Dev namespace and utilities: ServerManager, ProcessManager, PackGenerator, FileManager; new bin/dev delegates modes (hmr/static/prod/kill/help) to ServerManager; centralize pack generation, procfile orchestration, cleanup, kill and help behaviors.
Procfile templates & dummy bins
lib/generators/react_on_rails/templates/base/base/Procfile.dev*, spec/dummy/bin/shakapacker, spec/dummy/bin/shakapacker-dev-server, removed spec/dummy/bin/webpacker*, spec/dummy/Procfile.dev*, spec/dummy/**/Procfile.dev-*
Add HMR/static/prod-assets Procfile templates; remove Webpacker bootstrap/pre-generation in dummy bin scripts; replace references to Procfile.dev-static β†’ Procfile.dev-static-assets and update README references.
Tests & shared examples
spec/react_on_rails/**/*_spec.rb, spec/react_on_rails/dev/*, spec/react_on_rails/support/*, spec/dummy/bin/dev, spec/react_on_rails/support/shared_examples/*
Update tests to expect Shakapacker-only flows, auto-registration structure, add unit specs for Dev utilities (PackGenerator, ProcessManager, ServerManager), revise bin/dev spec and shared_examples to the new file layout.
CI/workflows & rake tasks
.github/workflows/*, rakelib/*.rake, removed rakelib/webpacker_examples.rake, rakelib/shakapacker_examples.rake, rakelib/example_type.rb
Remove Webpacker example tasks, consolidate examples to Shakapacker, rewrite CI matrices (ruby/node/dependency-level), rename envs to CI_DEPENDENCY_LEVEL, update cache/artifact keys and job commands to shakapacker-focused flows.
Generator messages, docs & changelog
lib/generators/react_on_rails/generator_messages.rb, CHANGELOG.md, CONTRIBUTING.md, lib/generators/USAGE, docs/additional-details/generator-details.md, TODO.md
Revamp post-install banners with dynamic sections (process manager, package manager, shakapacker status), update docs and changelog to describe breaking migration to Shakapacker 8.2+, testing guidance and contributor workflows.
Lint/config & small edits
.rubocop.yml, .prettierignore, eslint.config.ts, script/convert, misc formatting changes
Add RuboCop exclusions for large generators, update Prettier ignores, disable react/prop-types for generator templates, update conversion script and React/Node target bumps, and small formatting/whitespace tweaks.
Remove legacy Webpacker artifacts
deleted: lib/generators/react_on_rails/adapt_for_older_shakapacker_generator.rb, rakelib/webpacker_examples.rake, many webpacker-era template/bin files
Remove legacy compatibility generators, example tasks and webpacker-era scripts/templates/tests and related detection branches.

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
Loading
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
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120+ minutes

Possibly related PRs

Poem

A hop, a nibble, I patch the stream,
Procfiles hum and terminals gleam.
Shaka hops in, Webpacker retires,
Packs auto-register, spark new wires.
πŸ₯• A rabbit cheers: dev servers spring to life here!


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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning Although the linked issue was generator‑focused, the PR contains many broader, repository‑wide changes that exceed that narrow scope: removal of Webpacker support and related rake tasks/generators, large CI/workflow matrix rewrites, additions of a new Dev subsystem (ServerManager, ProcessManager, PackGenerator, FileManager) and new bin/dev, public API/visibility changes (packer utils and other method visibility/signature adjustments), template behavior changes (prerender defaults, removal of PropTypes) and lint/task additions. These are substantial, breaking, or policy-level changes that should be reviewed and justified separately from the generator update. Recommend splitting remaining unrelated or infra changes into focused PRs (CI/workflows, API/visibility changes, and the new Dev tooling) or, if they must land together for v16, expand the PR description and CHANGELOG with a clear migration plan, a list of all breaking API changes, and explicit test/upgrade instructions so reviewers can evaluate each top-level change independently.
βœ… Passed checks (4 passed)
Check name Status Explanation
Linked Issues Check βœ… Passed For [#1767]: the PR implements the three coding objectives: it switches generator and docs to prefer npm (package-manager detection and install_js_dependencies), adds automatic Shakapacker installation and detection (ensure_shakapacker_installed plus gemspec changes), and updates the generator/templates/tests to an auto-registration structure (creates app/javascript/src/.../ror_components, removes manual registration templates, and adds PackGenerator/generate_packs integration). Based on the provided summary, these coding requirements are satisfied.
Docstring Coverage βœ… Passed No functions found in the changes. Docstring coverage check skipped.
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check βœ… Passed The title clearly identifies the primary intentβ€”a generator overhaul and developer-experience improvements tied to the next major releaseβ€”and matches the changeset (generator refactor, Shakapacker integration/auto-install, new bin/dev, npm default). It is a single readable sentence that reflects the main scope of the PR. It does include an emoji and mentions both "v15" and "v16," which is slightly ambiguous but not misleading about the change.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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
 end
spec/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f4adcd9 and ec9aed6.

πŸ“’ 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 $CHILD_STATUS from the stdlib 'English' module (don't forget to require it) over $?.


[error] 30-30: Trailing whitespace detected.


[error] 33-33: Trailing whitespace detected.


[error] 37-37: Trailing whitespace detected.


[error] 38-38: Style/SpecialGlobalVars: Prefer $CHILD_STATUS from the stdlib 'English' module over $?.


[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 asserting missing_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 asserting missing_npm? false is correct for Windows.


127-131: Good negative case for npm on Windows.

Covers the missing-path branch for Windows detection.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 expectations

With 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 of yarn
  • Globally replace stale yarn commands in docs and example scripts with their npm counterparts (or clearly note both package managers)
πŸ“œ Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between ec9aed6 and f729db1.

πŸ“’ 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
Copy link
Contributor

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 of Procfile.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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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; prefer exit 1 over exit!.

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
end
lib/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f729db1 and 8646f7a.

πŸ“’ Files selected for processing (4)
  • lib/generators/react_on_rails/bin/dev (1 hunks)
  • lib/generators/react_on_rails/install_generator.rb (3 hunks)
  • spec/react_on_rails/binstubs/dev_spec.rb (2 hunks)
  • spec/react_on_rails/generators/install_generator_spec.rb (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • spec/react_on_rails/binstubs/dev_spec.rb
  • spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
πŸ““ Path-based instructions (1)
lib/**/*.rb

πŸ“„ CodeRabbit inference engine (CLAUDE.md)

Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)

Files:

  • lib/generators/react_on_rails/install_generator.rb
🧠 Learnings (1)
πŸ“š Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.

Applied to files:

  • lib/generators/react_on_rails/bin/dev
🧬 Code graph analysis (1)
lib/generators/react_on_rails/install_generator.rb (3)
lib/react_on_rails/git_utils.rb (1)
  • uncommitted_changes? (7-20)
lib/react_on_rails/utils.rb (1)
  • running_on_windows? (139-141)
lib/generators/react_on_rails/generator_messages.rb (2)
  • add_info (17-19)
  • add_error (9-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: dummy-app-integration-tests (oldest)
  • GitHub Check: dummy-app-integration-tests (newest)
  • GitHub Check: rspec-package-tests (newest)
  • GitHub Check: rspec-package-tests (oldest)
  • GitHub Check: examples (newest)
  • GitHub Check: examples (oldest)
  • GitHub Check: build
πŸ”‡ Additional comments (2)
lib/generators/react_on_rails/install_generator.rb (2)

65-66: npm prerequisite switch β€” LGTM.


28-34: No leftover Yarn checks or non-Bundler installer invocations detected.

@ihabadham ihabadham force-pushed the update-generator-for-v15 branch from 8646f7a to dbb8b62 Compare September 9, 2025 19:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_process
lib/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 8646f7a and dbb8b62.

πŸ“’ Files selected for processing (7)
  • lib/generators/react_on_rails/bin/dev (1 hunks)
  • lib/generators/react_on_rails/install_generator.rb (3 hunks)
  • lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (1 hunks)
  • lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (1 hunks)
  • spec/dummy/bin/dev (1 hunks)
  • spec/react_on_rails/binstubs/dev_spec.rb (2 hunks)
  • spec/react_on_rails/generators/install_generator_spec.rb (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
  • lib/generators/react_on_rails/install_generator.rb
  • lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
  • spec/dummy/bin/dev
  • spec/react_on_rails/generators/install_generator_spec.rb
🧰 Additional context used
πŸ““ Path-based instructions (1)
spec/**/*.rb

πŸ“„ CodeRabbit inference engine (CLAUDE.md)

Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)

Files:

  • spec/react_on_rails/binstubs/dev_spec.rb
🧠 Learnings (2)
πŸ“š Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
PR: shakacode/react_on_rails#1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
  before do
    original_value = ENV["VARIABLE_NAME"]
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
  end

  after do
    allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
    ENV["VARIABLE_NAME"] = original_value
  end

  it "tests something" do
    # Your test code here
  end
end
```
This practice ensures test isolation and reliability.

Applied to files:

  • spec/react_on_rails/binstubs/dev_spec.rb
πŸ“š Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.

Applied to files:

  • spec/react_on_rails/binstubs/dev_spec.rb

@ihabadham ihabadham force-pushed the update-generator-for-v15 branch from a61deeb to 243eb9f Compare September 10, 2025 12:49
Copy link

claude bot commented Sep 10, 2025

Code Review: Update generator for v15

πŸŽ‰ Overall Assessment

This 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 Practices

Strong Points:

  • Excellent migration strategy: Clean transition from Yarn to npm with proper method renaming (missing_yarn? β†’ missing_npm?)
  • Robust error handling: The ensure_shakapacker_installed method includes proper error checking and user feedback
  • DRY principle: The new bin/dev script eliminates code duplication with well-structured functions
  • Comprehensive test coverage: New specs cover all major scenarios including error conditions

Ruby Code Quality:

  • Proper exception handling in shakapacker_installed? method using rescue Gem::MissingSpecError
  • Good use of early returns in ensure_shakapacker_installed
  • Consistent coding style following project conventions

πŸ“ˆ Performance Considerations

Improvements:

  • Faster dependency resolution: npm typically has better dependency resolution than Yarn v1
  • Selective pack generation: The new dev script runs pack generation only when needed
  • Optimized production mode: Separate production-like mode with proper asset precompilation

Potential Concerns:

  • The installed? method in bin/dev could be optimized by caching results if called frequently
  • Consider adding timeout handling for the rails shakapacker:install system call

πŸ”’ Security Considerations

Good Security Practices:

  • No shell injection vulnerabilities - proper use of system() calls
  • Frozen string literals enabled
  • No hardcoded credentials or sensitive data

Recommendations:

  • Consider validating the success of rails shakapacker:install more thoroughly than just checking boolean result
  • The IO.popen calls in installed? are safe but could benefit from explicit command validation

πŸ—οΈ Architectural Improvements

Excellent Design Decisions:

  • Multi-mode development support: HMR, static, and production-like modes provide flexibility
  • Progressive enhancement: Graceful fallback from overmind to foreman to error message
  • Clear separation of concerns: Each development mode has its own function
  • User-friendly interface: Comprehensive help system and clear error messages

πŸ§ͺ Test Coverage Assessment

Comprehensive Testing:

  • βœ… All new methods have corresponding specs
  • βœ… Error conditions properly tested (missing tools, failed installs)
  • βœ… Cross-platform compatibility (Windows/Linux) maintained
  • βœ… Mock usage appropriate and doesn't over-mock

πŸ› Potential Issues

  1. File path handling in bin/dev:45: Consider using Rails-relative paths or checking if public/packs exists

  2. Process detection reliability: The installed? method could be more robust with better error handling

πŸ“ Documentation & User Experience

Excellent UX:

  • Clear command-line help with usage examples
  • Descriptive error messages with actionable instructions
  • Visual indicators (emojis) for different modes
  • Progressive disclosure of information

🎯 Recommendations

  1. Add timeout to Shakapacker installation for better reliability
  2. Consider adding a dry-run option for the generator to preview changes
  3. Add logging for debugging the development modes
  4. Configuration validation: Verify that required Procfiles exist before attempting to use them

🏁 Conclusion

This 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

  • βœ… lib/generators/react_on_rails/install_generator.rb
  • βœ… spec/dummy/bin/dev
  • βœ… spec/react_on_rails/generators/install_generator_spec.rb
  • βœ… spec/react_on_rails/binstubs/dev_spec.rb
  • βœ… Configuration templates

Great work on this modernization effort! πŸš€

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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
+      end

Optional: 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
-      end

Option 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
end

Mirror 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 false

and 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between a61deeb and 243eb9f.

πŸ“’ 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 calls system("rails shakapacker:install"), and the spec’s expectations correctly match that.

Likely an incorrect or invalid review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 243eb9f and 736f60d.

πŸ“’ 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 that ensure_shakapacker_installed uses the exact "rails shakapacker:install" command and the same informational and error messages asserted in the spec.

@ihabadham ihabadham mentioned this pull request Sep 10, 2025
3 tasks
@ihabadham ihabadham force-pushed the update-generator-for-v15 branch from 724b02b to d5ff477 Compare September 10, 2025 18:23
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 1
spec/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")
+end
lib/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 736f60d and 724b02b.

πŸ“’ 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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
+      end
spec/dummy/bin/dev (4)

6-10: Make installed? 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: Use exit 1 instead of exit!

exit! skips at_exit handlers and is unnecessary here. Standardize on exit 1 for error paths.

-    exit!
+    exit 1

Also 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: Replace system with exec in spec/dummy/bin/dev for cleaner signal propagation
No test stubs reference system 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 724b02b and d5ff477.

πŸ“’ 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 for Gem::Specification and Gem::Version aligns with RSpec/VerifiedDoubles.


136-138: No change needed: spec stubs the exact system call used by the generator β€” the implementation invokes system("rails shakapacker:install"), which matches the spec stub.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between d5ff477 and c02222f.

πŸ“’ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between c02222f and 18b6020.

πŸ“’ 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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: Gate nested_entries by Shakapacker version or annotate minimum supported version

If 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 workflow

Comment 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 18b6020 and 001aa9b.

πŸ“’ 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 to server: http looks correct for modern webpack-dev-server

Matches 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.

Copy link

claude bot commented Sep 11, 2025

Comprehensive Code Review - PR #1770: Update generator for v15

🟒 Overall Assessment

This 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.


βœ… Strengths

Code Quality & Best Practices

  • Clean Architecture: The new auto-registration system using ror_components subdirectory is well-designed
  • Consistent Patterns: File structure changes follow a logical hierarchy (app/javascript/src/HelloWorld/ror_components/)
  • Good Separation of Concerns: Server-side rendering logic separated in HelloWorldServer.js
  • Idempotent Operations: Gitignore updates check for existing entries before adding

Developer Experience Improvements

  • Enhanced bin/dev script: Excellent addition of multiple development modes (HMR, static, production-like)
  • Auto-installation: Automatic Shakapacker installation reduces setup friction
  • Clear Documentation: Help command in bin/dev provides useful guidance
  • Backward Compatibility: Maintains support for existing project structures

⚠️ Issues & Concerns

Medium Priority Issues

1. Error Handling Gap (install_generator.rb:98)

result = system("rails shakapacker:install")
  • No validation that Rails environment is properly set up
  • Should check if this is actually a Rails project before running Rails commands
  • Consider adding timeout for the system call

2. File Permission Security (install_generator.rb:86)

File.chmod(0o755, *files_to_become_executable)
  • Uses broad file permissions (755) without validation
  • Should verify files exist before chmod
  • Consider more restrictive permissions (744) for security

3. Path Traversal Risk (base_generator.rb:118-129)

gitignore_path = File.join(destination_root, ".gitignore")
gitignore_content = File.read(gitignore_path)
  • No validation that destination_root is safe
  • Should sanitize paths to prevent directory traversal

Minor Issues

4. Missing Error Handling (react_with_redux_generator.rb:29)

gsub_file(ror_component_file, "./HelloWorld.module.css", "../HelloWorld.module.css")
  • No validation that the file exists before string replacement
  • Could fail silently if file structure is unexpected

5. Inconsistent Naming

  • File renamed from webpackConfig.js β†’ generateWebpackConfigs.js but not reflected in all references
  • Consider a more consistent naming pattern across webpack files

πŸš€ Performance Considerations

Positive Impacts

  • Auto-registration: Eliminates manual bundle registration overhead
  • Nested entries: Shakapacker configuration optimizes entry point handling
  • Development modes: Different modes allow developers to choose appropriate performance trade-offs

Potential Concerns

  • Multiple file copies: Redux generator now copies files to both old and new locations - consider cleanup strategy
  • Pack generation: Automatic pack generation on every dev startup could slow boot time for large projects

πŸ”’ Security Analysis

Good Practices

  • No hardcoded secrets or credentials
  • Uses Rails generators framework properly
  • Follows principle of least privilege in most areas

Areas for Improvement

  • File operations need better validation (mentioned above)
  • System calls should be more defensive
  • Consider sandboxing the auto-installation process

πŸ§ͺ Test Coverage Assessment

Well Tested

  • βœ… bin/dev script functionality comprehensive
  • βœ… Existing generator core functionality
  • βœ… Auto-registration pack generation

Missing Test Coverage (Critical)

# Needed test cases:
it "creates new auto-registration directory structure"
it "updates gitignore safely" 
it "handles Shakapacker installation failures gracefully"
it "copies Redux components to both old and new structures"
it "validates file paths before operations"

The biggest gap is testing the new directory structure creation and dual file copying for Redux.


πŸ”§ Recommended Changes

High Priority

  1. Add path validation before file operations
  2. Improve error handling for Shakapacker installation
  3. Add missing tests for new directory structure
  4. Validate file existence before gsub operations

Medium Priority

  1. More restrictive file permissions where possible
  2. Cleanup strategy for dual file structure in Redux generator
  3. Timeout handling for system calls

Low Priority

  1. Consistent webpack file naming across all environments
  2. Performance optimization for pack generation

🎯 Conclusion

This is a high-quality PR that significantly improves the React on Rails development experience. The auto-registration feature is particularly valuable. The main concerns are around defensive programming - adding validation and error handling to make the generator more robust.

Recommendation: βœ… Approve with minor changes

The benefits far outweigh the issues, and the concerns raised are primarily defensive programming improvements rather than blocking problems.


Review completed with analysis of 15 modified files, security considerations, performance impact, and test coverage gaps.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 contains import * 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 001aa9b and 57224be.

πŸ“’ 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 -->

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 57224be and d4560d1.

πŸ“’ 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.

@ihabadham ihabadham requested a review from justin808 September 11, 2025 19:45
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 artifacts

Replace 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"
       end

Optional cleanup (rename for clarity):

-      def add_redux_yarn_dependencies
+      def add_redux_dependencies
         run "npm install redux react-redux"
       end

Actionable 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
+  end
spec/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between d4560d1 and 18e0376.

πŸ“’ 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
Copy link
Collaborator

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?

Copy link
Contributor Author

@ihabadham ihabadham Sep 12, 2025

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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.

Copy link

claude bot commented Sep 12, 2025

PR Review: Update generator for v15

🟒 Positive Changes

Architecture & Modernization:

  • βœ… Auto-registration system: The switch to file-system based component registry (ror_components) is excellent - eliminates manual bundle registration
  • βœ… Simplified deployment: Multiple Procfile variants provide clear deployment options (HMR, static, production-like)
  • βœ… Improved developer experience: Auto-installation of Shakapacker reduces setup friction
  • βœ… Better defaults: Enabling nested entry points and proper Shakapacker configuration

Code Quality:

  • βœ… Consistent naming: generateWebpackConfigs vs webpackConfig is more descriptive
  • βœ… Template consolidation: Moving from .tt templates to static files where appropriate
  • βœ… Path organization: New src/HelloWorld/ror_components/ structure follows conventions

🟑 Areas for Improvement

Test Coverage:

  • ⚠️ Missing tests: New ensure_shakapacker_installed method has good test coverage, but auto-registration components need integration tests
  • ⚠️ File generation verification: Should test that new directory structure (src/HelloWorld/ror_components/) is created correctly

Error Handling:

  • ⚠️ Silent failures: In update_gitignore_for_auto_registration, consider logging when gitignore updates are skipped
  • ⚠️ Path validation: No validation that shakapacker_installed? handles edge cases correctly

Documentation:

  • ⚠️ Migration path: Users upgrading from v14 may need guidance on how auto-registration affects existing components
  • ⚠️ Procfile variants: The multiple Procfile options could benefit from inline comments explaining use cases

πŸ”§ Technical Recommendations

  1. Add integration test for auto-registration workflow:
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
  1. Consider defensive programming in base_generator.rb:121:
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
  1. Validate bin/dev changes: The Ruby rewrite looks good but ensure all edge cases from the bash version are covered

πŸš€ Performance & Security

Performance: βœ… Auto-registration should improve build times by eliminating manual bundle management
Security: βœ… No security concerns identified - proper use of system commands with validation
Dependencies: βœ… Switch from Yarn requirement to npm is more flexible

πŸ“‹ Final Assessment

This 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 modules

This 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 exist

Since 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 18e0376 and beef2a7.

πŸ“’ 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 SSR

Re-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 directories

This ensures the ror_components target exists before copies.


33-36: Confirm Procfile names align with bin/dev orchestration

Verify that your new bin/dev expects exactly these names and that the docs mention each mode.


47-49: LGTM: relocate CSS module under src/HelloWorld

Matches the new auto-registration structure.


55-63: Webpack config set looks broadβ€”ensure no duplication/conflict

You’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 name

Expectations 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 usage

Component 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 bundles

Keeps Redux example self-contained while sharing styles with auto-registration.


22-29: LGTM: copy to auto-registration structure

Matches the new ror_components layout.


55-57: LGTM: updated registration config for HelloWorld

Import path and component name align with test expectations.


31-34: Verify gsub targets exist in the generated HelloWorld.client.jsx

Sandbox 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 pipefail

echo "PWD: $(pwd)"
git rev-parse --show-toplevel || true
git rev-parse --abbrev-ref HEAD || true

echo
echo "Root listing (first 120 lines):"
ls -la | sed -n '1,120p' || true

echo
echo "Searching repo (including hidden/ignored files) for 'yarn' occurrences..."
rg --hidden --no-ignore -n -S "yarn add|run yarn|\byarn\b" || true

echo
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
fi

echo
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 -->

Copy link

claude bot commented Sep 12, 2025

Code Review for PR #1770: Update generator for v15

I've thoroughly reviewed the changes in this PR that modernize the React on Rails generator for v15. Here's my comprehensive feedback:

βœ… Strengths

  1. Excellent Migration from Yarn to npm: The switch from Yarn to npm as the default package manager is well-implemented with proper fallback error messages and updated documentation.

  2. Auto-installation of Shakapacker: The automatic detection and installation of Shakapacker when missing is a great developer experience improvement. The implementation properly handles both success and failure cases.

  3. Enhanced bin/dev Script: The new Ruby-based bin/dev script is a significant improvement:

    • Multiple modes (HMR, static, production-like) provide flexibility
    • Clear user feedback with emojis and descriptive messages
    • Good error handling for missing dependencies (Overmind/Foreman)
    • Comprehensive help documentation
  4. Good Test Coverage: The test suite has been properly updated to cover:

    • npm detection (Windows and Unix)
    • Shakapacker auto-installation scenarios
    • bin/dev script behavior with different process managers

πŸ” Issues and Suggestions

1. Inconsistent Procfile Naming

The PR introduces multiple Procfile variants with inconsistent naming:

  • Procfile.dev-static-assets (referenced in bin/dev line 71)
  • Procfile.dev-static (in templates)
  • Procfile.dev-prod-assets vs Procfile.dev-production-assets

Recommendation: Standardize on one naming convention. I suggest:

  • Procfile.dev (default HMR)
  • Procfile.dev-static (static assets)
  • Procfile.dev-production (production-like)

2. Potential Race Condition in Pack Generation

The generate_packs function in bin/dev doesn't check if packs were successfully generated before proceeding. While it exits on failure, there might be edge cases where partial generation could cause issues.

Recommendation: Add a verification step to ensure required packs exist after generation.

3. Security Consideration: Production Mode on Port 3001

Running 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 Precompilation

The 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/dev

The 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 Updates

The PR checklist indicates documentation updates are pending. Key areas that need updating:

  • README should mention the new development modes
  • CHANGELOG.md needs an entry for this significant change
  • Migration guide for existing projects using Yarn

πŸ› Potential Bugs

  1. English Module Usage: Line 4 in bin/dev requires "English" but uses $CHILD_STATUS instead of the English constant. Either use $? directly or use $CHILD_STATUS consistently with the require.

  2. Missing File Path Validation: The webpack config generator doesn't validate that required client/server config files exist before requiring them (line 3-4 in generateWebpackConfigs.js).

πŸ“‹ Additional Recommendations

  1. Add npm version check: Similar to Node version checking, consider adding npm version validation to ensure compatibility.

  2. Environment variable documentation: Document the new environment variables (CLIENT_BUNDLE_ONLY, SERVER_BUNDLE_ONLY) in the configuration files.

  3. Rollback instructions: Provide clear instructions for cleaning up failed installations (e.g., rm -rf public/packs public/assets node_modules).

  4. Default auto_load_bundle: Consider keeping auto_load_bundle as false by default to maintain backward compatibility, with clear documentation on how to enable it.

βœ… Overall Assessment

This 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.

Copy link

claude bot commented Sep 17, 2025

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 Practices

Architecture & Design

  • Excellent modular design with the new ReactOnRails::Dev module separating concerns
  • Clean separation between server management, process management, and file operations
  • Smart use of Procfiles for different development modes (HMR, static, production-like)
  • Good use of Ruby's Thor::Error for better error handling in generators

Code Quality

  • Comprehensive error messages with actionable solutions (e.g., Node.js installation instructions)
  • Proper use of ANSI colors via Rainbow gem for better CLI output
  • Well-structured generator inheritance hierarchy
  • Good use of Ruby idioms and patterns

Developer Experience

  • Unified bin/dev script with intuitive subcommands
  • Auto-installation of Shakapacker when missing
  • Clear migration path documented in CHANGELOG.md
  • Helpful validation of prerequisites with bypass option

⚠️ Potential Issues & Suggestions

1. Process Termination Edge Cases
In lib/react_on_rails/dev/server_manager.rb:70-75, the process termination silently rescues all errors:

def terminate_processes(pids)
  pids.each do |pid|
    Process.kill("TERM", pid)
  rescue StandardError
    nil  # Silent failure
  end
end

Suggestion: Consider logging failed terminations or trying SIGKILL as fallback for stubborn processes.

2. Package Manager Detection
The package manager detection in install_generator.rb could be more robust. Consider checking for lock files (package-lock.json, yarn.lock, pnpm-lock.yaml) to determine the intended package manager.

3. Node Version Warning
While checking for Node < 18, the warning doesn't block installation. Consider making this a hard requirement given the v16 breaking changes theme.

🎯 Performance Considerations

  • Process Management: The pgrep approach for finding processes is efficient
  • File Operations: Good use of file existence checks before operations
  • CI Matrix: Smart reduction of test combinations while maintaining coverage
  • Consider implementing concurrent Procfile process startup for faster bin/dev initialization

πŸ”’ Security Analysis

βœ… Good Practices:

  • Proper use of File::NULL for suppressing error output
  • No hardcoded credentials or sensitive data
  • Safe process termination with SIGTERM

⚠️ Recommendations:

  • Validate Procfile contents before execution to prevent command injection
  • Consider adding rate limiting to bin/dev kill to prevent DoS scenarios
  • Add file permission checks when creating/modifying development scripts

πŸ§ͺ Test Coverage

Excellent Coverage βœ…

  • New specs for all Dev module components (ServerManager, ProcessManager, PackGenerator)
  • Updated generator specs for new behavior
  • Comprehensive CI matrix testing Ruby 3.2/3.4 and Node 20/22

Suggestions:

  1. Add integration tests for the full bin/dev workflow
  2. Test edge cases like missing Procfiles or malformed configurations
  3. Consider adding performance benchmarks for the new dev server modes

πŸ“ Documentation & Migration

Well Done:

  • Clear breaking changes documentation in CHANGELOG.md
  • Detailed CONTRIBUTING.md updates with generator testing workflow
  • Helpful inline comments in new modules

Missing/Could Improve:

  • Migration guide for projects using custom webpacker configurations
  • Troubleshooting section for common bin/dev issues
  • Examples of extending the Dev module for project-specific needs

πŸ› Minor Issues to Address

  1. Rubocop Configuration: Several exclusions added - ensure these are temporary
  2. Prettier Ignore: Broad *.yml ignore might hide formatting issues
  3. TODO.md: Contains 118 lines of TODOs - consider addressing or moving to issues

πŸ’‘ Recommendations

  1. Feature Flag Migration Path: Consider adding a temporary compatibility layer with deprecation warnings
  2. Telemetry: Add optional anonymous usage stats to understand adoption patterns
  3. Rollback Strategy: Document how to downgrade if issues arise
  4. Performance Metrics: Add benchmarks comparing v15 vs v16 startup times

✨ Overall Assessment

This is a high-quality PR that successfully modernizes React on Rails for contemporary development practices. The removal of Webpacker support and addition of the new dev server system are well-implemented. The code is clean, well-tested, and thoughtfully designed.

Recommendation: βœ… Approve with minor suggestions

The breaking changes are justified and well-documented. The new architecture provides a solid foundation for future enhancements. Address the minor security and error handling concerns mentioned above, and this will be an excellent v16 release.

Great work on this significant modernization effort! πŸŽ‰

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:install

Also 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 bc69dcd0

Also 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between bc69dcd and 3974fc2.

πŸ“’ 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: Verify bin/dev kill exists or update CONTRIBUTING.md.
CONTRIBUTING.md (lines 441–444) references bin/dev kill β€” confirm there is a bin/dev script with a kill subcommand; if not, implement it or update the doc to the repository's supported stop procedure.

Comment on lines +456 to +471
```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
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
```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.

justin808 and others added 2 commits September 16, 2025 16:23
…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]>
Copy link

claude bot commented Sep 17, 2025

Code Review for PR #1770: Breaking Changes for v16

Overall Assessment

This 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.

βœ… Strengths

1. Improved CI/CD Configuration

  • Matrix strategy now uses clearer naming (minimum/latest vs oldest/newest)
  • Ruby version matrix updated to 3.2-3.4 (dropping 3.0, adding 3.4)
  • Node version matrix updated to 20-22 (dropping 16, adding 22)
  • More explicit dependency-level tracking across test suites

2. Enhanced Generator Functionality

  • Auto-installation of Shakapacker when missing
  • Improved package manager detection with fallback strategies
  • Better error handling using Thor::Error instead of exit(1)
  • Clear, informative error messages with troubleshooting steps

3. Developer Experience Improvements

  • New bin/dev CLI supporting multiple modes (HMR, static, production-like)
  • Auto-detection and validation of JavaScript package managers
  • Helpful post-installation messages with emojis and clear formatting

πŸ” Areas of Concern

1. Security Considerations

The 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 Compatibility

The PR correctly marks these as breaking changes. Users will need to:

  • Upgrade Ruby to 3.2+ (from 3.0+)
  • Upgrade Node.js to 20+ (from 16+)
  • Migrate from Webpacker to Shakapacker 8.2+

Consider adding a migration guide to help users transition smoothly.

3. Test Coverage

Good test coverage for the install generator. However, consider adding tests for:

  • The new bin/dev CLI modes
  • Edge cases in package manager detection
  • Recovery scenarios when Shakapacker installation partially fails

πŸ“ Minor Suggestions

  1. File Cleanup: The .shakapacker_just_installed marker file (line 236) should be cleaned up after use or added to .gitignore

  2. Error Recovery: Consider adding rollback mechanisms if Shakapacker installation fails after Gemfile modification

🎯 Recommendations

  1. Documentation: Update the main README and migration guide for v16
  2. Deprecation: Consider adding deprecation warnings in v15.x for Webpacker users
  3. Validation: Add more robust validation for Node.js version compatibility

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! πŸŽ‰

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 β€” LGTM

Matches 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; prefer command -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"
       end

Option 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.tgz

Alternatively, use a placeholder:

  • react-on-rails-[VERSION].tgz
πŸ“œ Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 3974fc2 and f36e40d.

πŸ“’ 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 β€” LGTM

No manual bundle generation is appropriate here.


10-10: No‑op whitespace change

Nothing to do.


16-19: CSS location is correct; optional nit to rename variable

Both 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"' >> Gemfile

If 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]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 lint
lib/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 || prevents cleanup_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 bc69dcd0

Also 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 lint

Also 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 raise ArgumentError 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f36e40d and a9a7732.

πŸ“’ 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 with err: File::NULL and rejecting Process.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]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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). Prefer Gem::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 to npm 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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between a9a7732 and ed3bda3.

β›” 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 and nps 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 exec

package-scripts.yml contains autofix with script: rake autofix (lines 41–44). If CI environments lack Ruby/Bundler shims, change to bundle 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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to first

  1. Rails new with right params for things like no-javascript etc for simple app

  2. 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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

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
```

Copy link
Member

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
Copy link
Member

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]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 recommended

Server/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

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between ed3bda3 and 37917e5.

πŸ“’ 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 β€” OK

Correct 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) %>.

@justin808 justin808 changed the title Breaking Changes for v16 πŸš€ React on Rails v15 Generator Overhaul & Developer Experience Enhancement for v16 Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants