Address code review feedback: improve validation skip logic

Improvements based on code review:

1. **Nil-safe ARGV check**: Use safe navigation operator and check for empty ARGV
   - Prevents errors when ARGV is empty or nil
   - More defensive against edge cases

2. **Refactored to extract methods**: Created testable class methods
   - skip_version_validation?() - main entry point
   - running_generator?() - checks if running Rails generator
   - package_json_missing?() - checks if package.json exists
   - Improves testability and code clarity

3. **Added debug logging**: Log when validation is skipped
   - "Skipping validation - package.json not found"
   - "Skipping validation during generator runtime"
   - Helps debugging and understanding behavior

4. **Optimized check order**: Check package.json existence first
   - File existence check is cheaper than ARGV inspection
   - Handles more cases (initial setup, generators, etc.)

5. **Comprehensive test coverage**: Added 15 test cases
   - Tests for package.json missing scenarios
   - Tests for generator detection (both "generate" and "g")
   - Tests for edge cases (empty ARGV, other commands)
   - Tests for each helper method

All tests passing, RuboCop clean.

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

Co-Authored-By: Claude <noreply@anthropic.com>

Author: justin808

lib/react_on_rails/engine.rb

@@ -9,20 +9,44 @@ class Engine < ::Rails::Engine
     # Skip validation during installation tasks (e.g., shakapacker:install) or generator runtime
     initializer "react_on_rails.validate_version_and_package_compatibility" do
       config.after_initialize do
-        # Skip validation when running Rails generators - they will install packages during execution
-        running_generator = ARGV.first == "generate" || ARGV.first == "g"
-        next if running_generator
-
-        # Skip validation if package.json doesn't exist yet (during initial setup)
-        package_json = VersionChecker::NodePackageVersion.package_json_path
-        next unless File.exist?(package_json)
+        next if skip_version_validation?
 
         Rails.logger.info "[React on Rails] Validating package version and compatibility..."
         VersionChecker.build.validate_version_and_package_compatibility!
         Rails.logger.info "[React on Rails] Package validation successful"
       end
     end
 
+    # Determine if version validation should be skipped
+    # @return [Boolean] true if validation should be skipped
+    def self.skip_version_validation?
+      # Check package.json first as it's cheaper and handles more cases
+      if package_json_missing?
+        Rails.logger.debug "[React on Rails] Skipping validation - package.json not found"
+        return true
+      end
+
+      # Skip during generator runtime since packages are installed during execution
+      if running_generator?
+        Rails.logger.debug "[React on Rails] Skipping validation during generator runtime"
+        return true
+      end
+
+      false
+    end
+
+    # Check if we're running a Rails generator
+    # @return [Boolean] true if running a generator
+    def self.running_generator?
+      !ARGV.empty? && ARGV.first&.in?(%w[generate g])
+    end
+
+    # Check if package.json doesn't exist yet
+    # @return [Boolean] true if package.json is missing
+    def self.package_json_missing?
+      !File.exist?(VersionChecker::NodePackageVersion.package_json_path)
+    end
+
     config.to_prepare do
       ReactOnRails::ServerRenderingPool.reset_pool
     end

spec/react_on_rails/engine_spec.rb

@@ -0,0 +1,160 @@
+# frozen_string_literal: true
+
+require_relative "spec_helper"
+
+module ReactOnRails
+  RSpec.describe Engine do
+    describe ".skip_version_validation?" do
+      let(:package_json_path) { "/fake/path/package.json" }
+
+      before do
+        allow(VersionChecker::NodePackageVersion).to receive(:package_json_path)
+          .and_return(package_json_path)
+        allow(Rails.logger).to receive(:debug)
+      end
+
+      context "when package.json doesn't exist" do
+        before do
+          allow(File).to receive(:exist?).with(package_json_path).and_return(false)
+        end
+
+        it "returns true" do
+          expect(described_class.skip_version_validation?).to be true
+        end
+
+        it "logs debug message about missing package.json" do
+          described_class.skip_version_validation?
+          expect(Rails.logger).to have_received(:debug)
+            .with("[React on Rails] Skipping validation - package.json not found")
+        end
+      end
+
+      context "when package.json exists" do
+        before do
+          allow(File).to receive(:exist?).with(package_json_path).and_return(true)
+        end
+
+        context "when running a generator" do
+          before do
+            stub_const("ARGV", ["generate", "react_on_rails:install"])
+          end
+
+          it "returns true" do
+            expect(described_class.skip_version_validation?).to be true
+          end
+
+          it "logs debug message about generator runtime" do
+            described_class.skip_version_validation?
+            expect(Rails.logger).to have_received(:debug)
+              .with("[React on Rails] Skipping validation during generator runtime")
+          end
+        end
+
+        context "when running a generator with short form" do
+          before do
+            stub_const("ARGV", ["g", "react_on_rails:install"])
+          end
+
+          it "returns true" do
+            expect(described_class.skip_version_validation?).to be true
+          end
+        end
+
+        context "when ARGV is empty" do
+          before do
+            stub_const("ARGV", [])
+          end
+
+          it "returns false" do
+            expect(described_class.skip_version_validation?).to be false
+          end
+        end
+
+        context "when running other commands" do
+          %w[server console runner].each do |command|
+            context "when running '#{command}'" do
+              before do
+                stub_const("ARGV", [command])
+              end
+
+              it "returns false" do
+                expect(described_class.skip_version_validation?).to be false
+              end
+            end
+          end
+        end
+      end
+    end
+
+    describe ".running_generator?" do
+      context "when ARGV is empty" do
+        before do
+          stub_const("ARGV", [])
+        end
+
+        it "returns false" do
+          expect(described_class.running_generator?).to be false
+        end
+      end
+
+      context "when ARGV.first is 'generate'" do
+        before do
+          stub_const("ARGV", %w[generate model User])
+        end
+
+        it "returns true" do
+          expect(described_class.running_generator?).to be true
+        end
+      end
+
+      context "when ARGV.first is 'g'" do
+        before do
+          stub_const("ARGV", %w[g controller Users])
+        end
+
+        it "returns true" do
+          expect(described_class.running_generator?).to be true
+        end
+      end
+
+      context "when ARGV.first is another command" do
+        before do
+          stub_const("ARGV", ["server"])
+        end
+
+        it "returns false" do
+          expect(described_class.running_generator?).to be false
+        end
+      end
+    end
+
+    describe ".package_json_missing?" do
+      let(:package_json_path) { "/fake/path/package.json" }
+
+      before do
+        allow(VersionChecker::NodePackageVersion).to receive(:package_json_path)
+          .and_return(package_json_path)
+      end
+
+      context "when package.json exists" do
+        before do
+          allow(File).to receive(:exist?).with(package_json_path).and_return(true)
+        end
+
+        it "returns false" do
+          expect(described_class.package_json_missing?).to be false
+        end
+      end
+
+      context "when package.json doesn't exist" do
+        before do
+          allow(File).to receive(:exist?).with(package_json_path).and_return(false)
+        end
+
+        it "returns true" do
+          expect(described_class.package_json_missing?).to be true
+        end
+      end
+    end
+  end
+end