From 74c53001cf4e652d96d2948d0b1058a827b98e24 Mon Sep 17 00:00:00 2001 From: ConnorYoh <40631091+ConnorYoh@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:05:24 +0100 Subject: [PATCH] fix(saas): give auth-bootstrap data fetching a single owner (#7194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The problem Three pieces of user data (pro status, avatar metadata, profile picture) were being fetched from four different places: `initializeAuth` on mount, the `SIGNED_IN` handler, the `TOKEN_REFRESHED` handler, and the post-upgrade path. On a fresh login the first two both see a session, so everything got fetched twice. It didn't stop after login either — Supabase re-fires `SIGNED_IN` on token refresh and tab-visibility wakeups, so a refresh that emitted both events cost around 7 Supabase reads. ## The fix All four call sites now go through one `loadUserData(session)` that is idempotent per identity. The guard key is `user.id` + `is_anonymous`: - not the access token, which changes on every refresh and would defeat the guard entirely - the anonymous flag matters because a guest to authenticated upgrade keeps the same user id, and that is the one case where the data genuinely does need reloading **Per login: 6 fetches to 3. A repeat `SIGNED_IN` or `TOKEN_REFRESHED` fetches nothing.** The tests count real calls rather than asserting on shape. ## Two behaviour changes worth naming - `initializeAuth` now awaits the full load, so the initial spinner also waits on the profile-picture URL. Net login is still faster, since an entire duplicate pass is gone. - A tab-wake `SIGNED_IN` no longer revalidates entitlements. That revalidation was accidental rather than designed — `refreshProStatus()` is the intended path, and post-checkout is already handled by `CheckoutContext`. ## Scope Supabase-origin traffic only. This does not touch the ~20 authenticated requests hitting `SupabaseAuthenticationFilter`, because those go to the Stirling backend rather than the hosted Supabase project. That is a separate problem and is unmeasured, so it needs measuring before anything is optimised. Remaining items (a double `/api/v1/team/my` fetch, an effect keyed on `[user]` identity in `FolderContext`, the `portalAccess` spinner flash, and caching the auth filter's per-request Postgres round-trips) are tracked separately. ## Verification ``` npx tsc --noEmit --project editor/src/saas/tsconfig.json # exit 0 npx eslint --max-warnings=0 editor/src/saas/auth # exit 0 npx prettier --check editor/src/saas/auth/ # clean npx vitest run --project saas # 75 passed (20 files) ``` --- .../src/saas/auth/AuthProvider.test.tsx | 339 ++++++++++++++++++ frontend/editor/src/saas/auth/UseSession.tsx | 165 +++++---- 2 files changed, 427 insertions(+), 77 deletions(-) create mode 100644 frontend/editor/src/saas/auth/AuthProvider.test.tsx diff --git a/frontend/editor/src/saas/auth/AuthProvider.test.tsx b/frontend/editor/src/saas/auth/AuthProvider.test.tsx new file mode 100644 index 0000000000..588507d59c --- /dev/null +++ b/frontend/editor/src/saas/auth/AuthProvider.test.tsx @@ -0,0 +1,339 @@ +import { act, render, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { Session, User } from "@supabase/supabase-js"; + +/** + * Request-count tests for {@link AuthProvider}'s data loading. It used to fetch + * pro status, avatar metadata and the picture from two places at once, and + * Supabase re-fires SIGNED_IN on token refresh and tab wakeups, so it kept + * happening. These pin the call counts. + */ + +type AuthCallback = (event: string, session: Session | null) => void; + +const rpc = vi.fn(); +const createSignedUrl = vi.fn(); +const storageFrom = vi.fn((_bucket: string) => ({ createSignedUrl })); +const getSession = vi.fn(); +const onAuthStateChange = vi.fn(); +const unsubscribe = vi.fn(); + +vi.mock("@app/auth/supabase", () => ({ + supabase: { + auth: { + getSession: () => getSession(), + onAuthStateChange: (cb: AuthCallback) => onAuthStateChange(cb), + refreshSession: vi + .fn() + .mockResolvedValue({ data: { session: null }, error: null }), + signOut: vi.fn().mockResolvedValue({ error: null }), + }, + rpc: (...args: unknown[]) => rpc(...args), + storage: { from: (bucket: string) => storageFrom(bucket) }, + }, + debugAuthEvents: vi.fn(), +})); + +const syncOAuthAvatar = vi.fn(); +const getProfilePictureMetadata = vi.fn(); + +vi.mock("@app/services/avatarSyncService", () => ({ + syncOAuthAvatar: (...args: unknown[]) => syncOAuthAvatar(...args), + getProfilePictureMetadata: (...args: unknown[]) => + getProfilePictureMetadata(...args), + getProviderAvatarUrl: () => null, +})); + +const synchronizeUserUpgrade = vi.fn(); + +vi.mock("@app/services/userService", () => ({ + synchronizeUserUpgrade: (...args: unknown[]) => + synchronizeUserUpgrade(...args), +})); + +// Imported after the mocks so the provider picks them up. +const { AuthProvider, useAuth } = await import("./UseSession"); + +/** Surfaces `loading` so a test can assert on it rather than on the container. */ +function LoadingProbe() { + const { loading } = useAuth(); + return {String(loading)}; +} + +const USER_ID = "11111111-2222-3333-4444-555555555555"; + +function makeSession( + overrides: { token?: string; userId?: string; anonymous?: boolean } = {}, +): Session { + const user = { + id: overrides.userId ?? USER_ID, + email: "someone@example.com", + is_anonymous: overrides.anonymous ?? false, + app_metadata: { provider: "google" }, + user_metadata: { full_name: "Some One" }, + } as unknown as User; + + return { + access_token: overrides.token ?? "token-1", + refresh_token: "refresh-1", + expires_in: 3600, + token_type: "bearer", + user, + } as unknown as Session; +} + +/** Total requests the provider makes per user-data load. */ +function callCounts() { + return { + proStatus: rpc.mock.calls.length, + metadata: getProfilePictureMetadata.mock.calls.length, + picture: createSignedUrl.mock.calls.length, + avatarSync: syncOAuthAvatar.mock.calls.length, + }; +} + +function renderProvider() { + let authCallback: AuthCallback = () => {}; + onAuthStateChange.mockImplementation((cb: AuthCallback) => { + authCallback = cb; + return { data: { subscription: { unsubscribe } } }; + }); + + const utils = render( + + + , + ); + + /** + * Deliver an auth event and let its work finish. The provider defers with + * setTimeout(0), so a microtask flush is not enough: without draining real + * macrotasks the assertions run before any refetch and prove nothing. + */ + const fire = async (event: string, s: Session | null) => { + await act(async () => { + authCallback(event, s); + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + return { ...utils, fire }; +} + +describe("AuthProvider user-data loading", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.useRealTimers(); + sessionStorage.clear(); + + rpc.mockResolvedValue({ data: true, error: null }); + createSignedUrl.mockResolvedValue({ + data: { signedUrl: "https://example.test/avatar" }, + error: null, + }); + getProfilePictureMetadata.mockResolvedValue(null); + syncOAuthAvatar.mockResolvedValue(false); + synchronizeUserUpgrade.mockResolvedValue(undefined); + getSession.mockResolvedValue({ + data: { session: makeSession() }, + error: null, + }); + }); + + it("fetches each piece of user data exactly once on login", async () => { + const { fire } = renderProvider(); + + await waitFor(() => expect(createSignedUrl).toHaveBeenCalled()); + // The SIGNED_IN that follows a fresh login must not repeat the work. + await fire("SIGNED_IN", makeSession()); + + expect(callCounts()).toEqual({ + proStatus: 1, + metadata: 1, + picture: 1, + avatarSync: 1, + }); + }); + + it("does not refetch when SIGNED_IN repeats with a new access token", async () => { + const { fire } = renderProvider(); + await waitFor(() => expect(createSignedUrl).toHaveBeenCalled()); + const before = callCounts(); + + // What a tab-visibility wakeup or token refresh looks like: same user, + // different token. + await fire("SIGNED_IN", makeSession({ token: "token-2" })); + + expect(callCounts()).toEqual(before); + }); + + it("does not refetch on TOKEN_REFRESHED for the same identity", async () => { + const { fire } = renderProvider(); + await waitFor(() => expect(createSignedUrl).toHaveBeenCalled()); + const before = callCounts(); + + await fire("TOKEN_REFRESHED", makeSession({ token: "token-3" })); + + expect(callCounts()).toEqual(before); + }); + + it("keeps loading false across repeat auth events", async () => { + // Guards the Landing -> HomePage unmount: toggling `loading` on a wakeup + // would tear down the tree on every tab switch. + const { fire, getByTestId } = renderProvider(); + await waitFor(() => + expect(getByTestId("loading").textContent).toBe("false"), + ); + + await fire("SIGNED_IN", makeSession({ token: "token-4" })); + expect(getByTestId("loading").textContent).toBe("false"); + + await fire("TOKEN_REFRESHED", makeSession({ token: "token-5" })); + expect(getByTestId("loading").textContent).toBe("false"); + + expect(callCounts().proStatus).toBe(1); + }); + + it("clears the initial spinner without waiting for the avatar upload", async () => { + // syncOAuthAvatar re-uploads the provider image on a first login. Gating + // `loading` on it would stall account creation behind an image upload. + let releaseSync = () => {}; + syncOAuthAvatar.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseSync = () => resolve(false); + }), + ); + + const { getByTestId } = renderProvider(); + + await waitFor(() => + expect(getByTestId("loading").textContent).toBe("false"), + ); + // The picture read chains behind the sync, so it has not run yet either. + expect(createSignedUrl).not.toHaveBeenCalled(); + + await act(async () => { + releaseSync(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }); + + it("refetches after a guest upgrade, which keeps the same user id", async () => { + // The upgrade path is the one case where the id is unchanged but the data + // must be reloaded - hence keying on is_anonymous, not the id alone. + getSession.mockResolvedValue({ + data: { session: makeSession({ anonymous: true }) }, + error: null, + }); + sessionStorage.setItem("pendingUpgrade", "true"); + sessionStorage.setItem("upgradeProvider", "google"); + + const { fire } = renderProvider(); + await waitFor(() => expect(rpc).toHaveBeenCalledTimes(1)); + + await fire("USER_UPDATED", makeSession({ anonymous: false })); + + await waitFor(() => expect(rpc).toHaveBeenCalledTimes(2)); + expect(synchronizeUserUpgrade).toHaveBeenCalledWith("google"); + }); + + it("does not drop an upgrade that lands while the guest load is in flight", async () => { + // Coalescing on "something is in flight" alone would hand the upgrade the + // guest's promise and never fetch the real user's data. + getSession.mockResolvedValue({ + data: { session: makeSession({ anonymous: true }) }, + error: null, + }); + sessionStorage.setItem("pendingUpgrade", "true"); + sessionStorage.setItem("upgradeProvider", "google"); + + let releaseGuestLoad = () => {}; + const guestLoadBlocked = new Promise((resolve) => { + releaseGuestLoad = resolve; + }); + rpc.mockImplementationOnce(async () => { + await guestLoadBlocked; + return { data: false, error: null }; + }); + + const { fire } = renderProvider(); + await waitFor(() => expect(rpc).toHaveBeenCalledTimes(1)); + + // Deliver the upgrade with the guest load still deliberately unsettled, so + // the guard genuinely has an in-flight load to reason about. + await fire("USER_UPDATED", makeSession({ anonymous: false })); + await waitFor(() => expect(rpc).toHaveBeenCalledTimes(2), { + timeout: 1000, + }); + + // Let the abandoned guest load settle inside act, so its trailing state + // updates do not land after the test finishes. + await act(async () => { + releaseGuestLoad(); + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }); + + it("reloads for the same user after a sign-out", async () => { + const { fire } = renderProvider(); + await waitFor(() => expect(createSignedUrl).toHaveBeenCalled()); + + await fire("SIGNED_OUT", null); + await fire("SIGNED_IN", makeSession()); + + await waitFor(() => expect(rpc).toHaveBeenCalledTimes(2)); + }); + + it("keeps the initial spinner up when SIGNED_IN wins the race with initializeAuth", async () => { + // initializeAuth yields at `await getSession()`, so SIGNED_IN can land + // first. Marking the identity loaded up front would then clear the spinner + // while pro status was still in flight. + const session = makeSession(); + + let releaseSession = () => {}; + getSession.mockReturnValueOnce( + new Promise<{ data: { session: Session }; error: null }>((resolve) => { + releaseSession = () => resolve({ data: { session }, error: null }); + }), + ); + + let releaseProStatus = () => {}; + rpc.mockImplementationOnce( + () => + new Promise<{ data: boolean; error: null }>((resolve) => { + releaseProStatus = () => resolve({ data: true, error: null }); + }), + ); + + const { getByTestId, fire } = renderProvider(); + + // SIGNED_IN lands first and starts the load; pro status stays unsettled. + await fire("SIGNED_IN", session); + expect(rpc).toHaveBeenCalledTimes(1); + + // initializeAuth must now adopt that in-flight load, not short-circuit. + await act(async () => { + releaseSession(); + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(getByTestId("loading").textContent).toBe("true"); + // Adopted, not restarted. + expect(rpc).toHaveBeenCalledTimes(1); + + await act(async () => { + releaseProStatus(); + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await waitFor(() => + expect(getByTestId("loading").textContent).toBe("false"), + ); + expect(rpc).toHaveBeenCalledTimes(1); + }); +}); diff --git a/frontend/editor/src/saas/auth/UseSession.tsx b/frontend/editor/src/saas/auth/UseSession.tsx index 087fa99b65..101ed58d06 100644 --- a/frontend/editor/src/saas/auth/UseSession.tsx +++ b/frontend/editor/src/saas/auth/UseSession.tsx @@ -2,6 +2,7 @@ import { createContext, useContext, useEffect, + useRef, useState, ReactNode, useCallback, @@ -247,6 +248,78 @@ export function AuthProvider({ children }: { children: ReactNode }) { await fetchProfilePictureMetadata(); }, [fetchProfilePictureMetadata]); + // Refs, not state: the auth effect below has an empty dep array. + const loadedForRef = useRef(null); + const inFlightRef = useRef<{ key: string; promise: Promise } | null>( + null, + ); + + /** + * Sole owner of the per-user data fetching: mount-time init and every auth + * event route through here. Idempotent per identity, since Supabase re-fires + * SIGNED_IN and TOKEN_REFRESHED on token refresh and tab wakeups; pass + * `force` for a genuine reload. The returned promise excludes the profile + * picture, so awaiting it never blocks on an image download. + */ + const loadUserData = useCallback( + ( + sessionToLoad: Session | null, + opts?: { force?: boolean }, + ): Promise => { + const user = sessionToLoad?.user; + if (!user) { + loadedForRef.current = null; + return Promise.resolve(); + } + + // Not the access token, which changes every refresh. The anonymous flag + // matters: a guest upgrade keeps the same id and must still refetch. + const key = `${user.id}:${Boolean(user.is_anonymous)}`; + if (!opts?.force && loadedForRef.current === key) + return Promise.resolve(); + + // The second of a concurrent init/SIGNED_IN pair adopts this promise so it + // still awaits the load. A forced reload must not: the guest upgrade + // would be silently dropped. + if (!opts?.force && inFlightRef.current?.key === key) + return inFlightRef.current.promise; + + const run = (async () => { + // Off the awaited path: a first login re-uploads the provider avatar. + // The signed-URL read chains behind it because reading first 404s and + // silently falls back to the provider photo. + const avatarSync = syncOAuthAvatar(user).catch((err) => { + console.debug("[Auth Debug] Failed to sync OAuth avatar:", err); + return false; + }); + void avatarSync + .then(() => fetchProfilePicture(sessionToLoad)) + .catch((err) => { + console.debug("[Auth Debug] Failed to fetch profile picture:", err); + }); + + await Promise.all([ + fetchProStatus(sessionToLoad), + fetchProfilePictureMetadata(sessionToLoad), + ]); + // Only on success: set up front, a concurrent caller short-circuits on + // it and returns to a still-empty state. Also lets a failure retry. + loadedForRef.current = key; + })() + .catch((err) => { + console.debug("[Auth Debug] Failed to load user data:", err); + }) + .finally(() => { + // Only clear our own entry; a newer load may have superseded us. + if (inFlightRef.current?.promise === run) inFlightRef.current = null; + }); + + inFlightRef.current = { key, promise: run }; + return run; + }, + [fetchProStatus, fetchProfilePictureMetadata, fetchProfilePicture], + ); + const refreshSession = async () => { try { setLoading(true); @@ -312,23 +385,8 @@ export function AuthProvider({ children }: { children: ReactNode }) { }); setSession(data.session); - // Fetch pro status, profile picture metadata, and profile picture using the session from the response - if (data.session?.user) { - // Sync OAuth avatar in background; fetch the picture once the - // sync settles instead of guessing with a fixed delay. - syncOAuthAvatar(data.session.user) - .catch((err) => { - console.debug( - "[Auth Debug] Failed to sync OAuth avatar on init:", - err, - ); - return false; - }) - .then(() => fetchProfilePicture(data.session)); - - await fetchProStatus(data.session); - await fetchProfilePictureMetadata(data.session); - } + // Awaited so the spinner does not clear before pro status is known. + await loadUserData(data.session); } } catch (err) { console.error( @@ -374,58 +432,13 @@ export function AuthProvider({ children }: { children: ReactNode }) { setIsPro(null); setProfilePictureUrl(null); setProfilePictureMetadata(null); - } else if (event === "SIGNED_IN") { - console.debug("[Auth Debug] User signed in successfully"); - if (newSession?.user) { - // Note: we deliberately do NOT toggle `loading` here. Supabase - // also fires SIGNED_IN on tab visibility / token-refresh wakeups - // (per its docs: "SIGNED_IN is fired when a user signs in OR - // when the access token is refreshed"), and gating the UI on - // `loading` would unmount Landing -> HomePage every time the - // user switches tabs back. Initial-mount loading is handled by - // `initializeAuth` above; downstream fetches expose their own - // null/loading states. - - // Sync OAuth avatar in background (don't block other fetches) - const avatarSync = syncOAuthAvatar(newSession.user).catch( - (err) => { - console.debug( - "[Auth Debug] Failed to sync OAuth avatar:", - err, - ); - return false; - }, - ); - - // Fetch user data in parallel - Promise.all([ - fetchProStatus(newSession), - fetchProfilePictureMetadata(newSession), - ]).then(() => { - // Fetch the picture once the avatar sync settles. - avatarSync.then(() => { - fetchProfilePicture(newSession).finally(() => { - console.debug( - "[Auth Debug] User data fully loaded after sign in", - ); - }); - }); - }); - } - } else if (event === "TOKEN_REFRESHED") { - console.debug("[Auth Debug] Token refreshed"); - // Optionally refresh pro status, profile picture metadata, and profile picture on token refresh - if (newSession?.user) { - Promise.all([ - fetchProStatus(newSession), - fetchProfilePictureMetadata(newSession), - fetchProfilePicture(newSession), - ]).then(() => { - console.debug( - "[Auth Debug] User data refreshed after token refresh", - ); - }); - } + loadedForRef.current = null; + } else if (event === "SIGNED_IN" || event === "TOKEN_REFRESHED") { + console.debug("[Auth Debug] Signed in or token refreshed"); + // Deliberately does not touch `loading`: Supabase also fires + // SIGNED_IN on tab wakeups, and gating the UI on it would unmount + // Landing -> HomePage on every tab switch. Pinned by a test. + void loadUserData(newSession); } else if (event === "USER_UPDATED") { console.debug("[Auth Debug] User updated"); @@ -454,14 +467,8 @@ export function AuthProvider({ children }: { children: ReactNode }) { "[Auth Debug] User upgrade synchronized successfully", ); - // Refresh pro status, profile picture metadata, and profile picture after upgrade - if (newSession?.user) { - return Promise.all([ - fetchProStatus(newSession), - fetchProfilePictureMetadata(newSession), - fetchProfilePicture(newSession), - ]); - } + // Forced: same user id, so the guest's data must be replaced. + return loadUserData(newSession, { force: true }); }) .then(() => { console.debug( @@ -484,6 +491,10 @@ export function AuthProvider({ children }: { children: ReactNode }) { mounted = false; subscription.unsubscribe(); }; + // Empty and load-bearing: must subscribe once. The closures are recreated + // when `session` changes, so listing deps would re-subscribe on every auth + // event; every call above passes its session explicitly instead. No lint + // rule enforces this, so do not "fix" these deps. }, []); const { t } = useTranslation();