Skip to content
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.

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

Fix stale refresh URL caused by debouncing #1250

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

Conversation

klevo
Copy link

@klevo klevo commented Apr 21, 2024

There is a race condition with Turbo 8 refreshes and regular navigation through the application, caused by passing the URL to be refreshed into the debounced function.

If user visits another page, in between the page refresh arriving and the debounced refresh execution, the stale URL in the debounced refresh will navigate back to the page the user just came from.

This behavior is most apparent when there is a steady stream of turbo refreshes coming in and user is navigating through pages that receive these refreshes, but could happen at any (low) volume of page refreshes, if the timing is right.

Here's a video of the issue:

turbo-refresh-overriding-user-visit.mov
  • At the end you can observe I click on "Show this item" of the item named "Two", yet I remain on the index page. You can notice the desired page blinked in and out for a few microseconds too.
  • The cause is visible in the console: The second to last turbo visit to /items/2 is my user initiated action.
  • The following (last) entry in the console is the turbo refresh of the current /items page that just executed after the default debounce delay (you can see the Updated at times changed from from :05 to :06). This refresh essentially hijacked my click and resulted in me being returned to the index page.

This is on Safari 17.4.1. I also replicated the same issue in latest Chrome.

I created a simple Rails 7.1 app to demonstrate the issue that is used in the above video with full instructions.

The fix is rather straightforward: do not pass the refresh URL into the debounced function, rather always use the current browser URL.

@brunoprietog
Copy link
Contributor

This would still produce two requests, right? That's how I understand it. Shouldn't we cancel the page refresh in that case?

@klevo
Copy link
Author

klevo commented Apr 23, 2024

This would still produce two requests, right? That's how I understand it. Shouldn't we cancel the page refresh in that case?

That's correct. I haven't thought about a cancelling solution, but indeed that should be easy with the previous implementation, and comparing the passed URL with current URL and if they don't match, not performing a visit... I'll look into it in a bit.

@klevo
Copy link
Author

klevo commented Apr 23, 2024

  • Implemented approach to not issue a refresh visit if stale URL is detected.
  • Test case added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants