Fix starterpref reset (#6430)

* Prevent an empty starterpreferences object from being saved

* Fix ssui nullish coalescing

* Update src/utils/data.ts

Co-authored-by: Dean <69436131+emdeann@users.noreply.github.com>

* Fix starter preferences clearing, add tests

---------

Co-authored-by: Wlowscha <54003515+Wlowscha@users.noreply.github.com>
Co-authored-by: Dean <69436131+emdeann@users.noreply.github.com>

src/ui/starter-select-ui-handler.ts

@@ -72,7 +72,7 @@ import {
   rgbHexToRgba,
 } from "#utils/common";
 import type { StarterPreferences } from "#utils/data";
-import { loadStarterPreferences, saveStarterPreferences } from "#utils/data";
+import { deepCopy, loadStarterPreferences, saveStarterPreferences } from "#utils/data";
 import { getPokemonSpeciesForm, getPokerusStarters } from "#utils/pokemon-utils";
 import { toCamelCase, toTitleCase } from "#utils/strings";
 import { argbFromRgba } from "@material/material-color-utilities";
@@ -1148,7 +1148,8 @@ export class StarterSelectUiHandler extends MessageUiHandler {
       this.starterSelectContainer.setVisible(true);
 
       this.starterPreferences = loadStarterPreferences();
-      this.originalStarterPreferences = loadStarterPreferences();
+      // Deep copy the JSON (avoid re-loading from disk)
+      this.originalStarterPreferences = deepCopy(this.starterPreferences);
 
       this.allSpecies.forEach((species, s) => {
         const icon = this.starterContainers[s].icon;
@@ -1212,6 +1213,8 @@ export class StarterSelectUiHandler extends MessageUiHandler {
     preferences: StarterPreferences,
     ignoreChallenge = false,
   ): StarterAttributes {
+    // if preferences for the species is undefined, set it to an empty object
+    preferences[species.speciesId] ??= {};
     const starterAttributes = preferences[species.speciesId];
     const { dexEntry, starterDataEntry: starterData } = this.getSpeciesData(species.speciesId, !ignoreChallenge);
 
@@ -1828,9 +1831,15 @@ export class StarterSelectUiHandler extends MessageUiHandler {
       // The persistent starter data to apply e.g. candy upgrades
       const persistentStarterData = globalScene.gameData.starterData[this.lastSpecies.speciesId];
       // The sanitized starter preferences
-      let starterAttributes = this.starterPreferences[this.lastSpecies.speciesId];
+      if (this.starterPreferences[this.lastSpecies.speciesId] === undefined) {
-      // The original starter preferences
+        this.starterPreferences[this.lastSpecies.speciesId] = {};
-      const originalStarterAttributes = this.originalStarterPreferences[this.lastSpecies.speciesId];
+      }
+      if (this.originalStarterPreferences[this.lastSpecies.speciesId] === undefined) {
+        this.originalStarterPreferences[this.lastSpecies.speciesId] = {};
+      }
+      // Bangs are safe here due to the above check
+      const starterAttributes = this.starterPreferences[this.lastSpecies.speciesId]!;
+      const originalStarterAttributes = this.originalStarterPreferences[this.lastSpecies.speciesId]!;
 
       // this gets the correct pokemon cursor depending on whether you're in the starter screen or the party icons
       if (!this.starterIconsCursorObj.visible) {
@@ -2050,10 +2059,6 @@ export class StarterSelectUiHandler extends MessageUiHandler {
                         const option: OptionSelectItem = {
                           label: getNatureName(n, true, true, true, globalScene.uiTheme),
                           handler: () => {
-                            // update default nature in starter save data
-                            if (!starterAttributes) {
-                              starterAttributes = this.starterPreferences[this.lastSpecies.speciesId] = {};
-                            }
                             starterAttributes.nature = n;
                             originalStarterAttributes.nature = starterAttributes.nature;
                             this.clearText();
@@ -3408,8 +3413,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
       if (species) {
         const defaultDexAttr = this.getCurrentDexProps(species.speciesId);
         const defaultProps = globalScene.gameData.getSpeciesDexAttrProps(species, defaultDexAttr);
+        // Bang is correct due to the `?` before variant
         const variant = this.starterPreferences[species.speciesId]?.variant
-          ? (this.starterPreferences[species.speciesId].variant as Variant)
+          ? (this.starterPreferences[species.speciesId]!.variant as Variant)
           : defaultProps.variant;
         const tint = getVariantTint(variant);
         this.pokemonShinyIcon.setFrame(getVariantIcon(variant)).setTint(tint);
@@ -3634,7 +3640,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
 
         if (starterIndex > -1) {
           props = globalScene.gameData.getSpeciesDexAttrProps(species, this.starterAttr[starterIndex]);
-          this.setSpeciesDetails(species, {
+          this.setSpeciesDetails(
+            species,
+            {
               shiny: props.shiny,
               formIndex: props.formIndex,
               female: props.female,
@@ -3642,7 +3650,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
               abilityIndex: this.starterAbilityIndexes[starterIndex],
               natureIndex: this.starterNatures[starterIndex],
               teraType: this.starterTeras[starterIndex],
-          });
+            },
+            false,
+          );
         } else {
           const defaultAbilityIndex =
             starterAttributes?.ability ?? globalScene.gameData.getStarterSpeciesDefaultAbilityIndex(species);
@@ -3659,7 +3669,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
           props.formIndex = starterAttributes?.form ?? props.formIndex;
           props.female = starterAttributes?.female ?? props.female;
 
-          this.setSpeciesDetails(species, {
+          this.setSpeciesDetails(
+            species,
+            {
               shiny: props.shiny,
               formIndex: props.formIndex,
               female: props.female,
@@ -3667,7 +3679,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
               abilityIndex: defaultAbilityIndex,
               natureIndex: defaultNature,
               teraType: starterAttributes?.tera,
-          });
+            },
+            false,
+          );
         }
 
         if (!isNullOrUndefined(props.formIndex)) {
@@ -3704,7 +3718,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
         const defaultNature = globalScene.gameData.getSpeciesDefaultNature(species);
         const props = globalScene.gameData.getSpeciesDexAttrProps(species, defaultDexAttr);
 
-        this.setSpeciesDetails(species, {
+        this.setSpeciesDetails(
+          species,
+          {
             shiny: props.shiny,
             formIndex: props.formIndex,
             female: props.female,
@@ -3712,7 +3728,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
             abilityIndex: defaultAbilityIndex,
             natureIndex: defaultNature,
             forSeen: true,
-        });
+          },
+          false,
+        );
         this.pokemonSprite.setTint(0x808080);
       }
     } else {
@@ -3734,7 +3752,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
       this.pokemonFormText.setVisible(false);
       this.teraIcon.setVisible(false);
 
-      this.setSpeciesDetails(species!, {
+      this.setSpeciesDetails(
+        species!,
+        {
           // TODO: is this bang correct?
           shiny: false,
           formIndex: 0,
@@ -3742,7 +3762,9 @@ export class StarterSelectUiHandler extends MessageUiHandler {
           variant: 0,
           abilityIndex: 0,
           natureIndex: 0,
-      });
+        },
+        false,
+      );
       this.pokemonSprite.clearTint();
     }
   }
@@ -3764,7 +3786,7 @@ export class StarterSelectUiHandler extends MessageUiHandler {
     return { dexEntry: { ...copiedDexEntry }, starterDataEntry: { ...copiedStarterDataEntry } };
   }
 
-  setSpeciesDetails(species: PokemonSpecies, options: SpeciesDetails = {}): void {
+  setSpeciesDetails(species: PokemonSpecies, options: SpeciesDetails = {}, save = true): void {
     let { shiny, formIndex, female, variant, abilityIndex, natureIndex, teraType } = options;
     const forSeen: boolean = options.forSeen ?? false;
     const oldProps = species ? globalScene.gameData.getSpeciesDexAttrProps(species, this.dexAttrCursor) : null;
@@ -4176,8 +4198,10 @@ export class StarterSelectUiHandler extends MessageUiHandler {
 
     this.updateInstructions();
 
+    if (save) {
       saveStarterPreferences(this.originalStarterPreferences);
     }
+  }
 
   setTypeIcons(type1: PokemonType | null, type2: PokemonType | null): void {
     if (type1 !== null) {

src/utils/data.ts

@@ -8,7 +8,7 @@ import { AES, enc } from "crypto-js";
  * @param values - The object to be deep copied.
  * @returns A new object that is a deep copy of the input.
  */
-export function deepCopy(values: object): object {
+export function deepCopy<T extends object>(values: T): T {
   // Convert the object to a JSON string and parse it back to an object to perform a deep copy
   return JSON.parse(JSON.stringify(values));
 }
@@ -58,13 +58,28 @@ export function decrypt(data: string, bypassLogin: boolean): string {
   return AES.decrypt(data, saveKey).toString(enc.Utf8);
 }
 
+/**
+ * Check if an object has no properties of its own (its shape is `{}`). An empty array is considered a bare object.
+ * @param obj - Object to check
+ * @returns - Whether the object is bare
+ */
+export function isBareObject(obj: any): boolean {
+  if (typeof obj !== "object") {
+    return false;
+  }
+  for (const _ in obj) {
+    return false;
+  }
+  return true;
+}
+
 // the latest data saved/loaded for the Starter Preferences. Required to reduce read/writes. Initialize as "{}", since this is the default value and no data needs to be stored if present.
 // if they ever add private static variables, move this into StarterPrefs
 const StarterPrefers_DEFAULT: string = "{}";
 let StarterPrefers_private_latest: string = StarterPrefers_DEFAULT;
 
 export interface StarterPreferences {
-  [key: number]: StarterAttributes;
+  [key: number]: StarterAttributes | undefined;
 }
 // called on starter selection show once
 
@@ -74,11 +89,17 @@ export function loadStarterPreferences(): StarterPreferences {
       localStorage.getItem(`starterPrefs_${loggedInUser?.username}`) || StarterPrefers_DEFAULT),
   );
 }
-// called on starter selection clear, always
 
 export function saveStarterPreferences(prefs: StarterPreferences): void {
-  const pStr: string = JSON.stringify(prefs);
+  // Fastest way to check if an object has any properties (does no allocation)
+  if (isBareObject(prefs)) {
+    console.warn("Refusing to save empty starter preferences");
+    return;
+  }
+  // no reason to store `{}` (for starters not customized)
+  const pStr: string = JSON.stringify(prefs, (_, value) => (isBareObject(value) ? undefined : value));
   if (pStr !== StarterPrefers_private_latest) {
+    console.log("%cSaving starter preferences", "color: blue");
     // something changed, store the update
     localStorage.setItem(`starterPrefs_${loggedInUser?.username}`, pStr);
     // update the latest prefs

test/utils/data.test.ts

@@ -0,0 +1,39 @@
+import { deepCopy, isBareObject } from "#utils/data";
+import { describe, expect, it } from "vitest";
+
+describe("Utils - Data", () => {
+  describe("deepCopy", () => {
+    it("should create a deep copy of an object", () => {
+      const original = { a: 1, b: { c: 2 } };
+      const copy = deepCopy(original);
+      // ensure the references are different
+      expect(copy === original, "copied object should not compare equal").not;
+      expect(copy).toEqual(original);
+      // update copy's `a` to a different value and ensure original is unaffected
+      copy.a = 42;
+      expect(original.a, "adjusting property of copy should not affect original").toBe(1);
+      // update copy's nested `b.c` to a different value and ensure original is unaffected
+      copy.b.c = 99;
+      expect(original.b.c, "adjusting nested property of copy should not affect original").toBe(2);
+    });
+  });
+
+  describe("isBareObject", () => {
+    it("should properly identify bare objects", () => {
+      expect(isBareObject({}), "{} should be considered bare");
+      expect(isBareObject(new Object()), "new Object() should be considered bare");
+      expect(isBareObject(Object.create(null)));
+      expect(isBareObject([]), "an empty array should be considered bare");
+    });
+
+    it("should properly reject non-objects", () => {
+      expect(isBareObject(new Date())).not;
+      expect(isBareObject(null)).not;
+      expect(isBareObject(42)).not;
+      expect(isBareObject("")).not;
+      expect(isBareObject(undefined)).not;
+      expect(isBareObject(() => {})).not;
+      expect(isBareObject(new (class A {})())).not;
+    });
+  });
+});