diff --git a/AGENTS.md b/AGENTS.md index 93f53cf6c..b7e224611 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -707,3 +707,77 @@ only the base `--spacing: 0.25rem`, so a `var(--spacing-32)` "fix" resolves to n the `calc()` (the container height drops to `auto`). Multiple code-review models flag the function form as invalid CSS and suggest the `var()` form; don't take the bait. Verify by measuring, not by reading: the container computes to exactly `innerHeight − 128px` when the function resolved. + +## Dropping `errorHandler` from an `onError` also drops the RUM report + +The Datadog RUM SDK instruments `console.error` and reports each call as an error with +`error.source: "console"` — no `datadogLogs` init involved, and we have none. `errorHandler` +([`src/react-query/queryClient.ts`](src/react-query/queryClient.ts)) opens with `console.error(rawErr)`, +so a handled query/mutation rejection reaches RUM today purely as a side effect of showing its toast +— subject to [`shouldKeepEvent`](src/integrations/datadog/shouldKeepEvent.ts), which then drops +timeouts, 401s and third-party stacks in `beforeSend`. Reaching RUM is not the same as reaching +Error Tracking. + +That matters when replacing a toast with inline form copy: drop the `errorHandler` call and the +failure silently stops being reported. The auth forms that render their own failure keep an explicit `console.error(error)` for exactly this +reason — **at mutation level, in the `useMutation({ onError })`, not in the caller's `mutate(…, { +onError })` callback**. React Query skips the latter when the component unmounted mid-flight, which +is precisely when someone gave up on a slow sign-in and navigated away. Exclude control flow from it — +`SignIn` skips the unverified-email rejection there, because `submitForm` redirects into the +verification flow on it and reporting it would file every unverified sign-in. + +A related trap when reasoning about these forms: **a minimal `useForm` probe does not reproduce them.** +react-hook-form clears a `root` error on a resolver-rejected resubmit in isolation, but `SignUp` +demonstrably does not (#1677), and `handleSubmit(fn, () => clearErrors('root'))` does not change +that. Reproduce against the real component, and assert the request count so a "stale" alert cannot +actually be a second identical rejection. **Keeping the failure in component state, outside +react-hook-form, is the model that clears correctly** — sign-in has always done this and +forgot-password moved to it; sign-up is the remaining `root` user. (`ForgotPassword` keeps +its report in the caller because it reports conditionally: its CAPTCHA branch is deliberately silent +per #1658, and its non-retryable branch routes through `errorHandler`. The consequence is that a +forgot-password failure is reported only if the form is still mounted when it settles.) +The inverse trap is real too — a `.catch()` written to _reduce_ RUM noise must not `console.error`, +which is what #1658 was. `console.debug` is not collected, and is the channel for a swallowed +failure you still want in devtools. + +## A 5xx body never reaches an auth form + +`describeError` renders whatever the server sent, and before #1676 that reached the sign-in and +sign-up forms as a toast. Those pages are anonymous and the inline alert this PR added _persists_, +so [`describeAuthFailure`](src/features/auth/describeAuthFailure.ts) substitutes our own copy for +every 5xx (and 429) rather than deferring: a 5xx body is our infrastructure talking — Harper's +"exceeded request queue limit for resolving cache record", or an upstream +`connect ECONNREFUSED 10.0.3.x:9925` — none of it actionable by a signed-out visitor, and some of it +our topology. 4xx still defers, because that is where an authored, actionable reason lives. + +Which 5xx copy is shown turns on whether the request could already have been +processed, not on +[`curryRetryGatewayErrors`](src/integrations/api/retryGatewayErrors.ts)'s retry list — that +interceptor is installed on instance clients only +([`getInstanceClient.ts`](src/config/getInstanceClient.ts)), never on `apiClient`, so nothing +auto-retries an auth call and the retry the copy invites is the user's own. **Only 503 promises a +plain retry**, because a declining server very likely never processed the request. **502, 504, +any timeout, and `ERR_NETWORK` get a third message** instead: all three auth submits are +non-idempotent POSTs, and each of those means the request may already have been applied — axios +reports a CORS rejection and a connection dropped _after_ the POST both as `ERR_NETWORK`, so none of +them can claim the server was never reached. RFC 9110 §9.2.2: a retry is only safe once you know the +request was not applied. + +That message **states the uncertainty and stops** — the remediation is the caller's, because only +the form knows what recovery means for its endpoint ("check your inbox before requesting another +link", "check your email for a verification link", "try signing in again"). Do not fold a generic +"reload and try again" back into the shared copy: reloading an anonymous form performs no status +check, so following it just repeats the side effect this branch exists to avoid (#1668). +Everything else 5xx gets copy that does not promise waiting helps, plus a way to escalate — and +`SubmitErrorMessage` decides that from the message itself rather than from a prop each caller must +remember, which is what kept two of three forms from shipping without it. + +Do not "improve" this by rendering the server's 5xx sentence when it looks presentable. Two earlier +passes tried gating on whether the body held a usable sentence (truthiness, then a length and +leading-character heuristic); both leaked, and both duplicated `describeError`'s extraction where it +could drift. Gate on **status**, which cannot. + +Worth knowing for the telemetry half: with no body `describeError` falls back to `errorText(err.message)`, +so the pre-#1676 user saw the bare `Request failed with status code 503`. `"We had some trouble!"` is +the next fallback down and needs `message` absent too, which real AxiosErrors never are — it shows up +only in tests whose fixture omits it, so don't read a test expectation as production behavior here. diff --git a/e2e/README.md b/e2e/README.md index 30f80a6b9..b15c12373 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -120,7 +120,11 @@ Naming drives which project runs a spec: `*.anon.spec.ts` = no session, The only `data-testid`s in the app are in instance analytics. - **Auth is a cookie** (`POST /Login/`); the `Studio:PotentiallyAuthenticated` localStorage flag is only a hint. `storageState` captures the cookie. -- **Errors are Sonner toasts** (`[data-sonner-toast]`), not inline form messages. +- **Sign-in and sign-up render every submit failure inline** + (`p[role="alert"][data-slot="form-message"]`). **Forgot-password renders only CAPTCHA rejections + and retryable failures** (5xx/429/transport) inline and still toasts the rest, so assert on the + toast for a 4xx there. Don't write a case that expects the toast to reveal whether an account + exists — that page deliberately answers the same way either way. Errors elsewhere are Sonner toasts (`[data-sonner-toast]`). - **Verification is link/token-based** (`/#/verify-email?token=`) — no numeric code. ## Email round-trip (Mailosaur) diff --git a/e2e/tests/sign-in.anon.spec.ts b/e2e/tests/sign-in.anon.spec.ts index 622a40ea2..92526d298 100644 --- a/e2e/tests/sign-in.anon.spec.ts +++ b/e2e/tests/sign-in.anon.spec.ts @@ -7,7 +7,7 @@ import { expect, test } from '@playwright/test'; * Selector notes (from the component map): * -
, inputs name="email"/"password" with labels. * - Submit is a ; OAuth options are anchors. - * - Bad-credential errors are Sonner toasts ([data-sonner-toast]), NOT inline. + * - Submit failures render inline as

, not a toast. */ test.describe('sign-in page', () => { test.beforeEach(async ({ page }) => { diff --git a/src/features/auth/ForgotPassword.test.tsx b/src/features/auth/ForgotPassword.test.tsx index efc6486da..9c1d08d5c 100644 --- a/src/features/auth/ForgotPassword.test.tsx +++ b/src/features/auth/ForgotPassword.test.tsx @@ -39,6 +39,7 @@ vi.mock('@/lib/recaptcha/recaptchaScript', () => ({ }, })); +import { OUTCOME_UNKNOWN_MESSAGE, SERVER_ERROR_MESSAGE, SERVER_UNAVAILABLE_MESSAGE } from './describeAuthFailure'; import { ForgotPassword } from './ForgotPassword'; function wrapper() { @@ -176,10 +177,12 @@ describe('ForgotPassword — reCAPTCHA', () => { expect((await findByRole('alert')).textContent).toContain('Verification failed. Please try again.'); }); - it('leaves a non-CAPTCHA failure to the normal error path (toast, no inline notice)', async () => { + it('leaves a non-retryable failure to the normal error path (toast, no inline notice)', async () => { captchaState.token = 'human-token'; + // Deliberately not a 404 "no such account": this page promises not to reveal whether an + // address exists, so a fixture asserting that body would codify an enumeration oracle. post.mockRejectedValue( - { isAxiosError: true, response: { status: 500, data: 'boom' } } as AxiosError, + { isAxiosError: true, response: { status: 400, data: 'That address is not valid' } } as AxiosError, ); const { container, queryByRole } = renderForm(); @@ -189,4 +192,69 @@ describe('ForgotPassword — reCAPTCHA', () => { expect(queryByRole('alert')).toBeNull(); await waitFor(() => expect(toast.error).toHaveBeenCalled()); }); + + // The state model moved out of react-hook-form for exactly this: a `root` error survives a + // resubmit the resolver rejects, and this form now routes every retryable failure inline, not + // just CAPTCHA rejections. + it('drops a stale server failure on a resubmit the resolver rejects', async () => { + captchaState.token = 'human-token'; + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + post.mockRejectedValue({ isAxiosError: true, response: { status: 503 } } as AxiosError); + const { container, findByRole, queryByRole } = renderForm(); + + await submitWith(container, 'user@example.com'); + await findByRole('alert'); + + post.mockClear(); + await submitWith(container, 'not-an-email'); + + // One call proves the resolver rejected the second submit rather than it re-failing. + await waitFor(() => expect(queryByRole('alert')).toBeNull()); + expect(post).not.toHaveBeenCalled(); + consoleError.mockRestore(); + }); + + // And forgot-password's has to be forgot-password's — see the sign-up counterpart. + it('gives forgot-password’s own recovery when the outcome is unknown', async () => { + captchaState.token = 'human-token'; + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + post.mockRejectedValue({ isAxiosError: true, response: { status: 504 } } as AxiosError); + const { container, findByRole } = renderForm(); + + await submitWith(container, 'user@example.com'); + + const alert = await findByRole('alert'); + expect(alert.textContent).toContain(OUTCOME_UNKNOWN_MESSAGE); + expect(alert.textContent).toContain('Check your inbox before requesting another link.'); + expect(alert.textContent).not.toContain('verification link'); + consoleError.mockRestore(); + }); + + it('offers support for a 500, which retrying may not clear', async () => { + captchaState.token = 'human-token'; + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + post.mockRejectedValue({ isAxiosError: true, response: { status: 500 } } as AxiosError); + const { container, findByRole } = renderForm(); + + await submitWith(container, 'user@example.com'); + + const alert = await findByRole('alert'); + expect(alert.textContent).toContain(SERVER_ERROR_MESSAGE); + expect(alert.textContent).toContain('if this keeps happening'); + consoleError.mockRestore(); + }); + + it('reports a bodyless 503 inline, and still calls the RUM channel', async () => { + captchaState.token = 'human-token'; + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + post.mockRejectedValue({ isAxiosError: true, code: 'ERR_BAD_RESPONSE', response: { status: 503 } } as AxiosError); + const { container, findByRole } = renderForm(); + + await submitWith(container, 'user@example.com'); + + expect((await findByRole('alert')).textContent).toBe(SERVER_UNAVAILABLE_MESSAGE); + expect(toast.error).not.toHaveBeenCalled(); + expect(consoleError).toHaveBeenCalledWith(expect.objectContaining({ isAxiosError: true })); + consoleError.mockRestore(); + }); }); diff --git a/src/features/auth/ForgotPassword.tsx b/src/features/auth/ForgotPassword.tsx index 66f300502..d1ad752c2 100644 --- a/src/features/auth/ForgotPassword.tsx +++ b/src/features/auth/ForgotPassword.tsx @@ -1,4 +1,3 @@ -import { ContactUs } from '@/components/ContactUs'; import { Button } from '@/components/ui/button'; import { Form } from '@/components/ui/form/Form'; import { FormControl } from '@/components/ui/form/FormControl'; @@ -11,13 +10,18 @@ import { zodRequireEmail } from '@/lib/zod/email'; import { errorHandler } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { Link, useNavigate, useSearch } from '@tanstack/react-router'; -import { useEffect } from 'react'; +import { useCallback, useEffect, useState } from 'react'; import { useForm } from 'react-hook-form'; import { toast } from 'sonner'; import { z } from 'zod'; +import { SubmitErrorMessage } from './components/SubmitErrorMessage'; +import { describeRetryableAuthFailure } from './describeAuthFailure'; import { useCaptchaChallenge } from './hooks/useCaptchaChallenge'; import { useForgotPasswordMutation } from './hooks/useForgotPassword'; +// The link may already be on its way: sending them to the inbox beats a duplicate request. +const OUTCOME_UNKNOWN_RECOVERY = 'Check your inbox before requesting another link.'; + const ForgotPasswordSchema = z.object({ email: zodRequireEmail, }); @@ -32,8 +36,11 @@ export function ForgotPassword() { }, }); const email = methods.watch('email'); - const { setFocus, setError, clearErrors, control, handleSubmit, formState } = methods; - const submitError = formState.errors.root?.message; + const { setFocus, control, handleSubmit } = methods; + // Outside react-hook-form, like SignIn: a `root` error survives a resubmit the resolver rejects + // (#1677), and this form now routes far more than CAPTCHA rejections here. + const [submitError, setSubmitError] = useState(); + const clearSubmitError = useCallback(() => setSubmitError(undefined), []); useEffect(() => { setFocus('email'); @@ -43,8 +50,7 @@ export function ForgotPassword() { const captcha = useCaptchaChallenge('forgot_password'); const submitForm = async (formData: z.infer) => { - // Like SignUp: the resolver only rewrites field errors, so clear stale root. - clearErrors('root'); + setSubmitError(undefined); const captchaToken = await captcha.getToken(); submitForgotPasswordData({ ...formData, captchaToken }, { onSuccess: (message) => { @@ -60,10 +66,16 @@ export function ForgotPassword() { onError: (error) => { const captchaMessage = captcha.describeCaptchaError(error); if (captchaMessage) { - setError('root', { type: 'server', message: captchaMessage }); + setSubmitError(captchaMessage); + return; + } + const retryableMessage = describeRetryableAuthFailure(error, OUTCOME_UNKNOWN_RECOVERY); + if (retryableMessage) { + // The RUM channel for a handled rejection; nothing else on this path reports it. + console.error(error); + setSubmitError(retryableMessage); return; } - // Everything else keeps the toast it has always had. errorHandler(error); }, }); @@ -77,7 +89,7 @@ export function ForgotPassword() { )} /> - {submitError && ( -

- {submitError} - {captcha.supportSuggested && ( - <> - {' '} - if this keeps happening. - - )} -

- )} + diff --git a/src/features/auth/SignIn.test.tsx b/src/features/auth/SignIn.test.tsx new file mode 100644 index 000000000..236cd8cce --- /dev/null +++ b/src/features/auth/SignIn.test.tsx @@ -0,0 +1,151 @@ +/** + * @vitest-environment jsdom + */ +import { MutationCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { AxiosError } from 'axios'; +import { PropsWithChildren } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { post } = vi.hoisted(() => ({ post: vi.fn() })); +vi.mock('@/config/apiClient', () => ({ apiClient: { post } })); + +// One router object for the whole file, matching production's stable one. +const { navigate, router } = vi.hoisted(() => ({ navigate: vi.fn(), router: { invalidate: vi.fn() } })); +vi.mock('@tanstack/react-router', () => ({ + useNavigate: () => navigate, + useRouter: () => router, + useSearch: () => ({}), + Link: ({ children, ...rest }: PropsWithChildren<{ className?: string }>) => {children}, +})); + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), error: vi.fn(), dismiss: vi.fn() }, +})); + +vi.mock('@/integrations/datadog/datadog', () => ({ loginSuccessDatadogAction: vi.fn() })); +vi.mock('@/integrations/reo/reo', () => ({ reoClient: { identify: vi.fn() } })); + +import { toast } from 'sonner'; +// The app's own routing, not a copy: whether the failure ALSO reaches the global toast is part of +// what's under test, so restating `skipGlobalErrorToast` here would let these pass regardless. +import { mutationErrorHandler } from '@/react-query/queryClient'; +import { SERVER_ERROR_MESSAGE, SERVER_UNAVAILABLE_MESSAGE } from './describeAuthFailure'; +import { SignIn } from './SignIn'; + +function axiosError(status: number, data?: unknown): AxiosError { + return { isAxiosError: true, code: 'ERR_BAD_RESPONSE', response: { status, data } } as AxiosError; +} + +let queryClient: QueryClient; + +function renderSignIn() { + return render( + + + , + ); +} + +function fillValidForm() { + fireEvent.change(screen.getByLabelText('Email'), { target: { value: 'ada@example.com' } }); + fireEvent.change(screen.getByLabelText('Password'), { target: { value: 'correct horse battery' } }); +} + +function submit() { + fireEvent.click(screen.getByRole('button', { name: 'Sign In' })); +} + +beforeEach(() => { + queryClient = new QueryClient({ + mutationCache: new MutationCache({ onError: mutationErrorHandler }), + defaultOptions: { mutations: { retry: false } }, + }); + localStorage.clear(); +}); + +afterEach(() => vi.clearAllMocks()); + +describe('SignIn', () => { + it("reports the server's reason in the form rather than a toast", async () => { + post.mockRejectedValue(axiosError(401, { error: 'Invalid email or password' })); + + renderSignIn(); + fillValidForm(); + submit(); + + await waitFor(() => expect(screen.getByRole('alert').textContent).toContain('Invalid email or password')); + expect(toast.error).not.toHaveBeenCalled(); + expect(navigate).not.toHaveBeenCalled(); + }); + + it('says a bodyless 503 is worth reattempting, instead of a generic shrug', async () => { + post.mockRejectedValue(axiosError(503)); + + renderSignIn(); + fillValidForm(); + submit(); + + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(SERVER_UNAVAILABLE_MESSAGE)); + }); + + it('clears the previous failure when the form is resubmitted', async () => { + post.mockRejectedValueOnce(axiosError(503)); + + renderSignIn(); + fillValidForm(); + submit(); + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()); + + post.mockResolvedValueOnce({ data: { id: 'usr-1', email: 'ada@example.com', roles: {} } }); + submit(); + + await waitFor(() => expect(navigate).toHaveBeenCalled()); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + // `curryRetryGatewayErrors` retries 502/503/504 only, so a 500 must not promise that waiting + // helps — it offers a way to escalate instead. + it('offers support for a 500 rather than promising a retry helps', async () => { + post.mockRejectedValue(axiosError(500)); + + renderSignIn(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(alert.textContent).toContain(SERVER_ERROR_MESSAGE); + expect(alert.textContent).toContain('if this keeps happening'); + }); + + it('does not offer support for a 503, which waiting can clear', async () => { + post.mockRejectedValue(axiosError(503)); + + renderSignIn(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(alert.textContent).toBe(SERVER_UNAVAILABLE_MESSAGE); + }); + + it('shows no failure line before a submission fails', () => { + renderSignIn(); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + it('drops the previous failure on a resubmit the resolver rejects', async () => { + post.mockRejectedValue(axiosError(401, { error: 'Invalid email or password' })); + + renderSignIn(); + fillValidForm(); + submit(); + await waitFor(() => expect(screen.getByRole('alert').textContent).toContain('Invalid email or password')); + + fireEvent.change(screen.getByLabelText('Password'), { target: { value: '' } }); + submit(); + + await waitFor(() => expect(screen.getByText('Please enter your password.')).toBeTruthy()); + expect(screen.queryByRole('alert')).toBeNull(); + }); +}); diff --git a/src/features/auth/SignIn.tsx b/src/features/auth/SignIn.tsx index b923ba37d..c84f0cc74 100644 --- a/src/features/auth/SignIn.tsx +++ b/src/features/auth/SignIn.tsx @@ -12,6 +12,7 @@ import { Link, useSearch } from '@tanstack/react-router'; import { useForm } from 'react-hook-form'; import { GitHubAuthenticationButton } from './components/GitHubAuthenticationButton'; import { GoogleAuthenticationButton } from './components/GoogleAuthenticationButton'; +import { SubmitErrorMessage } from './components/SubmitErrorMessage'; import { useCloudSignIn } from './hooks/useCloudSignIn'; import { useLastUsedSignInMethod } from './hooks/useLastUsedSignInMethod'; @@ -31,7 +32,7 @@ export function SignIn() { const { handleSubmit, control } = methods; const email = methods.watch('email'); - const { submitForm, isPending } = useCloudSignIn(); + const { submitForm, isPending, submitError, clearSubmitError } = useCloudSignIn(); const { lastUsed, remember, recordMethod, disable, enable } = useLastUsedSignInMethod(); return ( @@ -41,7 +42,7 @@ export function SignIn() { )} /> + diff --git a/src/features/auth/SignUp.test.tsx b/src/features/auth/SignUp.test.tsx index f9e783810..0e627415f 100644 --- a/src/features/auth/SignUp.test.tsx +++ b/src/features/auth/SignUp.test.tsx @@ -41,6 +41,12 @@ import { toast } from 'sonner'; // reaches the global toast is part of what's under test, so restating that rule here would let // these tests pass even if `skipGlobalErrorToast` stopped being honored. import { mutationErrorHandler } from '@/react-query/queryClient'; +import { + OUTCOME_UNKNOWN_MESSAGE, + SERVER_ERROR_MESSAGE, + SERVER_UNAVAILABLE_MESSAGE, + TOO_MANY_ATTEMPTS_MESSAGE, +} from './describeAuthFailure'; import { SignUp } from './SignUp'; function axiosError(status: number, data?: unknown): AxiosError { @@ -48,6 +54,8 @@ function axiosError(status: number, data?: unknown): AxiosError { } // Fresh per test, so nothing leaks between them. +const UNPAIRED_SURROGATE = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(? vi.clearAllMocks()); describe('SignUp', () => { it("reports the server's reason in the form rather than a toast", async () => { - post.mockRejectedValue(axiosError(500, { code: 'InternalError', title: 'Signup is unavailable' })); + post.mockRejectedValue(axiosError(422, { code: 'InvalidEmail', title: 'That address is not accepted' })); renderSignUp(); fillValidForm(); submit(); - await waitFor(() => expect(screen.getByRole('alert').textContent).toContain('Signup is unavailable')); + await waitFor(() => expect(screen.getByRole('alert').textContent).toContain('That address is not accepted')); expect(toast.error).not.toHaveBeenCalled(); expect(navigate).not.toHaveBeenCalled(); }); + // A 4xx body renders verbatim, and an edge/WAF block page arrives as one long string. The cut is + // a UTF-16 slice, so it can land inside a surrogate pair — `truncate` strips the orphan. + it.each([ + ['ascii', 'x'.repeat(4000)], + // The leading odd-length run puts the 240th UTF-16 unit inside a surrogate pair; without it + // the cut lands cleanly between emoji and the test passes against a naive slice. + ['astral characters', `${'x'.repeat(11)}${'😀'.repeat(4000)}`], + ])('truncates a 4xx body of %s without corrupting it', async (_label, body) => { + post.mockRejectedValue(axiosError(403, body)); + + renderSignUp(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(Array.from(alert.textContent!).length).toBeLessThan(300); + expect(alert.textContent).toContain('…'); + // A split pair stays a lone surrogate in `textContent` — it only becomes U+FFFD at encoding + // time — so match one directly. (`String#isWellFormed` would say this too, but it needs the + // es2024 lib this repo does not target.) + expect(alert.textContent).not.toMatch(UNPAIRED_SURROGATE); + }); + + // Sign-up's recovery has to be sign-up's: pointing a would-be account holder at a password-reset + // inbox is the mis-advice this whole classification exists to prevent (#1668). + it('gives sign-up’s own recovery when the outcome is unknown', async () => { + post.mockRejectedValue(axiosError(504)); + + renderSignUp(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(alert.textContent).toContain(OUTCOME_UNKNOWN_MESSAGE); + expect(alert.textContent).toContain('Check your email for a verification link before signing up again.'); + expect(alert.textContent).not.toContain('requesting another link'); + }); + + it('offers support for a 500, which retrying may not clear', async () => { + post.mockRejectedValue(axiosError(500)); + + renderSignUp(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(alert.textContent).toContain(SERVER_ERROR_MESSAGE); + expect(alert.textContent).toContain('if this keeps happening'); + }); + + // The alert persists on an anonymous page, so a 5xx body never reaches it (#1676). + it('never renders a 5xx body, however sentence-shaped', async () => { + post.mockRejectedValue(axiosError(500, { error: 'connect ECONNREFUSED 10.0.3.14:9925' })); + + renderSignUp(); + fillValidForm(); + submit(); + + const alert = await waitFor(() => screen.getByRole('alert')); + expect(alert.textContent).toContain(SERVER_ERROR_MESSAGE); + expect(alert.textContent).not.toContain('10.0.3.14'); + }); + // Whatever central-manager rejects with has to reach the user — the form maps no status // codes of its own, so this must hold for a shape it has never seen. it.each([ @@ -106,7 +177,8 @@ describe('SignUp', () => { // A legacy "Code: sentence" body: the toast splits the first clause into its heading, and // the inline line has no heading — it must still read as a whole sentence. [409, 'Conflict: user already exists', 'Conflict: user already exists'], - [503, undefined, 'We had some trouble!'], + [503, undefined, SERVER_UNAVAILABLE_MESSAGE], + [429, undefined, TOO_MANY_ATTEMPTS_MESSAGE], ])('surfaces a %i rejection inline', async (status, data, expected) => { post.mockRejectedValue(axiosError(status, data)); @@ -117,6 +189,40 @@ describe('SignUp', () => { await waitFor(() => expect(screen.getByRole('alert').textContent).toContain(expected)); }); + it('still reports an inline failure to telemetry', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + post.mockRejectedValue(axiosError(503)); + + renderSignUp(); + fillValidForm(); + submit(); + + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()); + expect(consoleError).toHaveBeenCalledWith(expect.objectContaining({ isAxiosError: true })); + consoleError.mockRestore(); + }); + + // Documents a real defect (#1677), not desired behavior: the server failure outlives a resubmit + // the resolver rejected, so it sits next to a contradicting field error. `post` is asserted at + // one call to prove the second submit really was rejected client-side rather than re-failing. + // Adding `onInvalid: () => clearErrors('root')` does not change this; sign-in is unaffected + // because its failure lives outside react-hook-form. + it('leaves a stale server failure up after an invalid resubmit (#1677)', async () => { + post.mockRejectedValue(axiosError(409, 'User already exists')); + + renderSignUp(); + fillValidForm(); + submit(); + await waitFor(() => expect(screen.getByRole('alert').textContent).toContain('User already exists')); + + fireEvent.change(screen.getByLabelText('Email'), { target: { value: 'not-an-email' } }); + submit(); + + await waitFor(() => expect(screen.getByText('Please enter a valid email address.')).toBeTruthy()); + expect(post).toHaveBeenCalledTimes(1); + expect(screen.getByRole('alert').textContent).toContain('User already exists'); + }); + it('clears the previous failure when the form is resubmitted', async () => { post.mockRejectedValueOnce(axiosError(503)); diff --git a/src/features/auth/SignUp.tsx b/src/features/auth/SignUp.tsx index 0c86f014a..53db0aae5 100644 --- a/src/features/auth/SignUp.tsx +++ b/src/features/auth/SignUp.tsx @@ -1,4 +1,3 @@ -import { ContactUs } from '@/components/ContactUs'; import { Button } from '@/components/ui/button'; import { Form } from '@/components/ui/form/Form'; import { FormControl } from '@/components/ui/form/FormControl'; @@ -13,7 +12,6 @@ import { personNameRegex } from '@/lib/string/regex/personNameRegex'; import { clearUtmParamsFromUrl } from '@/lib/urls/clearUtmParams'; import { zodRequireEmail } from '@/lib/zod/email'; import { zodRequirePassword } from '@/lib/zod/password'; -import { describeError } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { Link, useNavigate, useSearch } from '@tanstack/react-router'; import { MouseEvent, useCallback, useEffect, useState } from 'react'; @@ -21,9 +19,14 @@ import { useForm } from 'react-hook-form'; import { z } from 'zod'; import { GitHubAuthenticationButton } from './components/GitHubAuthenticationButton'; import { GoogleAuthenticationButton } from './components/GoogleAuthenticationButton'; +import { SubmitErrorMessage } from './components/SubmitErrorMessage'; +import { describeAuthFailure } from './describeAuthFailure'; import { useCaptchaChallenge } from './hooks/useCaptchaChallenge'; import { useSignUpMutation } from './hooks/useSignUp'; +// The account may already exist: sending them to the inbox beats a resubmit that 409s (#1668). +const SIGN_UP_OUTCOME_UNKNOWN_RECOVERY = 'Check your email for a verification link before signing up again.'; + const SignUpSchema = z.object({ email: zodRequireEmail .max(80, { error: 'Email cannot be longer than 80 characters.' }), @@ -89,8 +92,8 @@ export function SignUp() { const submitForm = useCallback(async (formData: z.infer) => { // eslint-disable-next-line @typescript-eslint/no-unused-vars const { confirmPassword, acceptTerms, ...userData } = formData; - // Drop the previous attempt's failure explicitly — `handleSubmit` reruns the resolver, - // which only rewrites field errors, so a stale `root` would outlive the retry. + // Only reached on a valid submit, so a failure the resolver rejects keeps the previous one on + // screen — #1677, reproduced by this form's own test. An `onInvalid` handler does not fix it. clearErrors('root'); const captchaToken = await captcha.getToken(); submitSignUpData({ ...userData, captchaToken }, { @@ -109,15 +112,14 @@ export function SignUp() { // The sign-up mutation opts out of the global error toast (meta.skipGlobalErrorToast) // and renders the failure in the form instead. RUM showed people resubmitting the // same details two and three times before giving up (#1612): a toast that fades, - // away from the inputs, doesn't read as "this attempt failed". Deliberately status- - // agnostic — it reports whatever the server said rather than mapping specific codes. + // away from the inputs, doesn't read as "this attempt failed". onError: (error) => { - console.error(error); // `message`, not `description`: the latter is the toast's body, with the first clause // of a "Conflict: …" style message moved out into the heading this has no room for. setError('root', { type: 'server', - message: captcha.describeCaptchaError(error) ?? describeError(error).message, + message: captcha.describeCaptchaError(error) + ?? describeAuthFailure(error, SIGN_UP_OUTCOME_UNKNOWN_RECOVERY), }); }, }); @@ -302,17 +304,7 @@ export function SignUp() { /> {termsCheckbox} - {submitError && ( -

- {submitError} - {captcha.supportSuggested && ( - <> - {' '} - if this keeps happening. - - )} -

- )} +