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

Bugfix: Do not attempt to sign user out if clientNeedsToRefresh is true #5378

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

Mwindo
Copy link
Contributor

@Mwindo Mwindo commented Sep 18, 2024

I ran into an error today on test. I had two tabs open:

  1. Old tab needed to refresh (due to a deploy to test)
  2. Current tab needed to refresh (due to that same deploy to test)
  3. I refreshed the current tab. All good!
  4. I signed out of the current tab. This sent a broadcast message to sign out all tabs.
  5. The old tab got that broadcast message and tried to send a backend request as part of signing out. The request failed because of a backend color change, so the app in that tab said, "Looks like I need to refresh--better let the other tabs know too!"
  6. The current tab got a "you need to refresh" message and acted accordingly.
  7. Repeat indefinitely.

This PR is effectively an imperfect patch. A stale client should avoid sending any backend request, otherwise we will broadcast to other tabs that they need to refresh. (Alternatively, we can choose not to broadcast, but that feels like the less-good, less-user-friendly, more convenient solution. Alternatively alternatively, upon getting a "need to refresh" broadcast message, a tab could ping for a backend health check.) I don't believe we have any other "background tab" requests being sent to the back end at this time.

This was something I should have caught as part of my work on 10349, but I was focused on idle logout at the expense of user-initiated logout.

@Mwindo Mwindo added the to test label Sep 18, 2024
@Mwindo Mwindo marked this pull request as ready for review September 18, 2024 17:10
@Mwindo Mwindo marked this pull request as draft September 18, 2024 17:27
@Mwindo Mwindo marked this pull request as ready for review September 18, 2024 17:28
@Mwindo Mwindo merged commit a38000c into ustaxcourt:test Sep 19, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants