From b2ee95a8c4b9007926108a6a4aad41f06bd2fcb3 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sat, 10 Oct 2026 21:36:13 -0400 Subject: [PATCH 1/8] fix(domains): poll certificate progress fast only while there is any The Domains table polled the cluster's ChallengeCertificate rows every 5s for as long as it was open. A cluster without that table answers 404, which getSearchByValue reads as an empty result, so the poll never errored and pollUnlessForbidden could never stop it: one tab sent ~1,000 doomed requests in two hours. Poll at 5s only while a certificate is generating, or for three minutes after a domain change, and once a minute otherwise. The domain-ids mutation now starts that window and re-reads the rows at once, since nothing else ever invalidated them and the 5s poll was the only way a new challenge was seen. Closes #1630 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/features/cluster/DESIGN.md | 15 ++++ .../cluster/domains/Management.test.tsx | 16 +++++ .../domains/constants/tableDefinition.tsx | 9 ++- .../queries/getChallengeCertificates.test.ts | 72 +++++++++++++++++++ .../queries/getChallengeCertificates.ts | 41 +++++++++-- .../mutations/setDomainIdsOnCluster.ts | 2 + 7 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 src/features/cluster/domains/queries/getChallengeCertificates.test.ts diff --git a/DESIGN.md b/DESIGN.md index ae41b998a..f4e31e4af 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -8,7 +8,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/clusters/DESIGN.md`](src/features/clusters/DESIGN.md) — cluster list lifecycle classification, optional metadata and notification scope, and when the cluster form may terminate a cluster. -- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, and where the cluster card's sync chip gets its data. +- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, where the cluster card's sync chip gets its data, and how custom-domain cert intent and certificate progress work. - [`src/features/instance/databases/DESIGN.md`](src/features/instance/databases/DESIGN.md) — when the browse grid may show its spinner, why a table without a primary key can't be listed, why a filtered list pages without a count, why a table with a computed attribute exports by naming its columns, where columns for undeclared properties come from, and why a filter no index can answer asks first. diff --git a/src/features/cluster/DESIGN.md b/src/features/cluster/DESIGN.md index 87dbf1862..3ff9b371f 100644 --- a/src/features/cluster/DESIGN.md +++ b/src/features/cluster/DESIGN.md @@ -58,3 +58,18 @@ page for anything else. Handle it there, not per page: every page under the rout The cluster card links a `PROVISIONING` cluster to Starting-up and an `UPDATING` one to Scaling for every viewer, so a non-admin on a new cluster lands on that message instead of a dead card ([HarperFast/studio#1773](https://github.com/HarperFast/studio/issues/1773)). + +## Custom domains: cert intent is cluster-wide, and certificate progress is polled + +`generateDomainCerts` is one flag for the whole cluster. Central manager reads an absent flag on a `domainIds` edit as +unchanged, and stamps the result onto every running instance and the Cluster row, which later expansions read +([HarperFast/central-manager#669](https://github.com/HarperFast/central-manager/pull/669)). So only a bind states it: +an unbind that sent `false` would revoke it for every domain still bound +([HarperFast/studio#1626](https://github.com/HarperFast/studio/issues/1626)). + +Nothing pushes certificate progress. Central manager answers the domain change before host manager acts on it; host +manager then upserts a `ChallengeCertificate` row per added domain, and Studio reads progress from that row's +`issueDate` and `inProgress`. The Domains table polls those rows at 5s while one is generating or for three minutes +after a domain change (`watchChallengeCertificates`, which the mutation calls), and once a minute otherwise. A cluster +without the table answers 404, which `getSearchByValue` reads as an empty result, so an idle poll never errors and +nothing else stops it ([HarperFast/studio#1630](https://github.com/HarperFast/studio/issues/1630)). diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index 4711bddcf..82dfd078d 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -582,4 +582,20 @@ describe('DomainsManagement — binding and unbinding', () => { await waitFor(() => expect(put).toHaveBeenCalledOnce()); expect(sentBody()).toEqual({ domainIds: ['dom-c'], generateDomainCerts: true }); }); + + it('re-reads certificate progress as soon as a domain change lands, rather than on the idle poll', async () => { + put.mockResolvedValue({ data: {} }); + serve(boundDomains, () => ['dom-a', 'dom-b']); + const { findByText } = renderManagement(); + await findByText('a.example.com'); + await waitFor(() => expect(instancePost).toHaveBeenCalledOnce()); + + await unbind(findByText, 'a.example.com'); + + await waitFor(() => expect(instancePost).toHaveBeenCalledTimes(2)); + expect(instancePost.mock.calls[1][1]).toMatchObject({ + operation: 'search_by_value', + table: 'ChallengeCertificate', + }); + }); }); diff --git a/src/features/cluster/domains/constants/tableDefinition.tsx b/src/features/cluster/domains/constants/tableDefinition.tsx index 278f62349..0653c78b7 100644 --- a/src/features/cluster/domains/constants/tableDefinition.tsx +++ b/src/features/cluster/domains/constants/tableDefinition.tsx @@ -2,7 +2,10 @@ import { Button } from '@/components/ui/button'; import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; import { AssociateDomainWithCluster } from '@/features/cluster/domains/AssociateDomainWithCluster'; import { CertificateProgress } from '@/features/cluster/domains/components/CertificateProgress'; -import { useChallengeCertificates } from '@/features/cluster/domains/queries/getChallengeCertificates'; +import { + isGeneratingCertificate, + useChallengeCertificates, +} from '@/features/cluster/domains/queries/getChallengeCertificates'; import { VerifyDomainOwnership } from '@/features/cluster/domains/VerifyDomainOwnership'; import { useIsSettingDomainIds, useSetDomainIdsOnCluster } from '@/features/clusters/mutations/setDomainIdsOnCluster'; import { useDeleteDomain } from '@/features/organization/mutations/deleteDomain'; @@ -22,7 +25,7 @@ export function useDataTableColumns( ): Array> { const { data: challengeCertificates } = useChallengeCertificates(cluster.id); const isGeneratingCert = useMemo( - () => !!challengeCertificates?.some((cert) => !cert.issueDate || cert.inProgress), + () => !!challengeCertificates?.some(isGeneratingCertificate), [challengeCertificates], ); @@ -129,7 +132,7 @@ export function useDataTableColumns( return ( <>
Bound to this cluster
- {certForDomain && (!certForDomain.issueDate || certForDomain.inProgress) && ( + {certForDomain && isGeneratingCertificate(certForDomain) && (
diff --git a/src/features/cluster/domains/queries/getChallengeCertificates.test.ts b/src/features/cluster/domains/queries/getChallengeCertificates.test.ts new file mode 100644 index 000000000..837bb001f --- /dev/null +++ b/src/features/cluster/domains/queries/getChallengeCertificates.test.ts @@ -0,0 +1,72 @@ +import { QueryClient } from '@tanstack/react-query'; +import { AxiosError } from 'axios'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + CERTIFICATE_IDLE_POLL_MS, + CERTIFICATE_PROGRESS_POLL_MS, + CERTIFICATE_WATCH_AFTER_DOMAIN_CHANGE_MS, + ChallengeCertificate, + challengeCertificatesQueryKey, + getChallengeCertificatesQueryOptions, + watchChallengeCertificates, +} from './getChallengeCertificates'; + +vi.mock('@/config/getInstanceClient', () => ({ getInstanceClient: vi.fn() })); + +function certificate(overrides: Partial): ChallengeCertificate { + return { + domain: 'example.com', + issueDate: '2026-09-01T00:00:00Z', + renewalDate: '2026-11-01T00:00:00Z', + challengeToken: 'token', + challengeContent: 'content', + inProgress: false, + ...overrides, + }; +} + +function nextPoll(clusterId: string, data: ChallengeCertificate[] | undefined, error: unknown = null) { + const { refetchInterval } = getChallengeCertificatesQueryOptions(clusterId); + return (refetchInterval as (query: unknown) => number | false | undefined)({ state: { data, error } }); +} + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('ChallengeCertificate poll', () => { + it('idles when there is nothing to watch, including the empty result a missing table reads as', () => { + expect(nextPoll('clu-idle', [])).toBe(CERTIFICATE_IDLE_POLL_MS); + expect(nextPoll('clu-idle', undefined)).toBe(CERTIFICATE_IDLE_POLL_MS); + expect(nextPoll('clu-idle', [certificate({})])).toBe(CERTIFICATE_IDLE_POLL_MS); + }); + + it('polls at the progress rate while a certificate is pending or being renewed', () => { + expect(nextPoll('clu-pending', [certificate({}), certificate({ issueDate: null })])).toBe( + CERTIFICATE_PROGRESS_POLL_MS, + ); + expect(nextPoll('clu-pending', [certificate({ inProgress: true })])).toBe(CERTIFICATE_PROGRESS_POLL_MS); + }); + + it('polls at the progress rate for a while after a domain change, then idles again', async () => { + vi.useFakeTimers({ toFake: ['Date'] }); + vi.setSystemTime(new Date('2026-10-01T00:00:00Z')); + const queryClient = new QueryClient(); + const invalidate = vi.spyOn(queryClient, 'invalidateQueries'); + + await watchChallengeCertificates(queryClient, 'clu-watched'); + + expect(invalidate).toHaveBeenCalledWith({ queryKey: challengeCertificatesQueryKey('clu-watched') }); + expect(nextPoll('clu-watched', [])).toBe(CERTIFICATE_PROGRESS_POLL_MS); + expect(nextPoll('clu-unrelated', [])).toBe(CERTIFICATE_IDLE_POLL_MS); + + vi.setSystemTime(Date.now() + CERTIFICATE_WATCH_AFTER_DOMAIN_CHANGE_MS + 1); + expect(nextPoll('clu-watched', [])).toBe(CERTIFICATE_IDLE_POLL_MS); + }); + + it('still stops on a 403, whatever the cadence would have been', () => { + const forbidden = new AxiosError('Request failed with status code 403'); + forbidden.response = { status: 403 } as AxiosError['response']; + expect(nextPoll('clu-forbidden', [certificate({ issueDate: null })], forbidden)).toBe(false); + }); +}); diff --git a/src/features/cluster/domains/queries/getChallengeCertificates.ts b/src/features/cluster/domains/queries/getChallengeCertificates.ts index b3e9d74c8..42d0d14e6 100644 --- a/src/features/cluster/domains/queries/getChallengeCertificates.ts +++ b/src/features/cluster/domains/queries/getChallengeCertificates.ts @@ -1,7 +1,7 @@ import { getInstanceClient } from '@/config/getInstanceClient'; import { getSearchByValue } from '@/integrations/api/instance/database/getSearchByValue'; import { pollUnlessForbidden, retryUnlessRejected } from '@/react-query/pollUnlessForbidden'; -import { queryOptions, useQuery } from '@tanstack/react-query'; +import { QueryClient, queryOptions, useQuery } from '@tanstack/react-query'; export interface ChallengeCertificate { domain: string; @@ -12,9 +12,41 @@ export interface ChallengeCertificate { inProgress: boolean; } +export const CERTIFICATE_PROGRESS_POLL_MS = 5_000; +export const CERTIFICATE_IDLE_POLL_MS = 60_000; +/** Central manager answers a domain change before host manager acts on it, and host manager only + * writes a cluster's rows once it reconciles the change — after deploying the cert component and + * restarting, the first time. So the rows a change produces arrive some time after it succeeds. */ +export const CERTIFICATE_WATCH_AFTER_DOMAIN_CHANGE_MS = 3 * 60_000; + +const watchingUntil = new Map(); + +export function isGeneratingCertificate(certificate: ChallengeCertificate): boolean { + return !certificate.issueDate || certificate.inProgress; +} + +export function challengeCertificatesQueryKey(clusterId?: string) { + return [clusterId, 'ChallengeCertificate'] as const; +} + +export function watchChallengeCertificates(queryClient: QueryClient, clusterId: string) { + watchingUntil.set(clusterId, Date.now() + CERTIFICATE_WATCH_AFTER_DOMAIN_CHANGE_MS); + return queryClient.invalidateQueries({ queryKey: challengeCertificatesQueryKey(clusterId) }); +} + +function challengeCertificatePollInterval( + clusterId: string | undefined, + certificates: ChallengeCertificate[] | undefined, +): number { + const watching = !!clusterId && (watchingUntil.get(clusterId) ?? 0) > Date.now(); + return watching || certificates?.some(isGeneratingCertificate) + ? CERTIFICATE_PROGRESS_POLL_MS + : CERTIFICATE_IDLE_POLL_MS; +} + export function getChallengeCertificatesQueryOptions(clusterId?: string) { return queryOptions({ - queryKey: [clusterId, 'ChallengeCertificate'], + queryKey: challengeCertificatesQueryKey(clusterId), queryFn: async () => { if (!clusterId) { return []; } const instanceClient = getInstanceClient({ id: clusterId, forceFabricConnect: true }); @@ -32,9 +64,8 @@ export function getChallengeCertificatesQueryOptions(clusterId?: string) { return data; }, enabled: !!clusterId, - // Same always-on shape as the 10s pollers: gated only on `clusterId`, so a - // user without access to the cluster's data 403s every 5s until they navigate. - refetchInterval: pollUnlessForbidden(5000), + refetchInterval: (query) => + pollUnlessForbidden(challengeCertificatePollInterval(clusterId, query.state.data))(query), retry: retryUnlessRejected(), }); } diff --git a/src/features/clusters/mutations/setDomainIdsOnCluster.ts b/src/features/clusters/mutations/setDomainIdsOnCluster.ts index 11b0f3784..d07c6b375 100644 --- a/src/features/clusters/mutations/setDomainIdsOnCluster.ts +++ b/src/features/clusters/mutations/setDomainIdsOnCluster.ts @@ -1,4 +1,5 @@ import { apiClient } from '@/config/apiClient'; +import { watchChallengeCertificates } from '@/features/cluster/domains/queries/getChallengeCertificates'; import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; import { useIsMutating, useMutation, useQueryClient } from '@tanstack/react-query'; import { useCallback } from 'react'; @@ -33,6 +34,7 @@ export function useSetDomainIdsOnCluster() { const { queryKey } = getClusterInfoQueryOptions(clusterId); queryClient.setQueryData(queryKey, (cluster) => cluster && { ...cluster, domainIds }); void queryClient.invalidateQueries({ queryKey, exact: true }); + void watchChallengeCertificates(queryClient, clusterId); }, }); const { mutate } = mutation; From 10eec070b68be249da81fe2e940827eab12d564d Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 00:15:32 -0400 Subject: [PATCH 2/8] fix(domains): Bind sends only the ticked domains the list still offers The Bind selection was page state that outlived its rows. A ticked domain that was then removed, or bound to another cluster, was still counted and sent, and central manager refused every later Bind (404 not found, 400 already associated) until the page was left. Bind now counts and sends the selection intersected with the list's bindable rows, using the same predicate that renders the checkbox. A removal takes its row out of the cached list as soon as it succeeds, so the gap before the refetch lands cannot send it, and Bind waits while a removal is in flight, including a click queued before the controls disabled. Remove likewise refuses a click queued behind a Bind that is still sending its domain. Bind reads the list from the query client at click time, since a removal reaches the button a render later. Closes #1800 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/features/cluster/DESIGN.md | 5 + .../cluster/domains/Management.test.tsx | 143 +++++++++++++++++- src/features/cluster/domains/Management.tsx | 34 ++++- .../cluster/domains/canBindDomain.test.ts | 41 +++++ src/features/cluster/domains/canBindDomain.ts | 17 +++ .../domains/constants/tableDefinition.tsx | 19 ++- .../mutations/setDomainIdsOnCluster.ts | 8 +- .../organization/mutations/deleteDomain.ts | 24 ++- 9 files changed, 277 insertions(+), 16 deletions(-) create mode 100644 src/features/cluster/domains/canBindDomain.test.ts create mode 100644 src/features/cluster/domains/canBindDomain.ts diff --git a/DESIGN.md b/DESIGN.md index f4e31e4af..b0263d36f 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -8,7 +8,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/clusters/DESIGN.md`](src/features/clusters/DESIGN.md) — cluster list lifecycle classification, optional metadata and notification scope, and when the cluster form may terminate a cluster. -- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, where the cluster card's sync chip gets its data, and how custom-domain cert intent and certificate progress work. +- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, where the cluster card's sync chip gets its data, and how custom-domain cert intent, certificate progress and the Bind selection work. - [`src/features/instance/databases/DESIGN.md`](src/features/instance/databases/DESIGN.md) — when the browse grid may show its spinner, why a table without a primary key can't be listed, why a filtered list pages without a count, why a table with a computed attribute exports by naming its columns, where columns for undeclared properties come from, and why a filter no index can answer asks first. diff --git a/src/features/cluster/DESIGN.md b/src/features/cluster/DESIGN.md index 3ff9b371f..b0f35ad02 100644 --- a/src/features/cluster/DESIGN.md +++ b/src/features/cluster/DESIGN.md @@ -73,3 +73,8 @@ manager then upserts a `ChallengeCertificate` row per added domain, and Studio r after a domain change (`watchChallengeCertificates`, which the mutation calls), and once a minute otherwise. A cluster without the table answers 404, which `getSearchByValue` reads as an empty result, so an idle poll never errors and nothing else stops it ([HarperFast/studio#1630](https://github.com/HarperFast/studio/issues/1630)). + +The Bind selection is page state that outlives its rows, so Bind counts and sends only the ticked domains the current +list still offers for binding (`canBindDomain`, the same predicate that renders the checkbox). A removal takes its row +out of the cached list as soon as it succeeds, rather than when the refetch lands, and Bind waits while one is in flight +([HarperFast/studio#1800](https://github.com/HarperFast/studio/issues/1800)). diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index 82dfd078d..384630717 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -7,8 +7,8 @@ import { AxiosError } from 'axios'; import { PropsWithChildren, Suspense } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { get, post, put } = vi.hoisted(() => ({ get: vi.fn(), post: vi.fn(), put: vi.fn() })); -vi.mock('@/config/apiClient', () => ({ apiClient: { get, post, put } })); +const { get, post, put, del } = vi.hoisted(() => ({ get: vi.fn(), post: vi.fn(), put: vi.fn(), del: vi.fn() })); +vi.mock('@/config/apiClient', () => ({ apiClient: { get, post, put, delete: del } })); vi.mock('@tanstack/react-router', () => ({ useParams: () => ({ organizationId: 'org-1', clusterId: 'clu-1' }), @@ -583,6 +583,145 @@ describe('DomainsManagement — binding and unbinding', () => { expect(sentBody()).toEqual({ domainIds: ['dom-c'], generateDomainCerts: true }); }); + describe('a ticked domain the list no longer offers', () => { + const unboundC = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE', organizationId: 'org-1' }; + const unboundD = { id: 'dom-d', domain: 'd.example.com', status: 'ACTIVE', organizationId: 'org-1' }; + + function serveList(list: () => object[]) { + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: list() }) + : Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }) + ); + } + + function rowOf(getByText: (text: string) => HTMLElement, domain: string) { + return within(getByText(domain).closest('tr')!); + } + + async function tickBoth( + findByText: (text: string) => Promise, + getByText: (text: string) => HTMLElement, + ) { + await findByText('c.example.com'); + fireEvent.click(rowOf(getByText, 'c.example.com').getByRole('checkbox')); + fireEvent.click(rowOf(getByText, 'd.example.com').getByRole('checkbox')); + } + + it('is not sent by Bind once it has been removed, without waiting for the list to refetch', async () => { + let removed = false; + get.mockImplementation((url: string) => { + if (!url.startsWith('/Domain/')) { + return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); + } + return removed ? new Promise(() => {}) : Promise.resolve({ data: [unboundC, unboundD] }); + }); + del.mockImplementation(() => { + removed = true; + return Promise.resolve({ data: undefined }); + }); + put.mockResolvedValue({ data: {} }); + const { findByText, getByText, findByRole } = renderManagement(); + await tickBoth(findByText, getByText); + + fireEvent.click(rowOf(getByText, 'c.example.com').getByRole('button', { name: 'Remove Domain' })); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith('Domain removed!')); + const bind = await findByRole('button', { name: /Bind 1 Domain$/ }) as HTMLButtonElement; + await waitFor(() => expect(bind.disabled).toBe(false)); + fireEvent.click(bind); + + await waitFor(() => expect(put).toHaveBeenCalledOnce()); + expect(sentBody()).toEqual({ domainIds: ['dom-d'], generateDomainCerts: true }); + }); + + it('is not offered once the list shows it bound to another cluster', async () => { + let list: object[] = [unboundC]; + serveList(() => list); + const { findByText, getByText, queryByRole, queryClient } = renderManagement(); + await findByText('c.example.com'); + fireEvent.click(rowOf(getByText, 'c.example.com').getByRole('checkbox')); + expect(queryByRole('button', { name: /Bind 1 Domain/ })).not.toBeNull(); + + list = [{ ...unboundC, clusterId: 'clu-2' }]; + await queryClient.refetchQueries({ queryKey: ['org-1', 'domains'] }); + + await waitFor(() => expect(queryByRole('button', { name: /Bind \d+ Domain/ })).toBeNull()); + }); + + it('holds Bind while a removal is in flight', async () => { + let settle!: () => void; + serveList(() => [unboundC, unboundD]); + del.mockReturnValue( + new Promise((resolve) => { + settle = () => resolve({ data: undefined }); + }), + ); + const { findByText, getByText, getByRole } = renderManagement(); + await tickBoth(findByText, getByText); + const bind = getByRole('button', { name: /Bind 2 Domains/ }) as HTMLButtonElement; + + fireEvent.click(rowOf(getByText, 'c.example.com').getByRole('button', { name: 'Remove Domain' })); + + await waitFor(() => expect(bind.disabled).toBe(true)); + settle(); + await waitFor(() => expect(toast.success).toHaveBeenCalledWith('Domain removed!')); + }); + + it('refuses a Bind queued behind a removal before the controls disabled', async () => { + serveList(() => [unboundC, unboundD]); + del.mockReturnValue(new Promise(() => {})); + const { findByText, getByText, getByRole } = renderManagement(); + await tickBoth(findByText, getByText); + const bind = getByRole('button', { name: /Bind 2 Domains/ }); + + fireEvent.click(rowOf(getByText, 'c.example.com').getByRole('button', { name: 'Remove Domain' })); + fireEvent.click(bind); + + await waitFor(() => expect(del).toHaveBeenCalledOnce()); + await waitFor(() => expect((bind as HTMLButtonElement).disabled).toBe(true)); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(put).not.toHaveBeenCalled(); + }); + + it('is not removed by a click queued behind a Bind that is sending it', async () => { + serveList(() => [unboundC, unboundD]); + put.mockReturnValue(new Promise(() => {})); + const { findByText, getByText, getByRole } = renderManagement(); + await tickBoth(findByText, getByText); + const remove = rowOf(getByText, 'c.example.com').getByRole('button', { name: 'Remove Domain' }); + + fireEvent.click(getByRole('button', { name: /Bind 2 Domains/ })); + fireEvent.click(remove); + + await waitFor(() => expect(put).toHaveBeenCalledOnce()); + // Re-queried: the row re-renders as a new element once the controls disable. + await waitFor(() => + expect( + (rowOf(getByText, 'c.example.com').getByRole('button', { name: 'Remove Domain' }) as HTMLButtonElement) + .disabled, + ).toBe(true) + ); + expect(del).not.toHaveBeenCalled(); + }); + + it('is not sent by a click that lands after the list changed but before the button re-rendered', async () => { + serveList(() => [unboundC, unboundD]); + put.mockResolvedValue({ data: {} }); + const { findByText, getByText, getByRole, queryClient } = renderManagement(); + await tickBoth(findByText, getByText); + const bind = getByRole('button', { name: /Bind 2 Domains/ }); + + // React Query notifies observers on a later tick, so the click below still runs the + // handler from the render that showed both domains. + queryClient.setQueryData(['org-1', 'domains'], [unboundD]); + fireEvent.click(bind); + + await waitFor(() => expect(put).toHaveBeenCalledOnce()); + expect(sentBody()).toEqual({ domainIds: ['dom-d'], generateDomainCerts: true }); + }); + }); + it('re-reads certificate progress as soon as a domain change lands, rather than on the idle poll', async () => { put.mockResolvedValue({ data: {} }); serve(boundDomains, () => ['dom-a', 'dom-b']); diff --git a/src/features/cluster/domains/Management.tsx b/src/features/cluster/domains/Management.tsx index 16403b7d8..af825b9d4 100644 --- a/src/features/cluster/domains/Management.tsx +++ b/src/features/cluster/domains/Management.tsx @@ -20,6 +20,7 @@ import { unresolvedFailures, withinReconcileWindow, } from '@/features/cluster/domains/addDomainsSequentially'; +import { bindableDomainIds } from '@/features/cluster/domains/canBindDomain'; import { useDataTableColumns } from '@/features/cluster/domains/constants/tableDefinition'; import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; import { useIsSettingDomainIds, useSetDomainIdsOnCluster } from '@/features/clusters/mutations/setDomainIdsOnCluster'; @@ -28,6 +29,7 @@ import { DOMAIN_REQUIRED_MESSAGE, useAddDomainToOrganization, } from '@/features/organization/mutations/addDomainToOrganization'; +import { isDeletingDomain, useIsDeletingDomain } from '@/features/organization/mutations/deleteDomain'; import { validateDomainInOrganization } from '@/features/organization/mutations/validateDomainInOrganization'; import { getOrganizationDomainsQueryOptions } from '@/features/organization/queries/getOrganizationDomains'; import { useOrganizationRolePermissions } from '@/hooks/usePermissions'; @@ -37,7 +39,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 { onlineManager, useQuery, useSuspenseQuery } from '@tanstack/react-query'; +import { onlineManager, useQuery, useQueryClient, 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'; @@ -88,10 +90,30 @@ export function DomainsManagement() { ); }, []); + const domainIdsToBind = useMemo( + () => bindableDomainIds(selectedDomainIds, organizationDomains, cluster), + [cluster, organizationDomains, selectedDomainIds], + ); + const { mutate: setDomainIds } = useSetDomainIdsOnCluster(); const settingDomainIds = useIsSettingDomainIds(); + const deletingDomain = useIsDeletingDomain(); + const activeQueryClient = useQueryClient(); const onBindClick = useCallback(() => { + // From the query client, not this render: a removal reaches the button a render after it + // starts or lands, and a click in between would still send the domain being removed. + if (isDeletingDomain(activeQueryClient)) { + return; + } + const toBind = bindableDomainIds( + selectedDomainIds, + activeQueryClient.getQueryData(getOrganizationDomainsQueryOptions(organizationId).queryKey), + cluster, + ); + if (toBind.length === 0) { + return; + } if (!isRunning(cluster.status)) { toast.error('Cluster is currently ' + cluster.status, { description: 'To bind a domain, it must be running.' }); return; @@ -99,7 +121,7 @@ export function DomainsManagement() { setDomainIds({ clusterId: cluster.id, - domainIds: unique((cluster.domainIds?.slice() || []).concat(selectedDomainIds)), + domainIds: unique((cluster.domainIds?.slice() || []).concat(toBind)), generateDomainCerts: true, }, { onSuccess: () => { @@ -108,7 +130,7 @@ export function DomainsManagement() { setSelectedDomainIds([]); }, }); - }, [cluster, selectedDomainIds, setDomainIds]); + }, [activeQueryClient, cluster, organizationId, selectedDomainIds, setDomainIds]); const { mutateAsync: addDomain } = useAddDomainToOrganization(); const form = useForm({ @@ -324,14 +346,14 @@ export function DomainsManagement() { )} - {selectedDomainIds.length > 0 && ( + {domainIdsToBind.length > 0 && (
)} diff --git a/src/features/cluster/domains/canBindDomain.test.ts b/src/features/cluster/domains/canBindDomain.test.ts new file mode 100644 index 000000000..da7014b1e --- /dev/null +++ b/src/features/cluster/domains/canBindDomain.test.ts @@ -0,0 +1,41 @@ +import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { describe, expect, it } from 'vitest'; +import { bindableDomainIds, canBindDomain } from './canBindDomain'; + +const cluster = { id: 'clu-1', plans: [{ planId: 'standard' }] } as Cluster; +const domain = { id: 'dom-1', domain: 'a.example.com', status: 'ACTIVE' } as SchemaOrganizationDomain; + +describe('canBindDomain', () => { + it('offers a verified domain that no cluster holds', () => { + expect(canBindDomain(domain, cluster)).toBe(true); + }); + + it('does not offer a domain still awaiting verification', () => { + expect(canBindDomain({ ...domain, status: 'PENDING_VALIDATION' }, cluster)).toBe(false); + }); + + it('does not offer a domain bound to any cluster, this one included', () => { + expect(canBindDomain({ ...domain, clusterId: 'clu-2' }, cluster)).toBe(false); + expect(canBindDomain({ ...domain, clusterId: 'clu-1' }, cluster)).toBe(false); + }); + + it('offers nothing to a self-managed cluster', () => { + expect(canBindDomain(domain, { ...cluster, plans: [{ planId: 'self-hosted' }] } as Cluster)).toBe(false); + }); +}); + +describe('bindableDomainIds', () => { + const other = { ...domain, id: 'dom-2', domain: 'b.example.com' }; + + it('keeps the ticked ids the list still offers, in the order they were ticked', () => { + expect(bindableDomainIds(['dom-2', 'dom-1'], [domain, other], cluster)).toEqual(['dom-2', 'dom-1']); + }); + + it('drops a ticked id the list no longer has, or no longer offers', () => { + expect(bindableDomainIds(['dom-1', 'dom-2'], [{ ...domain, clusterId: 'clu-2' }], cluster)).toEqual([]); + }); + + it('offers nothing before the list has loaded', () => { + expect(bindableDomainIds(['dom-1'], undefined, cluster)).toEqual([]); + }); +}); diff --git a/src/features/cluster/domains/canBindDomain.ts b/src/features/cluster/domains/canBindDomain.ts new file mode 100644 index 000000000..cb9ed7c8e --- /dev/null +++ b/src/features/cluster/domains/canBindDomain.ts @@ -0,0 +1,17 @@ +import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; + +export function canBindDomain(domain: SchemaOrganizationDomain, cluster: Cluster): boolean { + return domain.status === 'ACTIVE' && !domain.clusterId && !clusterIsSelfManaged(cluster); +} + +/** The ticked ids the list still offers for binding, in the order they were ticked. */ +export function bindableDomainIds( + selectedDomainIds: string[], + domains: SchemaOrganizationDomain[] | undefined, + cluster: Cluster, +): string[] { + return selectedDomainIds.filter((id) => + domains?.some((domain) => domain.id === id && canBindDomain(domain, cluster)) + ); +} diff --git a/src/features/cluster/domains/constants/tableDefinition.tsx b/src/features/cluster/domains/constants/tableDefinition.tsx index 0653c78b7..7c62918cb 100644 --- a/src/features/cluster/domains/constants/tableDefinition.tsx +++ b/src/features/cluster/domains/constants/tableDefinition.tsx @@ -1,18 +1,24 @@ import { Button } from '@/components/ui/button'; import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; import { AssociateDomainWithCluster } from '@/features/cluster/domains/AssociateDomainWithCluster'; +import { canBindDomain } from '@/features/cluster/domains/canBindDomain'; import { CertificateProgress } from '@/features/cluster/domains/components/CertificateProgress'; import { isGeneratingCertificate, useChallengeCertificates, } from '@/features/cluster/domains/queries/getChallengeCertificates'; import { VerifyDomainOwnership } from '@/features/cluster/domains/VerifyDomainOwnership'; -import { useIsSettingDomainIds, useSetDomainIdsOnCluster } from '@/features/clusters/mutations/setDomainIdsOnCluster'; +import { + isSettingDomainIds, + useIsSettingDomainIds, + useSetDomainIdsOnCluster, +} from '@/features/clusters/mutations/setDomainIdsOnCluster'; import { useDeleteDomain } from '@/features/organization/mutations/deleteDomain'; import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; import { ColumnDef, createColumnHelper } from '@/lib/table'; import { queryClient } from '@/react-query/queryClient'; +import { useQueryClient } from '@tanstack/react-query'; import { useCallback, useMemo } from 'react'; import { toast } from 'sonner'; @@ -33,17 +39,23 @@ export function useDataTableColumns( const settingDomainIds = useIsSettingDomainIds(); const { mutate: deleteDomain, isPending: deletePending } = useDeleteDomain(); const isPending = settingDomainIds || deletePending; + const activeQueryClient = useQueryClient(); const deleteDomainById = useCallback((domainId: string) => { + // A Bind sending this domain disables Remove only once React re-renders. + if (isSettingDomainIds(activeQueryClient)) { + return; + } deleteDomain({ domainId: domainId, + organizationId: cluster.organizationId, }, { onSuccess: () => { void queryClient.invalidateQueries({ queryKey: [cluster.organizationId, 'domains'], refetchType: 'active' }); toast.success('Domain removed!'); }, }); - }, [deleteDomain, cluster.organizationId]); + }, [activeQueryClient, deleteDomain, cluster.organizationId]); const unbindDomainById = useCallback((domainId: string) => { // No `generateDomainCerts`: the flag is cluster-wide, so restating it here would decide cert @@ -66,8 +78,7 @@ export function useDataTableColumns( size: 50, cell: (props) => { const domain = props.row.original; - const canBind = domain.status === 'ACTIVE' && !domain.clusterId && !clusterIsSelfManaged(cluster); - if (!canBind) { + if (!canBindDomain(domain, cluster)) { return null; } return ( diff --git a/src/features/clusters/mutations/setDomainIdsOnCluster.ts b/src/features/clusters/mutations/setDomainIdsOnCluster.ts index d07c6b375..44bd5e754 100644 --- a/src/features/clusters/mutations/setDomainIdsOnCluster.ts +++ b/src/features/clusters/mutations/setDomainIdsOnCluster.ts @@ -1,7 +1,7 @@ import { apiClient } from '@/config/apiClient'; import { watchChallengeCertificates } from '@/features/cluster/domains/queries/getChallengeCertificates'; import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; -import { useIsMutating, useMutation, useQueryClient } from '@tanstack/react-query'; +import { QueryClient, useIsMutating, useMutation, useQueryClient } from '@tanstack/react-query'; import { useCallback } from 'react'; interface Params { @@ -41,13 +41,17 @@ export function useSetDomainIdsOnCluster() { // `useIsSettingDomainIds` disables the controls only once React re-renders, so a click already // queued behind the first change would otherwise build its list from the same stale cache. const mutateWhenIdle = useCallback((variables, options) => { - if (queryClient.isMutating({ mutationKey: setDomainIdsMutationKey }) === 0) { + if (!isSettingDomainIds(queryClient)) { mutate(variables, options); } }, [mutate, queryClient]); return { ...mutation, mutate: mutateWhenIdle }; } +export function isSettingDomainIds(queryClient: QueryClient): boolean { + return queryClient.isMutating({ mutationKey: setDomainIdsMutationKey }) > 0; +} + export function useIsSettingDomainIds(): boolean { return useIsMutating({ mutationKey: setDomainIdsMutationKey }) > 0; } diff --git a/src/features/organization/mutations/deleteDomain.ts b/src/features/organization/mutations/deleteDomain.ts index 4a7263726..34564c985 100644 --- a/src/features/organization/mutations/deleteDomain.ts +++ b/src/features/organization/mutations/deleteDomain.ts @@ -1,9 +1,11 @@ import { apiClient } from '@/config/apiClient'; -import { useMutation } from '@tanstack/react-query'; +import { getOrganizationDomainsQueryOptions } from '@/features/organization/queries/getOrganizationDomains'; +import { QueryClient, useIsMutating, useMutation, useQueryClient } from '@tanstack/react-query'; import z from 'zod'; export const DeleteDomainSchema = z.object({ domainId: z.string(), + organizationId: z.string(), }); async function deleteDomainSubmit(formData: z.infer) { @@ -12,8 +14,28 @@ async function deleteDomainSubmit(formData: z.infer) return data; } +const deleteDomainMutationKey = ['deleteDomain'] as const; + export function useDeleteDomain() { + const queryClient = useQueryClient(); return useMutation({ + mutationKey: deleteDomainMutationKey, mutationFn: (formData: z.infer) => deleteDomainSubmit(formData), + onSuccess: (_data, { domainId, organizationId }) => { + // Out of the list now, not when the refetch lands: a Bind selection is read against this + // list, and would send the deleted id in between. + queryClient.setQueryData( + getOrganizationDomainsQueryOptions(organizationId).queryKey, + (domains) => domains?.filter((domain) => domain.id !== domainId), + ); + }, }); } + +export function isDeletingDomain(queryClient: QueryClient): boolean { + return queryClient.isMutating({ mutationKey: deleteDomainMutationKey }) > 0; +} + +export function useIsDeletingDomain(): boolean { + return useIsMutating({ mutationKey: deleteDomainMutationKey }) > 0; +} From 169c64fe1e9dfcb0f03de6a95fc95f128d079398 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 10:57:53 -0400 Subject: [PATCH 3/8] fix(domains): invalidate through the page's query client, the same one the guards read The bind, unbind and remove handlers read mutation state and the domain list from useQueryClient() but still invalidated through the module-level queryClient import. Production provides that same instance, so nothing changed there, but any other provider (every test here) never saw the refetch. Tests now pin that each change refetches the list on the page's own client. Co-Authored-By: Claude Opus 5.5 --- .../cluster/domains/Management.test.tsx | 48 +++++++++++++++++++ src/features/cluster/domains/Management.tsx | 3 +- .../domains/constants/tableDefinition.tsx | 10 ++-- 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index 384630717..ca6b194be 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -583,6 +583,54 @@ describe('DomainsManagement — binding and unbinding', () => { expect(sentBody()).toEqual({ domainIds: ['dom-c'], generateDomainCerts: true }); }); + describe("refetches the domain list through the page's own query client", () => { + const unboundC = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE', organizationId: 'org-1' }; + + async function expectListRefetchedAfter(landed: string, fetchesBefore: number) { + await waitFor(() => expect(toast.success).toHaveBeenCalledWith(landed)); + await waitFor(() => expect(domainListFetches()).toBeGreaterThan(fetchesBefore)); + } + + it('once a bind lands', async () => { + put.mockResolvedValue({ data: {} }); + serve([unboundC], () => []); + const { findByText, getByRole } = renderManagement(); + fireEvent.click(within((await findByText('c.example.com')).closest('tr')!).getByRole('checkbox')); + const fetchesBefore = domainListFetches(); + + fireEvent.click(getByRole('button', { name: /Bind 1 Domain/ })); + + await expectListRefetchedAfter( + 'Domain(s) bound! Certificates are being generated in the background now.', + fetchesBefore, + ); + }); + + it('once an unbind lands', async () => { + put.mockResolvedValue({ data: {} }); + serve(boundDomains, () => ['dom-a', 'dom-b']); + const { findByText } = renderManagement(); + await findByText('a.example.com'); + const fetchesBefore = domainListFetches(); + + await unbind(findByText, 'a.example.com'); + + await expectListRefetchedAfter('Domain unbound!', fetchesBefore); + }); + + it('once a removal lands', async () => { + del.mockResolvedValue({ data: undefined }); + serve([unboundC], () => []); + const { findByText } = renderManagement(); + const row = (await findByText('c.example.com')).closest('tr')!; + const fetchesBefore = domainListFetches(); + + fireEvent.click(within(row).getByRole('button', { name: 'Remove Domain' })); + + await expectListRefetchedAfter('Domain removed!', fetchesBefore); + }); + }); + describe('a ticked domain the list no longer offers', () => { const unboundC = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE', organizationId: 'org-1' }; const unboundD = { id: 'dom-d', domain: 'd.example.com', status: 'ACTIVE', organizationId: 'org-1' }; diff --git a/src/features/cluster/domains/Management.tsx b/src/features/cluster/domains/Management.tsx index af825b9d4..7232e6c07 100644 --- a/src/features/cluster/domains/Management.tsx +++ b/src/features/cluster/domains/Management.tsx @@ -37,7 +37,6 @@ import { useRefreshClick } from '@/hooks/useRefreshClick'; import { SchemaOrganizationDomain } from '@/integrations/api/api.patch'; import { unique } from '@/lib/arrays/unique'; import { pluralize } from '@/lib/pluralize'; -import { queryClient } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { onlineManager, useQuery, useQueryClient, useSuspenseQuery } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; @@ -125,7 +124,7 @@ export function DomainsManagement() { generateDomainCerts: true, }, { onSuccess: () => { - void queryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); + void activeQueryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); toast.success('Domain(s) bound! Certificates are being generated in the background now.'); setSelectedDomainIds([]); }, diff --git a/src/features/cluster/domains/constants/tableDefinition.tsx b/src/features/cluster/domains/constants/tableDefinition.tsx index 7c62918cb..f24b4882d 100644 --- a/src/features/cluster/domains/constants/tableDefinition.tsx +++ b/src/features/cluster/domains/constants/tableDefinition.tsx @@ -17,7 +17,6 @@ import { useDeleteDomain } from '@/features/organization/mutations/deleteDomain' import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; import { ColumnDef, createColumnHelper } from '@/lib/table'; -import { queryClient } from '@/react-query/queryClient'; import { useQueryClient } from '@tanstack/react-query'; import { useCallback, useMemo } from 'react'; import { toast } from 'sonner'; @@ -51,7 +50,10 @@ export function useDataTableColumns( organizationId: cluster.organizationId, }, { onSuccess: () => { - void queryClient.invalidateQueries({ queryKey: [cluster.organizationId, 'domains'], refetchType: 'active' }); + void activeQueryClient.invalidateQueries({ + queryKey: [cluster.organizationId, 'domains'], + refetchType: 'active', + }); toast.success('Domain removed!'); }, }); @@ -65,11 +67,11 @@ export function useDataTableColumns( domainIds: (cluster.domainIds ?? []).filter((id) => id !== domainId), }, { onSuccess: () => { - void queryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); + void activeQueryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); toast.success('Domain unbound!'); }, }); - }, [setDomainIds, cluster]); + }, [activeQueryClient, setDomainIds, cluster]); return useMemo(() => [ columnHelper.display({ From e1c66ca05373a049db4db7beda39e18bc60156eb Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:22:32 -0400 Subject: [PATCH 4/8] test(domains): count the list refetch from when the change is sent A domain change that reads the list before sending it (as the pre-send check stacked on this branch does) would satisfy a count taken at click time with no invalidation at all. Counting from the request makes only the post-success refetch pass. Co-Authored-By: Claude Opus 5.5 --- .../cluster/domains/Management.test.tsx | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index ca6b194be..e7491652e 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -586,48 +586,50 @@ describe('DomainsManagement — binding and unbinding', () => { describe("refetches the domain list through the page's own query client", () => { const unboundC = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE', organizationId: 'org-1' }; - async function expectListRefetchedAfter(landed: string, fetchesBefore: number) { + // Counted when the request goes out, so a read made before sending it can't pass for the refetch. + let fetchesAtSend = 0; + function answerCounting(data: unknown) { + return () => { + fetchesAtSend = domainListFetches(); + return Promise.resolve({ data }); + }; + } + + async function expectListRefetchedAfter(landed: string) { await waitFor(() => expect(toast.success).toHaveBeenCalledWith(landed)); - await waitFor(() => expect(domainListFetches()).toBeGreaterThan(fetchesBefore)); + await waitFor(() => expect(domainListFetches()).toBeGreaterThan(fetchesAtSend)); } it('once a bind lands', async () => { - put.mockResolvedValue({ data: {} }); + put.mockImplementation(answerCounting({})); serve([unboundC], () => []); const { findByText, getByRole } = renderManagement(); fireEvent.click(within((await findByText('c.example.com')).closest('tr')!).getByRole('checkbox')); - const fetchesBefore = domainListFetches(); fireEvent.click(getByRole('button', { name: /Bind 1 Domain/ })); - await expectListRefetchedAfter( - 'Domain(s) bound! Certificates are being generated in the background now.', - fetchesBefore, - ); + await expectListRefetchedAfter('Domain(s) bound! Certificates are being generated in the background now.'); }); it('once an unbind lands', async () => { - put.mockResolvedValue({ data: {} }); + put.mockImplementation(answerCounting({})); serve(boundDomains, () => ['dom-a', 'dom-b']); const { findByText } = renderManagement(); - await findByText('a.example.com'); - const fetchesBefore = domainListFetches(); await unbind(findByText, 'a.example.com'); - await expectListRefetchedAfter('Domain unbound!', fetchesBefore); + await expectListRefetchedAfter('Domain unbound!'); }); it('once a removal lands', async () => { - del.mockResolvedValue({ data: undefined }); + del.mockImplementation(answerCounting(undefined)); serve([unboundC], () => []); const { findByText } = renderManagement(); const row = (await findByText('c.example.com')).closest('tr')!; - const fetchesBefore = domainListFetches(); fireEvent.click(within(row).getByRole('button', { name: 'Remove Domain' })); - await expectListRefetchedAfter('Domain removed!', fetchesBefore); + await expectListRefetchedAfter('Domain removed!'); }); }); From bc773b4e9ef56f6fa89590a2c09d3bc06c3bc04f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 01:04:02 -0400 Subject: [PATCH 5/8] fix(domains): keep a refused domain change or cluster edit's reason on the page, and pre-check it A refused PUT /Cluster/{id} (bind, unbind, or a cluster edit) produced only a toast that faded, and the domain changes sent whole lists Studio had not checked. RUM showed 39 of 40 such PUTs refused in a day on one cluster, with users retrying across three pages (#1686). Domain changes now pass intent (add, remove). The mutation builds the list from a fresh read of the cluster and the organization's domains rather than from what the page last saw, so a domain another member bound meanwhile is no longer silently unbound. Before sending, it refuses what central manager would refuse, mirroring its source: - the cluster is not RUNNING, or an instance is FAILED or in TRANSITIONING_INSTANCE_STATES (assertClusterMutable, sharedHelpers.js:587-606; STOPPED and UPDATED included); - the cluster is self-managed (updateCluster.js:166); - a domain the change touches is missing, unverified or bound to another cluster (updateCluster.js:173-191), the cluster's existing ones included; those point at support. It refuses rather than drops: central manager unbinds whatever the new list leaves out without checking which cluster holds it (updateCluster.js:199-205). Bind and unbind failures, refusals and central manager's own reasons alike, now stay on the Domains page; a cluster edit's failure stays beside its submit button. Both mutations skip the global toast and report real request failures to RUM at mutation level. #1801's cache write of the accepted list is gone, since every change now re-reads the cluster. Refs #1686 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/features/cluster/DESIGN.md | 25 ++ .../cluster/domains/Management.test.tsx | 289 ++++++++++++++++-- src/features/cluster/domains/Management.tsx | 221 ++++++++------ .../domains/constants/tableDefinition.tsx | 26 +- .../clusters/hooks/useUpdateCluster.ts | 4 + .../describeDomainChangeRefusal.test.ts | 137 +++++++++ .../mutations/describeDomainChangeRefusal.ts | 123 ++++++++ .../mutations/setDomainIdsOnCluster.ts | 70 ++++- .../clusters/upsert/ClusterBilling.tsx | 8 + .../clusters/upsert/ClusterDetails.tsx | 6 +- .../clusters/upsert/ClusterForm.test.tsx | 149 +++++++++ src/features/clusters/upsert/ClusterForm.tsx | 6 +- .../queries/getOrganizationDomains.ts | 2 +- 14 files changed, 912 insertions(+), 156 deletions(-) create mode 100644 src/features/clusters/mutations/describeDomainChangeRefusal.test.ts create mode 100644 src/features/clusters/mutations/describeDomainChangeRefusal.ts create mode 100644 src/features/clusters/upsert/ClusterForm.test.tsx diff --git a/DESIGN.md b/DESIGN.md index b0263d36f..c59b4dbe4 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -8,7 +8,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/clusters/DESIGN.md`](src/features/clusters/DESIGN.md) — cluster list lifecycle classification, optional metadata and notification scope, and when the cluster form may terminate a cluster. -- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, where the cluster card's sync chip gets its data, and how custom-domain cert intent, certificate progress and the Bind selection work. +- [`src/features/cluster/DESIGN.md`](src/features/cluster/DESIGN.md) — when the Starting-up and Scaling screens may report done, how clone progress is read, where the cluster card's sync chip gets its data, how custom-domain cert intent, certificate progress and the Bind selection work, and why a domain change is built from a fresh read. - [`src/features/instance/databases/DESIGN.md`](src/features/instance/databases/DESIGN.md) — when the browse grid may show its spinner, why a table without a primary key can't be listed, why a filtered list pages without a count, why a table with a computed attribute exports by naming its columns, where columns for undeclared properties come from, and why a filter no index can answer asks first. diff --git a/src/features/cluster/DESIGN.md b/src/features/cluster/DESIGN.md index b0f35ad02..f5b018903 100644 --- a/src/features/cluster/DESIGN.md +++ b/src/features/cluster/DESIGN.md @@ -78,3 +78,28 @@ The Bind selection is page state that outlives its rows, so Bind counts and send list still offers for binding (`canBindDomain`, the same predicate that renders the checkbox). A removal takes its row out of the cached list as soon as it succeeds, rather than when the refetch lands, and Bind waits while one is in flight ([HarperFast/studio#1800](https://github.com/HarperFast/studio/issues/1800)). + +## Custom domains: a domain change is built and checked from a fresh read + +`PUT /Cluster/{id}` replaces the cluster's whole `domainIds` list, and central manager unbinds every domain the new +list leaves out without checking which cluster that domain is bound to. So callers of `useSetDomainIdsOnCluster` pass +intent (`add`, `remove`), and the mutation builds the list from a fresh read of the cluster and of the organization's +domains, never from what the page last saw: another tab or member may have bound a domain since. + +Before sending, it refuses what central manager would refuse, so the user gets a reason instead of a bare 400 +([HarperFast/studio#1686](https://github.com/HarperFast/studio/issues/1686)), in +[`describeDomainChangeRefusal.ts`](../clusters/mutations/describeDomainChangeRefusal.ts): + +- **The cluster can't change.** Central manager's `assertClusterMutable` refuses every cluster update, edits included, + unless the cluster is `RUNNING` and no instance is `FAILED` or in its `TRANSITIONING_INSTANCE_STATES`, which include + `STOPPED` and `UPDATED`. Studio keeps a copy of that list. A status missing from the copy still reaches central + manager, whose own refusal is then shown, so the copy must never list more than central manager's does. +- **A domain it touches is refused.** That covers a missing, unverified or foreign-bound domain, and a self-managed + cluster. Studio refuses rather than drops. Dropping a domain bound elsewhere would unbind it there. Dropping a deleted + id doesn't help either: Harper resolves a missing id in the `domains` relationship to `undefined`, so central + manager's own removal loop fails on such a cluster whatever the request carries. A refusal over a domain the cluster + already lists points at support, since nothing on the page clears it. + +Failures stay on the page beside the control, for both domain changes and cluster edits, and the global toast is off +for both mutations. Each mutation reports a real request failure to RUM itself, at mutation level, but not a refusal, +which is Studio declining to send. diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index e7491652e..83ba45bb7 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -1,7 +1,7 @@ /** * @vitest-environment jsdom */ -import { MutationCache, onlineManager, QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { MutationCache, onlineManager, QueryCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { fireEvent, render, waitFor, within } from '@testing-library/react'; import { AxiosError } from 'axios'; import { PropsWithChildren, Suspense } from 'react'; @@ -10,8 +10,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const { get, post, put, del } = vi.hoisted(() => ({ get: vi.fn(), post: vi.fn(), put: vi.fn(), del: vi.fn() })); vi.mock('@/config/apiClient', () => ({ apiClient: { get, post, put, delete: del } })); +// One object, mutated rather than replaced: the page reads its params into dependency arrays. +const { routeParams } = vi.hoisted(() => ({ routeParams: { organizationId: 'org-1', clusterId: 'clu-1' } })); vi.mock('@tanstack/react-router', () => ({ - useParams: () => ({ organizationId: 'org-1', clusterId: 'clu-1' }), + useParams: () => routeParams, Link: ({ children }: PropsWithChildren) => {children}, })); @@ -28,7 +30,7 @@ vi.mock('@/hooks/usePermissions', () => ({ const { instancePost } = vi.hoisted(() => ({ instancePost: vi.fn() })); vi.mock('@/config/getInstanceClient', () => ({ getInstanceClient: () => ({ post: instancePost }) })); -import { mutationErrorHandler } from '@/react-query/queryClient'; +import { mutationErrorHandler, queryErrorHandler } from '@/react-query/queryClient'; import { toast } from 'sonner'; import { DomainsManagement } from './Management'; @@ -46,19 +48,19 @@ function conflict(): AxiosError { function renderManagement() { const queryClient = new QueryClient({ + queryCache: new QueryCache({ onError: queryErrorHandler }), mutationCache: new MutationCache({ onError: mutationErrorHandler }), defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, }); - return { - queryClient, - ...render( - - loading}> - - - , - ), - }; + const page = () => ( + + loading}> + + + + ); + const rendered = render(page()); + return { queryClient, ...rendered, rerenderPage: () => rendered.rerender(page()) }; } function domainListFetches(): number { @@ -87,7 +89,10 @@ beforeEach(() => { // `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()); +afterEach(() => { + vi.resetAllMocks(); + routeParams.clusterId = 'clu-1'; +}); describe('DomainsManagement — adding a domain', () => { it('states the conflict under the input instead of only in a toast that fades', async () => { @@ -497,7 +502,7 @@ describe('DomainsManagement — binding and unbinding', () => { expect(sentBody(1)).toEqual({ domainIds: [] }); }); - it('builds the next change on the list the last one left even when re-reading the cluster fails', async () => { + it('sends no change built on a cluster it could not re-read', async () => { let changed = false; put.mockImplementation(() => { changed = true; @@ -517,7 +522,6 @@ describe('DomainsManagement — binding and unbinding', () => { await unbind(findByText, 'a.example.com'); await waitFor(() => expect(toast.success).toHaveBeenCalledWith('Domain unbound!')); - await waitFor(() => expect(get.mock.calls.filter(([url]) => String(url) === '/Cluster/clu-1')).toHaveLength(2)); const unbindB = await waitFor(() => { const button = within(getByText('b.example.com').closest('tr')!).getByRole('button', { name: 'Unbind Domain', @@ -527,8 +531,8 @@ describe('DomainsManagement — binding and unbinding', () => { }); fireEvent.click(unbindB); - await waitFor(() => expect(put).toHaveBeenCalledTimes(2)); - expect(sentBody(1)).toEqual({ domainIds: [] }); + expect(await findByText("The domain change didn't go through: Network Error")).toBeTruthy(); + expect(put).toHaveBeenCalledOnce(); }); it('holds every domain action while one is in flight, since each sends the whole list', async () => { @@ -658,13 +662,13 @@ describe('DomainsManagement — binding and unbinding', () => { fireEvent.click(rowOf(getByText, 'd.example.com').getByRole('checkbox')); } - it('is not sent by Bind once it has been removed, without waiting for the list to refetch', async () => { + it('is not offered or sent by Bind once it has been removed, without waiting for the list to refetch', async () => { let removed = false; get.mockImplementation((url: string) => { if (!url.startsWith('/Domain/')) { return Promise.resolve({ data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: [] } }); } - return removed ? new Promise(() => {}) : Promise.resolve({ data: [unboundC, unboundD] }); + return Promise.resolve({ data: removed ? [unboundD] : [unboundC, unboundD] }); }); del.mockImplementation(() => { removed = true; @@ -788,3 +792,248 @@ describe('DomainsManagement — binding and unbinding', () => { }); }); }); + +describe('DomainsManagement — a domain change central manager would refuse', () => { + const domainA = { + id: 'dom-a', + domain: 'a.example.com', + status: 'ACTIVE', + clusterId: 'clu-1', + organizationId: 'org-1', + }; + const domainC = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE', organizationId: 'org-1' }; + const instanceNotRunning = + 'Cluster has an instance that is not RUNNING (ins-1: FAILED). Resolve the instance before the update.'; + const shownForInstanceNotRunning = `The domain change didn't go through: ${instanceNotRunning}`; + + /** Central manager's `assertClusterMutable` refusal, through Harper's REST problem-details serializer. */ + function refused(title: string): AxiosError { + return { + isAxiosError: true, + message: 'Request failed with status code 400', + response: { status: 400, data: { type: 'error:ClientError', code: 'ClientError', title, status: 400 } }, + } as AxiosError; + } + + function serve(domains: () => object[], cluster: object = {}) { + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: domains() }) + : Promise.resolve({ + data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: ['dom-a'], ...cluster }, + }) + ); + } + + type Rendered = ReturnType; + + async function tickC({ findByText }: Rendered) { + fireEvent.click(within((await findByText('c.example.com')).closest('tr')!).getByRole('checkbox')); + } + + async function bindC(rendered: Rendered) { + await tickC(rendered); + fireEvent.click(rendered.getByRole('button', { name: /Bind 1 Domain/ })); + } + + async function unbindA({ findByText }: Rendered) { + fireEvent.click( + within((await findByText('a.example.com')).closest('tr')!).getByRole('button', { name: 'Unbind Domain' }), + ); + } + + let consoleError: ReturnType; + beforeEach(() => { + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + afterEach(() => consoleError.mockRestore()); + + it("keeps central manager's reason on the page when it refuses a bind, instead of a toast that fades", async () => { + serve(() => [domainA, domainC]); + put.mockRejectedValue(refused(instanceNotRunning)); + const rendered = renderManagement(); + + await bindC(rendered); + + expect((await rendered.findByText(shownForInstanceNotRunning)).getAttribute('role')).toBe('alert'); + expect(toast.error).not.toHaveBeenCalled(); + expect(consoleError).toHaveBeenCalledWith(expect.objectContaining({ response: expect.anything() })); + }); + + it('shows a refused unbind the same way', async () => { + serve(() => [domainA, domainC]); + put.mockRejectedValue(refused(instanceNotRunning)); + const rendered = renderManagement(); + + await unbindA(rendered); + + expect(await rendered.findByText(shownForInstanceNotRunning)).toBeTruthy(); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it('drops the reason once the next change goes through', async () => { + serve(() => [domainA, domainC]); + put.mockRejectedValueOnce(refused(instanceNotRunning)).mockResolvedValue({ data: {} }); + const rendered = renderManagement(); + await bindC(rendered); + await rendered.findByText(shownForInstanceNotRunning); + + const bind = rendered.getByRole('button', { name: /Bind 1 Domain/ }) as HTMLButtonElement; + await waitFor(() => expect(bind.disabled).toBe(false)); + fireEvent.click(bind); + + await waitFor(() => expect(put).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(rendered.queryByText(shownForInstanceNotRunning)).toBeNull()); + }); + + // UPDATED is the trap: Studio's `isRunning` counts it as running, and central manager does not. + it.each(['FAILED', 'STOPPED', 'UPDATED'])('sends nothing while an instance is %s, and says which', async (status) => { + serve(() => [domainA, domainC], { + instances: [ + { id: 'ins-2', instanceFqdn: 'node-2.example.com', status: 'RUNNING' }, + { id: 'ins-1', instanceFqdn: 'node-1.example.com', status }, + ], + }); + const rendered = renderManagement(); + + await bindC(rendered); + + expect(await rendered.findByText(/Instance node-1\.example\.com/)).toBeTruthy(); + expect(put).not.toHaveBeenCalled(); + expect(consoleError).not.toHaveBeenCalledWith(expect.any(Error)); + }); + + it('sends no unbind while the cluster is not running', async () => { + serve(() => [domainA, domainC], { status: 'UPDATING' }); + const rendered = renderManagement(); + + await unbindA(rendered); + + expect(await rendered.findByText(/This cluster is UPDATING/)).toBeTruthy(); + expect(put).not.toHaveBeenCalled(); + }); + + it('still sends a change while every instance is running or gone', async () => { + serve(() => [domainA, domainC], { + instances: [ + { id: 'ins-1', instanceFqdn: 'node-1.example.com', status: 'RUNNING' }, + { id: 'ins-2', instanceFqdn: 'node-2.example.com', status: 'TERMINATED' }, + ], + }); + put.mockResolvedValue({ data: {} }); + const rendered = renderManagement(); + + await bindC(rendered); + + await waitFor(() => expect(put).toHaveBeenCalledOnce()); + }); + + it('refuses a ticked domain the fresh list shows bound elsewhere, and stops offering it', async () => { + let list: object[] = [domainA, domainC]; + serve(() => list); + const rendered = renderManagement(); + await tickC(rendered); + + list = [domainA, { ...domainC, clusterId: 'clu-2' }]; + fireEvent.click(rendered.getByRole('button', { name: /Bind 1 Domain/ })); + + expect(await rendered.findByText(/c\.example\.com is bound to cluster clu-2/)).toBeTruthy(); + expect(put).not.toHaveBeenCalled(); + await waitFor(() => expect(rendered.queryByRole('button', { name: /Bind \d+ Domain/ })).toBeNull()); + }); + + it('refuses every change while the cluster still lists a domain that no longer exists, and points at support', async () => { + serve(() => [domainA, domainC], { domainIds: ['dom-a', 'dom-gone'] }); + const rendered = renderManagement(); + + await unbindA(rendered); + + expect(await rendered.findByText(/still lists dom-gone, which isn't one of this organization's domains/)) + .toBeTruthy(); + expect(rendered.getByRole('link', { name: 'Contact us' })).toBeTruthy(); + expect(put).not.toHaveBeenCalled(); + }); + + it('builds the list from the cluster as central manager has it, keeping a domain bound meanwhile', async () => { + let clusterDomainIds = ['dom-a']; + let domains: object[] = [domainA, domainC]; + const domainB = { + id: 'dom-b', + domain: 'b.example.com', + status: 'ACTIVE', + clusterId: 'clu-1', + organizationId: 'org-1', + }; + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: domains }) + : Promise.resolve({ + data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: clusterDomainIds }, + }) + ); + put.mockResolvedValue({ data: {} }); + const rendered = renderManagement(); + await tickC(rendered); + + // Another member binds dom-b; this page has not re-read the cluster since. + clusterDomainIds = ['dom-a', 'dom-b']; + domains = [domainA, domainB, domainC]; + fireEvent.click(rendered.getByRole('button', { name: /Bind 1 Domain/ })); + + await waitFor(() => expect(put).toHaveBeenCalledOnce()); + expect(JSON.parse(JSON.stringify(put.mock.calls[0][1]))).toEqual({ + domainIds: ['dom-a', 'dom-b', 'dom-c'], + generateDomainCerts: true, + }); + }); + + it("does not show one cluster's failed change on another cluster's page", async () => { + get.mockImplementation((url: string) => + url.startsWith('/Domain/') + ? Promise.resolve({ data: [domainA, domainC] }) + : Promise.resolve({ + data: { + id: url.endsWith('clu-2') ? 'clu-2' : 'clu-1', + organizationId: 'org-1', + status: 'RUNNING', + domainIds: url.endsWith('clu-2') ? [] : ['dom-a'], + }, + }) + ); + put.mockRejectedValue(refused(instanceNotRunning)); + const rendered = renderManagement(); + await bindC(rendered); + await rendered.findByText(shownForInstanceNotRunning); + + routeParams.clusterId = 'clu-2'; + rendered.rerenderPage(); + + await waitFor(() => expect(get).toHaveBeenCalledWith('/Cluster/clu-2')); + // The page suspends on the new cluster, and hides rather than unmounts what it showed. + await waitFor(() => expect(rendered.queryByText('loading')).toBeNull()); + expect(rendered.queryByText(shownForInstanceNotRunning)).toBeNull(); + }); + + it('sends nothing when the fresh domain list cannot be read, and says so', async () => { + let listReads = 0; + get.mockImplementation((url: string) => { + if (!url.startsWith('/Domain/')) { + return Promise.resolve({ + data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: ['dom-a'] }, + }); + } + listReads += 1; + return listReads === 1 + ? Promise.resolve({ data: [domainA, domainC] }) + : Promise.reject(new AxiosError('Network Error', AxiosError.ERR_NETWORK)); + }); + const rendered = renderManagement(); + + await bindC(rendered); + + expect(await rendered.findByText("The domain change didn't go through: Network Error")).toBeTruthy(); + expect(put).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + expect(consoleError.mock.calls.filter(([logged]: unknown[]) => logged instanceof AxiosError)).toHaveLength(1); + }); +}); diff --git a/src/features/cluster/domains/Management.tsx b/src/features/cluster/domains/Management.tsx index 7232e6c07..0fc8204d6 100644 --- a/src/features/cluster/domains/Management.tsx +++ b/src/features/cluster/domains/Management.tsx @@ -7,7 +7,7 @@ import { FormItem } from '@/components/ui/form/FormItem'; 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 { SubmitErrorMessage } from '@/features/auth/components/SubmitErrorMessage'; import { addDomainsSequentially, describeDomainFailures, @@ -23,7 +23,12 @@ import { import { bindableDomainIds } from '@/features/cluster/domains/canBindDomain'; import { useDataTableColumns } from '@/features/cluster/domains/constants/tableDefinition'; import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; -import { useIsSettingDomainIds, useSetDomainIdsOnCluster } from '@/features/clusters/mutations/setDomainIdsOnCluster'; +import { DomainChangeRefusal } from '@/features/clusters/mutations/describeDomainChangeRefusal'; +import { + DomainChangeRefusedError, + useIsSettingDomainIds, + useSetDomainIdsOnCluster, +} from '@/features/clusters/mutations/setDomainIdsOnCluster'; import { AddOrganizationDomainSchema, DOMAIN_REQUIRED_MESSAGE, @@ -35,8 +40,8 @@ import { getOrganizationDomainsQueryOptions } from '@/features/organization/quer import { useOrganizationRolePermissions } from '@/hooks/usePermissions'; import { useRefreshClick } from '@/hooks/useRefreshClick'; import { SchemaOrganizationDomain } from '@/integrations/api/api.patch'; -import { unique } from '@/lib/arrays/unique'; import { pluralize } from '@/lib/pluralize'; +import { describeError } from '@/react-query/queryClient'; import { zodResolver } from '@hookform/resolvers/zod'; import { onlineManager, useQuery, useQueryClient, useSuspenseQuery } from '@tanstack/react-query'; import { useParams } from '@tanstack/react-router'; @@ -46,6 +51,12 @@ import { useForm } from 'react-hook-form'; import { toast } from 'sonner'; import z from 'zod'; +function describeDomainChangeFailure(error: Error): DomainChangeRefusal { + return error instanceof DomainChangeRefusedError + ? { message: error.message, needsSupport: error.needsSupport } + : { message: `The domain change didn't go through: ${describeError(error).message}`, needsSupport: false }; +} + export function DomainsManagement() { const { organizationId, clusterId }: { organizationId: string; clusterId: string } = useParams({ strict: false }); const { data: cluster } = useSuspenseQuery( @@ -94,7 +105,8 @@ export function DomainsManagement() { [cluster, organizationDomains, selectedDomainIds], ); - const { mutate: setDomainIds } = useSetDomainIdsOnCluster(); + const domainChange = useSetDomainIdsOnCluster(); + const { mutate: setDomainIds } = domainChange; const settingDomainIds = useIsSettingDomainIds(); const deletingDomain = useIsDeletingDomain(); const activeQueryClient = useQueryClient(); @@ -113,14 +125,11 @@ export function DomainsManagement() { if (toBind.length === 0) { return; } - if (!isRunning(cluster.status)) { - toast.error('Cluster is currently ' + cluster.status, { description: 'To bind a domain, it must be running.' }); - return; - } setDomainIds({ clusterId: cluster.id, - domainIds: unique((cluster.domainIds?.slice() || []).concat(toBind)), + organizationId, + add: toBind, generateDomainCerts: true, }, { onSuccess: () => { @@ -131,6 +140,21 @@ export function DomainsManagement() { }); }, [activeQueryClient, cluster, organizationId, selectedDomainIds, setDomainIds]); + const onUnbindClick = useCallback((domainId: string) => { + // No `generateDomainCerts`: the flag is cluster-wide, so restating it here would decide cert + // intent for every domain that stays bound. + setDomainIds({ clusterId: cluster.id, organizationId, remove: domainId }, { + onSuccess: () => { + void activeQueryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); + toast.success('Domain unbound!'); + }, + }); + }, [activeQueryClient, cluster.id, cluster.organizationId, organizationId, setDomainIds]); + + const domainChangeFailure = domainChange.error && domainChange.variables?.clusterId === cluster.id + ? describeDomainChangeFailure(domainChange.error) + : undefined; + const { mutateAsync: addDomain } = useAddDomainToOrganization(); const form = useForm({ resolver: zodResolver(AddOrganizationDomainSchema), @@ -237,7 +261,7 @@ export function DomainsManagement() { } }, [pendingDomains]); - const dataTableColumns = useDataTableColumns(cluster, selectedDomainIds, onToggleDomainSelection); + const dataTableColumns = useDataTableColumns(cluster, selectedDomainIds, onToggleDomainSelection, onUnbindClick); const domainValue = form.watch('domain'); const isApex = useMemo(() => { @@ -259,103 +283,106 @@ export function DomainsManagement() { columns={dataTableColumns} sortingState={sortingState} > -
- {update && ( -
- - ( - - New Domain Name - - - - {isApex && ( -
- Adding an apex domain? - -
- )} - - - Type in a domain like example.com or your.example.com, and you'll be guided through validating - and binding your cluster to it. - - - - )} - /> -
- + {...field} + /> + + {isApex && ( +
+ Adding an apex domain? + +
+ )} + + + Type in a domain like example.com or your.example.com, and you'll be guided through validating + and binding your cluster to it. + + + + )} + /> +
+ - {pendingDomains.length > 0 && ( + {pendingDomains.length > 0 && ( + + )} - )} - -
- - - )} +
+ + + )} - {domainIdsToBind.length > 0 && ( -
- -
- )} + {domainIdsToBind.length > 0 && ( +
+ +
+ )} +
+ ); diff --git a/src/features/cluster/domains/constants/tableDefinition.tsx b/src/features/cluster/domains/constants/tableDefinition.tsx index f24b4882d..67c12add0 100644 --- a/src/features/cluster/domains/constants/tableDefinition.tsx +++ b/src/features/cluster/domains/constants/tableDefinition.tsx @@ -8,11 +8,7 @@ import { useChallengeCertificates, } from '@/features/cluster/domains/queries/getChallengeCertificates'; import { VerifyDomainOwnership } from '@/features/cluster/domains/VerifyDomainOwnership'; -import { - isSettingDomainIds, - useIsSettingDomainIds, - useSetDomainIdsOnCluster, -} from '@/features/clusters/mutations/setDomainIdsOnCluster'; +import { isSettingDomainIds, useIsSettingDomainIds } from '@/features/clusters/mutations/setDomainIdsOnCluster'; import { useDeleteDomain } from '@/features/organization/mutations/deleteDomain'; import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; @@ -27,6 +23,7 @@ export function useDataTableColumns( cluster: Cluster, selectedDomainIds: string[], onToggleDomainSelection: (domainId: string) => void, + onUnbindClick: (domainId: string) => void, ): Array> { const { data: challengeCertificates } = useChallengeCertificates(cluster.id); const isGeneratingCert = useMemo( @@ -34,7 +31,6 @@ export function useDataTableColumns( [challengeCertificates], ); - const { mutate: setDomainIds } = useSetDomainIdsOnCluster(); const settingDomainIds = useIsSettingDomainIds(); const { mutate: deleteDomain, isPending: deletePending } = useDeleteDomain(); const isPending = settingDomainIds || deletePending; @@ -59,20 +55,6 @@ export function useDataTableColumns( }); }, [activeQueryClient, deleteDomain, cluster.organizationId]); - const unbindDomainById = useCallback((domainId: string) => { - // No `generateDomainCerts`: the flag is cluster-wide, so restating it here would decide cert - // intent for every domain that stays bound. - setDomainIds({ - clusterId: cluster.id, - domainIds: (cluster.domainIds ?? []).filter((id) => id !== domainId), - }, { - onSuccess: () => { - void activeQueryClient.invalidateQueries({ queryKey: [cluster.organizationId], refetchType: 'active' }); - toast.success('Domain unbound!'); - }, - }); - }, [activeQueryClient, setDomainIds, cluster]); - return useMemo(() => [ columnHelper.display({ id: 'select', @@ -155,7 +137,7 @@ export function useDataTableColumns( variant="destructiveGhost" className="text-muted-foreground text-xs" disabled={isPending} - onClick={() => unbindDomainById(domain.id)} + onClick={() => onUnbindClick(domain.id)} > Unbind Domain @@ -190,7 +172,7 @@ export function useDataTableColumns( ], [ cluster, deleteDomainById, - unbindDomainById, + onUnbindClick, isPending, challengeCertificates, isGeneratingCert, diff --git a/src/features/clusters/hooks/useUpdateCluster.ts b/src/features/clusters/hooks/useUpdateCluster.ts index 4c8e1436f..12587f079 100644 --- a/src/features/clusters/hooks/useUpdateCluster.ts +++ b/src/features/clusters/hooks/useUpdateCluster.ts @@ -21,5 +21,9 @@ async function onEditClusterSubmit( export function useEditClusterMutation() { return useMutation({ mutationFn: (clusterInfo) => onEditClusterSubmit(clusterInfo), + // ClusterForm shows the failure beside its submit button, so the fading toast is off. Reported + // here rather than in the form's `onError`, which React Query skips once the form unmounts. + meta: { skipGlobalErrorToast: true }, + onError: (error) => console.error(error), }); } diff --git a/src/features/clusters/mutations/describeDomainChangeRefusal.test.ts b/src/features/clusters/mutations/describeDomainChangeRefusal.test.ts new file mode 100644 index 000000000..fd341e49b --- /dev/null +++ b/src/features/clusters/mutations/describeDomainChangeRefusal.test.ts @@ -0,0 +1,137 @@ +import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { describe, expect, it } from 'vitest'; +import { describeClusterNotChangeable, describeUnsendableDomainChange } from './describeDomainChangeRefusal'; + +function clusterWith(fields: Partial = {}): Cluster { + return { + id: 'clu-1', + organizationId: 'org-1', + status: 'RUNNING', + plans: [{ planId: 'standard' }], + ...fields, + } as Cluster; +} + +function instance(status: string) { + return { id: 'ins-1', instanceFqdn: 'node-1.example.com', status } as NonNullable[number]; +} + +describe('describeClusterNotChangeable', () => { + it('lets a running cluster whose instances are all running or gone change', () => { + expect(describeClusterNotChangeable(clusterWith())).toBeUndefined(); + expect( + describeClusterNotChangeable( + clusterWith({ instances: [instance('RUNNING'), instance('TERMINATED'), instance('REMOVED')] }), + ), + ).toBeUndefined(); + }); + + it.each(['UPDATING', 'STOPPED', 'FAILED', 'PROVISIONING', undefined])('refuses a cluster that is %s', (status) => { + expect(describeClusterNotChangeable(clusterWith({ status }))?.needsSupport).toBe(false); + }); + + // Central manager's TRANSITIONING_INSTANCE_STATES, as of central-manager 38449f3f. + it.each([ + 'PROVISIONING', + 'PENDING_UPGRADE', + 'DRAINING', + 'UPDATING', + 'UPDATED', + 'CLONE_PENDING', + 'CLONE_READY', + 'CLONING', + 'UPDATING_HDB_NODES', + 'GENERATE_TOKEN', + 'TOKEN_GENERATED', + 'STOPPING', + 'STOPPED', + 'STARTING', + 'RESTARTING', + ])('refuses while an instance is %s, naming it', (status) => { + const refusal = describeClusterNotChangeable(clusterWith({ instances: [instance(status)] })); + expect(refusal).toEqual({ + message: expect.stringContaining(`node-1.example.com is ${status}`), + needsSupport: false, + }); + }); + + it('refuses while an instance has failed, and says only support can clear it', () => { + expect(describeClusterNotChangeable(clusterWith({ instances: [instance('FAILED')] }))).toEqual({ + message: expect.stringContaining('node-1.example.com has failed'), + needsSupport: true, + }); + }); + + it('leaves a status it does not know to central manager', () => { + expect(describeClusterNotChangeable(clusterWith({ instances: [instance('ERROR')] }))).toBeUndefined(); + expect(describeClusterNotChangeable(clusterWith({ instances: [instance('SOMETHING_NEW')] }))).toBeUndefined(); + }); +}); + +describe('describeUnsendableDomainChange', () => { + const bound = { + id: 'dom-a', + domain: 'a.example.com', + status: 'ACTIVE', + clusterId: 'clu-1', + } as SchemaOrganizationDomain; + const free = { id: 'dom-c', domain: 'c.example.com', status: 'ACTIVE' } as SchemaOrganizationDomain; + const held = clusterWith({ domainIds: ['dom-a'] }); + + it('lets a change through when every domain it touches is acceptable', () => { + expect(describeUnsendableDomainChange(held, { add: ['dom-c'], remove: undefined }, [bound, free])).toBeUndefined(); + expect(describeUnsendableDomainChange(held, { add: [], remove: 'dom-a' }, [bound, free])).toBeUndefined(); + }); + + it('lets a bind adopt a domain already bound here that the cluster does not list', () => { + expect( + describeUnsendableDomainChange(clusterWith({ domainIds: [] }), { add: ['dom-a'], remove: undefined }, [bound]), + ).toBeUndefined(); + }); + + it('refuses any change to a self-managed cluster', () => { + const selfManaged = clusterWith({ domainIds: ['dom-a'], plans: [{ planId: 'self-hosted' }] as Cluster['plans'] }); + expect(describeUnsendableDomainChange(selfManaged, { add: [], remove: 'dom-a' }, [bound])?.message).toMatch( + /self-managed/, + ); + }); + + it.each([ + ['missing from the list', [free], "isn't one of this organization's domains"], + ['not verified', [{ ...bound, status: 'PENDING_VALIDATION' }, free], 'is not verified'], + ['bound to another cluster', [{ ...bound, clusterId: 'clu-2' }, free], 'is bound to cluster clu-2'], + ])('refuses every change while a domain the cluster lists is %s, pointing at support', (_case, domains, problem) => { + expect(describeUnsendableDomainChange(held, { add: ['dom-c'], remove: undefined }, domains)).toEqual({ + message: expect.stringContaining(problem), + needsSupport: true, + }); + }); + + it('refuses unbinding a listed domain another cluster holds, since central manager would unbind it there', () => { + expect( + describeUnsendableDomainChange(held, { add: [], remove: 'dom-a' }, [{ ...bound, clusterId: 'clu-2' }]), + ).toEqual({ message: expect.stringContaining('is bound to cluster clu-2'), needsSupport: true }); + }); + + it.each([ + ['missing from the list', [bound], "dom-c isn't one of this organization's domains"], + ['not verified', [bound, { ...free, status: 'PENDING_VALIDATION' }], 'c.example.com is not verified'], + ['bound to another cluster', [bound, { ...free, clusterId: 'clu-2' }], 'c.example.com is bound to cluster clu-2'], + ])('refuses binding a domain that is %s, without pointing at support', (_case, domains, problem) => { + expect(describeUnsendableDomainChange(held, { add: ['dom-c'], remove: undefined }, domains)).toEqual({ + message: expect.stringContaining(problem), + needsSupport: false, + }); + }); + + it('refuses unbinding a domain the cluster does not list', () => { + const unlisted = clusterWith({ domainIds: [] }); + expect(describeUnsendableDomainChange(unlisted, { add: [], remove: 'dom-a' }, [bound])).toEqual({ + message: expect.stringContaining('without the cluster listing it'), + needsSupport: true, + }); + expect( + describeUnsendableDomainChange(unlisted, { add: [], remove: 'dom-c' }, [free]), + ).toEqual({ message: expect.stringContaining('is no longer bound to this cluster'), needsSupport: false }); + }); +}); diff --git a/src/features/clusters/mutations/describeDomainChangeRefusal.ts b/src/features/clusters/mutations/describeDomainChangeRefusal.ts new file mode 100644 index 000000000..42ded6f18 --- /dev/null +++ b/src/features/clusters/mutations/describeDomainChangeRefusal.ts @@ -0,0 +1,123 @@ +import { Cluster, SchemaOrganizationDomain } from '@/integrations/api/api.patch'; +import { clusterIsSelfManaged } from '@/integrations/api/clusterIsSelfManaged'; + +export interface DomainChangeRefusal { + message: string; + /** Nothing on the page clears it: the cluster's own record needs fixing. */ + needsSupport: boolean; +} + +// Central manager's `assertClusterMutable` refuses every `PUT /Cluster/{id}` while an instance is +// FAILED or in `TRANSITIONING_INSTANCE_STATES` (src/lib/sharedHelpers.js, src/constants/ +// sharedConstants.js). Hand-kept and never wider than that list: a status missing here still reaches +// central manager, whose own refusal is then shown. Not `isRunning`, which passes UPDATED. +const settlingInstanceStatuses = new Set([ + 'PROVISIONING', + 'PENDING_UPGRADE', + 'DRAINING', + 'UPDATING', + 'UPDATED', + 'CLONE_PENDING', + 'CLONE_READY', + 'CLONING', + 'UPDATING_HDB_NODES', + 'GENERATE_TOKEN', + 'TOKEN_GENERATED', + 'STOPPING', + 'STOPPED', + 'STARTING', + 'RESTARTING', +]); + +export function describeClusterNotChangeable(cluster: Cluster): DomainChangeRefusal | undefined { + if (cluster.status !== 'RUNNING') { + return { + message: `This cluster is ${cluster.status ?? 'not running'}. Its domains can change once it is running again.`, + needsSupport: false, + }; + } + for (const instance of cluster.instances ?? []) { + const name = instance.instanceFqdn || instance.id; + if (instance.status === 'FAILED') { + return { + message: `Instance ${name} has failed, and this cluster's domains can't change until it is recovered.`, + needsSupport: true, + }; + } + if (instance.status && settlingInstanceStatuses.has(instance.status)) { + return { + message: + `Instance ${name} is ${instance.status}. This cluster's domains can change once every instance is running.`, + needsSupport: false, + }; + } + } + return undefined; +} + +function domainProblem( + clusterId: string, + domain: SchemaOrganizationDomain | undefined, +): string | undefined { + if (!domain) { + return "isn't one of this organization's domains"; + } + if (domain.status !== 'ACTIVE') { + return 'is not verified'; + } + if (domain.clusterId && domain.clusterId !== clusterId) { + return `is bound to cluster ${domain.clusterId}`; + } + return undefined; +} + +/** + * Checks every domain a change touches against central manager's per-domain refusals + * (updateCluster.js), the ones the cluster already holds included: central manager rejects the + * whole request over any one of them, and unbinds whatever the new list leaves out without checking + * which cluster that domain is bound to. + */ +export function describeUnsendableDomainChange( + cluster: Cluster, + { add, remove }: { add: string[]; remove: string | undefined }, + domains: SchemaOrganizationDomain[], +): DomainChangeRefusal | undefined { + if (clusterIsSelfManaged(cluster)) { + return { message: "Custom domains can't be bound to a self-managed cluster.", needsSupport: false }; + } + const byId = new Map(domains.map((domain) => [domain.id, domain])); + const held = cluster.domainIds ?? []; + for (const id of held) { + const problem = domainProblem(cluster.id, byId.get(id)); + if (problem) { + return { + message: `This cluster still lists ${byId.get(id)?.domain ?? id}, which ${problem}. ` + + "Its domains can't change until that is cleared.", + needsSupport: true, + }; + } + } + for (const id of add) { + const problem = held.includes(id) ? undefined : domainProblem(cluster.id, byId.get(id)); + if (problem) { + return { + message: `${byId.get(id)?.domain ?? id} ${problem}, so it can't be bound. The list has been refreshed.`, + needsSupport: false, + }; + } + } + if (remove && !held.includes(remove)) { + const domain = byId.get(remove); + return domain?.clusterId === cluster.id + ? { + message: + `${domain.domain} is bound to this cluster without the cluster listing it, so it can't be unbound here.`, + needsSupport: true, + } + : { + message: `${domain?.domain ?? remove} is no longer bound to this cluster. The list has been refreshed.`, + needsSupport: false, + }; + } + return undefined; +} diff --git a/src/features/clusters/mutations/setDomainIdsOnCluster.ts b/src/features/clusters/mutations/setDomainIdsOnCluster.ts index 44bd5e754..ef252df5a 100644 --- a/src/features/clusters/mutations/setDomainIdsOnCluster.ts +++ b/src/features/clusters/mutations/setDomainIdsOnCluster.ts @@ -1,24 +1,45 @@ import { apiClient } from '@/config/apiClient'; import { watchChallengeCertificates } from '@/features/cluster/domains/queries/getChallengeCertificates'; -import { getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; +import { getClusterInfo, getClusterInfoQueryOptions } from '@/features/cluster/queries/getClusterInfoQuery'; +import { + describeClusterNotChangeable, + describeUnsendableDomainChange, + DomainChangeRefusal, +} from '@/features/clusters/mutations/describeDomainChangeRefusal'; +import { + getOrganizationDomains, + getOrganizationDomainsQueryOptions, +} from '@/features/organization/queries/getOrganizationDomains'; +import { unique } from '@/lib/arrays/unique'; import { QueryClient, useIsMutating, useMutation, useQueryClient } from '@tanstack/react-query'; import { useCallback } from 'react'; interface Params { clusterId: string; - domainIds: string[]; + organizationId: string; + add?: string[]; + remove?: string; /** Omit to leave the cluster's cert intent as it is: central manager reads an absent flag as * unchanged, where `false` revokes it. */ generateDomainCerts?: boolean; } -async function setDomainIdsOnCluster({ clusterId, domainIds, generateDomainCerts }: Params) { +export class DomainChangeRefusedError extends Error { + readonly needsSupport: boolean; + + constructor({ message, needsSupport }: DomainChangeRefusal) { + super(message); + this.name = 'DomainChangeRefusedError'; + this.needsSupport = needsSupport; + } +} + +async function setDomainIdsOnCluster(clusterId: string, domainIds: string[], generateDomainCerts?: boolean) { // TODO: API does not describe this endpoint permutation - const { data } = await apiClient.put(`/Cluster/${clusterId}` as '/Cluster/{id}', { + await apiClient.put(`/Cluster/${clusterId}` as '/Cluster/{id}', { domainIds, generateDomainCerts, }, { timeout: 0 }); - return data; } const setDomainIdsMutationKey = ['setDomainIdsOnCluster'] as const; @@ -27,19 +48,42 @@ export function useSetDomainIdsOnCluster() { const queryClient = useQueryClient(); const mutation = useMutation({ mutationKey: setDomainIdsMutationKey, - mutationFn: (params: Params) => setDomainIdsOnCluster(params), - onSuccess: (_data, { clusterId, domainIds }) => { - // Every change sends the whole list, built from this cache, so it has to hold the list - // this one left even if the re-read below fails. - const { queryKey } = getClusterInfoQueryOptions(clusterId); - queryClient.setQueryData(queryKey, (cluster) => cluster && { ...cluster, domainIds }); - void queryClient.invalidateQueries({ queryKey, exact: true }); + mutationFn: async ({ clusterId, organizationId, add = [], remove, generateDomainCerts }: Params) => { + // The PUT replaces the whole list and central manager unbinds whatever it leaves out, so the + // list is built from a fresh read, not from what this page last saw: another tab or member + // may have bound a domain since. Read around the query cache, whose error handler would toast + // a failed read this mutation already reports and the page already shows. + const [cluster, domains] = await Promise.all([ + getClusterInfo(clusterId), + getOrganizationDomains(organizationId), + ]); + queryClient.setQueryData(getClusterInfoQueryOptions(clusterId).queryKey, cluster); + queryClient.setQueryData(getOrganizationDomainsQueryOptions(organizationId).queryKey, domains); + const refusal = describeClusterNotChangeable(cluster) + ?? describeUnsendableDomainChange(cluster, { add, remove }, domains); + if (refusal) { + throw new DomainChangeRefusedError(refusal); + } + const domainIds = unique((cluster.domainIds ?? []).concat(add)).filter((id) => id !== remove); + await setDomainIdsOnCluster(clusterId, domainIds, generateDomainCerts); + }, + // The page shows the failure where the change was made, so the fading toast is off. Reported + // here rather than in a caller's `onError`, which React Query skips once the page unmounts. A + // refusal is Studio declining to send, not a failure. + meta: { skipGlobalErrorToast: true }, + onError: (error) => { + if (!(error instanceof DomainChangeRefusedError)) { + console.error(error); + } + }, + onSuccess: (_data, { clusterId }) => { + void queryClient.invalidateQueries({ queryKey: getClusterInfoQueryOptions(clusterId).queryKey, exact: true }); void watchChallengeCertificates(queryClient, clusterId); }, }); const { mutate } = mutation; // `useIsSettingDomainIds` disables the controls only once React re-renders, so a click already - // queued behind the first change would otherwise build its list from the same stale cache. + // queued behind the first change would otherwise start a second one alongside it. const mutateWhenIdle = useCallback((variables, options) => { if (!isSettingDomainIds(queryClient)) { mutate(variables, options); diff --git a/src/features/clusters/upsert/ClusterBilling.tsx b/src/features/clusters/upsert/ClusterBilling.tsx index 8080f949d..3ea712057 100644 --- a/src/features/clusters/upsert/ClusterBilling.tsx +++ b/src/features/clusters/upsert/ClusterBilling.tsx @@ -1,5 +1,6 @@ import { ContactUs } from '@/components/ContactUs'; import { Button } from '@/components/ui/button'; +import { SubmitErrorMessage } from '@/features/auth/components/SubmitErrorMessage'; import { PaymentMethodsDisplay } from '@/features/organization/billing/paymentMethod/PaymentMethodsDisplay'; import { getOrganizationQueryOptions } from '@/features/organization/queries/getOrganizationQuery'; import { SchemaPlan } from '@/integrations/api/api.gen'; @@ -19,6 +20,7 @@ interface ClusterBillingProps { readonly onSubmit?: () => void; readonly organizationId: string; readonly selectedPlan: SchemaPlan | undefined; + readonly submitError?: string; } export function ClusterBilling({ @@ -29,6 +31,7 @@ export function ClusterBilling({ onSubmit, organizationId, selectedPlan, + submitError, }: ClusterBillingProps) { const { data: organization } = useQuery(getOrganizationQueryOptions(organizationId)); const billing = organization?.billing; @@ -42,6 +45,11 @@ export function ClusterBilling({ const footer = ( <> + {submitError && ( +
+ +
+ )}
- - - - {domain.clusterId - ? 'To delete this domain, please unbind it from all clusters first.' - : 'Remember to clean up your DNS records!'} - - + <> +
{domain.domain}
+ + {/* A disabled button takes no pointer events, so the hover has to land on a wrapper. */} + +
+ +
+ + + {domain.clusterId + ? 'To delete this domain, please unbind it from all clusters first.' + : 'Remember to clean up your DNS records!'} + + + ); }, }), From 3fdf378f7fc69a1f73e88dcc4af34a48f0145389 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:28:09 -0400 Subject: [PATCH 7/8] fix(domains): a disabled Remove's wrapper takes the tab stop, so keyboard users get the tooltip A disabled button can't take focus, so the explanation for why Remove is off was hover-only. The wrapper the tooltip triggers on is now focusable exactly while the button is disabled; while it is enabled the button keeps the only tab stop and its focus still opens the tooltip. Co-Authored-By: Claude Opus 5.5 --- .../cluster/domains/Management.test.tsx | 42 +++++++++++++++---- .../domains/constants/tableDefinition.tsx | 7 ++-- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/features/cluster/domains/Management.test.tsx b/src/features/cluster/domains/Management.test.tsx index 761da4245..8bedd310e 100644 --- a/src/features/cluster/domains/Management.test.tsx +++ b/src/features/cluster/domains/Management.test.tsx @@ -2,7 +2,7 @@ * @vitest-environment jsdom */ import { MutationCache, onlineManager, QueryCache, QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { fireEvent, render, waitFor, within } from '@testing-library/react'; +import { act, fireEvent, render, waitFor, within } from '@testing-library/react'; import { AxiosError } from 'axios'; import { PropsWithChildren, Suspense } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -1039,10 +1039,10 @@ describe('DomainsManagement — a domain change central manager would refuse', ( }); describe('DomainsManagement — the Remove Domain tooltip', () => { - it.each([ - { state: 'bound', clusterId: 'clu-1', tip: 'To delete this domain, please unbind it from all clusters first.' }, - { state: 'unbound', clusterId: undefined, tip: 'Remember to clean up your DNS records!' }, - ])('opens over a $state domain', async ({ clusterId, tip }) => { + const boundTip = 'To delete this domain, please unbind it from all clusters first.'; + const unboundTip = 'Remember to clean up your DNS records!'; + + async function renderRemove(clusterId: string | undefined) { const domain = { id: 'dom-a', domain: 'a.example.com', status: 'ACTIVE', organizationId: 'org-1', clusterId }; get.mockImplementation((url: string) => url.startsWith('/Domain/') @@ -1051,9 +1051,17 @@ describe('DomainsManagement — the Remove Domain tooltip', () => { data: { id: 'clu-1', organizationId: 'org-1', status: 'RUNNING', domainIds: clusterId ? ['dom-a'] : [] }, }) ); - const { findByText, findByRole, queryByRole } = renderManagement(); - const name = await findByText('a.example.com'); + const rendered = renderManagement(); + const name = await rendered.findByText('a.example.com'); const remove = within(name.closest('tr')!).getByRole('button', { name: 'Remove Domain' }) as HTMLButtonElement; + return { ...rendered, name, remove }; + } + + it.each([ + { state: 'bound', clusterId: 'clu-1', tip: boundTip }, + { state: 'unbound', clusterId: undefined, tip: unboundTip }, + ])('opens over a $state domain', async ({ clusterId, tip }) => { + const { name, remove, findByRole, queryByRole } = await renderRemove(clusterId); expect(remove.disabled).toBe(!!clusterId); fireEvent.pointerMove(name); @@ -1063,4 +1071,24 @@ describe('DomainsManagement — the Remove Domain tooltip', () => { fireEvent.pointerMove(remove.parentElement!); expect((await findByRole('tooltip')).textContent).toBe(tip); }); + + it('opens on keyboard focus while Remove is disabled', async () => { + const { remove, findByRole } = await renderRemove('clu-1'); + const wrapper = remove.parentElement!; + + act(() => wrapper.focus()); + + expect(document.activeElement).toBe(wrapper); + expect((await findByRole('tooltip')).textContent).toBe(boundTip); + }); + + it('adds no second tab stop while Remove is enabled, and still opens when it takes focus', async () => { + const { remove, findByRole } = await renderRemove(undefined); + expect(remove.parentElement!.hasAttribute('tabindex')).toBe(false); + + act(() => remove.focus()); + + expect(document.activeElement).toBe(remove); + expect((await findByRole('tooltip')).textContent).toBe(unboundTip); + }); }); diff --git a/src/features/cluster/domains/constants/tableDefinition.tsx b/src/features/cluster/domains/constants/tableDefinition.tsx index 935729514..b86b3d5cf 100644 --- a/src/features/cluster/domains/constants/tableDefinition.tsx +++ b/src/features/cluster/domains/constants/tableDefinition.tsx @@ -84,18 +84,19 @@ export function useDataTableColumns( size: 150, cell: (props) => { const domain = props.row.original; + const removeDisabled = isPending || !!domain.clusterId; return ( <>
{domain.domain}
- {/* A disabled button takes no pointer events, so the hover has to land on a wrapper. */} + {/* A disabled button takes no pointer events or focus, so hover and Tab land on a wrapper. */} -
+
+ ); return ( <>
{domain.domain}
- {/* A disabled button takes no pointer events or focus, so hover and Tab land on a wrapper. */} + {/* A disabled button takes no pointer events or focus, so a bound domain's reason hangs on a wrapper. */} -
- -
+ {domain.clusterId ?
{removeButton}
: removeButton}