Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
6 changes: 5 additions & 1 deletion e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion e2e/tests/sign-in.anon.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { expect, test } from '@playwright/test';
* Selector notes (from the component map):
* - <form id="auth-signin-form">, inputs name="email"/"password" with labels.
* - Submit is a <button>Sign In</button>; OAuth options are <a> anchors.
* - Bad-credential errors are Sonner toasts ([data-sonner-toast]), NOT inline.
* - Submit failures render inline as <p role="alert" data-slot="form-message">, not a toast.
*/
test.describe('sign-in page', () => {
test.beforeEach(async ({ page }) => {
Expand Down
72 changes: 70 additions & 2 deletions src/features/auth/ForgotPassword.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();

Expand All @@ -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();
});
});
42 changes: 22 additions & 20 deletions src/features/auth/ForgotPassword.tsx
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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,
});
Expand All @@ -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<string>();
const clearSubmitError = useCallback(() => setSubmitError(undefined), []);

useEffect(() => {
setFocus('email');
Expand All @@ -43,8 +50,7 @@ export function ForgotPassword() {
const captcha = useCaptchaChallenge('forgot_password');

const submitForm = async (formData: z.infer<typeof ForgotPasswordSchema>) => {
// 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) => {
Expand All @@ -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);
},
});
Expand All @@ -77,7 +89,7 @@ export function ForgotPassword() {
<form
id="auth-forgot-password-form"
name="auth-forgot-password-form"
onSubmit={handleSubmit(submitForm)}
onSubmit={handleSubmit(submitForm, clearSubmitError)}
className="my-4"
>
<FormField
Expand All @@ -98,17 +110,7 @@ export function ForgotPassword() {
</FormItem>
)}
/>
{submitError && (
<p role="alert" data-slot="form-message" className="text-destructive text-sm">
{submitError}
{captcha.supportSuggested && (
<>
{' '}
<ContactUs overEmail /> if this keeps happening.
</>
)}
</p>
)}
<SubmitErrorMessage message={submitError} suggestSupport={captcha.supportSuggested} />
<Button type="submit" variant="submit" disabled={isPending || captcha.minting} className="w-full my-2">
Send Password Reset Email
</Button>
Expand Down
Loading