fix(core): delete a menu item's children together with the item on SQLite and D1

.changeset/menu-item-delete-children.md

@@ -0,0 +1,9 @@
+---
+"emdash": patch
+---
+
+Fixes the child items of a deleted menu item staying behind on SQLite and D1, where they still showed at the top level of the public menu but no longer in the admin menu editor. Deleting a menu item now deletes the items nested under it too.
+
+#### What should I do?
+
+Child items left behind by earlier deletes are not removed, so they still show at the top level of the public menu. To remove one, find it in the response of `GET /_emdash/api/menus/{name}` and delete it with `DELETE /_emdash/api/menus/{name}/items/{id}`. If the menu has translations, add `?locale={locale}` for the affected translation to both requests: without it, `GET` returns only one translation, and `DELETE` fails with `AMBIGUOUS_LOCALE`.

packages/core/src/database/repositories/menu.ts

@@ -13,7 +13,7 @@
  * single menu item survives content translations.
  */
 
-import type { Compilable, Insertable, Kysely, Selectable } from "kysely";
+import { sql, type Compilable, type Insertable, type Kysely, type Selectable } from "kysely";
 import { ulid } from "ulidx";
 
 import { localeMatches, localeOrder } from "../../i18n/query.js";
@@ -651,12 +651,28 @@ export class MenuRepository {
 		return rowToMenuItem(row);
 	}
 
-	/** Delete an item scoped to its menu. Returns false if nothing was deleted. */
+	/**
+	 * Delete an item and its descendants, scoped to its menu. Returns false if
+	 * the item was not found. SQLite/D1 has no `parent_id` cascade (dropped by
+	 * migration 036), so descendants are removed here for every dialect.
+	 */
 	async deleteItem(menuId: string, itemId: string): Promise<boolean> {
+		// A single statement, so a failure on D1 (no transactions) leaves the
+		// whole subtree in place. UNION, not UNION ALL, ends the walk on cyclic
+		// links from older API writes.
+		const subtreeIds = sql<string>`(
+			WITH RECURSIVE subtree(id) AS (
+				SELECT id FROM _emdash_menu_items WHERE id = ${itemId} AND menu_id = ${menuId}
+				UNION
+				SELECT item.id FROM _emdash_menu_items AS item
+				INNER JOIN subtree ON item.parent_id = subtree.id
+				WHERE item.menu_id = ${menuId}
+			)
+			SELECT id FROM subtree
+		)`;
 		const result = await this.db
 			.deleteFrom("_emdash_menu_items")
-			.where("id", "=", itemId)
-			.where("menu_id", "=", menuId)
+			.where("id", "in", subtreeIds)
 			.execute();
 		const deleted = result[0]?.numDeletedRows !== 0n;
 		if (deleted) invalidateMenuObjectCache();

packages/core/tests/integration/api/menu-item-parent-validation.test.ts

@@ -4,6 +4,7 @@ import { afterEach, beforeEach, expect, it } from "vitest";
 import {
 	handleMenuCreate,
 	handleMenuItemCreate,
+	handleMenuItemDelete,
 	handleMenuItemReorder,
 	handleMenuItemUpdate,
 } from "../../../src/api/handlers/menus.js";
@@ -252,6 +253,41 @@ describeEachDialect("menu item parent validation", (dialect) => {
 		},
 	);
 
+	it("deletes the descendants of a deleted menu item", async () => {
+		const parent = await createItem("Parent");
+		const child = await createItem("Child", parent.id);
+		await createItem("Grandchild", child.id);
+		const sibling = await createItem("Sibling");
+		const result = await handleMenuItemDelete(db, "primary", parent.id);
+		expect(result.success).toBe(true);
+		expect((await currentItems()).map((item) => item.id)).toEqual([sibling.id]);
+		const menu = await getMenuWithDb("primary", db);
+		expect(menu?.items.map((item) => item.label)).toEqual(["Sibling"]);
+	});
+
+	it("keeps the rest of the branch when a nested item is deleted", async () => {
+		const parent = await createItem("Parent");
+		const child = await createItem("Child", parent.id);
+		await createItem("Grandchild", child.id);
+		await createItem("Other child", parent.id);
+		await handleMenuItemDelete(db, "primary", child.id);
+		const menu = await getMenuWithDb("primary", db);
+		expect(menu?.items.map((item) => item.label)).toEqual(["Parent"]);
+		expect(menu?.items[0]?.children.map((item) => item.label)).toEqual(["Other child"]);
+	});
+
+	it("deletes an item whose legacy links form a cycle", async () => {
+		const a = await createItem("A");
+		const b = await createItem("B", a.id);
+		await db
+			.updateTable("_emdash_menu_items")
+			.set({ parent_id: b.id })
+			.where("id", "=", a.id)
+			.execute();
+		expect((await handleMenuItemDelete(db, "primary", a.id)).success).toBe(true);
+		expect(await currentItems()).toEqual([]);
+	});
+
 	it("allows full admin reorder batches with untouched legacy cycles", async () => {
 		const a = await createItem("A");
 		const b = await createItem("B", a.id);

packages/core/tests/workerd/menu-item-delete-d1.test.ts

@@ -0,0 +1,82 @@
+import { env } from "cloudflare:test";
+import { Kysely, sql } from "kysely";
+import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest";
+
+import { RawBindingD1Dialect } from "../../../cloudflare/src/db/d1-dialect.js";
+import {
+	handleMenuCreate,
+	handleMenuItemCreate,
+	handleMenuItemDelete,
+} from "../../src/api/handlers/menus.js";
+import { runMigrations } from "../../src/database/migrations/runner.js";
+import type { Database } from "../../src/database/types.js";
+import { resetD1Schema } from "./d1-schema.js";
+
+declare module "cloudflare:test" {
+	interface ProvidedEnv {
+		DB: D1Database;
+	}
+}
+
+let db: Kysely<Database>;
+
+beforeAll(() => {
+	db = new Kysely<Database>({ dialect: new RawBindingD1Dialect({ database: env.DB }) });
+});
+
+beforeEach(async () => {
+	await resetD1Schema(db);
+	await runMigrations(db);
+});
+
+afterAll(async () => {
+	await db.destroy();
+});
+
+async function create(label: string, parentId?: string) {
+	const result = await handleMenuItemCreate(db, "primary", {
+		type: "custom",
+		label,
+		customUrl: `/${label}`,
+		parentId,
+		sortOrder: 0,
+	});
+	if (!result.success) throw new Error(result.error.message);
+	return result.data.id;
+}
+
+describe("menu item deletion on D1", () => {
+	beforeEach(async () => {
+		await handleMenuCreate(db, { name: "primary", label: "Primary" });
+	});
+
+	it("deletes the descendants of a deleted menu item", async () => {
+		const parent = await create("parent");
+		const child = await create("child", parent);
+		await create("grandchild", child);
+		const other = await create("other");
+
+		expect((await handleMenuItemDelete(db, "primary", parent)).success).toBe(true);
+
+		const rows = await db.selectFrom("_emdash_menu_items").select("id").execute();
+		expect(rows.map((row) => row.id)).toEqual([other]);
+	});
+
+	it("keeps the whole subtree when the delete fails partway", async () => {
+		const parent = await create("parent");
+		await Promise.all(Array.from({ length: 60 }, (_, i) => create(`child-${i}`, parent)));
+		await sql`
+			CREATE TRIGGER fail_menu_parent_delete
+			BEFORE DELETE ON _emdash_menu_items
+			WHEN OLD.label = 'parent'
+			BEGIN
+				SELECT RAISE(ABORT, 'forced menu item delete failure');
+			END
+		`.execute(db);
+
+		expect((await handleMenuItemDelete(db, "primary", parent)).success).toBe(false);
+
+		const rows = await db.selectFrom("_emdash_menu_items").select("id").execute();
+		expect(rows).toHaveLength(61);
+	});
+});