Skip Snapshot Caching for redirect visits (#804)

* Fix flaky Firefox tests Some tests that visit the `navigation.html` fixture exercise scroll behavior by visiting links and other content that is below the fold. The `style="height: 200vh"` value declared on the page's `<section>` element is arbitrary, but double the screen size to force content below the fold. Unluckily, it cuts off _right_ at the beginning of the new `<iframe>` elements in Firefox browsers, which obscures the `<a>` element and prevents link clicks. To resolve that issue, this commit removes the `[style]` attribute, since the content is long enough to be scrollable. * Skip Snapshot Caching for redirect visits Closes [#794][] Reverts the implementation change made in [#674][], and in its place passes `shouldCacheSnapshot: false` alongside `willRender: false` for the `action: "replace"` Visit proposed when following a redirect. In order to test this behavior, this commit introduces the `readBodyMutationLogs` test utility function, along with the `window.bodyMutationLogs` property and the `BodyMutationLog` type. `BodyMutationLog` instances are pushed onto the log `Array` whenever the `<body>` element is replaced. [#794]: #794 [#674]: #674
hotwired · Jan 4, 2023 · e9692e7 · e9692e7
1 parent ee56716
commit e9692e7
Show file tree

Hide file tree

Showing 5 changed files with 43 additions and 0 deletions.
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -317,6 +317,8 @@ export class Visit implements FetchRequestDelegate {
       this.adapter.visitProposedToLocation(this.redirectedToLocation, {
         action: "replace",
         response: this.response,
+        shouldCacheSnapshot: false,
+        willRender: false,
       })
       this.followedRedirect = true
     }

diff --git a/src/tests/fixtures/rendering.html b/src/tests/fixtures/rendering.html
@@ -45,6 +45,7 @@ <h1>Rendering</h1>
       <p><a id="permanent-in-frame-element-link" href="/src/tests/fixtures/permanent_element.html" data-turbo-frame="frame">Permanent element in frame</a></p>
       <p><a id="permanent-in-frame-without-layout-element-link" href="/src/tests/fixtures/frames/without_layout.html" data-turbo-frame="frame">Permanent element in frame without layout</a></p>
       <p><a id="delayed-link" href="/__turbo/delayed_response">Delayed link</a></p>
+      <p><a id="redirect-link" href="/__turbo/redirect">Redirect link</a></p>
       <form>
         <input type="text" id="text-input">
         <input type="radio" id="radio-input">

diff --git a/src/tests/fixtures/test.js b/src/tests/fixtures/test.js
@@ -46,6 +46,17 @@
        }
      }
    }).observe(document, { subtree: true, childList: true, attributes: true })
+
+  window.bodyMutationLogs = []
+  addEventListener("turbo:load", () => {
+    new MutationObserver((mutations) => {
+      for (const { addedNodes } of mutations) {
+        for (const { localName, outerHTML } of addedNodes) {
+          if (localName == "body") bodyMutationLogs.push([outerHTML])
+        }
+      }
+    }).observe(document.documentElement, { childList: true })
+  }, { once: true })
 })([
   "turbo:click",
   "turbo:before-stream-render",

diff --git a/src/tests/functional/rendering_tests.ts b/src/tests/functional/rendering_tests.ts
@@ -6,7 +6,9 @@ import {
   isScrolledToTop,
   nextBeat,
   nextBody,
+  nextBodyMutation,
   nextEventNamed,
+  noNextBodyMutation,
   pathname,
   propertyForSelector,
   readEventLogs,
@@ -571,6 +573,13 @@ test("test error pages", async ({ page }) => {
   assert.equal(await page.textContent("body"), "404 Not Found: /nonexistent\n")
 })
 
+test("test rendering a redirect response replaces the body once and only once", async ({ page }) => {
+  await page.click("#redirect-link")
+  await nextBodyMutation(page)
+
+  assert.ok(await noNextBodyMutation(page), "replaces <body> element once")
+})
+
 function deepElementsEqual(
   page: Page,
   left: JSHandle<SVGElement | HTMLElement>[],

diff --git a/src/tests/helpers/page.ts b/src/tests/helpers/page.ts
@@ -10,6 +10,9 @@ type MutationAttributeName = string
 type MutationAttributeValue = string | null
 type MutationLog = [MutationAttributeName, Target, MutationAttributeValue]
 
+type BodyHTML = string
+type BodyMutationLog = [BodyHTML]
+
 export function attributeForSelector(page: Page, selector: string, attributeName: string): Promise<string | null> {
   return page.locator(selector).getAttribute(attributeName)
 }
@@ -112,6 +115,19 @@ export async function listenForEventOnTarget(page: Page, elementId: string, even
   }, eventName)
 }
 
+export async function nextBodyMutation(page: Page): Promise<string | null> {
+  let record: BodyMutationLog | undefined
+  while (!record) {
+    ;[record] = await readBodyMutationLogs(page, 1)
+  }
+  return record[0]
+}
+
+export async function noNextBodyMutation(page: Page): Promise<boolean> {
+  const records = await readBodyMutationLogs(page, 1)
+  return !records.some((record) => !!record)
+}
+
 export async function nextAttributeMutationNamed(
   page: Page,
   elementId: string,
@@ -185,6 +201,10 @@ async function readArray<T>(page: Page, identifier: string, length?: number): Pr
   )
 }
 
+export function readBodyMutationLogs(page: Page, length?: number): Promise<BodyMutationLog[]> {
+  return readArray<BodyMutationLog>(page, "bodyMutationLogs", length)
+}
+
 export function readEventLogs(page: Page, length?: number): Promise<EventLog[]> {
   return readArray<EventLog>(page, "eventLogs", length)
 }