Nicer handling of expected errors in the browser console

app/api/client.ts

@@ -82,13 +82,27 @@ export type ResultsPage<TItem> = { items: TItem[]; nextPage?: string | null }
 
 type HandledResult<T> = { type: 'success'; data: T } | { type: 'error'; data: ApiError }
 
+type ExpectedError = {
+  /**
+   * Why this error response is expected at the call site.
+   * Logged after "This error is expected: ", so this should be a sentence
+   * fragment starting lowercase and ending with punctuation, e.g.,
+   * "404 means the image name is not taken."
+   */
+  explanation: string
+  /** Expected HTTP status for the handled error case */
+  statusCode: number
+}
+
+const expectedErrorLabel = ({ statusCode }: ExpectedError) => `status ${statusCode}`
+
 // method: keyof Api would be strictly more correct, but making it a string
 // means we can call this directly in all the spots below instead of having to
 // make it generic over Api, which requires passing it as an argument to
 // getUseApiQuery, etc. This is fine because it is only being called inside
 // functions where `method` is already required to be an API method.
 const handleResult =
-  (method: string) =>
+  ({ method, errorsExpected }: { method: string; errorsExpected?: ExpectedError }) =>
   <Data>(result: ApiResult<Data>): HandledResult<Data> => {
     if (result.type === 'success') return { type: 'success', data: result.data }
 
@@ -111,15 +125,40 @@ const handleResult =
       const consolePage = window.location.pathname + window.location.search
       // TODO: need to change oxide.ts to put the HTTP method on the result in
       // order to log it here
-      console.error(
-        `More info about API ${error.statusCode || 'error'} on ${consolePage}
-
-API URL:        ${result.response.url}
+      const matchesExpectedError =
+        !!errorsExpected && error.statusCode === errorsExpected.statusCode
+      const logFn = matchesExpectedError ? console.info : console.error
+      const details = `API URL:        ${result.response.url}
 Request ID:     ${error.requestId}
 Error code:     ${error.errorCode}
-Error message:  ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message:  '.length))}
+Error message:  ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message:  '.length))}`
+      if (matchesExpectedError) {
+        logFn(
+          `API ${error.statusCode || 'error'} on ${consolePage}
+
+%cThis error is expected: %c${errorsExpected.explanation}
+Expected:       ${expectedErrorLabel(errorsExpected)}
+
+${details}
+`,
+          'font-weight: bold',
+          'font-weight: normal'
+        )
+      } else {
+        const mismatchDetails = errorsExpected
+          ? `
+Expected:       ${expectedErrorLabel(errorsExpected)}
+Reason:         ${errorsExpected.explanation}
 `
-      )
+          : ''
+        logFn(
+          `More info about API ${error.statusCode || 'error'} on ${consolePage}
+
+${mismatchDetails}${details}
+
+`
+        )
+      }
     }
 
     return { type: 'error', data: error }
@@ -234,9 +273,17 @@ export function ensurePrefetched<TData, TError>(
 export const q = <Params, Data>(
   f: (p: Params) => Promise<ApiResult<Data>>,
   params: Params,
-  options: UseQueryOtherOptions<Data> = {}
-) =>
-  queryOptions({
+  options: UseQueryOtherOptions<Data> & {
+    /**
+     * Expected error details. Matching errors are logged as `console.info`
+     * with the explanation; mismatches remain `console.error`.
+     * The explanation is rendered after "This error is expected: ".
+     */
+    errorsExpected?: ExpectedError
+  } = {}
+) => {
+  const { errorsExpected, ...rqOptions } = options
+  return queryOptions({
     // method name first means we can invalidate all calls to this endpoint by
     // invalidating [f.name] (see invalidateEndpoint)
     queryKey: [f.name, params],
@@ -248,7 +295,7 @@ export const q = <Params, Data>(
     // in the middle of prefetching
     queryFn: () =>
       f(params)
-        .then(handleResult(f.name))
+        .then(handleResult({ method: f.name, errorsExpected }))
         .then((result) => {
           if (result.type === 'success') return result.data
           throw result.data
@@ -258,8 +305,9 @@ export const q = <Params, Data>(
     // up as `error` state instead, pass `throwOnError: false` as an
     // option from the calling component and it will override this
     throwOnError: (err) => err.statusCode === 404,
-    ...options,
+    ...rqOptions,
   })
+}
 
 const ERRORS_ALLOWED = 'errors-allowed'
 
@@ -282,21 +330,30 @@ const ERRORS_ALLOWED = 'errors-allowed'
 export const qErrorsAllowed = <Params, Data>(
   f: (p: Params) => Promise<ApiResult<Data>>,
   params: Params,
-  options: UseQueryOtherOptions<HandledResult<Data>> = {}
-) =>
-  queryOptions({
+  options: UseQueryOtherOptions<HandledResult<Data>> & {
+    /**
+     * Expected error details. Matching errors are logged as `console.info`
+     * with the explanation; mismatches remain `console.error`.
+     * The explanation is rendered after "This error is expected: ".
+     */
+    errorsExpected: ExpectedError
+  }
+) => {
+  const { errorsExpected, ...rqOptions } = options
+  return queryOptions({
     // extra bit of key is important to distinguish from normal query. if we
     // hit a given endpoint twice on the same page, once the normal way and
     // once with errors allowed the responses have different shapes, so we do
     // not want to share the cache and mix them up
     queryKey: [f.name, params, ERRORS_ALLOWED],
-    queryFn: () => f(params).then(handleResult(f.name)),
+    queryFn: () => f(params).then(handleResult({ method: f.name, errorsExpected })),
     // No point having throwOnError because errors do not throw. Worth
     // considering still throwing for particular errors: sometimes we expect
     // a 403, other times we expect 404s. We could take a list of acceptable
     // status codes.
-    ...options,
+    ...rqOptions,
   })
+}
 
 // Unlike the query one, we don't need this to go through an options object
 // because we are not calling the mutation in two spots and sharing the options
@@ -326,7 +383,7 @@ export const useApiMutation = <Params, Data>(
       // us back Params, but TS can't prove Omit<Params & {signal?}, 'signal'>
       // === Params structurally.
       f(params as Params, { signal: __signal })
-        .then(handleResult(f.name))
+        .then(handleResult({ method: f.name }))
         .then((result) => {
           if (result.type === 'success') return result.data
           throw result.data

app/components/AttachFloatingIpModal.tsx

@@ -27,7 +27,16 @@ import { ModalForm } from './form/ModalForm'
 
 function IpPoolName({ ipPoolId }: { ipPoolId: string }) {
   const { data: result } = useQuery(
-    qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } })
+    qErrorsAllowed(
+      api.ipPoolView,
+      { path: { pool: ipPoolId } },
+      {
+        errorsExpected: {
+          explanation: 'the referenced IP pool may have been deleted.',
+          statusCode: 404,
+        },
+      }
+    )
   )
   // As with IpPoolCell, this should never happen, but to be safe …
   if (!result || result.type === 'error') return null

app/forms/image-upload.tsx

@@ -517,19 +517,20 @@ export default function ImageCreate() {
         // check that image name isn't taken before starting the whole thing
         const image = await queryClient
           .fetchQuery(
-            q(api.imageView, {
-              path: { image: values.imageName },
-              query: { project },
-            })
+            q(
+              api.imageView,
+              { path: { image: values.imageName }, query: { project } },
+              {
+                errorsExpected: {
+                  explanation: 'the image name may not exist yet.',
+                  statusCode: 404,
+                },
+              }
+            )
           )
           .catch((e) => {
             // eat a 404 since that's what we want. anything else should still blow up
-            if (e.statusCode === 404) {
-              console.info(
-                '/v1/images 404 is expected. It means the image name is not taken.'
-              )
-              return null
-            }
+            if (e.statusCode === 404) return null
             throw e
           })
         if (image) {

app/pages/project/instances/NetworkingTab.tsx

@@ -130,11 +130,18 @@ export async function clientLoader({ params }: LoaderFunctionArgs) {
       .fetchQuery(q(api.ipPoolList, { query: { limit: ALL_ISH } }))
       .then((pools) => {
         for (const pool of pools.items) {
-          // both IpPoolCell and the fetch in the model use errors-allowed
+          // both IpPoolCell and the fetch in the modal use errors-allowed
           // versions to avoid blowing up in the unlikely event of an error
-          const { queryKey } = qErrorsAllowed(api.ipPoolView, {
-            path: { pool: pool.id },
-          })
+          const { queryKey } = qErrorsAllowed(
+            api.ipPoolView,
+            { path: { pool: pool.id } },
+            {
+              errorsExpected: {
+                explanation: 'the referenced IP pool may have been deleted.',
+                statusCode: 404,
+              },
+            }
+          )
           queryClient.setQueryData(queryKey, { type: 'success', data: pool })
         }
       }),

app/pages/project/snapshots/SnapshotsPage.tsx

@@ -41,14 +41,26 @@ import { TableActions } from '~/ui/lib/Table'
 import { docLinks } from '~/util/links'
 import { pb } from '~/util/path-builder'
 
+const diskViewErrorsAllowedQ = (disk: string) =>
+  qErrorsAllowed(
+    api.diskView,
+    { path: { disk } },
+    {
+      errorsExpected: {
+        explanation: 'the source disk may have been deleted.',
+        statusCode: 404,
+      },
+    }
+  )
+
 const DiskNameFromId = ({
   value,
   onClick,
 }: {
   value: string
   onClick: (disk: Disk) => void
 }) => {
-  const { data } = useQuery(qErrorsAllowed(api.diskView, { path: { disk: value } }))
+  const { data } = useQuery(diskViewErrorsAllowedQ(value))
 
   if (!data) return <SkeletonCell />
   if (data.type === 'error') return <Badge color="neutral">Deleted</Badge>
@@ -85,10 +97,10 @@ export async function clientLoader({ params }: LoaderFunctionArgs) {
       .fetchQuery(q(api.diskList, { query: { project, limit: 200 } }))
       .then((disks) => {
         for (const disk of disks.items) {
-          queryClient.setQueryData(
-            qErrorsAllowed(api.diskView, { path: { disk: disk.id } }).queryKey,
-            { type: 'success', data: disk }
-          )
+          queryClient.setQueryData(diskViewErrorsAllowedQ(disk.id).queryKey, {
+            type: 'success',
+            data: disk,
+          })
         }
       }),
   ])

app/pages/system/silos/SiloScimTab.tsx

@@ -44,6 +44,18 @@ import { docLinks } from '~/util/links'
 
 export const handle = makeCrumb('SCIM')
 
+const scimTokenListErrorsAllowedQ = (silo: string) =>
+  qErrorsAllowed(
+    api.scimTokenList,
+    { query: { silo } },
+    {
+      errorsExpected: {
+        explanation: 'the current user may not have permission to list SCIM tokens.',
+        statusCode: 403,
+      },
+    }
+  )
+
 const colHelper = createColumnHelper<ScimClientBearerToken>()
 const staticColumns = [
   colHelper.accessor('id', {
@@ -74,7 +86,7 @@ const EmptyState = () => (
 export async function clientLoader({ params }: LoaderFunctionArgs) {
   const { silo } = getSiloSelector(params)
   // Use errors-allowed approach so 403s don't throw and break the loader
-  await queryClient.prefetchQuery(qErrorsAllowed(api.scimTokenList, { query: { silo } }))
+  await queryClient.prefetchQuery(scimTokenListErrorsAllowedQ(silo))
   return null
 }
 
@@ -86,7 +98,7 @@ type ModalState =
 export default function SiloScimTab() {
   const siloSelector = useSiloSelector()
   const { data: tokensResult } = usePrefetchedQuery(
-    qErrorsAllowed(api.scimTokenList, { query: siloSelector })
+    scimTokenListErrorsAllowedQ(siloSelector.silo)
   )
 
   const [modalState, setModalState] = useState<ModalState>(false)

app/table/cells/IpPoolCell.tsx

@@ -14,7 +14,16 @@ import { EmptyCell, SkeletonCell } from './EmptyCell'
 
 export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => {
   const { data: result } = useQuery(
-    qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } })
+    qErrorsAllowed(
+      api.ipPoolView,
+      { path: { pool: ipPoolId } },
+      {
+        errorsExpected: {
+          explanation: 'the referenced IP pool may have been deleted.',
+          statusCode: 404,
+        },
+      }
+    )
   )
   if (!result) return <SkeletonCell />
   // this should essentially never happen, but it's probably better than blowing

app/table/cells/IpVersionCell.tsx

@@ -14,7 +14,16 @@ import { EmptyCell, SkeletonCell } from './EmptyCell'
 
 export const IpVersionCell = ({ ipPoolId }: { ipPoolId: string }) => {
   const { data: result } = useQuery(
-    qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } })
+    qErrorsAllowed(
+      api.ipPoolView,
+      { path: { pool: ipPoolId } },
+      {
+        errorsExpected: {
+          explanation: 'the referenced IP pool may have been deleted.',
+          statusCode: 404,
+        },
+      }
+    )
   )
   if (!result) return <SkeletonCell />
   if (result.type === 'error') return <EmptyCell />

test/e2e/error-pages.e2e.ts

@@ -5,9 +5,7 @@
  *
  * Copyright Oxide Computer Company
  */
-import { expect, test } from '@playwright/test'
-
-import { getPageAsUser, hasConsoleMessage } from './utils'
+import { expect, expectConsoleMessage, getPageAsUser, test } from './utils'
 
 test('Shows 404 page when a resource is not found', async ({ page }) => {
   await page.goto('/nonexistent')
@@ -19,22 +17,17 @@ test('Shows 404 page when a resource is not found', async ({ page }) => {
   await expect(page.getByRole('button', { name: 'Sign out' })).toBeVisible()
 })
 
-test('Shows something went wrong page on other errors', async ({ page, browserName }) => {
+test('Shows something went wrong page on other errors', async ({ page }) => {
   await page.goto('/projects/error-503') // specially handled in mock server
   await expect(page.getByText('Something went wrong')).toBeVisible()
 
   // Invariant failed doesn't show up in the page...
   await expect(page.getByText('Invariant failed')).toBeHidden()
 
-  // But we do see it in the browser console. Skip Firefox because it handles
-  // these errors differently and it's hard to get the error text out.
-  // eslint-disable-next-line playwright/no-conditional-in-test
-  if (browserName !== 'firefox') {
-    const error =
-      'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]'
-    // eslint-disable-next-line playwright/no-conditional-expect
-    expect(await hasConsoleMessage(page, error)).toBeTruthy()
-  }
+  // But we do see it in the browser console
+  const error =
+    'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]'
+  await expectConsoleMessage(page, error)
 
   // test clicking sign out
   await page.getByRole('button', { name: 'Sign out' }).click()