Fix FOUC by moving stylesheets to head #686

justin808 · 2025-10-16T00:49:10Z

Summary

Fixed FOUC by moving stylesheet_pack_tag to <head> section
Used content_for :body_content pattern to ensure proper execution order
Keeps auto_load_bundle = true for automatic component pack loading
Fixes flash of unstyled content (FOUC) on https://staging.reactrails.com/

Problem

Stylesheets were loading at the end of the body (after SSR content), causing noticeable FOUC where navigation and content appeared unstyled initially, then styles flashed in after CSS loaded.

Root Cause

With auto_load_bundle = true, React on Rails' react_component helper automatically calls append_stylesheet_pack_tag during rendering. Since Shakapacker requires append helpers to execute before stylesheet_pack_tag, we couldn't simply move the stylesheet tag to the head - the body would render after the head, causing appends to happen too late.

Solution

Render the body content FIRST using content_for, then render the head with pack tags:

<% content_for :body_content do %>
  <%= react_component "NavigationBarApp" %>
  <div class="container mx-auto px-4 flex-grow">
    <%= yield %>
  </div>
  <%= react_component "Footer" %>
<% end %>
<!DOCTYPE html>
<html>
<head>
  <%= append_stylesheet_pack_tag('stimulus-bundle') %>
  <%= append_javascript_pack_tag('stimulus-bundle') %>
  <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>
  <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>
</head>
<body>
  <%= yield :body_content %>
</body>
</html>

Execution order:

content_for :body_content executes first, rendering all react_component calls and triggering their auto-appends
Explicit append_*_pack_tag calls in head
Main stylesheet_pack_tag and javascript_pack_tag render with ALL appends registered
yield :body_content outputs the pre-rendered body

This ensures:

All append_*_pack_tag calls (explicit + auto from react_component) happen first
Main pack tags render in the head with all appends registered
Stylesheets load before content renders, eliminating FOUC
auto_load_bundle stays enabled for automatic component packs

Changes

app/views/layouts/application.html.erb: Wrapped body in content_for :body_content, moved pack tags to head
app/views/layouts/stimulus_layout.html.erb: Same pattern applied
config/initializers/react_on_rails.rb: No changes - kept auto_load_bundle = true

Impact

✅ No FOUC - styles load immediately in head
✅ SSR still works - components render with styles
✅ Auto-loading preserved - auto_load_bundle still enabled
⚠️ Non-standard pattern - body content defined before DOCTYPE

Related Issues

React on Rails issue: FOUC with auto_load_bundle: Need better pattern for stylesheet_pack_tag in head react_on_rails#1864
Shakapacker documentation: Documentation: Clarify stylesheet_pack_tag should be in <head> to prevent FOUC shakapacker#720

Test Plan

Visit https://staging.reactrails.com/ and verify no FOUC
Check all pages load with styles immediately applied
Verify SSR still working properly
Confirm all tests pass
Check Network tab for no duplicate asset loading

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

This change is

coderabbitai · 2025-10-16T00:49:19Z

Walkthrough

Reworks both layouts to build main body content into a content_for :body_content block (NavigationBarApp, main container yield, Footer, redux hydration), moves generic stylesheet_pack_tag and javascript_pack_tag calls into the <head> alongside existing Stimulus append_* pack calls, and replaces inline body components with <%= yield :body_content %>.

Changes

Cohort / File(s)	Summary
Application layout `app/views/layouts/application.html.erb`	Wraps the previous body markup (NavigationBarApp, container yield, Footer, redux_store_hydration_data) in `content_for :body_content` and replaces inline body content with `<%= yield :body_content %>`; moves `stylesheet_pack_tag(..., media: 'all', 'data-turbolinks-track': true)` and `javascript_pack_tag(..., 'data-turbolinks-track': true, defer: true)` from body into the head; keeps existing `append_*_pack_tag` ordering in head.
Stimulus layout `app/views/layouts/stimulus_layout.html.erb`	Same pattern: introduced `content_for :body_content` for NavigationBarApp/container/Footer and render via `<%= yield :body_content %>`; relocated `stylesheet_pack_tag` and `javascript_pack_tag` into the head alongside Stimulus `append_*_pack_tag` includes; removed duplicate asset tags from the body.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Browser
  participant Rails as Rails Controller
  participant Layout as Layout (application / stimulus)
  participant View as View Template

  Browser->>Rails: GET page
  Rails->>Layout: Render layout
  Layout->>Layout: Render <head>:\n- append_stylesheet_pack_tag / append_javascript_pack_tag (Stimulus)\n- stylesheet_pack_tag (media: all, data-turbolinks-track)\n- javascript_pack_tag (defer, data-turbolinks-track)
  Layout->>View: Render view/body\n(inside View: content_for :body_content may be defined)
  View-->>Layout: content_for :body_content returned
  Layout->>Browser: Send HTML with assembled head and `<%= yield :body_content %>` in body

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Documentation: Clarify stylesheet_pack_tag should be in <head> to prevent FOUC shakapacker#720 — Moves pack tag placement into the head to avoid FOUC and adjust asset ordering; directly related to relocating stylesheet_pack_tag / javascript_pack_tag into <head>.

Poem

A rabbit tucked the packs on high,
Head neat, body light as sky.
Stimulus hummed, scripts deferred,
Footer waits where hops occurred.
Hop — the layout’s ship has sailed 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly summarizes the main goal of the pull request—preventing a flash of unstyled content by relocating stylesheet tags into the head—and aligns directly with the modifications made in the layout templates. It clearly communicates the problem being solved without extraneous detail and remains concise and actionable. Any teammate reviewing the history would immediately grasp the primary change from this title.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-fouc

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

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

github-actions · 2025-10-16T00:51:56Z

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:

`/deploy-review-app`

Deploy your PR branch for testing

`/delete-review-app`

Remove the review app when done

`/help`

Show detailed instructions, environment setup, and configuration options.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec52aa1 and bfb4db4.

📒 Files selected for processing (2)

app/views/layouts/application.html.erb (1 hunks)
config/initializers/react_on_rails.rb (1 hunks)

🔇 Additional comments (2)

config/initializers/react_on_rails.rb (1)

7-7: Configuration change looks good.

Disabling auto_load_bundle correctly prevents automatic stylesheet appending during component rendering, which enables moving stylesheet_pack_tag to the head. Ensure all required packs are now manually specified in the layout files.
app/views/layouts/application.html.erb (1)
24-24: Specify pack name in javascript_pack_tag.

Similar to line 9, this javascript_pack_tag call lacks an explicit pack name. It should likely specify which pack to load:
<%= javascript_pack_tag('application', 'data-turbolinks-track': true, defer: true) %>
Verify which JavaScript pack should be loaded here and whether the current syntax correctly loads the intended assets.

app/views/layouts/application.html.erb

claude · 2025-10-16T00:58:38Z

PR Review: Fix FOUC by moving stylesheets to head

Summary

This PR addresses a Flash of Unstyled Content (FOUC) issue by relocating stylesheet loading from the end of the <body> to the <head> section and disabling automatic bundle loading. The changes are minimal, focused, and well-documented.

Code Quality & Best Practices ✅

Strengths:

Minimal, focused changes: Only 2 files modified with 2 additions and 2 deletions - exactly what's needed
Excellent PR documentation: Clear problem statement, root cause analysis, and solution explanation
Proper ordering: Stylesheet placement in app/views/layouts/application.html.erb:9 follows best practice of loading styles before content
Logical flow: append_stylesheet_pack_tag is called before stylesheet_pack_tag, which is the correct order

Observations:

The change from auto_load_bundle = true to false shifts responsibility from automatic to manual bundle management
This is appropriate given the need for precise control over stylesheet loading order

Potential Issues & Considerations

1. Missing pack argument in `stylesheet_pack_tag` ⚠️

Location: app/views/layouts/application.html.erb:9

The current code:

<%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>

According to Rails/Shakapacker conventions, stylesheet_pack_tag typically requires a pack name as the first argument. The code appears to be missing which pack(s) to load. This might work if there's a default pack configured, but it's unclear and could be a bug.

Recommendation: Explicitly specify the pack name(s):

<%= stylesheet_pack_tag('application', media: 'all', 'data-turbolinks-track': true) %>

Or if loading multiple packs, verify which packs should be included here.

2. Same issue with `javascript_pack_tag` ⚠️

Location: app/views/layouts/application.html.erb:24

Similar to above, the JavaScript pack tag also lacks an explicit pack name:

<%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>

Note: These issues existed before this PR, but worth addressing.

3. Auto-load implications

With auto_load_bundle = false, you must manually specify all required packs. The PR description mentions "stimulus-bundle already specified," which appears correct in the layout. However, if additional components require other bundles, those would need manual specification now.

Recommendation: Add a comment in the config explaining this trade-off:

# Set to false to prevent automatic bundle appending during react_component rendering
# This allows precise control over asset load order (styles in head, not body)
# Note: All required packs must be manually specified in layouts
config.auto_load_bundle = false

Performance Considerations ✅

Positive impacts:

Eliminates FOUC: Moving stylesheets to <head> ensures styles are available before first paint
Better Core Web Vitals: Reduces Cumulative Layout Shift (CLS) by preventing style-related reflows
Optimal for SSR: With server-side rendering enabled (config.prerender = true), having styles in head is critical

Concerns:

Potential render blocking: Stylesheets in <head> are render-blocking by default. However, this is the correct trade-off for preventing FOUC
No async/defer on stylesheets: While you can't defer CSS, consider if critical CSS inlining would further improve performance (future enhancement, not required for this PR)

Overall: The performance trade-offs are appropriate and well-considered.

Security Concerns ✅

No security issues identified:

Changes are limited to asset loading order
No user input handling or data processing modifications
No changes to authentication, authorization, or data access patterns
Turbolinks tracking attributes maintained for cache busting

Test Coverage ⚠️

Current state:

No tests found that specifically verify stylesheet loading order
No tests for FOUC prevention
Existing system specs in spec/system/ test functionality but not asset loading

Impact assessment:

The changes are relatively low-risk from a functionality perspective
SSR continues to work (components render normally)
Visual regression is the primary concern

Recommendations:

Manual testing checklist (as noted in PR description):
- ✅ Visit https://staging.reactrails.com/ with cache disabled
- ✅ Verify no FOUC on initial page load
- ✅ Test all major pages (comments, router demos, etc.)
- ✅ Verify SSR still renders correctly
- ✅ Check browser DevTools Network tab for proper load order
Consider adding a system spec (optional but valuable):

# spec/system/asset_loading_spec.rb
require "rails_helper"

describe "Asset Loading" do
  it "loads stylesheets in head before body content" do
    visit root_path
    
    # Get the HTML source
    html = page.html
    
    # Verify stylesheet_pack_tag appears before body content
    stylesheet_position = html.index('stylesheet_pack_tag') || html.index('<link')
    body_position = html.index('<body')
    
    expect(stylesheet_position).to be < body_position
  end
end

Visual regression testing (if available):
- Screenshot comparison could catch FOUC issues automatically
- Tools like Percy, Chromatic, or BackstopJS could help

Additional Observations

Documentation

PR description is exemplary - clear, detailed, includes test plan
Commit message follows good conventions with problem/solution structure
Co-authored tag with Claude is transparent

Potential Future Enhancements

Consider using <link rel="preload"> for critical stylesheets
Evaluate if critical CSS inlining would provide additional performance benefits
Monitor Real User Monitoring (RUM) data to confirm FOUC elimination in production

Recommendation: Approve with Minor Suggestions ✅

This is a solid, well-reasoned change that addresses a real UX issue. The code quality is good, and the approach is sound.

Before merging:

⚠️ Verify the stylesheet_pack_tag and javascript_pack_tag calls work without explicit pack names - test in development/staging
✅ Complete the manual test plan on staging
✅ Ensure all CI checks pass (run the full test suite)

Optional improvements:

Add explicit pack names to *_pack_tag helpers for clarity
Add a comment in the initializer explaining the auto_load_bundle = false trade-off
Consider adding a basic system spec for asset load order

Great work on identifying and fixing this issue! The thorough documentation makes the change easy to review and understand.

Reviewed by: Claude Code
Review Date: 2025-10-16

claude · 2025-10-16T04:28:50Z

Code Review - PR #686: Fix FOUC by moving stylesheets to head

Summary

This PR addresses the Flash of Unstyled Content (FOUC) issue by relocating stylesheet loading from the end of <body> to the <head> section. The approach is sound and solves a real UX problem.

Code Quality & Best Practices

✅ Strengths:

Well-documented PR description - Clear problem statement, root cause analysis, and solution explanation
Consistent implementation - Both layout files (application.html.erb and stimulus_layout.html.erb) are updated identically
Proper ERB syntax - content_for blocks are correctly structured
Preserves existing functionality - All pack tags are maintained, just reordered

⚠️ Issues Identified:

CRITICAL: Missing configuration change

The PR description states that auto_load_bundle = false should be set in config/initializers/react_on_rails.rb, but this file is not included in the PR changes
Currently, line 7 of config/initializers/react_on_rails.rb still has config.auto_load_bundle = true
Impact: Without this change, the solution may not work as described. The react_component helper might still auto-append stylesheets, defeating the purpose of this PR
Recommendation: Add the configuration file change to this PR

Potential ordering issue:

<% content_for :head do %>
  <%= append_stylesheet_pack_tag('stimulus-bundle') %>
  <%= append_javascript_pack_tag('stimulus-bundle') %>
  <%= append_javascript_pack_tag('stores-registration') %>
<% end %>

This content_for block is defined inside the <body> tag but yields in the <head>
While Rails will collect and render this correctly, it's unconventional and may confuse developers
Better approach: Define content_for :head blocks before they're yielded, or move append calls directly into the <head> section

Potential Bugs & Issues

Execution order concern:
- <%= yield :head %> in the <head> section will execute before the content_for :head block in the body
- Rails handles this by buffering content_for blocks, but the first request may not have the expected content
- Test this carefully with fresh page loads (not cached)
Missing from PR but mentioned in description:
- The configuration change to auto_load_bundle = false is critical
- Without it, automatic bundle loading may still occur
Duplicate pack loading risk:
- With auto_load_bundle = true (current state), components might still try to auto-load packs
- Combined with manual pack specification, this could lead to duplicate asset loading
- Verify that assets aren't being loaded twice in dev tools Network tab

Performance Considerations

✅ Improvements:

Eliminates FOUC - Styles load before content renders, improving perceived performance
JavaScript deferred - defer: true maintains non-blocking behavior
Proper asset ordering - CSS in head, JS deferred is optimal

⚠️ Potential concerns:

Render-blocking CSS - Moving stylesheets to <head> is correct but does block initial render (this is intentional and necessary)
No apparent optimization for critical CSS - Consider extracting critical CSS inline for even faster first paint (future enhancement)

Security Concerns

✅ No security issues identified:

csrf_meta_tags properly included
No user input being rendered
Asset integrity maintained with data-turbolinks-track: true

Test Coverage

⚠️ Gaps identified:

No automated tests for FOUC fix:
- No new specs added to verify stylesheet loading order
- Consider adding a system/integration test that checks:
  - Stylesheets are in <head> tag
  - Stylesheets load before body content
  - No duplicate asset loading
Manual testing only:
- Test plan relies entirely on manual verification
- System specs exist (spec/system/pages_spec.rb, etc.) but none verify asset loading order
Suggested test additions:

# spec/system/asset_loading_spec.rb
require 'rails_helper'

RSpec.describe 'Asset Loading', type: :system do
  it 'loads stylesheets in head section' do
    visit root_path
    head_html = page.find('head', visible: false).native.inner_html
    expect(head_html).to include('stylesheet_pack_tag')
  end

  it 'does not load stylesheets in body' do
    visit root_path
    body_html = page.find('body').native.inner_html
    expect(body_html).not_to include('stylesheet_pack_tag')
  end
end

Action Items

Before merging:

⚠️ CRITICAL: Add the missing configuration change:

# config/initializers/react_on_rails.rb
config.auto_load_bundle = false

Consider refactoring the content_for :head pattern:
- Move append pack tags directly into <head> section, OR
- Add comments explaining the unconventional pattern
Add automated tests to verify:
- Stylesheets render in <head>
- No FOUC occurs (check computed styles on page load)
- No duplicate asset loading
Verify in staging that:
- SSR still works correctly
- All pages load with styles immediately
- DevTools Network tab shows no duplicate requests
- All existing tests pass

Overall Assessment

Good work on identifying and solving the FOUC issue! The approach is sound, but the PR is incomplete without the auto_load_bundle = false configuration change mentioned in the description. Please add that file to the PR and consider the testing recommendations above.

Rating: Approve with changes requested

Review generated with 🤖 Claude Code

Problem: - Stylesheets were loading at bottom of body, causing FOUC with SSR - Moving stylesheet_pack_tag to head failed because react_component with auto_load_bundle=true would call append_stylesheet_pack_tag during rendering, which must come before stylesheet_pack_tag Solution: - Set auto_load_bundle = false in react_on_rails config - Manually specify required packs in layout (stimulus-bundle) - Move stylesheet_pack_tag to head after append calls - This ensures styles load before content renders, preventing FOUC 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Problem: - Stylesheets at bottom of body caused FOUC with SSR - Moving to head failed because react_component (with auto_load_bundle) calls append_*_pack_tag during render, which must execute before the main *_pack_tag helpers Solution: - Use content_for :head block in body for explicit append calls - yield :head in head section before calling main pack tags - This ensures all appends (explicit + auto from react_component) execute before main pack tags, allowing styles in head - Keeps auto_load_bundle enabled for automatic component packs Changes: - Wrap append_*_pack_tag calls in content_for :head blocks - Add yield :head in head section before pack tags - Move stylesheet_pack_tag and javascript_pack_tag to head - Applied to both application.html.erb and stimulus_layout.html.erb Result: Stylesheets load in head, eliminating FOUC while maintaining auto_load_bundle functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

…der issue)

…ack tags Solution: - Wrap body content in content_for :body_content at TOP of layout - This executes all react_component calls first, triggering auto-appends - Then render head with append calls followed by main pack tags - Finally yield body content Execution order: 1. content_for :body_content executes (all react_component auto-appends happen) 2. Explicit append_*_pack_tag calls in head 3. stylesheet_pack_tag and javascript_pack_tag in head (with all appends registered) 4. yield :body_content outputs the body Result: Stylesheets load in head, eliminating FOUC, while preserving auto_load_bundle functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

claude · 2025-10-16T08:40:03Z

PR Review: Fix FOUC by moving stylesheets to head

Overview

This PR addresses Flash of Unstyled Content (FOUC) by restructuring the layout templates to load stylesheets in the <head> section before body content renders. The approach uses Rails' content_for pattern to manage execution order while maintaining auto_load_bundle = true.

Code Quality & Best Practices

✅ Strengths

Well-documented PR description: The PR provides excellent context explaining the problem, root cause, and solution approach. This makes the change easy to understand and review.
Correct use of Rails patterns: The content_for :body_content / yield :body_content pattern is a standard Rails approach for managing content blocks and is appropriate here.
Maintains functionality: The change preserves existing functionality (SSR, auto-loading) while fixing the visual issue.
Consistent application: Both layout files (application.html.erb and stimulus_layout.html.erb) receive identical treatment, ensuring consistency.

🔍 Observations & Suggestions

Naming convention: The PR description mentions using content_for :head, but the actual implementation uses content_for :body_content. While this works correctly, there's a slight documentation mismatch. The actual implementation is more semantically accurate since it's wrapping body content, not head content.
Indentation and formatting: The changes maintain consistent ERB formatting and proper indentation. Good adherence to style conventions.
Asset tag ordering: The pack tags are now correctly positioned:
- append_*_pack_tag calls in head (lines 8-10)
- Main stylesheet_pack_tag and javascript_pack_tag after appends (lines 12-13)
- This ensures all appends register before main tags render

Potential Issues

⚠️ Minor Concerns

Missing yield :head in diff: The PR description mentions calling yield :head in the head section, but the diff doesn't show this. Looking at the implementation, it appears the approach evolved to use content_for :body_content instead, which is actually cleaner. However, this creates confusion between the description and implementation.
Redux store hydration data location: In both layouts, redux_store_hydration_data is rendered at the end of body. This is correct for hydration, but worth noting that it's now inside the content_for :body_content block, which shouldn't affect functionality but changes the rendering order slightly.
No visual regression tests: The test plan mentions manual verification on staging, but there are no automated tests to prevent FOUC regressions. Consider adding:
- Visual regression tests (e.g., using Percy, Chromatic)
- Performance tests checking DOMContentLoaded timing
- Tests verifying stylesheet loads before body content

Performance Considerations

✅ Positive Impacts

Eliminates FOUC: Primary performance improvement from a user experience perspective - styles load immediately, eliminating visual jank.
Maintains defer on JavaScript: The defer: true attribute on JavaScript pack tags ensures scripts don't block rendering, which is optimal.
Proper CSS positioning: Stylesheets in <head> allow the browser to:
- Start downloading CSS immediately
- Parse styles before rendering body
- Avoid repaints from late-loading styles

⚠️ Considerations

Potential for duplicate style processing: Since the layout now has a more complex execution order (body content evaluated first to populate content_for, then head renders, then body renders), ensure there's no performance regression. Testing on production-like data would be valuable.
JavaScript defer attribute: While defer: true is good for performance, ensure all components properly handle deferred script execution and don't assume scripts run before body content is parsed.

Security Concerns

✅ No New Security Issues

CSRF protection maintained: csrf_meta_tags remains in the head section.
No new user input: Changes only restructure existing template logic without introducing new attack surfaces.
No changes to authentication/authorization: Asset loading changes don't affect security boundaries.
Turbolinks tracking maintained: The data-turbolinks-track attribute is preserved on both stylesheet and JavaScript pack tags.

Test Coverage

⚠️ Gaps Identified

No automated tests in PR: The PR doesn't include any test changes or additions.

Manual test plan only: While the test plan in the description is good, it relies entirely on manual verification:

- [ ] Visit https://staging.reactrails.com/ and verify no FOUC
- [ ] Check all pages load with styles immediately applied
- [ ] Verify SSR still working properly
- [ ] Confirm all tests pass
- [ ] Check Network tab for no duplicate asset loading

Recommended test additions:
- Integration tests: Verify stylesheets appear in head in rendered HTML
- System tests: Test critical user journeys still work (navigation, forms, etc.)
- Performance tests: Measure First Contentful Paint (FCP) and compare before/after
- Visual regression tests: Capture screenshots to prevent FOUC regressions

Additional Recommendations

Update PR description: Align the "Solution" section with the actual implementation (using :body_content instead of :head).

Add comments in templates: Consider adding brief comments explaining why the content_for :body_content wrapper exists:

<%# Wrap body content so layout can render head with all pack tags before body %>
<% content_for :body_content do %>
  ...
<% end %>

Monitor post-deployment:
- Check Real User Monitoring (RUM) metrics for FCP improvements
- Monitor for any SSR-related errors
- Verify no duplicate asset loading issues
Consider extracting shared layout code: Both application.html.erb and stimulus_layout.html.erb are very similar. Consider:
- A shared partial for navigation/footer
- A parent layout that both inherit from
- This would make future changes easier and reduce duplication

Summary

Overall Assessment: ✅ Approve with minor suggestions

This is a solid PR that solves a real UX problem (FOUC) using appropriate Rails patterns. The code changes are minimal, focused, and maintain backward compatibility. The main areas for improvement are:

Test coverage (add automated tests)
Documentation alignment (update PR description to match implementation)
Consider adding explanatory comments in templates

The security and performance implications are positive, and the approach is sound. Once the manual test plan is completed successfully, this is ready to merge.

Code Quality: ⭐⭐⭐⭐ (4/5) - Clean, idiomatic Rails code with good structure
Test Coverage: ⭐⭐ (2/5) - Manual test plan only, no automated tests
Documentation: ⭐⭐⭐⭐ (4/5) - Excellent PR description, minor mismatch with implementation
Security: ⭐⭐⭐⭐⭐ (5/5) - No concerns
Performance: ⭐⭐⭐⭐⭐ (5/5) - Positive improvement

claude · 2025-10-16T08:41:02Z

Code Review - PR #686: Fix FOUC by moving stylesheets to head

Overview

This PR addresses Flash of Unstyled Content (FOUC) by relocating stylesheet and JavaScript pack tags from the body to the <head> section. The implementation uses Rails content_for/yield pattern to manage execution order with React on Rails' auto-loading feature.

✅ Strengths

Correct Problem Identification: The PR correctly identifies and fixes the FOUC issue caused by CSS loading after SSR content renders.
Proper Execution Order: Using content_for :body_content to wrap body content and yielding it after pack tags is a clever solution that ensures:
- All append_*_pack_tag calls (including auto-generated ones) execute first
- Main pack tags render in the head with all dependencies registered
- Preserves auto_load_bundle = true functionality
Consistent Implementation: Both layout files follow the same pattern, maintaining consistency.
Performance Improvement: Moving CSS to <head> follows web performance best practices and eliminates FOUC.

⚠️ Potential Issues

1. Execution Order Concern (app/views/layouts/application.html.erb:8-11)

The current implementation has append_*_pack_tag calls inside the <head> section but before the content_for :body_content block executes:

<head>
  <%= append_stylesheet_pack_tag('stimulus-bundle') %>
  <%= append_javascript_pack_tag('stimulus-bundle') %>
  <%= append_javascript_pack_tag('stores-registration') %>
  <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>
  <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>
</head>
<body>
  <%= yield :body_content %>  <!-- This is where react_component auto-appends happen -->
</body>

Problem: The explicit append_*_pack_tag calls execute before the body content (which contains react_component calls with auto-appends). However, the main stylesheet_pack_tag and javascript_pack_tag also execute before the body, so they won't include the auto-appended packs from react_component helpers.

Expected behavior based on PR description:

"Wrap explicit append_*_pack_tag calls in content_for :head blocks, then call yield :head before the main *_pack_tag helpers"

Suggested Fix:

<% content_for :body_content do %>
  <% content_for :head do %>
    <%= append_stylesheet_pack_tag('stimulus-bundle') %>
    <%= append_javascript_pack_tag('stimulus-bundle') %>
    <%= append_javascript_pack_tag('stores-registration') %>
  <% end %>
  
  <%= react_component "NavigationBarApp" %>
  <div class="container mx-auto px-4 flex-grow">
    <%= yield %>
  </div>
  <%= react_component "Footer" %>
  <%= redux_store_hydration_data %>
<% end %>

<!DOCTYPE html>
<html>
<head>
  <title>React Webpack Rails Tutorial</title>
  <%= yield :head %>  <!-- Explicit appends execute here -->
  <%= stylesheet_pack_tag(media: 'all', 'data-turbolinks-track': true) %>
  <%= javascript_pack_tag('data-turbolinks-track': true, defer: true) %>
  <%= csrf_meta_tags %>
</head>
<body class="min-h-screen flex flex-col bg-sky-50 text-gray-700">
  <%= yield :body_content %>
</body>
</html>

This ensures:

Explicit append_* calls execute first (via yield :head)
Auto-appends from react_component execute next (when :body_content is defined)
Main pack tags execute last, including all appended packs

2. Missing DOCTYPE Declaration Order

The content_for :body_content block appears before the <!DOCTYPE html> declaration. While this works in ERB (since content_for doesn't output directly), it may confuse developers reading the code.

Recommendation: Add a comment explaining this pattern or restructure as suggested above.

🔍 Code Quality & Best Practices

Good Practices Observed:

✅ Using defer: true on JavaScript pack tags for non-blocking script loading
✅ Including data-turbolinks-track: true for proper Turbolinks cache invalidation
✅ Maintaining CSRF protection with csrf_meta_tags
✅ Comprehensive PR description with clear problem/solution

Minor Suggestions:

Indentation Consistency: The indentation looks correct, maintaining 2-space Ruby/Rails convention.
Comment Clarity: The existing comment about redux_store_hydration_data is helpful. Consider adding a brief comment explaining the content_for :body_content pattern for future maintainers.

🔒 Security Considerations

✅ No security concerns identified:

CSRF protection remains intact
No user input handling in these changes
No exposure of sensitive data
No changes to authentication/authorization

⚡ Performance Considerations

✅ Positive impact:

Eliminates FOUC: Major UX improvement
Proper CSS loading order: Stylesheets in <head> prevent render-blocking at body level
Deferred JavaScript: Scripts load with defer, maintaining non-blocking behavior

⚠️ Potential concern:

If the execution order issue exists (see Issue Need to add Capybara Feature tests #1), some component-specific packs might not load correctly, potentially requiring additional HTTP requests or causing runtime errors.

🧪 Test Coverage

Current state: PR description includes manual test plan but no automated tests added.

Recommendations:

Add a system/integration test to verify:
- Stylesheets appear in <head> section
- No FOUC occurs (can test by checking computed styles immediately after page load)
- All expected pack tags are present in the correct order

Example test:

# spec/system/fouc_prevention_spec.rb
require "rails_helper"

RSpec.describe "FOUC Prevention", type: :system do
  it "loads stylesheets in the head section" do
    visit root_path
    
    # Verify stylesheet is in head
    expect(page).to have_css('head link[rel="stylesheet"]')
    
    # Verify scripts are deferred
    expect(page).to have_css('head script[defer]')
  end
end

Verify existing system tests still pass to ensure no regression

📋 Checklist for Author

Before merging, please verify:

Test the execution order with auto_load_bundle = true - verify component-specific packs load correctly
Run full test suite to ensure no regressions
Test on staging environment (https://staging.reactrails.com/)
Verify Network tab shows no duplicate asset loading
Check that SSR still works correctly
Confirm FOUC is eliminated across all pages
Consider adding automated tests for this behavior

🎯 Recommendation

Conditional Approval ⚠️

The approach is sound, but please verify the execution order concern (Issue #1 above). If the current implementation works correctly with auto_load_bundle = true, then it's good to merge after:

Confirming all tests pass
Testing on staging
Adding a comment explaining the pattern

If issues arise with component pack loading, implement the suggested restructuring above.

💡 Additional Context

This is a common Rails pattern when dealing with helpers that need to register dependencies before the main tags render. The React on Rails gem's auto-loading feature adds complexity, making the execution order critical. Great job documenting the approach in the PR description!

Reviewed with Claude Code 🤖

Let me know if you'd like me to clarify any of these points or if you need help implementing the suggested changes!

justin808 · 2025-10-16T08:42:46Z

/deploy-review-app

github-actions · 2025-10-16T08:44:51Z

🎉 ✨ Deploy Complete! 🚀

🌐 ➡️ Open Review App

Deployment successful for PR #686, commit 4cc2aeb

🎮 Control Plane Console
📋 View Completed Action Build and Deploy Logs

github-actions · 2025-10-16T09:09:40Z

✅ Review app for PR #686 was successfully deleted

View Completed Delete Logs

Control Plane Organization

justin808 mentioned this pull request Oct 16, 2025

Documentation: Clarify stylesheet_pack_tag should be in <head> to prevent FOUC shakacode/shakapacker#720

Closed

coderabbitai bot requested changes Oct 16, 2025

View reviewed changes

app/views/layouts/application.html.erb Show resolved Hide resolved

coderabbitai bot approved these changes Oct 16, 2025

View reviewed changes

justin808 and others added 4 commits October 15, 2025 22:32

Attempt: Move append calls to head with yield for view-specific packs

e3b0b70

Align stimulus_layout with application layout (still has execution or…

5f7c02b

…der issue)

justin808 force-pushed the fix-fouc branch from 0dce1d0 to 5f7c02b Compare October 16, 2025 08:32

justin808 mentioned this pull request Oct 16, 2025

FOUC with auto_load_bundle: Need better pattern for stylesheet_pack_tag in head shakacode/react_on_rails#1864

Closed

github-actions bot temporarily deployed to review October 16, 2025 08:44 Inactive

justin808 mentioned this pull request Oct 16, 2025

Document content_for pattern to prevent FOUC with SSR and auto_load_bundle (#1864) shakacode/react_on_rails#1865

Merged

4 tasks

justin808 merged commit d054855 into master Oct 16, 2025
9 checks passed

justin808 deleted the fix-fouc branch October 16, 2025 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix FOUC by moving stylesheets to head #686

Fix FOUC by moving stylesheets to head #686

Uh oh!

justin808 commented Oct 16, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 16, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

claude bot commented Oct 16, 2025

Uh oh!

claude bot commented Oct 16, 2025

Uh oh!

claude bot commented Oct 16, 2025

Uh oh!

claude bot commented Oct 16, 2025

Uh oh!

justin808 commented Oct 16, 2025

Uh oh!

github-actions bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Oct 16, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix FOUC by moving stylesheets to head #686

Fix FOUC by moving stylesheets to head #686

Uh oh!

Conversation

justin808 commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Root Cause

Solution

Changes

Impact

Related Issues

Test Plan

Uh oh!

coderabbitai bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Oct 16, 2025

🚀 Quick Review App Commands

/deploy-review-app

/delete-review-app

/help

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot commented Oct 16, 2025

PR Review: Fix FOUC by moving stylesheets to head

Summary

Code Quality & Best Practices ✅

Potential Issues & Considerations

1. Missing pack argument in stylesheet_pack_tag ⚠️

2. Same issue with javascript_pack_tag ⚠️

3. Auto-load implications

Performance Considerations ✅

Security Concerns ✅

Test Coverage ⚠️

Additional Observations

Documentation

Potential Future Enhancements

Recommendation: Approve with Minor Suggestions ✅

Uh oh!

claude bot commented Oct 16, 2025

Code Review - PR #686: Fix FOUC by moving stylesheets to head

Summary

Code Quality & Best Practices

✅ Strengths:

⚠️ Issues Identified:

Potential Bugs & Issues

Performance Considerations

✅ Improvements:

⚠️ Potential concerns:

Security Concerns

Test Coverage

Action Items

Overall Assessment

Uh oh!

claude bot commented Oct 16, 2025

PR Review: Fix FOUC by moving stylesheets to head

Overview

Code Quality & Best Practices

✅ Strengths

🔍 Observations & Suggestions

Potential Issues

⚠️ Minor Concerns

Performance Considerations

✅ Positive Impacts

⚠️ Considerations

Security Concerns

✅ No New Security Issues

Test Coverage

justin808 commented Oct 16, 2025 •

edited

Loading

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

`/deploy-review-app`

`/delete-review-app`

`/help`

1. Missing pack argument in `stylesheet_pack_tag` ⚠️

2. Same issue with `javascript_pack_tag` ⚠️

github-actions bot commented Oct 16, 2025 •

edited

Loading

github-actions bot commented Oct 16, 2025 •

edited

Loading