From 9914deba4ed48da3aaaba9a0e9d2d4cd61dc7a69 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 22 May 2026 14:55:37 -0500 Subject: [PATCH] display API error messages during image upload --- app/api/__tests__/errors.spec.ts | 2 +- app/components/form/SideModalForm.tsx | 8 ++--- app/forms/image-upload.tsx | 29 +++++++++++++---- mock-api/msw/handlers.ts | 24 +++++++++++--- mock-api/msw/util.ts | 12 +++++-- test/e2e/image-upload.e2e.ts | 46 +++++++++++++++++++++------ 6 files changed, 94 insertions(+), 27 deletions(-) diff --git a/app/api/__tests__/errors.spec.ts b/app/api/__tests__/errors.spec.ts index a30ae58647..32cdd0b004 100644 --- a/app/api/__tests__/errors.spec.ts +++ b/app/api/__tests__/errors.spec.ts @@ -42,7 +42,6 @@ describe('processServerError', () => { }, } expect(processServerError('fakeThingView', parseError)).toEqual({ - code: undefined, message: 'Hi, you have an error', statusCode: 400, errorCode: undefined, @@ -60,6 +59,7 @@ describe('processServerError', () => { expect(processServerError('fakeThingView', clientError)).toEqual({ message: 'Error reading API response', statusCode: 200, + requestId: undefined, }) }) diff --git a/app/components/form/SideModalForm.tsx b/app/components/form/SideModalForm.tsx index 0d7e14bbf7..dc2ba135a1 100644 --- a/app/components/form/SideModalForm.tsx +++ b/app/components/form/SideModalForm.tsx @@ -9,8 +9,6 @@ import { useEffect, useId, useState, type ReactNode } from 'react' import type { FieldValues, UseFormReturn } from 'react-hook-form' -import type { ApiError } from '@oxide/api' - import { useShouldAnimateModal } from '~/hooks/use-should-animate-modal' import { Button } from '~/ui/lib/Button' import { Modal } from '~/ui/lib/Modal' @@ -39,8 +37,10 @@ type SideModalFormProps = { // require loading and error so we can't forget to hook them up. there are a // few forms that don't need them, so we'll use dummy values - /** Error from the API call */ - submitError: ApiError | null + // would be ApiError except image-upload synthesizes a local "name already + // exists" error that isn't from the API. The modal only reads message and + // errorCode anyway. + submitError: { message: string; errorCode?: string } | null loading: boolean /** Only needed if you need to override the default title (Create/Edit ${resourceName}) */ diff --git a/app/forms/image-upload.tsx b/app/forms/image-upload.tsx index 93e92573d6..f47d8b673c 100644 --- a/app/forms/image-upload.tsx +++ b/app/forms/image-upload.tsx @@ -18,7 +18,6 @@ import { q, queryClient, useApiMutation, - type ApiError, type BlockSize, type Disk, type Snapshot, @@ -54,6 +53,24 @@ import { formatBytes, GiB, KiB } from '~/util/units' // Padded because otherwise the numbers jump around a bit, e.g., when it goes // from 10.55 to 14.7 to 19.23 const fsize = (bytes: number) => formatBytes(bytes, { pad: true }).label +const genericUploadErrorMessage = 'Something went wrong. Please try again.' + +function getUploadErrorMessage(e: unknown): string { + if (!e || typeof e !== 'object') return genericUploadErrorMessage + const errorCode = 'errorCode' in e ? e.errorCode : undefined + const message = 'message' in e ? e.message : undefined + + // Mutation errors are ApiErrors with user-facing messages from + // processServerError. Show the API message only when it's actually + // user-facing: errorCode "Internal" maps to dropshot's generic 500 + // ("Internal Server Error") with no real detail, and a missing message + // means we got nothing to display. + return typeof errorCode === 'string' && + typeof message === 'string' && + errorCode !== 'Internal' + ? message + : genericUploadErrorMessage +} type FormValues = { imageName: string @@ -132,6 +149,7 @@ function getTmpDiskName(imageName: string) { // do the right thing in const specialNames = new Set([ 'disk-create-500', + 'disk-create-quota', 'import-start-500', 'import-stop-500', 'disk-finalize-500', @@ -195,7 +213,7 @@ export default function ImageCreate() { // are submitting, we rely on the RQ mutations themselves, plus a synthetic // mutation state representing the many calls of the bulk upload step. - const [formError, setFormError] = useState(null) + const [formError, setFormError] = useState<{ message: string } | null>(null) const [modalOpen, setModalOpen] = useState(false) const [modalError, setModalError] = useState(null) @@ -536,10 +554,7 @@ export default function ImageCreate() { if (image) { // TODO: set this error on the field instead of the whole form // TODO: make setError available here somehow :( - setFormError({ - errorCode: 'ObjectAlreadyExists', - message: 'Image name already exists', - }) + setFormError({ message: 'Image name already exists' }) return } @@ -552,7 +567,7 @@ export default function ImageCreate() { } catch (e) { if (e !== ABORT_ERROR) { console.error(e) - setModalError('Something went wrong. Please try again.') + setModalError(getUploadErrorMessage(e)) // abort anything in flight in case cancelEverything() } diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 96a0dd31e4..d5515b8951 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -55,6 +55,7 @@ import { getBlockSize, handleMetrics, handleOxqlMetrics, + internalError, invalidRequest, ipRangeLen, NotImplemented, @@ -157,7 +158,22 @@ export const handlers = makeHandlers({ errIfExists(db.disks, { name: body.name, project_id: project.id }) - if (body.name === 'disk-create-500') throw 500 + if (body.name === 'disk-create-500') throw internalError('disk create failed') + + // Mirrors the InsufficientCapacity error from omicron's virtual + // provisioning collection update when a project's storage quota is + // exceeded. See NOT_ENOUGH_STORAGE_SENTINEL in + // https://github.com/oxidecomputer/omicron/blob/aa608203/nexus/db-queries/src/db/queries/virtual_provisioning_collection_update.rs#L61-L67 + if (body.name === 'disk-create-quota') { + throw json( + { + error_code: 'InsufficientCapacity', + message: + 'Storage Limit Exceeded: Not enough storage to complete request. Either remove unneeded disks and snapshots to free up resources or contact the rack operator to request a capacity increase.', + }, + { status: 507 } + ) + } const { name, description, size, disk_backend } = body const diskSource = disk_backend.type === 'distributed' ? disk_backend.disk_source : null @@ -220,7 +236,7 @@ export const handlers = makeHandlers({ async diskBulkWriteImportStart({ path, query }) { const disk = lookup.disk({ ...path, ...query }) - if (disk.name === 'import-start-500') throw 500 + if (disk.name === 'import-start-500') throw internalError('import start failed') if (disk.state.state !== 'import_ready') { throw 'Can only enter state importing_from_bulk_write from import_ready' @@ -235,7 +251,7 @@ export const handlers = makeHandlers({ async diskBulkWriteImportStop({ path, query }) { const disk = lookup.disk({ ...path, ...query }) - if (disk.name === 'import-stop-500') throw 500 + if (disk.name === 'import-stop-500') throw internalError('import stop failed') if (disk.state.state !== 'importing_from_bulk_writes') { throw 'Can only stop import for disk in state importing_from_bulk_write' @@ -258,7 +274,7 @@ export const handlers = makeHandlers({ diskFinalizeImport: ({ path, query, body }) => { const disk = lookup.disk({ ...path, ...query }) - if (disk.name === 'disk-finalize-500') throw 500 + if (disk.name === 'disk-finalize-500') throw internalError('disk finalize failed') if (disk.state.state !== 'import_ready') { throw `Cannot finalize disk in state ${disk.state.state}. Must be import_ready.` diff --git a/mock-api/msw/util.ts b/mock-api/msw/util.ts index 13ccecc282..f3f88a2f18 100644 --- a/mock-api/msw/util.ts +++ b/mock-api/msw/util.ts @@ -113,8 +113,16 @@ export const NotImplemented = () => { export const invalidRequest = (message: string) => json({ error_code: 'InvalidRequest', message }, { status: 400 }) -export const internalError = (message: string) => - json({ error_code: 'InternalError', message }, { status: 500 }) +// 500s in Omicron come from Error::InternalError, which turns into dropshot's +// `for_internal_error`, which sets error_code "Internal" and a external message +// of "Internal Server Error". It also has an `internal_message` that gets +// logged but isn't sent to clients. We imitate that here. +// https://github.com/oxidecomputer/omicron/blob/985304a/common/src/api/external/error.rs#L474-L476 +// https://github.com/oxidecomputer/dropshot/blob/9b431d1/dropshot/src/error.rs#L229-L241 +export const internalError = (internalMessage?: string) => { + if (internalMessage) console.error(internalMessage) + return json({ error_code: 'Internal', message: 'Internal Server Error' }, { status: 500 }) +} export const errIfExists = >( collection: T[], diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index fcfd9476a3..0f7618328b 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -262,14 +262,39 @@ test.describe('Image upload', () => { await expectUploadProcess(page) }) + // generic 500s (errorCode "Internal") have no useful API message, so we fall + // back to a generic one; specific error codes like InsufficientCapacity carry + // a real user-facing message that we surface verbatim. + const genericMessage = 'Something went wrong. Please try again.' const failureCases = [ - { imageName: 'disk-create-500', stepText: 'Create temporary disk' }, - { imageName: 'import-start-500', stepText: 'Put disk in import mode' }, - { imageName: 'import-stop-500', stepText: 'Get disk out of import mode' }, - { imageName: 'disk-finalize-500', stepText: 'Finalize disk and create snapshot' }, + { + imageName: 'disk-create-500', + stepText: 'Create temporary disk', + message: genericMessage, + }, + { + imageName: 'disk-create-quota', + stepText: 'Create temporary disk', + message: 'Storage Limit Exceeded', + }, + { + imageName: 'import-start-500', + stepText: 'Put disk in import mode', + message: genericMessage, + }, + { + imageName: 'import-stop-500', + stepText: 'Get disk out of import mode', + message: genericMessage, + }, + { + imageName: 'disk-finalize-500', + stepText: 'Finalize disk and create snapshot', + message: genericMessage, + }, ] - for (const { imageName, stepText } of failureCases) { + for (const { imageName, stepText, message } of failureCases) { test(`failure ${imageName}`, async ({ page, browserName }) => { // eslint-disable-next-line playwright/no-skipped-test test.skip(browserName === 'webkit', 'safari. stop this') @@ -280,10 +305,13 @@ test.describe('Image upload', () => { const step = page.getByTestId(`upload-step: ${stepText}`) await expect(step).toHaveAttribute('data-status', 'error', { timeout: 15000 }) - await expectVisible(page, [ - 'text="Something went wrong. Please try again."', - 'role=button[name="Back"]', - ]) + + await expect(page.getByText(message)).toBeVisible() + // confirm we don't show both the generic and a specific API message + if (message !== genericMessage) { + await expect(page.getByText(genericMessage)).toBeHidden() + } + await expect(page.getByRole('button', { name: 'Back' })).toBeVisible() }) } })