From a2a63c8e804b983cb1f20f4c1d625deb5e2dd409 Mon Sep 17 00:00:00 2001 From: Maanil Verma Date: Sat, 8 Aug 2026 21:08:19 +0530 Subject: [PATCH 1/2] fix: warn instead of erroring on blocked or cancelled sign-in popups Google and GitHub sign-in use signInWithPopup. When the browser blocks the popup or the user closes it, Firebase throws auth/popup-blocked, auth/popup-closed-by-user or auth/cancelled-popup-request. These are user or browser actions, not app faults, so they now surface as a warn toast with copy that names the cause and the fix, rather than a red error. auth/popup-blocked and auth/account-exists-with-different-credential were missing from the locale entirely and fell through to the generic "something went wrong" message, which told the user nothing actionable. The account-collision copy does not name the original provider because the error does not carry it: AuthError.customData exposes only appName, email and phoneNumber, so naming it would need an extra fetchSignInMethodsForEmail round-trip. The regression net derives its table from the locale keys themselves instead of a hand-maintained list, so a code without its own message fails the suite. That hand-maintained list was the reason the two missing keys went unnoticed. --- src/composables/auth/useAuthActions.test.ts | 119 +++++++++++++++++--- src/composables/auth/useAuthActions.ts | 16 +++ src/locales/en/main.json | 6 +- 3 files changed, 122 insertions(+), 19 deletions(-) diff --git a/src/composables/auth/useAuthActions.test.ts b/src/composables/auth/useAuthActions.test.ts index b84c240ac49..af8d9435bf9 100644 --- a/src/composables/auth/useAuthActions.test.ts +++ b/src/composables/auth/useAuthActions.test.ts @@ -1,8 +1,10 @@ import { FirebaseError } from 'firebase/app' +import { AuthErrorCodes } from 'firebase/auth' import { createPinia, setActivePinia } from 'pinia' import { beforeEach, describe, expect, it, vi } from 'vitest' import { useAuthActions } from '@/composables/auth/useAuthActions' +import enLocale from '@/locales/en/main.json' import type { ComfyWorkflow } from '@/platform/workflow/management/stores/workflowStore' type ModifiedWorkflow = Pick @@ -36,18 +38,30 @@ const mockTrackAuthFailed = vi.hoisted(() => vi.fn()) const mockDistributionState = vi.hoisted(() => ({ isCloud: false })) const mockClearAllWorkflowStorage = vi.hoisted(() => vi.fn()) -const knownAuthErrorCodes = new Set([ - 'auth/invalid-credential', - 'auth/email-already-in-use', - 'auth/user-not-found' -]) +const authErrorMessages: Record = enLocale.auth.errors + +const firebaseCodesWithOwnMessage = Object.keys(authErrorMessages).filter( + (key) => key.startsWith('auth/') +) + +const popupPermissionCodes = [ + AuthErrorCodes.POPUP_CLOSED_BY_USER, + AuthErrorCodes.EXPIRED_POPUP_REQUEST, + AuthErrorCodes.POPUP_BLOCKED +] + +const accessErrorCodes = [ + 'auth/unauthorized-domain', + 'auth/invalid-dynamic-link-domain', + 'auth/unauthorized-continue-uri' +] vi.mock('@/i18n', () => ({ - t: (key: string, values?: { workflow?: string }) => - values?.workflow ? `${key}:${values.workflow}` : key, + t: (key: string, values?: Record) => + values ? `${key}:${Object.values(values).join(':')}` : key, st: (key: string, fallback: string) => { const code = key.replace('auth.errors.', '') - return knownAuthErrorCodes.has(code) ? key : fallback + return code in authErrorMessages ? key : fallback } })) @@ -366,17 +380,33 @@ describe('useAuthActions.reportError', () => { vi.clearAllMocks() }) - it('shows the friendly message for a known Firebase auth code', () => { - const { reportError } = useAuthActions() + it.for(firebaseCodesWithOwnMessage)( + 'maps %s to its own message rather than the generic fallback', + (code) => { + const { reportError } = useAuthActions() - reportError(new FirebaseError('auth/invalid-credential', 'raw firebase')) + reportError(new FirebaseError(code, 'raw firebase')) - expect(mockToastStore.add).toHaveBeenCalledWith({ - severity: 'error', - summary: 'g.error', - detail: 'auth.errors.auth/invalid-credential' - }) - expect(mockToastErrorHandler).not.toHaveBeenCalled() + expect(mockToastStore.add).toHaveBeenCalledWith( + expect.objectContaining({ detail: `auth.errors.${code}` }) + ) + expect(mockToastErrorHandler).not.toHaveBeenCalled() + } + ) + + it('gives every Firebase code a message distinct from the generic one', () => { + const generic = authErrorMessages['generic'] + const collisions = firebaseCodesWithOwnMessage.filter( + (code) => authErrorMessages[code] === generic + ) + + expect(collisions).toEqual([]) + }) + + it('covers every popup-permission code with its own message', () => { + expect(firebaseCodesWithOwnMessage).toEqual( + expect.arrayContaining(popupPermissionCodes) + ) }) it('shows the signupBlocked message when the error carries the signup_blocked token', () => { @@ -435,4 +465,59 @@ describe('useAuthActions.reportError', () => { expect(mockToastErrorHandler).toHaveBeenCalledWith(networkError) expect(mockToastStore.add).not.toHaveBeenCalled() }) + + it.for(popupPermissionCodes)( + 'warns rather than errors for %s, since the user or browser caused it', + (code) => { + const { reportError } = useAuthActions() + + reportError(new FirebaseError(code, 'raw firebase')) + + expect(mockToastStore.add).toHaveBeenCalledWith({ + severity: 'warn', + summary: 'g.warning', + detail: `auth.errors.${code}` + }) + expect(mockToastErrorHandler).not.toHaveBeenCalled() + } + ) + + it('reports an account collision as an error, not a popup warning', () => { + const { reportError, accessError } = useAuthActions() + + reportError( + new FirebaseError('auth/account-exists-with-different-credential', 'raw') + ) + + expect(mockToastStore.add).toHaveBeenCalledWith({ + severity: 'error', + summary: 'g.error', + detail: 'auth.errors.auth/account-exists-with-different-credential' + }) + expect(accessError.value).toBe(false) + }) + + it.for(accessErrorCodes)( + 'interpolates the domain and flips accessError for %s', + (code) => { + const { reportError, accessError } = useAuthActions() + + reportError(new FirebaseError(code, 'raw firebase')) + + expect(accessError.value).toBe(true) + expect(mockToastStore.add).toHaveBeenCalledWith({ + severity: 'error', + summary: 'g.error', + detail: `toastMessages.unauthorizedDomain:${window.location.hostname}:support@comfy.org` + }) + } + ) + + it('leaves accessError false for auth codes outside the domain group', () => { + const { reportError, accessError } = useAuthActions() + + reportError(new FirebaseError('auth/popup-blocked', 'raw firebase')) + + expect(accessError.value).toBe(false) + }) }) diff --git a/src/composables/auth/useAuthActions.ts b/src/composables/auth/useAuthActions.ts index 6eaf58a8414..ad8adb4edae 100644 --- a/src/composables/auth/useAuthActions.ts +++ b/src/composables/auth/useAuthActions.ts @@ -18,6 +18,13 @@ import { useAuthStore } from '@/stores/authStore' import type { BillingPortalTargetTier } from '@/stores/authStore' import { usdToMicros } from '@/utils/formatUtil' +/** Popup outcomes the user or their browser caused, not app faults. */ +const POPUP_PERMISSION_ERROR_CODES: readonly string[] = [ + AuthErrorCodes.POPUP_CLOSED_BY_USER, + AuthErrorCodes.EXPIRED_POPUP_REQUEST, + AuthErrorCodes.POPUP_BLOCKED +] + /** * Service for Firebase Auth actions. * All actions are wrapped with error handling. @@ -71,6 +78,15 @@ export const useAuthActions = () => { summary: t('g.error'), detail: t('auth.errors.signupBlocked') }) + } else if ( + error instanceof FirebaseError && + POPUP_PERMISSION_ERROR_CODES.includes(error.code) + ) { + toastStore.add({ + severity: 'warn', + summary: t('g.warning'), + detail: st(`auth.errors.${error.code}`, t('auth.errors.generic')) + }) } else if (error instanceof FirebaseError) { toastStore.add({ severity: 'error', diff --git a/src/locales/en/main.json b/src/locales/en/main.json index e13d2c81700..4f1b21838e3 100644 --- a/src/locales/en/main.json +++ b/src/locales/en/main.json @@ -2568,8 +2568,10 @@ "auth/operation-not-allowed": "This sign-in method is not currently supported.", "auth/invalid-credential": "Invalid login credentials. Please check your email and password.", "auth/network-request-failed": "Network error. Please check your connection and try again.", - "auth/popup-closed-by-user": "Sign-in was cancelled. Please try again.", - "auth/cancelled-popup-request": "Sign-in was cancelled. Please try again.", + "auth/popup-closed-by-user": "The sign-in window closed before sign-in finished. Please try again.", + "auth/cancelled-popup-request": "Another sign-in window was already open, so this one was cancelled. Please try again.", + "auth/popup-blocked": "Your browser blocked the sign-in window. Please allow pop-ups for this site and try again.", + "auth/account-exists-with-different-credential": "An account already exists with this email address but uses a different sign-in method. Please sign in the way you did originally.", "generic": "Something went wrong while signing you in. Please try again.", "signupBlocked": "We couldn't create your account right now. Please try again later. If this keeps happening, email support@comfy.org." }, From 773b4bf8a289220f13f662324a0e097308468399 Mon Sep 17 00:00:00 2001 From: Maanil Verma Date: Mon, 10 Aug 2026 19:37:50 +0530 Subject: [PATCH 2/2] chore: keep the locale diff to the auth error keys The unused-key pruner ran on an earlier commit and swept in unrelated changes: it added auth.login.signUpFreeTierPromo and dropped subscription.perYear / usdPerYear, none of which this PR is about. --- src/locales/en/main.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/locales/en/main.json b/src/locales/en/main.json index 4f1b21838e3..ec7e12c2b24 100644 --- a/src/locales/en/main.json +++ b/src/locales/en/main.json @@ -2490,7 +2490,6 @@ "cloudNewUser": "New to Comfy?", "cloudSignUp": "Sign up here", "freeRunsSuffix": "to get {count} free run. | to get {count} free runs.", - "signUpFreeTierPromo": "New here? {signUp} with Google to get {credits} free credits every month.", "emailLabel": "Email", "emailPlaceholder": "Enter your email", "passwordLabel": "Password", @@ -2726,8 +2725,10 @@ "comfyCloudLogo": "Comfy Cloud Logo", "beta": "BETA", "perMonth": "/ month", + "perYear": "/ year", "member": "member", "usdPerMonth": "USD / mo", + "usdPerYear": "USD / year", "usdPerMonthPerMember": "USD / mo / member", "creditSliderSave": "Save {percent}% ({amount})", "renewsDate": "Renews {date}",