diff --git a/AGENTS.md b/AGENTS.md index fb0f63eda..5c84bca26 100644 --- a/AGENTS.md +++ b/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`). diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index f5ba59c6d..56340d04f 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -487,7 +487,7 @@ const ProtocolFilters = ({ control }: { control: Control }) control={protocolForm.control} description={ <> - Enter a code (0) or range (e.g. 1–3). Leave blank for all + Enter a code (0) or range (e.g., 1–3). Leave blank for all traffic of type {selectedIcmpType}. } diff --git a/app/forms/floating-ip-edit.tsx b/app/forms/floating-ip-edit.tsx index abcae22aa..960f2e71e 100644 --- a/app/forms/floating-ip-edit.tsx +++ b/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() { - {instanceName ? ( - - {instanceName} - - ) : ( - - )} + diff --git a/app/pages/project/disks/DisksPage.tsx b/app/pages/project/disks/DisksPage.tsx index 96b6ae9b2..ad426fafa 100644 --- a/app/pages/project/disks/DisksPage.tsx +++ b/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) => , + cell: (info) => ( + + ), } ), colHelper.accessor('diskType', { diff --git a/app/pages/project/floating-ips/FloatingIpsPage.tsx b/app/pages/project/floating-ips/FloatingIpsPage.tsx index 2154ef875..5929d8754 100644 --- a/app/pages/project/floating-ips/FloatingIpsPage.tsx +++ b/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) => , + cell: (info) => , }), ] @@ -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] diff --git a/app/table/cells/InstanceLinkCell.tsx b/app/table/cells/InstanceLinkCell.tsx index 59b6d71f0..491797010 100644 --- a/app/table/cells/InstanceLinkCell.tsx +++ b/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 if (!instance) return - return ( - + const params = { project, instance: instance.name } + const to = + tab === 'networking' ? pb.instanceNetworking(params) : pb.instanceStorage(params) + + return cell ? ( + {instance.name} + ) : ( + {instance.name} - + ) } diff --git a/app/table/cells/IpPoolCell.tsx b/app/table/cells/IpPoolCell.tsx index e4cb6ba4d..fc234191b 100644 --- a/app/table/cells/IpPoolCell.tsx +++ b/app/table/cells/IpPoolCell.tsx @@ -26,8 +26,8 @@ export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => { ) ) if (!result) return - // 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 const pool = result.data return ( diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index cd42a33e3..c48e9768b 100644 --- a/mock-api/msw/handlers.ts +++ b/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, diff --git a/test/e2e/floating-ip-update.e2e.ts b/test/e2e/floating-ip-update.e2e.ts index 4ce1179c9..065358f48 100644 --- a/test/e2e/floating-ip-update.e2e.ts +++ b/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() diff --git a/test/e2e/instance-networking.e2e.ts b/test/e2e/instance-networking.e2e.ts index 0c1806433..91d8a09de 100644 --- a/test/e2e/instance-networking.e2e.ts +++ b/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') diff --git a/test/e2e/network-interface-create.e2e.ts b/test/e2e/network-interface-create.e2e.ts index 206b1fd4d..9d54f0994 100644 --- a/test/e2e/network-interface-create.e2e.ts +++ b/test/e2e/network-interface-create.e2e.ts @@ -22,7 +22,7 @@ test('can create a NIC with a specified IP address', async ({ page }) => { await page.getByLabel('Name').fill('nic-1') await page.getByLabel('VPC', { exact: true }).click() await page.getByRole('option', { name: 'mock-vpc' }).click() - await page.getByRole('button', { name: 'Subnet' }).click() + await page.getByRole('dialog').getByRole('button', { name: 'Subnet' }).click() await page.getByRole('option', { name: 'mock-subnet', exact: true }).click() // Select IPv4 only @@ -52,7 +52,7 @@ test('can create a NIC with a blank IP address', async ({ page }) => { await page.getByLabel('Name').fill('nic-2') await page.getByLabel('VPC', { exact: true }).click() await page.getByRole('option', { name: 'mock-vpc' }).click() - await page.getByRole('button', { name: 'Subnet' }).click() + await page.getByRole('dialog').getByRole('button', { name: 'Subnet' }).click() await page.getByRole('option', { name: 'mock-subnet', exact: true }).click() // Dual-stack is selected by default, so both fields should be visible @@ -92,7 +92,7 @@ test('can create a NIC with IPv6 only', async ({ page }) => { await page.getByLabel('Name').fill('nic-3') await page.getByLabel('VPC', { exact: true }).click() await page.getByRole('option', { name: 'mock-vpc' }).click() - await page.getByRole('button', { name: 'Subnet' }).click() + await page.getByRole('dialog').getByRole('button', { name: 'Subnet' }).click() await page.getByRole('option', { name: 'mock-subnet', exact: true }).click() // Select IPv6 only @@ -117,7 +117,7 @@ test('can create a NIC with dual-stack and explicit IPs', async ({ page }) => { await page.getByLabel('Name').fill('nic-4') await page.getByLabel('VPC', { exact: true }).click() await page.getByRole('option', { name: 'mock-vpc' }).click() - await page.getByRole('button', { name: 'Subnet' }).click() + await page.getByRole('dialog').getByRole('button', { name: 'Subnet' }).click() await page.getByRole('option', { name: 'mock-subnet', exact: true }).click() // Dual-stack is selected by default diff --git a/test/e2e/z-index.e2e.ts b/test/e2e/z-index.e2e.ts index 175bc554b..4c6c3c1a1 100644 --- a/test/e2e/z-index.e2e.ts +++ b/test/e2e/z-index.e2e.ts @@ -25,7 +25,7 @@ test('Dropdown content in SidebarModal shows on screen', async ({ page }) => { // clickable means they are not obscured due to having a too-low z-index await page.getByLabel('VPC', { exact: true }).click() await page.getByRole('option', { name: 'mock-vpc' }).click() - await page.getByRole('button', { name: 'Subnet' }).click() + await page.getByRole('dialog').getByRole('button', { name: 'Subnet' }).click() await page.getByRole('option', { name: 'mock-subnet', exact: true }).click() const sidebar = page.getByRole('dialog', { name: 'Add network interface' })