From b7f92207c68c2591fff69242766d07c419085ea6 Mon Sep 17 00:00:00 2001
From: Ben Hollis <ben@benhollis.net>
Date: Tue, 26 Nov 2024 23:42:31 -0800
Subject: [PATCH] Maybe improve error handling for manifest load/check?

---
 src/app/bungie-api/bungie-service-helper.ts | 13 ++-----
 src/app/manifest/d1-manifest-service.ts     | 30 ++++++----------
 src/app/manifest/manifest-service-json.ts   | 38 ++++++++-------------
 src/app/utils/dim-error.ts                  |  6 ++--
 src/app/wishlists/wishlist-fetch.ts         | 17 +++++----
 5 files changed, 39 insertions(+), 65 deletions(-)

diff --git a/src/app/bungie-api/bungie-service-helper.ts b/src/app/bungie-api/bungie-service-helper.ts
index 8028b56d62..a57d308b50 100644
--- a/src/app/bungie-api/bungie-service-helper.ts
+++ b/src/app/bungie-api/bungie-service-helper.ts
@@ -84,7 +84,7 @@ function dimErrorHandledHttpClient(httpClient: HttpClient): HttpClient {
 /**
  * if HttpClient throws an error (js, Bungie, http) this enriches it with DIM concepts and then re-throws it
  */
-function handleErrors(error: unknown): never {
+export function handleErrors(error: unknown): never {
   if (error instanceof DOMException && error.name === 'AbortError') {
     throw (
       navigator.onLine
@@ -99,6 +99,8 @@ function handleErrors(error: unknown): never {
   }
 
   if (error instanceof TypeError) {
+    // fetch throws this when the user is offline (and a number of other more static cases)
+    // https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch#exceptions
     throw (
       navigator.onLine
         ? new DimError('BungieService.NotConnectedOrBlocked')
@@ -111,15 +113,6 @@ function handleErrors(error: unknown): never {
   }
 
   if (error instanceof HttpStatusError) {
-    // "I don't think they exist" --Westley, The Princess Bride (1987)
-    if (error.status === -1) {
-      throw (
-        navigator.onLine
-          ? new DimError('BungieService.NotConnectedOrBlocked')
-          : new DimError('BungieService.NotConnected')
-      ).withError(error);
-    }
-
     // Token expired and other auth maladies
     if (error.status === 401 || error.status === 403) {
       throw new DimError('BungieService.NotLoggedIn').withError(error);
diff --git a/src/app/manifest/d1-manifest-service.ts b/src/app/manifest/d1-manifest-service.ts
index ab11d57a5a..be665525fd 100644
--- a/src/app/manifest/d1-manifest-service.ts
+++ b/src/app/manifest/d1-manifest-service.ts
@@ -1,3 +1,4 @@
+import { handleErrors } from 'app/bungie-api/bungie-service-helper';
 import { HttpStatusError, toHttpStatusError } from 'app/bungie-api/http-client';
 import { AllD1DestinyManifestComponents } from 'app/destiny1/d1-manifest-types';
 import { settingsSelector } from 'app/dim-api/selectors';
@@ -5,6 +6,7 @@ import { t } from 'app/i18next-t';
 import { loadingEnd, loadingStart } from 'app/shell/actions';
 import { del, get, set } from 'app/storage/idb-keyval';
 import { ThunkResult } from 'app/store/types';
+import { DimError } from 'app/utils/dim-error';
 import { convertToError } from 'app/utils/errors';
 import { errorLog, infoLog } from 'app/utils/log';
 import { dedupePromise } from 'app/utils/promises';
@@ -43,31 +45,19 @@ function doGetManifest(): ThunkResult<AllD1DestinyManifestComponents> {
       }
       return manifest;
     } catch (err) {
-      const e = convertToError(err);
-      let message = e.message;
-
-      if (e instanceof TypeError || (e instanceof HttpStatusError && e.status === -1)) {
-        message = navigator.onLine
-          ? t('BungieService.NotConnectedOrBlocked')
-          : t('BungieService.NotConnected');
-      } else if (e instanceof HttpStatusError) {
-        if (e.status === 503 || e.status === 522 /* cloudflare */) {
-          message = t('BungieService.Difficulties');
-        } else if (e.status < 200 || e.status >= 400) {
-          message = t('BungieService.NetworkError', {
-            status: e.status,
-            statusText: e.message,
-          });
-        }
+      let e = convertToError(err);
+      if (e instanceof DimError && e.cause) {
+        e = e.cause;
+      }
+      if (e.cause instanceof TypeError || e.cause instanceof HttpStatusError) {
       } else {
         // Something may be wrong with the manifest
         deleteManifestFile();
       }
 
-      const statusText = t('Manifest.Error', { error: message });
-      errorLog(TAG, 'Manifest loading error', { error: e }, e);
+      errorLog(TAG, 'Manifest loading error', e);
       reportException('manifest load', e);
-      throw new Error(statusText);
+      throw new DimError('Manifest.Error', t('Manifest.Error', { error: e.message })).withError(e);
     } finally {
       dispatch(loadingEnd(t('Manifest.Load')));
     }
@@ -112,6 +102,8 @@ function loadManifestRemote(
       saveManifestToIndexedDB(manifest, version);
 
       return manifest;
+    } catch (e) {
+      handleErrors(e); // throws
     } finally {
       dispatch(loadingEnd(t('Manifest.Download')));
     }
diff --git a/src/app/manifest/manifest-service-json.ts b/src/app/manifest/manifest-service-json.ts
index d883a52e26..1d65a82c8a 100644
--- a/src/app/manifest/manifest-service-json.ts
+++ b/src/app/manifest/manifest-service-json.ts
@@ -1,11 +1,13 @@
+import { handleErrors } from 'app/bungie-api/bungie-service-helper';
 import { HttpStatusError, toHttpStatusError } from 'app/bungie-api/http-client';
 import { settingsSelector } from 'app/dim-api/selectors';
 import { t } from 'app/i18next-t';
 import { loadingEnd, loadingStart } from 'app/shell/actions';
 import { del, get, set } from 'app/storage/idb-keyval';
 import { ThunkResult } from 'app/store/types';
+import { DimError } from 'app/utils/dim-error';
 import { emptyArray, emptyObject } from 'app/utils/empty';
-import { convertToError, errorMessage } from 'app/utils/errors';
+import { convertToError } from 'app/utils/errors';
 import { errorLog, infoLog, timer } from 'app/utils/log';
 import { dedupePromise } from 'app/utils/promises';
 import { LookupTable } from 'app/utils/util-types';
@@ -152,33 +154,20 @@ function doGetManifest(tableAllowList: string[]): ThunkResult<AllDestinyManifest
         throw new Error('Manifest corrupted, please reload');
       }
       return manifest;
-    } catch (e) {
-      let message = errorMessage(e);
-
-      if (e instanceof TypeError || (e instanceof HttpStatusError && e.status === -1)) {
-        message = navigator.onLine
-          ? t('BungieService.NotConnectedOrBlocked')
-          : t('BungieService.NotConnected');
-      } else if (e instanceof HttpStatusError) {
-        if (e.status === 503 || e.status === 522 /* cloudflare */) {
-          message = t('BungieService.Difficulties');
-        } else if (e.status < 200 || e.status >= 400) {
-          message = t('BungieService.NetworkError', {
-            status: e.status,
-            statusText: e.message,
-          });
-        }
+    } catch (err) {
+      let e = convertToError(err);
+      if (e instanceof DimError && e.cause) {
+        e = e.cause;
+      }
+      if (e.cause instanceof TypeError || e.cause instanceof HttpStatusError) {
       } else {
         // Something may be wrong with the manifest
-        await deleteManifestFile();
+        deleteManifestFile();
       }
 
-      const statusText = t('Manifest.Error', { error: message });
       errorLog(TAG, 'Manifest loading error', e);
       reportException('manifest load', e);
-      const error = new Error(statusText);
-      error.name = 'ManifestError';
-      throw error;
+      throw new DimError('Manifest.Error', t('Manifest.Error', { error: e.message })).withError(e);
     } finally {
       dispatch(loadingEnd(t('Manifest.Load')));
       stopTimer();
@@ -214,7 +203,8 @@ function loadManifest(tableAllowList: string[]): ThunkResult<AllDestinyManifestC
 
     try {
       return await loadManifestFromCache(version, tableAllowList);
-    } catch {
+    } catch (e) {
+      infoLog(TAG, 'Unable to use cached manifest, loading fresh manifest from Bungie.net', e);
       return dispatch(loadManifestRemote(version, components, tableAllowList));
     }
   };
@@ -286,7 +276,7 @@ export async function downloadManifestComponents(
         }
       }
       if (!body) {
-        throw error ?? new Error(`Table ${table}`);
+        handleErrors(error); // throws
       }
 
       // I couldn't figure out how to make these types work...
diff --git a/src/app/utils/dim-error.ts b/src/app/utils/dim-error.ts
index b35e808e96..00decff7ab 100644
--- a/src/app/utils/dim-error.ts
+++ b/src/app/utils/dim-error.ts
@@ -9,11 +9,11 @@ import { convertToError } from './errors';
  * The message is typically a localized error message.
  */
 export class DimError extends Error {
-  // A non-localized string to help identify/categorize errors for DIM developers. Usually the localization key of the message.
+  /** A non-localized string to help identify/categorize errors for DIM developers. Usually the localization key of the message. */
   code?: string;
-  // The error that caused this error, if there is one. Naming it 'cause' makes it automatically chain in Sentry.
+  /** The error that caused this error, if there is one. Naming it 'cause' makes it automatically chain in Sentry. */
   cause?: Error;
-  // Whether to show social links in the error report dialog
+  /** Whether to show social links in the error report dialog. */
   showSocials = true;
 
   /** Pass in just a message key to set the message to the localized version of that key, or override with the second parameter. */
diff --git a/src/app/wishlists/wishlist-fetch.ts b/src/app/wishlists/wishlist-fetch.ts
index 43c405f6f4..7fce5dcd05 100644
--- a/src/app/wishlists/wishlist-fetch.ts
+++ b/src/app/wishlists/wishlist-fetch.ts
@@ -1,3 +1,4 @@
+import { toHttpStatusError } from 'app/bungie-api/http-client';
 import { settingsSelector } from 'app/dim-api/selectors';
 import { t } from 'app/i18next-t';
 import { showNotification } from 'app/notifications/notifications';
@@ -73,15 +74,13 @@ export function fetchWishList(newWishlistSource?: string): ThunkResult {
     let wishListTexts: string[];
     try {
       wishListTexts = await Promise.all(
-        wishlistUrlsToFetch.map((url) =>
-          fetch(url).then((res) => {
-            if (res.status < 200 || res.status >= 300) {
-              throw new Error(`failed fetch -- ${res.status} ${res.statusText}`);
-            }
-
-            return res.text();
-          }),
-        ),
+        wishlistUrlsToFetch.map(async (url) => {
+          const res = await fetch(url);
+          if (res.status < 200 || res.status >= 300) {
+            throw await toHttpStatusError(res);
+          }
+          return res.text();
+        }),
       );
 
       // if this is a new wishlist, set the setting now that we know it's fetchable