diff --git a/apps/studio/data/profile/profile-create-mutation.ts b/apps/studio/data/profile/profile-create-mutation.ts index 25917ca50b3..f8317053092 100644 --- a/apps/studio/data/profile/profile-create-mutation.ts +++ b/apps/studio/data/profile/profile-create-mutation.ts @@ -28,6 +28,17 @@ export const useProfileCreateMutation = ({ return useMutation({ mutationFn: () => createProfile(), + // Retry transient failures (5xx, network, 429), but never 4xx - a 409 means the profile + // already exists, so retrying the POST can't help + retry: (failureCount, error) => { + const isClientError = error.code !== undefined && error.code >= 400 && error.code < 500 + if (isClientError && error.code !== 429) return false + return failureCount < 3 + }, + retryDelay: (failureCount, error) => { + if (error.code === 429 && error.retryAfter) return error.retryAfter * 1000 + return Math.min(1000 * 2 ** failureCount, 8000) + }, async onSuccess(data, variables, context) { await Promise.all([ queryClient.invalidateQueries({ queryKey: profileKeys.profile() }), diff --git a/apps/studio/lib/profile.test.tsx b/apps/studio/lib/profile.test.tsx new file mode 100644 index 00000000000..0caa2501c94 --- /dev/null +++ b/apps/studio/lib/profile.test.tsx @@ -0,0 +1,142 @@ +import { screen, waitFor } from '@testing-library/react' +import { HttpResponse } from 'msw' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { ProfileProvider, useProfile } from './profile' +import type { ProfileResponse } from '@/data/profile/profile-create-mutation' +import { customRender } from '@/tests/lib/custom-render' +import { addAPIMock, type APIErrorBody } from '@/tests/lib/msw' +import { createMockProfile } from '@/tests/lib/profile-helpers' + +vi.mock('common', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useParams: () => ({ ref: 'default' }), + useUser: () => ({ id: 'user-1', email: 'test@example.com' }), + useIsLoggedIn: () => true, + } +}) + +vi.mock('@sentry/nextjs', () => ({ captureMessage: vi.fn(), captureException: vi.fn() })) +vi.mock('@/lib/telemetry/track', () => ({ useTrack: () => vi.fn() })) + +const profileResponse = (username?: string) => + HttpResponse.json(createMockProfile({ username })) +const errorResponse = (message: string, status: number) => + HttpResponse.json({ message }, { status }) + +const ProfileStatus = () => { + const { profile, isSuccess } = useProfile() + return
{isSuccess ? `profile:${profile?.username}` : 'no-profile'}
+} + +const renderProvider = () => + customRender( + + + + ) + +// Serves each GET response in order, repeating the last one +const mockProfileGet = (responses: Array<'not-found' | 'found'>) => { + const getSpy = vi.fn() + addAPIMock({ + method: 'get', + path: '/platform/profile', + response: () => { + const next = responses[Math.min(getSpy.mock.calls.length, responses.length - 1)] + getSpy() + return next === 'found' + ? profileResponse('new-user') + : errorResponse("User's profile not found", 404) + }, + }) + return getSpy +} + +describe('ProfileProvider profile creation', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('creates the profile once when it does not exist', async () => { + const getSpy = mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return profileResponse() + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument()) + expect(postSpy).toHaveBeenCalledTimes(1) + expect(getSpy).toHaveBeenCalledTimes(2) + }) + + it('refetches the profile when creation returns 409', async () => { + mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return errorResponse('Profile already exists', 409) + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument()) + expect(postSpy).toHaveBeenCalledTimes(1) + }) + + it('does not loop POSTing when the profile is still not found after a 409', async () => { + const getSpy = mockProfileGet(['not-found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return errorResponse('Profile already exists', 409) + }, + }) + + renderProvider() + + // Initial GET, then the refetch triggered by the 409 + await waitFor(() => expect(getSpy).toHaveBeenCalledTimes(2)) + await new Promise((resolve) => setTimeout(resolve, 300)) + expect(postSpy).toHaveBeenCalledTimes(1) + expect(getSpy).toHaveBeenCalledTimes(2) + }) + + it('retries creation on a server error', async () => { + mockProfileGet(['not-found', 'found']) + const postSpy = vi.fn() + addAPIMock({ + method: 'post', + path: '/platform/profile', + response: () => { + postSpy() + return postSpy.mock.calls.length === 1 + ? errorResponse('Internal error', 500) + : profileResponse() + }, + }) + + renderProvider() + + await waitFor(() => expect(screen.getByText('profile:new-user')).toBeInTheDocument(), { + timeout: 5000, + }) + expect(postSpy).toHaveBeenCalledTimes(2) + }) +}) diff --git a/apps/studio/lib/profile.tsx b/apps/studio/lib/profile.tsx index b4f14943b4b..9097ddbcd61 100644 --- a/apps/studio/lib/profile.tsx +++ b/apps/studio/lib/profile.tsx @@ -1,4 +1,5 @@ import * as Sentry from '@sentry/nextjs' +import { useQueryClient } from '@tanstack/react-query' import { useIsLoggedIn, useUser } from 'common' import { useRouter } from 'next/router' import { createContext, PropsWithChildren, useContext, useEffect, useMemo } from 'react' @@ -6,7 +7,10 @@ import { toast } from 'sonner' import { useSignOut } from './auth' import { getGitHubProfileImgUrl } from './github' +import { organizationKeys } from '@/data/organizations/keys' +import { permissionKeys } from '@/data/permissions/keys' import { usePermissionsQuery } from '@/data/permissions/permissions-query' +import { profileKeys } from '@/data/profile/keys' import { useProfileCreateMutation } from '@/data/profile/profile-create-mutation' import { useProfileIdentitiesQuery } from '@/data/profile/profile-identities-query' import { useProfileQuery } from '@/data/profile/profile-query' @@ -32,6 +36,7 @@ export const ProfileContext = createContext({ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { const user = useUser() + const queryClient = useQueryClient() const isLoggedIn = useIsLoggedIn() const router = useRouter() const signOut = useSignOut() @@ -53,14 +58,13 @@ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { }, onError: (error) => { if (error.code === 409) { - // [Joshen] There's currently an assumption that createProfile is getting triggered - // multiple times unnecessarily, although the tracing the code i can't see why this might - // be happening unless GET profile is somehow returning `User's profile not found` incorrectly - // Adding a Sentry capture + toast in hopes to catch this while developing on local / staging + // The profile already exists (e.g. GET hit a lagging replica, or it was created + // elsewhere), so the earlier "not found" is stale. Refetch rather than leaving the + // user stuck on the cached error. Sentry capture is kept to monitor how often this occurs. Sentry.captureMessage('Profile already exists: ' + error.message) - if (process.env.NEXT_PUBLIC_ENVIRONMENT !== 'prod') { - toast.error('[DEV] createProfile called despite profile already exists: ' + error.message) - } + queryClient.invalidateQueries({ queryKey: profileKeys.profile() }) + queryClient.invalidateQueries({ queryKey: organizationKeys.list() }) + queryClient.invalidateQueries({ queryKey: permissionKeys.list() }) } else { Sentry.captureMessage('Failed to create users profile: ' + error.message) toast.error('Failed to create your profile. Please refresh to try again.') @@ -79,21 +83,21 @@ export const ProfileProvider = ({ children }: PropsWithChildren<{}>) => { enabled: isLoggedIn, }) + const isProfileNotFound = isError && error?.message === "User's profile not found" useEffect(() => { - if (!isError) return - // if the user does not yet exist, create a profile for them - if (error?.message === "User's profile not found") { - createProfile() - } + if (!isProfileNotFound || !user?.id) return + createProfile() + }, [isProfileNotFound, user?.id, createProfile]) - // [Alaister] If the user has a bad auth token, auth-js won't know about it - // and will think the user is authenticated. Since fetching the profile happens - // on every page load, we can check for a 401 here and sign the user out if - // they have a bad token. - if (error?.code === 401) { - signOut().then(() => router.push('/sign-in')) - } - }, [error, signOut, router, createProfile, isError]) + // [Alaister] If the user has a bad auth token, auth-js won't know about it + // and will think the user is authenticated. Since fetching the profile happens + // on every page load, we can check for a 401 here and sign the user out if + // they have a bad token. + const isUnauthorized = isError && error?.code === 401 + useEffect(() => { + if (!isUnauthorized) return + signOut().then(() => router.push('/sign-in')) + }, [isUnauthorized, signOut, router]) const { isInitialLoading: isLoadingPermissions } = usePermissionsQuery({ enabled: isLoggedIn })