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

Prevent Refresh from interrupting ongoing Visit #1213

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

seanpdoyle
Copy link
Contributor

Closes #1209

Defers the Session.refresh calls while handling <turbo-stream action="refresh"> elements if there is a Visit that's already initiated and being handled by the Navigator.

@rbclark
Copy link

rbclark commented Mar 1, 2024

Thank you for this! I've been unable to reproduce #1209 with these changes applied and everything else seems to still be functioning properly. Really appreciate the quick response!

@seanpdoyle
Copy link
Contributor Author

@afcapel @jorgemanrubia are either of you available to review this change?

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanpdoyle thanks for fixing this one!

@@ -125,6 +125,7 @@ export class Navigator {

visitCompleted(visit) {
this.delegate.visitCompleted(visit)
this.stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that, before, it was invoking stop only when starting visits and submitting forms. This looks correct but, since this is a pretty important path, any concerns about introducing unwanted side effects here? I can't think of anything in particular, but concerned about interrupting form submissions we were not interrupting before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct but, since this is a pretty important path, any concerns about introducing unwanted side effects here?

Now that you mention it, a Visit initiated by a link click is guaranteed to either do nothing (when canceled) or navigated. A form, on the other hand, has the potential to complete and succeed without navigating when handling stream responses. The same is true for a link click with [data-turbo-stream].

Acknowledging that, what is the desired order of operations for a Page Refresh that's competing with a Stream form submission?

It feels like the Visit should always win out, since that's requesting the most (and most up-to-date) content. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgemanrubia I've pushed up a change to remove the call to this.stop(), and replace it with delete this.currentVisit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgemanrubia are you available to re-review this?

@seanpdoyle seanpdoyle force-pushed the issue-1209 branch 2 times, most recently from be4c81f to 19267e7 Compare March 5, 2024 12:35
Closes [hotwired#1209]

Defers the `Session.refresh` calls while handling `<turbo-stream
action="refresh">` elements if there is a `Visit` that's already
initiated and being handled by the `Navigator`.

[hotwired#1209]: hotwired#1209
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @seanpdoyle.

@jorgemanrubia
Copy link
Member

This one is good to merge @afcapel from my side.

@klevo
Copy link

klevo commented Apr 20, 2024

I'm not sure this resolves the issue. I have reported a similar issue separately and after @seanpdoyle 's notification tested this patch, but it does not prevent the interruption in my test app.

@klevo
Copy link

klevo commented Apr 22, 2024

I'm not sure this resolves the issue. I have reported a similar issue separately and after @seanpdoyle 's notification tested this patch, but it does not prevent the interruption in my test app.

Actually these are most likely two separate issues. The issue I was referring to and opened a PR for is a scenario where user navigation succeeds but is then overridden by a debounced refresh from previous page. This one is about interrupting in-progress visits. So this is well worth merging 👍

@brunoprietog brunoprietog merged commit 1a0aeb6 into hotwired:main Jun 21, 2024
1 check passed
@seanpdoyle seanpdoyle deleted the issue-1209 branch June 21, 2024 18:19
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.

Turbo stream refresh takes precedence over user interaction
6 participants