Sanitize ModDB release entries at the parser boundary

src/domain/mods/moddb.ts

@@ -120,9 +120,32 @@ export function parseModListResponse(rawText: string): ModDbResponse<ModDbModSum
 export interface ModDbModDetail extends Record<string, unknown> {
   modid: number
   name: string
+  /** Always a list of strings, whatever the API sent: see {@link readModDetail}. */
+  tags: string[]
+  /** Always a list of objects, each with string `modversion` and string-only `tags`: see {@link readRelease}. */
+  releases: Record<string, unknown>[]
   logofile?: string
 }
 
+/**
+ * Cleans one entry of a detail's `releases` array.
+ *
+ * Every reader downstream, the update scan, the release list, the modpack planner, treats a release
+ * as an object with a string `modversion` and a `tags` array of strings, because that is what the
+ * ModDB documents. It is not what the ModDB always sends: a listing has been seen with a `null` in
+ * the array and with `null` under `tags`, which is what took the Manage Mods page down in beta.7.
+ *
+ * `modversion` is coerced to an empty string rather than dropping the release, because
+ * {@link newestReleaseFileId} reads `releases[0]` to build the download URL for the newest file:
+ * removing an entry here would silently shift which file "install newest" picks. Every reader
+ * already treats a falsy `modversion` as unusable, so an empty one costs that release nothing but
+ * the version comparisons it could never have taken part in anyway.
+ */
+function readRelease(value: unknown): Record<string, unknown> | undefined {
+  if (!isRecord(value)) return undefined
+  return { ...value, tags: cleanStrings(value["tags"]), modversion: typeof value["modversion"] === "string" ? value["modversion"] : "" }
+}
+
 function readModDetail(value: unknown): ModDbModDetail | undefined {
   if (!isRecord(value)) return undefined
 
@@ -132,9 +155,20 @@ function readModDetail(value: unknown): ModDbModDetail | undefined {
   // The install popup maps `releases` directly, so a detail without the array would crash it
   // instead of reaching its failure state. Unlike modid and name this is not a field the API is
   // known to always send, hence the explicit check rather than trusting the shape.
-  if (!Array.isArray(value["releases"])) return undefined
-
-  return { ...value, modid, name, ...(typeof value["logofile"] === "string" ? { logofile: value["logofile"] } : {}) }
+  const releases = value["releases"]
+  if (!Array.isArray(releases)) return undefined
+
+  return {
+    ...value,
+    modid,
+    name,
+    // Mod-level tags carry the same `null` the Vanilla Variants listing shipped in beta.7. The
+    // installed filters tolerate it on their own since #371, cleaning it here makes that tolerance
+    // a second line rather than the only one.
+    tags: cleanStrings(value["tags"]),
+    releases: releases.map(readRelease).filter((release): release is Record<string, unknown> => release !== undefined),
+    ...(typeof value["logofile"] === "string" ? { logofile: value["logofile"] } : {})
+  }
 }
 
 /**
@@ -155,10 +189,7 @@ export function parseModDetailResponse(rawText: string): ModDbResponse<ModDbModD
  * rather than trusted because it ends up in a URL.
  */
 export function newestReleaseFileId(detail: ModDbModDetail): number | undefined {
-  const newest = (detail["releases"] as unknown[])[0]
-  if (!isRecord(newest)) return undefined
-
-  const fileId = newest["fileid"]
+  const fileId = detail.releases[0]?.["fileid"]
   return typeof fileId === "number" && Number.isSafeInteger(fileId) && fileId > 0 ? fileId : undefined
 }
 

tests/domain/mods/moddb.test.ts

@@ -113,7 +113,7 @@ describe("parseModDetailResponse", () => {
     assert.equal(result.payload.modid, 1783)
     assert.equal(result.payload.name, "Config lib")
     assert.equal(result.payload.logofile, "https://moddbcdn.vintagestory.at/config.png")
-    assert.deepEqual(result.payload["releases"], [{ releaseid: 1, modversion: "1.12.0" }])
+    assert.deepEqual(result.payload["releases"], [{ releaseid: 1, modversion: "1.12.0", tags: [] }])
   })
 
   it("names a mod field that is not an object as malformed", () => {
@@ -142,6 +142,72 @@ describe("parseModDetailResponse", () => {
   })
 })
 
+/**
+ * Issue #370: the ModDB served Vanilla Variants with a `null` among its tags, and the first render
+ * of Manage Mods died on it. The hotfix taught the installed filters to survive that. These pin the
+ * same shapes at the boundary instead, so that no reader downstream has to know about them.
+ */
+describe("parseModDetailResponse: shapes the ModDB actually sends", () => {
+  function detailOf(mod: unknown): ModDbModDetail {
+    const result = parseModDetailResponse(JSON.stringify({ statuscode: "200", mod }))
+    if (!result.ok) throw new Error("unreachable")
+    return result.payload
+  }
+
+  it("keeps a null out of the mod's own tags, the Vanilla Variants payload that took the page down", () => {
+    const detail = detailOf({ modid: 1, name: "Vanilla Variants", tags: ["Cosmetics", "Crafting", "Storage", null], releases: [] })
+    assert.deepEqual(detail.tags, ["Cosmetics", "Crafting", "Storage"])
+  })
+
+  it("reads a missing or wrongly typed mod tags field as no tags at all", () => {
+    assert.deepEqual(detailOf({ modid: 1, name: "No Tags", releases: [] }).tags, [])
+    assert.deepEqual(detailOf({ modid: 1, name: "Odd Tags", tags: "Storage", releases: [] }).tags, [])
+  })
+
+  it("drops a release that is not an object, which every consumer dereferences unchecked", () => {
+    const detail = detailOf({ modid: 1, name: "Nulled", releases: [null, { releaseid: 2, modversion: "1.1.0" }, "not a release"] })
+    assert.deepEqual(
+      detail.releases.map((release) => release["releaseid"]),
+      [2]
+    )
+  })
+
+  it("keeps a null out of a release's tags, which reach evaluateModCompatibility raw", () => {
+    const detail = detailOf({
+      modid: 1,
+      name: "Tagged",
+      releases: [
+        { modversion: "1.1.0", tags: ["1.21.0", null] },
+        { modversion: "1.0.0", tags: null }
+      ]
+    })
+    assert.deepEqual(
+      detail.releases.map((release) => release["tags"]),
+      [["1.21.0"], []]
+    )
+  })
+
+  it("reads a release modversion that is not a string as an empty one, rather than dropping the release", () => {
+    // Dropping it would shift releases[0], which newestReleaseFileId turns into the download URL.
+    const detail = detailOf({ modid: 1, name: "Versionless", releases: [{ fileid: 42, modversion: null }] })
+    assert.equal(detail.releases[0]?.["modversion"], "")
+    assert.equal(newestReleaseFileId(detail), 42)
+  })
+
+  it("carries every other release field through untouched", () => {
+    const detail = detailOf({ modid: 1, name: "Whole", releases: [{ releaseid: 9, fileid: 42, mainfile: "https://mods.example/a.zip", modidstr: "a", changelog: "<p>hi</p>" }] })
+    assert.deepEqual(detail.releases[0], {
+      releaseid: 9,
+      fileid: 42,
+      mainfile: "https://mods.example/a.zip",
+      modidstr: "a",
+      changelog: "<p>hi</p>",
+      modversion: "",
+      tags: []
+    })
+  })
+})
+
 describe("newestReleaseFileId", () => {
   function detail(releases: unknown[]): ModDbModDetail {
     const result = parseModDetailResponse(JSON.stringify({ statuscode: "200", mod: { modid: 11016, name: "RiftLauncher", releases } }))

tests/renderer-dom/manageMods.test.tsx

@@ -1016,3 +1016,66 @@ describe("ManageMods: filtering the installed Mods", { timeout: 20000 }, () => {
     expect(screen.getByRole("button", { name: "VS Version" })).toBeTruthy()
   })
 })
+
+/**
+ * Issue #370, and Zaldaryon's review of #374 on top of it. Beta.7 shipped a blank Manage Mods page
+ * because the ModDB served Vanilla Variants with a `null` among its tags. The hotfix taught the
+ * installed filters to read around that, but `useGetCompleteInstalledMods` still walks the raw
+ * `releases` array first, so the page died before the filters ever ran.
+ *
+ * The payload below is deliberately raw and deliberately ugly: it goes through the real
+ * `parseModDetailResponse` on its way in, which is where the shape is now fixed.
+ */
+describe("ManageMods: a ModDB payload with nulls in it", () => {
+  const ALPHA_MALFORMED = JSON.stringify({
+    statuscode: "200",
+    mod: {
+      modid: 1,
+      assetid: 101,
+      name: "Alpha Mod",
+      // The Vanilla Variants tag list, verbatim.
+      tags: ["Cosmetics", "Crafting", "Storage", null],
+      releases: [
+        // A hole in the array: `release.modversion` on this one is what threw.
+        null,
+        { releaseid: 14, mainfile: "https://mods.example/alpha-1.3.0.zip", filename: "alpha-1.3.0.zip", fileid: 14, tags: null, modidstr: "alpha", modversion: "1.3.0" },
+        { releaseid: 13, mainfile: "https://mods.example/alpha-1.2.0.zip", filename: "alpha-1.2.0.zip", fileid: 13, tags: ["1.20.0", null], modidstr: "alpha", modversion: "1.2.0" },
+        { releaseid: 12, mainfile: "https://mods.example/alpha-1.1.0.zip", filename: "alpha-1.1.0.zip", fileid: 12, tags: ["1.20.0"], modidstr: "alpha", modversion: null }
+      ]
+    }
+  })
+
+  function queryMalformedModDb(url: string): Promise<string> {
+    if (url.endsWith("/mod/alpha")) return Promise.resolve(ALPHA_MALFORMED)
+    return queryModDb(url)
+  }
+
+  it("finishes loading and lists the Mod instead of taking the page down", async () => {
+    renderManageMods({ netManager: { queryURL: vi.fn(queryMalformedModDb) } })
+
+    // The page renders at all, which is the whole of #370.
+    expect(await screen.findByText("Alpha Mod", {}, { timeout: 3000 })).toBeTruthy()
+    expect(screen.getByText("Beta Mod")).toBeTruthy()
+    expect(screen.getByText("Gamma Mod")).toBeTruthy()
+    expect(screen.getByText("broken.zip")).toBeTruthy()
+  })
+
+  it("still reads the update off the releases that survived the cleaning", async () => {
+    const user = userEvent.setup()
+    renderManageMods({
+      netManager: { queryURL: vi.fn(queryMalformedModDb) },
+      modsManager: { getInstalledMods: vi.fn(async () => ({ mods: [aModScan().mods[0] as InstalledModType], errors: [] })) },
+      pathsManager: { deletePath: vi.fn(async () => true), downloadOnPath: vi.fn(async () => "/games/a/Mods/alpha-1.2.0.zip") }
+    })
+
+    await screen.findByText("Alpha Mod", {}, { timeout: 3000 })
+    expect(screen.getByText("Mods with updates")).toBeTruthy()
+
+    await user.click(screen.getByText("Update all").closest("button") as HTMLElement)
+
+    // 1.3.0 is newer still, but its tags came in as `null`: undeclared, so the offer is 1.2.0, the
+    // newest release actually tagged for this Installation once its own null tag was dropped.
+    const summary = await screen.findByRole("dialog", {}, { timeout: 3000 })
+    expect(within(summary).getByText("v1.2.0")).toBeTruthy()
+  })
+})