Skip to content

Wait for body to be replaced before caching snapshot when <head> changes #1494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
janko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Wait for body to be replaced before caching snapshot when <head> changes #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core/drive/page_view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export class PageView extends View {
this.delegate.viewWillCacheSnapshot()
const { lastRenderedLocation: location } = this
await nextEventLoopTick()
if (this.renderPromise) await this.renderPromise
const cachedSnapshot = snapshot.clone()
this.snapshotCache.put(location, cachedSnapshot)
return cachedSnapshot
Expand Down
60 changes: 60 additions & 0 deletions src/tests/fixtures/cache_racing.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Turbo</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
<script>
// Simulates a Stimulus controller that uses MutationObserver to detect
// when its element is removed from the DOM and cleans up accordingly.
// This matches how Stimulus actually works internally.
(function() {
let paragraph = null
let observer = null

function connect() {
const container = document.getElementById("component-container")
if (!container || paragraph) return

paragraph = document.createElement("p")
paragraph.id = "component-output"
paragraph.textContent = "Hello from component"
container.appendChild(paragraph)

// Watch for when our container is removed from the DOM (like Stimulus does)
observer = new MutationObserver((mutations) => {
for (const mutation of mutations) {
for (const node of mutation.removedNodes) {
if (node === container || node.contains?.(container)) {
disconnect()
return
}
}
}
})
// Must observe documentElement, not body, because Turbo replaces the entire body
observer.observe(document.documentElement, { childList: true, subtree: true })
}

function disconnect() {
if (paragraph) {
paragraph.remove()
paragraph = null
}
if (observer) {
observer.disconnect()
observer = null
}
}

addEventListener("turbo:load", connect)
})()
</script>
</head>
<body>
<h1>Cache Racing Test</h1>
<div id="component-container"></div>
<p><a id="link-to-page-with-different-head" href="/src/tests/fixtures/cache_racing_target.html">Page with different head</a></p>
</body>
</html>
15 changes: 15 additions & 0 deletions src/tests/fixtures/cache_racing_target.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Turbo</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
<!-- Different head content triggers head merging during render -->
<style></style>
</head>
<body>
<h1>Target Page</h1>
<p>This page has different head content (an extra style tag).</p>
</body>
</html>
35 changes: 35 additions & 0 deletions src/tests/functional/cache_racing_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect, test } from "@playwright/test"
import { nextEventNamed } from "../helpers/page"

test("caches snapshot after rendering completes to avoid element duplication", async ({ page }) => {
// This test verifies the fix for https://github.com/hotwired/turbo/issues/1397
//
// The bug: cacheSnapshot() is called without being awaited (visit.js), so it
// runs concurrently with rendering. Components like Stimulus controllers use
// MutationObserver to detect when their elements are removed from the DOM and
// clean up accordingly. Without waiting for the render to complete before
// cloning, snapshot.clone() could capture the DOM before cleanup completes,
// causing component elements to be duplicated when restoring from cache.
//
// The fix: cacheSnapshot() now awaits renderPromise before cloning, ensuring
// the DOM is in a consistent state after render and cleanup complete.

await page.goto("/src/tests/fixtures/cache_racing.html")
await nextEventNamed(page, "turbo:load")

// Verify component mounted exactly once
await expect(page.locator("#component-output")).toHaveCount(1)
await expect(page.locator("#component-output")).toHaveText("Hello from component")

// Navigate to a page with different <head> content (triggers the race condition)
await page.click("#link-to-page-with-different-head")
await nextEventNamed(page, "turbo:load")

// Go back - the page should be restored from cache
await page.goBack()
await nextEventNamed(page, "turbo:load")

// The component should appear exactly once, not duplicated
await expect(page.locator("#component-output")).toHaveCount(1)
await expect(page.locator("#component-output")).toHaveText("Hello from component")
})