Skip to content

Commit 9f6dc24

Browse files
justin808claude
andcommitted
Improve doctor async checks: enhance regex precision and add comprehensive tests
- Remove overly broad string form regex that could cause false positives - Add support for both :async symbol and async: true hash syntax - Add word boundary \b to prevent matching :async_mode or similar - Improve multi-line javascript_pack_tag detection - Add comprehensive inline documentation with examples - Add test for async: true pattern detection - Add test to confirm defer: "async" does not trigger false positive - Add test for multi-line javascript_pack_tag calls - Improve comment filtering logic to handle multi-line tags correctly - Use String#include? for better performance (RuboCop compliance) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 8382e94 commit 9f6dc24

File tree

2 files changed

+91
-11
lines changed

2 files changed

+91
-11
lines changed

lib/react_on_rails/doctor.rb

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,21 +1203,44 @@ def scan_pattern_for_async(pattern)
12031203
end
12041204

12051205
def file_has_async_pack_tag?(content)
1206-
content.match?(/javascript_pack_tag.*:async/) || content.match?(/javascript_pack_tag.*["']async["']/)
1206+
# Match javascript_pack_tag with :async symbol or async: true hash syntax
1207+
# Examples that should match:
1208+
# - javascript_pack_tag "app", :async
1209+
# - javascript_pack_tag "app", async: true
1210+
# - javascript_pack_tag "app", :async, other_option: value
1211+
# Examples that should NOT match:
1212+
# - javascript_pack_tag "app", defer: "async" (async is a string value, not the option)
1213+
# - javascript_pack_tag "app", :defer
1214+
# Note: Theoretical edge case `data: { async: true }` would match but is extremely unlikely
1215+
# in real code and represents a harmless false positive (showing a warning when not needed)
1216+
# Use word boundary \b to ensure :async is not part of a longer symbol like :async_mode
1217+
# [^<]* allows matching across newlines within ERB tags but stops at closing ERB tag
1218+
content.match?(/javascript_pack_tag[^<]*(?::async\b|async:\s*true)/)
12071219
end
12081220

12091221
def content_has_only_commented_async?(content)
12101222
# Check if all occurrences of javascript_pack_tag with :async are in comments
1223+
# Returns true if ONLY commented async usage exists (no active async usage)
1224+
# Note: We need to check the full content first (for multi-line tags) before line-by-line filtering
1225+
1226+
# First check if there's any javascript_pack_tag with :async in the full content
1227+
return true unless file_has_async_pack_tag?(content)
1228+
1229+
# Now check line-by-line to see if all occurrences are commented
1230+
# For multi-line tags, we check if the starting line is commented
12111231
has_uncommented_async = content.each_line.any? do |line|
1212-
# Skip ERB comments (<%# ... %>)
1213-
next if line.match?(/<%\s*#.*javascript_pack_tag.*:async/)
1214-
# Skip HAML comments (-# ...)
1215-
next if line.match?(/^\s*-#.*javascript_pack_tag.*:async/)
1216-
# Skip HTML comments (<!-- ... -->)
1217-
next if line.match?(/<!--.*javascript_pack_tag.*:async.*-->/)
1232+
# Skip lines that don't contain javascript_pack_tag
1233+
next unless line.include?("javascript_pack_tag")
12181234

1219-
# Check if line has javascript_pack_tag with :async
1220-
line.match?(/javascript_pack_tag.*:async/)
1235+
# Skip ERB comments (<%# ... %>) - matches ERB comment opening with optional whitespace
1236+
next if line.match?(/<%\s*#.*javascript_pack_tag/)
1237+
# Skip HAML comments (-# ...) - matches line-starting HAML comments
1238+
next if line.match?(/^\s*-#.*javascript_pack_tag/)
1239+
# Skip HTML comments (<!-- ... -->) - matches complete HTML comment blocks
1240+
next if line.match?(/<!--.*javascript_pack_tag/)
1241+
1242+
# If we reach here, this line has an uncommented javascript_pack_tag
1243+
true
12211244
end
12221245

12231246
!has_uncommented_async
@@ -1229,9 +1252,14 @@ def config_has_async_loading_strategy?
12291252

12301253
content = File.read(config_path)
12311254
# Check if generated_component_packs_loading_strategy is set to :async
1232-
# Filter out commented lines to avoid false positives
1255+
# Filter out commented lines (lines starting with # after optional whitespace)
12331256
content.each_line.any? do |line|
1234-
line !~ /^\s*#/ && line.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async/)
1257+
# Skip lines that start with # (after optional whitespace)
1258+
next if line.match?(/^\s*#/)
1259+
1260+
# Match: config.generated_component_packs_loading_strategy = :async
1261+
# Use word boundary \b to ensure :async is the complete symbol, not part of :async_mode etc.
1262+
line.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async\b/)
12351263
end
12361264
rescue Errno::ENOENT, Encoding::InvalidByteSequenceError, Encoding::UndefinedConversionError => e
12371265
# Log the error if Rails logger is available

spec/lib/react_on_rails/doctor_spec.rb

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,40 @@
292292
end
293293
end
294294

295+
context "when view files contain javascript_pack_tag with async: true" do
296+
before do
297+
allow(Dir).to receive(:glob).with("app/views/**/*.erb")
298+
.and_return(["app/views/layouts/application.html.erb"])
299+
allow(Dir).to receive(:glob).with("app/views/**/*.haml").and_return([])
300+
allow(File).to receive(:exist?).with("app/views/layouts/application.html.erb").and_return(true)
301+
allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
302+
.and_return('<%= javascript_pack_tag "app", async: true %>')
303+
allow(doctor).to receive(:relativize_path).with("app/views/layouts/application.html.erb")
304+
.and_return("app/views/layouts/application.html.erb")
305+
end
306+
307+
it "returns files with async" do
308+
files = doctor.send(:scan_view_files_for_async_pack_tag)
309+
expect(files).to include("app/views/layouts/application.html.erb")
310+
end
311+
end
312+
313+
context "when view files contain defer: \"async\" (false positive check)" do
314+
before do
315+
allow(Dir).to receive(:glob).with("app/views/**/*.erb")
316+
.and_return(["app/views/layouts/application.html.erb"])
317+
allow(Dir).to receive(:glob).with("app/views/**/*.haml").and_return([])
318+
allow(File).to receive(:exist?).with("app/views/layouts/application.html.erb").and_return(true)
319+
allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
320+
.and_return('<%= javascript_pack_tag "app", defer: "async" %>')
321+
end
322+
323+
it "does not return files (async is a string value, not the option)" do
324+
files = doctor.send(:scan_view_files_for_async_pack_tag)
325+
expect(files).to be_empty
326+
end
327+
end
328+
295329
context "when view files do not contain async" do
296330
before do
297331
allow(Dir).to receive(:glob).with("app/views/**/*.erb")
@@ -355,6 +389,24 @@
355389
expect(files).to be_empty
356390
end
357391
end
392+
393+
context "when javascript_pack_tag spans multiple lines" do
394+
before do
395+
allow(Dir).to receive(:glob).with("app/views/**/*.erb")
396+
.and_return(["app/views/layouts/application.html.erb"])
397+
allow(Dir).to receive(:glob).with("app/views/**/*.haml").and_return([])
398+
allow(File).to receive(:exist?).with("app/views/layouts/application.html.erb").and_return(true)
399+
allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
400+
.and_return("<%= javascript_pack_tag \"app\",\n :async %>")
401+
allow(doctor).to receive(:relativize_path).with("app/views/layouts/application.html.erb")
402+
.and_return("app/views/layouts/application.html.erb")
403+
end
404+
405+
it "returns files with async" do
406+
files = doctor.send(:scan_view_files_for_async_pack_tag)
407+
expect(files).to include("app/views/layouts/application.html.erb")
408+
end
409+
end
358410
end
359411

360412
describe "#config_has_async_loading_strategy?" do

0 commit comments

Comments
 (0)