Improve validation skip documentation and test coverage (#1924)

* Improve validation skip documentation and test coverage

Addresses feedback from PR #1923:

1. Enhanced documentation:
   - Added comprehensive method-level docs explaining ENV variable approach
   - Documented why ENV is preferred over ARGV (Rails modifies ARGV during init)
   - Added thread safety notes explaining process-global ENV scope
   - Cross-referenced engine.rb and install_generator.rb for clarity

2. Improved test coverage:
   - Added tests verifying ENV variable takes precedence over other checks
   - Tests confirm ENV check short-circuits before package.json/ARGV checks
   - Added 4 new test cases covering precedence scenarios

3. Thread safety documentation:
   - Documented that generators run in single process (not a concern)
   - Added note about parallel test scenarios (separate processes)
   - Clarified ENV cleanup in ensure block

Changes:
- lib/react_on_rails/engine.rb: Enhanced skip_validation? docs
- lib/generators/react_on_rails/install_generator.rb: Added run_generators docs
- spec/react_on_rails/engine_spec.rb: Added 4 precedence tests

All specs pass (21 examples, 0 failures)
RuboCop clean

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

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

* Refine documentation and test structure per code review

Addresses additional code review suggestions:

1. Edge Case Documentation:
   - Added @note about manual ENV variable setting
   - Documented use case for debugging/setup scenarios
   - Warned against bypassing validation in production

2. Improved Test Structure:
   - Refactored precedence tests to use nested contexts
   - Changed structure: "with other skip conditions also present"
     -> "when package.json exists and ARGV indicates generator"
     -> "when package.json is missing"
   - More descriptive test names focusing on behavior
   - Better organization showing ENV precedence clearly

3. Simplified Comments:
   - Reduced redundancy in install_generator.rb ensure block
   - Changed from 2 lines to 1 concise line
   - Removed redundant statement about concurrent generators

Changes maintain all functionality while improving readability
and addressing edge cases in documentation.

All specs pass (21 examples, 0 failures)
RuboCop clean

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

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

---------

Co-authored-by: Claude <[email protected]>

Author: justin808

lib/generators/react_on_rails/install_generator.rb

@@ -36,9 +36,23 @@ class InstallGenerator < Rails::Generators::Base
 
       # Removed: --skip-shakapacker-install (Shakapacker is now a required dependency)
 
+      # Main generator entry point
+      #
+      # Sets up React on Rails in a Rails application by:
+      # 1. Validating prerequisites
+      # 2. Installing required packages
+      # 3. Generating configuration files
+      # 4. Setting up example components
+      #
+      # @note Validation Skipping: Sets ENV["REACT_ON_RAILS_SKIP_VALIDATION"] to prevent
+      #   version validation from running during generator execution. The npm package
+      #   isn't installed until midway through the generator, so validation would fail
+      #   if run during Rails initialization. The ensure block guarantees cleanup even
+      #   if the generator fails.
       def run_generators
         # Set environment variable to skip validation during generator run
         # This is inherited by all invoked generators and persists through Rails initialization
+        # See lib/react_on_rails/engine.rb for the validation skip logic
         ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"
 
         if installation_prerequisites_met? || options.ignore_warnings?
@@ -59,6 +73,7 @@ def run_generators
           GeneratorMessages.add_error(error)
         end
       ensure
+        # Always clean up ENV variable, even if generator fails
         ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
         print_generator_messages
       end

lib/react_on_rails/engine.rb

@@ -18,9 +18,28 @@ class Engine < ::Rails::Engine
     end
 
     # Determine if version validation should be skipped
+    #
+    # This method checks multiple conditions to determine if package version validation
+    # should be skipped. Validation is skipped during setup scenarios where the npm
+    # package isn't installed yet (e.g., during generator execution).
+    #
     # @return [Boolean] true if validation should be skipped
+    #
+    # @note Thread Safety: ENV variables are process-global. In practice, Rails generators
+    #   run in a single process, so concurrent execution is not a concern. If running
+    #   generators concurrently (e.g., in parallel tests), ensure tests run in separate
+    #   processes to avoid ENV variable conflicts.
+    #
+    # @note Manual ENV Setting: While this ENV variable is designed to be set by generators,
+    #   users can manually set it (e.g., `REACT_ON_RAILS_SKIP_VALIDATION=true rails server`)
+    #   to bypass validation. This should only be done temporarily during debugging or
+    #   setup scenarios. The validation helps catch version mismatches early, so bypassing
+    #   it in production is not recommended.
     def self.skip_version_validation?
       # Skip if explicitly disabled via environment variable (set by generators)
+      # Using ENV variable instead of ARGV because Rails can modify/clear ARGV during
+      # initialization, making ARGV unreliable for detecting generator context. The ENV
+      # variable persists through the entire Rails initialization process.
       if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"
         Rails.logger.debug "[React on Rails] Skipping validation - disabled via environment variable"
         return true
@@ -33,6 +52,7 @@ def self.skip_version_validation?
       end
 
       # Skip during generator runtime since packages are installed during execution
+      # This is a fallback check in case ENV wasn't set, though ENV is the primary mechanism
       if running_generator?
         Rails.logger.debug "[React on Rails] Skipping validation during generator runtime"
         return true

spec/react_on_rails/engine_spec.rb

@@ -31,6 +31,45 @@ module ReactOnRails
           expect(Rails.logger).to have_received(:debug)
             .with("[React on Rails] Skipping validation - disabled via environment variable")
         end
+
+        context "with other skip conditions also present" do
+          context "when package.json exists and ARGV indicates generator" do
+            before do
+              allow(File).to receive(:exist?).with(package_json_path).and_return(true)
+              stub_const("ARGV", ["generate", "react_on_rails:install"])
+            end
+
+            it "prioritizes ENV over ARGV check" do
+              expect(described_class.skip_version_validation?).to be true
+            end
+
+            it "short-circuits before checking ARGV" do
+              described_class.skip_version_validation?
+              expect(Rails.logger).to have_received(:debug)
+                .with("[React on Rails] Skipping validation - disabled via environment variable")
+              expect(Rails.logger).not_to have_received(:debug)
+                .with("[React on Rails] Skipping validation during generator runtime")
+            end
+          end
+
+          context "when package.json is missing" do
+            before do
+              allow(File).to receive(:exist?).with(package_json_path).and_return(false)
+            end
+
+            it "prioritizes ENV over package.json check" do
+              expect(described_class.skip_version_validation?).to be true
+            end
+
+            it "short-circuits before checking package.json" do
+              described_class.skip_version_validation?
+              expect(Rails.logger).to have_received(:debug)
+                .with("[React on Rails] Skipping validation - disabled via environment variable")
+              expect(Rails.logger).not_to have_received(:debug)
+                .with("[React on Rails] Skipping validation - package.json not found")
+            end
+          end
+        end
       end
 
       context "when package.json doesn't exist" do