Skip to content

Simplify IpPoolSelector and call sites #3032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
david-crespo merged 4 commits into from
Feb 4, 2026
Merged

Simplify IpPoolSelector and call sites #3032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8da99ee
refactor IpPoolSelector, derive ipVersion
david-crespo Feb 4, 2026
fc1f8bf
handle default pool in defaultValues, don't sort
david-crespo Feb 4, 2026
688f05e
get rid of advanced according on floating IP create
david-crespo Feb 4, 2026
03e2f66
update e2es for removed advanced accordion
david-crespo Feb 4, 2026
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
86 changes: 28 additions & 58 deletions app/components/AttachEphemeralIpModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -17,7 +17,6 @@ import {
queryClient,
useApiMutation,
usePrefetchedQuery,
type IpVersion,
} from '~/api'
import { IpPoolSelector } from '~/components/form/fields/IpPoolSelector'
import { HL } from '~/components/HL'
Expand Down Expand Up @@ -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')
Expand All @@ -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 (
<Modal isOpen title="Attach ephemeral IP" onDismiss={onDismiss}>
Expand All @@ -124,10 +91,7 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void })
<IpPoolSelector
control={form.control}
poolFieldName="pool"
ipVersionFieldName="ipVersion"
pools={compatibleUnicastPools}
currentPool={pool}
setValue={form.setValue}
disabled={compatibleUnicastPools.length === 0}
compatibleVersions={compatibleVersions}
/>
Expand All @@ -136,17 +100,23 @@ export const AttachEphemeralIpModal = ({ onDismiss }: { onDismiss: () => void })
</Modal.Body>
<Modal.Footer
actionText="Attach"
disabled={disabledState.disabled}
disabledReason={disabledState.reason}
disabled={!!disabledReason}
disabledReason={disabledReason}
onAction={() => {
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',
Copy link
Collaborator Author

@david-crespo david-crespo Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Note v4 fallback where when there are no defaults doesn't really make sense because auto will fail in that case. What we should do is make the user pick. The simplest thing would be to always make the user pick and just not use the auto side here.

Copy link
Contributor

@charliepark charliepark Feb 4, 2026

Choose a reason for hiding this comment

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

I believe this is now how you have it, where the form is disabled with the No default compatible pool available; select a pool to continue message, so I don't know that anyone is going to end up hitting this.

Copy link
Collaborator Author

@david-crespo david-crespo Feb 4, 2026

Choose a reason for hiding this comment

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

Ah, that's right. Insight applied here for more cleanup 72b671a

},
},
})
}}
onDismiss={onDismiss}
Expand Down
56 changes: 5 additions & 51 deletions app/components/form/fields/IpPoolSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -19,30 +19,22 @@ const ALL_IP_VERSIONS: IpVersion[] = ['v4', 'v6']
type IpPoolSelectorProps = {
control: Control<any>
poolFieldName: string
ipVersionFieldName: string
pools: UnicastIpPool[]
/** Current value of the pool field */
currentPool: string | undefined
/** Function to update form values */
setValue: UseFormSetValue<any>
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
}

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
Expand All @@ -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,
])
Copy link
Collaborator Author

@david-crespo david-crespo Feb 4, 2026

Choose a reason for hiding this comment

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

I think this is only really necessary for instance create because that's the only one where the stack can change. We calculate it for ephemeral IP modal but we can do it once up front because it's in the context of an instance where the stack is fixed.


// 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])
Copy link
Collaborator Author

@david-crespo david-crespo Feb 4, 2026

Choose a reason for hiding this comment

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

this is not necessary at all if you derive it at submit time


return (
<div className="space-y-4">
{hasNoPools ? (
Expand Down
107 changes: 33 additions & 74 deletions app/forms/floating-ip-create.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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'
Expand All @@ -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()
Expand All @@ -75,43 +64,34 @@ export default function CreateFloatingIpSideModalForm() {
},
})

const form = useForm({ defaultValues })
const pool = form.watch('pool')

const [openItems, setOpenItems] = useState<string[]>([])
const form = useForm({
defaultValues: {
name: '',
description: '',
pool: defaultPool,
},
})

return (
<SideModalForm
form={form}
formType="create"
resourceName="floating IP"
onDismiss={() => 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 })
}}
Expand All @@ -120,28 +100,7 @@ export default function CreateFloatingIpSideModalForm() {
>
<NameField name="name" control={form.control} />
<DescriptionField name="description" control={form.control} />

<Accordion.Root
type="multiple"
className="mt-12 max-w-lg"
value={openItems}
onValueChange={setOpenItems}
>
<AccordionItem
isOpen={openItems.includes('advanced')}
label="Advanced"
value="advanced"
>
<IpPoolSelector
control={form.control}
poolFieldName="pool"
ipVersionFieldName="ipVersion"
pools={unicastPools}
currentPool={pool}
setValue={form.setValue}
/>
</AccordionItem>
</Accordion.Root>
<IpPoolSelector control={form.control} poolFieldName="pool" pools={unicastPools} />
</SideModalForm>
)
}
Loading
Loading