Fix missing badge for Turbo Drive page loads

[coalest]

Resolves #612

Git bisect pointed to turbo-rails repo commit d18b2ee being the cause of this bug. Previously Turbo Drive requests were displayed via the callback added to the window.fetch. That turbo-rails commit however change it so that a reference to window.fetch was stored in a variable when Turbo JS is loaded (before rack-mini-profiler has added the then callback) meaning that the callback to display those profiling request results doesn't get added to Turbo requests.

Adding a callback to window.Turbo.fetch is difficult as window.Turbo is a frozen object, so instead a turbo event listener is used.

[coalest]

Nevermind this only works for previous versions of Turbo, not the most recent.

[webzorg]

I just tried this and it works for me? What did not work for you?

Rails 8.0.1
turbo-rails (2.0.11) (this one is latest)

[coalest]

Whoops, when I tested this my browser was caching an old includes.js file without these changes.

I tried again and it does seem to work. Thanks for trying this out @webzorg

[webzorg]

@coalest hehe least I can do, primarily, thank you for keeping this gem healthy and clean =D can't wait to change this to master =D gem "rack-mini-profiler", path: "../rack-mini-profiler"

[pinzonjulian]

Hey! I'm giving it a try in the reproduction app I linked in the issue and it works but there's still a missing piece regarding new defaults in Turbo 8.

mini-profiler-prefetch-issue.mov

The issue here is due to the default setting in Turbo Rails for prefetching links on hover:

You'll notice in this video that:

• It still works for non-turbo requests
• On hover, mini-profiler does its job and changes the widget
• when you hover and then click the link, the badge disappears.

I'm also trying it out at my company and it doesn't seem to work so I'm trying to figure out what might be causing it. I'll report back with any findings

[coalest]

@pinzonjulian Thanks for testing it out. I was also surprised by that behavior at first. But I think it's an artifact due to how Turbo works.

Caching is disabled by RMP, which is why Turbo keeps fetching each of the partials on hover (even multiple times). So you might expect that even if you click on a link that has been prefetched, that Turbo would make another request once you perform the click (and you would see the badge after clicking). But Turbo caches the last request and uses an event listener to add that cached response (source). So if the link clicked matches the url of the last request, it just uses that cached response (source).

So the badge gets an additonal line item whenever a request gets sent, and when you click to a turbo link the badge stays if that link was clicked quickly (without hovering beforehand), otherwise that request gets added to the badge but clicking resets the badge to being empty again.

Regardless though of why it has this behavior, is this behavior ok or should it be fixed? If we can agree on what the desired behavior is, I can change it. Should the entire badge stay (with all the previous requests) when clicking? Should just the very last fetch request remain (the one for the link being clicked)?

[coalest]

If it's a matter of keeping the badge with all requests, all we would have to do is remove the call to pageTransition when clicking a turbo link:

diff --git a/lib/html/includes.js b/lib/html/includes.js
index 1c7438d..c12fa0e 100644
--- a/lib/html/includes.js
+++ b/lib/html/includes.js
@@ -548,7 +548,6 @@ var _MiniProfiler = (function() {
   var onTurboBeforeVisit = function onTurboBeforeVisit(e) {
     if(!e.defaultPrevented) {
       window.MiniProfilerContainer = document.querySelector('body > .profiler-results')
-      window.MiniProfiler.pageTransition()
     }
   }

Just keeping the last prefetched request would be a little more involved.

[pinzonjulian]

From a UX standpoint, when I use RMP I expect the badge to never disappear. In the video I sent I carefully hover and don't click to show the issue but in real world usage, I would just click without even realising prefetching is happening and expect the badge to be there.

It might get a tad confusing with prefetch requests piling up in the badge while you hover around the page but I think that if you're working on performance of a Turbo app, you know that's the case so I don't think it'd be an issue.

[nateberkopec]

I'd rather have prefetch results in the speed badge and then have a way to ignore them, later (which may already be possible with current config settings).

[nateberkopec]

Sorry, I realize my last comment wasn't helpful because that wasn't what you asked.

Should the entire badge stay (with all the previous requests) when clicking? Should just the very last fetch request remain (the one for the link being clicked)?

The latter. Presumably they're the same request (right?)

It might get a tad confusing with prefetch requests piling up in the badge while you hover around the page

To be honest, I like the behavior in the video.

Do we have precedent around how the badge works re: Turbo requests in the past? I like the way the video works but I also don't want to break anybody's expectations as to what we did before.

[coalest]

In order to see what the previous behavior was like, I tested this with RMP on the current master branch and Turbo on the last commit that works (Turbo 8.0.0-beta.1 - before they started storing window.fetch in a variable). And Turbo wasn't prefetching requests on hover, so clicking a Turbo Drive link and a normal link behave very similar. So I think we don't have precedence for how RMP should behave here?

Regarding this change suggested: someone hovers over a Turbo Drive link, it is prefetched and results are added to RMP badge, then clicks on the link, then we wipe all previous results from the badge except that prefetch request.

My only worry is that it could also confuse a user, in that they may think that clicking on the link triggered a request because the badge changed (5 requests 1000ms => 1 request 200ms for example). So RMP is kind of hiding the behavior of Turbo. Normally, I assume that whenever the RMP badge has updated that means a new request was just sent.

[nateberkopec]

Thanks for checking @coalest. Tbh I think this is just another one of the ways that RMP was designed around 2010s-era web applications. Probably the best solution would be for the "badge" to be more of a dynamic log that displayed the first 10 entries by default but let you scroll through history, and basically just kept the last 100 requests or something. But that's a much bigger lift.

I like the behavior displayed in the video. 🤷

[leonvogt]

I'm really looking forward to this PR!
It would be awesome to have better Turbo integration (again).

I feel the same as @pinzonjulian.
I would expect the badge to never disappear. I use prefetch in some of my apps and think it would be kind of confusing why the badge disappears after clicking on a link.

[coalest]

I just pushed commit 6f7f37d which should preserve the last prefetched result in the badge. One drawback is that the workaround is pretty tightly coupled with Turbo (I use Turbo.session.view.lastRenderedLocation.href to check if the Turbo is going to send a request or use it's cache). Although we are already relying on Turbo-specific event listeners, so maybe that's not a big deal.

I tested it myself and it looks right, but could someone else test it out locally too?

[leonvogt]

I briefly tried it in my app, and it works wonderfully!
Both for links with prefetch and links without prefetch, I see the badge with the latest request information after navigating. 👍

[pinzonjulian]

I can confirm that it works well in the reproduction app!

mini-profiler-working.mov

Sadly I still haven't been able to make it work at my company; the badge just keeps disappearing. I'm guessing it's something on our side now that this is solved.

Thanks for tackling it!

[coalest]

@pinzonjulian Bummer it's still not appearing on your company's app. I will take a look at turbo frames and turbo streams next to see if anything can be improved there.

[coalest]

@nateberkopec Would it be possible to get a code review on this PR? Or would you prefer a different maintainer to review it?