Skip to content

Handle stale auth better #2342

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Jul 8, 2025

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/7736

Description (What does it do?)

This PR makes a few frontend changes to improve handling of users whose credentials have expired. Primarily the changes are around ensuring that users/me api data is not stale.

  1. The users/me query (upon which auth is based) now refetches every time you re-focus on Learn's browser tab.
  2. The ForbiddenPage and RestrictedRoute now refetches fresh user/me data before showing you the "Forbidden" error; if your fresh data

How can this be tested?

Test refetchOnWindowFocus:

  1. Open two browser tabs (same window):
    • both logged out (anonymous user)
    • both on homepage (or any unrestricted route)
    • If you want, open dev console network inspector for both tabs.
  2. Log into one tab ("A"). After you've logged in, switch to other tab ("B").
    • Expectation: Tab "B" is also logged in.
  3. If you want, try the above in reverse (logging out).
  4. Repeat this on a restricted route.
    • Expectation: Tab "B" is redirected to login.

Test fresh auth checking:
The other changes in this PR aim to better handle cases where auth is stale due to expiring session. I'm not sure the best way to test this locally. What I've done is use two tabs, log out in one, and navigate in the other. But refetchOnWindowFocus prevents that situation from actually resulting in stale auth.

So my suggestion is:

  1. comment out the refetchOnWindowFocus line in users/queries.ts
  2. Open two tabs again, as an authenticated user in both.
  3. Log out in tab "A"
  4. Now in tab "B", try to access the dashboard. You should be redirected to login.

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/handle expiring auth Handle stale auth better Jul 9, 2025
@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Jul 9, 2025
Copy link
Contributor

@jonkafton jonkafton left a comment

Choose a reason for hiding this comment

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

This works great for tabs in the same window, though can you help me understand why it does not work across windows and where the difference is.

I'd expect that irrespective of different tab or window; the window receives focus, React Query determines that the query is set to refetchOnWindowFocus: "always" or that the cache is stale and then goes ahead to refetch.

Testing further, it does work across windows, but very unreliably. I wondered if it was the browser being conservative issuing focus events in relation to some focus stealing prevention rules, though I reliably get focus and visibilitychange events. I'm none the wiser.

Generally very nice changes, especially around the login href/redirect hooks.

@@ -19,7 +19,6 @@ const makeQueryClient = (): QueryClient => {
return new QueryClient({
defaultOptions: {
queries: {
refetchOnWindowFocus: false,
staleTime: Infinity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set some reasonable staleTime? Defaulting refetchOnWindowFocus to true here has no effect if the data is never stale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meant to comment on this... I removed the false since it has no effect with staleTime set to Infinity. (refetchOnWindowFocus: false | true ... neither has an effect with infinite staletime, just "always").,

@ChristopherChudzicki
Copy link
Contributor Author

This works great for tabs in the same window, though can you help me understand why it does not work across windows and where the difference is.

I'd expect that irrespective of different tab or window; the window receives focus, React Query determines that the query is set to refetchOnWindowFocus: "always" or that the cache is stale and then goes ahead to refetch.

Testing further, it does work across windows, but very unreliably. I wondered if it was the browser being conservative issuing focus events in relation to some focus stealing prevention rules, though I reliably get focus and visibilitychange events. I'm none the wiser.

I'm curious if you ever found it to work across windows. Having read a bit more on this, I don't think it should...

Despite its name, the default implementation of refetchOnWindowFocus uses visibilitychange event (in earlier versions of react-query, I guess it used both visibilitychange and focus).

MDN says:

This event fires with a visibilityState of hidden when a user navigates to a new page, switches tabs, closes the tab, minimizes or closes the browser, or, on mobile, switches from the browser to a different app.

(Emphasis mine, since I misread it at first. Only on mobile does switching apps fire the event.)


React query does make it easy to change the implementation to use focus. We could do that. I guess they droppedfocus because of some bugs.

I can see how devtool refocus fetching would be very annoying for developers 😁


The issues with focus event seem mostly to be about too much refetching, which doesn't really seem like a problem for one endpoint (users/me). OTOH, refetchOnWindowFocus detection mechanism is a global setting. And it's nice to keep the defaults...

I'm sort of inclined to add a reasonable staletime, like 10 or 15 minutes, to the user query (or maybe all of them?) and leave the default implementation (visibilitychange).

That wouldn't help the situation of logging out in one window and having incorrect state in the other, but it would at least guarantee fresh info if you left the tab open for 2 days while using some other app or window, then coming back to Learn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants