From c57e5b7639fd643b9fb580fa4abc66a3ee684fb3 Mon Sep 17 00:00:00 2001 From: Jarrod Servilla Date: Mon, 13 Jul 2026 14:31:20 -0400 Subject: [PATCH 1/2] fix(deck): tolerate stale deleted category in move-card menu (#88) A category can be deleted (by this user in another tab or by a collaborator) while the move-card menu is open and still showing it. Toggling that stale category previously threw inside startTransition, tripping the error boundary instead of failing gracefully. Defense on both sides: - Client (`applyCategories`) filters memberships against the live `subcategories` list before dispatch via the new exported `filterLiveCategories` helper, so a stale entry is never sent. - Server actions silently drop / degrade unknown category names instead of throwing: `setCardCategories` filters them out, and `moveCardTo` degrades a deleted target to an uncategorized MAINBOARD move. Regression tests cover the client filter and both server-action paths. --- .../deck/__tests__/categories.test.ts | 43 +++++++++++++++---- app/_actions/deck/categories.ts | 32 +++++++++----- .../builder/__tests__/move-card-menu.test.tsx | 26 ++++++++++- app/_components/builder/move-card-menu.tsx | 27 ++++++++++-- 4 files changed, 104 insertions(+), 24 deletions(-) diff --git a/app/_actions/deck/__tests__/categories.test.ts b/app/_actions/deck/__tests__/categories.test.ts index 312f61f..0a3e2d1 100644 --- a/app/_actions/deck/__tests__/categories.test.ts +++ b/app/_actions/deck/__tests__/categories.test.ts @@ -548,15 +548,34 @@ describe("setCardCategories", () => { expect(mockApply).not.toHaveBeenCalled(); }); - it("throws when a category is not in the deck registry", async () => { + it("silently drops a stale category no longer in the deck registry (issue #88)", async () => { mockCardFindUnique.mockResolvedValue( cardRow("dc-1", Zone.MAINBOARD, []) as never, ); + // "ghost" was deleted while the menu was open, so the registry only knows + // "ramp". The action should keep the surviving name rather than throw. mockCategoryFindMany.mockResolvedValue([{ name: "ramp" }] as never); - await expect( - setCardCategories(DECK_ID, "dc-1", ["Ramp", "Ghost"]), - ).rejects.toThrow('Category "ghost" not found in deck'); + await setCardCategories(DECK_ID, "dc-1", ["Ramp", "Ghost"]); + + expect(moveChange()).toEqual({ + op: "move", + deckCardId: "dc-1", + zone: Zone.MAINBOARD, + categories: ["ramp"], + }); + }); + + it("no-ops when every requested category is stale (issue #88)", async () => { + mockCardFindUnique.mockResolvedValue( + cardRow("dc-1", Zone.MAINBOARD, []) as never, + ); + mockCategoryFindMany.mockResolvedValue([] as never); + + await setCardCategories(DECK_ID, "dc-1", ["Ghost"]); + + // Filtering leaves an empty list that matches the card's current (empty) + // memberships, so no mutation is emitted. expect(mockApply).not.toHaveBeenCalled(); }); @@ -668,16 +687,22 @@ describe("moveCardTo", () => { expect(mockApply).not.toHaveBeenCalled(); }); - it("throws when the target subcategory does not exist", async () => { + it("degrades to an uncategorized MAINBOARD move when the target subcategory was deleted (issue #88)", async () => { mockCardFindUnique.mockResolvedValue( cardRow("dc-1", Zone.SIDEBOARD, []) as never, ); + // "ghost" was deleted while the menu was open. Rather than throwing, the + // card still moves into MAINBOARD with no membership. mockCategoryFindUnique.mockResolvedValue(null as never); - await expect( - moveCardTo(DECK_ID, "dc-1", Zone.MAINBOARD, "Ghost"), - ).rejects.toThrow('Category "ghost" not found in deck'); - expect(mockApply).not.toHaveBeenCalled(); + await moveCardTo(DECK_ID, "dc-1", Zone.MAINBOARD, "Ghost"); + + expect(moveChange()).toEqual({ + op: "move", + deckCardId: "dc-1", + zone: Zone.MAINBOARD, + categories: [], + }); }); it("is a no-op when the card is already in the target zone with the target primary", async () => { diff --git a/app/_actions/deck/categories.ts b/app/_actions/deck/categories.ts index 1e80e8d..2474411 100644 --- a/app/_actions/deck/categories.ts +++ b/app/_actions/deck/categories.ts @@ -283,17 +283,26 @@ export const moveCardTo = runOwnerDeckMutation( throw new Error("Card not found or unauthorized"); } + // If the target subcategory was deleted (by this user in another tab or + // by a collaborator) between menu-open and dispatch, degrade to an + // uncategorized MAINBOARD move rather than throwing, which would trip the + // error boundary. See issue #88. + let effectiveCategory = normalizedCategory; if (nextZone === Zone.MAINBOARD && normalizedCategory !== null) { - await assertCategoryExists(deckId, normalizedCategory); + const exists = await prisma.deckCategory.findUnique({ + where: { deckId_name: { deckId, name: normalizedCategory } }, + select: { id: true }, + }); + if (!exists) effectiveCategory = null; } const current = sourceCard.categoryLinks.map((l) => l.deckCategory.name); const nextCategories = - nextZone !== Zone.MAINBOARD || normalizedCategory === null + nextZone !== Zone.MAINBOARD || effectiveCategory === null ? [] : [ - normalizedCategory, - ...current.filter((name) => name !== normalizedCategory), + effectiveCategory, + ...current.filter((name) => name !== effectiveCategory), ]; if ( @@ -350,28 +359,29 @@ export const setCardCategories = runOwnerDeckMutation( throw new Error("Subcategories only apply to MAINBOARD cards"); } + let effective = normalized; if (normalized.length > 0) { const known = await prisma.deckCategory.findMany({ where: { deckId, name: { in: normalized } }, select: { name: true }, }); const knownNames = new Set(known.map((c) => c.name)); - for (const name of normalized) { - if (!knownNames.has(name)) { - throw new Error(`Category "${name}" not found in deck`); - } - } + // A category can be deleted (by this user in another tab or by a + // collaborator) while a move-card menu still shows it. Silently drop + // such stale names instead of throwing, which would trip the error + // boundary. See issue #88. + effective = normalized.filter((name) => knownNames.has(name)); } const current = sourceCard.categoryLinks.map((l) => l.deckCategory.name); - if (current.join("\u0000") === normalized.join("\u0000")) return; + if (current.join("\u0000") === effective.join("\u0000")) return; await applyChanges(deckId, userId, [ { op: "move", deckCardId, zone: Zone.MAINBOARD, - categories: normalized, + categories: effective, }, ]); }, diff --git a/app/_components/builder/__tests__/move-card-menu.test.tsx b/app/_components/builder/__tests__/move-card-menu.test.tsx index 7d66cda..5e8176a 100644 --- a/app/_components/builder/__tests__/move-card-menu.test.tsx +++ b/app/_components/builder/__tests__/move-card-menu.test.tsx @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; -import { orderZoneOptions } from "@/app/_components/builder/move-card-menu"; +import { + filterLiveCategories, + orderZoneOptions, +} from "@/app/_components/builder/move-card-menu"; describe("orderZoneOptions", () => { it("keeps Commander first when no commander is set", () => { @@ -32,3 +35,24 @@ describe("orderZoneOptions", () => { expect(set.slice(0, -1)).toEqual(unset); }); }); + +describe("filterLiveCategories (issue #88)", () => { + it("drops a stale membership no longer in the live registry", () => { + // "ghost" was deleted while the menu was open; toggling "draw" must not + // resend it. + expect( + filterLiveCategories(["ramp", "ghost", "draw"], ["ramp", "draw"]), + ).toEqual(["ramp", "draw"]); + }); + + it("preserves order of the surviving memberships", () => { + expect( + filterLiveCategories(["draw", "ramp"], ["ramp", "draw", "removal"]), + ).toEqual(["draw", "ramp"]); + }); + + it("returns an empty list when every name is stale", () => { + expect(filterLiveCategories(["ghost"], ["ramp"])).toEqual([]); + expect(filterLiveCategories([], ["ramp"])).toEqual([]); + }); +}); diff --git a/app/_components/builder/move-card-menu.tsx b/app/_components/builder/move-card-menu.tsx index e2f2c98..bafc383 100644 --- a/app/_components/builder/move-card-menu.tsx +++ b/app/_components/builder/move-card-menu.tsx @@ -66,6 +66,20 @@ export function orderZoneOptions(commanderSet: boolean): ZoneOption[] { return [...ZONE_OPTIONS.filter((o) => o.value !== "COMMANDER"), commander]; } +/** + * Drop membership names that are no longer in the live category registry. A + * category can be deleted (by this user in another tab or by a collaborator) + * while this menu is open, leaving a stale entry in `currentCategories`; + * dispatching it would trip the error boundary. Preserves order. See issue #88. + */ +export function filterLiveCategories( + next: string[], + subcategories: string[], +): string[] { + const live = new Set(subcategories); + return next.filter((c) => live.has(c)); +} + type Tab = "actions" | "category" | "zone" ; export function MoveCardMenu({ @@ -145,18 +159,25 @@ export function MoveCardMenu({ * are MAINBOARD-only. */ function applyCategories(next: string[]) { + // Drop any membership no longer in the live registry so a stale entry is + // never dispatched. The server actions tolerate unknown names too (issue + // #88); this keeps the optimistic view in sync once `subcategories` has + // refreshed. + const filtered = filterLiveCategories(next, subcategories); startTransition(async () => { dispatch({ type: "move", deckCardId, zone: "MAINBOARD", - categories: next, + categories: filtered, }); if (currentZone === "MAINBOARD") { - await enqueueMutation(() => setCardCategories(deckId, deckCardId, next)); + await enqueueMutation(() => + setCardCategories(deckId, deckCardId, filtered), + ); } else { await enqueueMutation(() => - moveCardTo(deckId, deckCardId, "MAINBOARD", next[0] ?? null), + moveCardTo(deckId, deckCardId, "MAINBOARD", filtered[0] ?? null), ); } }); From a7145da931389b2bbb882cedacc5f23115a07962 Mon Sep 17 00:00:00 2001 From: Jarrod Servilla Date: Wed, 15 Jul 2026 21:05:38 -0700 Subject: [PATCH 2/2] refactor(deck): extract shared checkCategoryExists helper (#88) assertCategoryExists and the moveCardTo effectiveCategory fallback ran the identical deckCategory.findUnique existence lookup. Extract it into a shared checkCategoryExists helper both reuse; missing-category behavior is unchanged (assert still throws, moveCardTo still degrades to an uncategorized move). --- app/_actions/deck/categories.ts | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/app/_actions/deck/categories.ts b/app/_actions/deck/categories.ts index 2474411..8c15a69 100644 --- a/app/_actions/deck/categories.ts +++ b/app/_actions/deck/categories.ts @@ -48,15 +48,22 @@ async function loadCategoryMembers( })); } -async function assertCategoryExists( +async function checkCategoryExists( deckId: string, name: string, -): Promise { +): Promise { const exists = await prisma.deckCategory.findUnique({ where: { deckId_name: { deckId, name } }, select: { id: true }, }); - if (!exists) { + return exists !== null; +} + +async function assertCategoryExists( + deckId: string, + name: string, +): Promise { + if (!(await checkCategoryExists(deckId, name))) { throw new Error(`Category "${name}" not found in deck`); } } @@ -288,12 +295,12 @@ export const moveCardTo = runOwnerDeckMutation( // uncategorized MAINBOARD move rather than throwing, which would trip the // error boundary. See issue #88. let effectiveCategory = normalizedCategory; - if (nextZone === Zone.MAINBOARD && normalizedCategory !== null) { - const exists = await prisma.deckCategory.findUnique({ - where: { deckId_name: { deckId, name: normalizedCategory } }, - select: { id: true }, - }); - if (!exists) effectiveCategory = null; + if ( + nextZone === Zone.MAINBOARD && + normalizedCategory !== null && + !(await checkCategoryExists(deckId, normalizedCategory)) + ) { + effectiveCategory = null; } const current = sourceCard.categoryLinks.map((l) => l.deckCategory.name);