disable snapshot for local disks

Author: david-crespo

AGENTS.md

@@ -16,14 +16,15 @@
 
 # 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
 
 - 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

app/api/util.spec.ts

@@ -151,6 +151,13 @@ 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

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<string, DiskState['state'][]>
 
+// 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<Disk>) for use in MSW handlers
+type SnapshotDisk =
+  | Pick<Disk, 'state' | 'diskType'>
+  | { 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<Disk>, which we pass to it in the MSW handlers
-  const test = (d: Pick<Disk, 'state'>) => 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<Disk>, which we pass to it in the MSW handlers
+    const test = (d: Pick<Disk, 'state'>) => 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'

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',

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 },

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<Disk, 'state' | 'diskType'>): 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

mock-api/disk.ts

@@ -203,6 +203,19 @@ export const disks: Json<Disk>[] = [
     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')

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<Api.Snapshot> = {

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,20 @@ 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')