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

onAuthStateChange fires every time we focus the tab #579

Open
altryne opened this issue Dec 21, 2022 · 13 comments · May be fixed by #684
Open

onAuthStateChange fires every time we focus the tab #579

altryne opened this issue Dec 21, 2022 · 13 comments · May be fixed by #684
Assignees
Labels
bug Something isn't working

Comments

@altryne
Copy link

altryne commented Dec 21, 2022

Bug report

Describe the bug

onAuthStateChange fires every time we blur (go to a different tab) and focus on the tab with supabase

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Go to https://n3-supabase.netlify.app/
  2. Log in
  3. Open devtools network tab
  4. blur (go to a different tab)
  5. focus (go back to the tab with an active supabase.js auth session)
  6. network shows a new request to /api/_supabase/session on every focus event
  7. no state was changed, but onAuthStateChange was fired which triggered the request in nuxt

Expected behavior

onAuthStateChange fires only when the state actually changes

Screenshots

CleanShot 2022-12-21 at 13 56 15@2x

System information

  • OS: macOS
  • Version of nuxt-supabase - "@nuxtjs/supabase": "^0.3.0",

Additional context

This doesn't seem to be a nuxt problem as it only subscribes once to the onAuthStateChange

@altryne altryne added the bug Something isn't working label Dec 21, 2022
@BghinC
Copy link

BghinC commented Jan 9, 2023

I have the same issue using Nuxt Supabase. The problem is still present when switching from Nuxt Supabase to @supabase/supabase-js

@nohehf
Copy link

nohehf commented Jan 9, 2023

My bad, mentioned the wrong issue, ignore the above

@soedirgo soedirgo transferred this issue from supabase/supabase-js Jan 10, 2023
@hf
Copy link
Contributor

hf commented Mar 1, 2023

@altryne Could you please share the code in your app that registers the onAuthStateChange callback?

@altryne
Copy link
Author

altryne commented Mar 1, 2023

Hey @hf (Stojan)
My code implements onAuthStateChange via nuxt-supabase:
https://supabase.nuxtjs.org/usage/composables/use-supabase-auth-client

The above example replicates my experiene, every time I switch to the tab and back from it, I get a session request sent in network tab

@altryne
Copy link
Author

altryne commented Mar 18, 2023

Hey, would it be possible to take a look at why this is happening? This could add an un-nessesary load on supabase if it's a bug, which seems like it is.

I think this is racking up a bunch of un-nessesary calls and is messing up with my code

@kjmph
Copy link

kjmph commented Apr 4, 2023

Hello @altryne, this is happening because of the visibilitychange window event. I suppose inside the SessionContextProvider the useEffect for onAuthStateChange could check that the callback's session variable's access_token is different than the useState session variable's access_token, and not call setSession when it is unchanged (oh, and session.access_token would have to be a dependency of this useEffect). That would at least prevent renders from occurring due to SessionContextProvider rewriting itself every time the window receives focus (which is how I noticed this bug). (Also, pardon, shouldn't this callback capture USER_UPDATED events and also call setSession with a new session object? Probably another bug.)

See here:
https://github.com/supabase/auth-helpers/blob/c4ffd00acbbf928285b270979b0a2bae5d411b46/packages/react/src/components/SessionContext.tsx#L87

Note though, as any other downstream onAuthStateChange consumer would have to know about this oddity (your reported bug), I believe the true fix should be inside gotrue-js. When the current session has not expired, and the access_token is unchanged, the _notifyAllSubscribers should not trigger with SIGNED_IN.

As the original patch was targeting React Native, I believe this extra notification is to get the access_token back resident in memory after the application regains focus. So, I would submit a pull request, yet I can't test React Native.

@hf hf self-assigned this May 17, 2023
@hf
Copy link
Contributor

hf commented May 17, 2023

Hey I'm thinking of transferring this issue to nuxt-modules/supabase because this feels like a two-part issue to me:

  1. Why is there a network request? Feels unnecessary when some cookies can just be set.
  2. Why is it called each time there's a focus? I'll investigate.

@hf
Copy link
Contributor

hf commented May 17, 2023

Can't transfer there, so a duplicate is open there. Investigating the event issue now.

@altryne
Copy link
Author

altryne commented May 17, 2023

Thank you @hf this seems to me that it does way way too many auth calls to Supa and I could potentially reduce a bunch of load!

@probablykasper
Copy link

I'm seeing this bug with SvelteKit, so it's not specific to Nuxt

@hf
Copy link
Contributor

hf commented May 18, 2023

From what I could find so far, SIGNED_IN has been emitted always when the tab becomes visible again in v2, so it may be a breaking change to remove at this point.

@hf hf linked a pull request May 18, 2023 that will close this issue
@hf
Copy link
Contributor

hf commented May 18, 2023

I think this fix should address the issue, though I'd like to think more about backward compatibility.

@robinjhuang
Copy link

It would be fantastic to see this PR merged soon. I'm encountering the same issue in a React application. In React, it's a common practice to store the session object in the component state. Because of that, each time a SIGNED_IN event fires and updates the session, it triggers a re-render of the entire application, impacting performance.

As a workaround, I'm currently comparing the token field of the incoming and existing session objects before updating the state. While this approach mitigates the issue, it feels more like a patch than a proper solution. Addressing this behavior directly within the Supabase library would be a much cleaner and more efficient way to handle it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants