Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions app/_actions/deck/__tests__/categories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PlannedChange>({
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();
});

Expand Down Expand Up @@ -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<PlannedChange>({
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 () => {
Expand Down
47 changes: 32 additions & 15 deletions app/_actions/deck/categories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,22 @@ async function loadCategoryMembers(
}));
}

async function assertCategoryExists(
async function checkCategoryExists(
deckId: string,
name: string,
): Promise<void> {
): Promise<boolean> {
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<void> {
if (!(await checkCategoryExists(deckId, name))) {
throw new Error(`Category "${name}" not found in deck`);
}
}
Expand Down Expand Up @@ -283,17 +290,26 @@ export const moveCardTo = runOwnerDeckMutation(
throw new Error("Card not found or unauthorized");
}

if (nextZone === Zone.MAINBOARD && normalizedCategory !== null) {
await assertCategoryExists(deckId, normalizedCategory);
// 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 checkCategoryExists(deckId, normalizedCategory))
) {
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 (
Expand Down Expand Up @@ -350,28 +366,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,
},
]);
},
Expand Down
26 changes: 25 additions & 1 deletion app/_components/builder/__tests__/move-card-menu.test.tsx
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand Down Expand Up @@ -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([]);
});
});
27 changes: 24 additions & 3 deletions app/_components/builder/move-card-menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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),
);
}
});
Expand Down
Loading