diff --git a/app/components/AttachEphemeralIpModal.tsx b/app/components/AttachEphemeralIpModal.tsx index be761fdc6..0605eceeb 100644 --- a/app/components/AttachEphemeralIpModal.tsx +++ b/app/components/AttachEphemeralIpModal.tsx @@ -6,7 +6,7 @@ * Copyright Oxide Computer Company */ -import { useCallback, useEffect, useMemo } from 'react' +import { useMemo } from 'react' import { useForm } from 'react-hook-form' import { @@ -17,7 +17,6 @@ import { queryClient, useApiMutation, usePrefetchedQuery, - type IpVersion, } from '~/api' import { IpPoolSelector } from '~/components/form/fields/IpPoolSelector' import { HL } from '~/components/HL' @@ -55,6 +54,11 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void }) return compatibleUnicastPools.some((p) => p.isDefault) }, [compatibleUnicastPools]) + const defaultPool = useMemo( + () => compatibleUnicastPools.find((p) => p.isDefault)?.name ?? '', + [compatibleUnicastPools] + ) + const instanceEphemeralIpAttach = useApiMutation(api.instanceEphemeralIpAttach, { onSuccess(ephemeralIp) { queryClient.invalidateEndpoint('instanceExternalIpList') @@ -67,54 +71,17 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void }) }, }) - const form = useForm<{ pool: string; ipVersion: IpVersion }>({ - defaultValues: { - pool: '', - ipVersion: 'v4', - }, - }) - - // Update ipVersion if only one version is compatible - useEffect(() => { - if (compatibleVersions.length === 1) { - form.setValue('ipVersion', compatibleVersions[0]) - } - }, [compatibleVersions, form]) + const form = useForm({ defaultValues: { pool: defaultPool } }) const pool = form.watch('pool') - const ipVersion = form.watch('ipVersion') - const disabledState = useMemo(() => { - if (compatibleVersions.length === 0) { - return { - disabled: true, - reason: 'Instance has no network interfaces with compatible IP stacks', - } - } - if (compatibleUnicastPools.length === 0) { - return { - disabled: true, - reason: 'No compatible unicast pools available for this instance', - } - } - if (!pool && !hasDefaultCompatiblePool) { - return { - disabled: true, - reason: 'No default compatible pool available; select a pool to continue', - } - } - return { disabled: false, reason: undefined } - }, [compatibleVersions, compatibleUnicastPools, pool, hasDefaultCompatiblePool]) - - const getEffectiveIpVersion = useCallback(() => { - if (pool) return ipVersion - - const { hasV4Default, hasV6Default } = getDefaultIps(compatibleUnicastPools) - - if (hasV4Default && !hasV6Default) return 'v4' - if (hasV6Default && !hasV4Default) return 'v6' - - return ipVersion - }, [pool, ipVersion, compatibleUnicastPools]) + const disabledReason = + compatibleVersions.length === 0 + ? 'Instance has no network interfaces with compatible IP stacks' + : compatibleUnicastPools.length === 0 + ? 'No compatible unicast pools available for this instance' + : !pool && !hasDefaultCompatiblePool + ? 'No default compatible pool available; select a pool to continue' + : undefined return ( @@ -124,10 +91,7 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void }) @@ -136,17 +100,23 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void }) { - const effectiveIpVersion = getEffectiveIpVersion() - + const { hasV4Default, hasV6Default } = getDefaultIps(compatibleUnicastPools) instanceEphemeralIpAttach.mutate({ path: { instance }, query: { project }, - body: pool - ? { poolSelector: { type: 'explicit', pool } } - : { poolSelector: { type: 'auto', ipVersion: effectiveIpVersion } }, + body: { + poolSelector: pool + ? { type: 'explicit', pool } + : { + type: 'auto', + // v4 fallback here should maybe be an error instead because + // it probably won't work on the API side + ipVersion: hasV4Default ? 'v4' : hasV6Default ? 'v6' : 'v4', + }, + }, }) }} onDismiss={onDismiss} diff --git a/app/components/form/fields/IpPoolSelector.tsx b/app/components/form/fields/IpPoolSelector.tsx index 74e1aa313..d08ca587f 100644 --- a/app/components/form/fields/IpPoolSelector.tsx +++ b/app/components/form/fields/IpPoolSelector.tsx @@ -5,8 +5,8 @@ * * Copyright Oxide Computer Company */ -import { useEffect, useMemo } from 'react' -import type { Control, UseFormSetValue } from 'react-hook-form' +import { useMemo } from 'react' +import type { Control } from 'react-hook-form' import * as R from 'remeda' import { poolHasIpVersion, type IpVersion, type UnicastIpPool } from '@oxide/api' @@ -19,19 +19,14 @@ const ALL_IP_VERSIONS: IpVersion[] = ['v4', 'v6'] type IpPoolSelectorProps = { control: Control poolFieldName: string - ipVersionFieldName: string pools: UnicastIpPool[] - /** Current value of the pool field */ - currentPool: string | undefined - /** Function to update form values */ - setValue: UseFormSetValue disabled?: boolean /** Compatible IP versions based on network interface type */ compatibleVersions?: IpVersion[] /** - * If true, automatically select a default pool when none is selected. - * If false, allow the field to remain empty to use API defaults. - * Default to true, to automatically select a default pool if available. + * If true, the pool field is required and defaults should be selected by + * the parent when available. If false, allow the field to remain empty to + * use API defaults. */ autoSelectDefault?: boolean } @@ -39,10 +34,7 @@ type IpPoolSelectorProps = { export function IpPoolSelector({ control, poolFieldName, - ipVersionFieldName, pools, - currentPool, - setValue, disabled = false, compatibleVersions = ALL_IP_VERSIONS, // When both a default IPv4 and default IPv6 pool exist, the component picks the @@ -59,44 +51,6 @@ export function IpPoolSelector({ const hasNoPools = sortedPools.length === 0 - // Set default pool selection on mount if none selected, or if current pool is no longer valid - useEffect(() => { - if (sortedPools.length > 0 && autoSelectDefault) { - const currentPoolValid = - currentPool && sortedPools.some((p) => p.name === currentPool) - - if (!currentPoolValid) { - // Only auto-select when there's an actual default pool - const defaultPool = sortedPools.find((p) => p.isDefault) - - if (defaultPool) { - setValue(poolFieldName, defaultPool.name) - setValue(ipVersionFieldName, defaultPool.ipVersion) - } else { - // Clear selection when current pool is invalid and no compatible default exists - setValue(poolFieldName, '') - } - } - } - }, [ - currentPool, - sortedPools, - poolFieldName, - ipVersionFieldName, - setValue, - autoSelectDefault, - ]) - - // Update IP version when pool changes - useEffect(() => { - if (currentPool) { - const selectedPool = sortedPools.find((p) => p.name === currentPool) - if (selectedPool) { - setValue(ipVersionFieldName, selectedPool.ipVersion) - } - } - }, [currentPool, sortedPools, ipVersionFieldName, setValue]) - return (
{hasNoPools ? ( diff --git a/app/forms/floating-ip-create.tsx b/app/forms/floating-ip-create.tsx index e8a73a21e..5da96482f 100644 --- a/app/forms/floating-ip-create.tsx +++ b/app/forms/floating-ip-create.tsx @@ -5,9 +5,7 @@ * * Copyright Oxide Computer Company */ -import * as Accordion from '@radix-ui/react-accordion' -import { useQuery } from '@tanstack/react-query' -import { useMemo, useState } from 'react' +import { useMemo } from 'react' import { useForm } from 'react-hook-form' import { useNavigate } from 'react-router' @@ -17,11 +15,10 @@ import { q, queryClient, useApiMutation, + usePrefetchedQuery, type FloatingIpCreate, - type IpVersion, } from '@oxide/api' -import { AccordionItem } from '~/components/AccordionItem' import { DescriptionField } from '~/components/form/fields/DescriptionField' import { IpPoolSelector } from '~/components/form/fields/IpPoolSelector' import { NameField } from '~/components/form/fields/NameField' @@ -34,32 +31,24 @@ import { ALL_ISH } from '~/util/consts' import { getDefaultIps } from '~/util/ip' import { pb } from '~/util/path-builder' -type FloatingIpCreateFormData = { - name: string - description: string - pool?: string - ipVersion: IpVersion -} +const poolList = q(api.projectIpPoolList, { query: { limit: ALL_ISH } }) -const defaultValues: FloatingIpCreateFormData = { - name: '', - description: '', - ipVersion: 'v4', +export async function clientLoader() { + await queryClient.fetchQuery(poolList) + return null } export const handle = titleCrumb('New Floating IP') export default function CreateFloatingIpSideModalForm() { - // Fetch 1000 to we can be sure to get them all. Don't bother prefetching - // because the list is hidden under the Advanced accordion. - const { data: allPools } = useQuery( - q(api.projectIpPoolList, { query: { limit: ALL_ISH } }) - ) + const { data: allPools } = usePrefetchedQuery(poolList) // Only unicast pools can be used for floating IPs - const unicastPools = useMemo( - () => allPools?.items.filter(isUnicastPool) || [], - [allPools] + const unicastPools = useMemo(() => allPools.items.filter(isUnicastPool), [allPools]) + + const defaultPool = useMemo( + () => unicastPools.find((p) => p.isDefault)?.name ?? '', + [unicastPools] ) const projectSelector = useProjectSelector() @@ -75,10 +64,13 @@ export default function CreateFloatingIpSideModalForm() { }, }) - const form = useForm({ defaultValues }) - const pool = form.watch('pool') - - const [openItems, setOpenItems] = useState([]) + const form = useForm({ + defaultValues: { + name: '', + description: '', + pool: defaultPool, + }, + }) return ( navigate(pb.floatingIps(projectSelector))} - onSubmit={({ pool, ipVersion, ...values }) => { - // When using default pool, derive ipVersion from available defaults - let effectiveIpVersion = ipVersion - if (!pool) { - const { hasV4Default, hasV6Default } = getDefaultIps(unicastPools) - - // If only one default exists, use that version - if (hasV4Default && !hasV6Default) { - effectiveIpVersion = 'v4' - } else if (hasV6Default && !hasV4Default) { - effectiveIpVersion = 'v6' - } - // If both exist, use form's ipVersion (user's choice) - } - + onSubmit={({ pool, name, description }) => { + const { hasV4Default, hasV6Default } = getDefaultIps(unicastPools) const body: FloatingIpCreate = { - ...values, - addressAllocator: pool - ? { - type: 'auto' as const, - poolSelector: { type: 'explicit' as const, pool }, - } - : { - type: 'auto' as const, - poolSelector: { type: 'auto' as const, ipVersion: effectiveIpVersion }, - }, + name, + description, + addressAllocator: { + type: 'auto', + poolSelector: pool + ? { type: 'explicit', pool } + : { + type: 'auto', + ipVersion: hasV4Default ? 'v4' : hasV6Default ? 'v6' : 'v4', + }, + }, } createFloatingIp.mutate({ query: projectSelector, body }) }} @@ -120,28 +100,7 @@ export default function CreateFloatingIpSideModalForm() { > - - - - - - + ) } diff --git a/app/forms/instance-create.tsx b/app/forms/instance-create.tsx index af800eadb..5a19a8d02 100644 --- a/app/forms/instance-create.tsx +++ b/app/forms/instance-create.tsx @@ -15,6 +15,7 @@ import { type UseFormSetValue, } from 'react-hook-form' import { Link, useNavigate, type LoaderFunctionArgs } from 'react-router' +import * as R from 'remeda' import { match, P } from 'ts-pattern' import type { SetRequired } from 'type-fest' @@ -142,7 +143,7 @@ export type InstanceCreateInput = Assign< sshPublicKeys: NonNullable // IP version for ephemeral IP when dual defaults exist ephemeralIpVersion: IpVersion - // Pool for ephemeral IP - used to sync with IpPoolSelector component + // Pool for ephemeral IP selection ephemeralIpPool: string } > @@ -763,9 +764,9 @@ const AdvancedAccordion = ({ const ephemeralIpPool = ephemeralIpPoolField.field.value // Initialize ephemeralIpPool once on mount if externalIps already has an explicit pool - const hasInitializedPoolRef = useRef(false) + const [poolInitDone, setPoolInitDone] = useState(false) useEffect(() => { - if (hasInitializedPoolRef.current) return + if (poolInitDone) return const initialPool = ephemeralIp?.poolSelector?.type === 'explicit' @@ -774,8 +775,8 @@ const AdvancedAccordion = ({ if (initialPool && !ephemeralIpPool) { ephemeralIpPoolField.field.onChange(initialPool) } - hasInitializedPoolRef.current = true - }, [ephemeralIp, ephemeralIpPool, ephemeralIpPoolField]) + setPoolInitDone(true) + }, [ephemeralIp, ephemeralIpPool, ephemeralIpPoolField, poolInitDone]) // Update externalIps when ephemeralIpPool or ephemeralIpVersion changes useEffect(() => { @@ -850,6 +851,32 @@ const AdvancedAccordion = ({ () => unicastPools.filter(poolHasIpVersion(compatibleVersions)), [unicastPools, compatibleVersions] ) + const sortedPools = useMemo( + () => R.sortBy(compatibleUnicastPools, (p) => [!p.isDefault, p.ipVersion, p.name]), + [compatibleUnicastPools] + ) + + useEffect(() => { + if (!poolInitDone || !assignEphemeralIp || sortedPools.length === 0) return + + const currentPoolValid = + ephemeralIpPool && sortedPools.some((p) => p.name === ephemeralIpPool) + if (currentPoolValid) return + + const defaultPool = sortedPools.find((p) => p.isDefault) + if (defaultPool) { + ephemeralIpPoolField.field.onChange(defaultPool.name) + } else { + ephemeralIpPoolField.field.onChange('') + } + }, [ + assignEphemeralIp, + ephemeralIpPool, + ephemeralIpPoolField, + poolInitDone, + setValue, + sortedPools, + ]) // Track previous ability to attach ephemeral IP to detect transitions const prevCanAttachRef = useRef(undefined) @@ -1009,10 +1036,7 @@ const AdvancedAccordion = ({ diff --git a/test/e2e/floating-ip-create.e2e.ts b/test/e2e/floating-ip-create.e2e.ts index 757080864..af406c6af 100644 --- a/test/e2e/floating-ip-create.e2e.ts +++ b/test/e2e/floating-ip-create.e2e.ts @@ -18,7 +18,6 @@ test('can create a floating IP', async ({ page }) => { 'role=heading[name*="Create floating IP"]', 'role=textbox[name="Name"]', 'role=textbox[name="Description"]', - 'role=button[name="Advanced"]', 'role=button[name="Create floating IP"]', ]) @@ -28,19 +27,8 @@ test('can create a floating IP', async ({ page }) => { .getByRole('textbox', { name: 'Description' }) .fill('A description for this Floating IP') - const advancedAccordion = page.getByRole('button', { name: 'Advanced' }) - const poolDropdown = page.getByLabel('Pool') - - // accordion content should be hidden - await expect(poolDropdown).toBeHidden() - - // open accordion - await advancedAccordion.click() - - // pool dropdown should now be visible - await expect(poolDropdown).toBeVisible() - // Default pool should be selected (ip-pool-1 is the v4 default) + const poolDropdown = page.getByLabel('Pool') await expect(poolDropdown).toContainText('ip-pool-1') // choose pool and submit diff --git a/test/e2e/ip-pool-silo-config.e2e.ts b/test/e2e/ip-pool-silo-config.e2e.ts index 8566e6468..098f33e49 100644 --- a/test/e2e/ip-pool-silo-config.e2e.ts +++ b/test/e2e/ip-pool-silo-config.e2e.ts @@ -56,14 +56,8 @@ test.describe('IP pool configuration: myriad silo (v4-only default)', () => { const page = await getPageAsUser(browser, 'Aryeh Kosman') await page.goto('/projects/kosman-project/floating-ips-new') - await page.getByRole('textbox', { name: 'Name', exact: true }).fill('test-fip') - - // Open advanced accordion to see pool selector - await page.getByRole('button', { name: 'Advanced' }).click() - // Pool dropdown should show IPv4 default pool const poolDropdown = page.getByLabel('Pool') - await expect(poolDropdown).toBeVisible() await expect(poolDropdown).toContainText('ip-pool-1') }) }) @@ -109,14 +103,8 @@ test.describe('IP pool configuration: thrax silo (v6-only default)', () => { const page = await getPageAsUser(browser, 'Elizabeth Anscombe') await page.goto('/projects/anscombe-project/floating-ips-new') - await page.getByRole('textbox', { name: 'Name', exact: true }).fill('test-fip') - - // Open advanced accordion to see pool selector - await page.getByRole('button', { name: 'Advanced' }).click() - // Pool dropdown should show IPv6 default pool const poolDropdown = page.getByLabel('Pool') - await expect(poolDropdown).toBeVisible() await expect(poolDropdown).toContainText('ip-pool-2') }) }) @@ -162,16 +150,8 @@ test.describe('IP pool configuration: pelerines silo (no defaults)', () => { const page = await getPageAsUser(browser, 'Theodor Adorno') await page.goto('/projects/adorno-project/floating-ips-new') - await page.getByRole('textbox', { name: 'Name', exact: true }).fill('test-fip') - - // Open advanced accordion to see pool selector - await page.getByRole('button', { name: 'Advanced' }).click() - - // Pool dropdown should be visible - const poolDropdown = page.getByLabel('Pool') - await expect(poolDropdown).toBeVisible() - // User should be able to select from available pools + const poolDropdown = page.getByLabel('Pool') await poolDropdown.click() await expect(page.getByRole('option', { name: 'ip-pool-1' })).toBeVisible() await expect(page.getByRole('option', { name: 'ip-pool-2' })).toBeVisible()