From 9f58a90253d2dc700c59e45e50d6d0b94b1f554a Mon Sep 17 00:00:00 2001
From: Brandon Sprague <brandon@sprague.mx>
Date: Wed, 13 Sep 2023 09:40:16 -0700
Subject: [PATCH] Address review comments

---
 frontend/app.vue                             |  4 +--
 frontend/components/modal/Error.vue          | 18 ++++++--------
 frontend/composables/newModal.ts             |  5 ++--
 frontend/composables/useAPI.ts               |  4 +--
 frontend/error.vue                           |  2 +-
 frontend/globalimports/present.ts            | 22 +++++++++++------
 frontend/layouts/default.vue                 |  3 ++-
 frontend/lib/error/index.ts                  | 26 +++++++++++++++++---
 frontend/pages/admin/pacta-version/[id].vue  | 20 ++++++---------
 frontend/pages/admin/pacta-version/index.vue |  8 +++---
 frontend/pages/admin/pacta-version/new.vue   |  4 +--
 11 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/frontend/app.vue b/frontend/app.vue
index 0552f11..1eea4dd 100644
--- a/frontend/app.vue
+++ b/frontend/app.vue
@@ -1,5 +1,5 @@
 <script setup lang="ts">
-const { loading: { onMountedWithLoading, clearLoading }, error: { errorModalVisible, error } } = useModal()
+const { loading: { clearLoading }, error: { errorModalVisible, error } } = useModal()
 
 const handleError = (err: Error) => {
   error.value = err
@@ -19,8 +19,6 @@ onErrorCaptured((err: unknown, _instance: ComponentPublicInstance | null, _info:
   handleError(error)
   return false // Don't propagate
 })
-
-onMountedWithLoading(() => { /* nothing to do */ }, 'defaultLayout.onMountedWithLoading')
 </script>
 
 <template>
diff --git a/frontend/components/modal/Error.vue b/frontend/components/modal/Error.vue
index 20d3806..b9fab68 100644
--- a/frontend/components/modal/Error.vue
+++ b/frontend/components/modal/Error.vue
@@ -1,32 +1,27 @@
 <script setup lang="ts">
-import { watch } from 'vue'
-
 interface Props {
-  isFullPage?: boolean
+  routeBackOnClose?: boolean
 }
 
 const props = withDefaults(defineProps<Props>(), {
-  isFullPage: false
+  routeBackOnClose: false
 })
 
 const { error: { errorModalVisible, error: modalError } } = useModal()
 const error = useError()
 const router = useRouter()
 
-watch(errorModalVisible, async (newV, oldV) => {
-  if (!props.isFullPage) {
-    return
-  }
-  // We only care about the case where the modal was just closed (i.e. has gone from visible -> not visible).
-  if (newV || !oldV) {
+const maybeGoBack = async () => {
+  if (!props.routeBackOnClose) {
     return
   }
+
   if (window.history.length > 1) {
     await clearError().then(router.back)
   } else {
     await clearError({ redirect: '/' })
   }
-})
+}
 
 const fullError = computed(() => {
   const err = error.value ?? modalError.value
@@ -49,6 +44,7 @@ const fullError = computed(() => {
     v-model:visible="errorModalVisible"
     header="An error ocurred"
     sub-header="Sorry about that, our team take bug reports seriously, and will try to make it right!"
+    @closed="maybeGoBack"
   >
     <StandardDebug
       label="Error Details"
diff --git a/frontend/composables/newModal.ts b/frontend/composables/newModal.ts
index a747780..d4410d0 100644
--- a/frontend/composables/newModal.ts
+++ b/frontend/composables/newModal.ts
@@ -35,7 +35,7 @@ export const useModal = () => {
     return () => loadingSet.value.delete(loadKey)
   }
   const clearLoading = () => { loadingSet.value.clear() }
-  const withLoadingAndErrorHandling = async <T> (fn: () => Promise<T>, opKey: string): (Promise<T>) => {
+  const withLoading = async <T> (fn: () => Promise<T>, opKey: string): (Promise<T>) => {
     startLoading(opKey)
     const p = fn()
     void p.finally(stopLoading(opKey))
@@ -77,7 +77,7 @@ export const useModal = () => {
     anyBlockingModalOpen,
     newModalVisibilityState,
     loading: {
-      withLoadingAndErrorHandling,
+      withLoading,
       onMountedWithLoading,
       startLoading,
       stopLoading,
@@ -88,7 +88,6 @@ export const useModal = () => {
     error: {
       error,
       errorModalVisible,
-      withLoadingAndErrorHandling,
       handleOAPIError
     },
     permissionDenied: {
diff --git a/frontend/composables/useAPI.ts b/frontend/composables/useAPI.ts
index e079319..4feda7e 100644
--- a/frontend/composables/useAPI.ts
+++ b/frontend/composables/useAPI.ts
@@ -19,9 +19,7 @@ export const useAPI = (): API => {
   // auth.
   let headers: Record<string, string> = {}
   if (process.server) {
-    headers = Object.entries(useRequestHeaders(['cookie']))
-      .filter((ent) => !!ent[1])
-      .reduce((a, v) => ({ ...a, [v[0]]: v[1] }), {})
+    headers = useRequestHeaders(['cookie'])
   }
 
   const userCfg = {
diff --git a/frontend/error.vue b/frontend/error.vue
index c909f6f..6095528 100644
--- a/frontend/error.vue
+++ b/frontend/error.vue
@@ -9,5 +9,5 @@ errorModalVisible.value = true
 </script>
 
 <template>
-  <ModalError :is-full-page="true" />
+  <ModalError :route-back-on-close="true" />
 </template>
diff --git a/frontend/globalimports/present.ts b/frontend/globalimports/present.ts
index baca720..6bb2522 100644
--- a/frontend/globalimports/present.ts
+++ b/frontend/globalimports/present.ts
@@ -1,15 +1,23 @@
-import { ErrorWithRemediation, Remediation } from '@/lib/error'
+import { createErrorWithRemediation, Remediation } from '@/lib/error'
 
-export const present = <T>(t: T | undefined | null, r: Remediation): T => {
+export const present = <T>(t: T | undefined | null, r: Remediation, cause?: string): T => {
   if (t === undefined) {
-    throw new ErrorWithRemediation(`expected to be present but was undefined: ${typeof t}.`, r)
+    throw createErrorWithRemediation({
+      name: 'present error',
+      message: 'expected to be present but was undefined',
+      cause
+    }, r)
   }
   if (t === null) {
-    throw new ErrorWithRemediation(`expected to be present but was null: ${typeof t}.`, r)
+    throw createErrorWithRemediation({
+      name: 'present error',
+      message: 'expected to be present but was null',
+      cause
+    }, r)
   }
   return t
 }
 
-export const presentOrSuggestReload = <T>(t: T | undefined | null): T => present(t, Remediation.Reload)
-export const presentOrFileBug = <T>(t: T | undefined | null): T => present(t, Remediation.FileBug)
-export const presentOrCheckURL = <T>(t: T | undefined | null): T => present(t, Remediation.CheckUrl)
+export const presentOrSuggestReload = <T>(t: T | undefined | null, cause?: string): T => present(t, Remediation.Reload, cause)
+export const presentOrFileBug = <T>(t: T | undefined | null, cause?: string): T => present(t, Remediation.FileBug, cause)
+export const presentOrCheckURL = <T>(t: T | undefined | null, cause?: string): T => present(t, Remediation.CheckUrl, cause)
diff --git a/frontend/layouts/default.vue b/frontend/layouts/default.vue
index 90309f5..fce89e3 100644
--- a/frontend/layouts/default.vue
+++ b/frontend/layouts/default.vue
@@ -1,6 +1,7 @@
 <script setup lang="ts">
+const { loading: { onMountedWithLoading }, anyBlockingModalOpen } = useModal()
 
-const { anyBlockingModalOpen } = useModal()
+onMountedWithLoading(() => { /* nothing to do */ }, 'defaultLayout.onMountedWithLoading')
 </script>
 
 <template>
diff --git a/frontend/lib/error/index.ts b/frontend/lib/error/index.ts
index 16025c7..a725790 100644
--- a/frontend/lib/error/index.ts
+++ b/frontend/lib/error/index.ts
@@ -1,3 +1,5 @@
+import { type NuxtError } from 'nuxt/app'
+
 export enum Remediation {
   None = 'none',
   Reload = 'reload',
@@ -5,8 +7,26 @@ export enum Remediation {
   CheckUrl = 'check-url',
 }
 
-export class ErrorWithRemediation extends Error {
-  constructor (msg: string, public readonly remediation: Remediation) {
-    super(msg)
+interface RemediationData {
+  type: 'remediation'
+  remediation: Remediation
+}
+
+type PACTAErrorData = RemediationData
+
+interface PACTAError extends NuxtError {
+  data: PACTAErrorData
+}
+
+export const createErrorWithRemediation = (err: string | Partial<NuxtError>, r: Remediation): PACTAError => {
+  const nuxtErr = createError(err)
+  if (!nuxtErr.data || typeof (nuxtErr.data) !== 'object') {
+    nuxtErr.data = {}
   }
+  nuxtErr.data.type = 'remediation'
+  nuxtErr.data.remediation = r
+
+  // TypeScript doesn't automatically pick up that `data` is always set, so we
+  // give it a nudge.
+  return nuxtErr as PACTAError
 }
diff --git a/frontend/pages/admin/pacta-version/[id].vue b/frontend/pages/admin/pacta-version/[id].vue
index 258d342..a542133 100644
--- a/frontend/pages/admin/pacta-version/[id].vue
+++ b/frontend/pages/admin/pacta-version/[id].vue
@@ -3,7 +3,7 @@ import { type PactaVersion, type PactaVersionChanges } from '@/openapi/generated
 
 const router = useRouter()
 const { pactaClient } = useAPI()
-const { error: { withLoadingAndErrorHandling, handleOAPIError } } = useModal()
+const { loading: { withLoading }, error: { handleOAPIError } } = useModal()
 const { fromParams } = useURLParams()
 
 const id = presentOrCheckURL(fromParams('id'))
@@ -11,7 +11,7 @@ const id = presentOrCheckURL(fromParams('id'))
 const prefix = `admin/pacta-version/${id}`
 const pactaVersion = useState<PactaVersion>(`${prefix}.pactaVersion`)
 const { data: persistedPactaVersion, error, refresh } = await useAsyncData(`${prefix}.getPactaVersion`, () => {
-  return withLoadingAndErrorHandling(() => {
+  return withLoading(() => {
     return pactaClient.findPactaVersionById(id)
       .then(handleOAPIError)
   }, `${prefix}.getPactaVersion`)
@@ -19,15 +19,11 @@ const { data: persistedPactaVersion, error, refresh } = await useAsyncData(`${pr
 if (error.value) {
   throw createError(error.value)
 }
-if (!persistedPactaVersion.value) {
-  throw createError({ message: 'PACTA version not found' })
-}
-pactaVersion.value = { ...persistedPactaVersion.value }
+
+pactaVersion.value = { ...presentOrCheckURL(persistedPactaVersion.value, 'no PACTA version in response') }
 const refreshPACTA = async () => {
   await refresh()
-  if (persistedPactaVersion.value) {
-    pactaVersion.value = { ...persistedPactaVersion.value }
-  }
+  pactaVersion.value = { ...presentOrCheckURL(persistedPactaVersion.value, 'no PACTA version in response after refresh') }
 }
 
 const changes = computed<PactaVersionChanges>(() => {
@@ -42,19 +38,19 @@ const changes = computed<PactaVersionChanges>(() => {
 })
 const hasChanges = computed<boolean>(() => Object.keys(changes.value).length > 0)
 
-const markDefault = () => withLoadingAndErrorHandling(
+const markDefault = () => withLoading(
   () => pactaClient.markPactaVersionAsDefault(id)
     .then(handleOAPIError)
     .then(refreshPACTA),
   `${prefix}.markPactaVersionAsDefault`
 )
-const deletePV = () => withLoadingAndErrorHandling(
+const deletePV = () => withLoading(
   () => pactaClient.deletePactaVersion(id)
     .then(handleOAPIError)
     .then(() => router.push('/admin/pacta-version')),
   `${prefix}.deletePactaVersion`
 )
-const saveChanges = () => withLoadingAndErrorHandling(
+const saveChanges = () => withLoading(
   () => pactaClient.updatePactaVersion(id, changes.value)
     .then(handleOAPIError)
     .then(refreshPACTA)
diff --git a/frontend/pages/admin/pacta-version/index.vue b/frontend/pages/admin/pacta-version/index.vue
index 691b4bc..4afc1c9 100644
--- a/frontend/pages/admin/pacta-version/index.vue
+++ b/frontend/pages/admin/pacta-version/index.vue
@@ -1,23 +1,23 @@
 <script setup lang="ts">
 const router = useRouter()
 const { pactaClient } = useAPI()
-const { error: { withLoadingAndErrorHandling, handleOAPIError } } = useModal()
+const { loading: { withLoading }, error: { handleOAPIError } } = useModal()
 
 const prefix = 'admin/pacta-version'
 const { data: pactaVersions, refresh } = await useAsyncData(`${prefix}.getPactaVersions`, () => {
-  return withLoadingAndErrorHandling(() => {
+  return withLoading(() => {
     return pactaClient.listPactaVersions().then(handleOAPIError)
   }, `${prefix}.getPactaVersions`)
 })
 
 const newPV = () => router.push('/admin/pacta-version/new')
-const markDefault = (id: string) => withLoadingAndErrorHandling(
+const markDefault = (id: string) => withLoading(
   () => pactaClient.markPactaVersionAsDefault(id)
     .then(handleOAPIError)
     .then(refresh),
   `${prefix}.markPactaVersionAsDefault`
 )
-const deletePV = (id: string) => withLoadingAndErrorHandling(
+const deletePV = (id: string) => withLoading(
   () => pactaClient.deletePactaVersion(id)
     .then(handleOAPIError)
     .then(refresh),
diff --git a/frontend/pages/admin/pacta-version/new.vue b/frontend/pages/admin/pacta-version/new.vue
index 54bfbf3..c5d88c1 100644
--- a/frontend/pages/admin/pacta-version/new.vue
+++ b/frontend/pages/admin/pacta-version/new.vue
@@ -3,7 +3,7 @@ import { type PactaVersion } from '@/openapi/generated/pacta'
 
 const router = useRouter()
 const { pactaClient } = useAPI()
-const { error: { withLoadingAndErrorHandling, handleOAPIError } } = useModal()
+const { loading: { withLoading }, error: { handleOAPIError } } = useModal()
 
 const prefix = 'admin/pacta-version/new'
 const pactaVersion = useState<PactaVersion>(`${prefix}.pactaVersion`, () => ({
@@ -16,7 +16,7 @@ const pactaVersion = useState<PactaVersion>(`${prefix}.pactaVersion`, () => ({
 }))
 
 const discard = () => router.push('/admin/pacta-version')
-const save = () => withLoadingAndErrorHandling(
+const save = () => withLoading(
   () => pactaClient.createPactaVersion(pactaVersion.value).then(handleOAPIError).then(() => router.push('/admin/pacta-version')),
   `${prefix}.save`
 )