diff --git a/DESIGN.md b/DESIGN.md index 5eb0a2a97..850025bb9 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, and why a table without a primary key can't be listed. 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; +}