Merge pull request #10844 from DestinyItemManager/wishlist-load

Improve wishlist loading

config/i18n.json

@@ -1503,14 +1503,15 @@
     "ExternalSourcePlaceholder": "Paste wish list URL here",
     "Header": "Wish List",
     "Import": "Load Wish List Rolls",
-    "ImportFailed": "No wish list information found.",
-    "ImportError": "Error loading wish list: {{error}}",
+    "ImportFailed": "None of your wish lists contained any valid rolls.",
+    "ImportError": "Error loading wish list from \"{{url}}\": {{error}}",
     "ImportNoFile": "No file selected.",
     "InvalidExternalSource": "Please enter a valid URL for your external wish list source. The URL must start with one of the following:",
     "JustAnotherTeam": "Just Another Team",
     "LastUpdated": "Last updated: {{lastUpdatedDate}} at {{lastUpdatedTime}}",
     "Num": "{{num, number}} rolls in your wish list",
     "NumRolls": "{{num, number}} rolls",
+    "DupeRolls": " (+{{num, number}} ignored dupes)",
     "PreMadeFiles": "Use A Pre-Made Wish List",
     "Refresh": "Refresh Wishlist",
     "SourceAlreadyAdded": "Wish List already added",

docs/CHANGELOG.md

@@ -1,7 +1,8 @@
 ## Next
 
 * Added a wishlist refresh button in Settings to help with wishlist development. Note that GitHub can take upwards of 10min to actually reflect your changes. Refreshing won't speed up GitHub.
-
+* Having a broken wish list in your settings will no longer prevent removing other wish lists.
+
 ## 8.50.1 <span class="changelog-date">(2024-12-12)</span>
 
 * Notes now appear in the tooltips on item tiles.

src/app/settings/WishListSettings.tsx

@@ -1,5 +1,6 @@
 import { settingSelector } from 'app/dim-api/selectors';
 import { ConfirmButton } from 'app/dim-ui/ConfirmButton';
+import ExternalLink from 'app/dim-ui/ExternalLink';
 import { PressTip } from 'app/dim-ui/PressTip';
 import { I18nKey, t } from 'app/i18next-t';
 import { showNotification } from 'app/notifications/notifications';
@@ -32,6 +33,8 @@ export default function WishListSettings() {
     dispatch(fetchWishList());
   }, [dispatch]);
 
+  // TODO: add a "local" source that can coexist with other sources?
+
   const activeWishlistUrls = settingsWishListSource
     ? settingsWishListSource.split('|').map((url) => url.trim())
     : [];
@@ -46,7 +49,10 @@ export default function WishListSettings() {
       showNotification({
         type: 'error',
         title: t('WishListRoll.Header'),
-        body: t('WishListRoll.ImportError', { error: errorMessage(e) }),
+        body: t('WishListRoll.ImportError', {
+          url: reloadWishListSource ?? '',
+          error: errorMessage(e),
+        }),
       });
     }
   };
@@ -55,7 +61,7 @@ export default function WishListSettings() {
     const reader = new FileReader();
     reader.onload = async () => {
       if (reader.result && typeof reader.result === 'string') {
-        const wishListAndInfo = toWishList([undefined, reader.result]);
+        const wishListAndInfo = toWishList([[undefined, reader.result]]);
         if (wishListAndInfo.wishListRolls.length) {
           dispatch(clearWishLists());
         }
@@ -150,10 +156,12 @@ export default function WishListSettings() {
           return (
             <BuiltinWishlist
               key={url}
+              url={url}
               name={builtinEntry.name}
               title={loadedData?.title}
               description={loadedData?.description}
               rollsCount={loadedData?.numRolls}
+              dupeRollsCount={loadedData?.dupeRolls}
               checked={true}
               onChange={(checked) => changeUrl(url, checked)}
             />
@@ -166,6 +174,7 @@ export default function WishListSettings() {
               title={loadedData?.title}
               description={loadedData?.description}
               rollsCount={loadedData?.numRolls}
+              dupeRollsCount={loadedData?.dupeRolls}
               onRemove={() => changeUrl(url, false)}
             />
           );
@@ -175,11 +184,13 @@ export default function WishListSettings() {
       {disabledBuiltinLists.map((list) => (
         <BuiltinWishlist
           key={list.url}
+          url={list.url}
           name={list.name}
           title={undefined}
           description={undefined}
           checked={false}
           rollsCount={undefined}
+          dupeRollsCount={undefined}
           onChange={(checked) => changeUrl(list.url, checked)}
         />
       ))}
@@ -198,23 +209,32 @@ export default function WishListSettings() {
 
 function BuiltinWishlist({
   name,
+  url,
   title,
   description,
   rollsCount,
+  dupeRollsCount,
   checked,
   onChange,
 }: {
   name: I18nKey;
+  url: string;
   title: string | undefined;
   description: string | undefined;
   rollsCount: number | undefined;
+  dupeRollsCount: number | undefined;
   checked: boolean;
   onChange: (checked: boolean) => void;
 }) {
   return (
     <div className={settingClass}>
       <Checkbox label={t(name)} name={name as keyof Settings} value={checked} onChange={onChange} />
-      {rollsCount !== undefined && t('WishListRoll.NumRolls', { num: rollsCount })}
+      <ExternalLink href={url}>
+        {rollsCount !== undefined && t('WishListRoll.NumRolls', { num: rollsCount })}
+        {dupeRollsCount !== undefined &&
+          dupeRollsCount > 0 &&
+          t('WishListRoll.DupeRolls', { num: dupeRollsCount })}
+      </ExternalLink>
       {(title || description) && (
         <div className={fineprintClass}>
           <b>{title}</b>
@@ -231,12 +251,14 @@ function UrlWishlist({
   title,
   description,
   rollsCount,
+  dupeRollsCount,
   onRemove,
 }: {
   url: string;
   title: string | undefined;
   description: string | undefined;
   rollsCount: number | undefined;
+  dupeRollsCount: number | undefined;
   onRemove: () => void;
 }) {
   return (
@@ -246,7 +268,12 @@ function UrlWishlist({
         <AppIcon icon={deleteIcon} title={t('Loadouts.Delete')} />
       </ConfirmButton>
       {!title && <div className={fineprintClass}>{url}</div>}
-      {rollsCount !== undefined && t('WishListRoll.NumRolls', { num: rollsCount })}
+      <ExternalLink href={url}>
+        {rollsCount !== undefined && t('WishListRoll.NumRolls', { num: rollsCount })}
+        {dupeRollsCount !== undefined &&
+          dupeRollsCount > 0 &&
+          t('WishListRoll.DupeRolls', { num: dupeRollsCount })}
+      </ExternalLink>
       {description && <div className={fineprintClass}>{description}</div>}
     </div>
   );

src/app/wishlists/types.ts

@@ -52,6 +52,8 @@ export interface WishListInfo {
   url: string | undefined;
   title?: string;
   description?: string;
-  /** The number of rolls from this wish list that actually made it in. */
+  /** The number of rolls from this wish list that actually made it in (e.g. were valid and unique). */
   numRolls: number;
+  /** The number of rolls in this list that were duplicates of other lists. */
+  dupeRolls: number;
 }

src/app/wishlists/wishlist-fetch.ts

@@ -6,6 +6,7 @@ import { setSettingAction } from 'app/settings/actions';
 import { settingsReady } from 'app/settings/settings';
 import { get } from 'app/storage/idb-keyval';
 import { ThunkResult } from 'app/store/types';
+import { errorMessage } from 'app/utils/errors';
 import { errorLog, infoLog } from 'app/utils/log';
 import { once } from 'es-toolkit';
 import { loadWishLists, touchWishLists } from './actions';
@@ -26,7 +27,7 @@ function hoursAgo(dateToCheck?: Date): number {
 }
 
 /**
- * this performs both the initial fetch (after setting a new wishlist) (when arg0 exists)
+ * this performs both the initial fetch (after setting a new wishlist) (when newWishlistSource exists)
  * and subsequent fetches (checking for updates) (arg-less)
  */
 export function fetchWishList(newWishlistSource?: string, manualRefresh?: boolean): ThunkResult {
@@ -48,9 +49,6 @@ export function fetchWishList(newWishlistSource?: string, manualRefresh?: boolea
       return;
     }
 
-    // Pipe | seperated URLs
-    const wishlistUrlsToFetch = validateWishListURLs(wishlistToFetch);
-
     const {
       lastFetched: wishListLastUpdated,
       wishListAndInfo: { source: loadedWishListSource, wishListRolls: loadedWishListRolls },
@@ -71,38 +69,48 @@ export function fetchWishList(newWishlistSource?: string, manualRefresh?: boolea
       return;
     }
 
-    let wishListTexts: string[];
-    try {
-      wishListTexts = await Promise.all(
-        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
-      if (newWishlistSource) {
-        dispatch(setSettingAction('wishListSource', newWishlistSource));
+    // Pipe | separated URLs
+    const wishlistUrlsToFetch = validateWishListURLs(wishlistToFetch);
+    const wishListResults = await Promise.allSettled(
+      wishlistUrlsToFetch.map(async (url) => {
+        const res = await fetch(url);
+        if (res.status < 200 || res.status >= 300) {
+          throw await toHttpStatusError(res);
+        }
+        return res.text();
+      }),
+    );
+
+    const wishLists: [url: string, text: string][] = [];
+    let hasSuccess = false;
+    for (let i = 0; i < wishlistUrlsToFetch.length; i++) {
+      const url = wishlistUrlsToFetch[i];
+      const result = wishListResults[i];
+      if (result.status === 'rejected') {
+        showNotification({
+          type: 'warning',
+          title: t('WishListRoll.Header'),
+          body: t('WishListRoll.ImportError', { url, error: errorMessage(result.reason) }),
+        });
+        errorLog(TAG, 'Unable to load wish list', url, result.reason);
+        return;
+      } else if (result.status === 'fulfilled') {
+        hasSuccess = true;
+        wishLists.push([url, result.value]);
       }
-    } catch (e) {
-      showNotification({
-        type: 'warning',
-        title: t('WishListRoll.Header'),
-        body: t('WishListRoll.ImportFailed'),
-      });
-      errorLog(TAG, 'Unable to load wish list', e);
+    }
+
+    if (!hasSuccess) {
+      // Give up if we couldn't fetch any of the lists
       return;
     }
 
-    const wishLists: [string, string][] = wishlistUrlsToFetch.map((url, idx) => [
-      url,
-      wishListTexts[idx],
-    ]);
+    // if this is a new wishlist, set the setting now that we know at least one list is fetchable
+    if (newWishlistSource && hasSuccess) {
+      dispatch(setSettingAction('wishListSource', newWishlistSource));
+    }
 
-    const wishListAndInfo = toWishList(...wishLists);
+    const wishListAndInfo = toWishList(wishLists);
     wishListAndInfo.source = wishlistToFetch;
 
     // Only update if the length changed. The wish list may actually be different - we don't do a deep check -

src/app/wishlists/wishlist-file.test.ts

@@ -15,5 +15,5 @@ const cases: [wishlist: string, result: WishListRoll][] = [
 ];
 
 test.each(cases)('parse wishlist line: %s', (wishlist, result) => {
-  expect(toWishList([undefined, wishlist]).wishListRolls[0]).toStrictEqual(result);
+  expect(toWishList([[undefined, wishlist]]).wishListRolls[0]).toStrictEqual(result);
 });

src/app/wishlists/wishlist-file.ts

@@ -24,9 +24,7 @@ const notesLabel = '//notes:';
  * one or more wish list text files, deduplicating within
  * and between lists.
  */
-export function toWishList(
-  ...files: [url: string | undefined, contents: string][]
-): WishListAndInfo {
+export function toWishList(files: [url: string | undefined, contents: string][]): WishListAndInfo {
   const stopTimer = timer(TAG, 'Parse wish list');
   try {
     const wishList: WishListAndInfo = {
@@ -42,9 +40,8 @@ export function toWishList(
         title: undefined,
         description: undefined,
         numRolls: 0,
+        dupeRolls: 0,
       };
-      let dupes = 0;
-
       let blockNotes: string | undefined = undefined;
 
       const lines = fileText.split('\n');
@@ -74,14 +71,13 @@ export function toWishList(
               wishList.wishListRolls.push(roll);
               info.numRolls++;
             } else {
-              dupes++;
+              info.dupeRolls++;
             }
           }
         }
       }
-
-      if (dupes > 0) {
-        warnLog(TAG, 'Discarded', dupes, 'duplicate rolls from wish list', url);
+      if (info.dupeRolls > 0) {
+        warnLog(TAG, 'Discarded', info.dupeRolls, 'duplicate rolls from wish list', url);
       }
       wishList.infos.push(info);
     }

src/locale/en.json

@@ -1483,12 +1483,13 @@
     "Clear": "Clear Wish List",
     "CopiedLine": "Wish List roll copied to clipboard",
     "CopyLine": "Copy Selected Perks as Wish List Roll",
+    "DupeRolls": " (+{{num, number}} ignored dupes)",
     "ExternalSource": "Add another wish list",
     "ExternalSourcePlaceholder": "Paste wish list URL here",
     "Header": "Wish List",
     "Import": "Load Wish List Rolls",
-    "ImportError": "Error loading wish list: {{error}}",
-    "ImportFailed": "No wish list information found.",
+    "ImportError": "Error loading wish list from \"{{url}}\": {{error}}",
+    "ImportFailed": "None of your wish lists contained any valid rolls.",
     "ImportNoFile": "No file selected.",
     "InvalidExternalSource": "Please enter a valid URL for your external wish list source. The URL must start with one of the following:",
     "JustAnotherTeam": "Just Another Team",