From 6da938fc74357e3555408262cdd4c13bab0b0cc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janko=20Marohni=C4=87?= Date: Mon, 2 Feb 2026 10:29:13 +0000 Subject: [PATCH] Wait for body to be replaced before caching snapshot when `` changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When navigating to a page with new `` children, the body replacement will be delayed by evaluating the new stylesheet/script elements. In Stimulus context, this means any controller `disconnect()` callbacks will be delayed as well. Because there is currently no waiting for the body to be replaced before capturing a snapshot of the previous page, the disconnect callbacks might not have finished in time. This can result in Turbo including dynamicaly created UI components in the page snapshot that didn't have time to tear down. On restoration visit, Turbo will load the cached snapshot together with the UI components, and then Stimulus `connect()` callbacks will create them again, resulting in duplicated UI components. We can mitigate this by waiting for the new page to render before capturing the snapshot – i.e. body to finish replacing – ensuring any `disconnect()` callbacks will have finished by then. --- src/core/drive/page_view.js | 1 + src/tests/fixtures/cache_racing.html | 60 +++++++++++++++++++++ src/tests/fixtures/cache_racing_target.html | 15 ++++++ src/tests/functional/cache_racing_tests.js | 35 ++++++++++++ 4 files changed, 111 insertions(+) create mode 100644 src/tests/fixtures/cache_racing.html create mode 100644 src/tests/fixtures/cache_racing_target.html create mode 100644 src/tests/functional/cache_racing_tests.js diff --git a/src/core/drive/page_view.js b/src/core/drive/page_view.js index 813566a1a..79f12f9fc 100644 --- a/src/core/drive/page_view.js +++ b/src/core/drive/page_view.js @@ -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 diff --git a/src/tests/fixtures/cache_racing.html b/src/tests/fixtures/cache_racing.html new file mode 100644 index 000000000..0a41609d0 --- /dev/null +++ b/src/tests/fixtures/cache_racing.html @@ -0,0 +1,60 @@ + + + + + Turbo + + + + + +

Cache Racing Test

+
+

Page with different head

+ + diff --git a/src/tests/fixtures/cache_racing_target.html b/src/tests/fixtures/cache_racing_target.html new file mode 100644 index 000000000..cea9ba103 --- /dev/null +++ b/src/tests/fixtures/cache_racing_target.html @@ -0,0 +1,15 @@ + + + + + Turbo + + + + + + +

Target Page

+

This page has different head content (an extra style tag).

+ + diff --git a/src/tests/functional/cache_racing_tests.js b/src/tests/functional/cache_racing_tests.js new file mode 100644 index 000000000..ecb095b32 --- /dev/null +++ b/src/tests/functional/cache_racing_tests.js @@ -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 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") +})