Fix Turbo navigation script tag

[justin808]

Summary

• Changed from deprecated data-turbolinks-track to modern data-turbo-track: 'reload'
• Explicitly added defer: true for clarity
• Fixes Turbo navigation issues in the dummy app

Test plan

• Verify dummy app loads correctly
• Confirm Turbo navigation works as expected

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

Release Notes

• Chores
  • Updated script loading configuration for improved performance and compatibility with current framework standards.

[coderabbitai]

Walkthrough

Updated the script tag in the application layout view to replace Turbolinks tracking with Turbo tracking. Changed from data-turbolinks-track: true to data-turbo-track: 'reload' and added the defer: true attribute for proper script loading behavior.

Changes

Cohort / File(s)	Change Summary
Turbolinks to Turbo Migration spec/dummy/app/views/layouts/application.html.erb	Updated Rails view helper call replacing data-turbolinks-track: true with data-turbo-track: 'reload' and added defer: true attribute for deferred script execution.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

• Single-file change with straightforward attribute replacement
• No logic modifications or functional impact beyond script loading behavior

Poem

🐰 From old tracks to newer rails we hop,
Turbo spins where Turbolinks would stop,
With defer: true and a reload so bright,
Scripts now load in proper order—what a delight! ✨

Pre-merge checks and finishing touches

✅ 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 describes the main change: updating the script tag for Turbo navigation compatibility.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/turbo-script-fix

---

📜 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 1a84b4a and d5a5f27.

📒 Files selected for processing (1)

• spec/dummy/app/views/layouts/application.html.erb (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.

📚 Learning: 2024-10-08T20:53:47.076Z

Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

• spec/dummy/app/views/layouts/application.html.erb

📚 Learning: 2025-02-13T16:50:26.861Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.

Applied to files:

• spec/dummy/app/views/layouts/application.html.erb

🔇 Additional comments (1)

spec/dummy/app/views/layouts/application.html.erb (1)

13-13: LGTM! Correct migration from Turbolinks to Turbo.

The change correctly replaces the deprecated data-turbolinks-track with data-turbo-track: 'reload', which is the standard attribute for Turbo. Adding defer: true explicitly is good for clarity and aligns with the comment on line 12.

---

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: Fix Turbo Navigation Script Tag

Summary

This PR fixes a critical Turbo navigation bug by updating from deprecated data-turbolinks-track to the modern data-turbo-track: 'reload' attribute. The change is correct and addresses a long-standing regression.

---

✅ Positive Findings

1. Correct Migration Path

• ✅ Changed from deprecated data-turbolinks-track: true to data-turbo-track: 'reload'
• ✅ Explicitly added defer: true which is critical for Turbo compatibility
• ✅ Aligns with the project's own documentation (docs/building-features/turbolinks.md:132-136)

2. Well-Documented Fix

• ✅ Clear PR description with test plan
• ✅ References the underlying issue (TURBO_NAVIGATION_TEST_ISSUE.md)
• ✅ Minimal, focused change (1 line modified)

3. Follows Best Practices

Per the Turbo documentation for Rails, the correct syntax for Turbo 7+ is:

data-turbo-track: 'reload'

The defer: true attribute is explicitly mentioned in the inline comment and is necessary to ensure scripts load in the correct order with Turbo navigation.

---

⚠️ Issues Found

1. CRITICAL: Pro Version Has Same Bug 🔴

The same deprecated attribute exists in the Pro version:

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

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

Should be updated to:

<%= javascript_pack_tag('client-bundle', 'data-turbo-track': 'reload', defer: true) %>

Note: Pro version also uses defer: false which should likely be defer: true for consistency, unless there's a specific reason for the difference.

Also on line 23-25, the stylesheet_pack_tag has:

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

Should be:

<%= stylesheet_pack_tag('client-bundle',
                        media: 'all',
                        'data-turbo-track': 'reload') %>

---

2. Missing Test Coverage 🟡

As noted in TURBO_NAVIGATION_TEST_ISSUE.md, this bug existed for over a year undetected. The test plan in the PR description is manual:

• Verify dummy app loads correctly
• Confirm Turbo navigation works as expected

Recommendation: Consider implementing the automated Playwright test described in TURBO_NAVIGATION_TEST_ISSUE.md to prevent future regressions.

---

3. Changelog Entry 🟡

According to CLAUDE.md, this qualifies as a user-visible bug fix and should have a changelog entry. Current PR doesn't include one.

Suggested entry (for CHANGELOG.md under #### Fixed):

- Fixed Turbo navigation by updating from deprecated `data-turbolinks-track` to `data-turbo-track: 'reload'` in dummy app layout. [PR 1906](https://github.com/shakacode/react_on_rails/pull/1906) by [justin808](https://github.com/justin808)

---

🔍 Code Quality Assessment

Security

✅ No security concerns - this is a frontend attribute change only

Performance

✅ Actually improves performance by enabling proper Turbo caching/reloading
✅ The defer: true attribute ensures scripts don't block HTML parsing

Best Practices

✅ Follows Rails/Turbo conventions
✅ Maintains existing inline documentation (comment about defer vs async)
✅ Minimal, focused change

Potential Bugs

⚠️ Pro version needs the same fix (see Issue #1 above)
✅ No other issues identified in the changed code

---

📋 Pre-Merge Checklist

Before merging, please ensure:

Required (per CLAUDE.md)

• Run bundle exec rubocop - must pass with zero offenses
• Ensure file ends with newline character (appears correct in diff)
• Consider adding changelog entry (see suggestion above)

Recommended

• Apply the same fix to react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
• Update both javascript_pack_tag (line 29) and stylesheet_pack_tag (line 23-25) in Pro version
• Manually test Turbo navigation in both dummy apps
• Consider implementing automated Playwright test for regression prevention

---

🎯 Recommendation

Approve with conditions: This fix is correct and necessary, but should be expanded to include the Pro version before merging.

Suggested Action Plan:

1. Apply identical changes to react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
2. Update both javascript_pack_tag and stylesheet_pack_tag in Pro version
3. Decide on defer: true vs defer: false for Pro version (recommend true for consistency)
4. Run bundle exec rubocop to ensure compliance
5. Optionally add changelog entry
6. Test both dummy apps manually

---

📚 References

• Turbo Documentation on Track Attributes
• Project docs: docs/building-features/turbolinks.md:129-136
• Issue context: TURBO_NAVIGATION_TEST_ISSUE.md
• Pro version file: react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:23,29

---

Great work identifying and fixing this regression! Just needs to be applied to the Pro version as well. 🚀

[claude]

PR Review: Fix Turbo navigation script tag

Summary

This PR updates the dummy app to use modern Turbo syntax instead of deprecated Turbolinks syntax. The change is small but important for compatibility.

---

Code Quality ✅

Positive aspects:

• ✅ The change correctly updates from deprecated data-turbolinks-track: true to modern data-turbo-track: 'reload'
• ✅ Explicitly adding defer: true improves code clarity (though javascript_pack_tag uses defer by default in modern Shakapacker)
• ✅ The comment above the line correctly notes that defer (not async) must be used for proper script loading order

---

Issues & Recommendations

🔴 CRITICAL: Inconsistent migration - Pro dummy app not updated

The React on Rails Pro dummy app at react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:25,29 still uses the deprecated syntax:

<%= stylesheet_pack_tag('client-bundle',
                        media: 'all',
                        'data-turbolinks-track': true) %>
...
<%= javascript_pack_tag('client-bundle', 'data-turbolinks-track': true, defer: false) %>

Recommendation: Update both locations in the Pro dummy app for consistency.

---

⚠️ MODERATE: Potential confusion between Turbolinks and Turbo

Context:

• The dummy app's package.json shows @hotwired/turbo-rails v8.0.4 is installed
• However, Gemfile.lock still shows the turbolinks gem (v5.2.1) is installed
• The codebase has extensive Turbolinks 5 support in the JavaScript code (turbolinksUtils.ts, event listeners for turbolinks:before-render, etc.)

Observations:

1. Using data-turbo-track: 'reload' is the correct syntax for Turbo (not Turbolinks)
2. However, if the Rails app is still using the Turbolinks gem (not turbo-rails gem), this change may not work as expected
3. The dummy app appears to be in a transitional state with both libraries present

Questions for clarification:

• Is the dummy app actually using Turbo or Turbolinks at runtime?
• Should this PR also remove the turbolinks gem dependency?
• Are there plans to fully migrate from Turbolinks 5 to Turbo 8?

Recommendation: Clarify the migration strategy and ensure gem dependencies match the view helpers being used.

---

📝 MINOR: Documentation should be updated

The file docs/building-features/turbolinks.md:132-133 still shows examples using the old syntax:

<%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track' => 'reload' %>
<%= javascript_include_tag 'application', 'data-turbolinks-track' => 'reload' %>

Recommendation: If migrating to Turbo, consider updating or adding documentation for Turbo 8 usage.

---

Test Coverage

Current test plan:

• Verify dummy app loads correctly
• Confirm Turbo navigation works as expected

Additional testing recommendations:

1. Test with both Turbolinks gem enabled and disabled (via DISABLE_TURBOLINKS env var)
2. Verify page navigation triggers proper React component cleanup/mounting
3. Check browser console for any Turbo/Turbolinks-related errors
4. Test server-side rendering still works correctly with navigation

---

Performance Considerations ✅

No performance concerns. The defer attribute ensures scripts load asynchronously while maintaining execution order.

---

Security Concerns ✅

No security issues identified.

---

Compatibility Concerns ⚠️

Potential breaking change:

• If users are following the dummy app as an example and still use Turbolinks (not Turbo), this change could cause confusion
• The React on Rails codebase extensively supports Turbolinks 5 but may not have equivalent Turbo 8 integration

---

Suggested Next Steps

1. Decide on migration strategy: Full Turbo migration or maintain Turbolinks support?
2. Update Pro dummy app to match this change
3. Update documentation to reflect Turbo 8 usage (or clarify Turbolinks vs Turbo)
4. Consider updating pageLifecycle.ts to also listen for Turbo events (turbo:before-render, turbo:render) in addition to Turbolinks events
5. Run full test suite including rake run_rspec:dummy and rake run_rspec:dummy_no_turbolinks

---

Changelog Requirement

Per CLAUDE.md, this change should NOT require a changelog entry as it's a test/example app update, not a user-visible gem feature change.

---

Linting Requirements ⚠️

MANDATORY before merge (per CLAUDE.md):

• Run bundle exec rubocop and fix ALL violations
• Ensure file ends with a newline character
• Git hooks should auto-run linting on commit

---

Overall Assessment

Verdict: Conditionally approve with recommended changes

The core change is correct for Turbo 8, but the PR should address the inconsistencies between the open-source and Pro dummy apps, and clarify the Turbolinks→Turbo migration strategy. The change is safe to merge once the Pro version is updated and testing confirms compatibility.

---

🤖 Review generated with Claude Code

[claude]

Code Review - PR #1906: Fix Turbo navigation script tag

Summary

This PR updates the dummy app's layout to use modern Turbo conventions instead of deprecated Turbolinks syntax. The change is straightforward and appropriate.

---

✅ Positive Aspects

1. Correct Migration Path: The change from data-turbolinks-track: true to data-turbo-track: 'reload' is the correct migration path from Turbolinks to Turbo (Hotwire).

2. Explicit defer attribute: Adding defer: true explicitly is good for clarity, even though it may have been implicit before.

3. Aligns with Dependencies: The Gemfile includes both turbo-rails and turbolinks, indicating a transitional state. This change moves the dummy app toward the modern Turbo standard.

---

⚠️ Issues and Concerns

1. Inconsistency Across Codebase (High Priority)

The search revealed multiple locations still using the old data-turbolinks-track syntax:

• react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:25 (line 25)
• react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:29 (line 29)

Recommendation: Update these files for consistency:

# Line 29 should change from:
<%= javascript_pack_tag('client-bundle', 'data-turbolinks-track': true, defer: false) %>

# To:
<%= javascript_pack_tag('client-bundle', 'data-turbo-track': 'reload', defer: false) %>

2. Documentation Needs Update (Medium Priority)

The file docs/building-features/turbolinks.md still shows old Turbolinks syntax in examples (lines 132-136):

<%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track' => 'reload' %>
<%= javascript_include_tag 'application', 'data-turbolinks-track' => 'reload' %>

Recommendation: Either:

• Update documentation to show both Turbolinks (legacy) and Turbo (modern) examples
• Add a migration guide section
• Clearly mark the Turbolinks examples as legacy/deprecated

3. Potential Breaking Change (Low-Medium Priority)

This change might affect:

• Users still using Turbolinks 5.x
• Integration tests that rely on the old attribute name
• Any selenium/system tests that check for specific data attributes

Recommendation: Verify that:

• Dummy app integration tests still pass
• Both turbo-rails and turbolinks gems can coexist (they should, but worth confirming)

4. Missing Context in Comment (Low Priority)

The inline comment on line 12 says:

<!--  NOTE: Must use defer and not async to keep async scripts loading in correct order  -->

This comment could be updated to clarify the Turbo-specific behavior if relevant.

---

🔍 Security Considerations

✅ No security concerns identified. This is a configuration change affecting asset loading behavior.

---

🧪 Test Coverage Recommendations

1. Add a system/integration test that verifies:

   • Turbo navigation works correctly
   • The script tag is rendered with correct attributes
   • Page transitions don't break React component hydration

2. Test both scenarios:

   • With turbo-rails enabled
   • With turbolinks enabled (if backward compatibility is needed)

Example test structure:

# spec/system/turbo_navigation_spec.rb
RSpec.describe "Turbo Navigation", type: :system do
  it "renders script tag with data-turbo-track attribute" do
    visit root_path
    expect(page).to have_css('script[data-turbo-track="reload"]')
  end
end

---

📝 Code Quality Assessment

Aspect	Rating	Notes
Correctness	✅ Good	Syntax is correct for Turbo
Consistency	⚠️ Fair	Pro dummy app needs same update
Documentation	⚠️ Fair	Docs still reference old syntax
Test Coverage	⚠️ Unknown	No test changes in PR
Best Practices	✅ Good	Following modern Turbo conventions

---

🎯 Recommended Actions Before Merge

1. Update Pro dummy app with matching changes to /react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb
2. Update documentation in docs/building-features/turbolinks.md to reflect modern practices
3. Verify test suite passes with the new attribute
4. Consider adding a system test for Turbo navigation (optional but recommended)
5. Update CHANGELOG.md if this affects end users (though it's primarily a test/dummy app change)

---

💡 Additional Observations

• The project currently depends on both turbo-rails (2.0.6) and turbolinks (5.2.1), suggesting a transitional phase
• Consider documenting the migration strategy for users moving from Turbolinks to Turbo
• The change is isolated to test/dummy apps, so impact on production users is minimal

---

Overall Assessment: ✅ Approve with minor changes recommended

The core change is correct and beneficial. Addressing the consistency issues across Pro dummy app and documentation would make this PR excellent.

Great work on modernizing the codebase! 🚀

[claude]

Code Review: Fix Turbo navigation script tag

Summary

This PR updates the javascript_pack_tag in the dummy app's application layout from the deprecated data-turbolinks-track to the modern data-turbo-track: 'reload' attribute, and explicitly adds defer: true.

Positive Aspects

✅ Correct modernization: The change from data-turbolinks-track to data-turbo-track is appropriate given that the app uses both turbo-rails (2.0.6) and legacy turbolinks (5.2.1), and Turbo is the modern replacement.

✅ Explicit defer parameter: Making defer: true explicit improves code clarity and matches the pattern used elsewhere in the codebase (e.g., lib/react_on_rails/helper.rb:479 where defer: true is explicitly passed to append_javascript_pack_tag).

✅ Maintains script loading order: The inline comment correctly notes that defer (not async) is needed to maintain proper script loading order, which is critical for React on Rails.

Concerns & Questions

1. Verify shakapacker's default defer behavior ⚠️

The PR description states this adds "explicit defer: true for clarity", but it's worth confirming whether javascript_pack_tag in shakapacker 9.0.0 already defaults to defer: true. If it does, this is purely a clarity improvement. If it doesn't, this is a behavioral change that should be highlighted and tested.

Action: Verify shakapacker 9.0.0's default behavior for javascript_pack_tag. If this changes the default behavior, the PR description should note this as a bug fix rather than just a clarification.

2. Test coverage ⚠️

The test plan is marked incomplete with empty checkboxes. Given that this affects the main application layout:

Recommended tests:

• ✓ Verify all existing system tests pass (especially spec/dummy/spec/system/integration_spec.rb which tests Turbo functionality)
• ✓ Manually test Turbo navigation between pages
• ✓ Verify the Turbo frame test still works (visit "/turbo_frame_tag_hello_world")

3. Dual Turbo/Turbolinks dependency 🤔

The dummy app has both turbo-rails (2.0.6) and turbolinks (5.2.1) in the Gemfile. This is unusual since Turbo is the successor to Turbolinks.

Questions:

• Is this intentional for testing backward compatibility?
• Should there be a follow-up to remove turbolinks entirely?
• Are there other views still using data-turbolinks-* attributes that need updating?

Verification: Check for other occurrences:

grep -r "turbolinks" spec/dummy/app/views --include="*.erb"

I found spec/dummy/app/views/pages/turbolinks_cache_disabled.erb which may need review.

Recommendations

1. Before merging:

   • ✓ Run bundle exec rubocop (per CLAUDE.md requirements)
   • ✓ Run full test suite: rake run_rspec:dummy
   • ✓ Verify Turbo navigation manually in the dummy app

2. Consider follow-up:

   • Audit all views for remaining data-turbolinks-* references
   • Clarify whether both turbo-rails and turbolinks should coexist
   • Document the defer behavior expectation

Code Quality: ✅ LGTM with verification

The change itself is clean and minimal. The only concern is ensuring proper test coverage and understanding the behavioral implications of the explicit defer: true.

---

Reviewed per: /home/runner/work/react_on_rails/react_on_rails/CLAUDE.md guidelines

[claude]

Code Review: Fix Turbo navigation script tag

Summary

This PR updates the dummy app's layout to use modern Turbo conventions, replacing deprecated data-turbolinks-track with data-turbo-track: 'reload' and explicitly adding defer: true. Overall, this is a good and necessary change that modernizes the codebase.

✅ Positive Findings

1. Correct Migration Path: The change from data-turbolinks-track to data-turbo-track: 'reload' aligns with modern Turbo conventions, as the project has turbo-rails gem in Gemfile.development_dependencies:25.

2. Explicit defer: true: Making the defer option explicit improves code clarity and follows best practices mentioned in the inline comment about script loading order.

3. Minimal Scope: The change is appropriately scoped to the dummy app, which is used for testing.

⚠️ Issues & Recommendations

1. Incomplete Migration - Pro Version Needs Updates

The same deprecated syntax exists in the Pro version's dummy app:

Files needing updates:

• react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:25 - stylesheet_pack_tag with data-turbolinks-track: true
• react_on_rails_pro/spec/dummy/app/views/layouts/application.html.erb:29 - javascript_pack_tag with data-turbolinks-track: true, defer: false

Recommendation: Update these files in the same PR to maintain consistency across the codebase.

2. Missing Test Updates

The existing tests in spec/dummy/spec/helpers/react_on_rails_helper_spec.rb verify that append_javascript_pack_tag is called with { defer: true }. While your change to the layout won't affect these tests directly, consider:

• Verifying that the dummy app still loads correctly with the new attributes
• Checking if any integration/system tests rely on the old Turbolinks behavior

3. Documentation Inconsistency

The documentation at docs/building-features/turbolinks.md:132-133 still shows the old Turbolinks syntax:

<%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track' => 'reload' %>
<%= javascript_include_tag 'application', 'data-turbolinks-track' => 'reload' %>

Recommendation: Update the documentation to show modern Turbo syntax examples, or clearly mark this section as "Legacy Turbolinks" since the doc already has a modern "Using Turbo" section at the top.

4. Consider Adding a Note About the Change

Given that this affects script loading behavior, it would be helpful to add a brief comment in the PR test plan checklist about:

• Verifying that components hydrate correctly
• Checking that no race conditions occur with deferred script loading
• Confirming Turbo navigation events work as expected

🔒 Security & Performance

• Security: No concerns. The change doesn't introduce any security vulnerabilities.
• Performance: The explicit defer: true is appropriate and maintains the correct script loading order as noted in the inline comment.

📋 Code Quality

• Style: Follows Ruby/ERB conventions
• Clarity: The explicit defer: true improves code readability
• Consistency: Once the Pro version is updated, this will be consistent across the codebase

✅ Final Recommendation

Approve with suggested changes:

1. REQUIRED: Update the Pro version's application.html.erb file with the same changes
2. RECOMMENDED: Update documentation to show modern Turbo syntax or clearly separate legacy vs. modern sections
3. NICE TO HAVE: Add a note in the test plan about verifying hydration behavior

Test Plan Additions

Add to your test plan:

• Verify components hydrate correctly with deferred scripts
• Confirm Turbo navigation works without race conditions
• Test that store hydration occurs at the correct time
• Verify no console errors related to script loading

Great work modernizing the codebase! 🚀