Fix failing install generator tests for v16 #1784
Conversation
…ry - set nested_entries to true
- lib/generators/react_on_rails/bin/dev: Use Justin's enhanced version with emojis and better Ruby practices - spec/dummy/bin/dev: Keep full-featured Ruby template instead of simplified bash version - spec/react_on_rails/binstubs/dev_spec.rb: Preserve comprehensive test coverage vs minimal testing
…er references Replace ERB template processing with static Procfiles using 'shakapacker' directly instead of dynamic <%= config[:packer_type] %> detection, per Justin's feedback. **Templates → Static Files:** - Convert Procfile.dev.tt → Procfile.dev (static shakapacker references) - Convert Procfile.dev-static.tt → Procfile.dev-static (static shakapacker references) - Add Procfile.dev-static-assets (for bin/dev static mode) - Add Procfile.dev-prod-assets (for bin/dev production-asset mode) **Generator Updates:** - Update base_generator.rb to copy Procfiles as static files instead of templates - Remove dependency on ReactOnRails::PackerUtils.packer_type for Procfiles **Script Updates:** - Update bin/dev scripts to reference Procfile.dev-static-assets for static mode - Update dummy app bin/dev to match generator template **Test Updates:** - Update dev_spec.rb to expect new Procfile names - Update shared examples to verify all 4 Procfiles are generated - Update test dummy files to use correct Procfile references Addresses feedback from PR review about updating Procfiles to match react_on_rails-demo-v15-ssr-auto-registration-bundle-splitting repo.
- Replace Procfile.dev-static with Procfile.dev-static-assets - Add Procfile.dev-prod-assets for production-asset testing mode - Update README.md with correct Procfile references Fixes bin/dev script failures in dummy app. Addresses CodeRabbit feedback.
- Update generator test expectations for new auto-registration structure - Fix Redux auto-registration to use Redux-connected component - Ensure component name consistency across Redux and non-Redux generators
- Remove unused Procfile.dev-static template and references - Update base_generator.rb to not copy the unused file - Update tests to not expect the unused file - Fix AdaptForOlderShakapackerGenerator to handle new Procfile structure - Add missing Procfile.dev-prod-assets to backward compatibility generator
- Replace ineffective rescue Errno::ENOENT with File.exist? pre-checks - Change exit! to exit 1 for better at_exit handler support The rescue blocks were dead code since system() calls to foreman/overmind don't raise Errno::ENOENT for missing files - they just return false.
- Implement auto-registration structure for Redux (HelloWorldApp component) - Fix component naming consistency (HelloWorld → HelloWorldApp for Redux) - Update directory structure to src/HelloWorldApp/ror_components/ - Fix import paths for new auto-registration structure - Remove manual bundle creation (eliminate hello-world-bundle.js) - Add cleanup of conflicting base generator files this is because before it, manual registration and non redux HelloWorld component were created even when specifying --redux
- Clean Base: Make BaseGenerator conditional based on Redux option - Only create HelloWorld structure for non-Redux components - Only copy HelloWorld.module.css for non-Redux components - Simplify Redux: Remove cleanup_base_generator_files method entirely - Result: Eliminates wasteful create-then-delete pattern - Verified: Both non-Redux (HelloWorld) and Redux (HelloWorldApp) work correctly This approach is much cleaner since auto-registration is now the default behavior.
… content - Add missing stub for node --version call in Node.js detection tests - Mock git status to simulate clean repository for generator tests - Mock shakapacker_binaries_exist? to skip installation during tests - Update test expectations to handle new helpful message format Fixes 4 RSpec test failures in install_generator_spec.rb that were caused by new Node.js version checking and updated generator message behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors CI workflows to multi-axis matrices (ruby/node/dependency-level), removes Webpacker support in favor of Shakapacker ≥ 8.2, overhauls generators/templates for auto-registration (ror_components), adds dev tooling (ServerManager/ProcessManager/PackGenerator/FileManager), updates Procfiles, rake tasks, specs, docs, and changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Developer
participant bin as bin/dev
participant SM as ReactOnRails::Dev::ServerManager
participant PG as PackGenerator
participant PM as ProcessManager
participant OM as overmind
participant FM as foreman
U->>bin: run (no args | hmr | static | prod)
bin->>SM: start(mode, procfile)
alt mode = production_like
SM->>PG: generate()
SM->>SM: precompile assets (RAILS_ENV=production)
SM->>PM: ensure_procfile(Procfile.dev-prod-assets)
SM->>PM: run_with_process_manager(Procfile.dev-prod-assets)
else mode = development/static
SM->>PG: generate()
SM->>PM: ensure_procfile(Procfile.dev or dev-static-assets)
SM->>PM: run_with_process_manager(procfile)
end
PM->>PM: cleanup stale sockets/pids
alt overmind installed
PM->>OM: start -f <procfile>
else foreman installed
PM->>FM: start -f <procfile>
else
PM-->>U: exit 1 (no process manager)
end
sequenceDiagram
autonumber
participant Dev as Developer
participant IG as InstallGenerator
participant Bundler as Bundler
participant Shaka as Shakapacker
Dev->>IG: rails g react_on_rails:install
IG->>IG: check Node / package manager
IG->>IG: ensure_shakapacker_installed
alt shakapacker missing
IG->>Bundler: add gem "shakapacker", "~> 8.2"
IG->>Bundler: bundle exec rails shakapacker:install
IG->>IG: write .shakapacker_just_installed
end
IG-->>Dev: proceed with base/react generators (auto-registration, configs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review for PR #1784: Fix failing install generator tests for v16SummaryThis PR addresses critical test failures in the v16 branch by fixing 4 RSpec tests in ✅ Positive Aspects
🔧 Code Quality ObservationsTest File Changes (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rakelib/example_type.rb (1)
13-23: Guard against unsupported or nil packer_type.Verified: ripgrep found only CHANGELOG references to webpacker removal; the initialize method still does
self.class.all[packer_type.to_sym] << selfand will raise on nil/unsupported values — validate packer_type and fail fast with a clear error. Apply this diff:def self.all - @all ||= { shakapacker_examples: [] } + @all ||= { shakapacker_examples: [] } end @@ def initialize(packer_type: nil, name: nil, generator_options: nil) - @packer_type = packer_type + @packer_type = packer_type @name = name @generator_options = generator_options - self.class.all[packer_type.to_sym] << self + packer_type_sym = packer_type&.to_sym + unless self.class.all.key?(packer_type_sym) + raise ReactOnRails::Error, + "Unsupported packer_type: #{packer_type.inspect}. Allowed: #{self.class.all.keys.join(', ')}" + end + self.class.all[packer_type_sym] << self endrakelib/run_rspec.rake (1)
67-70: Fix NameError: task args not captured; default packer.
packeris undefined inside the task block. Captureargsand provide a sensible default.- task :run_rspec, [:packer] => ["all_but_examples"] do - Rake::Task["run_rspec:#{packer}_examples"].invoke + task :run_rspec, [:packer] => ["all_but_examples"] do |_t, args| + packer = (args[:packer] || "shakapacker").to_s + Rake::Task["run_rspec:#{packer}_examples"].invokespec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)
69-74: Update CI gating var ('CI_PACKER_VERSION' → 'CI_DEPENDENCY_LEVEL')Workflows set CI_DEPENDENCY_LEVEL now; update the test to detect "-minimum" runs so the older packer matrix still skips the async option.
File: spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (lines ~69–74)
- if ENV["CI_PACKER_VERSION"] == "oldest" + # “minimum” matrix uses older Shakapacker lacking async option support + if ENV["CI_DEPENDENCY_LEVEL"]&.end_with?("-minimum") expect(helper).to have_received(:append_javascript_pack_tag).with("generated/component_name", { defer: false }) else expect(helper).to have_received(:append_javascript_pack_tag) .with("generated/component_name", { defer: false, async: true }) end
🧹 Nitpick comments (92)
lib/react_on_rails/test_helper/webpack_assets_status_checker.rb (2)
33-41: Inconsistent return value semantics (relative vs absolute paths).When
manifest_neededis true, the method returns["manifest.json"], but otherwise returns absolute file paths. Mixed path semantics can confuse callers and complicate assertions.Consider returning a fully qualified path for the manifest as well:
- return ["manifest.json"] if manifest_needed + return [File.join(generated_assets_full_path, "manifest.json")] if manifest_neededIf callers intentionally expect the short name, please confirm and add a brief comment clarifying this contract.
67-75: Reduce noisy stdout in test environments.These
putscalls can clutter CI logs. Preferwarn(stderr) or a logger with a debug level, and gate behind a verbosity flag.- puts "V" * 80 - puts "Please define config.webpack_generated_files (array) so the test helper knows " \ + warn "V" * 80 + warn "Please define config.webpack_generated_files (array) so the test helper knows " \ "which files are required. If you are using Shakapacker, you typically need to only " \ "include 'manifest.json'." - puts "Detected the possible following files to check for webpack compilation in " \ + warn "Detected the possible following files to check for webpack compilation in " \ "#{generated_assets_full_path}" - puts file_list.join("\n") - puts "^" * 80 + warn file_list.join("\n") + warn "^" * 80lib/react_on_rails/version_syntax_converter.rb (1)
8-12: Regex tweak is fine; consider anchoring and nil-safety.Placing '-' at the end of the class makes it literal in Ruby, so behavior is unchanged. For robustness, anchor the pattern and avoid partial matches; also guard against non-matching inputs.
- regex_match = rubygem_version.match(/(\d+\.\d+\.\d+)[.-]?(.+)?/) + regex_match = rubygem_version.match(/\A(\d+\.\d+\.\d+)(?:[.-](.+))?\z/) + return rubygem_version unless regex_match return "#{regex_match[1]}-#{regex_match[2]}" if regex_match[2] regex_match[1].to_slib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb (1)
218-218: Nit: Align ScoutAPM metric name with helper’s naming conventionUse a snake_case name to match existing instrumentation (e.g., react_component, react_component_hash) for easier dashboard grouping.
- instrument_method :exec_server_render_js, type: "ReactOnRails", name: "ExecJs React Server Rendering" + instrument_method :exec_server_render_js, type: "ReactOnRails", name: "exec_server_render_js"lib/react_on_rails/helper.rb (1)
347-350: Likely key mismatch: use "consoleReplayScript" (consistent with rest of codebase)Throughout this file we return/consume "consoleReplayScript". Using "consoleLogScript" probably yields nil and drops console replay in browser.
html = result["html"] - console_log_script = result["consoleLogScript"] - raw("#{html}#{console_log_script if render_options.replay_console}") + console_log_script = result["consoleReplayScript"] + raw("#{html}#{console_log_script if render_options.replay_console}")lib/generators/react_on_rails/templates/redux/base/app/javascript/bundles/HelloWorld/startup/HelloWorldApp.server.jsx (2)
1-5: Optional: collapse to a one‑liner re‑exportSlight tidy-up that avoids an eager import binding and keeps the file tiny.
Apply this diff:
-import HelloWorldApp from './HelloWorldApp.client'; -// This could be specialized for server rendering -// For example, if using React Router, we'd have the SSR setup here. - -export default HelloWorldApp; +// This could be specialized for server rendering +// For example, if using React Router, we'd have the SSR setup here. +export { default } from './HelloWorldApp.client';
2-3: Clarify future SSR customizationIf you intend to add React Router/Helmet or per-request store preloading later, keep this comment; otherwise consider pointing to docs or removing to reduce noise.
spec/dummy/spec/support/selenium_logger.rb (4)
16-23: Prefer warn over puts for non-fatal warnings; include exception class for quicker triage.Using warn sends to stderr and keeps test output cleaner; adding e.class helps spot systemic issues.
Apply:
- rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e - puts "Warning: Could not access browser logs: #{e.message}" + rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e + warn "[SeleniumLogger] Could not access browser logs: #{e.class}: #{e.message}"
26-31: Same logging tweak for driver logs block.Align stderr usage and message format.
- rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e - puts "Warning: Could not access driver logs: #{e.message}" + rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e + warn "[SeleniumLogger] Could not access driver logs: #{e.class}: #{e.message}"
16-31: DRY suggestion: extract a helper to fetch and process logs for :browser/:driver.Both blocks are identical aside from the type. A tiny helper reduces duplication and keeps rescue policy consistent.
Example:
+def fetch_log(type, log_only_list, errors) + page.driver.browser.logs.get(type).each do |entry| + log_only_list.include?(entry.level) ? puts(entry.message) : errors << entry.message + end +rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e + warn "[SeleniumLogger] Could not access #{type} logs: #{e.class}: #{e.message}" +end ... - begin - page.driver.browser.logs.get(:browser).each do |entry| - next if entry.message.include?("Download the React DevTools for a better development experience") - log_only_list.include?(entry.level) ? puts(entry.message) : errors << entry.message - end - rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e - warn "Warning: Could not access browser logs: #{e.message}" - end + begin + fetch_log(:browser, log_only_list, errors) + end ... - begin - page.driver.browser.logs.get(:driver).each do |entry| - log_only_list.include?(entry.level) ? puts(entry.message) : errors << entry.message - end - rescue Net::ReadTimeout, Selenium::WebDriver::Error::WebDriverError => e - warn "Warning: Could not access driver logs: #{e.message}" - end + begin + fetch_log(:driver, log_only_list, errors) + end
43-45: Pluralization bug; also avoid ActiveSupport present? dependency here.present? guarantees non-empty, so "#{ 's' unless clean_errors.empty? }" always appends "s". Use size==1 to singularize and Ruby’s any? to avoid AS dependency in support code.
- if clean_errors.present? - raise("JavaScript error#{'s' unless clean_errors.empty?} on the page:\n\n#{clean_errors.join("\n")}") - end + if clean_errors.any? + label = clean_errors.length == 1 ? "JavaScript error" : "JavaScript errors" + raise("#{label} on the page:\n\n#{clean_errors.join("\n")}") + endlib/react_on_rails/git_utils.rb (2)
10-27: Skip shelling out when git_installed is falseMinor perf/clarity: avoid running
git statuswhen we already know Git isn’t installed. Early‑return with the “Git not installed” message.Apply this diff:
def self.uncommitted_changes?(message_handler, git_installed: true) return false if ENV["COVERAGE"] == "true" - status = `git status --porcelain` - return false if git_installed && status&.empty? - - error = if git_installed - <<~MSG.strip - You have uncommitted changes. Please commit or stash them before continuing. - - The React on Rails generator creates many new files and it's important to keep - your existing changes separate from the generated code for easier review. - MSG - else - <<~MSG.strip - Git is not installed. Please install Git and commit your changes before continuing. - - The React on Rails generator creates many new files and version control helps - track what was generated versus your existing code. - MSG - end - message_handler.add_error(error) + unless git_installed + message_handler.add_error(<<~MSG.strip) + Git is not installed. Please install Git and commit your changes before continuing. + + The React on Rails generator creates many new files and version control helps + track what was generated versus your existing code. + MSG + return true + end + + status = `git status --porcelain` + return false if status&.empty? + + message_handler.add_error(<<~MSG.strip) + You have uncommitted changes. Please commit or stash them before continuing. + + The React on Rails generator creates many new files and it's important to keep + your existing changes separate from the generated code for easier review. + MSG true end
3-3: Remove unused require or use it
require "English"isn’t used here (no$CHILD_STATUS,$ERROR_INFO, etc.). Drop it or start using exit status to differentiate errors.spec/react_on_rails/git_utils_spec.rb (1)
36-49: Optional: make message assertions less brittleIf these messages evolve again, consider matching key substrings instead of full blocks (or referencing constants) to reduce churn.
Example:
- expect(message_handler).to receive(:add_error).with(<<~MSG.strip) - Git is not installed. Please install Git and commit your changes before continuing. - ... - MSG + expect(message_handler).to receive(:add_error) + .with(include("Git is not installed.", "version control helps"))spec/react_on_rails/configuration_spec.rb (1)
35-41: Tweak example description for clarity and consistencyUse “raise” (Ruby exceptions) and capitalize “Shakapacker”.
-it "does not throw if the generated assets dir is blank with shakapacker" do +it "does not raise when generated_assets_dir is blank with Shakapacker" doCHANGELOG.md (5)
30-43: Tighten wording and consistency in “Enhanced” bullets.Minor edits for clarity/grammar; no behavior change.
Apply this diff:
- - Improved error messages in install generator with clearer troubleshooting steps - - Enhanced package manager detection with multi-strategy validation - - Ensured that the RSC payload is injected after the component's HTML markup to improve the performance of the RSC payload injection. [PR 1738](https://github.com/shakacode/react_on_rails/pull/1738) by [AbanoubGhadban](https://github.com/AbanoubGhadban). - - Improved RSC rendering flow by eliminating double rendering of server components and reducing the number of HTTP requests. - - Updated communication protocol between Node Renderer and Rails to version 2.0.0 which supports the ability to upload multiple bundles at once. + - Improved install‑generator error messages with clearer troubleshooting steps. + - Enhanced package‑manager detection with multi‑strategy validation. + - Inject RSC payload after the component’s HTML markup to improve performance. [PR 1738](https://github.com/shakacode/react_on_rails/pull/1738) by [AbanoubGhadban](https://github.com/AbanoubGhadban). + - Reduced RSC double‑rendering and HTTP requests in the rendering flow. + - Upgraded Node Renderer ⇄ Rails protocol to 2.0.0, adding support for uploading multiple bundles.
46-49: Polish “Added” bullets (grammar).Tiny grammar tweak.
- - Support for returning React component from async render-function. [PR 1720](https://github.com/shakacode/react_on_rails/pull/1720) by [AbanoubGhadban](https://github.com/AbanoubGhadban). + - Support returning a React component from an async render function. [PR 1720](https://github.com/shakacode/react_on_rails/pull/1720) by [AbanoubGhadban](https://github.com/AbanoubGhadban).
37-37: Call out protocol upgrade with a PR/issue if available.Consider adding a PR reference for the protocol v2.0.0 upgrade for traceability.
Example:
- - Upgraded Node Renderer ⇄ Rails protocol to 2.0.0, adding support for uploading multiple bundles. + - Upgraded Node Renderer ⇄ Rails protocol to 2.0.0, adding support for uploading multiple bundles. [PR ####]
86-87: Format the migration mapping for readability.Break long inline mapping into a bulleted list.
- - Migration: - `config.force_load = true` → `config.immediate_hydration = true` - `react_component(force_load: true)` → `react_component(immediate_hydration: true)` - `redux_store(force_load: true)` → `redux_store(immediate_hydration: true)` + - Migration: + - `config.force_load = true` → `config.immediate_hydration = true` + - `react_component(force_load: true)` → `react_component(immediate_hydration: true)` + - `redux_store(force_load: true)` → `redux_store(immediate_hydration: true)`
1597-1599: Footnotes: add [15.0.0] link or remove brackets in that header.The 15.0.0 header is in bracketed form but lacks a footnote, yielding a dead link. Add a footnote (even if retracted) or un‑bracket the header.
Option A — add footnote:
[Unreleased]: https://github.com/shakacode/react_on_rails/compare/16.0.0...master [16.0.0]: https://github.com/shakacode/react_on_rails/compare/14.2.0...16.0.0 +[15.0.0]: https://github.com/shakacode/react_on_rails/compare/14.2.0...15.0.0Option B — change the header to plain text (no footnote needed):
-### [15.0.0] - 2025-08-28 - RETRACTED +### 15.0.0 - 2025-08-28 - RETRACTEDrakelib/release.rake (1)
58-58: Make command assembly robust (avoid nil interpolation and stray spaces).Interpolating a conditional inside the string works but can yield a trailing space and is brittle. Prefer assembling with Array#compact.join for safety and readability.
- sh_in_dir(gem_root, "gem bump --no-commit #{%(--version #{gem_version}) unless gem_version.strip.empty?}") + cmd = ["gem bump --no-commit", ("--version #{gem_version}" unless gem_version.strip.empty?)].compact.join(" ") + sh_in_dir(gem_root, cmd)lib/generators/react_on_rails/templates/base/base/babel.config.js.tt (1)
14-16: Avoid duplicating @babel/preset-react; replace or extend the existing one.Shakapacker’s default preset may already include
@babel/preset-react. Appending another instance can apply the preset twice with different options. Replace it in-place if present.- resultConfig.presets = [...resultConfig.presets, ...changesOnDefault.presets] + const reactIdx = resultConfig.presets.findIndex( + (p) => Array.isArray(p) && p[0] === '@babel/preset-react' + ) + if (reactIdx >= 0) { + resultConfig.presets[reactIdx] = changesOnDefault.presets[0] + } else { + resultConfig.presets = [...resultConfig.presets, ...changesOnDefault.presets] + }Also double‑check that
useBuiltIns: trueis intended withruntime: 'automatic'; it’s often unnecessary with the automatic runtime.lib/generators/USAGE (1)
3-9: Docs update reads well; minor enhancement.Consider mentioning supported package managers are auto‑detected during install to align with the new dynamic messages in tests.
spec/react_on_rails/locales_to_js_spec.rb (1)
49-56: Stabilize mtime assertions and cover both files.Avoid flakiness from FS timestamp precision and assert default.js is unchanged too.
Apply this diff:
- expect(File.mtime(translations_path)).to eq(ref_time) + expect(File.mtime(translations_path)).to be_within(1).of(ref_time) + expect(File.mtime(default_path)).to be_within(1).of(ref_time)lib/react_on_rails/packer_utils.rb (2)
35-37: Defensive guard for dev_server_url.
dev_server_urlassumes packer present; add a guard to raise a descriptive error when called without Shakapacker to aid debugging.def self.dev_server_url - "#{packer.dev_server.protocol}://#{packer.dev_server.host_with_port}" + raise_shakapacker_not_installed unless using_packer? + "#{packer.dev_server.protocol}://#{packer.dev_server.host_with_port}" end
46-57: Nil‑safety for version helpers.If
shakapackerisn’t available,shakapacker_version_as_arrayandshakapacker_version_requirement_met?can raise. Make them resilient.def self.shakapacker_version_as_array return @shakapacker_version_as_array if defined?(@shakapacker_version_as_array) - - match = shakapacker_version.match(ReactOnRails::VersionChecker::VERSION_PARTS_REGEX) + return [] unless (ver = shakapacker_version) + match = ver.match(ReactOnRails::VersionChecker::VERSION_PARTS_REGEX) @@ def self.shakapacker_version_requirement_met?(required_version) - Gem::Version.new(shakapacker_version) >= Gem::Version.new(required_version) + ver = shakapacker_version + return false unless ver + Gem::Version.new(ver) >= Gem::Version.new(required_version) endspec/react_on_rails/utils_spec.rb (2)
4-4: Prefer explicit require for test stability.Since Webpacker paths are removed,
require "shakapacker"is simpler and avoidsnilwhenpacker_typeis mocked.-require ReactOnRails::PackerUtils.packer_type +require "shakapacker"
53-61: Simplify Manifest constant access via packer constant.Avoid dynamic const name building; use the packer module directly for clarity.
- mock_manifest = instance_double(Object.const_get(ReactOnRails::PackerUtils.packer_type.capitalize)::Manifest) + packer_const = ReactOnRails::PackerUtils.packer + mock_manifest = instance_double(packer_const::Manifest) @@ - allow(ReactOnRails::PackerUtils.packer).to receive(:manifest).and_return(mock_manifest) + allow(packer_const).to receive(:manifest).and_return(mock_manifest)And:
- .and_raise(Object.const_get( - ReactOnRails::PackerUtils.packer_type.capitalize - )::Manifest::MissingEntryError) + packer_const = ReactOnRails::PackerUtils.packer + .and_raise(packer_const::Manifest::MissingEntryError)lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (1)
23-29: Docs wording nit + clarity.Consider “what Node package manager command is run” or “npm/yarn/pnpm command” for clarity since detection is dynamic.
script/convert (2)
35-39: React pinning: consider semver‑compatible range.Hard‑pinning to 18.0.0 may block security/bugfix upgrades. Prefer a caret range unless strict pinning is required by tests.
-gsub_file_content("../package.json", /"react": "[^"]*",/, '"react": "18.0.0",') -gsub_file_content("../package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",') +gsub_file_content("../package.json", /"react": "[^"]*",/, '"react": "^18.0.0",') +gsub_file_content("../package.json", /"react-dom": "[^"]*",/, '"react-dom": "^18.0.0",')Please confirm CI supports the wider range.
20-23: Regex‑based package edits: safer to parse JSON.Regex can over‑match in comments/peerDeps. Parsing ensures surgical updates.
I can provide a Ruby script using JSON.parse to update only the intended keys if you want to pursue this.
Also applies to: 35-39, 56-61
.rubocop.yml (2)
81-84: ClassLength exclusions: OK; add TODO to revisit.Helps land v16; plan a follow‑up to decompose.
149-157: YAML blank line lint.Fix extra blank lines to satisfy yamllint.
Apply this diff (remove the extra empties around these sections):
- RSpec/InstanceVariable: Exclude: - 'spec/react_on_rails/dev/**/*_spec.rb' # Dev module tests require global variable management - RSpec/StubbedMock: Exclude: - 'spec/react_on_rails/dev/**/*_spec.rb' # Dev module tests use mixed stub/mock patterns -spec/react_on_rails/support/shared_examples/react_with_redux_generator_examples.rb (1)
18-25: Consider asserting legacy paths are absent.Add negative checks to ensure old bundles/paths aren’t generated.
it "copies base redux files" do @@ app/javascript/src/HelloWorldApp/ror_components/HelloWorldApp.server.jsx].each { |file| assert_file(file) } + # Legacy paths should not exist + assert_no_file("app/javascript/bundles/HelloWorld/startup/HelloWorldApp.jsx") + assert_no_file("app/javascript/packs/hello-world-bundle.js") endlib/generators/react_on_rails/generator_helper.rb (2)
4-4: Avoid cross-file implicit dependency for JSON; require it where used.Requiring "json" here couples consumers to this helper. Prefer requiring JSON in files that actually use it (e.g., DevTestsGenerator), so helpers remain side‑effect free.
Apply this diff to drop the implicit require:
-require "json"
10-20: Usewarn(stderr) for warnings and keep messages stable for tests.Generator output often appears in test expectations;
putswrites to stdout and may pollute generator output. Usewarnand unify wording.Apply this diff:
- puts "Warning: package_json gem not available. This is expected before Shakapacker installation." - puts "Dependencies will be installed using the default package manager after Shakapacker setup." + warn "Warning: package_json gem not available before Shakapacker installation." + warn "Will install dependencies using the detected package manager after Shakapacker setup." @@ - puts "Warning: Could not read package.json: #{e.message}" - puts "This is normal before Shakapacker creates the package.json file." + warn "Warning: Could not read package.json: #{e.message}" + warn "This can be normal before Shakapacker creates package.json."lib/generators/react_on_rails/dev_tests_generator.rb (1)
10-10: No-op formatting change is fine; also addrequire "json"locally.This class calls JSON.parse (Lines 50–55) but doesn’t require JSON; it currently relies on generator_helper’s side effect. Add an explicit require here.
Apply this diff at the top of the file:
require "rails/generators" +require "json" require_relative "generator_helper"lib/react_on_rails/dev/file_manager.rb (3)
29-49: Handle empty/whitespace PID content explicitly.An empty server.pid raises ArgumentError and is handled, but a quick guard improves clarity and avoids exception-driven flow.
- pid_content = File.read(server_pid_file).strip + pid_content = File.read(server_pid_file).strip + if pid_content.empty? + remove_file_if_exists(server_pid_file, "stale Rails pid file") + return true + end
51-53: Tighten Overmind detection to avoid false positives.Use
pgrep -xto match the exact process name rather than any command line containing “overmind”.- !`pgrep -f "overmind" 2>/dev/null`.split("\n").empty? + !`pgrep -x overmind 2>/dev/null`.split("\n").empty?
66-74: Prefer FileUtils.rm_f and log failures.
File.deleteraises for EISDIR/EPERM, and racing deletes flip success/failure.FileUtils.rm_fis idempotent; also consider logging when deletion fails.+ require "fileutils" return false unless File.exist?(file_path) puts " 🧹 Cleaning up #{description}: #{file_path}" - File.delete(file_path) + FileUtils.rm_f(file_path) true - rescue StandardError - false + rescue StandardError => e + warn "Cleanup failed for #{file_path}: #{e.class}: #{e.message}" + falselib/react_on_rails/dev/process_manager.rb (3)
14-22:ensure_procfileis unused; either use it or drop it.
run_with_process_managerhandles validation; keep a single code path to avoid drift.Option A: remove
ensure_procfile.
Option B: call it fromrun_with_process_managerafter path validation.
24-45: Propagate subprocess exit status and surface failures.
systemreturn value isn’t checked; callers won’t know if startup failed. Capture status and exit accordingly.if installed?("overmind") - system("overmind", "start", "-f", procfile) + ok = system("overmind", "start", "-f", procfile) elsif installed?("foreman") - system("foreman", "start", "-f", procfile) + ok = system("foreman", "start", "-f", procfile) else @@ - exit 1 + exit 1 end + unless ok + warn "ERROR: Process manager failed to start with Procfile #{procfile}" + exit($?.exitstatus || 1) + end
49-57: Path validation: also reject newlines and null bytes.Guard against odd paths that may confuse tools.
- return false if procfile.match?(/[;&|`$(){}\[\]<>]/) + return false if procfile.match?(/[;&|`$(){}\[\]<>\\n\\0]/)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml (2)
103-107: allowed_hosts: 'auto' is convenient; ensure reverse-proxy cases are covered.If teams commonly use custom hostnames (e.g., *.local.test), consider documenting how to override to an array for explicit hosts.
58-67: Consider enabling SRI in production templates.Keeping
integrity.enabled: falseby default is safe, but many apps benefit from SRI in production. Consider toggling it on in production block as a recommended starting point.rakelib/run_rspec.rake (2)
40-47: Verify task naming consistency (rspec_task_namevs_short).Console log says it’s creating
example_type.rspec_task_name, but you definerspec_task_name_short. Ensure both the printed name and invoked task (Line 51) align to avoid confusion/missed tasks.
108-116: Minor: avoid leading space when no env vars and allow arrays for args.Harmless, but you can build the command more robustly.
- env_vars = env_tokens.join(" ") - sh_in_dir(path.realpath, "#{env_vars} bundle exec rspec #{rspec_args}") + env_vars = env_tokens.join(" ") + cmd = ["bundle exec rspec", rspec_args].reject(&:empty?).join(" ") + sh_in_dir(path.realpath, [env_vars, cmd].reject(&:empty?).join(" "))lib/generators/react_on_rails/generator_messages.rb (2)
29-31: Rainbow color ‘orange’ may not exist; prefer ‘yellow’.Some environments don’t expose
Rainbow#orange. Useyellowfor portability.- Rainbow("WARNING: #{msg}").orange + Rainbow("WARNING: #{msg}").yellow
124-131: Consider stubbing detection to avoid shell calls in tests.
system("which ...")is fine at runtime; ensure specs stubdetect_process_manager(notsystem) to keep tests hermetic..github/workflows/main.yml (3)
39-41: Run apt-get update before install to avoid 404s.Ubuntu runners can fail without an update.
- - name: Fix dependency for libyaml-dev - run: sudo apt install libyaml-dev + - name: Fix dependency for libyaml-dev + run: | + sudo apt-get update + sudo apt-get install -y libyaml-dev
153-157: Use restore action for cache retrieval (v4 best practice).This step restores, not saves.
- - name: Save test Webpack bundles to cache (for build number checksum used by RSpec job) - uses: actions/cache@v4 + - name: Restore test Webpack bundles cache (for build number checksum used by RSpec job) + uses: actions/cache/restore@v4
60-63: Yarn global with sudo can bypass PATH.Consider non‑sudo global install or
corepack enableto ensureyalcis on PATH consistently.Also applies to: 68-69, 158-161, 166-167
lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assets (1)
1-2: Bind to 0.0.0.0 for container/devproxy friendliness.Helps when running inside Docker/WSL or accessing from other devices; harmless elsewhere.
-web: bin/rails server -p 3000 +web: bin/rails server -p 3000 -b 0.0.0.0lib/react_on_rails/engine.rb (1)
12-16: Load rake tasks via glob to prevent packaging drift.Keeps this resilient if tasks are added/renamed; avoids load errors when a single file is missing.
- rake_tasks do - load File.expand_path("../tasks/ggenerate_packs.rake", __dir__) - load File.expand_path("../tasks/assets.rake", __dir__) - load File.expand_path("../tasks/locale.rake", __dir__) - end + rake_tasks do + Dir[File.expand_path("../tasks/*.rake", __dir__)].sort.each { |f| load f } + endlib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assets (1)
5-5: Use ‘web’ for consistency and bind to 0.0.0.0.Aligns with other Procfiles and Foreman conventions; improves DX in containers.
-rails: bundle exec rails s -p 3001 +web: bundle exec rails s -p 3001 -b 0.0.0.0If
ServerManager/tests expect the process name to berails, keep as-is; otherwise preferweb. Please confirm expectations in specs.lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.module.css (1)
1-4: Consider WCAG-friendly color and theming hook.Template green may fail contrast in some UIs; consider using a CSS variable with a default.
-.bright { - color: green; +.bright { + color: var(--helloWorld-accent, #0a7d2b); font-weight: bold; }lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.server.jsx (1)
1-5: One‑liner re‑export for brevityYou can re-export directly to reduce boilerplate.
Apply this diff:
-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; +// Specialized server entry; customize here if needed for SSR. +export { default } from './HelloWorld.client';lib/react_on_rails/dev.rb (1)
3-6: Avoid eagerly loading Dev utilities; use autoloadSwitching to autoload reduces load time and memory, especially in non-dev contexts.
Apply this diff:
-require_relative "dev/server_manager" -require_relative "dev/process_manager" -require_relative "dev/pack_generator" -require_relative "dev/file_manager"And add these inside the Dev module block:
module ReactOnRails module Dev + autoload :ServerManager, "react_on_rails/dev/server_manager" + autoload :ProcessManager, "react_on_rails/dev/process_manager" + autoload :PackGenerator, "react_on_rails/dev/pack_generator" + autoload :FileManager, "react_on_rails/dev/file_manager"lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx (1)
4-6: Provide a safe default for nameAvoid undefined rendering when props.name is missing.
Apply this diff:
-const HelloWorld = (props) => { - const [name, setName] = useState(props.name); +const HelloWorld = (props) => { + const [name, setName] = useState(props?.name ?? "Stranger");lib/react_on_rails/dev/pack_generator.rb (2)
14-17: Use File::NULL for cross‑platform redirection and DRY the commandThis avoids /dev/null portability issues and centralizes the command string.
Apply this diff:
- else - print "📦 Generating packs... " - success = system "bundle exec rake react_on_rails:generate_packs > /dev/null 2>&1" - puts success ? "✅" : "❌" + else + print "📦 Generating packs... " + cmd = "bundle exec rake react_on_rails:generate_packs" + null = File::NULL + success = system("#{cmd} > #{null} 2>&1") + puts success ? "✅" : "❌"
3-3: Remove unused require "English"Not used; trimming it avoids unnecessary requires.
Apply this diff:
-require "English"lib/generators/react_on_rails/templates/base/base/bin/dev (2)
21-28: Split rescue paths and send fallback notice to STDERR.Rescuing both requires together makes it impossible to fall back only when
react_on_rails/devis missing. Also preferwarnoverputsfor diagnostics.Apply this diff:
-begin - require "bundler/setup" - require "react_on_rails/dev" -rescue LoadError - # Fallback for when gem is not yet installed - puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" -end +begin + require "bundler/setup" +rescue LoadError + # Bundler not present; proceed and try to load the library next. +end + +begin + require "react_on_rails/dev" +rescue LoadError + # Fallback for when gem is not yet installed + warn "Loading ReactOnRails development tools..." + require_relative "../../lib/react_on_rails/dev" +end
42-45: Print unknown-arg errors to STDERR.Use
warnfor error output; keep guidance on STDOUT as needed.- 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"spec/react_on_rails/binstubs/dev_spec.rb (5)
8-19: Avoid leaking file handles when silencing IO.Close the redirected streams and restore originals.
- original_stderr = $stderr - original_stdout = $stdout - before(:all) do - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") - end + original_stderr = $stderr + original_stdout = $stdout + before(:all) do + @redirected_stderr = File.open(File::NULL, "w") + @redirected_stdout = File.open(File::NULL, "w") + $stderr = @redirected_stderr + $stdout = @redirected_stdout + end @@ - after(:all) do - $stderr = original_stderr - $stdout = original_stdout - end + after(:all) do + $stderr = original_stderr + $stdout = original_stdout + @redirected_stderr&.close + @redirected_stdout&.close + end
21-27: Prefer stubbing Kernel directly over any_instance_of.This avoids RSpec/AnyInstance and is cleaner.
def setup_script_execution # Mock ARGV to simulate no arguments (default HMR mode) stub_const("ARGV", []) # Mock pack generation and allow other system calls - allow_any_instance_of(Kernel).to receive(:system).and_return(true) + allow(Kernel).to receive(:system).and_return(true) end
28-35: Same here for exit stub.def setup_script_execution_for_tool_tests setup_script_execution # For tool selection tests, we don't care about file existence - just tool logic allow(File).to receive(:exist?).with("Procfile.dev").and_return(true) # Mock exit to prevent test termination - allow_any_instance_of(Kernel).to receive(:exit) + allow(Kernel).to receive(:exit) end
64-75: Stub Kernel.require and wrap load for isolation.
requireis onKernel; stubbing any instance is brittle. Wrappingloadreduces global side effects.- # Mock the require to succeed - allow_any_instance_of(Kernel).to receive(:require).with("bundler/setup").and_return(true) - allow_any_instance_of(Kernel).to receive(:require).with("react_on_rails/dev").and_return(true) + # Mock the require to succeed + allow(Kernel).to receive(:require).with("bundler/setup").and_return(true) + allow(Kernel).to receive(:require).with("react_on_rails/dev").and_return(true) @@ - load script_path + load script_path, true
36-41: Specs read script content for command support — consider adding a negative-path test.Add a case asserting unknown args print an error and exit(1).
Would you like me to add a small spec for
bin/dev fooexpectingKernel.exit(1)and an error message?Also applies to: 43-47, 48-51, 53-58, 59-63
spec/react_on_rails/dev/process_manager_spec.rb (6)
8-18: Close redirected IO to prevent leaks.Mirror the fix suggested in binstubs spec.
before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") + @original_stderr = $stderr + @original_stdout = $stdout + @redirected_stderr = File.open(File::NULL, "w") + @redirected_stdout = File.open(File::NULL, "w") + $stderr = @redirected_stderr + $stdout = @redirected_stdout end @@ after(:all) do $stderr = @original_stderr $stdout = @original_stdout + @redirected_stderr&.close + @redirected_stdout&.close end
45-51: Avoid any_instance_of for Kernel.system.Cleaner and RuboCop-friendly.
- allow_any_instance_of(Kernel).to receive(:system).and_return(true) + allow(Kernel).to receive(:system).and_return(true)
52-57: Use Kernel.system directly in expectations.- expect_any_instance_of(Kernel).to receive(:system).with("overmind", "start", "-f", "Procfile.dev") + expect(Kernel).to receive(:system).with("overmind", "start", "-f", "Procfile.dev")
59-65: Same for foreman branch.- expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") + expect(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev")
67-73: Prefer Kernel.exit over any_instance_of.- expect_any_instance_of(Kernel).to receive(:exit).with(1) + expect(Kernel).to receive(:exit).with(1)
45-51: Add a negative-path test for invalid procfile path.Exercise the
valid_procfile_path?guard (e.g.,"; rm -rf /"), expecting an error andexit(1).Want me to add a spec like:
it "exits on invalid procfile path" do allow(File).to receive(:readable?).and_return(true) expect(Kernel).to receive(:exit).with(1) described_class.run_with_process_manager("Procfile.dev; rm -rf /") endspec/react_on_rails/dev/server_manager_spec.rb (4)
9-19: Close redirected IO to avoid leaks.Same pattern as other specs.
before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") + @original_stderr = $stderr + @original_stdout = $stdout + @redirected_stderr = File.open(File::NULL, "w") + @redirected_stdout = File.open(File::NULL, "w") + $stderr = @redirected_stderr + $stdout = @redirected_stdout end @@ after(:all) do $stderr = @original_stderr $stdout = @original_stdout + @redirected_stderr&.close + @redirected_stdout&.close end
21-27: Avoid any_instance_of for Kernel.system/exit.Tighten stubs to module methods; appeases RuboCop RSpec/AnyInstance.
def mock_system_calls allow(ReactOnRails::Dev::PackGenerator).to receive(:generate).with(any_args) - allow_any_instance_of(Kernel).to receive(:system).and_return(true) - allow_any_instance_of(Kernel).to receive(:exit) + allow(Kernel).to receive(:system).and_return(true) + allow(Kernel).to receive(:exit) allow(ReactOnRails::Dev::ProcessManager).to receive(:ensure_procfile) allow(ReactOnRails::Dev::ProcessManager).to receive(:run_with_process_manager) end
56-63: Reduce brittleness of the production-like command assertion.String-equality on a long shell command is fragile. Match key parts instead.
- command = "RAILS_ENV=production NODE_ENV=production bundle exec rails assets:precompile" - expect_any_instance_of(Kernel).to receive(:system).with(command).and_return(true) + expect(Kernel).to receive(:system) + .with(a_string_including("RAILS_ENV=production", "NODE_ENV=production", "rails assets:precompile")) + .and_return(true)
70-75: Remove unused backtick stub.
kill_processesusesOpen3.capture2, not backticks.- allow_any_instance_of(Kernel).to receive(:`).and_return("")lib/generators/react_on_rails/templates/base/base/config/webpack/development.js.tt (2)
3-3: Remove unused shakapacker import.
devServerandinliningCssare no longer referenced; keeping this line triggers lint warnings in generated apps.Apply this diff:
-const { devServer, inliningCss } = require('shakapacker'); +// shakapacker not required directly here; generateWebpackConfigs handles it.
13-18: Guard against missing/duplicate plugin entries when adding React Refresh.Ensure
pluginsexists and avoid pushing duplicate ReactRefresh plugin instances.Apply this diff:
- clientWebpackConfig.plugins.push( - new ReactRefreshWebpackPlugin({ - // Use default overlay configuration for better compatibility - }), - ); + clientWebpackConfig.plugins = clientWebpackConfig.plugins || []; + const hasRefresh = clientWebpackConfig.plugins.some( + (p) => p && p.constructor && p.constructor.name === 'ReactRefreshWebpackPlugin' + ); + if (!hasRefresh) { + clientWebpackConfig.plugins.push( + new ReactRefreshWebpackPlugin({ + // Use default overlay configuration for better compatibility + }), + ); + }.github/workflows/examples.yml (2)
28-28: Avoid setting BUNDLE_FROZEN globally; pass --frozen only when needed.
BUNDLE_FROZENas the string "false" can still be misinterpreted; prefer explicit CLI flags.Apply this diff to remove the env and gate
--frozenon install:- BUNDLE_FROZEN: ${{ matrix.dependency-level == 'minimum' && 'false' || 'true' }}And update the install step:
- bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 + bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 ${{ matrix.dependency-level == 'latest' && '--frozen' || '' }}If you want, I can adjust other workflows similarly for consistency.
31-34: Ensure the base commit is fetchable by enabling full history.
git fetch origin $BASE_SHAcan fail with shallow clones; enable full fetch.Apply this diff:
- uses: actions/checkout@v4 with: persist-credentials: false + fetch-depth: 0spec/react_on_rails/generators/install_generator_spec.rb (1)
53-60: Remove unused lets to avoid dead code.
expected_non_reduxandexpected_reduxare never referenced.Apply this diff:
- let(:expected_non_redux) do - GeneratorMessages.format_info(GeneratorMessages.helpful_message_after_installation) - end - - let(:expected_redux) do - GeneratorMessages.format_info(GeneratorMessages.helpful_message_after_installation(component_name: "HelloWorldApp")) - endlib/generators/react_on_rails/bin/dev (2)
21-28: Fail fast with a helpful message if the dev tools aren’t available.The current fallback
require_relative "../../lib/react_on_rails/dev"will generally not exist in apps; emit guidance instead.Apply this diff:
begin require "bundler/setup" require "react_on_rails/dev" rescue LoadError - # Fallback for when gem is not yet installed - puts "Loading ReactOnRails development tools..." - require_relative "../../lib/react_on_rails/dev" + warn "ReactOnRails dev tools not found. Please run 'bundle install' first." + exit 1 end
32-35: Be explicit about the Procfile used for prod-like mode.Comment says prod uses
Procfile.dev-prod-assets; pass it explicitly for consistency with other branches.Apply this diff:
-when "production-assets", "prod" - ReactOnRails::Dev::ServerManager.start(:production_like) +when "production-assets", "prod" + ReactOnRails::Dev::ServerManager.start(:production_like, "Procfile.dev-prod-assets").github/workflows/rspec-package-specs.yml (1)
24-28: Optional: use setup-ruby’s built-in bundler cache.You can drop the manual cache and simplify with
bundler-cache: true.- - name: Setup Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: ${{ matrix.ruby-version }} - bundler: 2.5.9 + - name: Setup Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: ${{ matrix.ruby-version }} + bundler: 2.5.9 + bundler-cache: trueIf you adopt this, remove the manual “Save root ruby gems to cache” step.
lib/react_on_rails/dev/server_manager.rb (3)
69-75: Optionally escalate TERM→KILL for stubborn PIDs.Some PIDs won’t die on TERM; escalate after a short grace period.
def terminate_processes(pids) pids.each do |pid| - Process.kill("TERM", pid) - rescue StandardError - nil + begin + Process.kill("TERM", pid) + sleep 0.5 + Process.kill(0, pid) # still alive? + Process.kill("KILL", pid) + rescue Errno::ESRCH + # already gone + rescue StandardError + # ignore + end end end
287-291: Guard against negative padding in box rendering.Long lines can make
paddingnegative and raise. Clamp at zero.- padding = box_width - line.length - 2 - line + "#{' ' * padding}│" + padding = [box_width - line.length - 2, 0].max + line + (' ' * padding) + '│'
241-246: Parity: print feature summary for HMR mode too.Static/prod modes show a features box; HMR mode doesn’t. Consider adding similar messaging for consistency.
lib/generators/react_on_rails/base_generator.rb (2)
134-148: .gitignore pattern scope.Ignoring
**/generated/**is broad and may hide unrelated generated dirs. Consider scoping to the known path used by auto‑registration (e.g.,app/javascript/**/generated/**).
273-293: Non-interactive runs: default choice?
yes?prompts can hang in CI/non‑TTY contexts. Consider defaulting to “skip” when$stdin.tty?is false, or accept a--force-webpack-replaceoption.lib/generators/react_on_rails/install_generator.rb (1)
271-294: Windows support for package-manager detection.Using
whichfails on Windows. Reuse your OS check andwherethere.- def cli_exists?(command) - system("which #{command} > /dev/null 2>&1") - end + def cli_exists?(command) + locator = ReactOnRails::Utils.running_on_windows? ? "where" : "which" + system("#{locator} #{command} > /dev/null 2>&1") + endAlso applies to: 185-188
| - name: Git Stuff | ||
| if: matrix.versions == 'oldest' | ||
| if: matrix.dependency-level == 'minimum' | ||
| run: | | ||
| git config user.email "you@example.com" | ||
| git config user.name "Your Name" | ||
| git commit -am "stop generators from complaining about uncommitted code" | ||
| - run: cd spec/dummy && bundle info shakapacker |
There was a problem hiding this comment.
Prevent empty-commit failure.
git commit -am exits non‑zero when no changes; guard it.
- git commit -am "stop generators from complaining about uncommitted code"
+ git diff --quiet || git commit -am "stop generators from complaining about uncommitted code"📝 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.
| - name: Git Stuff | |
| if: matrix.versions == 'oldest' | |
| if: matrix.dependency-level == 'minimum' | |
| run: | | |
| git config user.email "you@example.com" | |
| git config user.name "Your Name" | |
| git commit -am "stop generators from complaining about uncommitted code" | |
| - run: cd spec/dummy && bundle info shakapacker | |
| - name: Git Stuff | |
| if: matrix.dependency-level == 'minimum' | |
| run: | | |
| git config user.email "you@example.com" | |
| git config user.name "Your Name" | |
| git diff --quiet || git commit -am "stop generators from complaining about uncommitted code" | |
| - run: cd spec/dummy && bundle info shakapacker |
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 200 to 206: the workflow unconditionally
runs `git commit -am ...` which fails when there are no staged changes; change
the step to first check for changes (for example, run `git status --porcelain`
or `git diff --quiet` and only run the commit when output/exit indicates
changes) or make the commit step tolerant (e.g., append `|| true`) so the job
does not fail on an empty commit attempt.
| - name: Install Node modules with Yarn for renderer package | ||
| run: | | ||
| yarn install --no-progress --no-emoji ${{ matrix.versions == 'newest' && '--frozen-lockfile' || '' }} | ||
| yarn install --no-progress --no-emoji ${{ matrix.node-version == '22' && '--frozen-lockfile' || '' }} | ||
| sudo yarn global add yalc |
There was a problem hiding this comment.
Yarn flag may be incompatible on Node 22 without pinning Yarn 1
--frozen-lockfile is Yarn 1; on Node 22, Corepack may activate Yarn 4+, where --immutable is expected. Pin Yarn 1 or switch flags conditionally.
Apply this diff to pin Yarn 1 consistently (then you can keep --frozen-lockfile):
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
cache: yarn
cache-dependency-path: '**/yarn.lock'
+ - name: Ensure Yarn v1 via Corepack
+ run: |
+ corepack enable
+ corepack prepare yarn@1.22.22 --activate
+ shell: bashOptional follow-up (if you prefer modern Yarn instead): use --immutable for Yarn ≥2 and drop pinning.
📝 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.
| - name: Install Node modules with Yarn for renderer package | |
| run: | | |
| yarn install --no-progress --no-emoji ${{ matrix.versions == 'newest' && '--frozen-lockfile' || '' }} | |
| yarn install --no-progress --no-emoji ${{ matrix.node-version == '22' && '--frozen-lockfile' || '' }} | |
| sudo yarn global add yalc | |
| - name: Setup Node | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: ${{ matrix.node-version }} | |
| cache: yarn | |
| cache-dependency-path: '**/yarn.lock' | |
| - name: Ensure Yarn v1 via Corepack | |
| run: | | |
| corepack enable | |
| corepack prepare yarn@1.22.22 --activate | |
| shell: bash | |
| - name: Install Node modules with Yarn for renderer package | |
| run: | | |
| yarn install --no-progress --no-emoji ${{ matrix.node-version == '22' && '--frozen-lockfile' || '' }} | |
| sudo yarn global add yalc |
🤖 Prompt for AI Agents
In .github/workflows/package-js-tests.yml around lines 35 to 38, the workflow
uses the Yarn 1 flag --frozen-lockfile but on Node 22 Corepack may activate Yarn
4+, causing a flag mismatch; pin Yarn 1 before running yarn install so
--frozen-lockfile is valid (for example, run a command prior to yarn install to
activate Yarn 1 like corepack prepare yarn@1.22.19 --activate or npm install -g
yarn@1), then keep the conditional --frozen-lockfile flag as-is; alternatively,
implement a conditional branch that uses --immutable for Yarn ≥2 and
--frozen-lockfile for Yarn 1 if you prefer not to pin.
| - name: Git Stuff | ||
| if: matrix.versions == 'oldest' | ||
| if: matrix.dependency-level == 'minimum' | ||
| run: | | ||
| git config user.email "you@example.com" | ||
| git config user.name "Your Name" | ||
| git commit -am "stop generators from complaining about uncommitted code" | ||
| - name: Set packer version environment variable | ||
| - name: Set dependency level environment variable |
There was a problem hiding this comment.
Make “Git Stuff” resilient when there’s nothing to commit.
git commit -am ... fails and breaks the job if there are no changes. Guard or ignore failures.
Apply:
- - name: Git Stuff
+ - name: Git Stuff
if: matrix.dependency-level == 'minimum'
run: |
git config user.email "you@example.com"
git config user.name "Your Name"
- git commit -am "stop generators from complaining about uncommitted code"
+ # Commit only if there are changes
+ if ! git diff --quiet; then
+ git commit -am "stop generators from complaining about uncommitted code"
+ fi📝 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.
| - name: Git Stuff | |
| if: matrix.versions == 'oldest' | |
| if: matrix.dependency-level == 'minimum' | |
| run: | | |
| git config user.email "you@example.com" | |
| git config user.name "Your Name" | |
| git commit -am "stop generators from complaining about uncommitted code" | |
| - name: Set packer version environment variable | |
| - name: Set dependency level environment variable | |
| - name: Git Stuff | |
| if: matrix.dependency-level == 'minimum' | |
| run: | | |
| git config user.email "you@example.com" | |
| git config user.name "Your Name" | |
| # Commit only if there are changes | |
| if ! git diff --quiet; then | |
| git commit -am "stop generators from complaining about uncommitted code" | |
| fi | |
| - name: Set dependency level environment variable |
🤖 Prompt for AI Agents
.github/workflows/rspec-package-specs.yml around lines 47 to 53: the "Git Stuff"
step runs git commit -am which fails the job when there are no staged changes;
update the step to first check for changes (e.g., run git diff --cached --quiet
or git status --porcelain and only run git commit if changes exist) or append a
safe fallback (e.g., || true) so the workflow does not fail when there is
nothing to commit.
| - Generator now requires at least one JavaScript package manager (npm, pnpm, yarn, or bun) | ||
| - Generator uses `Thor::Error` exceptions instead of `exit(1)` for better error handling | ||
| - Migration: Ensure you have a JavaScript package manager installed before running the generator |
There was a problem hiding this comment.
🧩 Analysis chain
Install generator: ensure Thor::Error replaces exit(1) and JS PM detection added.
Validate generator behavior matches the changelog.
🏁 Script executed:
#!/bin/bash
# No direct `exit(1)` in generators; Thor::Error used instead
rg -n -g 'lib/**' -e 'exit\s*\(\s*1\s*\)'
rg -n -g 'lib/**' -e 'Thor::Error'
# JS package manager detection (npm|pnpm|yarn|bun) invoked in generator
rg -n -g 'lib/**' -e 'node --version|npm --version|pnpm --version|yarn --version|bun --version'Length of output: 464
Enforce JS package manager requirement or update CHANGELOG
- Thor::Error is already used (lib/generators/react_on_rails/install_generator.rb:242, 266) — OK.
- The generator only checks Node (
node --versionat lib/generators/react_on_rails/install_generator.rb:100) and does not detect npm|pnpm|yarn|bun as the CHANGELOG claims (see migration note around line 258). Either add detection for any of npm/pnpm/yarn/bun and raise Thor::Error when none are present, or remove/adjust the package-manager requirement from CHANGELOG.
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 69–71, the changelog claims the generator requires
a JS package manager but the generator only checks Node
(lib/generators/react_on_rails/install_generator.rb around line 100) and does
not detect npm|pnpm|yarn|bun; implement detection in the generator or update the
changelog. Update lib/generators/react_on_rails/install_generator.rb to try
detecting any of npm, pnpm, yarn, or bun (e.g., run each tool's version/which
command or equivalent) and raise a Thor::Error with a clear message if none are
found; also ensure the migration note near line 258 reflects the actual behavior
(either note the requirement or remove the package-manager requirement from
CHANGELOG.md if you choose not to add detection).
| def add_js_dependencies | ||
| major_minor_patch_only = /\A\d+\.\d+\.\d+\z/ | ||
| if ReactOnRails::VERSION.match?(major_minor_patch_only) | ||
| package_json.manager.add(["react-on-rails@#{ReactOnRails::VERSION}"]) | ||
| else | ||
| # otherwise add latest | ||
| puts "Adding the latest react-on-rails NPM module. Double check this is correct in package.json" | ||
| package_json.manager.add(["react-on-rails"]) | ||
| add_react_on_rails_package | ||
| add_react_dependencies | ||
| add_css_dependencies | ||
| add_dev_dependencies | ||
| end | ||
|
|
||
| def install_js_dependencies | ||
| # Detect which package manager to use |
There was a problem hiding this comment.
Packages may be written to package.json but not installed.
When add_npm_dependencies succeeds, it only updates package.json; no install occurs, and the app may lack node modules. Ensure an install runs once after dependencies are added.
def add_js_dependencies
- add_react_on_rails_package
- add_react_dependencies
- add_css_dependencies
- add_dev_dependencies
+ add_react_on_rails_package
+ add_react_dependencies
+ add_css_dependencies
+ add_dev_dependencies
+ # Ensure node_modules are actually installed when package_json path succeeds
+ install_js_dependencies
endIf you prefer, gate install_js_dependencies behind a flag that tracks whether any add_npm_dependencies returned true.
Also applies to: 105-132, 160-178, 180-220, 196-208
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/base_generator.rb around lines 98-106 (and
similarly 105-132, 160-178, 180-220, 196-208), add logic so that when
add_npm_dependencies/add_react_on_rails_package/add_react_dependencies/add_css_dependencies/add_dev_dependencies
modify package.json they set a boolean flag (e.g., npm_deps_changed = true);
then ensure install_js_dependencies is invoked only if that flag is true (or
always run a package-manager install after adds) so package.json changes are
actually installed into node_modules; update the relevant methods to return a
truthy value when they change package.json and gate the install call on that
return value (or accept a CLI flag to force-install).
| require "react_on_rails/locales/base" | ||
| require "react_on_rails/locales/to_js" | ||
| require "react_on_rails/locales/to_json" | ||
| require "react_on_rails/dev" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Guard dev-only require to avoid prod bloat/LoadError.
Unconditional require "react_on_rails/dev" may pull in dev-only deps in production. Load only in dev/test.
-require "react_on_rails/dev"
+if defined?(Rails) && (Rails.env.development? || Rails.env.test?)
+ require "react_on_rails/dev"
+endPlease confirm no other files implicitly reference ReactOnRails::Dev in production code paths:
🏁 Script executed:
#!/bin/bash
# Verify dev code isn't referenced in non-test, non-spec files
rg -nP --type rb -g '!spec/**' -g '!test/**' '\bReactOnRails::Dev\b|require\s*[\'"]react_on_rails/dev[\'"]' -C2Length of output: 68
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for literal 'react_on_rails/dev' in Ruby files (excluding spec/ and test/)..."
rg -n --type rb -g '!spec/**' -g '!test/**' 'react_on_rails/dev' -C2 || true
echo
echo "Searching for 'ReactOnRails::Dev' constant usages (excluding spec/ and test/)..."
rg -n --type rb -g '!spec/**' -g '!test/**' 'ReactOnRails::Dev' -C2 || true
echo
echo "Showing lib/react_on_rails.rb (lines 1..200):"
sed -n '1,200p' lib/react_on_rails.rb || trueLength of output: 1365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for 'react_on_rails/dev' (exclude spec/ and test/)..."
if command -v rg >/dev/null 2>&1; then
rg -n -S -g '!spec/**' -g '!test/**' 'react_on_rails/dev' -C2 || true
else
git grep -n --untracked --no-color -I -e 'react_on_rails/dev' -- ':!spec' ':!test' || true
fi
echo
echo "Searching for 'ReactOnRails::Dev' (exclude spec/ and test/)..."
if command -v rg >/dev/null 2>&1; then
rg -n -S -g '!spec/**' -g '!test/**' 'ReactOnRails::Dev' -C2 || true
else
git grep -n --untracked --no-color -I -e 'ReactOnRails::Dev' -- ':!spec' ':!test' || true
fi
echo
echo "Show lib/react_on_rails.rb (first 250 lines):"
sed -n '1,250p' lib/react_on_rails.rb || trueLength of output: 1567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Searching for 'react_on_rails/dev' in Ruby files (excluding spec/ and test/):"
find . -type f -name '*.rb' -not -path './spec/*' -not -path './test/*' -print0 | xargs -0 grep -nI -H -e 'react_on_rails/dev' || true
echo
echo "2) Searching for 'ReactOnRails::Dev' in Ruby files (excluding spec/ and test/):"
find . -type f -name '*.rb' -not -path './spec/*' -not -path './test/*' -print0 | xargs -0 grep -nI -H -e 'ReactOnRails::Dev' || true
echo
echo "3) Searching for 'react_on_rails/dev' in any files (excluding spec/ and test/):"
find . -type f -not -path './spec/*' -not -path './test/*' -print0 | xargs -0 grep -nI -H 'react_on_rails/dev' || true
echo
echo "4) Searching for 'ReactOnRails::Dev' in any files (excluding spec/ and test/):"
find . -type f -not -path './spec/*' -not -path './test/*' -print0 | xargs -0 grep -nI -H 'ReactOnRails::Dev' || true
echo
echo "5) Show lib/react_on_rails/dev.rb (if present):"
if [ -f lib/react_on_rails/dev.rb ]; then
sed -n '1,240p' lib/react_on_rails/dev.rb || true
else
echo "lib/react_on_rails/dev.rb not found"
fiLength of output: 5114
Guard dev-only require to avoid production bloat
lib/react_on_rails.rb:29 currently unconditionally requires "react_on_rails/dev". Wrap it so the dev module loads only in development/test. Repo search shows only lib/react_on_rails/dev.rb, generator bin/dev, generator templates, and docs reference Dev — no runtime app references found.
-require "react_on_rails/dev"
+if defined?(Rails) && (Rails.env.development? || Rails.env.test?)
+ require "react_on_rails/dev"
+end📝 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.
| require "react_on_rails/dev" | |
| if defined?(Rails) && (Rails.env.development? || Rails.env.test?) | |
| require "react_on_rails/dev" | |
| end |
🤖 Prompt for AI Agents
In lib/react_on_rails.rb around line 29, the file unconditionally requires
"react_on_rails/dev"; change this to only load that file in non-production
environments by guarding the require with an environment check (e.g. ensure
Rails is defined and Rails.env is development or test, or fall back to
RACK_ENV/ENV checks) so the dev module is not pulled into production builds;
update the require line to be conditional accordingly.
| killed_any = kill_running_processes || cleanup_socket_files | ||
|
|
||
| print_kill_summary(killed_any) | ||
| end |
There was a problem hiding this comment.
Bug: socket cleanup skipped when processes are found.
kill_running_processes || cleanup_socket_files short-circuits and won’t clean sockets if any process was killed.
- killed_any = kill_running_processes || cleanup_socket_files
+ killed_any = kill_running_processes
+ sockets_cleaned = cleanup_socket_files
+ killed_any ||= sockets_cleaned📝 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.
| killed_any = kill_running_processes || cleanup_socket_files | |
| print_kill_summary(killed_any) | |
| end | |
| killed_any = kill_running_processes | |
| sockets_cleaned = cleanup_socket_files | |
| killed_any ||= sockets_cleaned | |
| print_kill_summary(killed_any) | |
| end |
🤖 Prompt for AI Agents
In lib/react_on_rails/dev/server_manager.rb around lines 29–32, the expression
`killed_any = kill_running_processes || cleanup_socket_files` short-circuits so
socket cleanup is skipped if any process was killed; change it to call both
functions and combine their results (e.g. run `killed_any =
kill_running_processes; cleaned = cleanup_socket_files; killed_any = killed_any
|| cleaned`) so sockets are always cleaned and `killed_any` correctly reflects
if either step did work.
| sh_in_dir(example_type.dir, | ||
| "echo \"gem 'react_on_rails', path: '#{relative_gem_root}'\" >> #{example_type.gemfile}") | ||
| sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '~> 8.0.0'\" >> #{example_type.gemfile}") | ||
| sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '>= 8.2.0'\" >> #{example_type.gemfile}") |
There was a problem hiding this comment.
Align example Gemfile constraint with gemspec
'>= 8.2.0' can pull in 9.x before we’re ready, while the gemspec pins ~> 8.2. Recommend matching the gemspec to avoid accidental major upgrades.
-sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '>= 8.2.0'\" >> #{example_type.gemfile}")
+sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '~> 8.2'\" >> #{example_type.gemfile}")📝 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.
| sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '>= 8.2.0'\" >> #{example_type.gemfile}") | |
| sh_in_dir(example_type.dir, "echo \"gem 'shakapacker', '~> 8.2'\" >> #{example_type.gemfile}") |
🤖 Prompt for AI Agents
In rakelib/shakapacker_examples.rake around line 37, the example Gemfile uses
"gem 'shakapacker', '>= 8.2.0'" which allows pulling 9.x; change the constraint
to match the gemspec by using "~> 8.2" instead (update the echo command to
append gem 'shakapacker', '~> 8.2' to the example Gemfile).
| specify "base generator contains a helpful message" do | ||
| # Mock git status to return clean repository | ||
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | ||
|
|
||
| # Mock Shakapacker installation check to skip installation | ||
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | ||
|
|
||
| run_generator_test_with_args(%w[], package_json: true) | ||
| # GeneratorMessages.output is an array with the git error being the first one | ||
| expect(GeneratorMessages.output).to include(expected) | ||
| # GeneratorMessages.output is an array | ||
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | ||
| expect(helpful_message).not_to be_nil | ||
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | ||
| expect(helpful_message).to include("bundle && npm install") | ||
| end |
There was a problem hiding this comment.
Make message assertion robust to npm vs yarn and clear previous output.
Tests currently hard-code “npm install” and don’t clear prior messages, causing flakiness across environments.
Apply this diff:
- run_generator_test_with_args(%w[], package_json: true)
- # GeneratorMessages.output is an array
- helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") }
+ GeneratorMessages.output.clear
+ run_generator_test_with_args(%w[], package_json: true)
+ helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") }
expect(helpful_message).not_to be_nil
expect(helpful_message).to include("🎉 React on Rails Successfully Installed!")
- expect(helpful_message).to include("bundle && npm install")
+ expect(helpful_message).to match(/bundle && (npm|yarn) install/)📝 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.
| specify "base generator contains a helpful message" do | |
| # Mock git status to return clean repository | |
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | |
| # Mock Shakapacker installation check to skip installation | |
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | |
| run_generator_test_with_args(%w[], package_json: true) | |
| # GeneratorMessages.output is an array with the git error being the first one | |
| expect(GeneratorMessages.output).to include(expected) | |
| # GeneratorMessages.output is an array | |
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | |
| expect(helpful_message).not_to be_nil | |
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | |
| expect(helpful_message).to include("bundle && npm install") | |
| end | |
| specify "base generator contains a helpful message" do | |
| # Mock git status to return clean repository | |
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | |
| # Mock Shakapacker installation check to skip installation | |
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | |
| GeneratorMessages.output.clear | |
| run_generator_test_with_args(%w[], package_json: true) | |
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | |
| expect(helpful_message).not_to be_nil | |
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | |
| expect(helpful_message).to match(/bundle && (npm|yarn) install/) | |
| end |
🤖 Prompt for AI Agents
In spec/react_on_rails/generators/install_generator_spec.rb around lines 61 to
74, the test hardcodes "npm install" and doesn't clear prior GeneratorMessages
output causing flakiness; before running the generator clear
GeneratorMessages.output (or reset its array) and change the assertion for the
install step to accept either "npm install" or "yarn install" (e.g., check that
helpful_message includes "bundle" and matches /(npm|yarn) install/ or check
includes "install" and either "npm" or "yarn"); keep the existing check for the
success banner and ensure the test explicitly sets up the message array to empty
before invoking run_generator_test_with_args.
| specify "react with redux generator contains a helpful message" do | ||
| # Mock git status to return clean repository | ||
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | ||
|
|
||
| # Mock Shakapacker installation check to skip installation | ||
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | ||
|
|
||
| run_generator_test_with_args(%w[--redux], package_json: true) | ||
| # GeneratorMessages.output is an array with the git error being the first one | ||
| expect(GeneratorMessages.output).to include(expected) | ||
| # GeneratorMessages.output is an array | ||
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | ||
| expect(helpful_message).not_to be_nil | ||
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | ||
| expect(helpful_message).to include("bundle && npm install") | ||
| end |
There was a problem hiding this comment.
Repeat robustness for Redux path.
Mirror the npm/yarn regex and clear output before running.
Apply this diff:
- run_generator_test_with_args(%w[--redux], package_json: true)
- # GeneratorMessages.output is an array
- helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") }
+ GeneratorMessages.output.clear
+ run_generator_test_with_args(%w[--redux], package_json: true)
+ helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") }
expect(helpful_message).not_to be_nil
expect(helpful_message).to include("🎉 React on Rails Successfully Installed!")
- expect(helpful_message).to include("bundle && npm install")
+ expect(helpful_message).to match(/bundle && (npm|yarn) install/)📝 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.
| specify "react with redux generator contains a helpful message" do | |
| # Mock git status to return clean repository | |
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | |
| # Mock Shakapacker installation check to skip installation | |
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | |
| run_generator_test_with_args(%w[--redux], package_json: true) | |
| # GeneratorMessages.output is an array with the git error being the first one | |
| expect(GeneratorMessages.output).to include(expected) | |
| # GeneratorMessages.output is an array | |
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | |
| expect(helpful_message).not_to be_nil | |
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | |
| expect(helpful_message).to include("bundle && npm install") | |
| end | |
| specify "react with redux generator contains a helpful message" do | |
| # Mock git status to return clean repository | |
| allow(ReactOnRails::GitUtils).to receive(:`).with("git status --porcelain").and_return("") | |
| # Mock Shakapacker installation check to skip installation | |
| allow_any_instance_of(InstallGenerator).to receive(:shakapacker_binaries_exist?).and_return(true) | |
| GeneratorMessages.output.clear | |
| run_generator_test_with_args(%w[--redux], package_json: true) | |
| helpful_message = GeneratorMessages.output.find { |msg| msg.include?("🎉 React on Rails Successfully Installed!") } | |
| expect(helpful_message).not_to be_nil | |
| expect(helpful_message).to include("🎉 React on Rails Successfully Installed!") | |
| expect(helpful_message).to match(/bundle && (npm|yarn) install/) | |
| end |
🤖 Prompt for AI Agents
In spec/react_on_rails/generators/install_generator_spec.rb around lines 76 to
89, the test for the redux generator should mirror the npm/yarn message handling
and ensure a clean GeneratorMessages.output before running; clear
GeneratorMessages.output (or reset it to []) before run_generator_test_with_args
and, instead of checking for the literal "bundle && npm install", match the
package-manager suggestion using the same npm/yarn regex used elsewhere in the
spec suite (so it accepts either npm or yarn output).
Code Review for PR 1784I have thoroughly reviewed this PR. Here is my comprehensive feedback: Overall Assessment - APPROVED ✅This PR successfully fixes 4 failing RSpec tests in install_generator_spec.rb while introducing significant improvements for v16. With 2040 additions and 816 deletions across 92 files, this represents a major modernization effort. Key ImprovementsTest Fixes (Primary Goal) ✅
CI/CD Modernization ✅
Breaking Changes for v16
|
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for fixing these critical test failures! Overall, this PR makes substantial improvements to both the test suite and the CI/CD pipeline. Here's my comprehensive review: ✅ Strengths
🔍 Code Quality & Best PracticesPositive:
Suggestions:
🐛 Potential Issues
⚡ Performance Considerations
🔒 Security ConcernsNo security issues identified. The changes appropriately:
🧪 Test CoverageGood:
Could be improved:
📋 Minor Observations
✅ RecommendationThis PR successfully addresses the failing tests and makes significant improvements to the project's infrastructure. The changes are well-thought-out and align with the v16 breaking changes objectives. I approve this PR with the minor suggestions above for future consideration. The critical test failures are resolved, and the codebase is in a better state. Great work on adapting the tests to the new architecture! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/react_on_rails/prender_error_spec.rb (1)
68-74: Avoid brittle hard-coded line number; derive path:line from the actual backtraceHard-coding ":20" will break on any edit above the raise. Use the first backtrace frame to assert the "path:line" segment and keep the method-fragment check regex-based.
- # Ruby 3.4 includes class/module names in backtrace method names, but core pattern remains - # Ruby 3.3: "spec/react_on_rails/prender_error_spec.rb:20:in `block (2 levels) in <module:ReactOnRails>'" - # Ruby 3.4: "spec/react_on_rails/prender_error_spec.rb:20:in `SomeClass#block (2 levels) in <module:ReactOnRails>'" - expect(message).to include("spec/react_on_rails/prender_error_spec.rb:20") - expect(message).to include("block (2 levels) in <module:ReactOnRails>") + # Backtrace formats vary across Ruby versions; derive a robust "path:line" from the first frame. + first_frame = err.backtrace.first + path_and_line = first_frame.split(':in').first # => "spec/react_on_rails/prender_error_spec.rb:<line>" + expect(message).to include(path_and_line) + expect(message).to match(/block \(2 levels\) in <module:ReactOnRails>/)spec/react_on_rails/utils_spec.rb (1)
9-11: Shakapacker-only matrix — LGTM, aligns with v15+ requirement.Using a local variable for the matrix matches our testing learning (avoid constants in specs). Consider inlining to reduce noise since there’s only one packer now, but not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/react_on_rails/prender_error_spec.rb(1 hunks)spec/react_on_rails/utils_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
spec/react_on_rails/prender_error_spec.rbspec/react_on_rails/utils_spec.rb
🧠 Learnings (2)
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
spec/react_on_rails/prender_error_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/utils_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/utils_spec.rb (1)
lib/react_on_rails/utils.rb (1)
smart_trim(244-258)
⏰ 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: examples (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
| result = described_class.smart_trim(s, 9) | ||
|
|
||
| # Ruby 3.4+ uses modern hash syntax: {a: "value"} | ||
| # Ruby 3.3 uses old syntax: {:a=>"value"} | ||
| expected_modern = "{a: #{Utils::TRUNCATION_FILLER}890\"}" | ||
| expected_legacy = "{:a=#{Utils::TRUNCATION_FILLER}890\"}" | ||
|
|
||
| expect(result).to satisfy("match either Ruby 3.3 or 3.4+ hash format") do |value| | ||
| value == expected_modern || value == expected_legacy | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Fix Ruby 3.4+ “modern hash syntax” expectation (missing quote and off‑by‑one).
For modern inspect output, the trimmed string should include the opening quote and keep “90” on the right. Current expected_modern will fail under Ruby 3.4+.
Apply this diff:
- expected_modern = "{a: #{Utils::TRUNCATION_FILLER}890\"}"
+ expected_modern = "{a: \"#{Utils::TRUNCATION_FILLER}90\"}"
@@
- expect(result).to satisfy("match either Ruby 3.3 or 3.4+ hash format") do |value|
- value == expected_modern || value == expected_legacy
- end
+ expect([expected_modern, expected_legacy]).to include(result)If you want to double‑check the underlying assumption about Ruby’s inspect output change:
🌐 Web query:
Did Ruby 3.4 change Hash#inspect (and to_s) to use modern {a: "v"} syntax for symbol keys instead of {:a=>"v"}?
💡 Result:
Yes. Ruby 3.4 changed Hash#inspect (and thus Hash#to_s/printed output) to use the modern symbol-key syntax (e.g. {a: "v"}) for symbol keys and to add spaces around => for non-symbol keys. [1][2][3]
Sources:
- Ruby 3.4 release notes (Hash#inspect rendering changed). [1]
- Ruby 3.4 changelog summary / RubyChanges. [2]
- Ruby bug tracker discussion noting the change and related inconsistency. [3]
Fix Ruby 3.4+ “modern hash syntax” expectation (missing quote and off‑by‑one).
Update expected_modern and simplify the matcher to accept either legacy or modern inspect output.
File: spec/react_on_rails/utils_spec.rb (lines 466–476)
- expected_modern = "{a: #{Utils::TRUNCATION_FILLER}890\"}"
+ expected_modern = "{a: \"#{Utils::TRUNCATION_FILLER}90\"}"
@@
- expect(result).to satisfy("match either Ruby 3.3 or 3.4+ hash format") do |value|
- value == expected_modern || value == expected_legacy
- end
+ expect([expected_modern, expected_legacy]).to include(result)📝 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.
| result = described_class.smart_trim(s, 9) | |
| # Ruby 3.4+ uses modern hash syntax: {a: "value"} | |
| # Ruby 3.3 uses old syntax: {:a=>"value"} | |
| expected_modern = "{a: #{Utils::TRUNCATION_FILLER}890\"}" | |
| expected_legacy = "{:a=#{Utils::TRUNCATION_FILLER}890\"}" | |
| expect(result).to satisfy("match either Ruby 3.3 or 3.4+ hash format") do |value| | |
| value == expected_modern || value == expected_legacy | |
| end | |
| end | |
| result = described_class.smart_trim(s, 9) | |
| # Ruby 3.4+ uses modern hash syntax: {a: "value"} | |
| # Ruby 3.3 uses old syntax: {:a=>"value"} | |
| expected_modern = "{a: \"#{Utils::TRUNCATION_FILLER}90\"}" | |
| expected_legacy = "{:a=#{Utils::TRUNCATION_FILLER}890\"}" | |
| expect([expected_modern, expected_legacy]).to include(result) |
🤖 Prompt for AI Agents
spec/react_on_rails/utils_spec.rb around lines 466–476: the modern-hash
expectation is missing a quote and off-by-one; fix expected_modern to include
the missing opening quote around the inspected value (e.g. set expected_modern
to "{a: \"#{Utils::TRUNCATION_FILLER}890\"}") and simplify the matcher to accept
either string form by comparing result to expected_modern || expected_legacy
(retain the existing satisfy block but use the corrected expected_modern).
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for this PR! I've conducted a thorough review of the changes. This is a substantial update that modernizes the React on Rails development experience with significant architectural improvements for v16. Overall Assessment: APPROVED with suggestionsThe PR successfully addresses the failing tests and introduces valuable enhancements to the v16 generator implementation. The changes are well-structured and align with modern Rails and JavaScript development practices. Strengths1. Enhanced Developer Experience
2. Modern Architecture
3. Component Auto-Registration
4. Test Coverage
Potential Issues and Suggestions1. Process Termination SafetyIn server_manager.rb:70-74, consider adding a graceful shutdown with timeout and SIGKILL fallback for stubborn processes. 2. Node Version DetectionThe node version check in install_generator.rb:99-118 could be more robust for non-standard installations by checking command execution status. 3. Package Manager DetectionConsider adding support for Bun package manager in base_generator.rb:105-117 alongside npm, yarn, and pnpm. 4. Error Handling in Pack Generationpack_generator.rb could benefit from more detailed error reporting to help developers debug failures. Performance Considerations
Security Review
Documentation and Code Quality
Test CoverageThe test fixes properly address:
Minor Suggestions
ConclusionThis is a high-quality PR that significantly improves the React on Rails development experience. The architectural improvements, better error handling, and modern tooling support make this a valuable update for v16. The test fixes ensure CI stability while maintaining comprehensive coverage. Recommendation: Merge after considering the process termination safety improvements mentioned above. Great work on modernizing the generator and development tooling! |
Code Review for PR #1784: Fix failing install generator tests for v16 🔍Thank you for this comprehensive PR that addresses critical test failures and introduces significant improvements for v16! Here's my detailed review: ✅ Strengths
🎯 Code Quality ObservationsArchitecture & Design
Test Coverage
|
…ager detection - Clear GeneratorMessages.output before tests to ensure clean state - Use flexible regex for package manager detection in assertions - Improves test robustness across different environments 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for fixing these failing tests! I have reviewed the changes and here is my feedback: ✅ Strengths
🔍 Suggestions for Improvement
|
Code Review for PR #1784: Fix failing install generator tests for v16✅ Overall AssessmentGood job fixing the failing tests! The changes appropriately handle the new v16 generator behavior and improve test isolation through proper mocking. 👍 Strengths
🔍 Observations & Suggestions1. Test Coverage (spec/react_on_rails/generators/install_generator_spec.rb)
2. Ruby Version Compatibility
3. Minor ImprovementsConsider these enhancements:
🐛 Potential IssuesNone detected - the changes appropriately fix the failing tests without introducing new problems. 🔒 SecurityNo security concerns - these are test-only changes with proper mocking. ⚡ PerformanceNo performance impact - test-only changes. ✅ RecommendationApproved - These changes successfully fix the failing tests and improve test reliability. The PR accomplishes its goal of unblocking CI for the v16 breaking changes. Great work on maintaining backward compatibility while fixing the test suite! 🎉 |
Code Review: Fix failing install generator tests for v16✅ Overall AssessmentThis PR effectively fixes the failing tests for v16 generator implementation with appropriate mocking and Ruby 3.4 compatibility updates. The changes are well-targeted and maintain test integrity. 🎯 Code Quality and Best PracticesStrengths:
Areas for Improvement:
🐛 Potential IssuesNone identified. The changes appropriately handle:
⚡ Performance ConsiderationsGood performance practices observed:
🔒 Security ConcernsNo security issues identified. The changes:
✅ Test CoverageExcellent coverage for:
Suggestions:
💡 Additional Recommendations
✨ SummaryThis is a well-crafted PR that addresses the test failures cleanly and maintainably. The mocking approach is superior to the previous file system manipulation, and the Ruby 3.4 compatibility fixes are thoughtful and forward-looking. Great work on maintaining backward compatibility while supporting newer Ruby versions! Approved ✅ - Ready to merge once CI passes. |
…ager detection - Clear GeneratorMessages.output before tests to ensure clean state - Use flexible regex for package manager detection in assertions - Improves test robustness across different environments 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
update-generator-for-v15
branch
from
8806a3b to
8f4e895
Compare
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for addressing the test failures in the install generator. Here's my comprehensive review of the changes: ✅ Strengths
🔍 Areas for Improvement1. Test Mocking Could Be More RobustThe install generator spec uses # More specific stubbing
allow(ReactOnRails::GitUtils).to receive(:uncommitted_changes?).and_return(false)2. Error Handling in Dev ToolsThe new # lib/react_on_rails/dev/process_manager.rb
def valid_procfile_path?(procfile)
# Add check for path traversal attempts
return false if procfile.include?("..") || procfile.start_with?("/")
# existing checks...
end3. Potential Race Condition in FileManagerThe # Between checking if process exists and removing the file,
# the process could start again
def cleanup_rails_pid_file
# Consider using file locking or atomic operations
end🐛 Potential Issues
💡 Suggestions
🔒 Security ConsiderationsThe procfile validation in
✨ Performance ConsiderationsThe new auto-registration feature generates packs dynamically, which is great for DX but consider:
🎯 Test CoverageWhile the PR fixes the failing tests, consider adding:
Overall AssessmentThis is a solid PR that successfully fixes the test failures and adds valuable new features. The code quality is good, and the new dev tools are well-structured. With the minor improvements suggested above, this will be an excellent addition to v16. Approved with suggestions ✅ Please address the security considerations and potential race conditions before merging. The other suggestions can be addressed in follow-up PRs if needed. |
Pull Request Review: Fix failing install generator tests for v16Thank you for this PR fixing the failing tests in ✅ Strengths
🔍 Areas for Improvement1. Test Coverage & Mocking Strategy
2. Potential Race ConditionsIn def self.kill_child_processes
child_pids = `pgrep -P #{Process.pid}`.split("\n").map(&:to_i)
# Potential race condition here if processes spawn between pgrep and kill
child_pids.each { |pid| Process.kill("TERM", pid) rescue nil }
endConsider using process groups or a more robust process management approach. 3. Security Considerations
4. Error HandlingIn several places, errors are rescued silently: Process.kill("TERM", pid) rescue nilThis could hide legitimate issues. Consider at least logging these failures. 5. Performance Implications
🐛 Potential Issues
📝 Minor Suggestions
✅ Test Coverage AssessmentThe test changes look comprehensive and properly handle the new v16 behavior:
🚀 Recommendations
VerdictThis PR successfully fixes the failing tests and significantly improves the developer experience with better error messages and tooling. The code quality is good, but there are opportunities for improvement in error handling, security, and cross-platform support. Recommendation: Approve with suggestions - The PR achieves its primary goal of fixing the tests and the improvements far outweigh the issues. The suggestions above can be addressed in follow-up PRs. Great work on modernizing the generator and improving the developer experience! 🎉 |
Code Review for PR #1784: Fix failing install generator tests for v16Overall AssessmentThis PR comprehensively addresses the test failures in the v16 breaking changes (PR #1770) by properly mocking external dependencies and adapting to the new generator behavior. The changes are well-structured and follow good testing practices. ✅ Strengths
📋 Code Quality Observations
🔍 Potential Issues
💡 Suggestions for Improvement
✅ Performance ConsiderationsThe changes have minimal performance impact as they primarily affect test execution. The mocking approach actually improves test performance by avoiding external command execution. ✅ Security ReviewNo security concerns identified. The changes are limited to test specifications and don't affect production code paths. ✅ Test CoverageThe PR maintains good test coverage for the generator functionality. The tests properly validate:
Verdict: APPROVED ✅This PR effectively fixes the failing tests while improving test reliability and maintainability. The changes are minimal, focused, and follow established testing patterns in the codebase. The fixes ensure CI will pass for the v16 breaking changes PR #1770. Recommendation: Merge after considering the minor suggestions above, particularly the platform-specific behavior verification for Windows systems. |
|
Code Review for PR 1784: Fix failing install generator tests for v16 Thank you for this PR that fixes the failing tests for the v16 generator implementation. I have reviewed the changes and here is my comprehensive feedback: STRENGTHS:
CODE QUALITY:
POTENTIAL ISSUES:
PERFORMANCE CONSIDERATIONS:
SECURITY:
TEST COVERAGE:
SUGGESTIONS FOR IMPROVEMENT:
OVERALL ASSESSMENT: Recommendation: Approved with minor suggestions. The CHANGELOG date should be corrected before merging. |
Code ReviewI've thoroughly reviewed PR #1784 and the changes look excellent! This PR successfully fixes the failing install generator tests for v16 and makes important updates to the CI pipeline. Here's my detailed feedback: ✅ Strengths
🔍 Code Quality Observations
💡 Minor Suggestions
✅ Security & Performance
✅ Overall AssessmentThis is a high-quality PR that successfully addresses the test failures while making valuable improvements to the codebase. The changes are well-structured, properly tested, and follow the project's conventions. The modernization of the CI pipeline will make the testing matrix more maintainable going forward. Recommendation: Ready to merge once CI passes. Great work on fixing these v16 compatibility issues! 🎉 |
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for this contribution to fix the failing generator tests! This is a massive PR with significant v16 breaking changes. Here's my review: ✅ Strengths
|
Code Review for PR #1784: Fix failing install generator tests for v16Thank you for this comprehensive PR that addresses test failures and introduces significant improvements to the React on Rails v16 generator. This is a substantial update with major enhancements to the development workflow. ✅ Strengths
🔍 Areas for Improvement1. Performance Considerations
2. Security Concerns
3. Potential Bugs
4. Code Quality Suggestions
5. Documentation & UX
📋 Specific Recommendations
✨ Minor Suggestions
📊 Test Coverage AnalysisThe test fixes properly address the v16 changes:
However, consider adding:
Overall AssessmentThis is a high-quality PR that significantly improves the developer experience with React on Rails. The new features are well-thought-out and the test fixes properly address the v16 breaking changes. With the suggested improvements around security, error handling, and Windows compatibility, this will be an excellent addition to v16. Recommendation: ✅ Approve with suggestions - The core functionality is solid and the test fixes are correct. The suggestions above would enhance robustness but aren't blockers for merging. Great work on modernizing the development workflow! 🚀 |
Code Review for PR #1784This PR successfully fixes the failing RSpec tests for the v16 generator implementation. The test fixes are well-structured and appropriate. Positive Aspects
Areas to Consider
RecommendationsThe PR effectively fixes the failing tests with minimal, focused changes. Additional test coverage for package manager detection can be added in a follow-up PR. APPROVED - Great work fixing these CI issues! |
|
@ihabadham can we close this PR? Check if I got your key fixes. |
3 participants
Summary
Fixes 4 failing RSpec tests in
install_generator_spec.rbthat were breaking due to changes in the v16 generator implementation:node --versioncall introduced by new version checkingThe failing tests were preventing CI from passing for the v16 breaking changes PR #1770 .
Pull Request checklist
Other Information
These test fixes ensure the generator tests properly validate the new v16 behavior while maintaining clean test isolation through proper mocking instead of file system manipulation.
This change is
Summary by CodeRabbit