Improve doctor async checks: better error handling and comment filtering

Addressed code review feedback to make the async usage detection more
robust and accurate:

1. Enhanced error handling:
   - Replace broad StandardError catches with specific exceptions
     (Errno::ENOENT, Encoding::InvalidByteSequenceError,
     Encoding::UndefinedConversionError)
   - Add debug logging when errors occur (if Rails logger available)

2. Add comment filtering to reduce false positives:
   - Filter out commented Ruby lines in config files (lines starting with #)
   - Skip ERB comments (<%# ... %>), HAML comments (-# ...), and HTML
     comments (<!-- ... -->) in view files
   - Prevent false alarms from commented-out code

3. Refactor for code quality:
   - Extract scan_pattern_for_async and file_has_async_pack_tag? helpers
   - Reduce cyclomatic complexity in scan_view_files_for_async_pack_tag
   - All RuboCop checks pass

4. Expand test coverage:
   - Add tests for ERB, HAML, and HTML comment scenarios
   - Add test for commented config with async strategy
   - All 13 async-related tests pass

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

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

Author: justin808

lib/react_on_rails/doctor.rb

@@ -1181,26 +1181,46 @@ def check_async_usage
     end
 
     def scan_view_files_for_async_pack_tag
-      files_with_async = []
-
-      # Scan app/views for .erb and .haml files
       view_patterns = ["app/views/**/*.erb", "app/views/**/*.haml"]
+      files_with_async = view_patterns.flat_map { |pattern| scan_pattern_for_async(pattern) }
+      files_with_async.compact
+    rescue Errno::ENOENT, Encoding::InvalidByteSequenceError, Encoding::UndefinedConversionError => e
+      # Log the error if Rails logger is available
+      Rails.logger.debug("Error scanning view files for async: #{e.message}") if defined?(Rails) && Rails.logger
+      []
+    end
 
-      view_patterns.each do |pattern|
-        Dir.glob(pattern).each do |file|
-          next unless File.exist?(file)
+    def scan_pattern_for_async(pattern)
+      Dir.glob(pattern).filter_map do |file|
+        next unless File.exist?(file)
 
-          content = File.read(file)
-          # Look for javascript_pack_tag with :async or "async"
-          if content.match?(/javascript_pack_tag.*:async/) || content.match?(/javascript_pack_tag.*["']async["']/)
-            files_with_async << relativize_path(file)
-          end
-        end
+        content = File.read(file)
+        next if content_has_only_commented_async?(content)
+        next unless file_has_async_pack_tag?(content)
+
+        relativize_path(file)
+      end
+    end
+
+    def file_has_async_pack_tag?(content)
+      content.match?(/javascript_pack_tag.*:async/) || content.match?(/javascript_pack_tag.*["']async["']/)
+    end
+
+    def content_has_only_commented_async?(content)
+      # Check if all occurrences of javascript_pack_tag with :async are in comments
+      has_uncommented_async = content.each_line.any? do |line|
+        # Skip ERB comments (<%# ... %>)
+        next if line.match?(/<%\s*#.*javascript_pack_tag.*:async/)
+        # Skip HAML comments (-# ...)
+        next if line.match?(/^\s*-#.*javascript_pack_tag.*:async/)
+        # Skip HTML comments (<!-- ... -->)
+        next if line.match?(/<!--.*javascript_pack_tag.*:async.*-->/)
+
+        # Check if line has javascript_pack_tag with :async
+        line.match?(/javascript_pack_tag.*:async/)
       end
 
-      files_with_async
-    rescue StandardError
-      []
+      !has_uncommented_async
     end
 
     def config_has_async_loading_strategy?
@@ -1209,8 +1229,13 @@ def config_has_async_loading_strategy?
 
       content = File.read(config_path)
       # Check if generated_component_packs_loading_strategy is set to :async
-      content.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async/)
-    rescue StandardError
+      # Filter out commented lines to avoid false positives
+      content.each_line.any? do |line|
+        line !~ /^\s*#/ && line.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async/)
+      end
+    rescue Errno::ENOENT, Encoding::InvalidByteSequenceError, Encoding::UndefinedConversionError => e
+      # Log the error if Rails logger is available
+      Rails.logger.debug("Error checking async loading strategy: #{e.message}") if defined?(Rails) && Rails.logger
       false
     end
   end

spec/lib/react_on_rails/doctor_spec.rb

@@ -307,6 +307,54 @@
         expect(files).to be_empty
       end
     end
+
+    context "when async is only in ERB comments" do
+      before do
+        allow(Dir).to receive(:glob).with("app/views/**/*.erb")
+                                    .and_return(["app/views/layouts/application.html.erb"])
+        allow(Dir).to receive(:glob).with("app/views/**/*.haml").and_return([])
+        allow(File).to receive(:exist?).with("app/views/layouts/application.html.erb").and_return(true)
+        allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
+                                     .and_return('<%# javascript_pack_tag "app", :async %>')
+      end
+
+      it "returns empty array" do
+        files = doctor.send(:scan_view_files_for_async_pack_tag)
+        expect(files).to be_empty
+      end
+    end
+
+    context "when async is only in HAML comments" do
+      before do
+        allow(Dir).to receive(:glob).with("app/views/**/*.erb").and_return([])
+        allow(Dir).to receive(:glob).with("app/views/**/*.haml")
+                                    .and_return(["app/views/layouts/application.html.haml"])
+        allow(File).to receive(:exist?).with("app/views/layouts/application.html.haml").and_return(true)
+        allow(File).to receive(:read).with("app/views/layouts/application.html.haml")
+                                     .and_return('-# javascript_pack_tag "app", :async')
+      end
+
+      it "returns empty array" do
+        files = doctor.send(:scan_view_files_for_async_pack_tag)
+        expect(files).to be_empty
+      end
+    end
+
+    context "when async is only in HTML comments" do
+      before do
+        allow(Dir).to receive(:glob).with("app/views/**/*.erb")
+                                    .and_return(["app/views/layouts/application.html.erb"])
+        allow(Dir).to receive(:glob).with("app/views/**/*.haml").and_return([])
+        allow(File).to receive(:exist?).with("app/views/layouts/application.html.erb").and_return(true)
+        allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
+                                     .and_return('<!-- <%= javascript_pack_tag "app", :async %> -->')
+      end
+
+      it "returns empty array" do
+        files = doctor.send(:scan_view_files_for_async_pack_tag)
+        expect(files).to be_empty
+      end
+    end
   end
 
   describe "#config_has_async_loading_strategy?" do
@@ -343,5 +391,17 @@
         expect(doctor.send(:config_has_async_loading_strategy?)).to be false
       end
     end
+
+    context "when :async strategy is commented out" do
+      before do
+        allow(File).to receive(:exist?).with("config/initializers/react_on_rails.rb").and_return(true)
+        allow(File).to receive(:read).with("config/initializers/react_on_rails.rb")
+                                     .and_return("# config.generated_component_packs_loading_strategy = :async")
+      end
+
+      it "returns false" do
+        expect(doctor.send(:config_has_async_loading_strategy?)).to be false
+      end
+    end
   end
 end