From ab6b96dd72729ffc11ec8d3c4c0ec9c35ad61ac1 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 21:48:51 +0200 Subject: [PATCH 1/8] fix: an empty folder value no longer reads as "QuickAdd couldn't read this" `hasUnreadableChildren` documented itself as true only for a value that could still be CARRYING choices, and then returned true for `""`, `0` and `false` - which are as empty as the `null` and `{}` it correctly returns false for. Four surfaces read that predicate, so a folder whose `choices` was `""` showed "QuickAdd couldn't read this folder's contents", lost its drop target, and got the scarier delete confirmation - for a value holding nothing at all. The rule the doc describes is already implemented one type over, by `isUnreadableCommandList` (#1593). Rather than a shared module, this splits out the value-level sibling `isUnreadableChoiceList` and keeps the two as the pair the repo already uses for this (`isChoiceLike`/`isCommandLike`, `hasChildChoices`/`hasCommandList`), with a table-driven test over the union of both shape lists as the ratchet that they answer identically. That test is what makes the "these surfaces cannot disagree" claim true rather than aspirational. The `""` / `0` / `false` rows join both shared shape lists, so every resilience test that sweeps the malformed tree now attacks them too. Closes #1611 --- src/utils/choiceUtils.ts | 44 +++++++++------ src/utils/malformedChoices.fixture.ts | 8 +++ src/utils/unreadableValuePredicates.test.ts | 60 +++++++++++++++++++++ 3 files changed, 97 insertions(+), 15 deletions(-) create mode 100644 src/utils/unreadableValuePredicates.test.ts diff --git a/src/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index 29b12608..26cd7bc8 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -94,27 +94,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 `hasChildChoices`/`hasCommandList`). 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); } /** diff --git a/src/utils/malformedChoices.fixture.ts b/src/utils/malformedChoices.fixture.ts index 2910e0a4..dd81149e 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/unreadableValuePredicates.test.ts b/src/utils/unreadableValuePredicates.test.ts new file mode 100644 index 00000000..f99e462b --- /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); + }); +}); From cc56364c45e297f96a56e85e581cb0aacc6962a1 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 21:56:10 +0200 Subject: [PATCH 2/8] fix: migrations stop reporting complete over data they could not read A migration runs once and is then flagged complete forever, so one that walks 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 set. `treeHasUnreadableChildren` existed to prevent exactly that, and asked the wrong question in two directions at once: UNDER-blocking. It looked at `Multi.choices` only. A Capture living inside `"commands": {"0": {"type": "NestedChoice", ...}}` was invisible to `walkAllChoices`, and the four migrations that guard reported complete anyway. `backfillFileOpeningDefaults` and `migrateFileOpeningSettings` had no guard at all, and both translate the legacy `openFileInNewTab` keys that runtime ignores - so a hidden choice loses its "open in new tab" preference for good. OVER-blocking. It returned true for ANY non-array `choices`, including `undefined`, `null` and `{}`, which carry nothing. One folder with no `choices` key therefore kept `removeMacroIndirection` pending forever: legacy macros never embedded, never removed, and every legacy macro choice in the vault dead, because nothing at runtime resolves a `macroId`. The guard is now derived from the walk it has to agree with - `walkSettings` reports the containers it stepped over, and `settingsTreeHasUnreadableData` is that report - so the two cannot drift. `walkChoice` resolves through `macroCommandsValueOf` like every other consumer, which also closes the array-valued `macro` hole where `.commands` reads as `undefined`, walks nothing, and looks perfectly readable. Each migration asks the question its own traversal can answer. The three that recurse `Multi.choices` themselves keep the folders-only question; blocking `removeMacroIndirection` on a `macro.commands` it never descends would be pure cost for the most expensive kind of pending. Two things found while doing this and fixed here because the guard is worthless without them: - `settings.macros` is as untrusted as the rest of data.json, and `settings.macros ?? []` passes `{"0": {...}}` through (not nullish). Three migrations then threw `macros is not iterable` - caught, reverted, and reported with a 15-second "please create an issue" notice on every launch. They now read through `rootMacrosOf` and only write back a real array. - `removeMacroIndirection` holds the only root WRITE among the migrations (`settings.choices.push`). It gets its own `Array.isArray` guard: a readability predicate must never be the only thing between a corrupt value and a TypeError. The two file-opening migrations no longer call `saveSettings()` themselves. migrate.ts already saves once after the whole run, and now that these can stay pending, a per-migration write would be a full data.json rewrite on every launch, straight into Obsidian Sync's whole-file last-write-wins. Closes #1610 --- .../backfillFileOpeningDefaults.test.ts | 7 +- src/migrations/backfillFileOpeningDefaults.ts | 27 +- .../consolidateFileExistsBehavior.ts | 11 +- src/migrations/helpers/choice-traversal.ts | 100 ++++++- ...entFileNameSettingMoveToDefaultBehavior.ts | 17 +- src/migrations/migrateFileOpeningSettings.ts | 31 +- src/migrations/migrationReadability.test.ts | 275 ++++++++++++++++++ ...lusionInsertAfterAndWriteToBottomOfFile.ts | 17 +- src/migrations/removeMacroIndirection.ts | 37 ++- src/utils/choiceUtils.test.ts | 36 ++- src/utils/choiceUtils.ts | 31 +- src/utils/macroUtils.ts | 17 ++ 12 files changed, 562 insertions(+), 44 deletions(-) create mode 100644 src/migrations/migrationReadability.test.ts diff --git a/src/migrations/backfillFileOpeningDefaults.test.ts b/src/migrations/backfillFileOpeningDefaults.test.ts index 5ccb2ac6..b4983a52 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 3d84e2fa..c582f6b8 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,22 @@ 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. + const unreadable = settingsTreeHasUnreadableData(plugin.settings); + + let migratedCount = 0; const backfillFileOpening = (choice: IChoice) => { @@ -67,7 +83,10 @@ const backfillFileOpeningDefaults: Migration = { `File opening defaults backfill complete. Updated ${migratedCount} choice(s).`, ); - await plugin.saveSettings(); + // See migrateFileOpeningSettings: migrate.ts owns the single post-migration + // save, so a per-migration write here is redundant and, while pending, is + // charged on every launch. + if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/consolidateFileExistsBehavior.ts b/src/migrations/consolidateFileExistsBehavior.ts index f2b9398d..b8099c84 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 93260834..471a62f5 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 { @@ -35,23 +46,36 @@ function walkChoice( 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; @@ -91,22 +115,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 47be7edc..5734beb3 100644 --- a/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts +++ b/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts @@ -6,6 +6,7 @@ import { isMultiChoice } from "./helpers/isMultiChoice"; import { isNestedChoiceCommand } from "./helpers/isNestedChoiceCommand"; import type { Migration, MigrationResult } from "./Migrations"; import { treeHasUnreadableChildren } from "src/utils/choiceUtils"; +import { isUnreadableCommandList, rootMacrosOf } from "src/utils/macroUtils"; type OldTemplateChoice = { type?: string; @@ -72,7 +73,12 @@ 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 legacy macros + // separately), so it asks the FOLDERS-ONLY question - blocking it on a + // `macro.commands` it never descends would strand it for nothing (#1610). + 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 +86,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 0d961d42..95b88484 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,21 @@ 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. + const unreadable = settingsTreeHasUnreadableData(plugin.settings); + let migratedCount = 0; @@ -56,9 +72,12 @@ const migrateFileOpeningSettings: Migration = { walkAllChoices(plugin, migrateFileOpening); log.logMessage(`Migration complete. Migrated ${migratedCount} choices.`); - - // Save the updated settings - await plugin.saveSettings(); + + // No saveSettings() here: migrate.ts re-syncs the store and saves once after + // every migration has run. Saving per-migration cost a full data.json write + // each - which, for a vault that stays PENDING, is a write on every single + // launch, straight into Obsidian Sync's whole-file last-write-wins. + if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/migrationReadability.test.ts b/src/migrations/migrationReadability.test.ts new file mode 100644 index 00000000..98da4909 --- /dev/null +++ b/src/migrations/migrationReadability.test.ts @@ -0,0 +1,275 @@ +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 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("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() }), + ]; + + it.each([ + ["backfillFileOpeningDefaults", backfillFileOpeningDefaults], + ["migrateFileOpeningSettings", migrateFileOpeningSettings], + ["consolidateFileExistsBehavior", consolidateFileExistsBehavior], + ])("%s stays pending instead of being flagged complete", async (_name, migration) => { + 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[] }; + expect(settings.choices[0].fileOpening ?? settings.choices[0].type).toBeTruthy(); + }); + + 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("steps over a hole in the legacy macro list", async () => { + const plugin = makePlugin({ + choices: [], + macros: [null, { id: "m", name: "M", commands: [] }], + migrations: {}, + }); + + await expect(removeMacroIndirection.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 61ab50e7..063a77f8 100644 --- a/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts +++ b/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts @@ -7,6 +7,7 @@ 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 { isUnreadableCommandList, rootMacrosOf } from "src/utils/macroUtils"; type SettingsWithLegacyMacros = QuickAdd["settings"] & { macros?: IMacro[] }; @@ -63,17 +64,27 @@ 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 legacy macros + // separately), so it asks the FOLDERS-ONLY question - blocking it on a + // `macro.commands` it never descends would strand it for nothing (#1610). + 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 46b1d625..cc9e68b0 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,9 @@ 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; const referencingChoices = choicesByMacroId.get(macro.id) ?? allChoices.filter( diff --git a/src/utils/choiceUtils.test.ts b/src/utils/choiceUtils.test.ts index afbf7c08..e8fb725d 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 26cd7bc8..f2ebb1ab 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -70,16 +70,35 @@ 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) => { + 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); } /** diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts index 81cd5cc4..92f9a89f 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 From 00ffcbab27b80ee77da36179db610c7f08999f55 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 22:03:54 +0200 Subject: [PATCH 3/8] fix: reordering the choice list no longer deletes choices it could not render ChoiceList filtered the rendered list to entries it could key, and a comment above that filter said it was a "render-time view only. Nothing here rewrites the tree." It was not: the same filtered array 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. For a `null` hole that is harmless - it carries nothing. But the filter also dropped any entry whose id was not a non-empty string, and { "name": "Daily note", "type": "Template", "templatePath": "...", "id": 12 } is a complete, working choice - runnable from the palette, listed by the CLI - whose id was written as a JSON number by a hand-edit, a script or a merge. It was invisible in the settings list, so the user could not miss it, and one reorder deleted it with no prompt and no undo. Verified end to end in a live vault: five choices in data.json, four rows rendered, one ArrowDown and the fifth is gone from disk. Repair rather than hide, mirroring what #1593 shipped for macro commands. `normalizeChoiceList` runs at the ChoiceView seam: an entry that cannot be keyed gets a fresh uuid and is KEPT, so it becomes visible, editable and deletable for the first time; only a hole is dropped. Ids are de-duplicated across the whole tree, since choice ids are globally unique (the command registry keys on `choice:`) unlike command ids. Nothing is persisted by this - the repair reaches disk with the user's first ordinary edit. A repaired id is always a fresh uuid, never `String(oldId)`. Coercing looks tempting because it would keep `quickadd:choice:12` alive, and it is wrong twice: ids are compared with `===` in `getChoice`, so a persisted 12 -> "12" 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 this replaces DELETED the choice, which broke the same reference and lost the choice with it. The seam is memoized on the raw store value, which is load-bearing rather than an optimization: this subscription fires on every settingsStore write, including the AI provider auto-sync a few seconds after launch, and every by-id write in the view resolves its target BEFORE an await - so re-minting inside that window turns `oldChoice.id === newChoice.id` into a no-op and silently discards the user's builder edits. Two more holes closed on the way: - ChoiceList's filter now de-duplicates ids, like CommandList's already did. A duplicate arriving after load reached the keyed {#each} and threw `each_key_duplicate`, and a POST-mount throw escapes mountComponent's try. - An Array entry in a choices list is read as a NESTED LIST and spliced in, the same reading `macroCommandsValueOf` gives an array-valued `macro`. `isChoiceLike([])` is true, so the alternative was spreading it into one nameless, typeless row. Mirrored in `normalizeCommandList` so the twins stay byte-symmetric. Closes #1608 --- src/gui/choiceList/ChoiceList.svelte | 39 ++-- .../choiceList/ChoiceView.malformed.test.ts | 103 ++++++++- src/gui/choiceList/ChoiceView.svelte | 49 +++- src/utils/choiceUtils.ts | 116 ++++++++++ src/utils/macroUtils.ts | 20 ++ src/utils/normalizeChoiceList.test.ts | 209 ++++++++++++++++++ 6 files changed, 511 insertions(+), 25 deletions(-) create mode 100644 src/utils/normalizeChoiceList.test.ts diff --git a/src/gui/choiceList/ChoiceList.svelte b/src/gui/choiceList/ChoiceList.svelte index 2a10dde3..2b84a28b 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 61d0f6e0..b457e38e 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,81 @@ 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("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 99f5d706..b064658a 100644 --- a/src/gui/choiceList/ChoiceView.svelte +++ b/src/gui/choiceList/ChoiceView.svelte @@ -37,6 +37,7 @@ childChoicesOf, hasChildChoices, isChoiceLike, + normalizeChoiceList, } from "../../utils/choiceUtils"; import type { ChoiceListActions } from "./choiceListActions"; import { choiceNoun } from "../../utils/choiceNoun"; @@ -86,12 +87,58 @@ // 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. + // + // MEMOIZED on the raw store value, and that is load-bearing rather than an + // optimization. This subscription fires on EVERY settingsStore write, including + // ones nothing here caused (the AI provider auto-sync lands one a few seconds + // after launch). Re-normalizing an unrepaired array each time would mint a + // fresh uuid each time, and every by-id write in this 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. zustand merges partials, so an unrelated setState leaves + // `state.choices` reference-identical and this memo hits. + let lastRawChoices: unknown; + let lastSeededChoices: IChoice[] = []; + function seedChoices(raw: unknown): 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 as IChoice[]; + if (raw === lastRawChoices) return lastSeededChoices; + const { choices: seeded, repaired } = normalizeChoiceList(raw); + lastRawChoices = raw; + lastSeededChoices = seeded; + // A command was registered at onload under the id data.json held, so a + // repaired choice's palette entry would otherwise point at an id that no + // longer exists. Register the new one. The stale entry survives until the + // next reload — an honest "Choice not found" rather than a silent miss, and + // the alternative was the choice being deleted outright. + for (const { choice } of repaired) { + if (choice.command) commandRegistry.enableCommand(choice); + } + 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/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index f2ebb1ab..946014e7 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -281,6 +281,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.ts b/src/utils/macroUtils.ts index 92f9a89f..bc443344 100644 --- a/src/utils/macroUtils.ts +++ b/src/utils/macroUtils.ts @@ -187,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; diff --git a/src/utils/normalizeChoiceList.test.ts b/src/utils/normalizeChoiceList.test.ts new file mode 100644 index 00000000..f919ac9d --- /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: [], + }); + } + }); +}); From 26fc7a664ab571658c503cf19e20c31f7f7cf415 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 22:08:18 +0200 Subject: [PATCH 4/8] fix: a duplicated macro no longer shares the original's nested ids and secrets `regenerateIds` re-ided the macro and its TOP-LEVEL commands only, so duplicating a macro 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. Nothing crashed, because it is not a within-list collision. But `buildUserScriptSecretId` keys a stored user-script secret on `command.id`, and `migrateUserScriptSecretSettings` re-adopts an existing secret by that id for any option declaring a stable `id` - so a duplicated macro's NESTED UserScript command adopted the ORIGINAL's secret slot, and 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. `regenerateIds` now recurses the whole macro: branch commands, a nested choice (and its macro, or its children if it is a folder). It stays total over whatever data.json holds - a non-array command list is left exactly as found, a hole is stepped over - and resolves the list through `macroCommandsValueOf`, so an array-valued macro is re-ided as the command list it is instead of having a non-index `id` written onto it that JSON.stringify would drop. That last shape turned out to hide the reported symptom from the fix. The secret sanitizers reached `choice.macro.commands` literally behind an `isRecord` gate, and `isRecord` accepts an array - so for `macro: [ ... ]` they walked nothing, the copy kept the original's literal `secretRef`, and editing the copy's key overwrote the original's SecretStorage slot. `stripUserScriptSecretRefsFromChoice`, `clearSecretsFromChoice` and `collectUserScriptPathsFromChoice` now read through `macroCommandsValueOf` like the macro builder does. The regression test asserts the SYMPTOM - the copy carries no secret reference belonging to the original - not just that the ids differ. An ids-only assertion passes while the reported bug still reproduces through the path that never re-ids. Closes #1609 --- src/services/choiceService.ts | 8 +- .../duplicateChoice.nestedIds.test.ts | 217 ++++++++++++++++++ src/utils/macroUtils.ts | 78 ++++++- src/utils/userScriptSecrets.ts | 26 ++- 4 files changed, 312 insertions(+), 17 deletions(-) create mode 100644 src/services/duplicateChoice.nestedIds.test.ts diff --git a/src/services/choiceService.ts b/src/services/choiceService.ts index 731d286a..a6b831bd 100644 --- a/src/services/choiceService.ts +++ b/src/services/choiceService.ts @@ -16,7 +16,7 @@ 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 { macroCommandsValueOf, regenerateIds } from "../utils/macroUtils"; import { childChoicesOf, flattenChoices, @@ -137,8 +137,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); diff --git a/src/services/duplicateChoice.nestedIds.test.ts b/src/services/duplicateChoice.nestedIds.test.ts new file mode 100644 index 00000000..ad7d1bcf --- /dev/null +++ b/src/services/duplicateChoice.nestedIds.test.ts @@ -0,0 +1,217 @@ +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 { createUserScriptSecretRef } from "../utils/userScriptSecrets"; + +/** + * #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", () => { + // The symptom, not the mechanism: an ids-only assertion passes while the + // reported bug still reproduces through a path that never re-ids. + 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("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("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/macroUtils.ts b/src/utils/macroUtils.ts index bc443344..f1018b22 100644 --- a/src/utils/macroUtils.ts +++ b/src/utils/macroUtils.ts @@ -229,17 +229,77 @@ 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[]) { + 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/userScriptSecrets.ts b/src/utils/userScriptSecrets.ts index 639bac19..808a9981 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)) { @@ -937,8 +945,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)) { From 8fc0d157b4e00d0615f39cb4bc3b87598cef901a Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 22:10:43 +0200 Subject: [PATCH 5/8] fix: deleting a macro whose commands could not be read now says so Delete is the one irreversible action in the settings list, and the two container types were treated asymmetrically. A folder whose `choices` value could not be read got a specific warning; a macro got only Deleting this choice will also delete its macro commands. which is true and says nothing about the case that matters - that `macro.commands` holds something QuickAdd could not read (`{"0": {...}}`, a string, a number). Since #1593 the macro builder tells the user about exactly that state and refuses to overwrite it, and then the one screen that was about to destroy it for good stayed quiet. The confirmation now 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` rather than `macro?.commands` is what makes the unreadable-`macro`-object case resolve here exactly as it does there - and it is also why an array-valued `macro` correctly keeps the ordinary line: the array IS the command list, the builder can read and edit it, so there is nothing to warn about. Closes #1612 --- src/services/choiceService.test.ts | 68 ++++++++++++++++++++++++++++++ src/services/choiceService.ts | 26 +++++++++++- 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/services/choiceService.test.ts b/src/services/choiceService.test.ts index a72f72d9..0a3a7251 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 a6b831bd..b94ff9f4 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 { macroCommandsValueOf, regenerateIds } from "../utils/macroUtils"; +import { + isUnreadableCommandList, + macroCommandsValueOf, + regenerateIds, +} from "../utils/macroUtils"; import { childChoicesOf, flattenChoices, @@ -269,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(" "); From d8581136bd5e2f96b866791679612a8c2b23cf49 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 22:53:08 +0200 Subject: [PATCH 6/8] fix: a command can no longer be dragged out of a branch editor into the macro builder `MacroBuilder.configureConditionalBranch` opens the branch editor OVER the still- open macro builder. Both mount a CommandList, and both registered their drop zone with the same svelte-dnd-action type. The library groups zones by type and hit-tests every zone in the group GEOMETRICALLY - a modal backdrop shields nothing - so the builder's list underneath was a live drop target for a drag inside the branch modal, and the two lists overlap almost exactly. Confirmed live: with the Then-branch editor open over a macro, dragging its one command down past the branch list's own bottom edge - still inside the modal - moved it into the macro builder underneath. Both editors then rendered a list that disagreed with the stored macro: the builder showed a command the macro did not have, and the conditional's summary read "Then: 0" for a branch that still had one. Each CommandList now gets its own zone type, so no two command lists are ever in the same group. QuickAdd offers no cross-list command drag - unlike a folder, a command has no "move into" gesture - so a group of one is the honest model, and leaving the zone is then an ordinary "outside any zone" drag: the command springs back. `dropFromOthersDisabled: true` was the first fix and is worse, which is only visible live. 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 QuickAdd's stripShadow (#1244/#883) drops the placeholder the library goes on to measure. The drag ended in an uncaught `Cannot read properties of undefined (reading 'getBoundingClientRect')` with the command gone from the source list: a worse outcome than the bug. Verified after the fix: the same drag leaves both lists and the stored macro untouched, an ordinary reorder within one list still works, and dev:errors is clean for both. Closes #1613 --- src/gui/MacroGUIs/CommandList.svelte | 25 ++++++++++++++++++++++++- src/gui/MacroGUIs/commandZoneId.test.ts | 16 ++++++++++++++++ src/gui/MacroGUIs/commandZoneId.ts | 19 +++++++++++++++++++ src/gui/shared/dndReorder.test.ts | 15 ++++++++++++++- src/gui/shared/dndReorder.ts | 2 +- 5 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 src/gui/MacroGUIs/commandZoneId.test.ts create mode 100644 src/gui/MacroGUIs/commandZoneId.ts diff --git a/src/gui/MacroGUIs/CommandList.svelte b/src/gui/MacroGUIs/CommandList.svelte index 44395a71..eecee824 100644 --- a/src/gui/MacroGUIs/CommandList.svelte +++ b/src/gui/MacroGUIs/CommandList.svelte @@ -3,6 +3,7 @@ import type { ICommand } from "../../types/macros/ICommand"; import { Platform } from "obsidian"; import { alertToScreenReader, type DndEvent, dndzone, SOURCES } from "svelte-dnd-action"; import { baseDndOptions, replaceById, stripShadow } from "../shared/dndReorder"; +import { nextCommandZoneId } from "./commandZoneId"; import { createDragArming } from "../shared/dragArming.svelte"; import { getCommandDisplayName } from "../../utils/macroHelpers"; import { snapshot } from "../svelte/persist.svelte"; @@ -69,6 +70,28 @@ 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. +// +// 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 zoneType = `command:${nextCommandZoneId()}`; + 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 +253,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/commandZoneId.test.ts b/src/gui/MacroGUIs/commandZoneId.test.ts new file mode 100644 index 00000000..20e32a41 --- /dev/null +++ b/src/gui/MacroGUIs/commandZoneId.test.ts @@ -0,0 +1,16 @@ +import { describe, expect, it } from "vitest"; +import { nextCommandZoneId } from "./commandZoneId"; + +/** + * #1613. Two CommandLists sharing a svelte-dnd-action zone `type` are mutually + * valid drop targets, because the library hit-tests every zone in a type group + * geometrically - a modal backdrop shields nothing. The conditional-branch editor + * opens on top of the still-open macro builder, so a command dragged a little too + * far down inside the branch editor landed in the builder underneath. + */ +describe("nextCommandZoneId", () => { + it("never hands out the same id twice", () => { + const ids = Array.from({ length: 50 }, () => nextCommandZoneId()); + expect(new Set(ids).size).toBe(ids.length); + }); +}); diff --git a/src/gui/MacroGUIs/commandZoneId.ts b/src/gui/MacroGUIs/commandZoneId.ts new file mode 100644 index 00000000..a2d12539 --- /dev/null +++ b/src/gui/MacroGUIs/commandZoneId.ts @@ -0,0 +1,19 @@ +let counter = 0; + +/** + * A process-unique suffix for a CommandList's svelte-dnd-action zone type. + * + * svelte-dnd-action groups drop zones by `type` and hit-tests every zone in the + * group geometrically, so two command lists sharing a type are mutually valid + * drop targets even when one is a modal stacked on top of the other. QuickAdd + * offers no cross-list command drag - a command has no "move into" gesture the + * way a folder does - so every list gets its own group of exactly one (#1613). + * + * A module-level counter rather than a uuid: it only has to be unique among the + * zones alive in this window, and a stable, readable `command:3` is easier to + * recognise in a DOM dump than a uuid. + */ +export function nextCommandZoneId(): number { + counter += 1; + return counter; +} diff --git a/src/gui/shared/dndReorder.test.ts b/src/gui/shared/dndReorder.test.ts index cefec901..b4e1b091 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 703f7c07..eb478520 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). */ From 48ed5a1501460f3992cbe433981329d9d659a0d9 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 23:33:23 +0200 Subject: [PATCH 7/8] fix: hold the id repair across a settings re-mount, and make the guards total Follow-ups from an adversarial review of the six fixes in this branch. Each of these was a real defect in them, found by probe rather than by reading. **The id repair leaked a dead palette entry per settings open.** The memo lived in ChoiceView, and the settings tab DESTROYS and re-mounts the view every time it is opened - so an unrepaired `{"id": 12, "command": true}` minted a fresh uuid and registered another command on every open, with nothing unregistering. It now lives in a module-level WeakMap keyed on the raw array (`seedChoiceTree`), so the cache outlives the component and dies with the array. Verified live: three opens, one stable id, two palette entries (the original id from data.json plus the one repair) rather than four. Keying that map 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. The registration loop is also wrapped now. It runs during component setup, so an uncaught throw escaped mountComponent and would have swapped the whole choice list for the error card - one failure must cost one command, the same argument main.ts makes around `addCommandForChoice`. **A hole in `settings.macros` still crashed two migrations.** `[null, {...}]` IS an array, so it reads as readable and reaches the loop, where `macro.commands` throws. Both now read through `commandListOf(macroCommandsValueOf(macro))`, which steps over the hole AND gives an array-valued legacy macro the same reading every other consumer gives it. **The traversal and the editor seam disagreed about a nested array.** `typeof [] === "object"`, so `walkChoice` handed the array to the visitor and descended nothing, reporting READABLE - while `normalizeChoiceList` splices such an entry's members into the tree. A migration could be flagged complete over a choice the settings tab then made real. The walk now splices too. **Two tests asserted nothing.** `expect(c.fileOpening ?? c.type).toBeTruthy()` falls back to the string "Template", so all three migration rows passed with their visitor bodies deleted; each row now carries its own effect assertion, and gutting any visitor fails its row. And #1613 shipped with no regression coverage at all - reverting the zone type left the full suite green. `CommandList.zoneType.test.ts` wraps svelte-dnd-action's action to record the type each list registers, which is the only observable seam (the library writes no attribute); it fails with `expected [ 'command', 'command' ] to not include 'command'` on the reverted tree. **`commandZoneId.ts` is gone.** Svelte's own `$props.id()` is a runtime-wide uid counter and is already used in-tree by LabeledField - a module, an export and a counter for that was 35 lines of nothing. Also: the direct #1609 symptom is now asserted through `buildUserScriptSecretId` (the copy's nested script resolves to a different secret slot), the comment claiming the two legacy-macro migrations' guards match their traversals is corrected to say they are knowingly narrower and why widening is inert, and the `normalizeCommandList` array branch gets the coverage its twin already had. --- src/gui/MacroGUIs/CommandList.svelte | 7 +- .../MacroGUIs/CommandList.zoneType.test.ts | 72 ++++++++++ src/gui/MacroGUIs/commandZoneId.test.ts | 16 --- src/gui/MacroGUIs/commandZoneId.ts | 19 --- .../choiceList/ChoiceView.malformed.test.ts | 31 +++++ src/gui/choiceList/ChoiceView.svelte | 55 ++++---- src/gui/choiceList/seedChoiceTree.ts | 54 ++++++++ src/migrations/backfillFileOpeningDefaults.ts | 9 +- src/migrations/helpers/choice-traversal.ts | 14 ++ ...entFileNameSettingMoveToDefaultBehavior.ts | 34 ++++- src/migrations/migrateFileOpeningSettings.ts | 9 +- src/migrations/migrationReadability.test.ts | 127 +++++++++++++++--- ...lusionInsertAfterAndWriteToBottomOfFile.ts | 34 ++++- .../duplicateChoice.nestedIds.test.ts | 36 ++++- src/utils/choiceUtils.ts | 2 +- src/utils/macroUtils.test.ts | 33 +++++ 16 files changed, 445 insertions(+), 107 deletions(-) create mode 100644 src/gui/MacroGUIs/CommandList.zoneType.test.ts delete mode 100644 src/gui/MacroGUIs/commandZoneId.test.ts delete mode 100644 src/gui/MacroGUIs/commandZoneId.ts create mode 100644 src/gui/choiceList/seedChoiceTree.ts diff --git a/src/gui/MacroGUIs/CommandList.svelte b/src/gui/MacroGUIs/CommandList.svelte index eecee824..00300ed0 100644 --- a/src/gui/MacroGUIs/CommandList.svelte +++ b/src/gui/MacroGUIs/CommandList.svelte @@ -3,7 +3,6 @@ import type { ICommand } from "../../types/macros/ICommand"; import { Platform } from "obsidian"; import { alertToScreenReader, type DndEvent, dndzone, SOURCES } from "svelte-dnd-action"; import { baseDndOptions, replaceById, stripShadow } from "../shared/dndReorder"; -import { nextCommandZoneId } from "./commandZoneId"; import { createDragArming } from "../shared/dragArming.svelte"; import { getCommandDisplayName } from "../../utils/macroHelpers"; import { snapshot } from "../svelte/persist.svelte"; @@ -80,6 +79,9 @@ const renderable = $derived.by(() => { // 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 @@ -90,7 +92,8 @@ const renderable = $derived.by(() => { // 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 zoneType = `command:${nextCommandZoneId()}`; +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 diff --git a/src/gui/MacroGUIs/CommandList.zoneType.test.ts b/src/gui/MacroGUIs/CommandList.zoneType.test.ts new file mode 100644 index 00000000..757b5d4c --- /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/MacroGUIs/commandZoneId.test.ts b/src/gui/MacroGUIs/commandZoneId.test.ts deleted file mode 100644 index 20e32a41..00000000 --- a/src/gui/MacroGUIs/commandZoneId.test.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { nextCommandZoneId } from "./commandZoneId"; - -/** - * #1613. Two CommandLists sharing a svelte-dnd-action zone `type` are mutually - * valid drop targets, because the library hit-tests every zone in a type group - * geometrically - a modal backdrop shields nothing. The conditional-branch editor - * opens on top of the still-open macro builder, so a command dragged a little too - * far down inside the branch editor landed in the builder underneath. - */ -describe("nextCommandZoneId", () => { - it("never hands out the same id twice", () => { - const ids = Array.from({ length: 50 }, () => nextCommandZoneId()); - expect(new Set(ids).size).toBe(ids.length); - }); -}); diff --git a/src/gui/MacroGUIs/commandZoneId.ts b/src/gui/MacroGUIs/commandZoneId.ts deleted file mode 100644 index a2d12539..00000000 --- a/src/gui/MacroGUIs/commandZoneId.ts +++ /dev/null @@ -1,19 +0,0 @@ -let counter = 0; - -/** - * A process-unique suffix for a CommandList's svelte-dnd-action zone type. - * - * svelte-dnd-action groups drop zones by `type` and hit-tests every zone in the - * group geometrically, so two command lists sharing a type are mutually valid - * drop targets even when one is a modal stacked on top of the other. QuickAdd - * offers no cross-list command drag - a command has no "move into" gesture the - * way a folder does - so every list gets its own group of exactly one (#1613). - * - * A module-level counter rather than a uuid: it only has to be unique among the - * zones alive in this window, and a stable, readable `command:3` is easier to - * recognise in a DOM dump than a uuid. - */ -export function nextCommandZoneId(): number { - counter += 1; - return counter; -} diff --git a/src/gui/choiceList/ChoiceView.malformed.test.ts b/src/gui/choiceList/ChoiceView.malformed.test.ts index b457e38e..54f5fecd 100644 --- a/src/gui/choiceList/ChoiceView.malformed.test.ts +++ b/src/gui/choiceList/ChoiceView.malformed.test.ts @@ -254,6 +254,37 @@ describe("ChoiceView over a malformed tree (#1566)", () => { 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 b064658a..c4d04d13 100644 --- a/src/gui/choiceList/ChoiceView.svelte +++ b/src/gui/choiceList/ChoiceView.svelte @@ -37,8 +37,8 @@ childChoicesOf, hasChildChoices, isChoiceLike, - normalizeChoiceList, } from "../../utils/choiceUtils"; + import { hasSeeded, seedChoiceTree } from "./seedChoiceTree"; import type { ChoiceListActions } from "./choiceListActions"; import { choiceNoun } from "../../utils/choiceNoun"; import { reportingHandler } from "../../utils/errorUtils"; @@ -99,34 +99,37 @@ // 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. - // - // MEMOIZED on the raw store value, and that is load-bearing rather than an - // optimization. This subscription fires on EVERY settingsStore write, including - // ones nothing here caused (the AI provider auto-sync lands one a few seconds - // after launch). Re-normalizing an unrepaired array each time would mint a - // fresh uuid each time, and every by-id write in this 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. zustand merges partials, so an unrelated setState leaves - // `state.choices` reference-identical and this memo hits. - let lastRawChoices: unknown; - let lastSeededChoices: IChoice[] = []; - function seedChoices(raw: unknown): IChoice[] { + // 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 as IChoice[]; - if (raw === lastRawChoices) return lastSeededChoices; - const { choices: seeded, repaired } = normalizeChoiceList(raw); - lastRawChoices = raw; - lastSeededChoices = seeded; - // A command was registered at onload under the id data.json held, so a - // repaired choice's palette entry would otherwise point at an id that no - // longer exists. Register the new one. The stale entry survives until the - // next reload — an honest "Choice not found" rather than a silent miss, and - // the alternative was the choice being deleted outright. + 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) commandRegistry.enableCommand(choice); + 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; } diff --git a/src/gui/choiceList/seedChoiceTree.ts b/src/gui/choiceList/seedChoiceTree.ts new file mode 100644 index 00000000..a5cf766b --- /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/migrations/backfillFileOpeningDefaults.ts b/src/migrations/backfillFileOpeningDefaults.ts index c582f6b8..77b6d0f7 100644 --- a/src/migrations/backfillFileOpeningDefaults.ts +++ b/src/migrations/backfillFileOpeningDefaults.ts @@ -32,9 +32,13 @@ const backfillFileOpeningDefaults: Migration = { // 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) => { @@ -83,9 +87,6 @@ const backfillFileOpeningDefaults: Migration = { `File opening defaults backfill complete. Updated ${migratedCount} choice(s).`, ); - // See migrateFileOpeningSettings: migrate.ts owns the single post-migration - // save, so a per-migration write here is redundant and, while pending, is - // charged on every launch. if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/helpers/choice-traversal.ts b/src/migrations/helpers/choice-traversal.ts index 471a62f5..02a3f32c 100644 --- a/src/migrations/helpers/choice-traversal.ts +++ b/src/migrations/helpers/choice-traversal.ts @@ -41,6 +41,15 @@ function walkChoice( visited: Set, ): void { if (!choice || typeof choice !== "object") return; + // An ARRAY entry is a NESTED LIST, not a choice - the same reading + // `normalizeChoiceList` gives it at the editor seam, and the two have to agree + // or this walk reports "readable" for a subtree the settings tab will happily + // splice back into the tree (#1608/#1610). `typeof [] === "object"`, so + // without this the visitor is handed the array itself and descends nothing. + if (Array.isArray(choice)) { + for (const child of choice) walkChoice(child, visitors, visited); + return; + } if (visited.has(choice)) return; visited.add(choice); @@ -79,6 +88,11 @@ function walkCommands( for (const command of commands) { if (!command || typeof command !== "object") continue; + // Same reading as `normalizeCommandList`: a nested list is spliced in. + if (Array.isArray(command)) { + walkCommands(command, visitors, visited); + continue; + } visitors.onCommand?.(command); diff --git a/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts b/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts index 5734beb3..ca3bf387 100644 --- a/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts +++ b/src/migrations/incrementFileNameSettingMoveToDefaultBehavior.ts @@ -6,7 +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 { isUnreadableCommandList, rootMacrosOf } from "src/utils/macroUtils"; +import { + commandListOf, + isUnreadableCommandList, + macroCommandsValueOf, + rootMacrosOf, +} from "src/utils/macroUtils"; type OldTemplateChoice = { type?: string; @@ -45,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) @@ -73,9 +82,20 @@ 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. - // This migration recurses `Multi.choices` itself (and legacy macros - // separately), so it asks the FOLDERS-ONLY question - blocking it on a - // `macro.commands` it never descends would strand it for nothing (#1610). + // 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); diff --git a/src/migrations/migrateFileOpeningSettings.ts b/src/migrations/migrateFileOpeningSettings.ts index 95b88484..f64b585d 100644 --- a/src/migrations/migrateFileOpeningSettings.ts +++ b/src/migrations/migrateFileOpeningSettings.ts @@ -33,6 +33,11 @@ const migrateFileOpeningSettings: Migration = { // 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); @@ -73,10 +78,6 @@ const migrateFileOpeningSettings: Migration = { log.logMessage(`Migration complete. Migrated ${migratedCount} choices.`); - // No saveSettings() here: migrate.ts re-syncs the store and saves once after - // every migration has run. Saving per-migration cost a full data.json write - // each - which, for a vault that stays PENDING, is a write on every single - // launch, straight into Obsidian Sync's whole-file last-write-wins. if (unreadable) return { complete: false }; }, }; diff --git a/src/migrations/migrationReadability.test.ts b/src/migrations/migrationReadability.test.ts index 98da4909..ffe8691e 100644 --- a/src/migrations/migrationReadability.test.ts +++ b/src/migrations/migrationReadability.test.ts @@ -93,6 +93,35 @@ describe("settingsTreeHasUnreadableData", () => { }); }); + it("reads a nested ARRAY as a list, the same way the editor seam does", () => { + // `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(false); + + void backfillFileOpeningDefaults.migrate(plugin); + + const nested = ( + (plugin.settings as unknown as { choices: Record[][] }) + .choices[0] + )[0]; + expect(nested.fileOpening).toEqual({ + location: "split", + direction: "horizontal", + mode: "default", + focus: false, + }); + }); + it("sees an unreadable conditional branch", () => { expect( settingsTreeHasUnreadableData({ @@ -155,24 +184,54 @@ describe("the guarded migrations over an unreadable macro.commands", () => { 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], - ["migrateFileOpeningSettings", migrateFileOpeningSettings], - ["consolidateFileExistsBehavior", consolidateFileExistsBehavior], - ])("%s stays pending instead of being flagged complete", async (_name, migration) => { - const plugin = makePlugin({ - choices: treeWithHiddenChoice(), - migrations: {}, - }); + [ + "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); + 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[] }; - expect(settings.choices[0].fileOpening ?? settings.choices[0].type).toBeTruthy(); - }); + 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: {} }); @@ -238,14 +297,46 @@ describe("an untrusted settings.macros", () => { ); }); - it("steps over a hole in the legacy macro list", async () => { + 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: [null, { id: "m", name: "M", commands: [] }], + macros: [[nested()]], migrations: {}, }); - await expect(removeMacroIndirection.migrate(plugin)).resolves.toBeUndefined(); + await expect(migration.migrate(plugin)).resolves.toBeUndefined(); }); }); diff --git a/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts b/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts index 063a77f8..9319c7e5 100644 --- a/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts +++ b/src/migrations/mutualExclusionInsertAfterAndWriteToBottomOfFile.ts @@ -7,7 +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 { isUnreadableCommandList, rootMacrosOf } from "src/utils/macroUtils"; +import { + commandListOf, + isUnreadableCommandList, + macroCommandsValueOf, + rootMacrosOf, +} from "src/utils/macroUtils"; type SettingsWithLegacyMacros = QuickAdd["settings"] & { macros?: IMacro[] }; @@ -31,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) @@ -64,9 +73,20 @@ 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. - // This migration recurses `Multi.choices` itself (and legacy macros - // separately), so it asks the FOLDERS-ONLY question - blocking it on a - // `macro.commands` it never descends would strand it for nothing (#1610). + // 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); diff --git a/src/services/duplicateChoice.nestedIds.test.ts b/src/services/duplicateChoice.nestedIds.test.ts index ad7d1bcf..6fe3d218 100644 --- a/src/services/duplicateChoice.nestedIds.test.ts +++ b/src/services/duplicateChoice.nestedIds.test.ts @@ -4,7 +4,11 @@ vi.mock("obsidian-dataview", () => ({ getAPI: vi.fn() })); import { duplicateChoice } from "./choiceService"; import type IChoice from "../types/choices/IChoice"; -import { createUserScriptSecretRef } from "../utils/userScriptSecrets"; +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 @@ -119,8 +123,8 @@ describe("duplicating a macro", () => { }); it("carries no secret reference belonging to the original", () => { - // The symptom, not the mechanism: an ids-only assertion passes while the - // reported bug still reproduces through a path that never re-ids. + // 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", @@ -133,6 +137,32 @@ describe("duplicating a macro", () => { 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 diff --git a/src/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index 946014e7..0f475c0a 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -122,7 +122,7 @@ export function rootChoicesOf(value: unknown): IChoice[] { * * The sibling of `isUnreadableCommandList` in macroUtils.ts, deliberately kept * as a sibling rather than a shared module (the same shape as - * `isChoiceLike`/`isCommandLike` and `hasChildChoices`/`hasCommandList`). The + * `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. * diff --git a/src/utils/macroUtils.test.ts b/src/utils/macroUtils.test.ts index e9d1ff51..1607e0ba 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")]; From 08c72011efea86c244b6935ad1daf855ea830ef4 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Mon, 27 Jul 2026 23:49:49 +0200 Subject: [PATCH 8/8] fix: a nested array in a list is data, and every guard now agrees about it Review follow-ups on this branch. `normalizeCommandList`/`normalizeChoiceList` read a nested ARRAY entry as a LIST and splice its members in - `isChoiceLike([])` is true, so the alternative is spreading it into one nameless row - but nothing else in the tree agreed, and the disagreement was reachable in two ways. Duplication treated a nested array as a COMMAND: `regenerateCommandIds` wrote an `id` onto it that JSON.stringify drops, and the secret walkers stepped past it - so every id and every literal `secretRef` inside it stayed shared with the original, which is #1609's symptom surviving #1609's fix. All three now recurse. The migration guards said "readable" for a nested array in `choices`, and the narrow migrations cannot descend one: `flattenChoices` pushes the array as if it WERE a choice, and the increment / mutual-exclusion recursions step straight past it. So `removeMacroIndirection` could classify a macro referenced from inside one as orphaned, duplicate it at the root and delete `settings.macros`, permanently. Both guards now REPORT a nested array rather than descending it - deliberately, and against the first fix here, which made the shared walk descend it. That walk can; the traversals the guard answers for cannot, and a guard that says "I saw everything" on behalf of a walker that did not is precisely the failure #1610 is about. The cost of the conservative answer is one launch: the user opens the settings tab, the seam splices the entries into real choices, and the next launch reads clean. Also from review: an entry in `settings.macros` with no usable `id` is no longer rehomed - the lookup missed, the orphan branch ran, and a nameless MacroChoice was pushed into the choice tree and made permanent when `macros` was deleted. And two test fixes - a `void`ed migration that only passed because the mutation happened before the first await, and two `it.each` rows that were byte-identical. --- src/migrations/helpers/choice-traversal.ts | 24 ++++++++++----- src/migrations/migrationReadability.test.ts | 29 +++++++++---------- src/migrations/removeMacroIndirection.ts | 4 +++ .../duplicateChoice.nestedIds.test.ts | 21 ++++++++++++++ src/utils/choiceUtils.ts | 7 +++++ src/utils/macroUtils.ts | 7 +++++ src/utils/userScriptSecrets.ts | 11 +++++++ 7 files changed, 80 insertions(+), 23 deletions(-) diff --git a/src/migrations/helpers/choice-traversal.ts b/src/migrations/helpers/choice-traversal.ts index 02a3f32c..3c002401 100644 --- a/src/migrations/helpers/choice-traversal.ts +++ b/src/migrations/helpers/choice-traversal.ts @@ -41,13 +41,20 @@ function walkChoice( visited: Set, ): void { if (!choice || typeof choice !== "object") return; - // An ARRAY entry is a NESTED LIST, not a choice - the same reading - // `normalizeChoiceList` gives it at the editor seam, and the two have to agree - // or this walk reports "readable" for a subtree the settings tab will happily - // splice back into the tree (#1608/#1610). `typeof [] === "object"`, so - // without this the visitor is handed the array itself and descends nothing. + // 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)) { - for (const child of choice) walkChoice(child, visitors, visited); + visitors.onUnreadable?.(); return; } if (visited.has(choice)) return; @@ -88,9 +95,10 @@ function walkCommands( for (const command of commands) { if (!command || typeof command !== "object") continue; - // Same reading as `normalizeCommandList`: a nested list is spliced in. + // 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)) { - walkCommands(command, visitors, visited); + visitors.onUnreadable?.(); continue; } diff --git a/src/migrations/migrationReadability.test.ts b/src/migrations/migrationReadability.test.ts index ffe8691e..d7e38657 100644 --- a/src/migrations/migrationReadability.test.ts +++ b/src/migrations/migrationReadability.test.ts @@ -2,6 +2,7 @@ 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"; @@ -93,7 +94,7 @@ describe("settingsTreeHasUnreadableData", () => { }); }); - it("reads a nested ARRAY as a list, the same way the editor seam does", () => { + 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 @@ -106,20 +107,18 @@ describe("settingsTreeHasUnreadableData", () => { expect( settingsTreeHasUnreadableData({ choices: [[hiddenCapture()]] as never }), - ).toBe(false); - - void backfillFileOpeningDefaults.migrate(plugin); - - const nested = ( - (plugin.settings as unknown as { choices: Record[][] }) - .choices[0] - )[0]; - expect(nested.fileOpening).toEqual({ - location: "split", - direction: "horizontal", - mode: "default", - focus: false, - }); + ).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", () => { diff --git a/src/migrations/removeMacroIndirection.ts b/src/migrations/removeMacroIndirection.ts index cc9e68b0..21ff7a6a 100644 --- a/src/migrations/removeMacroIndirection.ts +++ b/src/migrations/removeMacroIndirection.ts @@ -96,6 +96,10 @@ const removeMacroIndirection: Migration = { // 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/duplicateChoice.nestedIds.test.ts b/src/services/duplicateChoice.nestedIds.test.ts index 6fe3d218..e5798f28 100644 --- a/src/services/duplicateChoice.nestedIds.test.ts +++ b/src/services/duplicateChoice.nestedIds.test.ts @@ -178,6 +178,27 @@ describe("duplicating a macro", () => { ).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 []. diff --git a/src/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index 0f475c0a..de63795d 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -92,6 +92,13 @@ export function treeHasUnreadableChildren(choices: unknown): boolean { if (!Array.isArray(choices)) return true; 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; diff --git a/src/utils/macroUtils.ts b/src/utils/macroUtils.ts index f1018b22..a9eb2676 100644 --- a/src/utils/macroUtils.ts +++ b/src/utils/macroUtils.ts @@ -273,6 +273,13 @@ function regenerateMacroIds(macro: unknown, visited: Set): void { 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); diff --git a/src/utils/userScriptSecrets.ts b/src/utils/userScriptSecrets.ts index 808a9981..7211cf17 100644 --- a/src/utils/userScriptSecrets.ts +++ b/src/utils/userScriptSecrets.ts @@ -863,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; } @@ -935,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); } }