Refactor InstanceLinkCell and floating IP form (#3142)

Extracted from external subnets work coming in #3039 but not present yet
in that PR.

Author: david-crespo

AGENTS.md

@@ -4,6 +4,7 @@
 - Favor well-supported libraries, avoid premature abstractions, and use routes to capture state.
 - Before starting a feature, skim an existing page or form with similar behavior and mirror the conventions—this codebase is intentionally conventional. Look for similar pages in `app/pages` and forms in `app/forms` to use as templates.
 - `@oxide/api` is at `app/api` and `@oxide/api-mocks` is at `mock-api/index.ts`.
+- The language server often has out of date errors. tsgo is extremely fast, so confirm errors that come from the language server by running `npm run tsc`
 - Use Node.js 22+, then install deps and start the mock-backed dev server (skip if `npm run dev` is already running in another terminal):
 
   ```sh
@@ -24,7 +25,7 @@
 
 - Run local checks before sending PRs: `npm run lint`, `npm run tsc`, `npm test run`, and `npm run e2ec`.
 - You don't usually need to run all the e2e tests, so try to filter by file and tes t name like `npm run e2ec -- instance -g 'boot disk'`. CI will run the full set.
-- Keep Playwright specs focused on user-visible behavior—use accessible locators (`getByRole`, `getByLabel`), the helpers in `test/e2e/utils.ts` (`expectToast`, `expectRowVisible`, `selectOption`, `clickRowAction`), and close toasts so follow-on assertions aren’t blocked. Avoid Playwright’s legacy string selector syntax like `page.click(‘role=button[name="..."]’)`; prefer `page.getByRole(‘button’, { name: ‘...’ }).click()` and friends.
+- Keep Playwright specs focused on user-visible behavior—use accessible locators (`getByRole`, `getByLabel`), the helpers in `test/e2e/utils.ts` (`expectToast`, `expectRowVisible`, `selectOption`, `clickRowAction`), and close toasts so follow-on assertions aren’t blocked. Avoid Playwright’s legacy string selector syntax like `page.click(‘role=button[name="..."]’)`; prefer `page.getByRole(‘button’, { name: ‘...’ }).click()` and friends. Avoid `getByTestId` in e2e tests—prefer scoping with accessible locators like `page.getByRole(‘dialog’)` when possible.
 - Cover role-gated flows by logging in with `getPageAsUser`; exercise negative paths (e.g., forbidden actions) alongside happy paths as shown in `test/e2e/system-update.e2e.ts`.
 - Consider `expectVisible` and `expectNotVisible` deprecated: prefer `expect().toBeVisible()` and `toBeHidden()` in new code.
 - When UI needs new mock behavior, extend the MSW handlers/db minimally so E2E tests stay deterministic; prefer storing full API responses so subsequent calls see the updated state (`mock-api/msw/db.ts`, `mock-api/msw/handlers.ts`).

app/forms/firewall-rules-common.tsx

@@ -487,7 +487,7 @@ const ProtocolFilters = ({ control }: { control: Control<FirewallRuleValues> })
                   control={protocolForm.control}
                   description={
                     <>
-                      Enter a code (0) or range (e.g. 1&ndash;3). Leave blank for all
+                      Enter a code (0) or range (e.g., 1&ndash;3). Leave blank for all
                       traffic of type {selectedIcmpType}.
                     </>
                   }

app/forms/floating-ip-edit.tsx

@@ -6,12 +6,12 @@
  * Copyright Oxide Computer Company
  */
 import { useForm } from 'react-hook-form'
-import { Link, useNavigate, type LoaderFunctionArgs } from 'react-router'
+import { useNavigate, type LoaderFunctionArgs } from 'react-router'
 
 import {
   api,
-  getListQFn,
   q,
+  qErrorsAllowed,
   queryClient,
   useApiMutation,
   usePrefetchedQuery,
@@ -24,26 +24,40 @@ import { HL } from '~/components/HL'
 import { titleCrumb } from '~/hooks/use-crumbs'
 import { getFloatingIpSelector, useFloatingIpSelector } from '~/hooks/use-params'
 import { addToast } from '~/stores/toast'
-import { EmptyCell } from '~/table/cells/EmptyCell'
+import { InstanceLink } from '~/table/cells/InstanceLinkCell'
 import { IpPoolCell } from '~/table/cells/IpPoolCell'
 import { CopyableIp } from '~/ui/lib/CopyableIp'
 import { SideModalFormDocs } from '~/ui/lib/ModalLinks'
 import { PropertiesTable } from '~/ui/lib/PropertiesTable'
-import { ALL_ISH } from '~/util/consts'
 import { docLinks } from '~/util/links'
 import { pb } from '~/util/path-builder'
 import type * as PP from '~/util/path-params'
 
 const floatingIpView = ({ project, floatingIp }: PP.FloatingIp) =>
   q(api.floatingIpView, { path: { floatingIp }, query: { project } })
-const instanceList = (project: string) =>
-  getListQFn(api.instanceList, { query: { project, limit: ALL_ISH } })
 
 export async function clientLoader({ params }: LoaderFunctionArgs) {
   const selector = getFloatingIpSelector(params)
+  const fip = await queryClient.fetchQuery(floatingIpView(selector))
   await Promise.all([
-    queryClient.fetchQuery(floatingIpView(selector)),
-    queryClient.fetchQuery(instanceList(selector.project).optionsFn()),
+    queryClient.prefetchQuery(
+      // ip pool cell uses errors allowed, so we have to do that here to match
+      qErrorsAllowed(
+        api.ipPoolView,
+        { path: { pool: fip.ipPoolId } },
+        {
+          errorsExpected: {
+            explanation: 'the referenced IP pool may have been deleted.',
+            statusCode: 404,
+          },
+        }
+      )
+    ),
+    fip.instanceId
+      ? queryClient.prefetchQuery(
+          q(api.instanceView, { path: { instance: fip.instanceId } })
+        )
+      : null,
   ])
   return null
 }
@@ -58,10 +72,6 @@ export default function EditFloatingIpSideModalForm() {
   const onDismiss = () => navigate(pb.floatingIps({ project: floatingIpSelector.project }))
 
   const { data: floatingIp } = usePrefetchedQuery(floatingIpView(floatingIpSelector))
-  const { data: instances } = usePrefetchedQuery(
-    instanceList(floatingIpSelector.project).optionsFn()
-  )
-  const instanceName = instances.items.find((i) => i.id === floatingIp.instanceId)?.name
 
   const editFloatingIp = useApiMutation(api.floatingIpUpdate, {
     onSuccess(_floatingIp) {
@@ -100,19 +110,7 @@ export default function EditFloatingIpSideModalForm() {
           <IpPoolCell ipPoolId={floatingIp.ipPoolId} />
         </PropertiesTable.Row>
         <PropertiesTable.Row label="Instance">
-          {instanceName ? (
-            <Link
-              to={pb.instanceNetworking({
-                project: floatingIpSelector.project,
-                instance: instanceName,
-              })}
-              className="link-with-underline text-sans-md group"
-            >
-              {instanceName}
-            </Link>
-          ) : (
-            <EmptyCell />
-          )}
+          <InstanceLink instanceId={floatingIp.instanceId} tab="networking" />
         </PropertiesTable.Row>
       </PropertiesTable>
       <NameField name="name" control={form.control} />

app/pages/project/disks/DisksPage.tsx

@@ -30,7 +30,7 @@ import { getProjectSelector, useProjectSelector } from '~/hooks/use-params'
 import { useQuickActions } from '~/hooks/use-quick-actions'
 import { confirmDelete } from '~/stores/confirm-delete'
 import { addToast } from '~/stores/toast'
-import { InstanceLinkCell } from '~/table/cells/InstanceLinkCell'
+import { InstanceLink } from '~/table/cells/InstanceLinkCell'
 import { LinkCell } from '~/table/cells/LinkCell'
 import { useColsWithActions, type MenuAction } from '~/table/columns/action-col'
 import { Columns } from '~/table/columns/common'
@@ -68,7 +68,7 @@ export async function clientLoader({ params }: LoaderFunctionArgs) {
     queryClient.prefetchQuery(diskList({ project }).optionsFn()),
 
     // fetch instances and preload into RQ cache so fetches by ID in
-    // InstanceLinkCell can be mostly instant yet gracefully fall back to
+    // InstanceLink can be mostly instant yet gracefully fall back to
     // fetching individually if we don't fetch them all here
     queryClient.fetchQuery(instanceList({ project }).optionsFn()).then((instances) => {
       for (const instance of instances.items) {
@@ -165,7 +165,9 @@ export default function DisksPage() {
           (disk) => ('instance' in disk.state ? disk.state.instance : undefined),
           {
             header: 'Attached to',
-            cell: (info) => <InstanceLinkCell instanceId={info.getValue()} />,
+            cell: (info) => (
+              <InstanceLink instanceId={info.getValue()} tab="storage" cell />
+            ),
           }
         ),
         colHelper.accessor('diskType', {

app/pages/project/floating-ips/FloatingIpsPage.tsx

@@ -33,7 +33,7 @@ import { useQuickActions } from '~/hooks/use-quick-actions'
 import { confirmAction } from '~/stores/confirm-action'
 import { confirmDelete } from '~/stores/confirm-delete'
 import { addToast } from '~/stores/toast'
-import { InstanceLinkCell } from '~/table/cells/InstanceLinkCell'
+import { InstanceLink } from '~/table/cells/InstanceLinkCell'
 import { IpPoolCell } from '~/table/cells/IpPoolCell'
 import { useColsWithActions, type MenuAction } from '~/table/columns/action-col'
 import { Columns } from '~/table/columns/common'
@@ -102,7 +102,7 @@ const staticCols = [
   }),
   colHelper.accessor('instanceId', {
     header: 'Instance',
-    cell: (info) => <InstanceLinkCell instanceId={info.getValue()} />,
+    cell: (info) => <InstanceLink instanceId={info.getValue()} tab="networking" cell />,
   }),
 ]
 
@@ -227,7 +227,7 @@ export default function FloatingIpsPage() {
       ...(allFips?.items || []).map((f) => ({
         value: f.name,
         action: pb.floatingIpEdit({ project, floatingIp: f.name }),
-        navGroup: 'Go to floating IP',
+        navGroup: 'Edit floating IP',
       })),
     ],
     [project, allFips]

app/table/cells/InstanceLinkCell.tsx

@@ -7,6 +7,7 @@
  */
 
 import { useQuery } from '@tanstack/react-query'
+import { Link } from 'react-router'
 
 import { api, q } from '@oxide/api'
 
@@ -16,19 +17,31 @@ import { pb } from '~/util/path-builder'
 import { EmptyCell, SkeletonCell } from './EmptyCell'
 import { LinkCell } from './LinkCell'
 
-export const InstanceLinkCell = ({ instanceId }: { instanceId?: string | null }) => {
+type InstanceLinkProps = {
+  instanceId?: string | null
+  tab: 'storage' | 'networking'
+  /** Use table cell styling with hover highlight. */
+  cell?: boolean
+}
+
+export const InstanceLink = ({ instanceId, tab, cell }: InstanceLinkProps) => {
   const { project } = useProjectSelector()
   const { data: instance } = useQuery(
     q(api.instanceView, { path: { instance: instanceId! } }, { enabled: !!instanceId })
   )
 
-  // has to be after the hooks because hooks can't be executed conditionally
   if (!instanceId) return <EmptyCell />
   if (!instance) return <SkeletonCell />
 
-  return (
-    <LinkCell to={pb.instance({ project, instance: instance.name })}>
+  const params = { project, instance: instance.name }
+  const to =
+    tab === 'networking' ? pb.instanceNetworking(params) : pb.instanceStorage(params)
+
+  return cell ? (
+    <LinkCell to={to}>{instance.name}</LinkCell>
+  ) : (
+    <Link to={to} className="link-with-underline text-sans-md">
       {instance.name}
-    </LinkCell>
+    </Link>
   )
 }

app/table/cells/IpPoolCell.tsx

@@ -26,8 +26,8 @@ export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => {
     )
   )
   if (!result) return <SkeletonCell />
-  // this should essentially never happen, but it's probably better than blowing
-  // up the whole page if the pool is not found
+  // Defensive: the error case should never happen in practice. It should not be
+  // possible for a resource to reference a pool without that pool existing.
   if (result.type === 'error') return <EmptyCell />
   const pool = result.data
   return (

mock-api/msw/handlers.ts

@@ -368,10 +368,7 @@ export const handlers = makeHandlers({
   },
   floatingIpDetach({ path, query }) {
     const floatingIp = lookup.floatingIp({ ...path, ...query })
-    db.floatingIps = db.floatingIps.map((ip) =>
-      ip.id !== floatingIp.id ? ip : { ...ip, instance_id: undefined }
-    )
-
+    floatingIp.instance_id = undefined
     return floatingIp
   },
   imageList({ query }) {
@@ -1917,6 +1914,25 @@ export const handlers = makeHandlers({
 
     return { role_assignments }
   },
+  systemPolicyUpdate({ body, cookies }) {
+    requireFleetAdmin(cookies)
+
+    const newAssignments = body.role_assignments
+      .filter((r) => fleetRoles.some((role) => role === r.role_name))
+      .map((r) => ({
+        resource_type: 'fleet' as const,
+        resource_id: FLEET_ID,
+        ...R.pick(r, ['identity_id', 'identity_type', 'role_name']),
+      }))
+
+    const unrelatedAssignments = db.roleAssignments.filter(
+      (r) => !(r.resource_type === 'fleet' && r.resource_id === FLEET_ID)
+    )
+
+    db.roleAssignments = [...unrelatedAssignments, ...newAssignments]
+
+    return body
+  },
   systemMetric(params) {
     requireFleetViewer(params.cookies)
     return handleMetrics(params)
@@ -2348,25 +2364,6 @@ export const handlers = makeHandlers({
   supportBundleUpdate: NotImplemented,
   supportBundleView: NotImplemented,
   switchView: NotImplemented,
-  systemPolicyUpdate({ body, cookies }) {
-    requireFleetAdmin(cookies)
-
-    const newAssignments = body.role_assignments
-      .filter((r) => fleetRoles.some((role) => role === r.role_name))
-      .map((r) => ({
-        resource_type: 'fleet' as const,
-        resource_id: FLEET_ID,
-        ...R.pick(r, ['identity_id', 'identity_type', 'role_name']),
-      }))
-
-    const unrelatedAssignments = db.roleAssignments.filter(
-      (r) => !(r.resource_type === 'fleet' && r.resource_id === FLEET_ID)
-    )
-
-    db.roleAssignments = [...unrelatedAssignments, ...newAssignments]
-
-    return body
-  },
   systemQuotasList: NotImplemented,
   systemTimeseriesSchemaList: NotImplemented,
   systemUpdateRecoveryFinish: NotImplemented,

test/e2e/floating-ip-update.e2e.ts

@@ -31,6 +31,12 @@ test('can update a floating IP', async ({ page }) => {
   await clickRowAction(page, 'cola-float', 'Edit')
   await expectVisible(page, expectedFormElements)
 
+  // Properties table should show resolved instance and pool names
+  const dialog = page.getByRole('dialog')
+  await expect(dialog.getByText('ip-pool-1')).toBeVisible()
+  // cola-float is attached to db1
+  await expect(dialog.getByRole('link', { name: 'db1' })).toBeVisible()
+
   await page.fill('input[name=name]', updatedName)
   await page.getByRole('textbox', { name: 'Description' }).fill(updatedDescription)
   await page.getByRole('button', { name: 'Update floating IP' }).click()

test/e2e/instance-networking.e2e.ts

@@ -65,7 +65,7 @@ test('Instance networking tab — NIC table', async ({ page }) => {
   await page.getByRole('textbox', { name: 'Name' }).fill('nic-2')
   await page.getByLabel('VPC', { exact: true }).click()
   await page.getByRole('option', { name: 'mock-vpc' }).click()
-  await page.getByLabel('Subnet').click()
+  await page.getByRole('dialog').getByLabel('Subnet').click()
   await page.getByRole('option', { name: 'mock-subnet', exact: true }).click()
   await page
     .getByRole('dialog')