Revert PR #1972 and add comprehensive investigation #1980
Conversation
Creates detailed analysis of the component registration race condition that PR #1972 attempted to fix by changing the default loading strategy from :async to :defer. Key findings: - PR #1972 treats symptom (execution order) not root cause (timing) - Defer forces sequential execution, hurting performance - React on Rails already has component_registry_timeout feature - Race condition suggests timeout isn't working correctly - Need to investigate why hydration doesn't wait for registrations Recommendations: - Keep async as default for performance - Fix component registry timeout mechanism - Use defer only when truly necessary (documented cases) - Add proper synchronization instead of forced execution order This investigation provides foundation for implementing a proper fix that maintains performance while preventing race conditions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis PR investigates and implements a refined solution for component registration race conditions by introducing conditional loading strategy defaults based on Shakapacker version capabilities. The configuration now defaults to async for Shakapacker >= 8.2.0, sync for older versions, with conditional script loading in the layout and explicit strategy configuration in tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (9)📚 Learning: 2024-07-27T10:08:35.868ZApplied to files:
📚 Learning: 2025-09-15T21:24:48.207ZApplied to files:
📚 Learning: 2025-02-13T16:50:26.861ZApplied to files:
📚 Learning: 2025-09-16T08:01:11.146ZApplied to files:
📚 Learning: 2025-02-12T16:38:06.537ZApplied to files:
📚 Learning: 2025-04-26T21:55:55.874ZApplied to files:
📚 Learning: 2025-02-18T13:08:01.477ZApplied to files:
📚 Learning: 2025-10-23T17:22:01.074ZApplied to files:
📚 Learning: 2025-07-08T05:57:29.630ZApplied to files:
🧬 Code graph analysis (2)lib/react_on_rails/configuration.rb (1)
spec/react_on_rails/configuration_spec.rb (1)
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 #1980 - Revert PR #1972 and Add InvestigationSummaryThis PR reverts PR #1972 (which changed the default loading strategy from ✅ Strengths1. Excellent DocumentationThe
This is exactly the kind of documentation that helps future maintainers understand the decision-making process. 2. Thoughtful Technical AnalysisThe investigation correctly identifies that PR #1972 treated the symptom rather than the root cause:
3. Preserves Conditional LogicRestoring the 4. Code Quality
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review: PR #1980 - Revert PR #1972 and Add InvestigationOverall AssessmentThis is a well-reasoned revert with excellent supporting documentation. The decision to revert PR #1972 and investigate the root cause is the right approach. The investigation document is thorough and provides valuable analysis. Strengths1. Excellent DocumentationThe
This is exactly the kind of documentation that helps future maintainers. 2. Sound Technical ReasoningThe revert is justified:
3. Code Quality
Areas for Improvement1. Configuration Change InconsistencyIssue: In # Line 45: Explicitly sets defer instead of letting it default to async
config.generated_component_packs_loading_strategy = :deferProblem: The PR description and investigation document advocate for async as the default, but the dummy app is explicitly setting defer. This creates confusion. Recommendation: # Option 1: Remove the line entirely to use the smart default
# (async on Shakapacker >= 8.2.0, sync otherwise)
# Option 2: If defer is needed for testing, add a clear comment:
# Explicitly use defer for testing Redux shared store race conditions
config.generated_component_packs_loading_strategy = :defer2. Missing Test Coverage for Component Registry TimeoutThe investigation correctly identifies that the Recommendation: Add a test that verifies timeout behavior: # spec/react_on_rails/component_registry_spec.rb
it "waits for component registration before hydrating" do
# Test that hydration waits up to component_registry_timeout
# for components to register
end
it "fails gracefully after timeout expires" do
# Test that appropriate error is shown if component
# never registers within timeout period
end3. Potential Breaking Change Not DocumentedConcern: This revert changes behavior for users who upgraded to the version with PR #1972:
Recommendation:
4. Investigation Document LocationMinor: Recommendation: Consider moving to Potential Bugs & Security1. Race Condition Still Exists
|
Code ReviewOverall AssessmentThis is a well-reasoned revert with excellent documentation. The PR demonstrates strong engineering judgment by recognizing that PR #1972's solution was treating symptoms rather than addressing root causes. The addition of Strengths1. Excellent Documentation ✅The investigation document (
2. Performance-Conscious Decision ✅Correctly prioritizes
3. Preserves Existing Nuance ✅The
4. Code Quality ✅
Technical AnalysisConfiguration Changes (
|
Code Review: Revert PR #1972 and InvestigationI've reviewed this PR which reverts PR #1972 and adds a comprehensive investigation document. Here's my detailed feedback: ✅ Strengths1. Excellent Investigation DocumentThe
This is exactly the kind of documentation needed before implementing a proper fix. 2. Correct Architectural DecisionReverting PR #1972 is the right call because:
3. Code Quality
🔍 Findings & Observations1. The Real Issue: Component Registry Timeout Not WorkingThe open-source version of React on Rails has a non-functional waiting mechanism: packages/react-on-rails/src/ComponentRegistry.ts:63-64: getOrWaitForComponent(_name: string): never {
throw new Error('getOrWaitForComponent requires react-on-rails-pro package');
}This is a critical insight missing from the investigation document:
This explains why the race condition occurs! The open-source version immediately throws an error if a component isn't registered, rather than waiting up to 5000ms. 2. Test Expectations IssueLooking at the config in config.generated_component_packs_loading_strategy = :deferThis explicitly sets defer, which means:
These are two different things:
3. Conditional Logic DependencyThe restored
|
Code Review - PR #1980: Revert PR #1972 and Add InvestigationI've reviewed this PR and have feedback on code quality, architecture, and testing. Overall, this is a well-reasoned revert with excellent documentation, though there are some concerns to address. ✅ Strengths1. Excellent Investigation DocumentThe
2. Restores Nuanced BehaviorThe conditional logic in <% if uses_redux_shared_store? %>
<%= javascript_pack_tag('client-bundle', defer: true) %>
<% else %>
<%= javascript_pack_tag('client-bundle', async: true) %>
<% end %>This recognizes that only specific pages need 3. Correct Default BehaviorThe configuration change properly defaults to if PackerUtils.supports_async_loading?
self.generated_component_packs_loading_strategy ||= :async4. Tests Updated CorrectlyThe spec changes properly reflect the restored behavior.
|
Summary
This PR reverts PR #1972 and adds a comprehensive investigation document that analyzes the component registration race condition in depth.
What This PR Does
Reverts PR Fix component registration race condition with defer loading strategy #1972 - Restores the previous behavior where:
generated_component_packs_loading_strategydefaults to:async(Shakapacker >= 8.2.0)uses_redux_shared_store?Adds Investigation Document -
PR_1972_INVESTIGATION.mdprovides:Why Revert PR #1972?
PR #1972 "fixed" intermittent test failures by changing the default loading strategy from
:asyncto:defer. While this prevents the race condition, it:The Real Issue
React on Rails has a
component_registry_timeoutfeature (5000ms default) that should prevent this race condition. The fact that tests still fail suggests:Recommended Next Steps
From the investigation document:
Files Changed
lib/react_on_rails/configuration.rb- Reverted to async/sync strategy based on Shakapacker versionspec/dummy/app/views/layouts/application.html.erb- Restored conditional defer logicspec/dummy/config/initializers/react_on_rails.rb- Removed defer-specific commentsspec/react_on_rails/configuration_spec.rb- Updated test expectationsPR_1972_INVESTIGATION.md- NEW: Comprehensive investigation documentBreaking Changes
None for users. This restores the previous behavior that was working for most use cases. The intermittent test failures were edge cases that need proper investigation.
Performance Impact
✅ Positive - Restores async loading for better performance
Test Plan
Related Issues
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is
Summary by CodeRabbit
Bug Fixes
Documentation