diff --git a/DESIGN.md b/DESIGN.md index be3545a83..31bdfb9ce 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -6,7 +6,7 @@ This file collects architecture and design notes for studio that aren't otherwis - [`src/features/organizations/DESIGN.md`](src/features/organizations/DESIGN.md) — shared organization target policy, switch destinations, and account-menu behavior. -- [`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/clusters/DESIGN.md`](src/features/clusters/DESIGN.md) — cluster list lifecycle classification, optional metadata and notification scope, where a saved cluster draft reopens, 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, and where the cluster card's sync chip gets its data. diff --git a/src/features/clusters/ClustersList.test.tsx b/src/features/clusters/ClustersList.test.tsx index f9fe02e3e..a324de166 100644 --- a/src/features/clusters/ClustersList.test.tsx +++ b/src/features/clusters/ClustersList.test.tsx @@ -115,4 +115,19 @@ describe('ClustersList', () => { render(); expect(screen.getByText('Replace /org-a/new-cluster')).toBeTruthy(); }); + + it('sends a draft saved while editing back to the edit form of that cluster (#1852)', () => { + state.clusters = [cluster('Production', 'RUNNING', 'US East'), cluster('Staging', 'RUNNING', 'US West')]; + state.saved = { clusterId: 'Staging', clusterName: 'Staging', skipToBilling: true }; + render(); + expect(screen.getByText('Replace /org-a/Staging/edit')).toBeTruthy(); + }); + + it('stays on the list for an edit draft of a cluster this organization does not have', () => { + state.clusters = [cluster('Production', 'RUNNING', 'US East')]; + state.saved = { clusterId: 'clu-elsewhere', clusterName: 'Elsewhere', skipToBilling: true }; + render(); + expect(screen.queryByText(/^(Replace|Push) /)).toBeNull(); + expect(screen.getByRole('article').textContent).toBe('Production'); + }); }); diff --git a/src/features/clusters/ClustersList.tsx b/src/features/clusters/ClustersList.tsx index 6f55a7e03..18754dfd4 100644 --- a/src/features/clusters/ClustersList.tsx +++ b/src/features/clusters/ClustersList.tsx @@ -13,6 +13,7 @@ import { } from '@/features/clusters/lib/clusterListModel'; import { getRegionLocationsOptions } from '@/features/clusters/queries/getRegionLocationsQuery'; import { UpsertCluster } from '@/features/clusters/upsert'; +import { draftClusterIdOf } from '@/features/clusters/upsert/draftClusterIdOf'; import { OrgPageLayout } from '@/features/organization/components/OrgPageLayout'; import { getOrganizationQueryOptions } from '@/features/organization/queries/getOrganizationQuery'; import { useLocalStorage } from '@/hooks/useLocalStorage'; @@ -58,7 +59,14 @@ export function ClustersList() { return ; } if (savedClusterState) { - return ; + const draftClusterId = draftClusterIdOf(savedClusterState); + if (!draftClusterId) { + return ; + } + // An edit draft for a cluster this organization doesn't list is left saved for the organization it came from. + if (orgInfo?.clusters?.some(({ id }) => id === draftClusterId)) { + return ; + } } return ( diff --git a/src/features/clusters/DESIGN.md b/src/features/clusters/DESIGN.md index 16bb0b7b1..2959c5aa2 100644 --- a/src/features/clusters/DESIGN.md +++ b/src/features/clusters/DESIGN.md @@ -7,6 +7,17 @@ form clears the draft only once it mounts, so a pushed redirect left Back on the as long as the form was still loading. Billing's Stripe return page replaces itself on the way back to the form for the same reason: it strips the setup-intent secret from its own URL first, so going back to it would show a spinner forever. +Which form a draft belongs to is read the same way in all three places, by `upsert/draftClusterIdOf.ts` (#1852). A +draft saved during an edit carries `clusterId`, and nothing else does: a new-cluster draft, a failed cluster being tried +again (a whole `Cluster`, keyed `id`) and a `createCluster` link all leave it unset. The list and the billing return page +send an edit draft to that cluster's edit form, and the form applies a draft only when its `clusterId` matches the +route's, so an edit draft never fills a create form. The list redirects an edit draft only for a cluster the +organization lists; any other edit draft is left saved for the organization it came from. The form clears the draft +once it mounts and again when it saves, and `UpsertCluster` lets those clears through only for a draft of this form, so +visiting another form, including the create form an empty organization's list shows, never discards one. That check +reads the stored draft at the moment of the clear rather than a render's copy: the billing step saves a draft and clears +it again across an `await`, from a render that never saw it. + SystemStatus notices have no cluster association and remain in the cloud-wide notification banner/center. Per-cluster monitoring incidents and scheduled maintenance need a scoped backend contract before the list can attribute them. The UI preserves ClusterCard's permission checks and lifecycle actions. # Cluster configuration diff --git a/src/features/clusters/upsert/draftClusterIdOf.test.ts b/src/features/clusters/upsert/draftClusterIdOf.test.ts new file mode 100644 index 000000000..a2ae28580 --- /dev/null +++ b/src/features/clusters/upsert/draftClusterIdOf.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest'; +import { draftClusterIdOf } from './draftClusterIdOf'; + +describe('draftClusterIdOf', () => { + it('names the cluster a draft saved during an edit belongs to', () => { + expect(draftClusterIdOf({ clusterId: 'clu-1', clusterName: 'Production' })).toBe('clu-1'); + }); + + it.each([ + ['no draft', null], + ['a new-cluster draft', { clusterName: 'Fresh' }], + ['a failed cluster being tried again', { id: 'clu-9', name: 'Failed one', organizationId: 'org-a' }], + ['a createCluster link', 'shared'], + ['an empty cluster id', { clusterId: '' }], + ['a cluster id that is not a string', { clusterId: 42 }], + ])('names none for %s', (_label, saved) => { + expect(draftClusterIdOf(saved)).toBeUndefined(); + }); +}); diff --git a/src/features/clusters/upsert/draftClusterIdOf.ts b/src/features/clusters/upsert/draftClusterIdOf.ts new file mode 100644 index 000000000..31cbefac8 --- /dev/null +++ b/src/features/clusters/upsert/draftClusterIdOf.ts @@ -0,0 +1,7 @@ +export function draftClusterIdOf(savedClusterState: unknown): string | undefined { + if (typeof savedClusterState !== 'object' || savedClusterState === null) { + return undefined; + } + const { clusterId } = savedClusterState as { clusterId?: unknown }; + return typeof clusterId === 'string' && clusterId ? clusterId : undefined; +} diff --git a/src/features/clusters/upsert/index.test.tsx b/src/features/clusters/upsert/index.test.tsx new file mode 100644 index 000000000..a2f1bccf0 --- /dev/null +++ b/src/features/clusters/upsert/index.test.tsx @@ -0,0 +1,151 @@ +/** @vitest-environment jsdom */ +import type { Cluster } from '@/integrations/api/api.patch'; +import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import type { ReactNode } from 'react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { UpsertCluster } from './index'; + +const state = vi.hoisted(() => ({ + params: { organizationId: 'org-a' } as { organizationId: string; clusterId?: string }, + cluster: undefined as Cluster | undefined, + saved: null as unknown, +})); +vi.mock('@tanstack/react-router', () => ({ + useParams: () => state.params, + useRouteContext: () => ({ organization: { id: 'org-a', clusters: [] }, cluster: state.cluster }), +})); +vi.mock('@tanstack/react-query', () => ({ + useQuery: ({ queryKey }: { queryKey: string[] }) => ({ + data: { + plans: [{ id: 'shared', priceUsd: 10, deploymentDescription: 'Shared', performanceDescription: 'Basic' }], + regions: [], + versions: { value: [{ name: 'current', version: '5.0.0' }] }, + }[queryKey[0]], + }), +})); +vi.mock( + '@/features/cluster/queries/getPlanTypesQuery', + () => ({ getPlanTypesOptions: () => ({ queryKey: ['plans'] }) }), +); +vi.mock( + '@/features/clusters/queries/getRegionLocationsQuery', + () => ({ getRegionLocationsOptions: () => ({ queryKey: ['regions'] }) }), +); +vi.mock( + '@/features/clusters/queries/getHarperVersionsQuery', + () => ({ getHarperVersionsOptions: () => ({ queryKey: ['versions'] }) }), +); +vi.mock('@/hooks/useLocalStorage', () => ({ + useLocalStorage: () => [state.saved, (next: unknown) => { + state.saved = typeof next === 'function' ? next(state.saved) : next; + }], +})); +vi.mock( + '@/hooks/usePermissions', + () => ({ useOrganizationClusterPermissions: () => ({ create: true, update: true }) }), +); +vi.mock('@/components/SubNavMenu', () => ({ SubNavMenu: () => null })); +vi.mock( + '@/components/SubNavSimpleLayout', + () => ({ SubNavSimpleLayout: ({ children }: { children: ReactNode }) =>
{children}
}), +); +vi.mock( + '@/features/cluster/components/ClusterPageLayout', + () => ({ ClusterPageLayout: ({ children }: { children: ReactNode }) =>
{children}
}), +); +// Clears the draft on mount like the real form, and replays the billing step's save-then-clear from +// the setter it was first handed. +vi.mock('./ClusterForm', async () => { + const { useEffect } = await import('react'); + return { + ClusterForm: ({ defaultValues, startOffOnBilling, setSavedClusterState }: { + defaultValues: { clusterName: string }; + startOffOnBilling: boolean; + setSavedClusterState: (value: unknown) => void; + }) => { + useEffect(() => setSavedClusterState(null), [setSavedClusterState]); + return ( + <> +

{`form for "${defaultValues.clusterName}"${startOffOnBilling ? ', on billing' : ''}`}

+ + + ); + }, + }; +}); + +afterEach(() => { + cleanup(); + state.params = { organizationId: 'org-a' }; + state.cluster = undefined; + state.saved = null; +}); + +const editDraft = { + clusterId: 'clu-1', + clusterName: 'Production, resized', + deploymentDescription: 'Shared', + performanceDescription: 'Basic', + regionPlans: [], + instances: [], + skipToBilling: true, +}; + +function onEditRoute(id: string, name: string) { + state.params = { organizationId: 'org-a', clusterId: id }; + state.cluster = { id, name, organizationId: 'org-a', plans: [{ planId: 'shared', regionId: 'reg-east' }] }; +} + +describe('UpsertCluster — a saved cluster draft', () => { + it('reopens an edit draft on the edit form of the cluster it was saved from, then clears it', () => { + onEditRoute('clu-1', 'Production'); + state.saved = editDraft; + render(); + expect(screen.getByText('form for "Production, resized", on billing')).toBeTruthy(); + expect(state.saved).toBeNull(); + }); + + it('neither fills nor clears a new-cluster form with an edit draft (#1852)', () => { + state.saved = editDraft; + render(); + expect(screen.getByText('form for ""')).toBeTruthy(); + expect(state.saved).toBe(editDraft); + }); + + it("neither fills nor clears another cluster's edit form with an edit draft", () => { + onEditRoute('clu-2', 'Staging'); + state.saved = editDraft; + render(); + expect(screen.getByText('form for "Staging"')).toBeTruthy(); + expect(state.saved).toBe(editDraft); + }); + + it('still reopens a new-cluster draft on the new-cluster form, then clears it', () => { + state.saved = { ...editDraft, clusterId: undefined, clusterName: 'Fresh' }; + render(); + expect(screen.getByText('form for "Fresh", on billing')).toBeTruthy(); + expect(state.saved).toBeNull(); + }); + + it('still prefills a new-cluster form from a failed cluster being tried again, then clears it', () => { + state.saved = { id: 'clu-9', name: 'Failed one', organizationId: 'org-a', plans: [] }; + render(); + expect(screen.getByText('form for "Failed one"')).toBeTruthy(); + expect(state.saved).toBeNull(); + }); + + it('clears the draft the billing step saved when its redirect fails, though it rendered before the save', () => { + onEditRoute('clu-1', 'Production'); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Billing redirect fails' })); + expect(state.saved).toBeNull(); + }); +}); diff --git a/src/features/clusters/upsert/index.tsx b/src/features/clusters/upsert/index.tsx index 83d71401f..a54f654a8 100644 --- a/src/features/clusters/upsert/index.tsx +++ b/src/features/clusters/upsert/index.tsx @@ -19,9 +19,10 @@ import { groupThenKeyBy } from '@/lib/groupThenKeyBy'; import { LocalStorageKeys } from '@/lib/storage/localStorageKeys'; import { useQuery } from '@tanstack/react-query'; import { useParams, useRouteContext } from '@tanstack/react-router'; -import { ReactNode, useMemo } from 'react'; +import { ReactNode, useCallback, useMemo } from 'react'; import { z } from 'zod'; import { ClusterForm } from './ClusterForm'; +import { draftClusterIdOf } from './draftClusterIdOf'; import { isUpsertClusterSchema } from './isUpsertClusterSchema'; import { buildUpgradeVersionOptions } from './lib/buildUpgradeVersionOptions'; import { @@ -59,12 +60,21 @@ export function UpsertCluster() { organization: Organization; cluster?: Cluster; } = useRouteContext({ strict: false }); - const [savedClusterState, setSavedClusterState] = useLocalStorage< + const [storedClusterState, setStoredClusterState] = useLocalStorage< | null | ({ clusterId?: string; } & (UpsertClusterSchemaType | Cluster)) >(LocalStorageKeys.SavedClusterState, null); + // A draft belongs to the form it was saved from: only that form reads it, and only that form may + // clear it. The clear checks the draft stored at that moment, because the billing step saves one + // and clears it again across an await, from a render that never saw it. + const savedClusterState = draftClusterIdOf(storedClusterState) === clusterId ? storedClusterState : null; + const setSavedClusterState = useCallback( + (value: null | ({ clusterId?: string } & UpsertClusterSchemaType)) => + setStoredClusterState((stored) => value === null && draftClusterIdOf(stored) !== clusterId ? stored : value), + [clusterId, setStoredClusterState], + ); const { data: planTypes } = useQuery(getPlanTypesOptions(organizationId)); const { data: regionLocationsColocated } = useQuery(getRegionLocationsOptions({ diff --git a/src/features/organization/billing/confirm/ProcessSetupIntent.tsx b/src/features/organization/billing/confirm/ProcessSetupIntent.tsx index 3cd766cc4..90c3d63ae 100644 --- a/src/features/organization/billing/confirm/ProcessSetupIntent.tsx +++ b/src/features/organization/billing/confirm/ProcessSetupIntent.tsx @@ -1,4 +1,5 @@ import { Loading } from '@/components/Loading'; +import { draftClusterIdOf } from '@/features/clusters/upsert/draftClusterIdOf'; import { useLocalStorage } from '@/hooks/useLocalStorage'; import { useProcessStripePaymentMethod } from '@/integrations/stripe/useProcessStripePaymentMethod'; import { LocalStorageKeys } from '@/lib/storage/localStorageKeys'; @@ -16,19 +17,16 @@ export function ProcessSetupIntent() { const { setup_intent_client_secret: clientSecretAfterHash }: { setup_intent_client_secret?: string; } = useSearch({ strict: false }); - const [savedClusterState] = useLocalStorage< - { - clusterId?: string; - } & unknown | null - >(LocalStorageKeys.SavedClusterState, null); + const [savedClusterState] = useLocalStorage(LocalStorageKeys.SavedClusterState, null); const clientSecret = clientSecretBeforeHash || clientSecretAfterHash; const stripe = useStripe(); const processStripePaymentMethod = useProcessStripePaymentMethod(organizationId); const navigateBack = useCallback(() => { + const draftClusterId = draftClusterIdOf(savedClusterState); const to = savedClusterState - ? savedClusterState.clusterId - ? `../../${savedClusterState.clusterId}/edit` + ? draftClusterId + ? `../../${draftClusterId}/edit` : '../../new-cluster' : '../'; window.history.replaceState(null, '', currentUrlIncludingHash());