Repository navigation
fix(clusters): explain the two saves a region change takes instead of failing the update - #1849
dawsontoth wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces client-side validation and guidance to prevent users from attempting to move a cluster out of all its current regions in a single update, which is rejected by the central manager. It adds helper functions in describeRegionSwap.ts to detect invalid region swaps and display helpful guidance messages, integrates this validation into ClusterForm, and includes comprehensive tests. Feedback on the pull request points out a potential bug where an undefined selectedPlan could incorrectly flag a plan as a single-region tier, and suggests a fix to explicitly guard the check.
| const regionSwap = describeRegionSwap({ | ||
| currentRegionNames, | ||
| selectedRegionNames: data.regionPlans.map(regionPlan => regionPlan.regionName), | ||
| singleRegionTier: !selectedPlan?.priceUsd || !!selectedPlan.allowedRegionIds?.length, | ||
| }); |
There was a problem hiding this comment.
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.
| 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), | |
| }); |
There was a problem hiding this comment.
Declining. I applied the suggestion first and it made the guidance worse, so I took it back out.
When the selected tier isn't in the catalog, selectedPlan is undefined, so totalPrice is 0 (ClusterForm.tsx:301). At 0, ClusterRegions hides Add Additional Region Usage (ClusterRegions.tsx:47, "Free plans can only add a single region"). The form therefore already treats an unknown tier as single-region. With the suggestion, the message would say "add Europe as an additional region and save", and the form shows no control to do that. The current predicate matches the form, and a16e5c5 adds a test that pins the message and the missing button together.
A plan that is undefined only while data loads doesn't reach this message. The guidance appears only when the selection drops every current region, and an edit opens on the cluster's current regions.
🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
…pdate 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.
Regions compare the way central manager matches them: a catalog region by name, whatever its latency tier, and a custom region by id (regionCohortKey), since its name is free text.
Refs #1311
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 regions the check compares: a catalog plan by its region name, a custom region by its id. Refs #1311 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rom the catalog With no plan for the selected tier the price is 0, so the form hides Add Additional Region Usage. The guidance has to say to create a new cluster, not to add a region the form won't let the user add. Refs #1311 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a16e5c5 to
74b524a
Compare
With Europe and Canada chosen to replace US, the steps said to add Europe and left Canada out, so following them to the letter dropped a region. Name each region being added, and drop two comments that only restated the code. Refs #1311 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
⊙ Problem
Changing the region of an existing cluster fails at the API with "Cannot delete all region plans from a cluster and add new region plan in the same update" (#1311). Central manager refuses any single update that leaves every current region while adding a new one, so a move takes two saves: add the new region and save, then remove the old one. The form gave no hint of that: a user who switched their only region from US to Europe went through payment review and then got the raw API error.
Reproduced on
stagein a real browser before this rebase (Playwright, local dev server, mocked API): on/#/<org>/<cluster>/editwith the cluster in US, choosing Europe left the submit button enabled with no message, and submitting sentregionPlans: [{ regionId: 'eu-1', … }], the exact update the server refuses. #1780 does not change that path.Stack: #1780 ← #1842 ← this PR ← #1851 ← #1870. Based on #1842's branch (
claude/1275-region-removal-keeps-siblings); retarget as the PRs below it merge.💡 Solution
On an edit, the form now compares the regions selected with the regions the cluster runs in now. When none of the current regions is kept, the first region field shows the steps, for example "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.", and submitting stays blocked until a current region is kept. The check mirrors only the case the server is certain to refuse, so it never blocks an update the server would accept.
Regions compare the way central manager matches them. A catalog region matches by name, whatever its latency tier. A custom region (#1780's
oreg-…) matches by id, since its name is free text: a custom region named "US" does not keep a cluster in the catalog's US. That is #1780'sregionCohortKey, and it is how central-manager#892's update decides which current plans are deleted (src/resources/cluster/updateCluster.js:399-404) before refusing a delete-all-and-add update (:436-441). Before the rebase the check compared region names, which #1780's id-keyed form no longer holds.🔧 Changes
src/features/clusters/upsert/lib/describeRegionSwap.ts(new) —RegionCohortis a region as central manager matches it (a key plus the name to show).regionCohortsOfPlansresolves a cluster's plans through feat(clusters): org-owned custom regions — picker group and units on the cluster form #1780's region lookup, falling back to what a plan carries when the lookup lacks it.describeRegionSwapreturns the guidance, or null when any current region is kept, when nothing is chosen yet, or when there are no current regions (creating). It names every region being added and has a separate message for a single-region tier.src/features/clusters/upsert/ClusterForm.tsx— a newcurrentRegionsprop. feat(clusters): org-owned custom regions — picker group and units on the cluster form #1780's refinement loop already resolves each selected id and computes its cohort for the duplicate check; it now also collects them, and the refinement adds the guidance as an issue onregionPlans.0.regionIdfor hosted clusters. feat(clusters): org-owned custom regions — picker group and units on the cluster form #1780's early return for a version-only edit covers this check too.src/features/clusters/upsert/index.tsx— derivescurrentRegionsfrom the edited cluster's server plans (not the form draft, which a billing redirect can restore), through a lookup over both catalogs and feat(clusters): org-owned custom regions — picker group and units on the cluster form #1780'sorganizationRegions, and passes it in; it is empty when creating.src/features/clusters/DESIGN.md— records the server rule, how regions are matched, that the client mirrors only the certain refusal, and the gap below.Not covered: the server also counts shrinking a kept region as leaving it (a smaller latency tier of a catalog region, or fewer units of a custom one), so shrinking every current region while adding a new one is refused too. That combination is not mirrored here and still surfaces the server's error.
✅ Verification
Route: component tests driving the real
ClusterFormin jsdom (Radix selects opened and picked through clicks, #1780's region lookup), unit tests for the helper.src/features/clusters/upsert/ClusterForm.test.tsx— six cases under "ClusterForm editing regions": swapping US for Europe shows the guidance and disables submit (Investigate and improve UI process for changing cluster region #1311); adding a region and keeping US clears it and re-enables submit; the free tier gets the cannot-move-in-place message; a kept custom region is matched by id, so adding a catalog region beside it needs no guidance (new with the port); a tier missing from the catalog gets the single-region message, and the form shows no add-region button for it; a new cluster never sees it. The harness gainedclusterId,currentRegions,organizationRegionsandcanUseCustomRegionsoptions.src/features/clusters/upsert/lib/describeRegionSwap.test.ts(new) — plan resolution (a catalog plan by name, a custom region by id, the fallback for a plan the lookup lacks, self-hosted plans skipped), each null and message branch, and a custom region matched by id rather than name.ctx.addIssuecall disabled, the four positive component cases go red and the new-cluster and custom-region cases stay green. Comparing selected regions by name instead of cohort turns the custom-region case red. Naming only the first added region turns the several-regions helper test red.npx vitest run src/features/clusters/upsert→ 101 passed (exit 0). Full gate via the pre-commit hook at a8e2443: vitest 400 files / 3714 tests passed,oxlintclean,dprint check --stagedclean, commitlint ok;npx tsc -bexit 0.stagebefore it.Cross-model review on the rebased branch (one round, vs #1842's rebuilt branch): codex (graded), gemini and the Harper domain adjudicator all ran. The adjudicator found no case where the check blocks an update the server would accept, and kept five nits. Taken in a8e2443: the steps named only the first of several added regions, and two comments restated the code. Rejected: explicit import extensions (this repo's imports are extensionless), building the cohort list on a create (it fills inside the loop the refinement already runs, over at most 50 rows), and a segmented zod path (every other issue in this refinement, #1780's included, uses the dotted form, and the message lands on the field). Its open decisions (mirror the server rule, block rather than warn, the single-region advice, do-less via mapping the server error) are the calls above; the do-less option would let the update reach payment review and fail there, which is the report in #1311.
Before the rebase, three rounds (codex and gemini; the domain adjudicator ran in the third) fixed the multi-region message's "remove the others" and moved the plan derivation into the helper with a test. gemini-code-assist's suggestion to read a missing
selectedPlanas a multi-region tier was declined: such a plan prices at 0, and at 0 the form hides Add Additional Region Usage, which 74b524a pins.Closes #1311
🤖 Generated by Anthropic Claude Code (Claude Opus 5.5); posted via @dawsontoth.
🤖 Generated with Claude Code
Related PRs: #1842 overlaps (this branch is stacked on it), #1780 overlaps (base of the stack; the check uses its region ids, lookup and cohort key), #1851 overlaps (stacked on this branch)
Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; blocked=cursor-composer(no-receipt); declined=cursor-grok,cursor-kimi,cursor-muse; rounds=4; full=1 @ a8e2443
Review-Attention: read ~4m (decisions: client-mirror-of-cm-rule, blocking-vs-advisory, single-region-tier-advice, do-less-alternative) @ a8e2443