Fix component registration race condition with defer loading strategy

[justin808]

Problem

CI tests were failing intermittently with component registration errors:

This was caused by a race condition where React hydration started before component registrations completed.

Root Cause

With script loading (default on Shakapacker >= 8.2.0):

• Both and generated component packs used
• Scripts execute as soon as downloaded, in any order
• When main bundle executed first, React couldn't find registered components

Solution

Changed default loading strategy to for deterministic execution order:

1. Configuration - Default to (was )
2. Layout - Use for (was conditional async/defer)
3. Result - Scripts execute in DOM order after HTML parsing

With :

• All scripts download in parallel (fast)
• Execute in DOM order after parsing
• Generated component packs execute before main bundle
• Component registrations complete before React hydration

Testing

Fixes CI failures in:

• spec/system/integration_spec.rb[1:1:6:1:2]
• spec/system/integration_spec.rb[1:1:6:2:2]

Files Changed

• lib/react_on_rails/configuration.rb - Changed default strategy to :defer
• spec/dummy/app/views/layouts/application.html.erb - Always use defer for client-bundle
• spec/dummy/config/initializers/react_on_rails.rb - Updated comments

---

🤖 Generated with Claude Code

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

---

This change is 

Summary by CodeRabbit

• Bug Fixes

  • Fixed component registration timing to ensure completion before hydration

• Refactor

  • Simplified component loading strategy to consistently use defer behavior
  • Removed automatic async loading detection for more predictable configuration

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 57 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 5abe219 and fe22829.

📒 Files selected for processing (1)

• spec/react_on_rails/configuration_spec.rb (2 hunks)

Walkthrough

This PR changes the default component bundle loading strategy from potentially asynchronous to consistently deferred, ensuring component registrations complete before hydration. Removes automatic async-enabling logic and simplifies script tag behavior in the dummy app configuration.

Changes

Cohort / File(s)	Summary
Default loading strategy lib/react_on_rails/configuration.rb	Changed default for generated_component_packs_loading_strategy from conditional :async to :defer; removed automatic async-enabling logic when supported; tightened error handling to raise when :async is requested but unsupported; removed PerceivedComplexity from RuboCop directives
Dummy app layout spec/dummy/app/views/layouts/application.html.erb	Replaced conditional defer/async selection logic with unconditional defer: true for client bundle script execution
Dummy app configuration spec/dummy/config/initializers/react_on_rails.rb	Removed explicit config.generated_component_packs_loading_strategy = :defer setting; now relies on framework default behavior

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Key area: Verify the default behavior change in configuration.rb is correct and understand why automatic async-enabling was problematic
• Confirm error-handling path for unsupported async requests functions as intended

Possibly related PRs

• Add configurable loading strategy for generated component packs #1712: Introduces the automatic async-enabling logic that this PR removes; both touch generated_component_packs_loading_strategy defaults
• Document defer script impact on streaming and Selective Hydration #1927: Modifies script-loading behavior and generated_component_packs_loading_strategy references including dummy app layout changes
• Fix Turbo navigation script tag #1906: Updates dummy app layout script tag to use defer: true for client bundle loading

Suggested labels

bug

Suggested reviewers

• alexeyr-ci
• AbanoubGhadban
• Judahmeek
• alexeyr-ci2

Poem

🐰 The defer hops steady, no more async race,
Components register at their proper pace,
Defaults now certain, no guessing the game,
React on Rails runs cleaner, forever the same! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the main objective: fixing a component registration race condition by using the defer loading strategy.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #1972

Summary

This PR addresses an intermittent race condition in CI tests by changing the default loading strategy for generated component packs from :async to :defer. The fix is well-thought-out and properly addresses the root cause.

---

✅ Strengths

1. Root Cause Analysis: Excellent diagnosis of the race condition. The PR clearly explains how defer scripts execute in DOM order after HTML parsing, preventing the main bundle from executing before component registrations complete.

2. Simplified Logic: The refactored configuration code is cleaner and easier to understand:

   • Removed complex conditional logic around Shakapacker version detection for default value
   • Now defaults to :defer unconditionally, with clear rationale in comments
   • Simplified rubocop disable comments (removed unnecessary Metrics/PerceivedComplexity)

3. Comprehensive Changes: The PR addresses the issue at multiple levels:

   • Configuration default
   • Layout template
   • Example initializer comments

4. Documentation: Good inline comments explaining the "why" behind using :defer.

---

🔍 Potential Issues & Questions

1. Missing Test Coverage (Medium Priority)

The configuration validation logic is changed but I don't see corresponding unit tests added/updated. Consider adding tests to spec/react_on_rails/configuration_spec.rb:

describe '.generated_component_packs_loading_strategy' do
  context 'when not explicitly set' do
    it 'defaults to :defer' do
      ReactOnRails.configure do |config|
        config.auto_load_bundle = true
      end
      expect(ReactOnRails.configuration.generated_component_packs_loading_strategy).to eq(:defer)
    end
  end
  
  context 'when explicitly set to :async' do
    context 'with Shakapacker < 8.2.0' do
      it 'raises an error' do
        allow(ReactOnRails::PackerUtils).to receive(:supports_async_loading?).and_return(false)
        expect do
          ReactOnRails.configure do |config|
            config.generated_component_packs_loading_strategy = :async
          end
        end.to raise_error(ReactOnRails::Error, /does not support async script loading/)
      end
    end
  end
end

2. Breaking Change Consideration (Low Priority)

While this is the right default, existing users who explicitly configured their apps for :async behavior may be surprised by this change. Consider:

• Adding a note to CHANGELOG.md about this being a behavior change (even though it only affects the default)
• Updating migration/upgrade guide if one exists

3. Removed uses_redux_shared_store? Logic (Question)

The layout previously had conditional logic based on uses_redux_shared_store?:

• With Redux shared stores: used defer: true
• Without Redux: used async: true

Now everything uses defer: true. Questions:

• Is this intentional for ALL pages, or only for the test dummy app?
• Could this impact performance for production apps that don't use auto_load_bundle?
• The helper method uses_redux_shared_store? is still defined in application_controller.rb:23-40 but no longer used - should it be removed?

4. Documentation Updates Needed (Medium Priority)

The PR mentions docs/building-features/streaming-server-rendering.md in the grep results. That file likely needs updates to reflect:

• The new default behavior
• When developers might still want :async (if ever)
• Performance implications of :defer vs :async

---

🔒 Security Concerns

None identified. This is a client-side script loading strategy change with no security implications.

---

⚡ Performance Considerations

1. Positive: :defer still allows parallel downloading of scripts (unlike :sync)
2. Trade-off: Scripts execute after HTML parsing completes rather than immediately when downloaded
   • For small apps: Negligible difference
   • For large apps with heavy DOM: Could delay JS execution by a few milliseconds vs :async
3. Overall: The reliability gain from preventing race conditions far outweighs the minimal performance trade-off

---

📝 Recommendations

Must Do Before Merge:

1. Add unit tests for the new default behavior in configuration_spec.rb
2. Update CHANGELOG.md with a note about this default change (if not already done)
3. Decide on uses_redux_shared_store?: Either remove the unused helper method or document why it's kept

Should Do:

1. Update documentation in docs/building-features/streaming-server-rendering.md to reflect new defaults
2. Review Pro package: Check if react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb needs similar changes
3. Add integration test: Consider a specific test that verifies component registration completes before hydration

Nice to Have:

1. Add a comment in the code explaining why :async was avoided (reference to this PR or issue)
2. Document the performance characteristics of each loading strategy in the configuration docs

---

🎯 Conclusion

This is a solid fix that addresses a real race condition with a sensible default. The code changes are clean and the reasoning is sound.

Recommendation: Approve with minor changes (add tests + update docs)

The fix correctly identifies that deterministic execution order is more important than the marginal performance benefit of :async loading, especially given that :defer still provides parallel downloads.

---

Code Quality Score: 8.5/10

• ✅ Solves the problem correctly
• ✅ Clean, readable code
• ✅ Good inline documentation
• ⚠️ Missing test coverage for the change
• ⚠️ Documentation updates needed

Great work on identifying and fixing this race condition! 🎉

[claude]

Code Review - PR #1972: Fix component registration race condition with defer loading strategy

Summary

This PR addresses an important race condition issue with component registration when using the defer loading strategy. The fix is well-reasoned and properly addresses the root cause.

---

✅ Strengths

1. Clear Problem Identification

The PR correctly identifies the race condition with async script loading where:

• Both client-bundle and generated component packs use async
• Scripts execute in download order (non-deterministic)
• Component registrations may not complete before React hydration begins

2. Appropriate Solution

Changing the default to :defer ensures:

• Scripts download in parallel (maintaining performance)
• Execution happens in DOM order (deterministic)
• Generated component packs always execute before main bundle
• Component registrations complete before hydration

3. Comprehensive Test Updates

The test changes correctly reflect the new default behavior across different Shakapacker versions.

4. Code Simplification

The refactoring in configuration.rb (lines 179-185) is cleaner:

• Removes the conditional based on supports_async_loading?
• Simplifies the logic flow
• Maintains backward compatibility with explicit configuration

---

⚠️ Concerns & Recommendations

1. Documentation Consistency Issue (High Priority)

Problem: The documentation in docs/building-features/streaming-server-rendering.md explicitly warns AGAINST using defer: true for streaming server rendering, stating:

"Deferred scripts (defer: true) only execute after the entire HTML document has finished parsing and streaming. This defeats the key benefit of React 18's Selective Hydration feature"

However, this PR:

• Makes defer the default for generated component packs
• Changes application.html.erb to ALWAYS use defer: true for client-bundle
• Removes the conditional that allowed async: true for non-Redux pages

Impact: This change may negatively affect streaming server rendering performance for applications that don't use Redux shared stores.

Recommendation:

# spec/dummy/app/views/layouts/application.html.erb
<% if uses_redux_shared_store? %>
  <%# defer required for Redux shared stores with inline component registration %>
  <%= javascript_pack_tag('client-bundle', 'data-turbo-track': 'reload', defer: true) %>
<% else %>
  <%# async recommended for optimal streaming hydration (Shakapacker >= 8.2.0) %>
  <%= javascript_pack_tag('client-bundle', 'data-turbo-track': 'reload', async: true) %>
<% end %>

Or update the documentation to reflect the new approach if defer is now recommended for all cases.

2. Missing RuboCop Violation (Critical - CI Blocker)

Location: lib/react_on_rails/configuration.rb:157

The diff shows:

# rubocop:disable Metrics/CyclomaticComplexity
def validate_generated_component_packs_loading_strategy
  # rubocop:enable Metrics/CyclomaticComplexity

Issue: The method no longer has complexity issues after simplification, but the disable/enable comments remain.

Action Required: Run bundle exec rubocop and remove unnecessary disable comments. Per CLAUDE.md:

BEFORE EVERY COMMIT/PUSH:

1. ALWAYS run bundle exec rubocop and fix ALL violations

3. Potential Performance Regression

Concern: While defer ensures correct execution order, it may be slower than async for applications that don't have the race condition issue (e.g., those not using auto_load_bundle).

Recommendation: Consider documenting when users can safely opt into :async for better performance:

# config/initializers/react_on_rails.rb
# If you're not using auto_load_bundle or have a custom registration strategy,
# you may use :async for better performance:
# config.generated_component_packs_loading_strategy = :async

4. Test Coverage Gap

The tests validate the configuration defaults but don't include:

• Integration tests demonstrating the race condition is actually fixed
• Tests verifying component registration completes before hydration
• Tests for the interaction between layout template and configuration

Recommendation: Add a system/integration test that:

1. Uses auto_load_bundle
2. Verifies components are registered before hydration
3. Tests the actual script loading sequence

---

🔒 Security Considerations

✅ No security concerns identified. The change only affects script loading strategy.

---

📊 Performance Considerations

Before (async):

• Scripts execute as soon as downloaded
• Potentially faster initial execution
• Non-deterministic order (race condition)

After (defer):

• Scripts execute after HTML parsing, in DOM order
• Slightly slower initial execution
• Deterministic order (no race condition)

Trade-off: Correctness is prioritized over marginal performance gain. This is the right choice for a default behavior.

---

🧪 Test Coverage Assessment

✅ Good: Configuration spec updated for both Shakapacker versions
✅ Good: Default values tested
⚠️ Missing: Integration test demonstrating the fix works in practice
⚠️ Missing: Tests for layout template changes

---

📝 Action Items

Must Fix Before Merge:

1. ✅ Run bundle exec rubocop and fix all violations (especially line 157)
2. ⚠️ Resolve documentation conflict with streaming-server-rendering.md
3. ⚠️ Decide on layout strategy: conditional (async for non-Redux) or always defer

Nice to Have:

4. Add integration test demonstrating race condition fix
5. Document performance trade-offs and when async can be used
6. Update CHANGELOG.md per repository conventions

---

🎯 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5)
Solution Correctness: ⭐⭐⭐⭐⭐ (5/5)
Test Coverage: ⭐⭐⭐ (3/5)
Documentation: ⭐⭐ (2/5) - conflicts with existing docs

Recommendation: Request changes to address documentation conflict and RuboCop violations before merging.

---

Great work identifying and fixing this race condition! The core solution is solid; it just needs alignment with the streaming documentation and compliance with the project's linting requirements.

[justin808]

@AbanoubGhadban This requires careful review before we release 16.2.0