Do not invalidate pages in visits that don't render #1144

afcapel · 2024-01-29T17:39:22Z

Fixes #1047
Supersedes #1143

Rendering a frame with a data-turbo-action was rendering the response twice.

First, the FrameController in charge of the frame renders the response using a FrameRenderer, while processing callback the loadResponse callback.

turbo/src/core/frames/frame_controller.js

Line 314 in b7187f1

await this.view.render(renderer)

But when we update a frame with a data-turbo-action, we also issue a visit to update the browser URL & history:

turbo/src/core/frames/frame_controller.js

Line 373 in b7187f1

session.visit(frame.src, options)

This session visit was also triggering a loadResponse callback, which in turn was instantiating a PageSnapshot and rendering it.

This caused problems when the response for the frame didn't include all the tracked assets in the page. The page snapshot considered the assets stale and triggered a reload of the page with reason tracked_element_mismatch.

This commit fixes the issue by not rendering the page snapshot when navigating from a frame.

The commit includes a new test that reproduces the issue returning a turbo frame in an HTML document with an empty <head>. Before this fix, the response would trigger a full page reload.

This is the call stack before the fix. Notice the two calls to loadResponse.

Fixes #1047 Rendering a frame with a data-turbo-action was rendering the response twice. First, the `FrameController` in charge of the frame renders the response using a FrameRenderer. https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L314 But when we update a frame with a data-turbo-action, we also issue a visit to update the browser URL & history: https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L373 This session visit was also triggering a loadResponse callback, which in turn was instantiating a `PageSnapshot` and rendering it. This caused problems when the response for the frame didn't include all the tracked assets in the page. The page snapshot considered the assets stale and triggered a reload of the page with reason `tracked_element_mismatch`. This commit fixes the issue by not rendering the page snapshot when navigating within a frame. The commit includes a new test that reproduces the issue returning a turbo frame in an HTML document with an empty `<head>`. Before this fix, the response would trigger a full page reload.

src/core/frames/frame_snapshot.js

domchristie · 2024-01-29T20:43:06Z

Nice! A couple of things:

Is fromFrame set for the same purpose as willRender. As far as I can tell, willRender is only set to false for this frame visit case, in which case, could we just re-use that?

What is the frame visit actually doing? It doesn’t render and it’s instantiated with updateHistory: false. It looks like FrameController#changeHistory is already called and it doesn’t render, so is it needed at all?

domchristie · 2024-01-30T09:57:24Z

What is the frame visit actually doing? It doesn’t render and it’s instantiated with updateHistory: false. It looks like FrameController#changeHistory is already called and it doesn’t render, so is it needed at all?

😵‍💫 From the looks of the failing tests (any my own brief manual tests), it looks like the visit is doing a fair bit (on the caching front?)

Skipping the rendering when the visit comes from a frame navigation breaks a few tests related to caching. Instead, let's make sure we don't invalidate the view if the visit is not rendering.

afcapel · 2024-01-30T10:19:31Z

As far as I can tell, willRender is only set to false for this frame visit case, in which case, could we just re-use that?

That's a good idea. I've updated the PR so now we use willRender to ensure we are not invalidating a page in a visit that wouldn't render anyway.

What is the frame visit actually doing? It doesn’t render and it’s instantiated with updateHistory: false. It looks like FrameController#changeHistory is already called and it doesn’t render, so is it needed at all?

This was introduced in #398 to add navigation-like behavior to to frame navigations with data-turbo-action. It's a bit difficult to follow everything that's happening, and it'd be nice if we had some abstractions to do this without having to issue a new visit but that's a big change that would require changing many internal parts of Turbo.

For now this small patch fixes #1047, which is the main blocker for a Turbo 8 final release. If we're happy with this I think are ready for the final release.

kevinmcconnell · 2024-01-30T10:38:56Z

src/core/view.js

@@ -77,7 +78,7 @@ export class View {
        this.#resolveRenderPromise(undefined)
        delete this.renderPromise
      }
-    } else {
+    } else if (willRender) {


I don't think it necessarily needs addressed here, but I find it confusing that we have shouldRender and willRender cases. It's not clear how they are different. If you should render, doesn't that imply that you will?

I think the easiest answer is probably to rename willRender. It really means that the view is being used in a context where rendering is not required. Or more specifically, it's being used to do all the other operations that normally accompany a page render, but after a frame update has already rendered.

But ideally we'd separate the responsibilities to avoid this sort of conditional check happening here. So maybe we're as well to stick with the existing language for now, and then look at refactoring in another PR which keeps the same behaviour?

Yes, agreed to all of that 👍

I agree the shouldRender / willRender is confusing here.

Is there a plan to refactor before releasing v8? If not then I reckon we should clarify willRender by renaming and adding a comment. e.g.:

// A workaround to ignore tracked element mismatch reloads when performing a promoted Visit from a frame navigation const shouldInvalidate = willRender

I'd opened #741 to limit the occurrence of willRender.

I'd also opened #430 to try and introduce some abstractions for Frame navigation. They've both been kept up to date originally, but I eventually stopped dragging them forward. I've rebased them to catch up with the latest.

Would either be worthwhile to consider for inclusion in 8?

brunoprietog · 2024-01-30T11:36:06Z

If we're happy with this I think are ready for the final release.

@afcapel Sorry to bother with this but, have you considered any of these concerns before releasing the stable version? #1080 (Turbo crashes completely, I opened #1098 to address it). Other concerns with full page reloading when assets are diferent #1105 and #1116. Thanks!

pfeiffer · 2024-01-30T13:49:19Z

If we're happy with this I think are ready for the final release.

@afcapel Sorry to bother with this but, have you considered any of these concerns before releasing the stable version? #1080 (Turbo crashes completely, I opened #1098 to address it)

I second that closing #1080 (causing blank pages with replace navigations!), would need to be part of a stable release. The potential fix in #1098 makes a lot of sense. We're seeing flashes of unwanted content (stale snapshots) while performing refreshes with morphing. It kinda defeats the purpose of the new shiny morph refreshes.

Let me know if we can do anything to help out here?

afcapel · 2024-01-30T14:32:38Z

Ok, I'll do a final round before publishing a RC 👍

Aiming to fix:

All of them have PRs already open to address them, so it's a matter of validating and merging.

Anything else we should consider a blocker?

pfeiffer · 2024-01-30T14:52:11Z

Anything else we should consider a blocker?

Would #1123 make sense to include as well?

brunoprietog · 2024-01-30T14:59:20Z

@seanpdoyle has made a big effort with contributions and there are still many unmerged, several bug fixes and some very old ones. If you have time, it would be great if you could review as much as you can, obviously within your availability.

afcapel · 2024-01-30T15:35:06Z

Sure, I'd also like to clean up the remaining PRs, but unfortunately right now I'm focused on another project and have limited availability. We could always do more, but for context, we've already merged 72 PRs since v7.3.0.

seanpdoyle · 2024-01-30T15:49:15Z

Out of the pull requests that I've opened at the moment, I consider two worth merging into 8:

Drive turbo-frame with Turbo.visit(url, { frame:, action: }) #1135
- closes changing turbo frame src manually doesn't update main url #468
- closes data-turbo-action="advance" doesn't update browser URL #489
Morph with ignoreActiveValue: true #1141 (but depends on idiomorph@main)

The others either require additional troubleshooting, or are refactors and abstractions.

afcapel · 2024-01-30T15:56:16Z

Cool, thanks @seanpdoyle I'll be sure to review those two as well before the RC.

As for other refactorings, they are of course welcome but they'll probably have to wait until after the v8 release.

afcapel requested a review from kevinmcconnell January 29, 2024 17:39

afcapel mentioned this pull request Jan 29, 2024

Don't include Turbo-Frame header in promoted frame navigations #1143

Closed

seanpdoyle reviewed Jan 29, 2024

View reviewed changes

src/core/frames/frame_snapshot.js Outdated Show resolved Hide resolved

afcapel force-pushed the no-page-render branch from c397232 to 3709bb1 Compare January 29, 2024 17:44

Do not invalidate if the visit is not rendering

1dfb6fd

Skipping the rendering when the visit comes from a frame navigation breaks a few tests related to caching. Instead, let's make sure we don't invalidate the view if the visit is not rendering.

afcapel changed the title ~~Do not render page snapshots when navigating within a frame~~ Do not invalidate pages in visits that don't render Jan 30, 2024

kevinmcconnell approved these changes Jan 30, 2024

View reviewed changes

Add clarifying comment

e2b26d7

afcapel merged commit c740fc5 into main Jan 30, 2024
2 checks passed

afcapel deleted the no-page-render branch January 30, 2024 18:44

seanpdoyle mentioned this pull request Feb 21, 2024

Turbo Frame Responses missing tracked elements hotwired/turbo-rails#580

Open

YoutacRandS-VA mentioned this pull request Jun 19, 2024

[Snyk] Upgrade @hotwired/turbo from 7.3.0 to 8.0.4 YoutacRandS-VA/osv.dev#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not invalidate pages in visits that don't render #1144

Do not invalidate pages in visits that don't render #1144

afcapel commented Jan 29, 2024

domchristie commented Jan 29, 2024

domchristie commented Jan 30, 2024

afcapel commented Jan 30, 2024

kevinmcconnell Jan 30, 2024

afcapel Jan 30, 2024

domchristie Jan 30, 2024

seanpdoyle Jan 30, 2024 •

edited

Loading

brunoprietog commented Jan 30, 2024

pfeiffer commented Jan 30, 2024

afcapel commented Jan 30, 2024 •

edited

Loading

pfeiffer commented Jan 30, 2024

brunoprietog commented Jan 30, 2024

afcapel commented Jan 30, 2024

seanpdoyle commented Jan 30, 2024 •

edited

Loading

afcapel commented Jan 30, 2024

Do not invalidate pages in visits that don't render #1144

Do not invalidate pages in visits that don't render #1144

Conversation

afcapel commented Jan 29, 2024

domchristie commented Jan 29, 2024

domchristie commented Jan 30, 2024

afcapel commented Jan 30, 2024

kevinmcconnell Jan 30, 2024

Choose a reason for hiding this comment

afcapel Jan 30, 2024

Choose a reason for hiding this comment

domchristie Jan 30, 2024

Choose a reason for hiding this comment

seanpdoyle Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

brunoprietog commented Jan 30, 2024

pfeiffer commented Jan 30, 2024

afcapel commented Jan 30, 2024 • edited Loading

pfeiffer commented Jan 30, 2024

brunoprietog commented Jan 30, 2024

afcapel commented Jan 30, 2024

seanpdoyle commented Jan 30, 2024 • edited Loading

afcapel commented Jan 30, 2024

seanpdoyle Jan 30, 2024 •

edited

Loading

afcapel commented Jan 30, 2024 •

edited

Loading

seanpdoyle commented Jan 30, 2024 •

edited

Loading