From db91764ad9cb4a10331494544145278128d01198 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 3 Sep 2026 10:20:35 -0400 Subject: [PATCH 1/3] fix(auth): answer a retryable sign-in failure with copy, not the server's words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 2026-09-01 the central-manager API has answered 503 on its unauthenticated auth resources: 17 of the 26 non-credential /Login responses in a 24h window, and three of four affected sessions never signed in. With no body the user was shown the bare "Request failed with status code 503", which says nothing about whether to retry or whether the request got far enough to check anything — RUM caught one session resubmitting nine times over seventeen minutes. Sign-in also had no inline failure line at all, so every rejection went to a toast that fades, away from the inputs. Sign-up got that treatment in #1613; sign-in never did. - describeAuthFailure answers every 5xx with our own copy and never renders the server's. These pages are anonymous and the new alert persists, and a 5xx body is our own infrastructure talking — Harper's queue internals, an upstream ECONNREFUSED with an internal address — none of it actionable by a signed-out visitor. 4xx still defers, because that is where an authored reason lives. - 429 gets its own copy: telling someone throttled that it "isn't a problem with the details you entered" invites the resubmit that rolls the window forward. - Transport codes are enumerated, not defaulted. A timeout reached the server and found it slow, so it reads as server failure; a client-side config fault (ERR_INVALID_URL, ERR_BAD_OPTION) and a local abort claim nothing. - SignIn renders its failure beside the inputs and clears it on an invalid resubmit too, since handleSubmit skips the submit handler when the resolver rejects. - Forgot-password routes retryable failures inline alongside its CAPTCHA rejections. - The three auth forms now share one SubmitErrorMessage rather than three copies of the same role="alert" markup. The 503 itself is server-side and stays open on #1676. Refs #1676 --- AGENTS.md | 66 ++++++++ e2e/README.md | 6 +- e2e/tests/sign-in.anon.spec.ts | 2 +- src/features/auth/ForgotPassword.test.tsx | 35 +++- src/features/auth/ForgotPassword.tsx | 23 ++- src/features/auth/SignIn.test.tsx | 151 ++++++++++++++++++ src/features/auth/SignIn.tsx | 6 +- src/features/auth/SignUp.test.tsx | 92 ++++++++++- src/features/auth/SignUp.tsx | 26 +-- .../auth/components/SubmitErrorMessage.tsx | 44 +++++ src/features/auth/describeAuthFailure.test.ts | 121 ++++++++++++++ src/features/auth/describeAuthFailure.ts | 67 ++++++++ .../auth/hooks/useCloudSignIn.test.tsx | 100 +++++++++++- src/features/auth/hooks/useCloudSignIn.ts | 23 ++- src/features/auth/hooks/useSignUp.ts | 4 + 15 files changed, 714 insertions(+), 52 deletions(-) create mode 100644 src/features/auth/SignIn.test.tsx create mode 100644 src/features/auth/components/SubmitErrorMessage.tsx create mode 100644 src/features/auth/describeAuthFailure.test.ts create mode 100644 src/features/auth/describeAuthFailure.ts diff --git a/AGENTS.md b/AGENTS.md index 93f53cf6c..a7014eead 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -707,3 +707,69 @@ 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` and +`ForgotPassword` demonstrably do 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. (`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, unchanged by +#1676, 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 and +any timeout get a third message** instead: all three auth submits are non-idempotent POSTs, and each +of those means the request had already been handed upstream, so the write's outcome is unknown and +"try again" turns a completed sign-up into a 409 with the verification mail already sent (#1668). +That copy says to reload before retrying. +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..a10917a63 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 { 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,32 @@ describe('ForgotPassword — reCAPTCHA', () => { expect(queryByRole('alert')).toBeNull(); await waitFor(() => expect(toast.error).toHaveBeenCalled()); }); + + 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..e6a1fc819 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'; @@ -15,6 +14,8 @@ import { useEffect } 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'; @@ -63,7 +64,13 @@ export function ForgotPassword() { setError('root', { type: 'server', message: captchaMessage }); return; } - // Everything else keeps the toast it has always had. + const retryableMessage = describeRetryableAuthFailure(error); + if (retryableMessage) { + // The RUM channel for a handled rejection; nothing else on this path reports it. + console.error(error); + setError('root', { type: 'server', message: retryableMessage }); + return; + } errorHandler(error); }, }); @@ -98,17 +105,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..23f13343d 100644 --- a/src/features/auth/SignUp.test.tsx +++ b/src/features/auth/SignUp.test.tsx @@ -41,6 +41,7 @@ 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 { 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 +49,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); + }); + + 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 +157,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 +169,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..7501ba4df 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,6 +19,8 @@ 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'; @@ -89,8 +89,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 +109,13 @@ 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), }); }, }); @@ -302,17 +300,7 @@ export function SignUp() { /> {termsCheckbox} - {submitError && ( -

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

- )} +