Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/features/clusters/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
74 changes: 73 additions & 1 deletion src/features/clusters/upsert/ClusterForm.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,17 @@ beforeAll(() => {
});
afterEach(cleanup);

function renderForm(defaultValues: Partial<UpsertClusterSchemaType>) {
function renderForm(
defaultValues: Partial<UpsertClusterSchemaType>,
{ clusterId, currentRegionNames = [] }: { clusterId?: string; currentRegionNames?: string[] } = {},
) {
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
return render(
<QueryClientProvider client={queryClient}>
<ClusterForm
alreadyUsingFree={false}
clusterId={clusterId}
currentRegionNames={currentRegionNames}
defaultValues={{
clusterName: 'Preview',
abbreviatedName: '',
Expand Down Expand Up @@ -175,3 +180,70 @@ describe('ClusterForm regions', () => {
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<HTMLButtonElement>('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();
});
});
13 changes: 13 additions & 0 deletions src/features/clusters/upsert/ClusterForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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<string, Record<string, SchemaPlan>>;
harperVersions: HarperVersionsResponse | undefined;
Expand All @@ -56,6 +59,7 @@ interface ClusterFormProps {
export function ClusterForm({
alreadyUsingFree,
clusterId,
currentRegionNames,
defaultValues,
deploymentToPerformanceToPlan,
harperVersions,
Expand Down Expand Up @@ -165,10 +169,19 @@ export function ClusterForm({
}
}
}
const regionSwap = describeRegionSwap({
currentRegionNames,
selectedRegionNames: data.regionPlans.map(regionPlan => regionPlan.regionName),
singleRegionTier: !selectedPlan?.priceUsd || !!selectedPlan.allowedRegionIds?.length,
});
Comment on lines +172 to +176

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When selectedPlan is undefined (e.g., during initial load or if the selected deployment/performance descriptions are temporarily invalid), !selectedPlan?.priceUsd evaluates to true. This incorrectly flags the plan as a singleRegionTier, which can display a misleading "single-region tier" error message to the user instead of a generic or no error message.

We should explicitly guard this check to ensure selectedPlan is defined before determining if it is a single-region tier.

Suggested change
const regionSwap = describeRegionSwap({
currentRegionNames,
selectedRegionNames: data.regionPlans.map(regionPlan => regionPlan.regionName),
singleRegionTier: !selectedPlan?.priceUsd || !!selectedPlan.allowedRegionIds?.length,
});
const regionSwap = describeRegionSwap({
currentRegionNames,
selectedRegionNames: data.regionPlans.map(regionPlan => regionPlan.regionName),
singleRegionTier: !!selectedPlan && (!selectedPlan.priceUsd || !!selectedPlan.allowedRegionIds?.length),
});

if (regionSwap) {
ctx.addIssue({ code: 'custom', path: ['regionPlans.0.regionName'], message: regionSwap });
}
}
}, [
alreadyUsingFree,
colocatedRegionNameToLatencyToRegion,
currentRegionNames,
dedicatedRegionNameToLatencyToRegion,
deploymentToPerformanceToPlan,
]);
Expand Down
13 changes: 13 additions & 0 deletions src/features/clusters/upsert/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -118,6 +119,17 @@ export function UpsertCluster() {
return false;
}, [cluster?.id, organization?.clusters, planTypes]);

const currentRegionNames = useMemo(
() =>
clusterId
? regionNamesOfPlans(cluster?.plans ?? [], [
...(regionLocationsColocated ?? []),
...(regionLocationsDedicated ?? []),
])
: [],
[cluster, clusterId, regionLocationsColocated, regionLocationsDedicated],
);

const deploymentToPerformanceToPlan = useMemo<Record<string, Record<string, SchemaPlan>>>(
() =>
groupThenKeyBy(planTypes?.sort(sortByField('priceUsd')) || [], 'deploymentDescription', 'performanceDescription'),
Expand Down Expand Up @@ -277,6 +289,7 @@ export function UpsertCluster() {
<ClusterForm
alreadyUsingFree={alreadyUsingFree}
clusterId={clusterId}
currentRegionNames={currentRegionNames}
defaultValues={defaultValues}
deploymentToPerformanceToPlan={deploymentToPerformanceToPlan}
harperVersions={harperVersions}
Expand Down
59 changes: 59 additions & 0 deletions src/features/clusters/upsert/lib/describeRegionSwap.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import type { SchemaRegion } from '@/integrations/api/api.gen';
import { describe, expect, it } from 'vitest';
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();
});

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 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 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.",
);
});

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.",
);
});
});
38 changes: 38 additions & 0 deletions src/features/clusters/upsert/lib/describeRegionSwap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
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.
*/
export function describeRegionSwap({ currentRegionNames, selectedRegionNames, singleRegionTier }: {
currentRegionNames: readonly string[];
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))) {
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 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.`;
}
Loading