Skip to content

Commit b9cea7e

Browse files
authored
Fix error handling in disk attach modal (#3346)
Fixes two bugs found while doing #3345: 1. **Silent attach failure in the create-then-attach flow.** In the unlikely event you create a disk from the storage tab and the subsequent attach call fails (e.g., the instance started in the meantime), there was no visible error: the error only rendered inside the attach modal, which isn't open anymore. You'd get the "Disk created" toast but the disk just wouldn't be attached. Now it shows an error toast. 2. **Stale error leaking between modal uses.** In that failure scenario, the error stuck to the shared mutation, so the *next* time you opened the "Attach existing disk" modal, it displayed the old error from the create flow before you'd done anything. Now the attach modal's mutation is its own, created fresh on each open. Moving the mutation inside its own component means we don't have to do `reset()`s to avoid holding onto state across different openings of the attach modal.
1 parent 4464677 commit b9cea7e

2 files changed

Lines changed: 55 additions & 21 deletions

File tree

app/forms/disk-attach.tsx

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,55 @@ import { useQuery } from '@tanstack/react-query'
99
import { useMemo } from 'react'
1010
import { useForm } from 'react-hook-form'
1111

12-
import { api, q, type ApiError, type DiskType } from '@oxide/api'
12+
import {
13+
api,
14+
q,
15+
queryClient,
16+
useApiMutation,
17+
type ApiError,
18+
type DiskType,
19+
} from '@oxide/api'
1320

1421
import { ComboboxField } from '~/components/form/fields/ComboboxField'
1522
import { ModalForm } from '~/components/form/ModalForm'
16-
import { useProjectSelector } from '~/hooks/use-params'
23+
import { HL } from '~/components/HL'
24+
import { useInstanceSelector, useProjectSelector } from '~/hooks/use-params'
25+
import { addToast } from '~/stores/toast'
1726
import { toComboboxItems } from '~/ui/lib/Combobox'
1827
import { ALL_ISH } from '~/util/consts'
1928

2029
const defaultValues = { name: '' }
2130

31+
/**
32+
* Attach modal for the instance storage tab. Owns the attach mutation so its
33+
* loading and error state can't outlive the modal. `AttachDiskModalForm` below
34+
* stays mutation-free because the instance create form also uses it (with a
35+
* setState `onSubmit`) on a route where no instance exists yet.
36+
*/
37+
export function AttachDiskModal({ onDismiss }: { onDismiss: () => void }) {
38+
const { project, instance } = useInstanceSelector()
39+
40+
const attachDisk = useApiMutation(api.instanceDiskAttach, {
41+
onSuccess(disk) {
42+
queryClient.invalidateEndpoint('instanceDiskList')
43+
onDismiss()
44+
// prettier-ignore
45+
addToast(<>Disk <HL>{disk.name}</HL> attached</>)
46+
},
47+
})
48+
49+
return (
50+
<AttachDiskModalForm
51+
onDismiss={onDismiss}
52+
onSubmit={({ name }) => {
53+
attachDisk.mutate({ path: { instance }, query: { project }, body: { disk: name } })
54+
}}
55+
loading={attachDisk.isPending}
56+
submitError={attachDisk.error}
57+
/>
58+
)
59+
}
60+
2261
type AttachDiskProps = {
2362
/** If defined, this overrides the usual mutation */
2463
onSubmit: (diskAttach: { name: string; size: number; diskType: DiskType }) => void

app/pages/project/instances/StorageTab.tsx

Lines changed: 14 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import { Storage24Icon } from '@oxide/design-system/icons/react'
2525

2626
import { HL } from '~/components/HL'
2727
import { DiskStateBadge, DiskTypeBadge, ReadOnlyBadge } from '~/components/StateBadge'
28-
import { AttachDiskModalForm } from '~/forms/disk-attach'
28+
import { AttachDiskModal } from '~/forms/disk-attach'
2929
import { CreateDiskSideModalForm } from '~/forms/disk-create'
3030
import { getInstanceSelector, useInstanceSelector } from '~/hooks/use-params'
3131
import { useQuickActions } from '~/hooks/use-quick-actions'
@@ -320,14 +320,22 @@ export default function StorageTab() {
320320
]
321321
)
322322

323-
const attachDisk = useApiMutation(api.instanceDiskAttach, {
323+
// attach step of the create-then-attach flow only; the attach modal owns its
324+
// own mutation. The create modal closes on create success, so this runs in
325+
// the background and failures must surface as a toast.
326+
const attachCreatedDisk = useApiMutation(api.instanceDiskAttach, {
324327
onSuccess(disk) {
325328
queryClient.invalidateEndpoint('instanceDiskList')
326-
setShowDiskCreate(false)
327-
setShowDiskAttach(false)
328329
// prettier-ignore
329330
addToast(<>Disk <HL>{disk.name}</HL> attached</>)
330331
},
332+
onError(err, variables) {
333+
addToast({
334+
title: `Failed to attach disk ${variables.body.disk}`,
335+
content: err.message,
336+
variant: 'error',
337+
})
338+
},
331339
})
332340

333341
const bootDisksTable = useReactTable({
@@ -434,24 +442,11 @@ export default function StorageTab() {
434442
onSuccess={({ name }) => {
435443
// TODO: this should probably be done with `mutateAsync` and
436444
// awaited, but it's a pain, so punt for now
437-
attachDisk.mutate({ ...instancePathQuery, body: { disk: name } })
438-
}}
439-
/>
440-
)}
441-
{showDiskAttach && (
442-
<AttachDiskModalForm
443-
onDismiss={() => {
444-
setShowDiskAttach(false)
445-
// clear API errors on the mutation
446-
attachDisk.reset()
447-
}}
448-
onSubmit={({ name }) => {
449-
attachDisk.mutate({ ...instancePathQuery, body: { disk: name } })
445+
attachCreatedDisk.mutate({ ...instancePathQuery, body: { disk: name } })
450446
}}
451-
loading={attachDisk.isPending}
452-
submitError={attachDisk.error}
453447
/>
454448
)}
449+
{showDiskAttach && <AttachDiskModal onDismiss={() => setShowDiskAttach(false)} />}
455450
{selectedDisk && (
456451
<DiskDetailSideModal
457452
disk={selectedDisk}

0 commit comments

Comments
 (0)