Bind on the Domains page sends only the domains still offered for binding

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, and why a table without a primary key can't be listed.
 

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)).

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<HTMLElement>,
+			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']);

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,18 +90,38 @@ 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;
 		}
 
 		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() {
 					</Form>
 				)}
 
-				{selectedDomainIds.length > 0 && (
+				{domainIdsToBind.length > 0 && (
 					<div className="flex-0 self-start md:pt-6.5">
 						<Button
 							variant="submit"
 							onClick={onBindClick}
-							disabled={settingDomainIds}
+							disabled={settingDomainIds || deletingDomain}
 						>
-							<Save /> Bind {pluralize(selectedDomainIds.length, 'Domain', 'Domains')}
+							<Save /> Bind {pluralize(domainIdsToBind.length, 'Domain', 'Domains')}
 						</Button>
 					</div>
 				)}

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([]);
+	});
+});

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))
+	);
+}

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 (