fix: a malformed data.json no longer costs you the choices QuickAdd could not draw

src/gui/MacroGUIs/CommandList.svelte

@@ -69,6 +69,32 @@ const renderable = $derived.by(() => {
 	});
 });
 
+// A drop-zone type unique to THIS list, so no two command lists are ever drop
+// targets for each other (#1613).
+//
+// They used to share `type: "command"`. The conditional-branch editor opens ON
+// TOP of the still-open macro builder, and svelte-dnd-action hit-tests every
+// registered zone of a type GEOMETRICALLY — a modal backdrop shields nothing — so
+// dragging a command a little too far down inside the branch editor dropped it
+// into the builder underneath, and both editors then rendered a list that
+// disagreed with the stored macro.
+//
+// The id comes from Svelte's own `$props.id()` (a runtime-wide uid counter,
+// already used by LabeledField), so no bespoke counter is needed.
+//
+// A unique type rather than `dropFromOthersDisabled`, which was tried first and
+// is worse: the target does refuse, but the library then runs its
+// "left for a zone that refuses" path, which re-dispatches the origin's items
+// twice — once with its shadow placeholder and once without — and our
+// stripShadow (#1244/#883) drops the placeholder the library goes on to measure,
+// so the drag ends in an uncaught TypeError with the command gone from the
+// source list. With a unique type the other zone is not a candidate at all, so
+// leaving this one is an ordinary "outside of any zone" drag: the command
+// springs back, which is the correct behaviour for a gesture QuickAdd does not
+// offer.
+const zoneId = $props.id();
+const zoneType = `command:${zoneId}`;
+
 const isMobile = Platform.isMobile;
 // Desktop: drag is armed by grabbing the handle (shared with the choices list; see
 // createDragArming for the click-swallow failsafe). Mobile: no handle — the whole row
@@ -230,7 +256,7 @@ async function configureOpenFile(command: IOpenFileCommand) {
 	use:dndzone={baseDndOptions({
 		items: renderable,
 		dragDisabled,
-		type: "command",
+		type: zoneType,
 		// A command's `.name` differs from its rendered label for Choice/Conditional
 		// commands (getCommandDisplayName resolves the referenced choice's name / the
 		// "If …" summary), so the pill must resolve the label the same way the row does.

src/gui/MacroGUIs/CommandList.zoneType.test.ts

@@ -0,0 +1,72 @@
+import { describe, expect, it, vi } from "vitest";
+import type * as DndAction from "svelte-dnd-action";
+
+/**
+ * #1613. The macro builder and the conditional-branch editor both mount a
+ * CommandList, and the branch modal opens ON TOP of the still-open builder.
+ * svelte-dnd-action groups drop zones by `type` and hit-tests every zone in the
+ * group GEOMETRICALLY - a modal backdrop shields nothing - so while both lists
+ * shared `type: "command"`, dragging a command a little too far down inside the
+ * branch editor dropped it into the builder underneath.
+ *
+ * The zone type is the whole fix, and it is invisible in the DOM: the library
+ * writes no attribute for it. Its `dndzone` action is the only observable seam,
+ * so this test WRAPS the real action (rather than stubbing it, which would lose
+ * the behaviour under test) and records the options each list registers with.
+ *
+ * Its own file because `vi.mock` is module-graph-wide, and a stray mock of
+ * svelte-dnd-action would leak into the other CommandList tests.
+ */
+
+const registeredTypes: (string | undefined)[] = [];
+
+vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() }));
+
+vi.mock("svelte-dnd-action", async () => {
+	const actual = await vi.importActual<typeof DndAction>("svelte-dnd-action");
+	return {
+		...actual,
+		dndzone: (node: HTMLElement, options: { type?: string }) => {
+			registeredTypes.push(options?.type);
+			return actual.dndzone(node, options as never);
+		},
+	};
+});
+
+import { render } from "@testing-library/svelte";
+import { App } from "obsidian";
+import CommandList from "./CommandList.svelte";
+import type QuickAdd from "../../main";
+import type { ICommand } from "../../types/macros/ICommand";
+
+const wait = (id: string): ICommand =>
+	({ id, name: "Wait", type: "Wait", time: 100 }) as unknown as ICommand;
+
+const renderList = (commands: ICommand[]) =>
+	render(CommandList, {
+		props: {
+			commands,
+			app: new App() as never,
+			plugin: {} as unknown as QuickAdd,
+			deleteCommand: vi.fn(),
+			saveCommands: vi.fn(),
+		} as never,
+	});
+
+describe("CommandList's drop-zone type", () => {
+	it("is unique per list, so two command lists are never targets for each other", () => {
+		registeredTypes.length = 0;
+
+		renderList([wait("a")]);
+		renderList([wait("b")]);
+
+		expect(registeredTypes).toHaveLength(2);
+		// The literal that used to be shared. Any two lists carrying it are in the
+		// same group and hit-test each other.
+		expect(registeredTypes).not.toContain("command");
+		expect(registeredTypes[0]).not.toBe(registeredTypes[1]);
+		for (const type of registeredTypes) {
+			expect(type).toMatch(/^command:/);
+		}
+	});
+});

src/gui/choiceList/ChoiceList.svelte

@@ -48,22 +48,31 @@
     } = $props();
 
     // Everything rendered and handed to the dnd zone is filtered to entries that
-    // are actually renderable choices: an object with an id. `data.json` is
-    // untrusted, so the list can contain a hole (a `null` or a stray primitive
-    // from a bad hand-edit or a truncated write) or an object with no id at all.
-    // svelte-dnd-action reads `.id` on every item, and the keyed {#each} needs
-    // that id to be present AND unique - two id-less entries raise
-    // `each_key_duplicate`, the #1451 crash, which without this filter blanks the
-    // entire list (#1566).
+    // can actually be keyed: an object with a unique, non-empty string id.
+    // svelte-dnd-action reads `.id` on every item and the keyed {#each} throws
+    // `each_key_duplicate` on a repeat (#1451) — and on a POST-mount update that
+    // throw escapes mountComponent's try entirely (see its doc comment), so this
+    // has to be a $derived, not a one-off check at setup.
     //
-    // Render-time view only. Nothing here rewrites the tree; a junk entry stays in
-    // data.json until an edit walks past it. It cannot be hiding a choice - only a
-    // Multi's `choices` value can, and that is preserved separately.
-    const renderable = $derived(
-        rootChoicesOf(choices).filter(
-            (choice) => isChoiceLike(choice) && typeof choice.id === "string" && choice.id !== "",
-        ),
-    );
+    // ChoiceView normalizes the tree at its seam before it ever gets here, so in
+    // practice this filter is a no-op: an id-less, non-string-id or duplicate-id
+    // choice has already been given a fresh uuid and KEPT. That matters, because
+    // the persist path below writes back the list this zone was seeded with — a
+    // filter that silently dropped a real choice would delete it from data.json on
+    // the first reorder, which is exactly what it used to do (#1608). The only
+    // thing it can still drop is a `null`/primitive hole, which carries nothing.
+    //
+    // Identical in shape to CommandList's `renderable`, deliberately.
+    const renderable = $derived.by(() => {
+        const seen = new Set<string>();
+        return rootChoicesOf(choices).filter((choice) => {
+            if (!isChoiceLike(choice)) return false;
+            const id = choice.id;
+            if (typeof id !== "string" || id === "" || seen.has(id)) return false;
+            seen.add(id);
+            return true;
+        });
+    });
 
     // Resolve once: at the top level there is no incoming rootReorder, so the list's
     // own handler IS the top-level handler; nested lists receive it explicitly.

src/gui/choiceList/ChoiceView.malformed.test.ts

@@ -11,6 +11,8 @@ import ChoiceView from "./ChoiceView.svelte";
 import type QuickAdd from "../../main";
 import type IChoice from "../../types/choices/IChoice";
 import type { Plain } from "../svelte/persist.svelte";
+import { settingsStore } from "../../settingsStore";
+import { tick } from "svelte";
 import {
 	folder,
 	leaf,
@@ -133,23 +135,31 @@ describe("ChoiceView over a malformed tree (#1566)", () => {
 		expect(getByLabelText("Configure Survivor")).toBeInTheDocument();
 	});
 
-	it("drops entries that cannot be keyed instead of losing the whole list", () => {
+	it("repairs entries that cannot be keyed instead of hiding them", () => {
 		// Two entries with no id give the keyed {#each} the same (undefined) key,
-		// which raises `each_key_duplicate` - the #1451 crash. The boundary would
-		// catch it, but the user would lose every real row with it.
-		const { getByLabelText, queryByText, container } = renderChoiceView([
+		// which raises `each_key_duplicate` - the #1451 crash. Hiding them avoided
+		// the crash and cost the user the choices instead: the list the view renders
+		// is the list it persists, so the first reorder deleted them (#1608). They
+		// are given a fresh uuid at the seam and KEPT, so they can finally be seen,
+		// edited and deleted.
+		const { getByLabelText, getByText, container } = renderChoiceView([
 			{ name: "Missing id" } as IChoice,
 			{ name: "No id" } as IChoice,
 			leaf("Survivor", "survivor"),
 		]);
 
 		expect(getByLabelText("Configure Survivor")).toBeInTheDocument();
-		// Absent from the DOM entirely, not merely unkeyed: a regression that still
-		// painted them as rows without a data-choice-id would pass a count check.
-		expect(queryByText("Missing id")).toBeNull();
-		expect(queryByText("No id")).toBeNull();
+		expect(getByText("Missing id")).toBeInTheDocument();
+		expect(getByText("No id")).toBeInTheDocument();
 		expect(container.querySelector(".qaChoicesUnavailable")).toBeNull();
-		expect(container.querySelectorAll("[data-choice-id]")).toHaveLength(1);
+		// Every row keyed, and keyed DISTINCTLY - the repair must not hand the two
+		// id-less entries the same replacement.
+		const ids = [...container.querySelectorAll("[data-choice-id]")].map((el) =>
+			el.getAttribute("data-choice-id"),
+		);
+		expect(ids).toHaveLength(3);
+		expect(new Set(ids).size).toBe(3);
+		expect(ids).toContain("survivor");
 	});
 
 	it("refuses to render, rather than to offer a CTA, when the root list is unreadable", () => {
@@ -169,6 +179,112 @@ describe("ChoiceView over a malformed tree (#1566)", () => {
 		expect(saveChoices).not.toHaveBeenCalled();
 	});
 
+	it("a reorder can only reorder - it can never delete", () => {
+		// #1608, the headline. `renderable` seeded the drop zone AND was what the
+		// persist path wrote back, so the first drag or ArrowDown committed the
+		// FILTERED list and every entry the filter had dropped was gone from
+		// data.json - including a complete, working choice whose id happened to be a
+		// JSON number, from a row the user could never see.
+		const saveChoices = vi.fn<(next: Plain<IChoice[]>) => void>();
+		const numeric = {
+			id: 12,
+			name: "Daily note",
+			type: "Template",
+			command: false,
+			templatePath: "Templates/Daily.md",
+		} as unknown as IChoice;
+		const tree = [
+			leaf("Alpha", "alpha"),
+			numeric,
+			null as unknown as IChoice,
+			{ name: "No id", type: "Capture", command: false } as IChoice,
+			leaf("Beta", "beta"),
+		];
+		const { container } = renderChoiceView(tree, saveChoices);
+
+		const handle = container.querySelector<HTMLElement>(
+			'[data-choice-id="alpha"] .qa-drag-handle',
+		)!;
+		handle.dispatchEvent(
+			new KeyboardEvent("keydown", { key: "ArrowDown", bubbles: true }),
+		);
+
+		expect(saveChoices).toHaveBeenCalled();
+		const saved = saveChoices.mock.calls.at(-1)![0] as unknown as Record<
+			string,
+			unknown
+		>[];
+
+		// Everything that carried data survived; only the `null` hole is gone.
+		expect(saved.map((c) => c.name)).toEqual([
+			"Daily note",
+			"Alpha",
+			"No id",
+			"Beta",
+		]);
+		// ...and byte-identical apart from the reorder and the repaired ids.
+		const daily = saved.find((c) => c.name === "Daily note")!;
+		expect(daily.templatePath).toBe("Templates/Daily.md");
+		expect(daily.type).toBe("Template");
+		expect(typeof daily.id).toBe("string");
+		expect(saved.filter((c) => c.name === "Alpha")[0].id).toBe("alpha");
+	});
+
+	it("keeps repaired ids STABLE across an unrelated settings store write", async () => {
+		// The seam is fed by a subscription that fires on EVERY settingsStore write,
+		// including ones nothing in this view caused (the AI provider auto-sync lands
+		// one a few seconds after launch). Re-minting on each of those would not just
+		// churn rows: every by-id write here resolves its target before an await -
+		// handleConfigureChoice captures the choice, awaits the builder, then matches
+		// on `oldChoice.id === newChoice.id` - so a re-mint inside that window turns
+		// the match into a no-op and silently discards the user's edits.
+		const tree = [{ name: "No id", type: "Capture", command: false } as IChoice];
+		settingsStore.setState({ choices: tree });
+		const { container } = renderChoiceView(tree);
+		const idBefore = container
+			.querySelector("[data-choice-id]")!
+			.getAttribute("data-choice-id");
+
+		// zustand merges partials, so `state.choices` stays reference-identical.
+		settingsStore.setState({ disableOnlineFeatures: true });
+		await tick();
+
+		const row = container.querySelector("[data-choice-id]");
+		expect(row).not.toBeNull();
+		expect(row!.getAttribute("data-choice-id")).toBe(idBefore);
+	});
+
+	it("keeps repaired ids stable, and registers one command, across a RE-MOUNT", () => {
+		// The settings tab destroys and re-mounts this view every time it is opened,
+		// so a memo living in the component would miss it: each open would mint a
+		// fresh uuid for the same unrepaired choice and register another command for
+		// it, leaving one dead palette entry per open (nothing is persisted at seed
+		// time, so the old id is still what `getChoice` resolves).
+		const addCommandForChoice = vi.fn();
+		const tree = [
+			{ name: "No id", type: "Capture", command: true } as IChoice,
+		];
+
+		const ids = [0, 1, 2].map(() => {
+			const { container, unmount } = render(ChoiceView, {
+				props: {
+					app: new App() as never,
+					plugin: { addCommandForChoice } as unknown as QuickAdd,
+					choices: tree,
+					saveChoices: vi.fn(),
+				},
+			});
+			const id = container
+				.querySelector("[data-choice-id]")!
+				.getAttribute("data-choice-id");
+			unmount();
+			return id;
+		});
+
+		expect(new Set(ids).size, `ids across mounts: ${ids.join(", ")}`).toBe(1);
+		expect(addCommandForChoice).toHaveBeenCalledTimes(1);
+	});
+
 	it("does not persist a fabricated [] when an unrelated folder is collapsed", () => {
 		// The regression that a naive "use the accessor everywhere" fix introduces:
 		// updateMultiById re-spreads every folder it walks past, and collapsing runs

src/gui/choiceList/ChoiceView.svelte

@@ -38,6 +38,7 @@
 		hasChildChoices,
 		isChoiceLike,
 	} from "../../utils/choiceUtils";
+	import { hasSeeded, seedChoiceTree } from "./seedChoiceTree";
 	import type { ChoiceListActions } from "./choiceListActions";
 	import { choiceNoun } from "../../utils/choiceNoun";
 	import { reportingHandler } from "../../utils/errorUtils";
@@ -86,12 +87,61 @@
 	// component's life; untrack avoids a spurious state_referenced_locally warning).
 	const commandRegistry = new CommandRegistry(untrack(() => plugin));
 
+	// The seam where an unrenderable choice is REPAIRED rather than hidden (#1608).
+	//
+	// ChoiceList can only render an entry it can key, and the list it renders is
+	// the list its persist path writes back — so before this, an entry with a
+	// missing or non-string id was invisible in the settings tab AND deleted from
+	// data.json by the first drag or ArrowDown. Re-keying here makes it visible,
+	// editable and deletable instead, and leaves ChoiceList's filter able to drop
+	// only a hole, which carries nothing.
+	//
+	// Nothing is persisted by this: the repair reaches disk with the user's first
+	// ordinary edit, so opening and closing the settings tab changes nothing —
+	// the same contract CommandSequenceEditor's constructor keeps for commands.
+	// That holds only because the memo behind `seedChoiceTree` outlives this
+	// component; the settings tab destroys and re-mounts the view on every open.
+	function seedChoices(raw: IChoice[]): IChoice[] {
+		// A non-array root is left EXACTLY as it is: `rootUnreadable` refuses to
+		// render it, which is what keeps anything in this view from saving over it.
+		if (!Array.isArray(raw)) return raw;
+		const alreadySeeded = hasSeeded(raw);
+		const { choices: seeded, repaired } = seedChoiceTree(raw);
+		if (alreadySeeded) return seeded;
+
+		// Commands were registered at onload from the ids data.json held, so a
+		// repaired choice has no command under its NEW id. Register one, so it works
+		// the moment the repair is persisted. Until then the OLD id is still what
+		// `getChoice` resolves, so the new entry reports "Choice not found" if run —
+		// an honest error, and the alternative was the choice being deleted outright.
+		//
+		// One failure must cost one command, not the whole list: this runs during
+		// component setup, so an uncaught throw escapes mountComponent and swaps the
+		// entire choice list for the error card (the same argument as main.ts's
+		// per-choice guard around addCommandForChoice).
+		for (const { choice } of repaired) {
+			if (!choice.command) continue;
+			try {
+				commandRegistry.enableCommand(choice);
+			} catch (err) {
+				log.logError(
+					`Could not register a command for the repaired choice "${choice.name}": ${
+						err instanceof Error ? err.message : String(err)
+					}`,
+				);
+			}
+		}
+		return seeded;
+	}
+
+	choices = seedChoices(choices);
+
 	// Keep choices in sync with external store changes. The subscribe callback runs
 	// only on store changes (not during this effect's synchronous setup), so the
 	// effect registers no reactive deps and subscribes exactly once.
 	$effect(() => {
 		const unsubSettingsStore = settingsStore.subscribe((settings) => {
-			choices = settings.choices;
+			choices = seedChoices(settings.choices);
 			disableOnlineFeatures = settings.disableOnlineFeatures;
 		});
 		return () => unsubSettingsStore();