Add doctor checks for :async usage without React on Rails Pro #2010
Conversation
WalkthroughAdds development-time diagnostics that scan view templates and the React on Rails initializer for async script-loading usage, and reports errors with file/config locations and guidance when Pro is not installed. Scanning is robust to missing files and encoding issues. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as check_development
participant Doctor as Doctor (lib/react_on_rails/doctor.rb)
participant FS as File system (views + initializer)
participant Reporter as Reporter (add_error / noop)
rect rgba(0,128,128,0.06)
Dev ->> Doctor: check_async_usage
Doctor ->> Doctor: return if Pro installed
alt Pro installed
Doctor ->> Reporter: skip async checks
else Pro not installed
Doctor ->> FS: scan_view_files_for_async_pack_tag()
FS -->> Doctor: list of async view files (may be empty)
Doctor ->> FS: config_has_async_loading_strategy?()
FS -->> Doctor: initializer indicates :async? (bool)
alt any async detected
Doctor ->> Reporter: add_error(details: file paths, config lines, guidance)
else none detected
Doctor ->> Reporter: noop
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #2010: Add doctor checks for :async usage without React on Rails ProSummaryThis PR adds proactive detection of :async loading strategy usage in projects without React on Rails Pro, helping developers identify configuration issues that can cause component registration race conditions. The implementation is solid and well-tested. Strengths
Potential Issues and Suggestions1. Regex Pattern Potential Edge Cases (Minor) Location: lib/react_on_rails/doctor.rb:1195 The regex patterns may miss some edge cases like commented code, multi-line calls, or hash syntax variations (async: true). Consider using the /m flag to match newlines and handle more variations. 2. Performance Consideration (Low Priority) Location: lib/react_on_rails/doctor.rb:1183-1204 Reads entire file contents into memory for each view file. For large projects this could be slow, but since doctor checks run on-demand (not in hot path), current implementation is acceptable. Only optimize if users report performance issues. 3. Configuration File Detection (Minor Edge Case) Location: lib/react_on_rails/doctor.rb:1206-1215 Only checks config/initializers/react_on_rails.rb. Some projects might use different initializer filenames or configure in config/environments/development.rb. Not critical since standard name is well-documented. 4. Test Coverage Gap (Minor) Tests cover async in views only, async in config only, and no async usage. Missing test case for async in BOTH views AND config simultaneously. Security and PerformanceSecurity: No concerns identified - reads files from known safe locations, no user input execution, proper exception handling Performance: Acceptable - doctor checks run on-demand, file globbing efficient for typical Rails apps, early returns prevent unnecessary work Final AssessmentOverall Quality: Excellent (5/5) Recommendation: APPROVE with optional minor enhancements The code is production-ready as-is. All identified issues are minor edge cases that do not block merging. This PR demonstrates thorough testing (156 examples), clear documentation, consistent code quality, helpful user experience, and proper integration with existing systems. Great work! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/react_on_rails/doctor.rb (2)
1151-1181: Consider adding top-level error handling.The method logic is sound and correctly checks for async usage without Pro. However, consider wrapping the entire method body in a rescue clause to ensure that any unexpected errors during scanning don't break the entire doctor run.
Apply this diff to add defensive error handling:
def check_async_usage # When Pro is installed, async is fully supported and is the default behavior # No need to check for async usage in this case return if ReactOnRails::Utils.react_on_rails_pro? async_issues = [] # Check 1: javascript_pack_tag with :async in view files view_files_with_async = scan_view_files_for_async_pack_tag unless view_files_with_async.empty? async_issues << "javascript_pack_tag with :async found in view files:" view_files_with_async.each do |file| async_issues << " • #{file}" end end # Check 2: generated_component_packs_loading_strategy = :async if config_has_async_loading_strategy? async_issues << "config.generated_component_packs_loading_strategy = :async in initializer" end return if async_issues.empty? # Report errors if async usage is found without Pro checker.add_error("🚫 :async usage detected without React on Rails Pro") async_issues.each { |issue| checker.add_error(" #{issue}") } checker.add_info(" 💡 :async can cause race conditions. Options:") checker.add_info(" 1. Upgrade to React on Rails Pro (recommended for :async support)") checker.add_info(" 2. Change to :defer or :sync loading strategy") checker.add_info(" 📖 https://www.shakacode.com/react-on-rails/docs/guides/configuration/") +rescue StandardError => e + checker.add_warning("⚠️ Could not complete async usage check: #{e.message}") + Rails.logger.debug("Async usage check error: #{e.full_message}") if defined?(Rails) && Rails.logger end
1209-1224: Note potential edge case with multi-line comments.The comment filtering logic correctly handles single-line comments in ERB, HAML, and HTML. However, it may not correctly handle multi-line scenarios:
- Multi-line ERB tags split across lines
- Multi-line HTML comments wrapping ERB tags
These edge cases are rare in practice, but if false positives occur, consider using a more sophisticated parsing approach or documenting this limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(2 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/lib/react_on_rails/doctor_spec.rb
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (3)
lib/react_on_rails/doctor.rb (3)
176-176: LGTM!The integration of
check_async_usageinto the development checks flow is well-placed and follows the existing pattern.
1183-1191: LGTM!Good error handling for file scanning operations. The method appropriately catches file and encoding errors, logs them for debugging, and returns an empty array to prevent the doctor run from failing.
1226-1240: LGTM!The config checking logic is sound:
- Correctly filters out commented lines while preserving lines with inline comments (which should still be flagged if they set async)
- Proper regex pattern with flexible whitespace handling
- Appropriate error handling with debug logging
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/react_on_rails/doctor.rb (1)
1221-1247: Consider clarifying multi-line tag comment detection behavior.The method filters out commented async usage by checking if lines containing
javascript_pack_tagare commented. For multi-line tags, this checks the opening line containingjavascript_pack_tag. While the comment on line 1230 explains this, the behavior means that a tag like:<%= javascript_pack_tag "app", :async %>is considered uncommented (correctly), but
<%# javascript_pack_tag "app", :async %>is considered commented (correctly). The implementation is sound, but you might consider adding a test case explicitly documenting the expected behavior for edge cases like when the opening tag is uncommented but contains only the method name, with
:asyncon a subsequent line within the same ERB block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(2 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (8)
spec/lib/react_on_rails/doctor_spec.rb (3)
192-268: Excellent test coverage for async usage detection.The test suite comprehensively covers the Pro-aware behavior, async detection in views and config, and the absence of async usage. The structure is clear and properly isolated with mocks.
270-410: Thorough testing of view file scanning with edge cases.The tests cover various syntax forms (
:async,async: true), false positive prevention (defer: "async"), comment scenarios across ERB/HAML/HTML, and multi-line tag handling. This comprehensive coverage ensures the scanner behaves correctly across different template formats.
412-458: Complete config strategy detection test coverage.The tests validate detection of
:asyncstrategy, differentiation from other strategies (:defer), handling of missing config files, and proper filtering of commented-out configuration. This ensures robust config file parsing.lib/react_on_rails/doctor.rb (5)
176-176: Good integration of async check into development diagnostics.The new
check_async_usagemethod is appropriately placed in the development checks section, ensuring it runs during normal doctor diagnosis.
1151-1181: Well-implemented Pro-aware async usage validation.The implementation correctly:
- Skips validation when Pro is installed (async is supported)
- Aggregates findings from both view files and config
- Reports clear error messages with actionable guidance
- Provides documentation links for resolution
The early return for Pro installations prevents false positives and aligns with the learning that Pro features should be validated through proper channels.
Based on learnings
1183-1203: Robust view file scanning with proper error handling.The implementation scans both ERB and HAML templates, filters out commented usage, relativizes paths for display, and includes appropriate error handling for file system and encoding issues. The use of
Rails.logger.debugfor error logging is appropriate for a diagnostic tool.
1205-1219: Regex pattern correctly addresses past review feedback.The single regex pattern
/javascript_pack_tag[^<]*(?::async\b|async:\s*true)/correctly matches:
:asyncsymbol syntaxasync: truehash syntaxThe pattern appropriately avoids false positives like
defer: "async"(where "async" is a string value, not the async option). The use of word boundary\band the[^<]*construct for matching within ERB tags handles multi-line tags correctly.The past review comment regarding the overly broad second regex pattern has been properly addressed in commit 9f6dc24.
1249-1268: Solid config detection with appropriate filtering.The method correctly:
- Filters out commented lines before checking for
:asyncstrategy- Uses word boundary
\bto match the complete symbol, not partial matches like:async_mode- Includes error handling for file system and encoding errors
- Returns
falseon errors (safe default)The implementation properly detects the async loading strategy while avoiding false positives from commented configuration.
Adds proactive detection of :async loading strategy usage in projects without React on Rails Pro, which can cause component registration race conditions. The doctor now checks for: 1. javascript_pack_tag with :async in view files 2. config.generated_component_packs_loading_strategy = :async in initializer When detected without Pro, provides clear guidance: - Upgrade to React on Rails Pro (recommended) - Change to :defer or :sync loading strategy This complements PR #1993's configuration validation by adding runtime doctor checks that help users identify and fix async usage issues during development. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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 <[email protected]>
…nsive 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 <[email protected]>
justin808
force-pushed
the
justin808/doctor-check-async-requires-pro
branch
from
9f6dc24 to
4cdff17
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(2 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/doctor.rbspec/lib/react_on_rails/doctor_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/react_on_rails/doctor.rbspec/lib/react_on_rails/doctor_spec.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/doctor.rbspec/lib/react_on_rails/doctor_spec.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (9)
lib/react_on_rails/doctor.rb (6)
176-176: LGTM!The integration of
check_async_usageinto the development checks flow is well-placed and consistent with other diagnostic checks.
1151-1181: LGTM!The
check_async_usagemethod correctly implements the Pro feature gate and provides clear, actionable error messages with guidance. The early return when Pro is installed is appropriate, and the aggregation of issues before reporting is well-structured.
1183-1191: LGTM!Robust error handling with appropriate fallback behavior for a diagnostic tool. The debug logging helps troubleshooting without cluttering output.
1193-1203: LGTM!The filtering logic is efficient and correctly skips files where async is only in comments. Using
filter_mapis a good choice for combining filter and map operations.
1205-1219: Well-crafted regex pattern with excellent documentation.The regex correctly matches both
:asyncsymbol andasync: truehash syntax while avoiding false positives. The word boundary\bensures it doesn't match longer symbols like:async_mode. The documented theoretical edge casedata: { async: true }is acceptable as it would only produce a harmless warning in extremely unlikely scenarios.
1249-1268: LGTM!The config detection logic correctly filters out commented lines and uses word boundaries to ensure precise matching. Error handling with debug logging is appropriate for a diagnostic tool.
spec/lib/react_on_rails/doctor_spec.rb (3)
192-268: LGTM!Comprehensive test coverage for the Pro/non-Pro branching and async detection in both view files and initializer config. The tests correctly verify error reporting and the no-issue scenarios.
270-410: Good test coverage for common scenarios.The tests comprehensively cover detection of
:asyncsymbol,async: truehash syntax, false positive prevention fordefer: "async", various comment styles (ERB, HAML, HTML), and multi-line tags.Note: Additional test coverage is needed for the false positive scenario identified in the implementation review (uncommented
javascript_pack_tagwithout:asynccoexisting with commented:async).
412-458: LGTM!Complete test coverage for all branches of
config_has_async_loading_strategy?, including the :async detection, different strategies, missing file, and commented-out configuration scenarios.
Extract comment pattern regexes to named constants for better maintainability and reusability. Add defensive log_debug helper method to safely handle Rails.logger access. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/react_on_rails/doctor.rb(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (7)
lib/react_on_rails/doctor.rb (7)
1151-1154: LGTM! Well-organized comment pattern constants.The comment patterns for ERB, HAML, and HTML are clearly defined and properly support the filtering logic used later in the async detection.
1156-1186: LGTM! Well-structured async usage detection.The method correctly:
- Skips checks when Pro is installed (async is fully supported)
- Aggregates issues from both view files and config
- Provides clear error messages with actionable guidance
- Links to relevant documentation
1188-1196: LGTM! Robust file scanning with appropriate error handling.The method properly scans ERB and HAML files, handles file system and encoding errors gracefully, and logs issues for debugging.
1198-1208: LGTM! Clean file processing logic.The method correctly filters files by checking for uncommented async usage and returns relativized paths for better readability.
1210-1224: LGTM! Well-designed regex pattern with clear documentation.The regex correctly matches both
:asyncsymbol andasync: truehash syntax while using word boundaries to avoid false positives. The[^<]*pattern properly handles multi-line tags within ERB blocks, and the comment thoroughly explains edge cases.
1254-1273: LGTM! Correct config parsing with proper comment filtering.The method correctly:
- Checks file existence before reading
- Filters out commented lines
- Uses word boundary to match the complete
:asyncsymbol- Handles errors gracefully with safe fallback
1275-1279: LGTM! Clean logging helper with proper guards.The method safely logs debug messages only when Rails.logger is available, preventing errors in environments where logging may not be configured.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review CompleteTasks:
SummaryThis PR adds proactive detection for Critical Issue: False Positive Bug 🔴Location: Problem: The
Example that triggers false positive: <%= javascript_pack_tag "app", defer: true %>
<%# javascript_pack_tag "other", :async %>Current behavior: Reports async usage ❌ (false positive) Root cause: Line 1238 checks Fix required: # Line 1236-1238 should be:
has_uncommented_async = content.each_line.any? do |line|
# Skip lines that don't contain javascript_pack_tag with :async
next unless file_has_async_pack_tag?(line) # Changed from line.include?("javascript_pack_tag")
# Skip ERB comments (<%# ... %>)
# ... rest of the codeMissing test coverage: There's no test case for this specific scenario. The existing test Other Review FindingsStrengths ✅
Code Quality Observations
Minor Suggestions (non-blocking)
Recommendation❌ DO NOT MERGE until the critical false positive bug is fixed. Required changes:
After fix:
Test Case to Addcontext "when file has uncommented pack tag without :async and commented one with :async" 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", defer: true %>
<%# javascript_pack_tag "other", :async %>')
end
it "does not return files (only commented line has :async)" do
files = doctor.send(:scan_view_files_for_async_pack_tag)
expect(files).to be_empty
end
endGreat work on the comprehensive testing and thoughtful implementation! Just need to fix this one bug and you're good to go. |
… tags Critical bug fix: The content_has_only_commented_async? method was incorrectly flagging files that had: - Uncommented javascript_pack_tag WITHOUT :async - Commented javascript_pack_tag WITH :async The previous implementation checked if ANY uncommented javascript_pack_tag existed, not specifically ones with :async. This caused false positives. Solution: Refactored to remove all commented lines first, then check if :async exists in the remaining content. This handles both single-line and multi-line tags correctly and is more robust. Added comprehensive test case to prevent regression. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #2010: Add doctor checks for :async usage without React on Rails ProThank you for this well-implemented feature! I've reviewed the changes and overall the implementation is solid with excellent test coverage. Here's my detailed feedback: ✅ Strengths
🔍 Potential Issues & Suggestions1. Performance ConsiderationLocation: The Suggestion: def scan_view_files_for_async_pack_tag
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_debug("Error scanning view files for async: #{e.message}")
[]
endConsider adding early exit if no files match patterns, though this is likely negligible in practice. 2. Regex Pattern Potential Edge CaseLocation: The regex Potential Issue: <%= javascript_pack_tag "app", defer: true
<div>Some content</div>
<%= some_other_tag :async %>Analysis: This is extremely unlikely in real code and would be caught by the comment detection logic, but worth noting. Suggestion (optional): Consider using 3. Missing Edge Case in TestsObservation: Tests cover many scenarios but could add: context "when file contains async: false (explicitly disabled)" do
before do
# Test that async: false doesn't trigger false positive
allow(File).to receive(:read).with("app/views/layouts/application.html.erb")
.and_return('<%= javascript_pack_tag "app", async: false %>')
end
it "does not return files (async is explicitly disabled)" do
files = doctor.send(:scan_view_files_for_async_pack_tag)
expect(files).to be_empty
end
endImpact: Low - the regex 4. Potential Enhancement: Line NumbersLocation: When reporting files with async usage, it could be helpful to include line numbers: async_issues << " • #{file}:#{line_number}"This would help developers quickly locate the issue in large view files. Trade-off: Requires refactoring to track line numbers during scanning, so maybe not worth the complexity for this use case. 🛡️ Security Considerations✅ No security concerns identified
📊 Code Quality
🎯 Testing Recommendations
📝 Minor Nitpicks
✅ RecommendationAPPROVE with minor suggestions. This is production-ready code that:
The suggestions above are optimizations and enhancements, not blockers. 📚 Additional NotesPer
Great work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/react_on_rails/doctor.rb (1)
1210-1224: Well-crafted regex with thorough documentation.The regex correctly handles both
:asyncsymbol andasync: truehash syntax with word boundaries preventing false positives. The detailed comment is helpful.One rare edge case:
[^<]*stops at<characters, so string literals containing<in arguments (e.g.,data: { message: "<hello>" }) would prevent matching:asyncafter them. This is extremely unlikely in practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(2 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/lib/react_on_rails/doctor_spec.rb
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (7)
lib/react_on_rails/doctor.rb (7)
176-176: LGTM - Clean integration into development checks.The async usage check is appropriately placed alongside other development environment validations.
1150-1155: Well-designed comment detection patterns.The patterns correctly handle ERB, HAML, and HTML comment syntaxes to filter out commented async usage.
1156-1186: Excellent validation logic with helpful guidance.The Pro check correctly prevents false positives, and the error messages provide actionable guidance with clear upgrade/alternative options.
1188-1208: Solid file scanning implementation.Good use of
filter_mapfor efficiency, appropriate error handling for file-related exceptions, and relative paths for user-friendly output.
1226-1244: Correct comment filtering logic.The implementation correctly handles the scenario from past review comments: when uncommented
javascript_pack_tagexists alongside commented async usage, it properly filters the commented lines and only checks uncommented content for async. This resolves the previous false-positive concern.
1246-1265: Robust config detection with proper comment handling.The method correctly filters commented config lines and uses word boundaries to avoid false matches with similar symbols like
:async_mode.
1267-1271: LGTM - Defensive logging helper.Proper guard against missing Rails.logger for robust operation across different runtime contexts.
Extended the async detection functionality to support Slim templates in addition to ERB and HAML: - Added SLIM_COMMENT_PATTERN constant for Slim comment syntax (/) - Added app/views/**/*.slim to view file scanning patterns - Included Slim comment filtering in content_has_only_commented_async? - Added comprehensive test cases for Slim files: * Slim files with uncommented :async (should detect) * Slim files with commented :async (should not detect) - Added slim stub mocks to all existing test cases for consistency This ensures users of Slim templates get the same async detection warnings as ERB and HAML users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/react_on_rails/doctor.rb (3)
1151-1156: Comment patterns cover common single-line comment syntaxes but miss multi-line blocks.The ERB/HAML/SLIM/HTML comment patterns are correct for single-line comments and align with the specs. However, they will not filter async usage inside multi-line HTML or HAML/SLIM block comments (e.g.,
-#or/followed by indented lines), which could produce rare false positives.If you want to be stricter, consider extending the filtering logic in
content_has_only_commented_async?to track HAML/SLIM block comments and multi-line HTML comments, rather than relying on per-line regexes alone. This can be done by walkingeach_linewith a simple state machine for “inside block comment”.- ERB_COMMENT_PATTERN = /<%\s*#.*javascript_pack_tag/ - HAML_COMMENT_PATTERN = /^\s*-#.*javascript_pack_tag/ - SLIM_COMMENT_PATTERN = %r{^\s*/.*javascript_pack_tag} - HTML_COMMENT_PATTERN = /<!--.*javascript_pack_tag/ + ERB_COMMENT_PATTERN = /<%\s*#.*javascript_pack_tag/ + HAML_COMMENT_PATTERN = /^\s*-#.*javascript_pack_tag/ + SLIM_COMMENT_PATTERN = %r{^\s*/.*javascript_pack_tag} + HTML_COMMENT_PATTERN = /<!--.*javascript_pack_tag/ + + # NOTE: These patterns only handle single-line comments. For block comments + # (e.g., HAML `-#` or HTML `<!--` with async on a following line), we rely + # on additional logic in content_has_only_commented_async?.
1227-1246: Behavior and comment forcontent_has_only_commented_async?are slightly out of sync.The method currently returns
truewhen:
- there is only commented async usage, or
- there is no async usage at all (because of the early
return true unless file_has_async_pack_tag?(content)).This matches how it’s used (skip files with no active async), but the comments say “ONLY commented async usage exists”. To avoid confusion for future maintainers, consider clarifying the docstring to reflect both “none” and “commented-only” cases.
- def content_has_only_commented_async?(content) - # Check if all occurrences of javascript_pack_tag with :async are in comments - # Returns true if ONLY commented async usage exists (no active async usage) + def content_has_only_commented_async?(content) + # Check if there is any *active* javascript_pack_tag with :async. + # Returns true when there is no active async usage (either none at all, + # or all occurrences are in comments).
1248-1267: Config detection works for common cases; can be made more resilient to multi-line assignments.
config_has_async_loading_strategy?correctly ignores commented lines and detectsconfig.generated_component_packs_loading_strategy = :asyncon a single line. However, it will miss a configuration split across lines (e.g.,config.generated_component_packs_loading_strategy =\n :async), which is valid Ruby and not uncommon in longer configs.You can handle this by stripping commented lines and applying a single regex over the whole string instead of per-line matching;
\s*will naturally consume newlines.- def config_has_async_loading_strategy? - config_path = "config/initializers/react_on_rails.rb" - return false unless File.exist?(config_path) - - content = File.read(config_path) - # Check if generated_component_packs_loading_strategy is set to :async - # Filter out commented lines (lines starting with # after optional whitespace) - content.each_line.any? do |line| - # Skip lines that start with # (after optional whitespace) - next if line.match?(/^\s*#/) - - # Match: config.generated_component_packs_loading_strategy = :async - # Use word boundary \b to ensure :async is the complete symbol, not part of :async_mode etc. - line.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async\b/) - end - rescue Errno::ENOENT, Encoding::InvalidByteSequenceError, Encoding::UndefinedConversionError => e + def config_has_async_loading_strategy? + config_path = "config/initializers/react_on_rails.rb" + return false unless File.exist?(config_path) + + content = File.read(config_path) + + # Remove commented-out lines, then look for an :async assignment (handles multi-line) + uncommented = content.each_line.reject { |line| line.match?(/^\s*#/) }.join + uncommented.match?(/config\.generated_component_packs_loading_strategy\s*=\s*:async\b/) + rescue Errno::ENOENT, Encoding::InvalidByteSequenceError, Encoding::UndefinedConversionError => e # Log the error if Rails logger is available log_debug("Error checking async loading strategy: #{e.message}") false endspec/lib/react_on_rails/doctor_spec.rb (2)
271-476: Comprehensive view-scanning specs with good regression coverage.The tests around
#scan_view_files_for_async_pack_tagare thorough:
- They validate both
:asyncandasync: true.- They guard against the
defer: "async"false positive.- They verify commented-only async across ERB/HAML/HTML/SLIM, the mixed commented/uncommented scenario, Slim templates, and multi-line pack tags.
These examples align well with the helper methods and would have caught the earlier false-positive issues. Once you extend comment handling for multi-line HTML/HAML/SLIM block comments (if you choose to), adding a couple of specs for those patterns will slot naturally into this block.
478-524: Initializer config specs mirror the doctor behavior well.The tests for
#config_has_async_loading_strategy?cover:
- positive detection for
:async,- negative for
:defer,- missing config file, and
- commented-out async.
This matches the current implementation. If you adopt the suggested multi-line assignment handling in the implementation, consider adding a spec for a split assignment (
= \n :async) to lock in that behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/doctor.rb(2 hunks)spec/lib/react_on_rails/doctor_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
spec/lib/react_on_rails/doctor_spec.rblib/react_on_rails/doctor.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/react_on_rails/doctor.rb
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
lib/react_on_rails/doctor.rb
🧬 Code graph analysis (1)
lib/react_on_rails/doctor.rb (2)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(256-264)lib/react_on_rails/system_checker.rb (2)
add_error(16-18)add_info(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (8)
lib/react_on_rails/doctor.rb (6)
171-177: Good integration of async check into development diagnostics.Hooking
check_async_usageintocheck_developmentensures async misuse is surfaced alongside other dev-time issues without affecting other sections. No changes needed here.
1157-1187: Async usage gating correctly respects Pro installations and provides actionable guidance.The
check_async_usageflow looks solid: it short-circuits whenReactOnRails::Utils.react_on_rails_pro?is true, aggregates findings from both view scanning and config, and reports a single primary error plus detailed bullets and remediation steps. The messaging is clear and consistent with the docs and Pro behavior. No changes needed.
1189-1197: View scanning is robust and safely handles IO/encoding errors.
scan_view_files_for_async_pack_tagcovers ERB/HAML/SLIM, delegates toscan_pattern_for_async, and gracefully logs and ignores filesystem/encoding errors vialog_debug. This matches the goal of a best-effort doctor check without crashing the app. Looks good.
1199-1209: Efficient pattern-based scanning with correct path relativization.Using
Dir.glob(pattern).filter_mapwithcontent_has_only_commented_async?andfile_has_async_pack_tag?keeps scanning simple and efficient, and returningrelativize_path(file)gives user-friendly output. This is a good balance between simplicity and usability.
1211-1225: Regex for detecting async pack tags is appropriate and avoids known false positives.The pattern
/javascript_pack_tag[^<]*(?::async\b|async:\s*true)/correctly matches both:asyncandasync: true, supports multi-line tags (as covered by specs), and avoids string-valuedasynclikedefer: "async". The word boundary on:asyncalso prevents matching:async_mode. The noteddata: { async: true }case is an acceptable, documented false positive. No changes needed.
1269-1273:log_debughelper is safe and keeps doctor logs unobtrusive.Guarding on
defined?(Rails.logger) && Rails.loggerbefore calling.debugprevents noisy errors when Rails isn’t fully loaded. This is an appropriate lightweight logging strategy for doctor internals.spec/lib/react_on_rails/doctor_spec.rb (2)
192-212: Solid coverage of Pro vs non‑Pro branching incheck_async_usage.The specs correctly verify that the check is skipped when
react_on_rails_pro?is true and that no errors/warnings are recorded in that case, which guards against unnecessary noise for Pro users.
214-268: Clear scenarios for async usage in views vs config and “no issue” case.These examples nicely exercise:
- async usage in a view file,
- async loading strategy configured in the initializer, and
- a defer-only config that should not report errors.
The expectations match the implementation’s error messages and give good confidence that user-facing doctor output is stable. No changes needed.
Summary
Adds proactive doctor checks to detect
:asyncloading strategy usage in projects without React on Rails Pro, which can cause component registration race conditions.Changes
The doctor now checks for and reports errors when it detects:
javascript_pack_tagwith:asyncin view filesconfig.generated_component_packs_loading_strategy = :asyncin initializerWhen detected without Pro, provides clear guidance to either upgrade to Pro or use
:defer/:syncloading strategies.This complements PR #1993's configuration validation by adding runtime detection during development to help developers identify and fix async usage issues.
Testing
All new tests pass (155+ examples in doctor spec). The checks are skipped when React on Rails Pro is installed.
This change is
Summary by CodeRabbit
New Features
Tests