diff --git a/src/gui/MacroGUIs/CommandList.svelte b/src/gui/MacroGUIs/CommandList.svelte index 44395a71b..00300ed0d 100644 --- a/src/gui/MacroGUIs/CommandList.svelte +++ b/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. diff --git a/src/gui/MacroGUIs/CommandList.zoneType.test.ts b/src/gui/MacroGUIs/CommandList.zoneType.test.ts new file mode 100644 index 000000000..757b5d4ce --- /dev/null +++ b/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("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:/); + } + }); +}); diff --git a/src/gui/choiceList/ChoiceList.svelte b/src/gui/choiceList/ChoiceList.svelte index 2a10dde37..2b84a28be 100644 --- a/src/gui/choiceList/ChoiceList.svelte +++ b/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(); + 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. diff --git a/src/gui/choiceList/ChoiceView.malformed.test.ts b/src/gui/choiceList/ChoiceView.malformed.test.ts index 61d0f6e05..54f5fecd4 100644 --- a/src/gui/choiceList/ChoiceView.malformed.test.ts +++ b/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) => 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( + '[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 diff --git a/src/gui/choiceList/ChoiceView.svelte b/src/gui/choiceList/ChoiceView.svelte index 99f5d7064..c4d04d132 100644 --- a/src/gui/choiceList/ChoiceView.svelte +++ b/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(); diff --git a/src/gui/choiceList/seedChoiceTree.ts b/src/gui/choiceList/seedChoiceTree.ts new file mode 100644 index 000000000..a5cf766b3 --- /dev/null +++ b/src/gui/choiceList/seedChoiceTree.ts @@ -0,0 +1,54 @@ +import type IChoice from "../../types/choices/IChoice"; +import { + normalizeChoiceList, + type NormalizedChoiceList, +} from "../../utils/choiceUtils"; + +const seededByRaw = new WeakMap(); + +/** + * `normalizeChoiceList`, memoized on the identity of the raw array it was given. + * + * The repair is not persisted - it reaches disk with the user's first ordinary + * edit - so the same unrepaired array is normalized again every time anything + * re-reads it, and each of those would mint DIFFERENT uuids for the same choice. + * Two things go wrong when that happens, and neither is cosmetic: + * + * - ChoiceView's subscription fires on EVERY settingsStore write, including + * ones nothing in the view caused (the AI provider auto-sync lands one a few + * seconds after launch). Every by-id write in the view 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. + * - The settings tab DESTROYS and re-mounts ChoiceView every time it is + * opened. A memo living in the component would miss that, so a repaired + * `command: true` choice would be registered under a fresh id on every + * open, leaving one dead palette entry per open. + * + * Hence a module-level WeakMap rather than component state: the cache outlives + * the component, and is keyed on the array object so an unrelated `setState` + * (zustand merges partials, leaving `state.choices` reference-identical) hits it + * while a genuine tree change misses it. Entries die with the array. + * + * Keying on the choice's own previous id would NOT work: `undefined` collides + * across every id-less entry, so two siblings would be handed the same uuid - + * `each_key_duplicate` again (#1451). + */ +export function seedChoiceTree(raw: IChoice[]): NormalizedChoiceList { + const cached = seededByRaw.get(raw); + if (cached) return cached; + + const result = normalizeChoiceList(raw); + seededByRaw.set(raw, result); + return result; +} + +/** + * Whether this exact array has already been seeded. Lets the caller run its + * one-time side effects (command registration) only on a cache MISS, without + * having to model that itself. + */ +export function hasSeeded(raw: IChoice[]): boolean { + return seededByRaw.has(raw); +} diff --git a/src/gui/shared/dndReorder.test.ts b/src/gui/shared/dndReorder.test.ts index cefec9012..b4e1b0915 100644 --- a/src/gui/shared/dndReorder.test.ts +++ b/src/gui/shared/dndReorder.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { SHADOW_PLACEHOLDER_ITEM_ID } from "svelte-dnd-action"; -import { replaceById, stripShadow } from "./dndReorder"; +import { baseDndOptions, replaceById, stripShadow } from "./dndReorder"; const item = (id: string, extra: Record = {}) => ({ id, ...extra }); @@ -58,3 +58,16 @@ describe("replaceById", () => { expect(out.map((i) => i.id)).toEqual(["a", "b"]); }); }); + +describe("baseDndOptions", () => { + it("keeps the coupled pill options together", () => { + // These four move as a set (see the doc comment): breaking one silently + // makes the custom drag pill fight the library's own clone. + const options = baseDndOptions({ items: [], dragDisabled: false }); + + expect(options.morphDisabled).toBe(true); + expect(options.useCursorForDetection).toBe(true); + expect(options.centreDraggedOnCursor).toBe(false); + expect(options.autoAriaDisabled).toBe(true); + }); +}); diff --git a/src/gui/shared/dndReorder.ts b/src/gui/shared/dndReorder.ts index 703f7c07d..eb4785203 100644 --- a/src/gui/shared/dndReorder.ts +++ b/src/gui/shared/dndReorder.ts @@ -46,7 +46,7 @@ export function replaceById( * - zoneItemTabIndex:-1 keeps rows out of the tab order, * - delayTouchStart gates touch drags (desktop is gated by the dragArmed handle). * Per-zone overrides: items, dragDisabled, type, dropTargetClasses, flipDurationMs (kept - * in sync with animate:flip), and resolveLabel (the pill text — defaults to item.name; + * in sync with animate:flip), resolveLabel (the pill text — defaults to item.name; * the macro builder passes getCommandDisplayName, since a command's `.name` differs from * its rendered label for Choice/Conditional commands). */ diff --git a/src/migrations/backfillFileOpeningDefaults.test.ts b/src/migrations/backfillFileOpeningDefaults.test.ts index 5ccb2ac65..b4983a52c 100644 --- a/src/migrations/backfillFileOpeningDefaults.test.ts +++ b/src/migrations/backfillFileOpeningDefaults.test.ts @@ -204,6 +204,11 @@ describe("backfillFileOpeningDefaults migration", () => { mode: "default", focus: true, }); - expect(plugin.saveSettings).toHaveBeenCalled(); + // migrate.ts owns the single post-migration save (it re-syncs the store and + // saves after the whole run). A per-migration write here is redundant, and + // once this migration can stay PENDING it would be charged on every launch - + // a full data.json rewrite per launch, into Obsidian Sync's whole-file + // last-write-wins. + expect(plugin.saveSettings).not.toHaveBeenCalled(); }); }); diff --git a/src/migrations/backfillFileOpeningDefaults.ts b/src/migrations/backfillFileOpeningDefaults.ts index 3d84e2fa9..77b6d0f7a 100644 --- a/src/migrations/backfillFileOpeningDefaults.ts +++ b/src/migrations/backfillFileOpeningDefaults.ts @@ -1,6 +1,6 @@ import { log } from "../logger/logManager"; import type QuickAdd from "../main"; -import type { Migration } from "./Migrations"; +import type { Migration, MigrationResult } from "./Migrations"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type IChoice from "../types/choices/IChoice"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; @@ -8,7 +8,10 @@ import { coerceLegacyOpenFileInNewTab, createFileOpeningFromLegacy, } from "./helpers/file-opening-legacy"; -import { walkAllChoices } from "./helpers/choice-traversal"; +import { + settingsTreeHasUnreadableData, + walkAllChoices, +} from "./helpers/choice-traversal"; import { normalizeFileOpening, type FileOpeningSettings, @@ -16,9 +19,26 @@ import { const backfillFileOpeningDefaults: Migration = { description: "Backfill missing file opening defaults for older choices", - migrate: async (plugin: QuickAdd) => { + migrate: async (plugin: QuickAdd): Promise => { log.logMessage("Starting file opening defaults backfill..."); + // Both halves of this migration MOVE data: they translate the legacy + // `openFileInNewTab` / `openFileInMode` keys into `fileOpening`, and nothing + // at runtime reads the legacy keys. A choice hidden behind a container this + // walk could not read would therefore lose its "open in new tab" preference + // permanently once the migration is flagged complete, even after the user + // repairs data.json. Stay pending instead (#1610). + // + // A MISSING `fileOpening`, by contrast, needs no guard at all: the engines + // call `normalizeFileOpening(this.choice.fileOpening)` on every run, so the + // defaults half is fully compensated at runtime. + // + // This migration does NOT call saveSettings() itself: migrate.ts re-syncs the + // store and saves once after the whole run. A per-migration write would be a + // full data.json rewrite, and once this can stay PENDING that is one on every + // launch, straight into Obsidian Sync's whole-file last-write-wins. + const unreadable = settingsTreeHasUnreadableData(plugin.settings); + let migratedCount = 0; const backfillFileOpening = (choice: IChoice) => { @@ -67,7 +87,7 @@ const backfillFileOpeningDefaults: Migration = { `File opening defaults backfill complete. Updated ${migratedCount} choice(s).`, ); - await plugin.saveSettings(); + if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/consolidateFileExistsBehavior.ts b/src/migrations/consolidateFileExistsBehavior.ts index f2b9398d2..b8099c84b 100644 --- a/src/migrations/consolidateFileExistsBehavior.ts +++ b/src/migrations/consolidateFileExistsBehavior.ts @@ -4,10 +4,12 @@ import { isTemplateChoice, normalizeTemplateChoice, } from "./helpers/normalizeTemplateFileExistsBehavior"; -import { walkAllChoices } from "./helpers/choice-traversal"; +import { + settingsTreeHasUnreadableData, + walkAllChoices, +} from "./helpers/choice-traversal"; import type { Migration, MigrationResult } from "./Migrations"; import type { IMacro } from "src/types/macros/IMacro"; -import { treeHasUnreadableChildren } from "src/utils/choiceUtils"; type SettingsWithLegacyMacros = QuickAdd["settings"] & { macros?: IMacro[] }; @@ -22,7 +24,10 @@ const consolidateFileExistsBehavior: Migration = { // data.json, which is the opposite of what a migration should do (#1566). // When it is unreadable the walk below covers nothing, so stay pending and // re-run once the user has repaired the file. - const treeReadable = !treeHasUnreadableChildren(plugin.settings.choices); + // This migration walks with `walkAllChoices`, so it must ask the WIDE + // question - a nested choice hidden behind an unreadable `macro.commands` + // is one this visitor would have normalized (#1610). + const treeReadable = !settingsTreeHasUnreadableData(settings); if (Array.isArray(plugin.settings.choices)) { plugin.settings.choices = deepClone(plugin.settings.choices); } diff --git a/src/migrations/helpers/choice-traversal.ts b/src/migrations/helpers/choice-traversal.ts index 93260834d..3c0024018 100644 --- a/src/migrations/helpers/choice-traversal.ts +++ b/src/migrations/helpers/choice-traversal.ts @@ -6,7 +6,11 @@ import type { IConditionalCommand } from "../../types/macros/Conditional/ICondit import { CommandType } from "../../types/macros/CommandType"; import type { ICommand } from "../../types/macros/ICommand"; import type { INestedChoiceCommand } from "../../types/macros/QuickCommands/INestedChoiceCommand"; -import { rootChoicesOf } from "../../utils/choiceUtils"; +import { isUnreadableChoiceList } from "../../utils/choiceUtils"; +import { + isUnreadableCommandList, + macroCommandsValueOf, +} from "../../utils/macroUtils"; export type ChoiceVisitor = (choice: IChoice) => void; export type CommandVisitor = (command: ICommand) => void; @@ -14,6 +18,13 @@ export type CommandVisitor = (command: ICommand) => void; interface Visitors { onChoice?: ChoiceVisitor; onCommand?: CommandVisitor; + /** + * Called with every container value this walk had to step over because it + * could not read it. What makes `settingsTreeHasUnreadableData` trustworthy: + * the guard is the walk, so it cannot answer for a subtree the walk enters + * (or skips) differently. + */ + onUnreadable?: () => void; } function isMultiChoice(choice: IChoice): choice is MultiChoice { @@ -30,31 +41,66 @@ function walkChoice( visited: Set, ): void { if (!choice || typeof choice !== "object") return; + // An ARRAY entry is a NESTED LIST, which the editor seam splices into the tree + // (`normalizeChoiceList`) - so it can be CARRYING choices, and `typeof [] === + // "object"` means the visitor would otherwise be handed the array itself and + // descend nothing. + // + // Reported unreadable rather than descended, even though this walk could + // descend it. The narrower migrations cannot: `flattenChoices` pushes the array + // as if it were a choice, and the increment / mutual-exclusion recursions step + // straight past it. Descending here would make the guard say "I saw everything" + // on behalf of traversals that did not, which is exactly the failure #1610 is + // about. Staying pending costs one launch: the user opens the settings tab, the + // seam splices the entries into real choices, and the next launch is readable. + if (Array.isArray(choice)) { + visitors.onUnreadable?.(); + return; + } if (visited.has(choice)) return; visited.add(choice); visitors.onChoice?.(choice); - if (isMultiChoice(choice) && Array.isArray(choice.choices)) { - for (const child of choice.choices) { - walkChoice(child, visitors, visited); + if (isMultiChoice(choice)) { + if (Array.isArray(choice.choices)) { + for (const child of choice.choices) { + walkChoice(child, visitors, visited); + } + } else if (isUnreadableChoiceList(choice.choices)) { + visitors.onUnreadable?.(); } } if (isMacroChoice(choice)) { - walkCommands(choice.macro?.commands, visitors, visited); + // `macroCommandsValueOf`, not `choice.macro?.commands`: an ARRAY-valued + // `macro` IS the command list (writing `.commands` onto an Array is dropped + // by JSON.stringify, so that is the only recoverable reading - see #1593). + // Reading `.commands` off it yields `undefined`, which walks nothing and + // reports readable, so a nested choice inside such a macro was invisible to + // every migration AND to the guard that is supposed to notice (#1610). + walkCommands(macroCommandsValueOf(choice.macro), visitors, visited); } } function walkCommands( - commands: ICommand[] | undefined, + commands: unknown, visitors: Visitors, visited: Set, ): void { - if (!Array.isArray(commands)) return; + if (!Array.isArray(commands)) { + if (isUnreadableCommandList(commands)) visitors.onUnreadable?.(); + return; + } for (const command of commands) { if (!command || typeof command !== "object") continue; + // Same reasoning as the choice side: a nested list can be carrying commands, + // and reporting it is honest about what the narrower walkers see. + if (Array.isArray(command)) { + visitors.onUnreadable?.(); + continue; + } visitors.onCommand?.(command); @@ -91,22 +137,70 @@ function walkSettings( // The ROOT `choices` is untrusted too: loadSettings deliberately leaves a // non-array value in place rather than replacing it with [] (#1566). - for (const choice of rootChoicesOf(settings.choices)) { - walkChoice(choice, visitors, visited); + // + // The root is judged STRICTLY - any non-array is unreadable - while every + // container below it uses the "could this be carrying data" rule. The + // asymmetry is not an oversight: `loadSettings` merges over DEFAULT_SETTINGS, + // so a MISSING `choices` key is already `[]` by the time anything sees it. + // A root that is not an array is therefore always corruption, never an honest + // empty - which is why ChoiceView refuses to render it at all rather than + // showing the "No choices yet" hero whose CTA would write over it. + if (Array.isArray(settings.choices)) { + for (const choice of settings.choices) { + walkChoice(choice, visitors, visited); + } + } else { + visitors.onUnreadable?.(); } const legacyMacros = settings.macros; if (Array.isArray(legacyMacros)) { for (const macro of legacyMacros) { - const commands = - macro && typeof macro === "object" - ? (macro as { commands?: ICommand[] }).commands - : undefined; - walkCommands(commands, visitors, visited); + walkCommands(macroCommandsValueOf(macro), visitors, visited); } + } else if (isUnreadableCommandList(legacyMacros)) { + // A pre-consolidation vault keeps its macros here, and this container is + // as untrusted as the others. `undefined` is the NORMAL post-migration + // state, so only a value that could still be carrying macros counts. + visitors.onUnreadable?.(); } } +/** + * Whether anything in `settings` is hidden from {@link walkAllChoices} behind a + * container it could not read. + * + * This is the question a migration that MOVES data has to ask. Migrations run + * exactly once and are then flagged complete forever, so one that walked past a + * subtree it could not see would leave that subtree un-migrated permanently - + * even after the user repairs `data.json` by hand. Such a migration must return + * `{ complete: false }` instead and retry on a later launch (see + * `MigrationResult`). + * + * Derived from the traversal itself rather than reimplemented alongside it: an + * independent predicate answered for `Multi.choices` only and never descended + * `macro.commands`, so a `NestedChoice` inside `"commands": {"0": {...}}` was + * invisible to both the walk and the guard, and the migrations reported complete + * anyway (#1610). + * + * Ask this only if you migrate through `walkAllChoices`. A migration with its own + * narrower traversal must ask the narrower question - see + * `treeHasUnreadableChildren` - or it blocks itself on data it was never going to + * touch. + */ +export function settingsTreeHasUnreadableData(settings: { + choices: IChoice[]; + macros?: unknown; +}): boolean { + let unreadable = false; + walkSettings(settings, { + onUnreadable: () => { + unreadable = true; + }, + }); + return unreadable; +} + export function walkAllChoices(plugin: QuickAdd, visitor: ChoiceVisitor): void { walkSettings( plugin.settings as { choices: IChoice[]; macros?: unknown }, diff --git a/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts b/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts index 47be7edcd..ca3bf387c 100644 --- a/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts +++ b/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts @@ -6,6 +6,12 @@ import { isMultiChoice } from "./helpers/isMultiChoice"; import { isNestedChoiceCommand } from "./helpers/isNestedChoiceCommand"; import type { Migration, MigrationResult } from "./Migrations"; import { treeHasUnreadableChildren } from "src/utils/choiceUtils"; +import { + commandListOf, + isUnreadableCommandList, + macroCommandsValueOf, + rootMacrosOf, +} from "src/utils/macroUtils"; type OldTemplateChoice = { type?: string; @@ -44,10 +50,14 @@ function recursiveRemoveIncrementFileName(choices: IChoice[]): IChoice[] { } function removeIncrementFileName(macros: IMacro[]): IMacro[] { + // `macros` is untrusted end to end, not just at its root: a hole (`null`, a + // stray primitive) survives a truncated write and must be stepped over + // rather than dereferenced, and an ARRAY-valued macro IS its command list + // (#1593). Both reach `macro.commands` otherwise, and the TypeError aborts + // the whole migration - caught, reverted, and reported with a 15-second + // "please create an issue" notice on every launch. for (const macro of macros) { - if (!Array.isArray(macro.commands)) continue; - - for (const command of macro.commands) { + for (const command of commandListOf(macroCommandsValueOf(macro))) { if ( isNestedChoiceCommand(command) && isOldTemplateChoice(command.choice) @@ -72,7 +82,23 @@ const incrementFileNameSettingMoveToDefaultBehavior: Migration = { // See the sibling migrations (#1566): never rewrite a corrupt root with [], // and stay PENDING when the choices half could not run, so a vault repaired // by hand is still migrated. - const treeReadable = !treeHasUnreadableChildren(plugin.settings.choices); + // This migration recurses `Multi.choices` itself and walks the legacy + // `settings.macros` separately, so it asks the FOLDERS-ONLY question plus + // "can I read the macros container at all" - blocking it on a `macro.commands` + // inside the CHOICE tree, which it never descends, would strand it for + // nothing (#1610). + // + // Knowingly narrower than its own traversal in one place: it DOES descend + // each legacy macro's own `commands`, and an unreadable one there is skipped + // and flagged complete. Widening the guard was tried and is inert - + // `removeMacroIndirection` runs later in the same launch, rehomes the macro + // into the choice tree and deletes `settings.macros`, so the second launch + // sees a readable `macros` and completes un-migrated anyway. Fixing it means + // moving this migration onto the shared walk; see #1627 for the sibling + // case. + const treeReadable = + !treeHasUnreadableChildren(plugin.settings.choices) && + !isUnreadableCommandList(settings.macros); if (Array.isArray(plugin.settings.choices)) { const choicesCopy = deepClone(plugin.settings.choices); plugin.settings.choices = deepClone( @@ -80,11 +106,16 @@ const incrementFileNameSettingMoveToDefaultBehavior: Migration = { ); } - const macrosCopy = deepClone(settings.macros ?? []); + // `settings.macros` is untrusted too, and `?? []` passes `{"0": {...}}` + // straight through (not nullish) - which then threw `macros is not + // iterable`, aborting and reverting the migration on every launch. Read + // through the total accessor, and only WRITE back when the original really + // was an array, so a malformed value survives to be recovered by hand. + const macrosCopy = deepClone(rootMacrosOf(settings.macros)); const macros = removeIncrementFileName(macrosCopy); // Save the migrated macros back to settings - later migrations still need it - settings.macros = macros; + if (Array.isArray(settings.macros)) settings.macros = macros; // DO NOT delete macros here – later migrations still need it. diff --git a/src/migrations/migrateFileOpeningSettings.ts b/src/migrations/migrateFileOpeningSettings.ts index 0d961d42b..f64b585dc 100644 --- a/src/migrations/migrateFileOpeningSettings.ts +++ b/src/migrations/migrateFileOpeningSettings.ts @@ -1,10 +1,13 @@ import { log } from "../logger/logManager"; import type QuickAdd from "../main"; -import type { Migration } from "./Migrations"; +import type { Migration, MigrationResult } from "./Migrations"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type IChoice from "../types/choices/IChoice"; -import { walkAllChoices } from "./helpers/choice-traversal"; +import { + settingsTreeHasUnreadableData, + walkAllChoices, +} from "./helpers/choice-traversal"; import { coerceLegacyOpenFileInNewTab, createFileOpeningFromLegacy, @@ -17,8 +20,26 @@ type LegacyFileOpeningChoice = (ITemplateChoice | ICaptureChoice) & { const migrateFileOpeningSettings: Migration = { description: "Migrate legacy openFileInNewTab settings to new fileOpening format", - migrate: async (plugin: QuickAdd) => { + migrate: async (plugin: QuickAdd): Promise => { log.logMessage("Starting migration of file opening settings..."); + + // Both halves of this migration MOVE data: they translate the legacy + // `openFileInNewTab` / `openFileInMode` keys into `fileOpening`, and nothing + // at runtime reads the legacy keys. A choice hidden behind a container this + // walk could not read would therefore lose its "open in new tab" preference + // permanently once the migration is flagged complete, even after the user + // repairs data.json. Stay pending instead (#1610). + // + // A MISSING `fileOpening`, by contrast, needs no guard at all: the engines + // call `normalizeFileOpening(this.choice.fileOpening)` on every run, so the + // defaults half is fully compensated at runtime. + // + // This migration does NOT call saveSettings() itself: migrate.ts re-syncs the + // store and saves once after the whole run. A per-migration write would be a + // full data.json rewrite, and once this can stay PENDING that is one on every + // launch, straight into Obsidian Sync's whole-file last-write-wins. + const unreadable = settingsTreeHasUnreadableData(plugin.settings); + let migratedCount = 0; @@ -56,9 +77,8 @@ const migrateFileOpeningSettings: Migration = { walkAllChoices(plugin, migrateFileOpening); log.logMessage(`Migration complete. Migrated ${migratedCount} choices.`); - - // Save the updated settings - await plugin.saveSettings(); + + if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/migrationReadability.test.ts b/src/migrations/migrationReadability.test.ts new file mode 100644 index 000000000..d7e386579 --- /dev/null +++ b/src/migrations/migrationReadability.test.ts @@ -0,0 +1,365 @@ +import { describe, expect, it, vi } from "vitest"; +import type QuickAdd from "src/main"; +import type IChoice from "src/types/choices/IChoice"; +import { settingsTreeHasUnreadableData } from "./helpers/choice-traversal"; +import { treeHasUnreadableChildren } from "src/utils/choiceUtils"; +import removeMacroIndirection from "./removeMacroIndirection"; +import backfillFileOpeningDefaults from "./backfillFileOpeningDefaults"; +import migrateFileOpeningSettings from "./migrateFileOpeningSettings"; +import consolidateFileExistsBehavior from "./consolidateFileExistsBehavior"; +import incrementFileNameSettingMoveToDefaultBehavior from "./incrementFileNameSettingMoveToDefaultBehavior"; +import mutualExclusionInsertAfterAndWriteToBottomOfFile from "./mutualExclusionInsertAfterAndWriteToBottomOfFile"; + +/** + * #1610. A migration is run once and then flagged complete FOREVER, so one that + * silently walked past a subtree it could not read leaves that subtree + * un-migrated permanently - repairing data.json by hand afterwards does not help, + * because the flag is already set. + * + * The guard therefore has to answer for exactly what the migration's own + * traversal covers. Too narrow and data is lost silently; too wide and the + * migration blocks itself on data it was never going to touch, which for + * `removeMacroIndirection` means every legacy macro choice in the vault stays + * dead. + */ + +// Factories, not shared constants: these migrations MUTATE what they walk, so a +// shared object would carry one test's migration into the next one's assertions. +const hiddenCapture = () => + ({ + id: "hidden", + name: "Hidden capture", + type: "Capture", + command: false, + openFileInNewTab: { enabled: true, direction: "horizontal", focus: false }, + }) as unknown as IChoice; + +const nested = () => ({ + id: "n", + name: "N", + type: "NestedChoice", + choice: hiddenCapture(), +}); + +const macroChoiceWith = (commands: unknown, macro?: unknown): IChoice => + ({ + id: "macro-1", + name: "Macro", + type: "Macro", + command: false, + runOnStartup: false, + macro: macro !== undefined ? macro : { id: "m", name: "Macro", commands }, + }) as unknown as IChoice; + +const makePlugin = (settings: Record) => + ({ + settings, + saveSettings: vi.fn(), + }) as unknown as QuickAdd; + +describe("settingsTreeHasUnreadableData", () => { + it("sees a nested choice hidden behind an unreadable macro.commands", () => { + // The #1610 headline: `treeHasUnreadableChildren` asked about Multi.choices + // only, so this tree reported READABLE and the migrations that walk it + // reported complete over a choice they never visited. + expect( + settingsTreeHasUnreadableData({ + choices: [macroChoiceWith({ "0": nested() })] as IChoice[], + }), + ).toBe(true); + }); + + it("actually VISITS an array-valued macro rather than calling it unreadable", async () => { + // `[].commands` is `undefined`, which walks nothing and reports readable - a + // silent hole in both the walk and the guard derived from it. The array IS + // the command list (writing `.commands` onto an Array is dropped by + // JSON.stringify), so this tree is genuinely readable AND the nested choice + // inside it has to be migrated, not merely declared visible. + const plugin = makePlugin({ + choices: [macroChoiceWith(undefined, [nested()])], + migrations: {}, + }); + + const result = await backfillFileOpeningDefaults.migrate(plugin); + + expect(result).toBeUndefined(); + const choices = (plugin.settings as unknown as Record) + .choices as Record>[]; + const visited = (choices[0].macro as unknown as Record[])[0]; + expect((visited.choice as Record).fileOpening).toEqual({ + location: "split", + direction: "horizontal", + mode: "default", + focus: false, + }); + }); + + it("reports a nested ARRAY, which the narrow migrations cannot descend", async () => { + // `typeof [] === "object"`, so without an explicit branch the visitor is + // handed the array itself, descends nothing, and reports READABLE - while + // `normalizeChoiceList` splices its members into the tree at the settings + // seam. The two halves must agree, or a migration is flagged complete over + // a choice the settings tab then makes real. + const plugin = makePlugin({ + choices: [[hiddenCapture()]], + migrations: {}, + }); + + expect( + settingsTreeHasUnreadableData({ choices: [[hiddenCapture()]] as never }), + ).toBe(true); + expect(treeHasUnreadableChildren([[hiddenCapture()]])).toBe(true); + + // ...so every migration stays PENDING rather than completing over a choice + // its own traversal cannot reach. The user's first visit to the settings tab + // splices the entry into a real choice, and the next launch is readable. + await expect( + backfillFileOpeningDefaults.migrate(plugin), + ).resolves.toEqual({ complete: false }); + await expect( + removeMacroIndirection.migrate(plugin), + ).resolves.toEqual({ complete: false }); + }); + + it("sees an unreadable conditional branch", () => { + expect( + settingsTreeHasUnreadableData({ + choices: [ + macroChoiceWith([ + { + id: "c", + name: "If", + type: "Conditional", + thenCommands: { "0": nested() }, + elseCommands: [], + }, + ]), + ] as IChoice[], + }), + ).toBe(true); + }); + + it("sees an unreadable legacy settings.macros", () => { + expect( + settingsTreeHasUnreadableData({ + choices: [] as IChoice[], + macros: { "0": { id: "m", name: "M", commands: [nested()] } }, + }), + ).toBe(true); + // ...but a missing one is the NORMAL post-consolidation state. + expect(settingsTreeHasUnreadableData({ choices: [] as IChoice[] })).toBe(false); + expect( + settingsTreeHasUnreadableData({ choices: [] as IChoice[], macros: [] }), + ).toBe(false); + }); + + it("is strict about the root and lenient about every container below it", () => { + for (const root of [undefined, null, {}, "", 0, false]) { + expect( + settingsTreeHasUnreadableData({ choices: root as unknown as IChoice[] }), + `root ${JSON.stringify(root) ?? "undefined"}`, + ).toBe(true); + } + for (const empty of [undefined, null, {}, "", 0, false]) { + expect( + settingsTreeHasUnreadableData({ + choices: [macroChoiceWith(empty)] as IChoice[], + }), + `commands ${JSON.stringify(empty) ?? "undefined"}`, + ).toBe(false); + } + }); +}); + +describe("the guarded migrations over an unreadable macro.commands", () => { + const treeWithHiddenChoice = () => [ + { + id: "t", + name: "T", + type: "Template", + command: false, + openFileInNewTab: { enabled: true, direction: "horizontal", focus: false }, + }, + macroChoiceWith({ "0": nested() }), + ]; + + // Each row carries its OWN effect assertion. A shared one that merely checked + // "something is truthy" would pass with every visitor body deleted, which is + // exactly what the first version of this test did. + const legacyFileOpening = { + location: "split", + direction: "horizontal", + mode: "default", + focus: false, + }; + + it.each([ + [ + "backfillFileOpeningDefaults", + backfillFileOpeningDefaults, + (c: Record) => + expect(c.fileOpening).toEqual(legacyFileOpening), + ], + [ + "migrateFileOpeningSettings", + migrateFileOpeningSettings, + (c: Record) => + expect(c.fileOpening).toEqual(legacyFileOpening), + ], + [ + "consolidateFileExistsBehavior", + consolidateFileExistsBehavior, + (c: Record) => + expect(c.fileExistsBehavior).toBeDefined(), + ], + ])( + "%s stays pending instead of being flagged complete", + async (_name, migration, assertReadableHalfRan) => { + const plugin = makePlugin({ + choices: treeWithHiddenChoice(), + migrations: {}, + }); + + const result = await migration.migrate(plugin); + + expect(result).toEqual({ complete: false }); + // The readable half still ran - staying pending must not mean doing + // nothing, or a user who never repairs data.json gets no migration at all. + const settings = plugin.settings as unknown as { + choices: Record[]; + }; + assertReadableHalfRan(settings.choices[0]); + }, + ); + + it("does not write data.json itself while pending", async () => { + const plugin = makePlugin({ choices: treeWithHiddenChoice(), migrations: {} }); + await backfillFileOpeningDefaults.migrate(plugin); + await migrateFileOpeningSettings.migrate(plugin); + expect(plugin.saveSettings).not.toHaveBeenCalled(); + }); + + it("does NOT block removeMacroIndirection, which never descends commands", async () => { + // It walks with flattenChoices. Blocking it here would be pure cost, and + // expensive cost: nothing at runtime resolves `macroId`, so every legacy + // macro choice stays dead until this migration completes. + const plugin = makePlugin({ + choices: [ + macroChoiceWith({ "0": nested() }), + { id: "legacy", name: "Legacy", type: "Macro", macroId: "old-macro" }, + ], + macros: [{ id: "old-macro", name: "Old", commands: [] }], + migrations: {}, + }); + + const result = await removeMacroIndirection.migrate(plugin); + + expect(result).toBeUndefined(); + const settings = plugin.settings as unknown as Record; + expect(settings.macros).toBeUndefined(); + const legacy = (settings.choices as Record[])[1]; + expect(legacy.macroId).toBeUndefined(); + expect(legacy.macro).toEqual({ id: "old-macro", name: "Old", commands: [] }); + }); +}); + +describe("an untrusted settings.macros", () => { + // Each of these used to throw `macros is not iterable`, which migrate.ts + // catches, reverts and reports with a 15-second "please create an issue" + // notice - on every single launch, because the flag stays unset. + const arrayLikeMacros = { "0": { id: "m", name: "M", commands: [] } }; + + it.each([ + ["removeMacroIndirection", removeMacroIndirection], + [ + "incrementFileNameSettingMoveToDefaultBehavior", + incrementFileNameSettingMoveToDefaultBehavior, + ], + [ + "mutualExclusionInsertAfterAndWriteToBottomOfFile", + mutualExclusionInsertAfterAndWriteToBottomOfFile, + ], + ])("%s stays pending and leaves the value verbatim", async (_name, migration) => { + const plugin = makePlugin({ + choices: [], + macros: arrayLikeMacros, + migrations: {}, + }); + + const result = await migration.migrate(plugin); + + expect(result).toEqual({ complete: false }); + // Never rewritten to the [] the READ accessor hands back: the user has to be + // able to recover this by hand. + expect((plugin.settings as unknown as Record).macros).toEqual( + arrayLikeMacros, + ); + }); + + it.each([ + ["removeMacroIndirection", removeMacroIndirection], + [ + "incrementFileNameSettingMoveToDefaultBehavior", + incrementFileNameSettingMoveToDefaultBehavior, + ], + [ + "mutualExclusionInsertAfterAndWriteToBottomOfFile", + mutualExclusionInsertAfterAndWriteToBottomOfFile, + ], + ])("%s steps over a hole in the legacy macro list", async (_name, migration) => { + // `macros: [null, ...]` IS an array, so it reads as readable and reaches the + // loop - where `macro.commands` on a hole throws, aborting and reverting the + // migration on every launch. + const plugin = makePlugin({ + choices: [], + macros: [null, "stray", { id: "m", name: "M", commands: [] }], + migrations: {}, + }); + + await expect(migration.migrate(plugin)).resolves.not.toThrow(); + }); + + it.each([ + [ + "incrementFileNameSettingMoveToDefaultBehavior", + incrementFileNameSettingMoveToDefaultBehavior, + ], + [ + "mutualExclusionInsertAfterAndWriteToBottomOfFile", + mutualExclusionInsertAfterAndWriteToBottomOfFile, + ], + ])("%s reads an ARRAY-valued legacy macro as its command list", async (_name, migration) => { + const plugin = makePlugin({ + choices: [], + macros: [[nested()]], + migrations: {}, + }); + + await expect(migration.migrate(plugin)).resolves.toBeUndefined(); + }); +}); + +describe("a corrupt root choices value", () => { + it("never reaches removeMacroIndirection's push", async () => { + for (const root of [null, {}, "not a list", 7]) { + const plugin = makePlugin({ + choices: root, + macros: [{ id: "orphan", name: "Orphan", commands: [] }], + migrations: {}, + }); + + // The point of the separate Array.isArray guard: a readability predicate + // must never be the only thing standing between a corrupt value and a + // TypeError out of `settings.choices.push`. + await expect( + removeMacroIndirection.migrate(plugin), + ).resolves.toEqual({ complete: false }); + const settings = plugin.settings as unknown as Record; + expect(settings.choices).toEqual(root); + // ...and the orphan source is still there to be rehomed on a later launch. + expect(settings.macros).toEqual([ + { id: "orphan", name: "Orphan", commands: [] }, + ]); + } + }); +}); diff --git a/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts b/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts index 61ab50e7b..9319c7e5c 100644 --- a/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts +++ b/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts @@ -7,6 +7,12 @@ import type { Migration, MigrationResult } from "./Migrations"; import { deepClone } from "src/utils/deepClone"; import type QuickAdd from "src/main"; import { treeHasUnreadableChildren } from "src/utils/choiceUtils"; +import { + commandListOf, + isUnreadableCommandList, + macroCommandsValueOf, + rootMacrosOf, +} from "src/utils/macroUtils"; type SettingsWithLegacyMacros = QuickAdd["settings"] & { macros?: IMacro[] }; @@ -30,10 +36,14 @@ function recursiveMigrateSettingInChoices(choices: IChoice[]): IChoice[] { } function migrateSettingsInMacros(macros: IMacro[]): IMacro[] { + // `macros` is untrusted end to end, not just at its root: a hole (`null`, a + // stray primitive) survives a truncated write and must be stepped over + // rather than dereferenced, and an ARRAY-valued macro IS its command list + // (#1593). Both reach `macro.commands` otherwise, and the TypeError aborts + // the whole migration - caught, reverted, and reported with a 15-second + // "please create an issue" notice on every launch. for (const macro of macros) { - if (!Array.isArray(macro.commands)) continue; - - for (const command of macro.commands) { + for (const command of commandListOf(macroCommandsValueOf(macro))) { if ( isNestedChoiceCommand(command) && isCaptureChoice(command.choice) @@ -63,17 +73,38 @@ const mutualExclusionInsertAfterAndWriteToBottomOfFile: Migration = { // choices half simply has not run, so stay PENDING - migrations are flagged // once and never retried, and a vault repaired by hand deserves to be // migrated. The macros half below is independent and still runs. - const treeReadable = !treeHasUnreadableChildren(plugin.settings.choices); + // This migration recurses `Multi.choices` itself and walks the legacy + // `settings.macros` separately, so it asks the FOLDERS-ONLY question plus + // "can I read the macros container at all" - blocking it on a `macro.commands` + // inside the CHOICE tree, which it never descends, would strand it for + // nothing (#1610). + // + // Knowingly narrower than its own traversal in one place: it DOES descend + // each legacy macro's own `commands`, and an unreadable one there is skipped + // and flagged complete. Widening the guard was tried and is inert - + // `removeMacroIndirection` runs later in the same launch, rehomes the macro + // into the choice tree and deletes `settings.macros`, so the second launch + // sees a readable `macros` and completes un-migrated anyway. Fixing it means + // moving this migration onto the shared walk; see #1627 for the sibling + // case. + const treeReadable = + !treeHasUnreadableChildren(plugin.settings.choices) && + !isUnreadableCommandList(settings.macros); if (Array.isArray(plugin.settings.choices)) { const choicesCopy = deepClone(plugin.settings.choices); plugin.settings.choices = recursiveMigrateSettingInChoices(choicesCopy); } - const macrosCopy = deepClone(settings.macros ?? []); + // `settings.macros` is untrusted too, and `?? []` passes `{"0": {...}}` + // straight through (not nullish) - which then threw `macros is not + // iterable`, aborting and reverting the migration on every launch. Read + // through the total accessor, and only WRITE back when the original really + // was an array, so a malformed value survives to be recovered by hand. + const macrosCopy = deepClone(rootMacrosOf(settings.macros)); const macros = migrateSettingsInMacros(macrosCopy); // Save the migrated macros back to settings - later migrations still need it - settings.macros = macros; + if (Array.isArray(settings.macros)) settings.macros = macros; // DO NOT delete macros here – later migrations still need it. diff --git a/src/migrations/removeMacroIndirection.ts b/src/migrations/removeMacroIndirection.ts index 46b1d6258..21ff7a6a7 100644 --- a/src/migrations/removeMacroIndirection.ts +++ b/src/migrations/removeMacroIndirection.ts @@ -6,6 +6,7 @@ import { flattenChoices, treeHasUnreadableChildren, } from "src/utils/choiceUtils"; +import { isUnreadableCommandList, rootMacrosOf } from "src/utils/macroUtils"; import type { Migration, MigrationResult } from "./Migrations"; type LegacySettings = QuickAdd["settings"] & { macros?: LegacyMacro[] }; @@ -31,6 +32,13 @@ const removeMacroIndirection: Migration = { // then delete `settings.macros`; the hidden choice would keep a dangling // macroId forever, because a completed migration never retries. Stay // pending instead, so a vault repaired by hand is still migrated (#1566). + // + // It walks with `flattenChoices`, which descends `Multi.choices` only, so it + // asks the FOLDERS-ONLY question rather than `settingsTreeHasUnreadableData`. + // Blocking it on an unreadable `macro.commands` it never descends would be + // pure cost, and pending is not cheap here the way it is for the fileOpening + // migrations: nothing at runtime resolves `macroId`, so every legacy macro + // choice in the vault stays dead until this one completes (#1610). if (treeHasUnreadableChildren(settings.choices)) { log.logMessage( "QuickAdd could not read part of the choice list, so legacy macros were left in place to be migrated later.", @@ -38,8 +46,32 @@ const removeMacroIndirection: Migration = { return { complete: false }; } + // The one root WRITE among the migrations (`settings.choices.push` below), so + // it needs its own `Array.isArray` guard rather than leaning on the + // readability question above: a guard must never be the only thing standing + // between a corrupt value and a TypeError. Guarding the push ALONE would be + // worse than the crash - the migration would go on to `delete settings.macros` + // and lose the orphans it failed to rehome. + if (!Array.isArray(settings.choices)) { + log.logMessage( + "QuickAdd could not read the choice list, so legacy macros were left in place to be migrated later.", + ); + return { complete: false }; + } + + // A legacy `macros` value that is not an array is as untrusted as the rest of + // data.json; `?? []` let `{"0": {...}}` through and threw `oldMacros is not + // iterable`. Stay pending rather than deleting `macros` below having rehomed + // nothing. + if (isUnreadableCommandList(settings.macros)) { + log.logMessage( + "QuickAdd could not read the legacy macro list, so it was left in place to be migrated later.", + ); + return { complete: false }; + } + // Check if we have the old macros array - const oldMacros = settings.macros ?? []; + const oldMacros = rootMacrosOf(settings.macros); // Map macroId → all choices that reference it const choicesByMacroId = new Map(); @@ -61,6 +93,13 @@ const removeMacroIndirection: Migration = { // Process each macro from the old macros array for (const macro of oldMacros) { + // A hole (`null`, a stray primitive) carries no macro to rehome, and + // dereferencing `macro.id` on one would abort the whole migration. + if (!macro || typeof macro !== "object") continue; + // Nor can an entry with no usable identity: with no `id` the lookup below + // misses, the orphan branch runs, and a nameless MacroChoice is pushed + // into the choice tree - then made permanent when `macros` is deleted. + if (typeof macro.id !== "string" || macro.id === "") continue; const referencingChoices = choicesByMacroId.get(macro.id) ?? allChoices.filter( diff --git a/src/services/choiceService.test.ts b/src/services/choiceService.test.ts index a72f72d94..0a3a7251e 100644 --- a/src/services/choiceService.test.ts +++ b/src/services/choiceService.test.ts @@ -525,6 +525,74 @@ describe("choiceService", () => { expect(message).toContain("macro commands"); }); + // #1612. Delete is the one irreversible action in the list, and the two + // container types were asymmetric: a folder whose children could not be read + // said so, a macro whose commands could not be read said only the generic + // line - true, and silent about the case that matters. Since #1593 the + // builder tells the user about that state and refuses to overwrite it, so + // the one screen that is about to destroy it must not be the quiet one. + it.each([ + ["array-turned-object commands", { "0": { id: "c", type: "Wait" } }, undefined], + ["string commands", "not a list", undefined], + ["number commands", 7, undefined], + ["an unreadable macro object", undefined, "not a macro"], + ["a numeric macro", undefined, 7], + ])("says QuickAdd could not read the commands: %s", async (_label, commands, macroValue) => { + mocks.yesNoPrompt.mockResolvedValue(true); + const macro = { + id: "m", + name: "MyMacro", + type: "Macro", + command: false, + runOnStartup: false, + macro: macroValue !== undefined ? macroValue : { id: "mm", name: "MyMacro", commands }, + } as unknown as IChoice; + + await expect(deleteChoiceWithConfirmation(macro, fakeApp)).resolves.toBe( + true, + ); + const message = mocks.yesNoPrompt.mock.calls[0][2] as string; + expect(message).toContain("couldn't read this macro's commands"); + expect(message).toContain("still stored under it in data.json"); + }); + + it.each([ + ["an empty list", []], + ["a real list", [{ id: "c", name: "Wait", type: "Wait", time: 1 }]], + ["no commands key", undefined], + ["a null macro", null], + // An OBJECT-valued macro is a macro object with no commands - #1593's + // `isMacroObject`. It is empty, not unreadable, and the builder can write + // a real list into it, so it keeps the ordinary line. + ["an object macro with no commands key", undefined], + // An ARRAY-valued macro IS the command list, so the builder can read and + // edit it - it must keep the ordinary line, not the alarming one. + ["an array-valued macro", "ARRAY"], + ])("keeps the ordinary line when the commands are readable: %s", async (label, value) => { + mocks.yesNoPrompt.mockResolvedValue(true); + const macroValue = + value === "ARRAY" + ? [{ id: "c", name: "Wait", type: "Wait", time: 1 }] + : value === null + ? null + : { id: "mm", name: "MyMacro", ...(value === undefined ? {} : { commands: value }) }; + const macro = { + id: "m", + name: "MyMacro", + type: "Macro", + command: false, + runOnStartup: false, + macro: macroValue, + } as unknown as IChoice; + + await deleteChoiceWithConfirmation(macro, fakeApp); + const message = mocks.yesNoPrompt.mock.calls[0][2] as string; + expect(message, label).toContain( + "Deleting this choice will also delete its macro commands.", + ); + expect(message, label).not.toContain("couldn't read"); + }); + it("clears nested user-script secrets when deleting a Macro choice", async () => { mocks.yesNoPrompt.mockResolvedValue(true); const deleteSecret = vi.fn(); diff --git a/src/services/choiceService.ts b/src/services/choiceService.ts index 731d286a5..b94ff9f42 100644 --- a/src/services/choiceService.ts +++ b/src/services/choiceService.ts @@ -16,7 +16,11 @@ import type ITemplateChoice from "../types/choices/ITemplateChoice"; import { MacroChoice } from "../types/choices/MacroChoice"; import { MultiChoice } from "../types/choices/MultiChoice"; import { TemplateChoice } from "../types/choices/TemplateChoice"; -import { regenerateIds } from "../utils/macroUtils"; +import { + isUnreadableCommandList, + macroCommandsValueOf, + regenerateIds, +} from "../utils/macroUtils"; import { childChoicesOf, flattenChoices, @@ -137,8 +141,10 @@ function collectUserScriptPathsFromChoice( ): void { if (!isRecord(choice)) return; - if (choice.type === "Macro" && isRecord(choice.macro)) { - const commands = choice.macro.commands; + if (choice.type === "Macro") { + // Same reading as the macro builder and the secret sanitizer: an + // ARRAY-valued `macro` IS the command list (#1609). + const commands = macroCommandsValueOf(choice.macro); if (Array.isArray(commands)) { for (const command of commands) { collectUserScriptPathsFromCommand(command, paths); @@ -267,10 +273,28 @@ export async function deleteChoiceWithConfirmation( return `Deleting this folder will also delete everything inside it: ${parts.join(" and ")}.`; }; + // Delete is the one irreversible action in the settings list, and until #1612 + // the two container types were treated asymmetrically: a folder whose children + // could not be read said so, a macro whose commands could not be read said only + // the generic line - which is true, and silent about the case that matters. + // + // Since #1593 the macro builder tells the user about that state and refuses to + // overwrite it. This reads the SAME predicate through the SAME accessor the + // builder resolves its list with, so the screen that reports the state and the + // screen that destroys it cannot disagree. `macroCommandsValueOf` (not + // `macro?.commands`) is what makes the unreadable-`macro`-object case - a + // string, a number, an array-turned-object - resolve here exactly as it does + // there; a genuinely array-valued `macro` IS the command list, is readable, and + // correctly keeps the ordinary line. + const buildMacroWarning = (macroChoice: IMacroChoice): string => + isUnreadableCommandList(macroCommandsValueOf(macroChoice.macro)) + ? "QuickAdd couldn't read this macro's commands. Deleting it also deletes whatever is still stored under it in data.json." + : "Deleting this choice will also delete its macro commands."; + const body = [ `Are you sure you want to delete '${choice.name}'?`, isMulti ? buildMultiWarning(choice as IMultiChoice) : "", - isMacro ? "Deleting this choice will also delete its macro commands." : "", + isMacro ? buildMacroWarning(choice as IMacroChoice) : "", ] .filter(Boolean) .join(" "); diff --git a/src/services/duplicateChoice.nestedIds.test.ts b/src/services/duplicateChoice.nestedIds.test.ts new file mode 100644 index 000000000..e5798f28b --- /dev/null +++ b/src/services/duplicateChoice.nestedIds.test.ts @@ -0,0 +1,268 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); + +import { duplicateChoice } from "./choiceService"; +import type IChoice from "../types/choices/IChoice"; +import { + buildUserScriptSecretId, + createUserScriptSecretRef, +} from "../utils/userScriptSecrets"; +import type { IUserScript } from "../types/macros/IUserScript"; + +/** + * #1609. `regenerateIds` re-ided the macro and its TOP-LEVEL commands only, so a + * duplicated macro shared every id below that with its original. Nothing crashed + * - it is not a within-list collision - but a stored user-script secret is keyed + * on `command.id` (`buildUserScriptSecretId`), so the copy re-adopted the + * original's secret slot: change the API key on the copy and you changed it on + * the original. + */ + +const collectIds = (value: unknown, out: unknown[] = []): unknown[] => { + if (Array.isArray(value)) { + for (const entry of value) collectIds(entry, out); + return out; + } + if (typeof value !== "object" || value === null) return out; + for (const [key, nested] of Object.entries(value)) { + if (key === "id") out.push(nested); + else collectIds(nested, out); + } + return out; +}; + +const collectSecretRefs = (value: unknown, out: string[] = []): string[] => { + if (typeof value !== "object" || value === null) return out; + if (Array.isArray(value)) { + for (const entry of value) collectSecretRefs(entry, out); + return out; + } + const record = value as Record; + if (typeof record.secretRef === "string") out.push(record.secretRef); + for (const nested of Object.values(record)) collectSecretRefs(nested, out); + return out; +}; + +const userScript = (id: string) => ({ + id, + name: "secrets.js", + type: "UserScript", + path: "scripts/secrets.js", + settings: { "API key": createUserScriptSecretRef("secrets-api-key") }, +}); + +const macroChoice = (macro: unknown): IChoice => + ({ + id: "macro-choice", + name: "Macro", + type: "Macro", + command: false, + runOnStartup: false, + macro, + }) as unknown as IChoice; + +const deepMacroCommands = () => [ + userScript("top-script"), + { + id: "cond", + name: "If", + type: "Conditional", + condition: { mode: "variable", variableName: "x", operator: "isTruthy" }, + thenCommands: [userScript("then-script")], + elseCommands: [ + { + id: "nested-cmd", + name: "Inner", + type: "NestedChoice", + choice: { + id: "inner-macro-choice", + name: "Inner macro", + type: "Macro", + macro: { + id: "inner-macro", + name: "Inner macro", + commands: [userScript("inner-script")], + }, + }, + }, + ], + }, + { + id: "nested-folder-cmd", + name: "Folder", + type: "NestedChoice", + choice: { + id: "inner-folder", + name: "Inner folder", + type: "Multi", + choices: [{ id: "inner-leaf", name: "Leaf", type: "Capture" }], + }, + }, +]; + +describe("duplicating a macro", () => { + it("shares no id with the original, at any depth", () => { + const source = macroChoice({ + id: "m", + name: "Macro", + commands: deepMacroCommands(), + }); + + const copy = duplicateChoice(source); + + const originalIds = collectIds(source); + const copyIds = collectIds(copy); + expect(copyIds).toHaveLength(originalIds.length); + expect( + copyIds.filter((id) => originalIds.includes(id)), + "ids shared with the original", + ).toEqual([]); + // Every id inside the copy is distinct from every other. + expect(new Set(copyIds).size).toBe(copyIds.length); + }); + + it("carries no secret reference belonging to the original", () => { + // Pins the STRIP half: it already recursed branch commands and a nested + // choice's own macro before this change, and must keep doing so. + const source = macroChoice({ + id: "m", + name: "Macro", + commands: deepMacroCommands(), + }); + + const copy = duplicateChoice(source); + + expect(collectSecretRefs(source)).toContain("secrets-api-key"); + expect(collectSecretRefs(copy)).toEqual([]); + }); + + it("gives a nested user script a DIFFERENT stored-secret slot", () => { + // The reported symptom, asserted directly. Stripping the ref is not enough: + // `migrateUserScriptSecretSettings` re-adopts an existing secret by + // `buildUserScriptSecretId`, which keys on `command.id` - so while the copy's + // nested command kept the original's id, the copy silently adopted the + // original's slot and the next edit overwrote the original's key. + const source = macroChoice({ + id: "m", + name: "Macro", + commands: deepMacroCommands(), + }); + + const copy = duplicateChoice(source); + + const nestedScript = (choice: IChoice): IUserScript => { + const commands = (choice as unknown as Record>) + .macro.commands as Record[]; + const conditional = commands[1] as unknown as Record; + return conditional.thenCommands[0]; + }; + + expect( + buildUserScriptSecretId(nestedScript(copy), "API key"), + ).not.toBe(buildUserScriptSecretId(nestedScript(source), "API key")); + }); + + it("does the same for an ARRAY-valued macro, where the array IS the commands", () => { + // `isRecord([])` is true, so the sanitizer read `.commands` off an array, + // got `undefined`, and stripped nothing - the copy kept the original's + // literal secretRef, and editing its key wrote over the original's slot. + const source = macroChoice(deepMacroCommands()); + + const copy = duplicateChoice(source); + + expect(collectSecretRefs(copy)).toEqual([]); + const originalIds = collectIds(source); + expect( + collectIds(copy).filter((id) => originalIds.includes(id)), + ).toEqual([]); + }); + + it("recurses a NESTED command array, which is a list and not a command", () => { + // `normalizeCommandList` splices a nested array into the list at the editor + // seam, so its entries are real commands. Treating the array itself as one + // writes an `id` that JSON.stringify drops and leaves every id inside it - + // and every secretRef - shared with the original. + const source = macroChoice({ + id: "m", + name: "M", + commands: [[userScript("nested-in-array")]], + }); + + const copy = duplicateChoice(source) as unknown as Record< + string, + Record + >; + + const inner = copy.macro.commands[0][0] as Record; + expect(inner.id).not.toBe("nested-in-array"); + expect(collectSecretRefs(copy)).toEqual([]); + }); + + it("leaves a malformed command list exactly as found", () => { + // A WRITE path: `{"0": {...}}` must survive on disk to be recovered by hand, + // so the copy is as faithful as the original rather than "repaired" to []. + for (const commands of [{ "0": userScript("hidden") }, "not a list", 7, null]) { + const source = macroChoice({ id: "m", name: "M", commands }); + + const copy = duplicateChoice(source) as unknown as Record< + string, + Record + >; + + expect(copy.macro.commands, JSON.stringify(commands)).toEqual(commands); + expect(copy.macro.id).not.toBe("m"); + } + }); + + it("steps over a hole rather than dereferencing it", () => { + const source = macroChoice({ + id: "m", + name: "M", + commands: [null, "stray", userScript("real")], + }); + + const copy = duplicateChoice(source) as unknown as Record< + string, + Record + >; + + const commands = copy.macro.commands as unknown[]; + expect(commands).toHaveLength(3); + expect(commands[0]).toBeNull(); + expect(commands[1]).toBe("stray"); + expect((commands[2] as Record).id).not.toBe("real"); + }); + + it("gives a SHARED node one new id rather than two", () => { + // `deepClone` is `structuredClone`, which preserves shared references. Two + // pointers to one object are one command, so it must be re-ided once - not + // visited twice and left with the second mint under the first pointer. + const shared = { id: "shared", name: "Wait", type: "Wait", time: 1 }; + const source = macroChoice({ + id: "m", + name: "M", + commands: [ + { + id: "cond", + name: "If", + type: "Conditional", + thenCommands: [shared], + elseCommands: [shared], + }, + ], + }); + + const copy = duplicateChoice(source) as unknown as Record< + string, + Record[]> + >; + + const cond = copy.macro.commands[0] as unknown as Record< + string, + Record[] + >; + expect(cond.thenCommands[0]).toBe(cond.elseCommands[0]); + expect(cond.thenCommands[0].id).not.toBe("shared"); + }); +}); diff --git a/src/utils/choiceUtils.test.ts b/src/utils/choiceUtils.test.ts index afbf7c08a..e8fb725d2 100644 --- a/src/utils/choiceUtils.test.ts +++ b/src/utils/choiceUtils.test.ts @@ -389,17 +389,47 @@ describe("treeHasUnreadableChildren (#1566)", () => { it("is true for an unreadable folder at any depth", () => { // The whole point: a root-only check passes this, and the migration that // relies on it then destroys data it could not see. - expect(treeHasUnreadableChildren([brokenFolder({})])).toBe(true); expect( - treeHasUnreadableChildren([multi("Folder", [brokenFolder(undefined)])]), + treeHasUnreadableChildren([brokenFolder({ "0": choice("Hidden") })]), + ).toBe(true); + expect( + treeHasUnreadableChildren([multi("Folder", [brokenFolder("not a list")])]), ).toBe(true); expect( treeHasUnreadableChildren([ - multi("Outer", [multi("Inner", [brokenFolder(null)])]), + multi("Outer", [multi("Inner", [brokenFolder(7)])]), ]), ).toBe(true); }); + it("is false for a folder whose children value carries nothing", () => { + // #1610/#1611. Below the root this asks the same question the folder hint + // asks (isUnreadableChoiceList), and `undefined`/`null`/`{}`/`""`/`0`/`false` + // hide no choices - they are an empty folder. Calling them unreadable kept + // removeMacroIndirection PENDING FOREVER over one folder with no `choices` + // key: legacy macros were never embedded, never removed, and every legacy + // macro choice in the vault stayed dead, because nothing at runtime resolves + // a `macroId`. + for (const empty of [undefined, null, {}, "", 0, false]) { + expect( + treeHasUnreadableChildren([brokenFolder(empty)]), + JSON.stringify(empty) ?? "undefined", + ).toBe(false); + } + }); + + it("keeps the ROOT strict, where any non-array is corruption", () => { + // The asymmetry with the folder rule is deliberate: loadSettings merges over + // DEFAULT_SETTINGS, so a MISSING `choices` key is already []. A root that is + // not an array is therefore never an honest empty - and removeMacroIndirection + // pushes straight into it. + for (const root of [undefined, null, {}, "", 0, false, "not a list", 7]) { + expect(treeHasUnreadableChildren(root), JSON.stringify(root) ?? "undefined").toBe( + true, + ); + } + }); + it("steps over a hole in the list rather than calling it unreadable", () => { expect( treeHasUnreadableChildren([null as unknown as IChoice, choice("Leaf")]), diff --git a/src/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index 29b126085..de63795de 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -70,16 +70,42 @@ export function hasChildChoices(choice: IChoice): boolean { * repairs data.json by hand. Such a migration must stay pending instead, and * this is the question it has to ask about the WHOLE tree, not just the root. * + * This is the narrow, FOLDERS-ONLY question, for the migrations that recurse + * `Multi.choices` themselves (removeMacroIndirection via `flattenChoices`, + * incrementFileName..., mutualExclusion...). A migration that walks with + * `walkAllChoices` reaches macro commands too and must ask the wider + * `settingsTreeHasUnreadableData` instead. Matching the guard to the traversal + * is deliberate: blocking `removeMacroIndirection` on an unreadable + * `macro.commands` it was never going to descend would strand every legacy macro + * choice in the vault (nothing at runtime resolves `macroId`) in exchange for + * nothing at all. + * + * The root is judged strictly and everything below it by + * {@link isUnreadableChoiceList} - see the same asymmetry, and why, in + * `walkSettings`. A folder with no `choices` key carries nothing, and treating + * it as unreadable (as this did until #1610) kept `removeMacroIndirection` + * pending forever over a folder that was merely empty. + * * See #1566, and `MigrationResult` in src/migrations/Migrations.ts. */ export function treeHasUnreadableChildren(choices: unknown): boolean { if (!Array.isArray(choices)) return true; - return choices.some((choice) => { - if (!isChoiceLike(choice)) return false; - if (choice.type !== "Multi") return false; - if (!Array.isArray((choice as IMultiChoice).choices)) return true; - return treeHasUnreadableChildren((choice as IMultiChoice).choices); - }); + const walk = (list: IChoice[]): boolean => + list.some((choice) => { + // A nested ARRAY can be carrying choices - the editor seam splices one + // into the tree - and `flattenChoices` pushes it as if it were a choice + // rather than descending it. So removeMacroIndirection would classify a + // macro referenced from inside one as orphaned, duplicate it at the root + // and delete `settings.macros`. Stay pending until the seam has repaired + // it (#1608/#1610). + if (Array.isArray(choice)) return true; + if (!isChoiceLike(choice)) return false; + if (choice.type !== "Multi") return false; + const children: unknown = (choice as IMultiChoice).choices; + if (!Array.isArray(children)) return isUnreadableChoiceList(children); + return walk(children); + }); + return walk(choices); } /** @@ -94,27 +120,41 @@ export function rootChoicesOf(value: unknown): IChoice[] { } /** - * Whether a Multi's `choices` holds something we cannot read, as opposed to + * Whether a `choices` VALUE holds something we cannot read, as opposed to * nothing at all. True only for the malformed shapes that can still CARRY * choices: a non-empty object where an array belongs (`{"0": {...}, "1": {...}}`, - * the classic array-turned-object JSON artefact) or some other non-empty - * primitive. `undefined`, `null` and `{}` carry nothing, so for those the folder - * really is empty and the ordinary empty-folder hint is the honest thing to say. + * the classic array-turned-object JSON artefact) or a non-empty primitive. + * `undefined`, `null`, `{}`, `""`, `0` and `false` carry nothing, so for those + * the folder really is empty and the ordinary empty-folder hint is honest. * - * This is the line between "degrade quietly" and "tell the user": a folder whose - * contents we cannot read must not claim to be empty, must offer no affordance - * that would overwrite the value, and must say so before it is deleted. The - * hint, the drop target and the delete confirmation all read this one predicate - * so they cannot disagree. + * The sibling of `isUnreadableCommandList` in macroUtils.ts, deliberately kept + * as a sibling rather than a shared module (the same shape as + * `isChoiceLike`/`isCommandLike` and `rootChoicesOf`/`rootMacrosOf`). The + * two must answer identically for every value; `unreadableValuePredicates.test.ts` + * is the ratchet that says so, over the union of both shape lists. + * + * This is the line between "degrade quietly" and "tell the user": a container + * whose contents we cannot read must not claim to be empty, must offer no + * affordance that would overwrite the value, and must say so before it is + * deleted. The hint, the drop target and the delete confirmation all read this + * one predicate so they cannot disagree. */ +export function isUnreadableChoiceList(value: unknown): boolean { + if (Array.isArray(value)) return false; + if (value === undefined || value === null) return false; + if (typeof value === "object") return Object.keys(value).length > 0; + // A non-empty primitive was never empty; an empty one carries nothing. The + // `""`/`0`/`false` arm is #1611: this function documented that rule from the + // start and its `: true` arm contradicted it, so a folder whose `choices` was + // `""` got the "couldn't read this" notice, lost its drop target, and got the + // scarier delete confirmation - for a value carrying nothing at all. + return Boolean(value); +} + +/** {@link isUnreadableChoiceList}, asked about a Multi node rather than a value. */ export function hasUnreadableChildren(choice: IChoice): boolean { if (!isMultiChoice(choice)) return false; - const children: unknown = choice.choices; - if (children === undefined || children === null) return false; - if (Array.isArray(children)) return false; - // A non-array object is only lossy when it actually has keys; anything else - // non-nullish (a string, a number) was never empty either. - return typeof children === "object" ? Object.keys(children).length > 0 : true; + return isUnreadableChoiceList(choice.choices); } /** @@ -248,6 +288,122 @@ export function dedupeChoicesById(choices: IChoice[]): IChoice[] { return walk(choices); } +export interface RepairedChoiceId { + /** The id the choice had, exactly as `data.json` held it. */ + previousId: unknown; + /** The choice as it is now, under an id that can be keyed. */ + choice: IChoice; +} + +export interface NormalizedChoiceList { + choices: IChoice[]; + /** False when `choices` is the input array itself, unchanged. */ + changed: boolean; + repaired: RepairedChoiceId[]; +} + +/** + * The choice tree an EDITOR should work over: every entry an object with an id + * that is unique across the whole tree. + * + * The sibling of `normalizeCommandList` (macroUtils.ts) and the same argument, + * on the list one level up. `ChoiceList` renders a keyed `{#each ... (choice.id)}` + * and seeds svelte-dnd-action from the same array, so an entry with no usable id + * cannot be rendered - and the list it filtered for rendering is the list its + * persist path writes back. So the filter was not a "render-time view only" after + * all: the first drag or ArrowDown wrote the filtered array to disk, and + * + * { "name": "Daily note", "type": "Template", "templatePath": "...", "id": 12 } + * + * - a complete, working, runnable choice whose id was written as a JSON number by + * a hand-edit, a script or a merge - was deleted with no prompt and no undo, from + * a row the user could never see in the first place (#1608). + * + * The two cases are NOT the same and are deliberately not treated the same: + * + * - An entry that cannot be KEYED (id missing, empty, not a string, or already + * used elsewhere in the tree) is a real choice. It is given a fresh uuid and + * kept, so it becomes visible, editable and deletable for the first time. + * - A `null` or a stray primitive carries nothing. There is nothing to re-key, + * every walker already steps over one, and it is dropped. + * - An ARRAY entry is read as a NESTED LIST and its members are spliced in - + * the same recoverable reading `macroCommandsValueOf` gives an array-valued + * `macro`. `isChoiceLike([])` is true, so the alternative is spreading it + * into one nameless, typeless row whose delete dialog says `delete + * 'undefined'`. + * + * A repaired id is always a fresh uuid, never a coercion of the old value. + * `String(12)` looks tempting - it would keep the registered `quickadd:choice:12` + * alive - but ids are compared with `===` in `getChoice`, so a `12` -> `"12"` + * rewrite silently breaks a `ChoiceCommand{choiceId: 12}` (MacroChoiceEngine + * matches /not found/i and skips the step), and "that string is free" can only + * mean "not seen YET" during a pre-order walk, so it can also steal a healthy + * later sibling's id. A stored reference to a malformed id does break here - but + * the behaviour it replaces DELETED the choice on the first reorder, which broke + * the same reference and lost the choice with it. + * + * Recurses only through `hasChildChoices`, so a folder whose `choices` value + * could not be read is passed through exactly as found - never replaced with the + * `[]` that `childChoicesOf` reads it as. + * + * Returns the input array itself when there was nothing to change, so a healthy + * tree is provably untouched, and takes `unknown` because `settings.choices` is. + * A non-array root reads as `[]` here; the CALLER must refuse to render (and + * therefore to save) rather than pass it in - see ChoiceView's `rootUnreadable`. + */ +export function normalizeChoiceList(value: unknown): NormalizedChoiceList { + if (!Array.isArray(value)) return { choices: [], changed: false, repaired: [] }; + + const seen = new Set(); + const repaired: RepairedChoiceId[] = []; + + const walk = (list: unknown[]): IChoice[] => { + let changed = false; + const out: IChoice[] = []; + + for (const entry of list) { + if (Array.isArray(entry)) { + changed = true; + out.push(...walk(entry)); + continue; + } + if (!isChoiceLike(entry)) { + changed = true; + continue; + } + + let node: IChoice = entry; + if (hasChildChoices(node)) { + // `hasChildChoices` already proved this is a real array. + const children = (node as IMultiChoice).choices as IChoice[]; + const next = walk(children); + if (next !== children) { + node = { ...(node as IMultiChoice), choices: next } as IChoice; + changed = true; + } + } + + const id: unknown = node.id; + if (typeof id === "string" && id !== "" && !seen.has(id)) { + seen.add(id); + out.push(node); + continue; + } + + const replacement = { ...node, id: uuidv4() } as IChoice; + seen.add(replacement.id); + repaired.push({ previousId: id, choice: replacement }); + out.push(replacement); + changed = true; + } + + return changed ? out : (list as IChoice[]); + }; + + const choices = walk(value); + return { choices, changed: choices !== value, repaired }; +} + export interface FlatChoicePathEntry { choice: IChoice; id: string; diff --git a/src/utils/macroUtils.test.ts b/src/utils/macroUtils.test.ts index e9d1ff51b..1607e0ba5 100644 --- a/src/utils/macroUtils.test.ts +++ b/src/utils/macroUtils.test.ts @@ -84,6 +84,39 @@ describe("isCommandLike", () => { }); }); +describe("normalizeCommandList over a nested array entry", () => { + // `isCommandLike([])` is true, so without an explicit branch an array entry + // takes the re-key path and the normalizer MANUFACTURES `{"0":…,"1":…,id:…}` - + // two real commands collapsed into one nameless row, persisted by the next + // edit. Read as a nested list instead, byte-symmetric with + // `normalizeChoiceList` (#1608). + it("splices a nested list in, preserving order and ids", () => { + const { commands, changed } = normalizeCommandList([ + wait("a"), + [wait("b"), wait("c")], + wait("d"), + ]); + + expect(changed).toBe(true); + expect(commands.map((c) => c.id)).toEqual(["a", "b", "c", "d"]); + }); + + it("re-keys an id that collides ACROSS nesting levels", () => { + const { commands } = normalizeCommandList([wait("dup"), [wait("dup")]]); + + expect(commands).toHaveLength(2); + expect(commands[0].id).toBe("dup"); + expect(commands[1].id).not.toBe("dup"); + expect(commands[1].id).not.toBe(""); + }); + + it("splices a junk array away to nothing", () => { + const { commands } = normalizeCommandList([[1, 2, 3], wait("a")]); + + expect(commands.map((c) => c.id)).toEqual(["a"]); + }); +}); + describe("normalizeCommandList", () => { it("is identity for a healthy list - same array, nothing changed", () => { const commands = [wait("a"), wait("b")]; diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts index 81cd5cc46..a9eb2676e 100644 --- a/src/utils/macroUtils.ts +++ b/src/utils/macroUtils.ts @@ -87,6 +87,23 @@ export function commandListOf(value: unknown): ICommand[] { return Array.isArray(value) ? value : []; } +/** + * The pre-consolidation `settings.macros` array, as something always safe to + * iterate. The sibling of `rootChoicesOf` for the other legacy root container: + * it is as untrusted as the rest of `data.json`, and three migrations used to + * reach it through `settings.macros ?? []`, which passes `{"0": {...}}` straight + * through (not nullish) and then throws `macros is not iterable` - aborting the + * migration, reverting it, and firing a 15-second "please create an issue" + * notice on every single launch. + * + * READ view only. A WRITE back to `settings.macros` must stay guarded by + * `Array.isArray`, or it persists this `[]` over the value the user needs to + * recover by hand. + */ +export function rootMacrosOf(value: unknown): T[] { + return Array.isArray(value) ? (value as T[]) : []; +} + /** * Whether `value` is a real command array, i.e. whether a WRITE path may rebuild * it. Guards the `{ ...macro, commands: ... }` rebuilds so a malformed list is @@ -170,6 +187,26 @@ export function normalizeCommandList(value: unknown): NormalizedCommandList { const commands: ICommand[] = []; for (const entry of input) { + // An ARRAY entry is read as a NESTED LIST and spliced in, the same + // recoverable reading `macroCommandsValueOf` gives an array-valued `macro`. + // `isCommandLike([])` is true, so the alternative is spreading it into one + // nameless, typeless row. Kept byte-symmetric with `normalizeChoiceList`. + if (Array.isArray(entry)) { + const inner = normalizeCommandList(entry); + for (const command of inner.commands) { + const id = command.id; + if (typeof id === "string" && id !== "" && !seen.has(id)) { + seen.add(id); + commands.push(command); + continue; + } + const replacement = { ...command, id: uuidv4() }; + seen.add(replacement.id); + commands.push(replacement); + } + changed = true; + continue; + } if (!isCommandLike(entry)) { changed = true; continue; @@ -192,17 +229,84 @@ export function normalizeCommandList(value: unknown): NormalizedCommandList { /** * Regenerates all IDs in a macro to prevent collisions after duplication. * - * Total over a malformed macro: a missing or non-array `commands` is left - * exactly as it is (this is a WRITE path - see `hasCommandList`), and a hole in - * the list is stepped over rather than dereferenced. Reached from - * `duplicateChoice`, which runs over whatever the user's data.json holds. + * Recurses the whole macro, not just its top level. Re-iding only the outermost + * commands left every id BELOW that identical between the original and the copy - + * a Conditional's `thenCommands`/`elseCommands`, a NestedChoice's inner choice, + * and that choice's own macro if it had one (#1609). Nothing crashed, because it + * is not a within-list collision, but: + * + * - `buildUserScriptSecretId` keys a stored user-script secret on + * `command.id`, so a duplicated macro's NESTED UserScript command re-adopted + * the ORIGINAL's secret slot. Setting the API key on the copy set it on the + * original. + * - the nested `IChoice` kept its id too, colliding with the original's in + * every by-id walk over commands (packageTraversal, collectChoiceClosure). + * + * Total over a malformed macro, because `duplicateChoice` runs over whatever + * data.json holds: the command list is resolved through `macroCommandsValueOf` + * (so an ARRAY-valued macro is re-ided as the command list it is, rather than + * having a non-index `id` written onto it that JSON.stringify would drop), a + * non-array list is left exactly as found (this is a WRITE path - see + * `hasCommandList`), and a hole is stepped over rather than dereferenced. + * + * `visited` is about SHARED references, not cycles: `deepClone` is + * `structuredClone`, which preserves both. Two pointers to one command are one + * command, so it is re-ided once rather than twice. (A true cycle cannot come + * out of `data.json` - JSON has no way to express one - and the secret sanitizer + * `duplicateChoice` runs afterwards would recurse forever on one regardless, so + * this does not claim to make that survivable.) */ export function regenerateIds(macro: IMacro): void { + regenerateMacroIds(macro, new Set()); +} + +function regenerateMacroIds(macro: unknown, visited: Set): void { if (!isCommandLike(macro)) return; - macro.id = uuidv4(); - if (!hasCommandList(macro.commands)) return; - macro.commands.forEach((command) => { - if (!isCommandLike(command)) return; + if (visited.has(macro)) return; + visited.add(macro); + // Only a real macro OBJECT has an `id` worth minting. Writing one onto an + // array-valued macro is a no-op JSON.stringify discards, not a repair. + if (isMacroObject(macro)) macro.id = uuidv4(); + regenerateCommandIds(macroCommandsValueOf(macro), visited); +} + +function regenerateCommandIds(commands: unknown, visited: Set): void { + if (!hasCommandList(commands)) return; + for (const command of commands as ICommand[]) { + // A nested ARRAY is a command list (`normalizeCommandList` splices one in), + // so recurse rather than writing an `id` onto it that JSON.stringify drops - + // which would leave every id inside it shared with the original. + if (Array.isArray(command)) { + regenerateCommandIds(command, visited); + continue; + } + if (!isCommandLike(command)) continue; + if (visited.has(command)) continue; + visited.add(command); + command.id = uuidv4(); - }); + + const branching = command as unknown as { + thenCommands?: unknown; + elseCommands?: unknown; + choice?: unknown; + }; + regenerateCommandIds(branching.thenCommands, visited); + regenerateCommandIds(branching.elseCommands, visited); + regenerateChoiceIds(branching.choice, visited); + } +} + +function regenerateChoiceIds(choice: unknown, visited: Set): void { + if (!isCommandLike(choice)) return; + if (visited.has(choice)) return; + visited.add(choice); + + const node = choice as { id?: unknown; type?: unknown; macro?: unknown; choices?: unknown }; + node.id = uuidv4(); + + if (node.type === "Macro") regenerateMacroIds(node.macro, visited); + if (node.type === "Multi" && Array.isArray(node.choices)) { + for (const child of node.choices) regenerateChoiceIds(child, visited); + } } diff --git a/src/utils/malformedChoices.fixture.ts b/src/utils/malformedChoices.fixture.ts index 2910e0a43..dd81149e7 100644 --- a/src/utils/malformedChoices.fixture.ts +++ b/src/utils/malformedChoices.fixture.ts @@ -52,6 +52,11 @@ export const MALFORMED_CHILDREN_SHAPES: { { key: "missing", value: undefined, lossy: false }, { key: "null", value: null, lossy: false }, { key: "emptyObject", value: {}, lossy: false }, + // Falsy primitives carry nothing, exactly like `null` - #1611, where the + // predicate's `: true` arm contradicted its own doc and reported them lossy. + { key: "emptyString", value: "", lossy: false }, + { key: "zero", value: 0, lossy: false }, + { key: "false", value: false, lossy: false }, { key: "arrayLikeObject", value: { "0": leaf("Hidden", "hidden-1") }, lossy: true }, { key: "string", value: "not a list", lossy: true }, { key: "number", value: 7, lossy: true }, @@ -73,6 +78,9 @@ export const MALFORMED_COMMANDS_SHAPES: { { key: "missing", value: undefined, lossy: false }, { key: "null", value: null, lossy: false }, { key: "emptyObject", value: {}, lossy: false }, + { key: "emptyString", value: "", lossy: false }, + { key: "zero", value: 0, lossy: false }, + { key: "false", value: false, lossy: false }, { key: "arrayLikeObject", value: { "0": waitCommand("hidden-cmd") }, lossy: true }, { key: "string", value: "not a list", lossy: true }, { key: "number", value: 7, lossy: true }, diff --git a/src/utils/normalizeChoiceList.test.ts b/src/utils/normalizeChoiceList.test.ts new file mode 100644 index 000000000..f919ac9dd --- /dev/null +++ b/src/utils/normalizeChoiceList.test.ts @@ -0,0 +1,209 @@ +import { describe, expect, it } from "vitest"; +import { normalizeChoiceList } from "./choiceUtils"; +import type IChoice from "../types/choices/IChoice"; +import type IMultiChoice from "../types/choices/IMultiChoice"; +import { + folder, + leaf, + malformedFolder, + malformedTree, + malformedSnapshot, + MALFORMED_CHILDREN_SHAPES, +} from "./malformedChoices.fixture"; + +const childrenOf = (choice: IChoice): IChoice[] => + (choice as IMultiChoice).choices ?? []; + +const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; + +describe("normalizeChoiceList", () => { + it("hands back the input array itself when there is nothing to change", () => { + // The contract that makes this safe to run on every distinct store value: a + // healthy tree is provably untouched, so no identity churn reaches Svelte. + const tree = [leaf("A", "a"), folder("F", "f", [leaf("B", "b")])]; + const result = normalizeChoiceList(tree); + + expect(result.choices).toBe(tree); + expect(result.changed).toBe(false); + expect(result.repaired).toEqual([]); + expect((result.choices[1] as IMultiChoice).choices).toBe( + (tree[1] as IMultiChoice).choices, + ); + }); + + it("KEEPS a whole choice whose id is a JSON number", () => { + // #1608's exact example. It is complete, working and runnable from the + // palette - and it was invisible in settings, then deleted by the first + // reorder. + const numeric = { + id: 12, + name: "Daily note", + type: "Template", + command: false, + templatePath: "Templates/Daily.md", + } as unknown as IChoice; + + const { choices, changed, repaired } = normalizeChoiceList([ + leaf("A", "a"), + numeric, + leaf("B", "b"), + ]); + + expect(changed).toBe(true); + expect(choices).toHaveLength(3); + const repairedChoice = choices[1] as unknown as Record; + expect(repairedChoice.name).toBe("Daily note"); + expect(repairedChoice.templatePath).toBe("Templates/Daily.md"); + expect(repairedChoice.id).toMatch(UUID); + expect(repaired).toEqual([{ previousId: 12, choice: choices[1] }]); + }); + + it("mints a uuid rather than coercing the old id to a string", () => { + // `String(12)` would keep `quickadd:choice:12` alive, and is wrong twice: + // ids are compared with `===` in getChoice (so a persisted 12 -> "12" breaks + // a ChoiceCommand{choiceId: 12} silently), and "that string is free" can + // only mean "not seen YET" mid-walk, so it can steal a healthy later + // sibling's id. + const { choices } = normalizeChoiceList([ + { id: 12, name: "Numeric", type: "Template" } as unknown as IChoice, + leaf("Healthy", "12"), + ]); + + expect(choices[0].id).not.toBe("12"); + expect(choices[0].id).toMatch(UUID); + // The healthy sibling keeps the id it had. A choice with a non-empty string + // id is NEVER re-keyed on account of a malformed sibling. + expect(choices[1].id).toBe("12"); + }); + + it("keeps the healthy sibling's id whichever order they appear in", () => { + const { choices } = normalizeChoiceList([ + leaf("Healthy", "12"), + { id: 12, name: "Numeric", type: "Template" } as unknown as IChoice, + ]); + + expect(choices[0].id).toBe("12"); + expect(choices[1].id).toMatch(UUID); + }); + + it("gives two id-less entries DISTINCT ids", () => { + const { choices } = normalizeChoiceList([ + { name: "One", type: "Template" } as IChoice, + { name: "Two", type: "Template" } as IChoice, + ]); + + expect(choices).toHaveLength(2); + expect(choices[0].id).not.toBe(choices[1].id); + }); + + it("de-duplicates ids across the WHOLE tree, not just within a list", () => { + // Choice ids are globally unique (the command registry keys on + // `choice:`), unlike command ids which only have to be unique in their + // own list. + const { choices } = normalizeChoiceList([ + leaf("Root", "dup"), + folder("F", "f", [leaf("Nested", "dup")]), + ]); + + const nested = childrenOf(choices[1])[0]; + expect(choices[0].id).toBe("dup"); + expect(nested.id).toMatch(UUID); + expect(nested.name).toBe("Nested"); + }); + + it("drops a hole, which carries nothing", () => { + const { choices, repaired } = normalizeChoiceList([ + null, + "stray", + 7, + leaf("Survivor", "s"), + ]); + + expect(choices).toHaveLength(1); + expect(choices[0].name).toBe("Survivor"); + expect(repaired).toEqual([]); + }); + + it("splices an ARRAY entry in as a nested list rather than spreading it", () => { + // `isChoiceLike([])` is true, so spreading would produce one nameless, + // typeless row whose delete dialog says `delete 'undefined'`. The array is + // read as a nested list - the same reading `macroCommandsValueOf` gives an + // array-valued `macro`. + const { choices } = normalizeChoiceList([ + leaf("A", "a"), + [leaf("B", "b"), leaf("C", "c")], + leaf("D", "d"), + ]); + + expect(choices.map((c) => c.name)).toEqual(["A", "B", "C", "D"]); + expect(choices.map((c) => c.id)).toEqual(["a", "b", "c", "d"]); + }); + + it("splices junk arrays away to nothing", () => { + const { choices } = normalizeChoiceList([[1, 2, 3], leaf("A", "a")]); + expect(choices.map((c) => c.name)).toEqual(["A"]); + }); + + it("leaves an unreadable folder's children value exactly as found", () => { + for (const shape of MALFORMED_CHILDREN_SHAPES.filter((s) => s.lossy)) { + const broken = malformedFolder("Broken", "broken", shape.value); + const { choices } = normalizeChoiceList([broken]); + + // Never replaced with the [] that childChoicesOf reads it as: the user + // needs it on disk to recover by hand. + expect( + (choices[0] as unknown as Record).choices, + shape.key, + ).toEqual(shape.value); + } + }); + + it("repairs a choice hidden inside an otherwise healthy folder", () => { + const { choices, changed } = normalizeChoiceList([ + folder("F", "f", [{ name: "Hidden", type: "Template" } as IChoice]), + ]); + + expect(changed).toBe(true); + const nested = childrenOf(choices[0])[0]; + expect(nested.name).toBe("Hidden"); + expect(nested.id).toMatch(UUID); + }); + + it("is idempotent: normalizing twice changes nothing the second time", () => { + const first = normalizeChoiceList(malformedTree()); + const second = normalizeChoiceList(first.choices); + + expect(second.changed).toBe(false); + expect(second.choices).toBe(first.choices); + }); + + it("preserves every unreadable container in the shared malformed tree", () => { + const tree = malformedTree(); + const before = malformedSnapshot(tree); + + const { choices } = normalizeChoiceList(tree); + + expect(malformedSnapshot(choices)).toBe(before); + }); + + it("never mutates its input", () => { + const tree = malformedTree(); + const before = JSON.stringify(tree); + + normalizeChoiceList(tree); + + expect(JSON.stringify(tree)).toBe(before); + }); + + it("reads a non-array root as empty and reports no change", () => { + // The caller must refuse to render such a root rather than passing it in + // (ChoiceView.rootUnreadable); this only guarantees it cannot throw. + for (const root of [undefined, null, {}, "not a list", 7]) { + expect(normalizeChoiceList(root)).toEqual({ + choices: [], + changed: false, + repaired: [], + }); + } + }); +}); diff --git a/src/utils/unreadableValuePredicates.test.ts b/src/utils/unreadableValuePredicates.test.ts new file mode 100644 index 000000000..f99e462b3 --- /dev/null +++ b/src/utils/unreadableValuePredicates.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { isUnreadableChoiceList } from "./choiceUtils"; +import { isUnreadableCommandList } from "./macroUtils"; +import { + MALFORMED_CHILDREN_SHAPES, + MALFORMED_COMMANDS_SHAPES, +} from "./malformedChoices.fixture"; + +/** + * `isUnreadableChoiceList` and `isUnreadableCommandList` ask the same question of + * two different containers, and four user-facing surfaces read one or the other: + * the folder hint and the folder's drop target read the choice one, the macro + * builder's card and its suppressed controls read the command one, and the delete + * confirmation reads BOTH (#1612). They are deliberately siblings rather than one + * shared function - the repo's existing shape for this pair (`isChoiceLike` / + * `isCommandLike`, `hasChildChoices` / `hasCommandList`) - so this test is what + * buys the "they cannot disagree" claim their doc comments make. + * + * #1611 was exactly this drift: the choice-side predicate documented the rule the + * command-side one implements, and returned `true` for `""` / `0` / `false`. + */ + +const EVERY_SHAPE: { key: string; value: unknown; lossy: boolean }[] = [ + ...MALFORMED_CHILDREN_SHAPES, + ...MALFORMED_COMMANDS_SHAPES, + // Values neither list carries, because they are not a container shape either + // type produces - but the predicates still have to agree about them. + { key: "emptyArray", value: [], lossy: false }, + { key: "nonEmptyArray", value: [{ id: "a" }], lossy: false }, + { key: "NaN", value: Number.NaN, lossy: false }, + { key: "negativeZero", value: -0, lossy: false }, + { key: "true", value: true, lossy: true }, + { key: "whitespaceString", value: " ", lossy: true }, + { key: "objectWithProto", value: Object.create(null) as object, lossy: false }, +]; + +describe("the two unreadable-container predicates", () => { + it("answer identically for every shape either container shows up in", () => { + for (const shape of EVERY_SHAPE) { + expect( + isUnreadableChoiceList(shape.value), + `choice side: ${shape.key}`, + ).toBe(isUnreadableCommandList(shape.value)); + } + }); + + it("answer what the shared fixture says is lossy", () => { + for (const shape of EVERY_SHAPE) { + expect(isUnreadableChoiceList(shape.value), shape.key).toBe(shape.lossy); + } + }); + + it("is false for an array, which is always readable however wrong its entries", () => { + // The line the whole cluster rests on: a real array is READABLE even when it + // cannot be rendered. Unrenderable entries are repaired at the editor seam + // (#1593, #1608); only a non-array container is "we cannot read this". + expect(isUnreadableChoiceList([null, "stray", { name: "no id" }])).toBe(false); + expect(isUnreadableCommandList([null, "stray", { name: "no id" }])).toBe(false); + }); +}); diff --git a/src/utils/userScriptSecrets.ts b/src/utils/userScriptSecrets.ts index 639bac19c..7211cf176 100644 --- a/src/utils/userScriptSecrets.ts +++ b/src/utils/userScriptSecrets.ts @@ -2,6 +2,7 @@ import type { App } from "obsidian"; import { log } from "../logger/logManager"; import type { IUserScript } from "../types/macros/IUserScript"; import { extractScriptFromMarkdown } from "./extractScriptFromMarkdown"; +import { macroCommandsValueOf } from "./macroUtils"; const USER_SCRIPT_SECRET_PREFIX = "quickadd-user-script"; const SECRET_MARKER = "__quickaddSecret"; @@ -804,10 +805,17 @@ async function clearSecretsFromChoice( let cleared = true; - if (choice.type === "Macro" && isRecord(choice.macro)) { + if (choice.type === "Macro") { + // `macroCommandsValueOf`, not `choice.macro.commands`: an ARRAY-valued `macro` + // IS the command list, and `isRecord` accepts an array, so reading `.commands` + // off it yielded `undefined` and this walked nothing at all. That is the shape + // where a duplicated macro kept the ORIGINAL's literal `secretRef` and editing + // the copy's key overwrote the original's SecretStorage slot (#1609). cleared = - (await clearUserScriptSecretsFromCommands(app, choice.macro.commands)) && - cleared; + (await clearUserScriptSecretsFromCommands( + app, + macroCommandsValueOf(choice.macro), + )) && cleared; } if (choice.type === "Multi" && Array.isArray(choice.choices)) { @@ -855,6 +863,11 @@ export async function clearUserScriptSecretsFromCommands( let cleared = true; for (const command of commands) { + if (Array.isArray(command)) { + cleared = + (await clearUserScriptSecretsFromCommands(app, command)) && cleared; + continue; + } cleared = (await clearUserScriptSecretsFromCommand(app, command)) && cleared; } @@ -927,6 +940,12 @@ export function stripUserScriptSecretRefsFromCommands( if (!Array.isArray(commands)) return; for (const command of commands) { + // A nested ARRAY is a command list, not a command (see normalizeCommandList). + // Missing it leaves the copy holding the original's literal secretRef. + if (Array.isArray(command)) { + stripUserScriptSecretRefsFromCommands(command, options); + continue; + } stripUserScriptSecretRefsFromCommand(command, options); } } @@ -937,8 +956,16 @@ export function stripUserScriptSecretRefsFromChoice( ): void { if (!isRecord(choice)) return; - if (choice.type === "Macro" && isRecord(choice.macro)) { - stripUserScriptSecretRefsFromCommands(choice.macro.commands, options); + if (choice.type === "Macro") { + // `macroCommandsValueOf`, not `choice.macro.commands`: an ARRAY-valued `macro` + // IS the command list, and `isRecord` accepts an array, so reading `.commands` + // off it yielded `undefined` and this walked nothing at all. That is the shape + // where a duplicated macro kept the ORIGINAL's literal `secretRef` and editing + // the copy's key overwrote the original's SecretStorage slot (#1609). + stripUserScriptSecretRefsFromCommands( + macroCommandsValueOf(choice.macro), + options, + ); } if (choice.type === "Multi" && Array.isArray(choice.choices)) {