Fix generator robustness issues: package manager detection, validation skip, and webpack safety

This commit addresses three code review issues:

1. Fix package manager hard-coding (High Priority)
   - Add detect_package_manager_and_exact_flag helper to GeneratorHelper
   - Detect package manager from lock files (yarn, pnpm, bun, npm)
   - Update all package installation methods to use detected PM
   - Warn users when multiple lock files are detected
   - Fixes issue where npm was hard-coded, causing conflicts for yarn/pnpm/bun users

2. Fix fragile ARGV-based generator detection (Medium Priority)
   - Replace ARGV.any? check with explicit ENV variable
   - Install generator sets REACT_ON_RAILS_SKIP_VALIDATION=true
   - More robust than string matching on ARGV
   - Works correctly in all contexts (rake, console, tests)
   - Prevents false positives from paths containing "generate" or "g"

3. Add defensive checks to webpack CSS Modules config (Low Priority)
   - Add comprehensive type and existence checks before accessing loader properties
   - Prevents potential runtime errors with malformed webpack configs
   - Consistent with defensive pattern used for SCSS configuration

All changes verified with:
- bundle exec rubocop: 0 offenses
- Install generator specs: 53 examples, 0 failures

Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

Author: justin808

.github/workflows/lint-js-and-ruby.yml

@@ -72,7 +72,7 @@ jobs:
         run: cd spec/dummy && RAILS_ENV="test" bundle exec rake react_on_rails:generate_packs
       - name: Detect dead code
         run: |
-          yarn run knip
+          yarn run knip --no-config-hints
           yarn run knip --production
       - name: Lint JS
         run: yarn run eslint --report-unused-disable-directives

knip.ts

@@ -10,6 +10,8 @@ const config: KnipConfig = {
       ignoreBinaries: [
         // Has to be installed globally
         'yalc',
+        // Used in package.json scripts (devDependency, so unlisted in production mode)
+        'nps',
         // Pro package binaries used in Pro workflows
         'playwright',
         'e2e-test',
@@ -108,6 +110,9 @@ const config: KnipConfig = {
         'bin/.*',
       ],
       ignoreDependencies: [
+        // Build-time dependencies not detected by Knip in any mode
+        '@babel/runtime',
+        'mini-css-extract-plugin',
         // There's no ReScript plugin for Knip
         '@rescript/react',
         // The Babel plugin fails to detect it

lib/generators/react_on_rails/generator_helper.rb

@@ -95,4 +95,35 @@ def add_documentation_reference(message, source)
   def component_extension(options)
     options.typescript? ? "tsx" : "jsx"
   end
+
+  # Detects the package manager based on lock files and returns the appropriate command and exact flag
+  # Returns: [package_manager, exact_flag, add_command]
+  # Examples: ["yarn", "--exact", "add"], ["npm", "--save-exact", "install"]
+  def detect_package_manager_and_exact_flag
+    lock_files = {
+      "yarn.lock" => ["yarn", "--exact", "add"],
+      "pnpm-lock.yaml" => ["pnpm", "--save-exact", "add"],
+      "bun.lockb" => ["bun", "--exact", "add"],
+      "package-lock.json" => ["npm", "--save-exact", "install"]
+    }
+
+    detected = []
+    lock_files.each do |lock_file, config|
+      detected << [lock_file, config] if File.exist?(File.join(destination_root, lock_file))
+    end
+
+    # Warn if multiple lock files detected
+    if detected.size > 1
+      GeneratorMessages.add_warning(<<~MSG.strip)
+        ⚠️  Multiple package manager lock files detected:
+        #{detected.map { |lf, _| "  • #{lf}" }.join("\n")}
+
+        This can cause dependency conflicts. Consider using only one package manager.
+        Using #{detected.first[0]} based on file precedence.
+      MSG
+    end
+
+    # Return first detected, or default to npm
+    detected.empty? ? ["npm", "--save-exact", "install"] : detected.first[1]
+  end
 end

lib/generators/react_on_rails/install_generator.rb

@@ -37,6 +37,10 @@ class InstallGenerator < Rails::Generators::Base
       # Removed: --skip-shakapacker-install (Shakapacker is now a required dependency)
 
       def run_generators
+        # Set environment variable to skip validation during generator run
+        # This is inherited by all invoked generators
+        ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"
+
         if installation_prerequisites_met? || options.ignore_warnings?
           invoke_generators
           add_bin_scripts
@@ -55,6 +59,7 @@ def run_generators
           GeneratorMessages.add_error(error)
         end
       ensure
+        ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
         print_generator_messages
       end
 
@@ -430,8 +435,7 @@ def add_js_dependencies
       def add_react_on_rails_package
         major_minor_patch_only = /\A\d+\.\d+\.\d+\z/
 
-        # Always use direct npm install with --save-exact to ensure exact version matching
-        # The package_json gem doesn't support --save-exact flag
+        # Use detected package manager with --save-exact flag to ensure exact version matching
         react_on_rails_pkg = if ReactOnRails::VERSION.match?(major_minor_patch_only)
                                "react-on-rails@#{ReactOnRails::VERSION}"
                              else
@@ -441,7 +445,8 @@ def add_react_on_rails_package
                              end
 
         puts "Installing React on Rails package..."
-        success = system("npm", "install", "--save-exact", react_on_rails_pkg)
+        package_manager, exact_flag, add_command = detect_package_manager_and_exact_flag
+        success = system(package_manager, add_command, exact_flag, react_on_rails_pkg)
         @ran_direct_installs = true if success
         handle_npm_failure("react-on-rails package", [react_on_rails_pkg]) unless success
       end
@@ -461,7 +466,8 @@ def add_react_dependencies
           return
         end
 
-        success = system("npm", "install", *react_deps)
+        package_manager, _exact_flag, add_command = detect_package_manager_and_exact_flag
+        success = system(package_manager, add_command, *react_deps)
         @ran_direct_installs = true if success
         handle_npm_failure("React dependencies", react_deps) unless success
       end
@@ -479,7 +485,8 @@ def add_css_dependencies
           return
         end
 
-        success = system("npm", "install", *css_deps)
+        package_manager, _exact_flag, add_command = detect_package_manager_and_exact_flag
+        success = system(package_manager, add_command, *css_deps)
         @ran_direct_installs = true if success
         handle_npm_failure("CSS dependencies", css_deps) unless success
       end
@@ -497,7 +504,10 @@ def add_dev_dependencies
           return
         end
 
-        success = system("npm", "install", "--save-dev", *dev_deps)
+        package_manager, _exact_flag, add_command = detect_package_manager_and_exact_flag
+        # For dev dependencies, we need to add the --dev or -D flag depending on the package manager
+        dev_flag = package_manager == "npm" ? "--save-dev" : "-D"
+        success = system(package_manager, add_command, dev_flag, *dev_deps)
         @ran_direct_installs = true if success
         handle_npm_failure("development dependencies", dev_deps, dev: true) unless success
       end

lib/react_on_rails/engine.rb

@@ -13,8 +13,8 @@ class Engine < ::Rails::Engine
         package_json = VersionChecker::NodePackageVersion.package_json_path
         next unless File.exist?(package_json)
 
-        # Skip validation when running generators (packages may not be installed yet)
-        next if defined?(Rails::Generators) && ARGV.any? { |arg| arg.include?("generate") || arg.include?("g") }
+        # Skip validation when generators explicitly set this flag (packages may not be installed yet)
+        next if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"
 
         Rails.logger.info "[React on Rails] Validating package version and compatibility..."
         VersionChecker.build.validate_version_and_package_compatibility!

spec/dummy/config/webpack/commonWebpackConfig.js

@@ -34,7 +34,17 @@ if (scssConfigIndex !== -1 && baseClientWebpackConfig.module.rules[scssConfigInd
 baseClientWebpackConfig.module.rules.forEach((rule) => {
   if (Array.isArray(rule.use)) {
     rule.use.forEach((loader) => {
-      if (loader.loader && loader.loader.includes('css-loader') && loader.options?.modules) {
+      if (
+        loader &&
+        typeof loader === 'object' &&
+        loader.loader &&
+        typeof loader.loader === 'string' &&
+        loader.loader.includes('css-loader') &&
+        loader.options &&
+        typeof loader.options === 'object' &&
+        loader.options.modules &&
+        typeof loader.options.modules === 'object'
+      ) {
         // eslint-disable-next-line no-param-reassign
         loader.options.modules.namedExport = false;
         // eslint-disable-next-line no-param-reassign