Document Turbo/Hotwire integration with auto-registration (#2342)#2345
Document Turbo/Hotwire integration with auto-registration (#2342)#2345
Conversation
Document the content_for :body_content pattern needed when using auto_load_bundle: true with Turbo. This addresses the ordering conflict between Turbo's head loading requirement and auto-registration's dynamic pack tag appending during body rendering. Closes #2342 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds documentation describing a Turbo + auto-bundling load-order conflict and documents the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 for PR #2345SummaryThis PR adds excellent documentation for a common pain point when using Turbo/Hotwire with React on Rails' auto-registration feature. The documentation is clear, well-structured, and addresses issue #2342 effectively. ✅ Strengths
📝 Suggestions for Improvement1. Minor: Consistent TerminologyThe document uses both "auto-registration" and "auto-bundling" to refer to the same feature. Consider being consistent:
Suggestion: Since the linked doc is titled "auto-bundling-file-system-based-automated-bundle-generation.md", consider using "auto-bundling" or "auto-loading" consistently, or define both terms once. 2. Enhancement: Add Cross-ReferenceConsider adding a reciprocal link from the auto-bundling docs back to this Turbo section, since that doc also mentions the 3. Minor: ERB Example CompletenessThe example shows explicit appends: <%= append_stylesheet_pack_tag('stimulus-bundle') %>
<%= append_javascript_pack_tag('stimulus-bundle') %>
<%= append_javascript_pack_tag('stores-registration') %>Question: Are these explicit appends necessary in the example, or would the auto-appends from 4. Enhancement: Link ValidationThe external links look correct but couldn't be validated in this environment:
Suggestion: Before merging, manually verify these links are accessible and point to the correct sections. 🔍 Technical Correctness✅ ERB Syntax: All code blocks use correct ERB syntax 🎯 Test CoveragePer the PR description, the checklist items are:
📋 Pre-Merge ChecklistBased on CLAUDE.md requirements:
🎉 RecommendationLGTM with minor suggestions! This is high-quality documentation that will save users significant debugging time. The suggestions above are optional improvements - none are blockers. The PR effectively addresses issue #2342 by documenting a non-obvious but necessary pattern for combining Turbo with auto-registration. This will be a valuable reference for the community. Suggested next steps:
|
Greptile OverviewGreptile SummaryThis PR adds documentation for using Turbo/Hotwire with React on Rails' auto-registration feature, addressing issue #2342. Key Changes:
Technical Accuracy: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Rails as Rails Template Engine
participant CF as content_for Block
participant RC as react_component Helper
participant Pack as Pack Tag Accumulator
participant Head as HTML Head
participant Body as HTML Body
Note over Rails,Body: Step 1: Process content_for block FIRST
Rails->>CF: Evaluate content_for :body_content block
CF->>RC: Call react_component with auto_load_bundle: true
RC->>Pack: append_javascript_pack_tag("generated/ComponentName")
Pack-->>RC: Bundle registered
RC-->>CF: Return component HTML
CF-->>Rails: Body content captured (not yet output)
Note over Rails,Body: Step 2: Render HTML head
Rails->>Head: Render <head> section
Head->>Pack: javascript_pack_tag (retrieve all appended packs)
Pack-->>Head: Return script tags for all accumulated bundles
Head-->>Rails: Head rendered with all component bundles
Note over Rails,Body: Step 3: Output body content
Rails->>Body: yield :body_content
Body-->>Rails: Output previously captured HTML
Note over Rails,Body: Result: Correct loading order achieved
|
- Use "auto-bundling" terminology consistently (matching linked doc title) - Clarify ERB example: explicit appends are for non-React packs - Update Shakapacker link from master to main branch - Add cross-reference from auto-bundling FOUC section back to Turbo docs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review: Documentation for Turbo/Hotwire Integration with Auto-Registration✅ Overall AssessmentThis PR provides excellent documentation for a common integration challenge. The addition is well-written, technically accurate, and directly addresses issue #2342. Great work! 🎯 Strengths
📝 Minor Suggestions1. ERB Example - Minor ClarificationIn the ERB example at lines 89-93, the comment says: <%# Optional: Explicitly load non-React packs (Stimulus, shared stores, etc.) %>
<%# React component bundles are auto-appended by react_component calls above %>Suggestion: This is already clear, but you could add one more line to emphasize the key point: <%# Optional: Explicitly load non-React packs (Stimulus, shared stores, etc.) %>
<%# React component bundles are already auto-appended by react_component calls above %>
<%# Do NOT manually append component bundles here - they're already included %>This extra emphasis might prevent users from redundantly adding component packs manually. 2. Consider Adding a Common Pitfall SectionYou might consider adding a brief "Common Pitfalls" callout: **Common Pitfalls:**
- ❌ Don't place `javascript_pack_tag` before `content_for` blocks that call `react_component`
- ❌ Don't manually append component bundles if using `auto_load_bundle: true`
- ✅ Do let `react_component` handle bundle appending automatically3. Anchor Link PrecisionThe cross-reference link in the auto-bundling doc is: [Using auto-bundling with Turbo](../building-features/turbolinks.md#turbo-with-auto-bundling)This works great! Just verify the anchor ✅ Technical Accuracy
🔒 Security Considerations
📊 Test CoverageDocumentation Testing (from PR description):
Recommendation: Before merging, manually verify:
🎨 Style & Conventions
📚 Documentation CompletenessWhat's covered:
Optional enhancements (not blocking):
🚀 RecommendationApprove with minor optional suggestions. This PR is ready to merge as-is. The optional suggestions above are enhancements, not blockers. The documentation will save developers significant debugging time and addresses a real pain point. Excellent contribution! ✅ Checklist for Merge
Great work on this documentation improvement! 🎉 |
- Add warning comment: "Do NOT manually append component bundles" - Add Common Pitfalls section with clear dos and don'ts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review: PR #2345 - Document Turbo/Hotwire Integration with Auto-RegistrationOverall Assessment✅ Excellent documentation addition that addresses a real pain point (#2342) for users combining Turbo with auto-bundling. The writing is clear, well-structured, and provides actionable guidance. Strengths
Suggestions for Improvement1. Anchor Link Verification (Minor)The new content references: [Shakapacker Preventing FOUC guide](https://github.com/shakacode/shakapacker/blob/main/docs/preventing_fouc.md#the-content_for-body_content-pattern)The anchor Recommendation: Verify the anchor exists or remove it if uncertain. 2. Code Example Consistency (Minor)Lines 92-94 show optional manual appends. Consider clarifying whether these are truly "optional" or just examples - when would users need these vs when they wouldn't? 3. Example Specificity (Minor)The example uses specific component names ( 4. Testing Consideration (Question)Should there be a working example in the dummy app that demonstrates this pattern? This would:
However, I understand this might be beyond the scope of a documentation PR. 5. Changelog Entry (Important)According to
This is a documentation improvement that helps users solve a real problem. Consider adding a changelog entry: #### Documentation
- [PR 2345](https://github.com/shakacode/react_on_rails/pull/2345) by [justin808](https://github.com/justin808): Document Turbo/Hotwire integration with auto-registration patternTechnical Correctness✅ The solution is technically sound:
Security & Performance✅ No security concerns: Documentation only, example code follows Rails security best practices Style & Conventions✅ Follows project conventions:
RecommendationsBefore merging:
Optional enhancements (could be follow-up PRs):
Final VerdictAPPROVE with minor suggestions ✅ This PR significantly improves the documentation and will help many users struggling with this exact issue. The writing is clear, the solution is correct, and it addresses a real pain point. The only required change is adding a changelog entry. The other suggestions are optional improvements. Great work documenting this non-obvious pattern! This will save users hours of debugging. 🎉 |
) ## Summary - Documents the `content_for :body_content` pattern needed when using auto_load_bundle with Turbo - Explains the ordering conflict between Turbo's head loading requirement and auto-registration's dynamic pack tag appending - Provides a working layout example with step-by-step comments - Links to related Shakapacker FOUC guide and Turbo documentation ## Related Issue Closes #2342 ## Test plan - [ ] Documentation renders correctly in GitHub - [ ] Code examples are syntactically correct - [ ] Links to external docs are valid 🤖 Generated with [Claude Code](https://claude.ai/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added "Turbo with Auto-Bundling" subsection with guidance to resolve bundle ordering when using Turbo alongside auto-bundling tools * Provides a practical example and stepwise rationale to ensure correct head/body bundle ordering and address unusual template ordering * Updated additional resources: Shakapacker link now points to main branch and a Turbo/Hotwire integration entry was added * Turbo Streams and legacy content remain intact <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Summary
content_for :body_contentpattern needed when using auto_load_bundle with TurboRelated Issue
Closes #2342
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit