Make non-pro dummy app use deferred scripts only (fixes race conditions while hydrating components there)

[AbanoubGhadban]

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

---

This change is 

Summary by CodeRabbit

• Refactor
  • Standardized JavaScript bundle loading to always use deferred loading for the client bundle.
  • Removed page-level conditional that selectively altered asset loading based on shared state usage.

[coderabbitai]

Walkthrough

Removed the conditional logic that selected async vs defer loading for the client JS bundle and removed the uses_redux_shared_store? helper from the sample app; the layout now always loads the client bundle with defer: true.

Changes

Cohort / File(s)	Summary
Layout: script loading simplified spec/dummy/app/views/layouts/application.html.erb	Removed conditional branches and comments for async/defer selection; javascript_pack_tag for client-bundle now always uses defer: true.
Controller: helper removed spec/dummy/app/controllers/application_controller.rb	Removed the public helper method uses_redux_shared_store? and its helper_method declaration used previously to drive layout branching.

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant Layout
  participant ClientBundle

  rect rgb(230,245,255)
    Note over Layout: Old flow (conditional)
    Browser->>Layout: Request page
    Layout-->>Browser: HTML with conditional script tag (async OR defer)
    Browser->>ClientBundle: Load script (async/defer based on condition)
  end

  rect rgb(245,255,230)
    Note over Layout: New flow (always defer)
    Browser->>Layout: Request page
    Layout-->>Browser: HTML with script tag (defer)
    Browser->>ClientBundle: Load script (defer)
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Small number of files with straightforward removals.
• Check any tests or views referencing uses_redux_shared_store?.
• Verify client bundle loading behavior in integration/system tests or manual smoke test.

Possibly related PRs

• Fix component registration race condition with defer loading strategy #1972 — implements the same layout change to always use defer for the client bundle.
• Document defer script impact on streaming and Selective Hydration #1927 — previously added/managed the uses_redux_shared_store? conditional logic that this change removes.

Suggested reviewers

• rameziophobia
• justin808

Poem

🐰 A bundle deferred, no branches to see,
Async hops away — now calm and free,
A helper tucked out of the meadow’s store,
Cleaner rails, and fewer leaves to floor. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: switching the non-pro dummy app to use deferred scripts only, and explains the purpose (fixing race conditions during component hydration).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 investigate-image-example-failing-test

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19ae082 and 1c939c7.

📒 Files selected for processing (1)

• spec/dummy/app/controllers/application_controller.rb (0 hunks)

💤 Files with no reviewable changes (1)

• spec/dummy/app/controllers/application_controller.rb

⏰ 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). (3)

• GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
• GitHub Check: dummy-app-integration-tests (3.2, 20, minimum)
• GitHub Check: claude-review

---

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 #1991

Summary

This PR simplifies the dummy app's layout by removing conditional script loading logic and standardizing on defer: true for all pages. The change removes the conditional check for Redux shared stores and applies deferred loading universally.

---

Analysis

What Changed

• Removed: Conditional logic that chose between defer: true and async: true based on uses_redux_shared_store?
• Applied: defer: true uniformly to all pages in the non-pro dummy app
• Files affected: spec/dummy/app/views/layouts/application.html.erb

Impact Assessment

Positive:

1. Fixes race conditions: The PR title mentions this addresses race condition issues during hydration, which is the primary benefit
2. Simplifies code: Removes conditional logic and the need for uses_redux_shared_store? helper method
3. Ensures reliability: defer: true guarantees proper execution order for Redux shared stores with inline registration

Concerns:

1. Performance regression: According to the project's own documentation (docs/building-features/streaming-server-rendering.md:282-288), async: true provides better performance than defer: true with Shakapacker >= 8.2.0:

   • async: true enables optimal Time to Interactive (TTI)
   • async: true allows Selective Hydration to work immediately
   • defer: true forces components to wait until the entire page finishes loading

2. Breaks best practices for non-Redux pages: Pages without Redux shared stores would benefit from async: true, but this change forces them to use defer: true

3. Missing context: The PR description is incomplete (still contains template text), making it difficult to understand:

   • What specific race condition was occurring?
   • Which tests were failing?
   • Why is this change necessary for the dummy app specifically?

---

Recommendations

Critical - Before Merging:

1. Complete the PR description:

   • Explain what race condition was happening
   • Document which tests were failing (branch name suggests image-example-failing-test)
   • Clarify why the conditional approach wasn't working

2. Consider a more nuanced solution:

   • Keep defer: true only for pages that actually use Redux shared stores
   • Use async: true for pages without Redux stores (optimal performance)
   • If the helper method logic is broken, fix it rather than removing the optimization

3. Update the uses_redux_shared_store? helper:

   • The helper method at spec/dummy/app/controllers/application_controller.rb:27 is now unused
   • Either remove it or update it if you plan to restore conditional loading later

4. Add tests:

   • Add/update E2E tests to verify the race condition is fixed
   • Verify hydration works correctly on all affected pages
   • Test both Redux and non-Redux pages

5. Update CHANGELOG (if this affects users):

   • If this is just a test app fix, no changelog needed
   • If this reflects a recommendation change, document it

Performance Considerations:

According to the documentation, the non-pro dummy app's current configuration should work with both approaches. The fact that it doesn't suggests there might be a deeper issue:

• Is the immediate_hydration configuration properly set?
• Are there timing issues with component registration?
• Is Shakapacker version >= 8.2.0?

Suggestion: Debug the root cause rather than applying a blanket workaround that reduces performance for all pages.

---

Code Quality: ✅ No Issues

• The change itself is clean and straightforward
• Proper ERB syntax
• Follows project conventions

Security: ✅ No Concerns

• No security implications

Testing: ⚠️ Needs Attention

• PR checklist shows tests not yet added/updated
• No clear test coverage for the race condition fix

---

Questions for Author:

1. What specific race condition was occurring? Can you provide error logs or test failures?
2. Have you verified this doesn't negatively impact page load performance on non-Redux pages?
3. Is there a reason the conditional approach can't be fixed instead of removed?
4. Should this change be documented in any upgrade guides or configuration docs?

---

Verdict: Request Changes

While the change is technically correct and will prevent race conditions, it's a blanket solution that sacrifices performance. I recommend:

1. Complete the PR description with context
2. Consider restoring conditional logic with a fix for the race condition
3. Add test coverage
4. Document the decision rationale

The PR can merge as-is if this is purely a test fixture simplification and performance isn't a concern for the dummy app, but it should be clearly documented why we're diverging from best practices.

[claude]

Code Review for PR #1991

Summary

This PR simplifies the dummy app by removing conditional script loading logic and standardizing on defer: true for all pages. This is a reasonable approach that prioritizes consistency and eliminates potential race conditions in the test/dummy environment.

---

✅ Positive Aspects

1. Simplification & Maintainability

• Removes 21 lines of conditional logic from the application controller and layout
• Eliminates the uses_redux_shared_store? helper method and its maintenance burden
• Makes the dummy app easier to understand for contributors

2. Consistency with Best Practices

• Aligns with the documentation's guidance that Redux shared stores with inline registration require defer: true
• The removed conditional logic had potential for bugs if the action list wasn't kept up-to-date
• Per docs/building-features/streaming-server-rendering.md and docs/api-reference/redux-store-api.md, defer: true is mandatory for this use case

3. Race Condition Prevention

• The PR title correctly identifies that this fixes race conditions during hydration
• Using defer: true ensures scripts execute in order after HTML parsing completes
• Prevents the async bundle from executing before inline component registration scripts

4. Appropriate for Test Environment

• The dummy app is for testing, not production optimization
• Consistency is more valuable than micro-optimizations in test code
• All test pages will now behave uniformly

---

⚠️ Areas for Improvement

1. Missing PR Description (Critical)
The PR body is still using the template placeholder text. Please update it with:

• Why this change was made (race conditions observed during hydration)
• What pages were affected
• Why defer: true is the correct solution (inline Redux store registration requirement)
• Mention that this standardizes the dummy app, not production apps

2. Missing CHANGELOG Entry (Required)
Per CLAUDE.md, user-visible changes need CHANGELOG entries. However, since this only affects the spec/dummy test app (not the gem or package), you should:

• Either add a CHANGELOG entry noting "Fixed race conditions in dummy app by standardizing on deferred script loading"
• Or clarify in the PR description why this doesn't need a CHANGELOG entry (internal test app only)

3. Missing Tests
Consider adding a Playwright E2E test to verify:

• Redux shared store pages hydrate correctly without race conditions
• Components become interactive after hydration
• No console errors during hydration

Example test location: spec/dummy/e2e/playwright/e2e/react_on_rails/redux_shared_store.spec.js

4. Documentation Update Consideration
The removed code had helpful inline comments about when to use defer vs async. Consider whether any of this information should be preserved in:

• spec/dummy/README.md or similar documentation
• The existing docs at docs/building-features/streaming-server-rendering.md already cover this well

5. Performance Note for Documentation
While defer: true is correct for the dummy app, production apps should:

• Avoid inline component registration when possible
• Use async: true with proper bundling for optimal performance (requires React on Rails Pro's immediate_hydration)
• This could be mentioned in the PR description to help future developers understand the tradeoff

---

🔍 Code Quality

File: spec/dummy/app/controllers/application_controller.rb

• ✅ Clean removal of unused helper method
• ✅ No lingering dependencies

File: spec/dummy/app/views/layouts/application.html.erb

• ✅ Simplified to single javascript_pack_tag call
• ✅ Maintains necessary attributes (data-turbo-track: 'reload')
• ⚠️ Lost the helpful comment explaining defer vs async tradeoffs

---

🧪 Testing Recommendations

Before merging, please verify:

1. Run the full test suite:

   rake  # Runs lint and all tests except examples

2. Manually test affected pages:

   cd spec/dummy
   bin/rails server
   # Visit all pages that use Redux shared stores:
   # - /server_side_hello_world_shared_store
   # - /client_side_hello_world_shared_store
   # - /server_side_redux_app
   # Check browser console for errors
   # Verify components are interactive

3. Run Playwright E2E tests:

   cd spec/dummy
   yarn test:e2e

4. Check for hydration warnings:

   • Look for React hydration mismatch warnings in console
   • Verify no "component not registered" errors

---

📋 Pre-Merge Checklist

Per CLAUDE.md and the PR template:

• Update PR description with actual summary (remove template)
• Decide on CHANGELOG entry (or document why not needed)
• Consider adding E2E test for Redux shared store hydration
• Run bundle exec rubocop and fix any violations
• Ensure all files end with newline (should be automatic via pre-commit hooks)
• Verify CI passes (all tests green)
• Manual testing of affected pages completed

---

🎯 Recommendation

Approve with minor changes required:

The core change is sound and improves the codebase. Before merging, please:

1. Fill out the PR description (most important)
2. Address the CHANGELOG decision
3. Consider adding a test (strongly recommended but not blocking)

The removed conditional logic was error-prone (action name string matching) and the uniform approach is more maintainable for a test app.

---

💡 Future Considerations

For production apps (not this dummy app), developers should note:

• defer: true is required when using redux_store helper with inline registration
• For better performance, use component registration in bundles (not inline) with async: true
• React on Rails Pro's immediate_hydration feature enables async scripts without race conditions
• See docs/building-features/streaming-server-rendering.md for detailed guidance

Great work simplifying the test infrastructure! 🚀

[claude]

Code Review - PR #1991

Summary

This PR simplifies script loading in the dummy app by removing conditional logic for defer vs async script loading and standardizing on defer: true for all pages. The change removes the uses_redux_shared_store? helper method and its conditional rendering logic.

Positive Aspects ✅

1. Simplification: Removes unnecessary complexity from the dummy app by eliminating conditional script loading logic
2. Consistency: Standardizes script loading behavior across all test pages
3. Bug Fix: The PR title mentions fixing race conditions during hydration, which aligns with using defer: true
4. Clean Code: Removes 29 lines of now-unnecessary code

Issues & Concerns ⚠️

1. Incomplete PR Description - The PR description template was not filled out. It should include what race conditions were observed, how this change fixes them, and any related test failures.

2. Missing Documentation Context - According to the streaming-server-rendering.md documentation (lines 260-277), Redux shared stores with inline script registration require defer: true instead of async: true. The PR correctly uses defer: true, but the explanatory comment in application.html.erb was removed. Consider keeping a brief comment for future reference.

3. Missing Test Coverage - The PR checklist items are unchecked. Consider adding tests that specifically verify no race conditions occur with shared stores, update documentation references, and add a CHANGELOG entry if this fixes a user-visible issue.

4. Configuration Alignment - Good! The spec/dummy/config/initializers/react_on_rails.rb shows config.generated_component_packs_loading_strategy = :defer which aligns well with this change.

[claude]

Security & Performance Considerations

Security 🔒
No security concerns identified. The change is purely about script loading timing.

Performance ⚡
Using defer: true on all pages may slightly delay script execution compared to async: true for pages that do not need it. However, for a test dummy app, consistency and correctness are more important than optimal performance. For production apps, developers should still follow the guidance in the docs.

Recommendations

1. Fill out the PR description with details about what race conditions were observed and how this fixes them

2. Add a brief comment in application.html.erb explaining why defer: true is used for future reference

3. Run the full test suite to ensure no regressions: rake run_rspec:dummy and the Playwright e2e tests

4. Consider adding CHANGELOG entry if this fixes a user-visible issue (following the format in CLAUDE.md)

5. Verify RuboCop compliance before merging: bundle exec rubocop

Verdict

This is a good simplification that makes the dummy app more maintainable and fixes potential race conditions. The code quality is good, but the PR needs better description of the problem being solved, completion of the checklist items, and verification that all tests pass.

Once these items are addressed, this should be good to merge! 👍