diff --git a/AGENTS.md b/AGENTS.md index cf3a75dd5..5b6aed583 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,7 @@ # API utilities & constants -- Treat `app/api/util.ts` (and friends) as a thin translation layer: mirror backend rules only when the UI needs them, keep the client copy minimal, and always link to the authoritative Omicron source so reviewers can verify the behavior. +- Treat `app/api/util.ts` (and friends) as a thin translation layer: mirror backend rules only when the UI needs them, keep the client copy minimal, and always link to the authoritative Omicron source so reviewers can verify the behavior. Only keep 7 chars of the commit hash in the URL. - API constants live in `app/api/util.ts` with links to Omicron source. # Testing code @@ -24,6 +24,7 @@ - Run local checks before sending PRs: `npm run lint`, `npm run tsc`, `npm test run`, and `npm run e2ec`; pass `-- --ui` for Playwright UI mode or project/name filters like `npm run e2ec -- instance -g 'boot disk'`. - 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. - 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`). - Co-locate Vitest specs next to the code they cover; use Testing Library utilities (`render`, `renderHook`, `fireEvent`, fake timers) to assert observable output rather than implementation details (`app/ui/lib/FileInput.spec.tsx`, `app/hooks/use-pagination.spec.ts`). - For sweeping styling changes, coordinate with the visual regression harness and follow `test/visual/README.md` for the workflow. @@ -115,3 +116,4 @@ - Check `app/util/*` for string formatting, date handling, IP parsing, etc. Check `types/util.d.ts` for type helpers. - Use `validateName` for resource names, `validateDescription` for descriptions, `validateIp`/`validateIpNet` for IPs. - Role helpers live in `app/api/roles.ts`. +- Use ts-pattern exhaustive match when doing conditional logic on union types to make sure all arms are handled diff --git a/app/api/util.spec.ts b/app/api/util.spec.ts index 51fdf260a..b21fe731b 100644 --- a/app/api/util.spec.ts +++ b/app/api/util.spec.ts @@ -151,6 +151,24 @@ test('diskCan', () => { expect(diskCan.delete({ state: { state: 'attached', instance: 'xyz' } })).toBe(false) expect(diskCan.delete({ state: { state: 'detached' } })).toBe(true) + // snapshot requires distributed disk type + expect(diskCan.snapshot({ state: { state: 'detached' }, diskType: 'distributed' })).toBe( + true + ) + expect( + diskCan.snapshot({ + state: { state: 'attached', instance: 'x' }, + diskType: 'distributed', + }) + ).toBe(true) + expect(diskCan.snapshot({ state: { state: 'creating' }, diskType: 'distributed' })).toBe( + false + ) + expect(diskCan.snapshot({ state: { state: 'detached' }, diskType: 'local' })).toBe(false) + expect( + diskCan.snapshot({ state: { state: 'attached', instance: 'x' }, diskType: 'local' }) + ).toBe(false) + // @ts-expect-error typechecker rejects actions that don't exist // eslint-disable-next-line @typescript-eslint/no-unused-expressions diskCan.abc diff --git a/app/api/util.ts b/app/api/util.ts index cd77823be..527edf756 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -7,12 +7,14 @@ */ import * as R from 'remeda' +import { match } from 'ts-pattern' import { bytesToGiB } from '~/util/units' import type { Disk, DiskState, + DiskType, Instance, InstanceState, Measurement, @@ -176,14 +178,12 @@ export function instanceAutoRestartingSoon(i: Instance) { ) } -const diskActions = { +const diskStateActions = { // this is a weird one because the list of states is dynamic and it includes // 'creating' in the unwind of the disk create saga, but does not include // 'creating' in the disk delete saga, which is what we care about // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/src/app/sagas/disk_delete.rs?plain=1#L110 delete: ['detached', 'faulted'], - // TODO: link to API source. It's hard to determine from the saga code what the rule is here. - snapshot: ['attached', 'detached'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L173-L176 attach: ['creating', 'detached'], // https://github.com/oxidecomputer/omicron/blob/6dd9802/nexus/db-queries/src/db/datastore/disk.rs#L313-L314 @@ -192,6 +192,27 @@ const diskActions = { setAsBootDisk: ['attached'], } satisfies Record +// snapshot has a type check in addition to state check +// https://github.com/oxidecomputer/omicron/blob/078f636/nexus/src/app/snapshot.rs#L100-L109 +// NOTE: .states only captures the state requirement; local disks cannot be +// snapshotted regardless of state +const snapshotStates: DiskState['state'][] = ['attached', 'detached'] +// accept both camelCase (Disk) and snake_case (Json) for use in MSW handlers +type SnapshotDisk = + | Pick + | { state: DiskState; disk_type: DiskType } +const canSnapshot = (d: SnapshotDisk) => { + const diskType = 'diskType' in d ? d.diskType : d.disk_type + return ( + snapshotStates.includes(d.state.state) && + match(diskType) + .with('distributed', () => true) + .with('local', () => false) + .exhaustive() + ) +} +canSnapshot.states = snapshotStates + export function diskTransitioning(diskState: DiskState['state']) { return ( diskState === 'attaching' || @@ -201,13 +222,16 @@ export function diskTransitioning(diskState: DiskState['state']) { ) } -export const diskCan = R.mapValues(diskActions, (states: DiskState['state'][]) => { - // only have to Pick because we want this to work for both Disk and - // Json, which we pass to it in the MSW handlers - const test = (d: Pick) => states.includes(d.state.state) - test.states = states - return test -}) +export const diskCan = { + ...R.mapValues(diskStateActions, (states: DiskState['state'][]) => { + // only have to Pick because we want this to work for both Disk and + // Json, which we pass to it in the MSW handlers + const test = (d: Pick) => states.includes(d.state.state) + test.states = states + return test + }), + snapshot: canSnapshot, +} /** Hard coded in the API, so we can hard code it here. */ export const FLEET_ID = '001de000-1334-4000-8000-000000000000' diff --git a/app/pages/project/disks/DisksPage.tsx b/app/pages/project/disks/DisksPage.tsx index 2660cd20c..f782f513f 100644 --- a/app/pages/project/disks/DisksPage.tsx +++ b/app/pages/project/disks/DisksPage.tsx @@ -41,7 +41,7 @@ import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' import type * as PP from '~/util/path-params' -import { fancifyStates } from '../instances/common' +import { fancifyStates, snapshotDisabledReason } from '../instances/common' export const handle = makeCrumb('Disks', (p) => pb.disks(getProjectSelector(p))) @@ -123,11 +123,7 @@ export default function DisksPage() { }, }) }, - disabled: !diskCan.snapshot(disk) && ( - <> - Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted - - ), + disabled: snapshotDisabledReason(disk), }, { label: 'Delete', diff --git a/app/pages/project/instances/StorageTab.tsx b/app/pages/project/instances/StorageTab.tsx index 163222927..b69fd2a84 100644 --- a/app/pages/project/instances/StorageTab.tsx +++ b/app/pages/project/instances/StorageTab.tsx @@ -12,7 +12,6 @@ import * as R from 'remeda' import { api, - diskCan, genName, instanceCan, q, @@ -42,7 +41,7 @@ import { EMBody, EmptyMessage } from '~/ui/lib/EmptyMessage' import { TableEmptyBox } from '~/ui/lib/Table' import { links } from '~/util/links' -import { fancifyStates } from './common' +import { snapshotDisabledReason } from './common' export async function clientLoader({ params }: LoaderFunctionArgs) { const { project, instance } = getInstanceSelector(params) @@ -147,9 +146,7 @@ export default function StorageTab() { const getSnapshotAction = useCallback( (disk: InstanceDisk) => ({ label: 'Snapshot', - disabled: !diskCan.snapshot(disk) && ( - <>Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted - ), + disabled: snapshotDisabledReason(disk), onActivate() { createSnapshot({ query: { project }, diff --git a/app/pages/project/instances/common.tsx b/app/pages/project/instances/common.tsx index 9f9f43e67..097a7580c 100644 --- a/app/pages/project/instances/common.tsx +++ b/app/pages/project/instances/common.tsx @@ -6,6 +6,9 @@ * Copyright Oxide Computer Company */ import { createContext, useContext, type ReactNode } from 'react' +import { match } from 'ts-pattern' + +import { diskCan, type Disk } from '@oxide/api' import { intersperse } from '~/util/array' import { invariant } from '~/util/invariant' @@ -19,6 +22,17 @@ const white = (s: string) => ( export const fancifyStates = (states: string[]) => intersperse(states.map(white), <>, , <> or ) +/** Returns a disabled reason if the disk cannot be snapshotted, false otherwise */ +export function snapshotDisabledReason(disk: Pick): ReactNode { + if (diskCan.snapshot(disk)) return false + return match(disk.diskType) + .with('distributed', () => ( + <>Only disks in state {fancifyStates(diskCan.snapshot.states)} can be snapshotted + )) + .with('local', () => 'Only distributed disks support snapshots') + .exhaustive() +} + type MetricsContextValue = { startTime: Date endTime: Date diff --git a/mock-api/disk.ts b/mock-api/disk.ts index 4178dd9cd..260729e1b 100644 --- a/mock-api/disk.ts +++ b/mock-api/disk.ts @@ -203,6 +203,19 @@ export const disks: Json[] = [ block_size: 2048, disk_type: 'distributed', }, + { + id: 'b8e3de3a-3c97-4f23-a3f3-73e7d3d3b9c1', + name: 'local-disk', + description: 'A local disk that cannot be snapshotted', + project_id: project.id, + time_created: new Date().toISOString(), + time_modified: new Date().toISOString(), + state: { state: 'detached' }, + device_path: '/local', + size: 12 * GiB, + block_size: 2048, + disk_type: 'local', + }, // put a ton of disks in project 2 so we can use it to test comboboxes ...Array.from({ length: 1010 }).map((_, i) => { const numStr = (i + 1).toString().padStart(4, '0') diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 3f678f5c2..50cd157b7 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -1126,7 +1126,10 @@ export const handlers = makeHandlers({ const disk = lookup.disk({ ...query, disk: body.disk }) if (!diskCan.snapshot(disk)) { - throw 'Cannot snapshot disk in state ' + disk.state.state + throw match(disk.disk_type) + .with('distributed', () => 'Cannot snapshot disk in state ' + disk.state.state) + .with('local', () => 'Only distributed disks support snapshots') + .exhaustive() } const newSnapshot: Json = { diff --git a/test/e2e/disks.e2e.ts b/test/e2e/disks.e2e.ts index cb1001614..fa59fde89 100644 --- a/test/e2e/disks.e2e.ts +++ b/test/e2e/disks.e2e.ts @@ -30,7 +30,7 @@ test('List disks and snapshot', async ({ page }) => { await page.goto('/projects/mock-project/disks') const table = page.getByRole('table') - await expect(table.getByRole('row')).toHaveCount(12) // 11 + header + await expect(table.getByRole('row')).toHaveCount(13) // 12 + header // check one attached and one not attached await expectRowVisible(table, { @@ -68,6 +68,22 @@ test('Disk snapshot error', async ({ page }) => { await expectToast(page, 'Failed to create snapshotCannot snapshot disk') }) +test('Local disk snapshot disabled', async ({ page }) => { + await page.goto('/projects/mock-project/disks') + + const row = page.getByRole('row', { name: 'local-disk', exact: false }) + await row.getByRole('button', { name: 'Row actions' }).click() + + const snapshotItem = page.getByRole('menuitem', { name: 'Snapshot' }) + await expect(snapshotItem).toBeDisabled() + + // hover to see tooltip with disabled reason + await snapshotItem.hover() + await expect(page.getByRole('tooltip')).toHaveText( + 'Only distributed disks support snapshots' + ) +}) + test.describe('Disk create', () => { test.beforeEach(async ({ page }) => { await page.goto('/projects/mock-project/disks-new')