From 88a4ff656adfda09a81dcdb0391868de5735a304 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 01:26:51 -0400 Subject: [PATCH 1/2] fix(clusters): explain the two saves a region swap takes before the update fails Central manager refuses a cluster update that leaves every region the cluster runs in while adding another ("Cannot delete all region plans from a cluster and add new region plan in the same update"). Changing an existing cluster's only region went all the way through payment review and then failed with that error. Flag that selection on an edit, under the first region, with the steps: keep the current region, add the new one and save, then remove the old one. A tier that allows a single region cannot do that, so it says the cluster cannot move in place instead. Submitting stays blocked until a current region is kept. Refs #1311 Co-Authored-By: Claude Opus 5.5 --- src/features/clusters/DESIGN.md | 6 ++ .../clusters/upsert/ClusterForm.test.tsx | 74 ++++++++++++++++++- src/features/clusters/upsert/ClusterForm.tsx | 13 ++++ src/features/clusters/upsert/index.tsx | 12 +++ .../upsert/lib/describeRegionSwap.test.ts | 40 ++++++++++ .../clusters/upsert/lib/describeRegionSwap.ts | 26 +++++++ 6 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 src/features/clusters/upsert/lib/describeRegionSwap.test.ts create mode 100644 src/features/clusters/upsert/lib/describeRegionSwap.ts diff --git a/src/features/clusters/DESIGN.md b/src/features/clusters/DESIGN.md index d764a3820..ecea1d316 100644 --- a/src/features/clusters/DESIGN.md +++ b/src/features/clusters/DESIGN.md @@ -18,3 +18,9 @@ the cluster it was editing. A tier with `allowedRegionIds` takes one region, and the form corrects that region whenever it is the only one left, which includes the moment a second region is removed. The correction keeps the chosen region and moves only its latency when the tier offers that region at all, and falls back to the default region only when it does not (#1275). + +Central manager refuses a cluster update that leaves every region the cluster runs in while adding another, matching +regions by name, so moving a cluster takes two saves. `lib/describeRegionSwap.ts` blocks that selection on an edit with +the steps instead of letting the update fail (#1311). It mirrors only the case the server is certain to refuse, so it +can never block an update the server would accept. The server also counts shrinking a kept region as leaving it; a +selection that shrinks every current region while adding one is not mirrored and still gets the server's error. diff --git a/src/features/clusters/upsert/ClusterForm.test.tsx b/src/features/clusters/upsert/ClusterForm.test.tsx index 9da2c6263..d2121d738 100644 --- a/src/features/clusters/upsert/ClusterForm.test.tsx +++ b/src/features/clusters/upsert/ClusterForm.test.tsx @@ -48,12 +48,17 @@ beforeAll(() => { }); afterEach(cleanup); -function renderForm(defaultValues: Partial) { +function renderForm( + defaultValues: Partial, + { clusterId, currentRegionNames = [] }: { clusterId?: string; currentRegionNames?: string[] } = {}, +) { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); return render( { expect(selectedRegionPlans(container)).toEqual(['US / 90ms, small distribution']); }); }); + +async function pickOption(currentText: string, option: string) { + const trigger = screen.getAllByRole('combobox').find(combobox => combobox.textContent === currentText); + if (!trigger) { + throw new Error(`No select is showing ${currentText}`); + } + await act(async () => { + fireEvent.click(trigger); + }); + await act(async () => { + fireEvent.click(screen.getByRole('option', { name: option })); + }); +} + +const submitButton = () => + screen.getByRole('button', { name: /Confirm Payment Details|Edit Cluster/ }); +const swapGuidance = () => screen.queryByText(/can't move out of all its current regions/); + +describe('ClusterForm editing regions', () => { + const editing = { clusterId: 'clu-a', currentRegionNames: ['US'] }; + const runningInUs = { regionPlans: [{ regionName: 'US', latencyDescription: '90ms, small distribution' }] }; + + it('explains the two saves a region swap takes instead of letting the update fail (#1311)', async () => { + const { container } = renderForm(runningInUs, editing); + + await pickOption('US', 'Europe'); + + expect(selectedRegionPlans(container)).toEqual(['Europe / 60ms, small distribution']); + expect(swapGuidance()?.textContent).toBe( + "A cluster can't move out of all its current regions in one update. Keep US, add Europe as an additional region and save, then remove US once that update has finished.", + ); + expect(submitButton().disabled).toBe(true); + }); + + it('clears the guidance once a current region is kept alongside the new one', async () => { + renderForm(runningInUs, editing); + await pickOption('US', 'Europe'); + expect(swapGuidance()).not.toBeNull(); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Add Additional Region Usage' })); + }); + await pickOption('Asia', 'US'); + + expect(swapGuidance()).toBeNull(); + expect(submitButton().disabled).toBe(false); + }); + + it('says a single-region tier cannot move in place', async () => { + renderForm({ ...runningInUs, performanceDescription: 'Free tier' }, editing); + + await pickOption('US', 'Europe'); + + expect(swapGuidance()?.textContent).toBe( + "A cluster can't move out of all its current regions in one update, and this tier allows only one region, so it can't move from US to Europe. Create a new cluster in Europe instead.", + ); + }); + + it('never applies to a new cluster', async () => { + const { container } = renderForm(runningInUs); + + await pickOption('US', 'Europe'); + + expect(selectedRegionPlans(container)).toEqual(['Europe / 60ms, small distribution']); + expect(swapGuidance()).toBeNull(); + }); +}); diff --git a/src/features/clusters/upsert/ClusterForm.tsx b/src/features/clusters/upsert/ClusterForm.tsx index 225cd7369..de71542de 100644 --- a/src/features/clusters/upsert/ClusterForm.tsx +++ b/src/features/clusters/upsert/ClusterForm.tsx @@ -31,6 +31,7 @@ import { z } from 'zod'; import { ClusterBilling } from './ClusterBilling'; import { ClusterDetails } from './ClusterDetails'; import { calculateInstanceFQDN } from './lib/calculateInstanceFQDN'; +import { describeRegionSwap } from './lib/describeRegionSwap'; import { PartialUpgrade } from './lib/detectPartialUpgrade'; import { pickDefaultDeploymentPerformanceAndRegionPlans } from './lib/pickDefaultDeploymentPerformanceAndRegionPlans'; import { PriceDisplay } from './PriceDisplay'; @@ -39,6 +40,8 @@ import { specifiedAbbreviatedName, UpsertClusterSchema, UpsertClusterSchemaType interface ClusterFormProps { alreadyUsingFree: boolean; clusterId?: string; + /** Names of the regions the cluster being edited runs in now; empty when creating. */ + currentRegionNames: readonly string[]; defaultValues: UpsertClusterSchemaType; deploymentToPerformanceToPlan: Record>; harperVersions: HarperVersionsResponse | undefined; @@ -56,6 +59,7 @@ interface ClusterFormProps { export function ClusterForm({ alreadyUsingFree, clusterId, + currentRegionNames, defaultValues, deploymentToPerformanceToPlan, harperVersions, @@ -165,10 +169,19 @@ export function ClusterForm({ } } } + const regionSwap = describeRegionSwap({ + currentRegionNames, + selectedRegionNames: data.regionPlans.map(regionPlan => regionPlan.regionName), + singleRegionTier: !selectedPlan?.priceUsd || !!selectedPlan.allowedRegionIds?.length, + }); + if (regionSwap) { + ctx.addIssue({ code: 'custom', path: ['regionPlans.0.regionName'], message: regionSwap }); + } } }, [ alreadyUsingFree, colocatedRegionNameToLatencyToRegion, + currentRegionNames, dedicatedRegionNameToLatencyToRegion, deploymentToPerformanceToPlan, ]); diff --git a/src/features/clusters/upsert/index.tsx b/src/features/clusters/upsert/index.tsx index e36ddabe4..f85f176bc 100644 --- a/src/features/clusters/upsert/index.tsx +++ b/src/features/clusters/upsert/index.tsx @@ -118,6 +118,17 @@ export function UpsertCluster() { return false; }, [cluster?.id, organization?.clusters, planTypes]); + const currentRegionNames = useMemo(() => { + if (!clusterId) { + return []; + } + const catalog = [...(regionLocationsColocated ?? []), ...(regionLocationsDedicated ?? [])]; + return (cluster?.plans ?? []).flatMap(plan => { + const name = catalog.find(region => region.id === plan.regionId)?.region ?? plan.region; + return name ? [name] : []; + }); + }, [cluster, clusterId, regionLocationsColocated, regionLocationsDedicated]); + const deploymentToPerformanceToPlan = useMemo>>( () => groupThenKeyBy(planTypes?.sort(sortByField('priceUsd')) || [], 'deploymentDescription', 'performanceDescription'), @@ -277,6 +288,7 @@ export function UpsertCluster() { + describeRegionSwap({ currentRegionNames, selectedRegionNames, singleRegionTier }); + +describe('describeRegionSwap', () => { + it('stays quiet for a new cluster, which has no current regions', () => { + expect(swap([], ['Europe'])).toBeNull(); + }); + + it('stays quiet while any current region is kept', () => { + expect(swap(['US'], ['US'])).toBeNull(); + expect(swap(['US'], ['Europe', 'US'])).toBeNull(); + expect(swap(['US', 'Asia'], ['Asia', 'Europe'])).toBeNull(); + }); + + it('stays quiet until a region is actually chosen', () => { + expect(swap(['US'], [])).toBeNull(); + expect(swap(['US'], [''])).toBeNull(); + }); + + it('names the region to keep and the one to add', () => { + expect(swap(['US'], ['', 'Europe'])).toBe( + "A cluster can't move out of all its current regions in one update. Keep US, add Europe as an additional region and save, then remove US once that update has finished.", + ); + }); + + it('asks to keep one of several current regions', () => { + expect(swap(['US', 'Asia', 'US'], ['Europe', 'Canada'])).toBe( + "A cluster can't move out of all its current regions in one update. Keep one of US and Asia, add Europe as an additional region and save, then remove the others once that update has finished.", + ); + }); + + it('does not suggest a second region on a tier that allows only one', () => { + expect(swap(['US'], ['Europe'], true)).toBe( + "A cluster can't move out of all its current regions in one update, and this tier allows only one region, so it can't move from US to Europe. Create a new cluster in Europe instead.", + ); + }); +}); diff --git a/src/features/clusters/upsert/lib/describeRegionSwap.ts b/src/features/clusters/upsert/lib/describeRegionSwap.ts new file mode 100644 index 000000000..c96806f01 --- /dev/null +++ b/src/features/clusters/upsert/lib/describeRegionSwap.ts @@ -0,0 +1,26 @@ +const regionList = new Intl.ListFormat('en', { style: 'long', type: 'conjunction' }); + +/** + * Central manager refuses an update that leaves every region a cluster runs in while adding a new one, matching + * regions by name, so moving a cluster takes two saves. Returns guidance for a selection it would refuse, or null. + */ +export function describeRegionSwap({ currentRegionNames, selectedRegionNames, singleRegionTier }: { + currentRegionNames: readonly string[]; + selectedRegionNames: readonly string[]; + singleRegionTier: boolean; +}): string | null { + const current = [...new Set(currentRegionNames.filter(Boolean))]; + const selected = selectedRegionNames.filter(Boolean); + if (!current.length || !selected.length || selected.some(name => current.includes(name))) { + return null; + } + const from = regionList.format(current); + const to = selected[0]; + const rule = "A cluster can't move out of all its current regions in one update"; + if (singleRegionTier) { + return `${rule}, and this tier allows only one region, so it can't move from ${from} to ${to}. Create a new cluster in ${to} instead.`; + } + return current.length === 1 + ? `${rule}. Keep ${from}, add ${to} as an additional region and save, then remove ${from} once that update has finished.` + : `${rule}. Keep one of ${from}, add ${to} as an additional region and save, then remove the others once that update has finished.`; +} From c7381f36f56102be74651ae06edd9cd7da9d6211 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 01:32:47 -0400 Subject: [PATCH 2/2] fix(clusters): say which regions to remove after a multi-region swap With several current regions the guidance said "remove the others" after the first save, but the regions left by then are the ones the user kept. Name those instead, skip the work entirely when creating, and test how a cluster's plans resolve to the region names the check compares. Refs #1311 Co-Authored-By: Claude Opus 5.5 --- src/features/clusters/upsert/index.tsx | 21 ++++++++-------- .../upsert/lib/describeRegionSwap.test.ts | 25 ++++++++++++++++--- .../clusters/upsert/lib/describeRegionSwap.ts | 14 ++++++++++- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/features/clusters/upsert/index.tsx b/src/features/clusters/upsert/index.tsx index f85f176bc..83d71401f 100644 --- a/src/features/clusters/upsert/index.tsx +++ b/src/features/clusters/upsert/index.tsx @@ -27,6 +27,7 @@ import { buildUpgradeVersionOptions } from './lib/buildUpgradeVersionOptions'; import { calculateDefaultDeploymentPerformanceAndRegionPlans, } from './lib/calculateDefaultDeploymentPerformanceAndRegionPlans'; +import { regionNamesOfPlans } from './lib/describeRegionSwap'; import { detectPartialUpgrade } from './lib/detectPartialUpgrade'; import { UpsertClusterSchema, UpsertClusterSchemaType } from './upsertClusterSchema'; @@ -118,16 +119,16 @@ export function UpsertCluster() { return false; }, [cluster?.id, organization?.clusters, planTypes]); - const currentRegionNames = useMemo(() => { - if (!clusterId) { - return []; - } - const catalog = [...(regionLocationsColocated ?? []), ...(regionLocationsDedicated ?? [])]; - return (cluster?.plans ?? []).flatMap(plan => { - const name = catalog.find(region => region.id === plan.regionId)?.region ?? plan.region; - return name ? [name] : []; - }); - }, [cluster, clusterId, regionLocationsColocated, regionLocationsDedicated]); + const currentRegionNames = useMemo( + () => + clusterId + ? regionNamesOfPlans(cluster?.plans ?? [], [ + ...(regionLocationsColocated ?? []), + ...(regionLocationsDedicated ?? []), + ]) + : [], + [cluster, clusterId, regionLocationsColocated, regionLocationsDedicated], + ); const deploymentToPerformanceToPlan = useMemo>>( () => diff --git a/src/features/clusters/upsert/lib/describeRegionSwap.test.ts b/src/features/clusters/upsert/lib/describeRegionSwap.test.ts index 7e97b4663..67ad1f055 100644 --- a/src/features/clusters/upsert/lib/describeRegionSwap.test.ts +++ b/src/features/clusters/upsert/lib/describeRegionSwap.test.ts @@ -1,9 +1,28 @@ +import type { SchemaRegion } from '@/integrations/api/api.gen'; import { describe, expect, it } from 'vitest'; -import { describeRegionSwap } from './describeRegionSwap'; +import { describeRegionSwap, regionNamesOfPlans } from './describeRegionSwap'; const swap = (currentRegionNames: string[], selectedRegionNames: string[], singleRegionTier = false) => describeRegionSwap({ currentRegionNames, selectedRegionNames, singleRegionTier }); +describe('regionNamesOfPlans', () => { + const catalog = [ + { id: 'us-1', region: 'US', latencyDescription: '83ms', instanceCount: 2 }, + { id: 'eu-1', region: 'Europe', latencyDescription: '58ms', instanceCount: 2 }, + ] as SchemaRegion[]; + + it('names each plan by its catalog region, falling back to the name the plan carries', () => { + expect(regionNamesOfPlans([ + { planId: 'shared', regionId: 'eu-1' }, + { planId: 'shared', regionId: 'retired-1', region: 'Asia' }, + ], catalog)).toEqual(['Europe', 'Asia']); + }); + + it('skips plans with no region, such as self-hosted instances', () => { + expect(regionNamesOfPlans([{ planId: 'self-hosted-0' }], catalog)).toEqual([]); + }); +}); + describe('describeRegionSwap', () => { it('stays quiet for a new cluster, which has no current regions', () => { expect(swap([], ['Europe'])).toBeNull(); @@ -26,9 +45,9 @@ describe('describeRegionSwap', () => { ); }); - it('asks to keep one of several current regions', () => { + it('asks to keep at least one of several current regions', () => { expect(swap(['US', 'Asia', 'US'], ['Europe', 'Canada'])).toBe( - "A cluster can't move out of all its current regions in one update. Keep one of US and Asia, add Europe as an additional region and save, then remove the others once that update has finished.", + "A cluster can't move out of all its current regions in one update. Keep at least one of US and Asia, add Europe as an additional region and save, then remove the old regions that are left once that update has finished.", ); }); diff --git a/src/features/clusters/upsert/lib/describeRegionSwap.ts b/src/features/clusters/upsert/lib/describeRegionSwap.ts index c96806f01..c0fbce98e 100644 --- a/src/features/clusters/upsert/lib/describeRegionSwap.ts +++ b/src/features/clusters/upsert/lib/describeRegionSwap.ts @@ -1,5 +1,14 @@ +import { SchemaRegion, SchemaRegionPlan } from '@/integrations/api/api.gen'; + const regionList = new Intl.ListFormat('en', { style: 'long', type: 'conjunction' }); +export function regionNamesOfPlans(plans: readonly SchemaRegionPlan[], catalog: readonly SchemaRegion[]): string[] { + return plans.flatMap(plan => { + const name = catalog.find(region => region.id === plan.regionId)?.region ?? plan.region; + return name ? [name] : []; + }); +} + /** * Central manager refuses an update that leaves every region a cluster runs in while adding a new one, matching * regions by name, so moving a cluster takes two saves. Returns guidance for a selection it would refuse, or null. @@ -9,6 +18,9 @@ export function describeRegionSwap({ currentRegionNames, selectedRegionNames, si selectedRegionNames: readonly string[]; singleRegionTier: boolean; }): string | null { + if (!currentRegionNames.length) { + return null; + } const current = [...new Set(currentRegionNames.filter(Boolean))]; const selected = selectedRegionNames.filter(Boolean); if (!current.length || !selected.length || selected.some(name => current.includes(name))) { @@ -22,5 +34,5 @@ export function describeRegionSwap({ currentRegionNames, selectedRegionNames, si } return current.length === 1 ? `${rule}. Keep ${from}, add ${to} as an additional region and save, then remove ${from} once that update has finished.` - : `${rule}. Keep one of ${from}, add ${to} as an additional region and save, then remove the others once that update has finished.`; + : `${rule}. Keep at least one of ${from}, add ${to} as an additional region and save, then remove the old regions that are left once that update has finished.`; }