From 60a726883be85c174e88ae832ef95f3c0f084ec9 Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Fri, 4 Sep 2026 00:51:47 +0900 Subject: fix(plugins): make groupByTier duplicate-key-proof; normalize tiers at the boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PluginTable.svelte renders a keyed each ({#each groups as group (group.tier)}); groupByTier's old "rest" bucket for unknown tiers fell back to key "standard", producing a DUPLICATE standard group (and thus an each_key_duplicate crash in Svelte 5, blanking both manager tables) whenever tierless rows were present alongside canonical standard rows. groupByTier now buckets through a Map keyed by the NORMALIZED tier — each canonical tier can yield at most one group, so duplicate keys are impossible by construction, and no row is ever dropped. Unknown/missing tiers fold into the standard bucket rather than forming a separate trailing group. The normalization (normalizeTier, in types.ts next to tierLabel/TIER_ORDER) also runs at the pluginNodes boundary, so every node's tier is canonical before any tier-keyed consumer sees it — including tierLabel's exhaustive switch, which rendered an empty badge for out-of-contract values. Normalizing into an existing bucket (option a) was chosen over a synthetic "unknown" group because it needs no new UI vocabulary and keeps every consumer total. Tests: all-canonical order, unknown tier ("kernel"), and missing tier each assert no duplicate keys, no dropped rows, and the right group/row counts from the Loaded-table view; the boundary test pins pluginNodes' canonical output. Plugins-feature vitest green (95 tests); svelte-check, biome, and vite build green. --- src/features/plugins/logic/types.ts | 17 +++++ src/features/plugins/logic/view-model.test.ts | 102 +++++++++++++++++++++++++- src/features/plugins/logic/view-model.ts | 40 +++++++--- 3 files changed, 147 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/features/plugins/logic/types.ts b/src/features/plugins/logic/types.ts index 401913a..702483b 100644 --- a/src/features/plugins/logic/types.ts +++ b/src/features/plugins/logic/types.ts @@ -42,3 +42,20 @@ export function tierLabel(tier: PluginTier): string { /** Display order of the tiers within a manager table (Core first). */ export const TIER_ORDER: readonly PluginTier[] = ["core", "providers", "standard"]; + +/** + * Runtime-normalize a tier value to the canonical THREE: anything that is not + * exactly `core` | `providers` | `standard` — a missing tier, or a value + * outside the contract ("kernel" is NOT a tier; the kernel hosts plugins, it + * is not one) — folds into `"standard"`, the safe catch-all classification. + * The wire contract pins the three values, so this only ever fires on a + * contract violation (the backend rejects tierless externals at boot and + * directory-scanned plugins fail manifest validation); folding rather than + * surfacing the raw value keeps every tier-keyed consumer total — e.g. + * `tierLabel`'s exhaustive switch (an out-of-contract value would render an + * empty badge) and the manager table's keyed grouping (a non-canonical key + * would create a second, colliding group). + */ +export function normalizeTier(tier: unknown): PluginTier { + return tier === "core" || tier === "providers" || tier === "standard" ? tier : "standard"; +} diff --git a/src/features/plugins/logic/view-model.test.ts b/src/features/plugins/logic/view-model.test.ts index c4a4547..680ba94 100644 --- a/src/features/plugins/logic/view-model.test.ts +++ b/src/features/plugins/logic/view-model.test.ts @@ -1,6 +1,6 @@ -import type { PluginListResponse } from "@dispatch/transport-contract"; +import type { PluginListResponse, PluginTier } from "@dispatch/transport-contract"; import { describe, expect, it } from "vitest"; -import { emptyListResponse } from "./test-fixtures"; +import { emptyListResponse, node } from "./test-fixtures"; import { groupByTier, pluginNodes, splitSections } from "./view-model"; function listResponse(overrides: Partial = {}): PluginListResponse { @@ -103,6 +103,55 @@ describe("pluginNodes", () => { it("handles the empty response", () => { expect(pluginNodes(emptyListResponse())).toEqual([]); }); + + it("normalizes a missing/unknown tier at the boundary so node tiers are canonical", () => { + // Contract-violation casts: the wire type pins three tier values, but the + // view-model must survive a backend that emits a tierless or out-of- + // contract row (the original duplicate-key crash trigger). + const response = listResponse({ + loaded: [ + { + id: "kernel", + name: "Kernel", + version: "1.0.0", + tier: "core", + trust: "bundled", + locked: true, + dependsOn: [], + }, + { + id: "tierless", + name: "Tierless", + version: "1.0.0", + tier: undefined as unknown as PluginTier, + trust: "external", + locked: false, + dependsOn: [], + }, + { + id: "kernelish", + name: "Kernelish", + version: "1.0.0", + tier: "kernel" as unknown as PluginTier, + trust: "external", + locked: false, + dependsOn: [], + }, + ], + }); + const nodes = pluginNodes(response); + expect(nodes.find((n) => n.id === "tierless")?.tier).toBe("standard"); + expect(nodes.find((n) => n.id === "kernelish")?.tier).toBe("standard"); + + // The Loaded table's grouping: exactly two groups (core + standard), no + // duplicate keys, every row present. + const groups = groupByTier(nodes.filter((n) => n.section === "loaded")); + const keys = groups.map((g) => g.tier); + expect(new Set(keys).size).toBe(keys.length); + expect(keys).toEqual(["core", "standard"]); + expect(groups[0]?.rows.map((r) => r.id)).toEqual(["kernel"]); + expect(groups[1]?.rows.map((r) => r.id)).toEqual(["kernelish", "tierless"]); + }); }); describe("groupByTier", () => { @@ -117,6 +166,55 @@ describe("groupByTier", () => { const groups = groupByTier([pluginNodes(listResponse())[2] ?? []].flat()); expect(groups.map((g) => g.tier)).toEqual(["standard"]); }); + + it("emits one group per canonical tier, all three present", () => { + const groups = groupByTier([ + node({ id: "s", tier: "standard" }), + node({ id: "c", tier: "core" }), + node({ id: "p", tier: "providers" }), + ]); + expect(groups.map((g) => g.tier)).toEqual(["core", "providers", "standard"]); + expect(groups.flatMap((g) => g.rows.map((r) => r.id))).toEqual(["c", "p", "s"]); + }); + + it("never emits duplicate group keys — an unknown tier folds into standard", () => { + // Contract violation cast: "kernel" is NOT a tier (the kernel hosts + // plugins, it is not one). groupByTier must stay total anyway. + const groups = groupByTier([ + node({ id: "s-row", tier: "standard" }), + node({ id: "k-row", tier: "kernel" as PluginTier }), + ]); + const keys = groups.map((g) => g.tier); + // No duplicates (the keyed each `(group.tier)` crashes on them)… + expect(new Set(keys).size).toBe(keys.length); + // …and the unknown-tier row is NOT dropped — it merges into standard. + expect(keys).toEqual(["standard"]); + expect(groups[0]?.rows.map((r) => r.id)).toEqual(["k-row", "s-row"]); + }); + + it("never emits duplicate group keys — a missing tier folds into standard", () => { + const groups = groupByTier([ + node({ id: "s-row", tier: "standard" }), + node({ id: "m-row", tier: undefined as unknown as PluginTier }), + ]); + const keys = groups.map((g) => g.tier); + expect(new Set(keys).size).toBe(keys.length); + expect(keys).toEqual(["standard"]); + expect(groups[0]?.rows.map((r) => r.id)).toEqual(["m-row", "s-row"]); + }); + + it("keeps unknown/missing tiers out of the canonical groups' identity (no second standard group)", () => { + // The old implementation pushed a trailing "rest" group keyed + // `rest[0]?.tier ?? "standard"` — a DUPLICATE standard key when canonical + // standard rows existed (the headless-Chromium each_key_duplicate crash). + const groups = groupByTier([ + node({ id: "a", tier: "standard" }), + node({ id: "b", tier: "bogus" as PluginTier }), + node({ id: "c", tier: undefined as unknown as PluginTier }), + ]); + expect(groups.map((g) => g.tier)).toEqual(["standard"]); + expect(groups[0]?.rows).toHaveLength(3); + }); }); describe("splitSections", () => { diff --git a/src/features/plugins/logic/view-model.ts b/src/features/plugins/logic/view-model.ts index 23d57d9..20d7d68 100644 --- a/src/features/plugins/logic/view-model.ts +++ b/src/features/plugins/logic/view-model.ts @@ -1,5 +1,5 @@ import type { PluginListResponse } from "@dispatch/transport-contract"; -import { type PluginNode, type PluginSection, TIER_ORDER } from "./types"; +import { normalizeTier, type PluginNode, type PluginSection, TIER_ORDER } from "./types"; /** * Normalize a `GET /plugins` response into the flat `PluginNode` list the @@ -21,7 +21,10 @@ export function pluginNodes(response: PluginListResponse): readonly PluginNode[] id: row.id, name: row.name, version: row.version, - tier: row.tier, + // Normalize at the boundary: a missing/out-of-contract tier (backend + // bug, stale contract) folds into "standard" so every node's tier is + // canonical and the tier-keyed consumers below stay total. + tier: normalizeTier(row.tier), trust: row.trust, loaded: true, locked: row.locked, @@ -35,7 +38,7 @@ export function pluginNodes(response: PluginListResponse): readonly PluginNode[] id: row.id, name: row.name, version: row.version, - tier: row.tier, + tier: normalizeTier(row.tier), trust: row.trust, loaded: false, locked: false, @@ -49,7 +52,7 @@ export function pluginNodes(response: PluginListResponse): readonly PluginNode[] id: row.id, name: row.name, version: row.version, - tier: row.tier, + tier: normalizeTier(row.tier), trust: row.trust, loaded: false, locked: false, @@ -79,19 +82,36 @@ export function pluginNodes(response: PluginListResponse): readonly PluginNode[] * Cluster nodes for one manager table: tier groups in display order (Core → * Providers → Standard), each tier's rows sorted by id. Tiers with no rows * are omitted. Used by the loaded table's tier grouping (plan 07 §7.2). + * + * Duplicate-key-proof BY CONSTRUCTION: nodes are bucketed through a Map keyed + * by the NORMALIZED tier, so each of the three canonical tiers can yield at + * most one group — `PluginTable.svelte` renders a keyed each + * (`{#each groups as group (group.tier)}`), and a duplicate key throws + * `each_key_duplicate` in Svelte 5, aborting the whole manager render. The + * normalization also means an unknown/missing tier (contract violation — the + * backend rejects tierless externals at boot and directory scans fail + * validation) folds into the `standard` bucket instead of forming a separate + * trailing group; the old fallback (`rest[0]?.tier ?? "standard"`) produced a + * DUPLICATE `standard` key when canonical standard rows were present. + * groupByTier normalizes on the way in even though `pluginNodes` already + * does at the boundary — it is exported and must be safe for any caller. */ export function groupByTier( nodes: readonly PluginNode[], ): ReadonlyArray<{ readonly tier: PluginNode["tier"]; readonly rows: readonly PluginNode[] }> { + const byTier = new Map(); + for (const n of nodes) { + const tier = normalizeTier(n.tier); + const rows = byTier.get(tier); + if (rows) rows.push(n); + else byTier.set(tier, [n]); + } const out: { tier: PluginNode["tier"]; rows: PluginNode[] }[] = []; for (const tier of TIER_ORDER) { - const rows = nodes.filter((n) => n.tier === tier).sort((a, b) => a.id.localeCompare(b.id)); - if (rows.length > 0) out.push({ tier, rows }); + const rows = byTier.get(tier); + if (!rows || rows.length === 0) continue; + out.push({ tier, rows: rows.sort((a, b) => a.id.localeCompare(b.id)) }); } - // Any tier outside TIER_ORDER (defensive — the contract pins three values). - const known = new Set(TIER_ORDER); - const rest = nodes.filter((n) => !known.has(n.tier)).sort((a, b) => a.id.localeCompare(b.id)); - if (rest.length > 0) out.push({ tier: rest[0]?.tier ?? "standard", rows: rest }); return out; } -- cgit v1.2.3