From bc773b4e9ef56f6fa89590a2c09d3bc06c3bc04f Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 01:04:02 -0400 Subject: [PATCH 1/4] 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 3/4] 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}