From 0765c0ea2f0a111b9443d756d98a14995bb74671 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 8 Sep 2026 10:19:41 -0400 Subject: [PATCH] fix(domains): report each added domain's outcome instead of aborting on the first conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `onSubmitClick` awaited `addDomain` with no error handling, so a rejected add — a 409 on a domain the org already has — left two defects behind. react-hook-form re-throws whatever `handleSubmit`'s callback throws (`react-hook-form@7.86.0`, `dist/index.esm.mjs:3222`), so the rejection escaped the DOM submit handler as an unhandled promise rejection. RUM recorded 11 `handling:unhandled` events in 24h alongside the 11 handled ones the global toast already produced. The loop also aborted on the first rejection, discarding the domains created before it: `refetch()`, `form.reset()` and the success toast were all skipped, so the table never showed the new records. Because the "Add www as well" button writes `example.com, www.example.com` into the field in one click, a partial failure was the normal outcome for an apex domain — and resubmitting the whole string then conflicted on the entry that had succeeded. Sessions retried 2-5 times before anything stuck. Now every entry is attempted and both halves come back, so the caller commits what succeeded and states what did not beside the input; the mutation opts out of the global toast, as the auth hooks do. Five details the sequence made load-bearing: - A failure is excused only by evidence the submit *created* the domain: a name the refreshed list has and a pre-submit snapshot did not. Presence alone cannot tell "this POST committed" from "the org already owned it", and the org list necessarily contains a name the server rejected as a duplicate. An absent snapshot (list still loading) credits nothing, since it is indistinguishable from an org that owns nothing. - Only an indeterminate failure is arbitrated that way — no response at all, or 408/502/503/504, where a gateway in front of central-manager can answer after the row committed. Any other status is the origin saying nothing was written. - Add is gated on `form.formState.isSubmitting`, not the mutation's `isPending`. The latter drops when the last add settles, while the refetch still runs with the submitted text in the field — a window this reordering introduces. - At most one Error Tracking event per submit, and only for a failure nobody expects. A 409 is the user naming a domain the org owns and the form says so inline; a 5xx or a network failure must not be hidden behind an earlier conflict. Per-entry logging would let a paste of a dozen owned domains bury real signal — the class behind #1371, #1386 and #1645. - The parsed list is capped and deduped case-insensitively. Attempting every entry removes the fail-fast loop's accidental circuit breaker, so a stray paste would otherwise fire one POST per whitespace-delimited token; and central-manager's `addDomain` only rejects a duplicate that is already ACTIVE, so `Example.com, example.com` would create two PENDING_VALIDATION rows and two DNS challenges for one identity. Closes #1685 --- .../cluster/domains/Management.test.tsx | 407 ++++++++++++++ src/features/cluster/domains/Management.tsx | 104 +++- .../domains/addDomainsSequentially.test.ts | 506 ++++++++++++++++++ .../cluster/domains/addDomainsSequentially.ts | 195 +++++++ .../mutations/addDomainToOrganization.ts | 11 +- src/lib/errorStatus.ts | 6 + src/react-query/pollUnlessForbidden.ts | 9 +- 7 files changed, 1214 insertions(+), 24 deletions(-) create mode 100644 src/features/cluster/domains/Management.test.tsx create mode 100644 src/features/cluster/domains/addDomainsSequentially.test.ts create mode 100644 src/features/cluster/domains/addDomainsSequentially.ts create mode 100644 src/lib/errorStatus.ts diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx new file mode 100644 index 000000000..2d6a874f9 --- /dev/null +++ b/src/features/cluster/domains/Management.test.tsx @@ -0,0 +1,407 @@ +/** + * @vitest-environment jsdom + */ +import { MutationCache, onlineManager, QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { fireEvent, render, waitFor } from '@testing-library/react'; +import { AxiosError } from 'axios'; +import { PropsWithChildren, Suspense } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { get, post } = vi.hoisted(() => ({ get: vi.fn(), post: vi.fn() })); +vi.mock('@/config/apiClient', () => ({ apiClient: { get, post } })); + +vi.mock('@tanstack/react-router', () => ({ + useParams: () => ({ organizationId: 'org-1', clusterId: 'clu-1' }), + Link: ({ children }: PropsWithChildren) => {children}, +})); + +vi.mock('sonner', () => ({ + toast: { success: vi.fn(), error: vi.fn(), loading: vi.fn(), dismiss: vi.fn() }, +})); + +vi.mock('@/hooks/usePermissions', () => ({ + useOrganizationRolePermissions: () => ({ create: true, update: true, remove: true, view: true }), +})); + +// The table's ChallengeCertificate poll builds its own client and sets `retry`/`refetchInterval` +// that override this test client's `retry: false`, so without this it issues real retried XHR. +const { instancePost } = vi.hoisted(() => ({ instancePost: vi.fn() })); +vi.mock('@/config/getInstanceClient', () => ({ getInstanceClient: () => ({ post: instancePost }) })); + +import { mutationErrorHandler } from '@/react-query/queryClient'; +import { toast } from 'sonner'; + +import { DomainsManagement } from './Management'; + +/** Traced from central-manager's `addDomain` through Harper's REST problem-details serializer. */ +function conflict(): AxiosError { + return { + isAxiosError: true, + response: { + status: 409, + data: { type: 'error:ClientError', code: 'ClientError', title: 'Domain already exists', status: 409 }, + }, + } as AxiosError; +} + +function renderManagement() { + const queryClient = new QueryClient({ + mutationCache: new MutationCache({ onError: mutationErrorHandler }), + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + return render( + + loading}> + + + , + ); +} + +function domainListFetches(): number { + return get.mock.calls.filter((call) => String(call[0]).startsWith('/Domain/')).length; +} + +function addButton(container: HTMLElement): HTMLButtonElement { + // `[type=submit]`, not the button's text: "Add www as well" also contains "Add". + return container.querySelector('form#cluster-add-domain-form button[type="submit"]')!; +} + +function submitWith(container: HTMLElement, domain: string) { + const input = container.querySelector('input[name="domain"]')!; + fireEvent.change(input, { target: { value: domain } }); + fireEvent.submit(container.querySelector('form#cluster-add-domain-form')!); +} + +beforeEach(() => { + instancePost.mockResolvedValue({ status: 200, data: [] }); + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: [] }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); +}); + +// `clearAllMocks` keeps implementations, so a `post.mockRejectedValue` would leak into the next +// test; reset so a test that forgets to set `post` fails instead of inheriting one. +afterEach(() => vi.resetAllMocks()); + +describe('DomainsManagement — adding a domain', () => { + it('states the conflict under the input instead of only in a toast that fades', async () => { + post.mockRejectedValue(conflict()); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + + expect((await findByText(/example\.com — Domain already exists/)).textContent).toBeTruthy(); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('keeps the domain that was created and asks again only for the one that conflicted', async () => { + post.mockImplementation((_url: string, body: { domain: string }) => + body.domain === 'www.example.com' ? Promise.reject(conflict()) : Promise.resolve({ data: { id: 'dom-1' } }) + ); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com, www.example.com'); + + await findByText(/www\.example\.com — Domain already exists/); + expect(toast.success).toHaveBeenCalledOnce(); + expect(String(vi.mocked(toast.success).mock.calls[0][0])).toContain('1 Domain added'); + expect(container.querySelector('input[name="domain"]')!.value).toBe('www.example.com'); + }); + + it('clears the input and the message once every domain is accepted', async () => { + post.mockResolvedValue({ data: { id: 'dom-1' } }); + const { container, queryByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + + await waitFor(() => expect(toast.success).toHaveBeenCalledOnce()); + expect(container.querySelector('input[name="domain"]')!.value).toBe(''); + expect(queryByText(/Domain already exists/)).toBeNull(); + }); + + it('refetches the list even when the add reported a failure, because the record may exist anyway', async () => { + // A POST that created the domain and then timed out reports as a rejection. Without a + // refetch the new record is invisible and every retry conflicts. + post.mockRejectedValue({ + isAxiosError: true, + code: 'ECONNABORTED', + message: 'timeout of 0ms exceeded', + } as AxiosError); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + const before = domainListFetches(); + + submitWith(container, 'example.com'); + + await findByText(/example\.com — timeout of 0ms exceeded/); + await waitFor(() => expect(domainListFetches()).toBeGreaterThan(before)); + }); + + it('states the conflict even though the org list contains the domain — owning it is why it 409d', async () => { + post.mockRejectedValue(conflict()); + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: [{ id: 'dom-1', domain: 'example.com', status: 'ACTIVE' }] }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + + await findByText(/example\.com — Domain already exists/); + expect(toast.success).not.toHaveBeenCalled(); + expect(container.querySelector('input[name="domain"]')!.value).toBe('example.com'); + }); + + it('does not credit an owned domain whose POST died without a response — it was there before', async () => { + post.mockRejectedValue({ + isAxiosError: true, + code: 'ECONNABORTED', + message: 'timeout of 0ms exceeded', + } as AxiosError); + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: [{ id: 'dom-1', domain: 'example.com', status: 'ACTIVE' }] }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + + await findByText(/example\.com — timeout of 0ms exceeded/); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it('believes the refreshed list over the rejection when a timed-out POST actually committed', async () => { + post.mockRejectedValue({ + isAxiosError: true, + code: 'ECONNABORTED', + message: 'timeout of 0ms exceeded', + } as AxiosError); + let listed: unknown[] = []; + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: listed }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + const { container, queryByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + listed = [{ id: 'dom-1', domain: 'example.com', status: 'PENDING_VALIDATION' }]; + + submitWith(container, 'example.com'); + + await waitFor(() => expect(toast.success).toHaveBeenCalledOnce()); + expect(queryByText(/timeout of 0ms exceeded/)).toBeNull(); + expect(container.querySelector('input[name="domain"]')!.value).toBe(''); + }); + + it('keeps Add disabled until the whole submit finishes, not just the mutation', async () => { + post.mockResolvedValue({ data: { id: 'dom-1' } }); + let releaseRefetch: () => void = () => {}; + const held = new Promise<{ data: unknown[] }>((resolve) => { + releaseRefetch = () => resolve({ data: [] }); + }); + let domainGets = 0; + get.mockImplementation((url: string) => { + if (url.startsWith('/Domain/')) { + domainGets += 1; + return domainGets === 1 ? Promise.resolve({ data: [] }) : held; + } + return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); + }); + const { container } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + + await waitFor(() => expect(post).toHaveBeenCalledOnce()); + await waitFor(() => expect(addButton(container).disabled).toBe(true)); + releaseRefetch(); + await waitFor(() => expect(addButton(container).disabled).toBe(false)); + }); + + it('refuses a stray paste instead of firing one POST per token', async () => { + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, Array.from({ length: 40 }, (_, i) => `d${i}.example.com`).join(' ')); + + await findByText(/Add at most 20 at a time/); + expect(post).not.toHaveBeenCalled(); + }); + + it('still runs the add while offline instead of pausing the mutation and hanging the form', async () => { + // `onlineManager`, not a `navigator.onLine` spy: React Query gates on the former, and with + // the default `networkMode` it pauses the mutation before `mutationFn` — `mutateAsync` + // never settles and the form locks with no message. + post.mockRejectedValue({ isAxiosError: true, code: 'ERR_NETWORK', message: 'Network Error' } as AxiosError); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + // Connectivity drops after the page loaded — offline from the start, the suspense query + // pauses and there is no form to submit. + onlineManager.setOnline(false); + try { + const listFetchesBefore = domainListFetches(); + + submitWith(container, 'example.com'); + + await findByText(/example\.com — Network Error/); + expect(post).toHaveBeenCalledOnce(); + // No reconcile attempt: React Query would pause it, so waiting would only delay this. + expect(domainListFetches()).toBe(listFetchesBefore); + expect(addButton(container).disabled).toBe(false); + } finally { + onlineManager.setOnline(true); + } + }); + + it('locks "Add www as well" during a submit, since settling rewrites the field it writes', async () => { + post.mockResolvedValue({ data: { id: 'dom-1' } }); + let releaseRefetch: () => void = () => {}; + const held = new Promise<{ data: unknown[] }>((resolve) => { + releaseRefetch = () => resolve({ data: [] }); + }); + let domainGets = 0; + get.mockImplementation((url: string) => { + if (url.startsWith('/Domain/')) { + domainGets += 1; + return domainGets === 1 ? Promise.resolve({ data: [] }) : held; + } + return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); + }); + const { container } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + const wwwButton = () => + [...container.querySelectorAll('button')].find((b) => b.textContent?.includes('Add www as well')); + + // An apex domain is what surfaces the button at all. + submitWith(container, 'example.com'); + + await waitFor(() => expect(post).toHaveBeenCalledOnce()); + await waitFor(() => expect(wwwButton()?.disabled).toBe(true)); + releaseRefetch(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')!.value).toBe('')); + }); + + it('locks the form until the domain list has loaded, since crediting needs a pre-submit list', async () => { + let releaseList: () => void = () => {}; + const held = new Promise<{ data: unknown[] }>((resolve) => { + releaseList = () => resolve({ data: [] }); + }); + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? held + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + const { container } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + expect(container.querySelector('input[name="domain"]')!.disabled).toBe(true); + expect(addButton(container).disabled).toBe(true); + + releaseList(); + + await waitFor(() => expect(addButton(container).disabled).toBe(false)); + expect(container.querySelector('input[name="domain"]')!.disabled).toBe(false); + }); + + it('unlocks the form when the domain list fails to load, rather than stranding it', async () => { + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.reject(new AxiosError('Network Error', 'ERR_NETWORK')) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + const { container } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + await waitFor(() => expect(addButton(container).disabled).toBe(false)); + }); + + it('refuses an oversized paste with its own message, not the empty-input one', async () => { + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'a.example.com, '.repeat(600)); + + await findByText(/too much text to be domain names/); + expect(post).not.toHaveBeenCalled(); + }); + + it('says something on a punctuation-only submit instead of doing nothing at all', async () => { + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, ',,'); + + await findByText(/Enter a domain name/); + expect(post).not.toHaveBeenCalled(); + }); + + it('locks the input while the submit runs, since settling rewrites it', async () => { + post.mockResolvedValue({ data: { id: 'dom-1' } }); + let releaseRefetch: () => void = () => {}; + const held = new Promise<{ data: unknown[] }>((resolve) => { + releaseRefetch = () => resolve({ data: [] }); + }); + let domainGets = 0; + get.mockImplementation((url: string) => { + if (url.startsWith('/Domain/')) { + domainGets += 1; + return domainGets === 1 ? Promise.resolve({ data: [] }) : held; + } + return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); + }); + const { container } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + const input = () => container.querySelector('input[name="domain"]')!; + + submitWith(container, 'example.com'); + + await waitFor(() => expect(post).toHaveBeenCalledOnce()); + await waitFor(() => expect(input().disabled).toBe(true)); + releaseRefetch(); + await waitFor(() => expect(input().disabled).toBe(false)); + }); + + it('does not wait on the list when every failure was determinate — it cannot change the verdict', async () => { + post.mockRejectedValue(conflict()); + let domainGets = 0; + get.mockImplementation((url: string) => { + if (url.startsWith('/Domain/')) { + domainGets += 1; + return Promise.resolve({ data: [] }); + } + return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); + }); + const { container, findByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + const before = domainGets; + + submitWith(container, 'example.com'); + + await findByText(/example\.com — Domain already exists/); + expect(domainGets).toBe(before); + }); + + it('drops the message on a resubmit that succeeds, so a stale failure cannot linger', async () => { + post.mockRejectedValueOnce(conflict()); + const { container, findByText, queryByText } = renderManagement(); + await waitFor(() => expect(container.querySelector('input[name="domain"]')).not.toBeNull()); + + submitWith(container, 'example.com'); + await findByText(/example\.com — Domain already exists/); + + post.mockResolvedValue({ data: { id: 'dom-1' } }); + submitWith(container, 'example.com'); + + await waitFor(() => expect(queryByText(/Domain already exists/)).toBeNull()); + }); +}); diff --git a/src/features/cluster/domains/Management.tsx b/src/features/cluster/domains/Management.tsx index 18a6f99de..84c720c4b 100644 --- a/src/features/cluster/domains/Management.tsx +++ b/src/features/cluster/domains/Management.tsx @@ -8,11 +8,24 @@ import { FormLabel } from '@/components/ui/form/FormLabel'; import { FormMessage } from '@/components/ui/form/FormMessage'; import { Input } from '@/components/ui/input'; import { isRunning } from '@/components/ui/utils/badgeStatus'; +import { + addDomainsSequentially, + describeDomainFailures, + domainNameCounts, + isIndeterminate, + MAX_DOMAINS_PER_SUBMIT, + MAX_INPUT_LENGTH, + namesCreated, + parseDomainList, + unresolvedFailures, + withinReconcileWindow, +} from '@/features/cluster/domains/addDomainsSequentially'; import { useDataTableColumns } from '@/features/cluster/domains/constants/tableDefinition'; import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; import { useSetDomainIdsOnCluster } from '@/features/clusters/mutations/setDomainIdsOnCluster'; import { AddOrganizationDomainSchema, + DOMAIN_REQUIRED_MESSAGE, useAddDomainToOrganization, } from '@/features/organization/mutations/addDomainToOrganization'; import { validateDomainInOrganization } from '@/features/organization/mutations/validateDomainInOrganization'; @@ -24,7 +37,7 @@ import { unique } from '@/lib/arrays/unique'; import { pluralize } from '@/lib/pluralize'; import { queryClient } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; -import { useQuery, useSuspenseQuery } from '@tanstack/react-query'; +import { onlineManager, useQuery, useSuspenseQuery } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; import { ListTodoIcon, PlusIcon, RefreshCwIcon, Save } from 'lucide-react'; import { useCallback, useMemo, useState } from 'react'; @@ -43,6 +56,7 @@ export function DomainsManagement() { data: organizationDomains, refetch, isFetching, + isPending: isDomainListPending, isRefetching, } = useQuery(getOrganizationDomainsQueryOptions(organizationId)); @@ -95,7 +109,7 @@ export function DomainsManagement() { }); }, [cluster, selectedDomainIds, setDomainIds]); - const { mutateAsync: addDomain, isPending: isAddPending } = useAddDomainToOrganization(); + const { mutateAsync: addDomain } = useAddDomainToOrganization(); const form = useForm({ resolver: zodResolver(AddOrganizationDomainSchema), defaultValues: { @@ -104,23 +118,70 @@ export function DomainsManagement() { }, }); + // Every control the settle path rewrites shares one gate. `isDomainListPending` covers the + // first load: without a pre-submit list nothing can be credited, so a POST that commits and + // then times out in that window would invite a retry that creates a second PENDING row. A + // failed load leaves `isPending` false — crediting stays off, but the form still works. + const formLocked = isDomainListPending || form.formState.isSubmitting; + const onSubmitClick = useCallback( async (formData: z.infer) => { - if (formData) { - const domains = formData.domain.split(/[,\s]+/).map((d: string) => d.trim()).filter(Boolean); - for (const domain of domains) { - await addDomain({ ...formData, domain }); - } - form.reset(); - await refetch(); + if (formData.domain.length > MAX_INPUT_LENGTH) { + form.setError('domain', { + type: 'server', + message: `That is too much text to be domain names. Paste at most ${MAX_DOMAINS_PER_SUBMIT}.`, + }); + return; + } + const attempted = parseDomainList(formData.domain); + if (attempted.length === 0) { + form.setError('domain', { type: 'server', message: DOMAIN_REQUIRED_MESSAGE }); + return; + } + if (attempted.length > MAX_DOMAINS_PER_SUBMIT) { + form.setError('domain', { + type: 'server', + message: `That is ${attempted.length} domains. Add at most ${MAX_DOMAINS_PER_SUBMIT} at a time.`, + }); + return; + } + const before = organizationDomains && domainNameCounts(organizationDomains); + const { added, failures } = await addDomainsSequentially( + attempted, + (domain) => addDomain({ ...formData, domain }), + ); + + // Only refetch when it can change something: a newly created domain has to reach the + // table, and an indeterminate failure needs the list to arbitrate. A submit that only + // hit determinate rejections changed nothing, and waiting on the retry backoff would + // just delay the inline message and keep the form locked. + // `onlineManager`, not `navigator.onLine`: it is the signal React Query itself gates on, so + // this is exactly the condition under which the refetch would pause rather than answer. + // Asking anyway would burn the whole reconcile window before the inline message appears. A + // client timeout is a different thing — the server was reached and may have committed — so + // that still reconciles. + const needsList = onlineManager.isOnline() + && (added.length > 0 || failures.some(({ error, attempted: sent }) => sent && isIndeterminate(error))); + const refreshed = needsList ? await withinReconcileWindow(refetch()) : undefined; + const created = refreshed ? namesCreated(before, domainNameCounts(refreshed.data)) : new Set(); + const unresolved = unresolvedFailures(failures, created); + const landed = added.length + failures.length - unresolved.length; + + if (landed > 0) { toast.success( - `${ - pluralize(domains.length, 'Domain', 'Domains') - } added! Please add the txt record above to your domain registrar.`, + `${pluralize(landed, 'Domain', 'Domains')} added! Please add the txt record above to your domain registrar.`, ); } + + if (unresolved.length === 0) { + form.reset(); + return; + } + + form.setValue('domain', unresolved.map(({ domain }) => domain).join(', ')); + form.setError('domain', { type: 'server', message: describeDomainFailures(unresolved) }); }, - [addDomain, form, refetch], + [addDomain, form, organizationDomains, refetch], ); const onValidateClick = useCallback(async () => { @@ -192,12 +253,23 @@ export function DomainsManagement() { New Domain Name - + {isApex && (
Adding an apex domain? -
@@ -215,7 +287,7 @@ export function DomainsManagement() { diff --git a/src/features/cluster/domains/addDomainsSequentially.test.ts b/src/features/cluster/domains/addDomainsSequentially.test.ts new file mode 100644 index 000000000..3f1575a7c --- /dev/null +++ b/src/features/cluster/domains/addDomainsSequentially.test.ts @@ -0,0 +1,506 @@ +import { + addDomainsSequentially, + describeDomainFailures, + domainNameCounts, + MAX_DOMAINS_PER_SUBMIT, + MAX_INPUT_LENGTH, + namesCreated, + NOT_ATTEMPTED, + parseDomainList, + unresolvedFailures, + withinReconcileWindow, +} from '@/features/cluster/domains/addDomainsSequentially'; +import { errorStatus } from '@/lib/errorStatus'; +import { AxiosError, AxiosHeaders } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest'; + +/** Traced from central-manager's `addDomain` through Harper's REST serializer, where + * `code = error.code ?? constructor.name` — so it is the class name, not the status phrase. */ +const CONFLICT_BODY = { + type: 'error:ClientError', + code: 'ClientError', + title: 'Domain already exists', + status: 409, + instance: '/Domain/', +}; + +function axiosFailure(status: number, data: unknown): AxiosError { + const config = { headers: new AxiosHeaders() }; + return new AxiosError(`Request failed with status code ${status}`, 'ERR_BAD_RESPONSE', config, undefined, { + status, + statusText: '', + headers: {}, + config, + data, + }); +} + +function conflict(data: unknown = CONFLICT_BODY): AxiosError { + return axiosFailure(409, data); +} + +let consoleMock: MockInstance; + +beforeEach(() => { + consoleMock = vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + consoleMock.mockRestore(); +}); + +describe('parseDomainList', () => { + it('splits the comma-and-space form the "Add www as well" button writes', () => { + expect(parseDomainList('example.com, www.example.com')).toEqual(['example.com', 'www.example.com']); + }); + + it('collapses a repeat, which the server would otherwise accept twice as separate pending rows', () => { + expect(parseDomainList('example.com, example.com')).toEqual(['example.com']); + expect(parseDomainList('example.com, www.example.com, example.com')).toEqual([ + 'example.com', + 'www.example.com', + ]); + }); + + it('case-folds, because DNS names are case-insensitive and the duplicate check is not', () => { + expect(parseDomainList('Example.COM, example.com')).toEqual(['example.com']); + expect(parseDomainList('WWW.Example.com')).toEqual(['www.example.com']); + }); + + it('drops empty entries rather than submitting a blank domain', () => { + expect(parseDomainList(' example.com ,, \n ')).toEqual(['example.com']); + expect(parseDomainList('')).toEqual([]); + }); +}); + +describe('addDomainsSequentially', () => { + it('resolves instead of rejecting when an add fails, so the rejection cannot escape the submit handler', async () => { + const outcome = await addDomainsSequentially(['example.com'], () => Promise.reject(conflict())); + + expect(outcome.added).toEqual([]); + expect(outcome.failures).toMatchObject([{ domain: 'example.com', message: 'Domain already exists' }]); + }); + + it("reports the reason as one sentence, not the toast's heading-plus-body split", async () => { + // The toast would head this with `code`, which is the thrown class name — "ClientError". + const real = await addDomainsSequentially(['a.example.com'], () => Promise.reject(conflict())); + const withDetail = await addDomainsSequentially( + ['b.example.com'], + () => Promise.reject(conflict({ ...CONFLICT_BODY, detail: 'in this organization' })), + ); + const legacyString = await addDomainsSequentially( + ['c.example.com'], + () => Promise.reject(conflict('Conflict: domain already exists')), + ); + + expect(real.failures[0].message).toBe('Domain already exists'); + expect(withDetail.failures[0].message).toBe('Domain already exists: in this organization'); + expect(legacyString.failures[0].message).toBe('Conflict: domain already exists'); + }); + + it('keeps the domains added before a failure, and reports only the ones that failed', async () => { + const addOne = vi.fn((domain: string) => + domain === 'www.example.com' ? Promise.reject(conflict()) : Promise.resolve({ id: 'dom-1' }) + ); + + const outcome = await addDomainsSequentially(['example.com', 'www.example.com'], addOne); + + expect(outcome.added).toEqual(['example.com']); + expect(outcome.failures.map(({ domain }) => domain)).toEqual(['www.example.com']); + }); + + it('attempts every entry after a failure rather than stopping at the first', async () => { + const addOne = vi.fn((domain: string) => + domain === 'first.example.com' ? Promise.reject(conflict()) : Promise.resolve({ id: 'dom-2' }) + ); + + const outcome = await addDomainsSequentially( + ['first.example.com', 'second.example.com', 'third.example.com'], + addOne, + ); + + expect(addOne).toHaveBeenCalledTimes(3); + expect(outcome.added).toEqual(['second.example.com', 'third.example.com']); + }); + + it('adds one at a time, because the endpoint conflicts on duplicates', async () => { + const inFlight: string[] = []; + const addOne = vi.fn(async (domain: string) => { + inFlight.push(domain); + expect(inFlight).toHaveLength(1); + await Promise.resolve(); + inFlight.pop(); + }); + + await addDomainsSequentially(['a.example.com', 'b.example.com'], addOne); + + expect(addOne.mock.calls.map(([domain]) => domain)).toEqual(['a.example.com', 'b.example.com']); + }); + + it('reports nothing for a submit that only conflicted — the form already says so inline', async () => { + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + () => Promise.reject(conflict()), + ); + + expect(outcome.failures).toHaveLength(3); + expect(console.error).not.toHaveBeenCalled(); + }); + + it('reports a server failure even when an expected conflict came first', async () => { + const serverError = axiosFailure(500, { code: 'Error', title: 'boom' }); + await addDomainsSequentially( + ['owned.example.com', 'new.example.com'], + (domain) => domain === 'owned.example.com' ? Promise.reject(conflict()) : Promise.reject(serverError), + ); + + expect(console.error).toHaveBeenCalledExactlyOnceWith(serverError); + }); + + it('reports one event for a batch that failed the same way, however many entries', async () => { + const same = axiosFailure(500, { code: 'Error', title: 'boom' }); + await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + () => Promise.reject(same), + ); + + expect(console.error).toHaveBeenCalledOnce(); + }); + + it('reports each distinct refusal, so a 400 is not hidden behind a 403', async () => { + const forbidden = axiosFailure(403, { code: 'ClientError', title: 'not permitted' }); + const badRequest = axiosFailure(400, { code: 'ClientError', title: 'not a domain' }); + await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + (domain) => + domain === 'a.example.com' + ? Promise.reject(conflict()) + : domain === 'b.example.com' + ? Promise.reject(forbidden) + : Promise.reject(badRequest), + ); + + expect(console.error).toHaveBeenCalledTimes(2); + expect(console.error).toHaveBeenCalledWith(forbidden); + expect(console.error).toHaveBeenCalledWith(badRequest); + }); + + it('stops the batch on a response-less failure instead of spending a timeout per name', async () => { + const offline = new AxiosError('Network Error', 'ERR_NETWORK'); + const addOne = vi.fn(() => Promise.reject(offline)); + + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + addOne, + ); + + expect(addOne).toHaveBeenCalledOnce(); + expect(outcome.failures.map(({ domain, message }) => [domain, message])).toEqual([ + ['a.example.com', 'Network Error'], + ['b.example.com', NOT_ATTEMPTED], + ['c.example.com', NOT_ATTEMPTED], + ]); + }); + + it('keeps every un-attempted name in the outcome, so none is silently dropped', async () => { + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com'], + () => Promise.reject(new AxiosError('timeout of 60000ms exceeded', 'ECONNABORTED')), + ); + + expect(outcome.added).toEqual([]); + expect(outcome.failures).toHaveLength(2); + }); + + it('does not report an un-attempted entry, which has no failure of its own', async () => { + await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + () => Promise.reject(new AxiosError('Network Error', 'ERR_NETWORK')), + ); + + expect(console.error).toHaveBeenCalledOnce(); + }); + + it('reports a client exception that follows a refusal', async () => { + // The dedup keys on status, so the pairing that could hide one is a status plus a + // statusless error. A 403 is a per-name refusal and does not abort, so the `TypeError` + // is still reached — and it is the only statusless failure the batch can reach. + const forbidden = axiosFailure(403, { code: 'ClientError', title: 'not permitted' }); + const bug = new TypeError('x is not a function'); + await addDomainsSequentially( + ['a.example.com', 'b.example.com'], + (domain) => domain === 'a.example.com' ? Promise.reject(forbidden) : Promise.reject(bug), + ); + + expect(console.error).toHaveBeenCalledTimes(2); + expect(console.error).toHaveBeenCalledWith(forbidden); + expect(console.error).toHaveBeenCalledWith(bug); + }); + + it('stops on a 500, so a degraded server is not asked 19 more times at 60s each', async () => { + const serverError = axiosFailure(500, { code: 'Error', title: 'boom' }); + const addOne = vi.fn(() => Promise.reject(serverError)); + + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + addOne, + ); + + expect(addOne).toHaveBeenCalledOnce(); + expect(outcome.failures.map(({ message }) => message)).toEqual([ + 'boom', + NOT_ATTEMPTED, + NOT_ATTEMPTED, + ]); + }); + + it('never produces two attempted failures without a status, which is what the status dedup relies on', async () => { + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com', 'c.example.com'], + () => Promise.reject(new TypeError('x is not a function')), + ); + const responseless = outcome.failures.filter(({ error, attempted }) => + attempted && errorStatus(error) === undefined + ); + + expect(responseless).toHaveLength(1); + }); + + it('keeps going after a status-carrying failure, which says nothing about the next name', async () => { + const addOne = vi.fn((domain: string) => + domain === 'a.example.com' ? Promise.reject(conflict()) : Promise.resolve({ id: 'dom-1' }) + ); + + const outcome = await addDomainsSequentially(['a.example.com', 'b.example.com'], addOne); + + expect(addOne).toHaveBeenCalledTimes(2); + expect(outcome.added).toEqual(['b.example.com']); + }); + + it('reports a network failure, which carries no status at all', async () => { + const offline = new AxiosError('Network Error', 'ERR_NETWORK'); + await addDomainsSequentially(['a.example.com'], () => Promise.reject(offline)); + + expect(console.error).toHaveBeenCalledExactlyOnceWith(offline); + }); +}); + +describe('describeDomainFailures', () => { + it('names the domain alongside the reason', () => { + expect( + describeDomainFailures([{ + domain: 'example.com', + message: 'Domain already exists', + error: undefined, + attempted: true, + }]), + ) + .toBe('example.com — Domain already exists'); + }); + + it('keeps every failure on one line', () => { + const text = describeDomainFailures([ + { domain: 'a.example.com', message: 'Domain already exists', error: undefined, attempted: true }, + { domain: 'b.example.com', message: 'not a domain', error: undefined, attempted: true }, + ]); + + expect(text).toBe('a.example.com — Domain already exists; b.example.com — not a domain'); + expect(text).not.toContain('\n'); + }); +}); + +describe('unresolvedFailures', () => { + const timedOut = { domain: 'example.com', message: 'timeout of 0ms exceeded', error: undefined, attempted: true }; + const conflicted = { + domain: 'other.example.com', + message: 'Domain already exists', + error: undefined, + attempted: true, + }; + + it('drops a failure the refreshed list corroborates, because the POST committed before it timed out', () => { + const created = namesCreated(new Map(), domainNameCounts([{ domain: 'example.com' }] as never)); + + expect(unresolvedFailures([timedOut, conflicted], created)).toEqual([conflicted]); + }); + + it('keeps a response-less failure on a name the org already held, since nothing was created', () => { + const held = domainNameCounts([{ domain: 'example.com' }] as never); + + expect(unresolvedFailures([timedOut], namesCreated(held, held))).toEqual([timedOut]); + }); + + it('keeps a 409 the list contains: already owning the domain is the reason it 409d', () => { + const owned = { + domain: 'example.com', + message: 'Domain already exists', + error: axiosFailure(409, CONFLICT_BODY), + attempted: true, + }; + const created = namesCreated(new Map(), domainNameCounts([{ domain: 'example.com' }] as never)); + + expect(unresolvedFailures([owned], created)).toEqual([owned]); + }); + + it('keeps another 4xx refusal the list happens to contain', () => { + const forbidden = { + domain: 'example.com', + message: 'not permitted', + error: axiosFailure(403, { code: 'ClientError', title: 'not permitted' }), + attempted: true, + }; + const created = namesCreated(new Map(), domainNameCounts([{ domain: 'example.com' }] as never)); + + expect(unresolvedFailures([forbidden], created)).toEqual([forbidden]); + }); + + it('keeps every failure the list does not mention', () => { + expect(unresolvedFailures([timedOut, conflicted], new Set())).toEqual([timedOut, conflicted]); + }); +}); + +describe('a committed duplicate PENDING row', () => { + it('is credited, because central-manager accepts a second row for the same name', () => { + // The POST landed and then timed out, so it reports as a failure. Presence-based + // reconciliation could not see the new row and the retry would create a third. + const before = domainNameCounts([{ domain: 'example.com' }] as never); + const after = domainNameCounts([{ domain: 'example.com' }, { domain: 'example.com' }] as never); + const timedOut = { domain: 'example.com', message: 'timeout of 0ms exceeded', error: undefined, attempted: true }; + + expect([...namesCreated(before, after)]).toEqual(['example.com']); + expect(unresolvedFailures([timedOut], namesCreated(before, after))).toEqual([]); + }); +}); + +describe('domainNameCounts', () => { + it('case-folds so the names compare against parseDomainList output', () => { + expect(domainNameCounts([{ domain: 'Example.COM' }] as never).has('example.com')).toBe(true); + }); + + it('treats a missing list as nothing present', () => { + expect(domainNameCounts(undefined).size).toBe(0); + }); +}); + +describe('namesCreated', () => { + it('keeps only what the submit added', () => { + const before = domainNameCounts([{ domain: 'old.example.com' }] as never); + const after = domainNameCounts([{ domain: 'old.example.com' }, { domain: 'new.example.com' }] as never); + + expect([...namesCreated(before, after)]).toEqual(['new.example.com']); + }); + + it('credits nothing when the org already held the name', () => { + const held = domainNameCounts([{ domain: 'example.com' }] as never); + + expect(namesCreated(held, held).size).toBe(0); + }); +}); + +describe('indeterminate statuses', () => { + const created = domainNameCounts([{ domain: 'example.com' }] as never); + const before = domainNameCounts([] as never); + + // AGENTS.md's rule for the auth forms' non-idempotent POSTs: only a 4xx refusal proves the + // request was not applied. A 500 can be a failure after the insert, so it has to be excusable + // or a committed-then-500'd row stays invisible and every retry 409s. + it.each([408, 500, 502, 503, 504])('excuses a %i, because the write may already have landed', (status) => { + const failure = { domain: 'example.com', message: 'server', error: axiosFailure(status, {}), attempted: true }; + + expect(unresolvedFailures([failure], namesCreated(before, created))).toEqual([]); + }); + + it.each([400, 403, 404, 409, 422])('keeps a %i, a refusal made before any work', (status) => { + const failure = { domain: 'example.com', message: 'refused', error: axiosFailure(status, {}), attempted: true }; + + expect(unresolvedFailures([failure], namesCreated(before, created))).toEqual([failure]); + }); +}); + +describe('namesCreated with no pre-submit list', () => { + it('credits nothing, because an unloaded list looks the same as an empty org', () => { + const after = domainNameCounts([{ domain: 'example.com' }] as never); + + expect(namesCreated(undefined, after).size).toBe(0); + }); + + it('so a response-less failure on an already-owned domain still stands', () => { + const failure = { domain: 'example.com', message: 'timeout of 0ms exceeded', error: undefined, attempted: true }; + const after = domainNameCounts([{ domain: 'example.com' }] as never); + + expect(unresolvedFailures([failure], namesCreated(undefined, after))).toEqual([failure]); + }); +}); + +describe('MAX_DOMAINS_PER_SUBMIT', () => { + it('is small enough that a stray paste is refused rather than attempted', () => { + expect(MAX_DOMAINS_PER_SUBMIT).toBeLessThanOrEqual(20); + }); +}); + +describe('MAX_INPUT_LENGTH', () => { + it('leaves room for a maximal legitimate batch, so the cap is what refuses one', () => { + const maximal = Array.from( + { length: MAX_DOMAINS_PER_SUBMIT }, + (_, i) => `${String(i).padStart(3, '0')}${'a'.repeat(237)}.example.com`, + ).join(', '); + + expect(maximal.length).toBeLessThanOrEqual(MAX_INPUT_LENGTH); + expect(parseDomainList(maximal)).toHaveLength(MAX_DOMAINS_PER_SUBMIT); + }); +}); + +describe('an entry the batch never reached', () => { + const skipped = { domain: 'example.com', message: NOT_ATTEMPTED, error: undefined, attempted: false }; + + it('is never excused, even when the refreshed list shows the name', () => { + // Somebody else's write during the outage. This submit never POSTed it, so crediting it + // would toast "1 Domain added!" for a request that was never sent. + const created = namesCreated(new Map(), domainNameCounts([{ domain: 'example.com' }] as never)); + + expect(unresolvedFailures([skipped], created)).toEqual([skipped]); + }); + + it('stays in the retry list rather than silently leaving it', async () => { + const outcome = await addDomainsSequentially( + ['a.example.com', 'b.example.com'], + () => Promise.reject(new AxiosError('Network Error', 'ERR_NETWORK')), + ); + const created = namesCreated(new Map(), domainNameCounts([{ domain: 'b.example.com' }] as never)); + + expect(unresolvedFailures(outcome.failures, created).map(({ domain }) => domain)).toEqual([ + 'a.example.com', + 'b.example.com', + ]); + }); +}); + +describe('stopsBatch', () => { + it.each([401, 429])('stops on a %i, which is about the session or the client, not the name', async (status) => { + const addOne = vi.fn(() => Promise.reject(axiosFailure(status, {}))); + + await addDomainsSequentially(['a.example.com', 'b.example.com', 'c.example.com'], addOne); + + expect(addOne).toHaveBeenCalledOnce(); + }); + + it.each([400, 403, 404, 409, 422])('keeps going past a %i, which refuses only that name', async (status) => { + const addOne = vi.fn(() => Promise.reject(axiosFailure(status, {}))); + + await addDomainsSequentially(['a.example.com', 'b.example.com', 'c.example.com'], addOne); + + expect(addOne).toHaveBeenCalledTimes(3); + }); +}); + +describe('withinReconcileWindow', () => { + it('passes the value through when the list answers in time', async () => { + await expect(withinReconcileWindow(Promise.resolve('listed'), 50)).resolves.toBe('listed'); + }); + + it('gives up rather than holding the inline message behind a retried GET', async () => { + const neverAnswers = new Promise(() => {}); + + await expect(withinReconcileWindow(neverAnswers, 10)).resolves.toBeUndefined(); + }); +}); diff --git a/src/features/cluster/domains/addDomainsSequentially.ts b/src/features/cluster/domains/addDomainsSequentially.ts new file mode 100644 index 000000000..886a8dae9 --- /dev/null +++ b/src/features/cluster/domains/addDomainsSequentially.ts @@ -0,0 +1,195 @@ +import { SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { errorStatus } from '@/lib/errorStatus'; +import { describeError } from '@/react-query/queryClient'; + +export interface DomainAdditionFailure { + domain: string; + message: string; + error: unknown; + /** False for a name the batch stopped before reaching. Nothing can excuse it: this submit + * never sent it, so its appearance in the refreshed list is somebody else's write. */ + attempted: boolean; +} + +export interface DomainAdditionOutcome { + added: string[]; + failures: DomainAdditionFailure[]; +} + +/** Attempting every entry means nothing else bounds a stray paste, which splits into one POST + * per whitespace-delimited token. */ +export const MAX_DOMAINS_PER_SUBMIT = 20; + +/** + * Whether the write may have landed despite the failure — the only question that decides if the + * refreshed list gets to overrule the rejection. + * + * A 4xx other than 408 is the server refusing the request before doing work, so nothing was + * written. Everything else may already have been applied: no response at all and a 408 say nothing + * about the origin, and a 5xx can be a failure *after* the insert. AGENTS.md states the same rule + * for the auth forms' non-idempotent POSTs — "only 503 promises a plain retry ... each of those + * means the request may already have been applied" (RFC 9110 §9.2.2). Erring toward indeterminate + * is safe here because crediting also requires the name to be newly present in the list, so an + * add that never happened cannot be credited either way. + */ +export function isIndeterminate(error: unknown): boolean { + const status = errorStatus(error); + if (status === undefined || status === 408) { + return true; + } + return status >= 500; +} + +/** Room for `MAX_DOMAINS_PER_SUBMIT` names at the 253-character DNS maximum plus separators, so a + * legitimate maximal batch never hits it — the bound exists only to keep a pasted file from + * running the split and its allocations over megabytes of text. */ +export const MAX_INPUT_LENGTH = 20 * 254 + 64; + +export const NOT_ATTEMPTED = 'Not attempted.'; + +/** One submit can carry several domains — the "Add www as well" button writes + * `example.com, www.example.com` into the field. Case-folded because central-manager's + * `addDomain` only rejects a duplicate that is already ACTIVE, so `Example.com, example.com` + * would create two PENDING_VALIDATION rows and two DNS challenges for one identity. */ +export function parseDomainList(input: string): string[] { + return [...new Set(input.split(/[,\s]+/).map((domain) => domain.trim().toLowerCase()).filter(Boolean))]; +} + +/** + * Whether this failure is about more than the one name, and so ends the batch. + * + * A per-name refusal (400/403/404/409/422) says nothing about the next name, so the loop goes on. + * Everything else is about the server or the session: an indeterminate failure means a degraded or + * unreachable server that would answer the next 19 names the same way, each costing the client's + * full 60s timeout; a 401 has already cleared auth and started a redirect + * (`src/lib/unauthorizedResponseHandler.ts`), so the rest would POST unauthenticated; a 429 is the + * limiter refusing this client, and 19 immediate retries are the one thing not to send it. + */ +function stopsBatch(error: unknown): boolean { + const status = errorStatus(error); + return isIndeterminate(error) || status === 401 || status === 429; +} + +/** + * Resolves with both halves, never rejects — for two reasons. The form has to be able to commit + * the domains created before a failure, and react-hook-form re-throws whatever `handleSubmit`'s + * callback throws, so a rejection here escapes as an unhandled promise rejection. + */ +export async function addDomainsSequentially( + domains: string[], + addOne: (domain: string) => Promise, +): Promise { + const added: string[] = []; + const failures: DomainAdditionFailure[] = []; + for (let i = 0; i < domains.length; i += 1) { + const domain = domains[i]; + try { + await addOne(domain); + added.push(domain); + } catch (error) { + failures.push({ domain, message: describeError(error).message, error, attempted: true }); + if (stopsBatch(error)) { + for (const skipped of domains.slice(i + 1)) { + failures.push({ domain: skipped, message: NOT_ATTEMPTED, error: undefined, attempted: false }); + } + break; + } + } + } + reportUnexpectedFailures(failures); + return { added, failures }; +} + +/** + * Stands in for the global mutation handler this mutation opts out of, whose `console.error` the + * RUM SDK reports. + * + * One event per distinct status, which is what keeps both halves. A 409 is expected and stated + * inline, so it is never reported — a paste of a dozen owned domains would otherwise bury real + * signal (#1371, #1386, #1645). But collapsing to a single event per submit hid the second class: + * a 500 on one domain and a 403 on another is a permissions regression behind a server fault, and + * only one of them reached RUM. + */ +function reportUnexpectedFailures(failures: DomainAdditionFailure[]): void { + const reported = new Set(); + for (const { error, attempted } of failures) { + // An entry the batch never reached is not a failure of its own. + if (!attempted || errorStatus(error) === 409) { + continue; + } + const status = errorStatus(error); + if (reported.has(status)) { + continue; + } + reported.add(status); + console.error(error); + } +} + +/** The reconciliation is an optimisation — it can only ever excuse a failure, never create one — + * so it must not hold the inline message. The shared query client sets no `retry`, leaving React + * Query's default of 3 retries at up to the 60s client timeout each; a timed-out POST is exactly + * the case that reconciles, so without a bound the form would sit disabled for minutes. */ +export const RECONCILE_TIMEOUT_MS = 4000; + +/** Resolves with `undefined` if the list does not answer in time, which credits nothing. */ +export async function withinReconcileWindow( + pending: Promise, + timeoutMs = RECONCILE_TIMEOUT_MS, +): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + pending, + new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), timeoutMs); + }), + ]); + } finally { + clearTimeout(timer); + } +} + +/** Counts, not a set: central-manager accepts a second PENDING_VALIDATION row for a name it + * already holds, so presence alone cannot see one appear. */ +export function domainNameCounts(domains: SchemaOrganizationDomain[] | undefined): Map { + const counts = new Map(); + for (const row of domains ?? []) { + const name = String(row?.domain ?? '').toLowerCase(); + counts.set(name, (counts.get(name) ?? 0) + 1); + } + return counts; +} + +/** `created` is what the submit *added*, not what the list holds: the org list necessarily + * contains a name the server rejected as a duplicate. */ +export function unresolvedFailures( + failures: DomainAdditionFailure[], + created: Set, +): DomainAdditionFailure[] { + return failures.filter(({ domain, error, attempted }) => + !attempted || !isIndeterminate(error) || !created.has(domain) + ); +} + +/** Names the list holds more of than it did. An absent `before` is a list that never loaded, + * indistinguishable from an org that owns nothing, so nothing can be credited against it. */ +export function namesCreated( + before: Map | undefined, + after: Map, +): Set { + if (!before) { + return new Set(); + } + const created = new Set(); + for (const [name, count] of after) { + if (count > (before.get(name) ?? 0)) { + created.add(name); + } + } + return created; +} + +export function describeDomainFailures(failures: DomainAdditionFailure[]): string { + return failures.map(({ domain, message }) => `${domain} — ${message}`).join('; '); +} diff --git a/src/features/organization/mutations/addDomainToOrganization.ts b/src/features/organization/mutations/addDomainToOrganization.ts index 93b332c45..66b0c231a 100644 --- a/src/features/organization/mutations/addDomainToOrganization.ts +++ b/src/features/organization/mutations/addDomainToOrganization.ts @@ -2,8 +2,10 @@ import { apiClient } from '@/config/apiClient'; import { useMutation } from '@tanstack/react-query'; import z from 'zod'; +export const DOMAIN_REQUIRED_MESSAGE = 'Enter a domain name.'; + export const AddOrganizationDomainSchema = z.object({ - domain: z.string(), + domain: z.string().trim().min(1, DOMAIN_REQUIRED_MESSAGE), organizationId: z.string(), }); @@ -16,5 +18,12 @@ export async function onAddDomainToOrganizationSubmit(formData: z.infer) => onAddDomainToOrganizationSubmit(formData), + meta: { skipGlobalErrorToast: true }, + // Not React Query's default `'online'`: that pauses the mutation before `mutationFn` runs + // while `onlineManager` says offline, so `mutateAsync` never settles, the sequential loop + // hangs on its first await with the form locked and no message, and the queued POST fires + // later for a submit the user abandoned. Letting it run means axios rejects with + // `ERR_NETWORK` immediately, which the caller reports inline like any other failure. + networkMode: 'always', }); } diff --git a/src/lib/errorStatus.ts b/src/lib/errorStatus.ts new file mode 100644 index 000000000..426d83b9d --- /dev/null +++ b/src/lib/errorStatus.ts @@ -0,0 +1,6 @@ +/** HTTP status off an axios-style error, tolerating both the axios shape + * (`error.response.status`) and a bare `{ status }`. */ +export function errorStatus(err: unknown): number | undefined { + return (err as { response?: { status?: number } })?.response?.status + ?? (err as { status?: number })?.status; +} diff --git a/src/react-query/pollUnlessForbidden.ts b/src/react-query/pollUnlessForbidden.ts index 115bf4a05..f19d0f5e2 100644 --- a/src/react-query/pollUnlessForbidden.ts +++ b/src/react-query/pollUnlessForbidden.ts @@ -1,3 +1,5 @@ +import { errorStatus } from '@/lib/errorStatus'; + /** The only part of React Query's `Query` this wrapper reads. Typed structurally * (rather than as `Query`) because `Query` is invariant in its data type, so a * concrete `Query` will not accept a `(query: Query) => …` @@ -6,13 +8,6 @@ interface QueryErrorState { state: { error: unknown }; } -/** HTTP status off an axios-style error, tolerating both the axios shape - * (`error.response.status`) and a bare `{ status }`. */ -function errorStatus(err: unknown): number | undefined { - return (err as { response?: { status?: number } })?.response?.status - ?? (err as { status?: number })?.status; -} - /** A 403 means the caller is authenticated but not permitted on this resource. * Unlike a 401 (session lost — the auth layer clears auth and redirects), a 403 * is stable: the same request will keep failing until permissions change, so