From f02f23e35e90fbbea623af6e66b941e9e6af4a34 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 18 Oct 2026 21:44:21 +0200 Subject: [PATCH 1/5] refactor(cli): move the CLI package import into a shared service The CLI imported a package without the modal: it analysed it, decided every choice and file, applied the import, stored the choices, synced their commands and saved. That body moves to importPackageUnattended, which takes the decisions as a function of whether each choice or file already exists. The CLI keeps its flags, its trust gate and its report, and imports stay queued. --- src/cli/importPackageCli.ts | 124 ++--------------------- src/services/packageImportUnattended.ts | 126 ++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 114 deletions(-) create mode 100644 src/services/packageImportUnattended.ts diff --git a/src/cli/importPackageCli.ts b/src/cli/importPackageCli.ts index 0099df8a1..b74fb81a6 100644 --- a/src/cli/importPackageCli.ts +++ b/src/cli/importPackageCli.ts @@ -1,29 +1,13 @@ import type { CliData, CliFlags } from "obsidian"; import type QuickAdd from "../main"; import { settingsStore } from "../settingsStore"; -import type { AssetConflict } from "../gui/PackageManager/importDecisions"; -import { - defaultAssetDestinationFor, - effectiveChoiceMode, - reconcileMode, -} from "../gui/PackageManager/importDecisions"; -import { syncImportedChoiceCommands } from "../services/packageImportCommands"; -import type { - AssetImportDecision, - AssetImportMode, - ChoiceImportDecision, - ChoiceImportMode, -} from "../services/packageImportService"; -import { - analysePackage, - analysePackagePreview, - applyPackageImport, - readQuickAddPackage, -} from "../services/packageImportService"; +import { effectiveChoiceMode, reconcileMode } from "../gui/PackageManager/importDecisions"; +import type { AssetImportMode, ChoiceImportMode } from "../services/packageImportService"; +import { analysePackagePreview, readQuickAddPackage } from "../services/packageImportService"; +import { importPackageUnattended } from "../services/packageImportUnattended"; import { requiresAcknowledgement } from "../services/packagePreview"; import type IChoice from "../types/choices/IChoice"; import { flattenChoices } from "../utils/choiceUtils"; -import { escapesVaultBoundary } from "../utils/vaultPathBoundary"; import { isTruthy } from "./params"; export const PACKAGE_IMPORT_COMMAND = "quickadd:package-import"; @@ -64,43 +48,6 @@ function parseMode( return value as T; } -/** - * Where each bundled file lands when nobody edits the destination. The modal - * lets the reader rename a colliding file; the CLI cannot, so two templates - * that share a file name (`One/Meeting.md`, `Two/Meeting.md`) keep their - * original paths instead of both claiming `Templates/Meeting.md` and having - * the whole import refused. - */ -function cliAssetDestinations( - conflicts: ReadonlyArray>, - templateFolderPaths: unknown, -): Map { - const defaults = new Map( - conflicts.map((conflict) => [ - conflict.originalPath, - defaultAssetDestinationFor(conflict, templateFolderPaths), - ]), - ); - // Fold case unconditionally: on a case-sensitive vault this only sends an - // extra file back to its original path, which is always a valid choice. - const claims = new Map(); - for (const destination of defaults.values()) { - const key = destination.toLowerCase(); - claims.set(key, (claims.get(key) ?? 0) + 1); - } - for (const [originalPath, destination] of defaults) { - if ((claims.get(destination.toLowerCase()) ?? 0) > 1) { - defaults.set(originalPath, originalPath); - } - } - return defaults; -} - -// Imports read the choices, analyse, write files, then store the merged -// choices. Two overlapping runs would each merge into the snapshot they took -// and the later store would drop the earlier import, so runs are queued. -let importQueue: Promise = Promise.resolve(); - /** * CLI seam for the import modal: same analysis, same trust gate, same default * decisions (existing ids are overwritten, templates land in the template @@ -108,17 +55,7 @@ let importQueue: Promise = Promise.resolve(); * end to end without driving the modal — the docs' example packages are * verified this way. */ -export function importPackageHandler( - plugin: QuickAdd, - params: CliData, -): Promise<{ ok: boolean; [key: string]: unknown }> { - const run = importQueue.then(() => importPackage(plugin, params)); - // A failed import must not poison the queue for the next one. - importQueue = run.catch(() => undefined); - return run; -} - -async function importPackage( +export async function importPackageHandler( plugin: QuickAdd, params: CliData, ): Promise<{ ok: boolean; [key: string]: unknown }> { @@ -131,9 +68,7 @@ async function importPackage( const { app } = plugin; const { pkg } = await readQuickAddPackage(app, path); - const previousChoices = settingsStore.getState().choices; - const analysis = await analysePackage(app, previousChoices, pkg); - const preview = await analysePackagePreview(app, previousChoices, pkg); + const preview = await analysePackagePreview(app, settingsStore.getState().choices, pkg); if (requiresAcknowledgement(preview) && !isTruthy(params.acknowledge)) { return { @@ -147,51 +82,12 @@ async function importPackage( }; } - const choiceDecisions: ChoiceImportDecision[] = analysis.choiceConflicts.map( - (conflict) => ({ - choiceId: conflict.choiceId, - mode: effectiveChoiceMode( - choiceMode ?? (conflict.exists ? "overwrite" : "import"), - conflict.exists, - ), - }), - ); - - const destinations = cliAssetDestinations( - analysis.assetConflicts, - settingsStore.getState().templateFolderPaths, - ); - const assetDecisions: AssetImportDecision[] = []; - for (const conflict of analysis.assetConflicts) { - const destinationPath = - destinations.get(conflict.originalPath) ?? conflict.originalPath; - // applyPackageImport rejects a destination outside the vault; do not - // stat it first (the package, and so this path, is untrusted input). - const exists = - destinationPath === conflict.originalPath - ? conflict.exists - : !escapesVaultBoundary(destinationPath) && - (await app.vault.adapter.exists(destinationPath)); - assetDecisions.push({ - originalPath: conflict.originalPath, - destinationPath, - mode: reconcileMode(assetMode ?? (exists ? "overwrite" : "write"), exists), - }); - } - - const result = await applyPackageImport({ - app, - existingChoices: previousChoices, - aiProviders: settingsStore.getState().ai.providers, - pkg, - choiceDecisions, - assetDecisions, + const result = await importPackageUnattended(plugin, pkg, { + choice: (exists) => + effectiveChoiceMode(choiceMode ?? (exists ? "overwrite" : "import"), exists), + asset: (exists) => reconcileMode(assetMode ?? (exists ? "overwrite" : "write"), exists), }); - settingsStore.setState((state) => ({ ...state, choices: result.updatedChoices })); - syncImportedChoiceCommands(plugin, previousChoices, result); - await plugin.saveSettings(); - // Report final ids: a duplicated choice gets a fresh id during apply. const updatedById = new Map( flattenChoices(result.updatedChoices).map((choice) => [choice.id, choice]), diff --git a/src/services/packageImportUnattended.ts b/src/services/packageImportUnattended.ts new file mode 100644 index 000000000..0ebf57638 --- /dev/null +++ b/src/services/packageImportUnattended.ts @@ -0,0 +1,126 @@ +import type QuickAdd from "../main"; +import { settingsStore } from "../settingsStore"; +import type { AssetConflict } from "../gui/PackageManager/importDecisions"; +import { defaultAssetDestinationFor } from "../gui/PackageManager/importDecisions"; +import type { QuickAddPackage } from "../types/packages/QuickAddPackage"; +import { escapesVaultBoundary } from "../utils/vaultPathBoundary"; +import { syncImportedChoiceCommands } from "./packageImportCommands"; +import type { + ApplyImportResult, + AssetImportDecision, + AssetImportMode, + ChoiceImportDecision, + ChoiceImportMode, +} from "./packageImportService"; +import { analysePackage, applyPackageImport } from "./packageImportService"; + +/** What an import with nobody to ask does with each choice and file, by whether it is already in the vault. */ +export interface UnattendedImportModes { + choice: (exists: boolean) => ChoiceImportMode; + asset: (exists: boolean) => AssetImportMode; +} + +/** + * Where each bundled file lands when nobody edits the destination. The modal + * lets the reader rename a colliding file; an unattended import cannot, so two + * templates that share a file name (`One/Meeting.md`, `Two/Meeting.md`) keep + * their original paths instead of both claiming `Templates/Meeting.md` and + * having the whole import refused. + */ +function defaultAssetDestinations( + conflicts: ReadonlyArray>, + templateFolderPaths: unknown, +): Map { + const defaults = new Map( + conflicts.map((conflict) => [ + conflict.originalPath, + defaultAssetDestinationFor(conflict, templateFolderPaths), + ]), + ); + // Fold case unconditionally: on a case-sensitive vault this only sends an + // extra file back to its original path, which is always a valid choice. + const claims = new Map(); + for (const destination of defaults.values()) { + const key = destination.toLowerCase(); + claims.set(key, (claims.get(key) ?? 0) + 1); + } + for (const [originalPath, destination] of defaults) { + if ((claims.get(destination.toLowerCase()) ?? 0) > 1) { + defaults.set(originalPath, originalPath); + } + } + return defaults; +} + +// Imports read the choices, analyse, write files, then store the merged +// choices. Two overlapping runs would each merge into the snapshot they took +// and the later store would drop the earlier import, so runs are queued. +let importQueue: Promise = Promise.resolve(); + +/** + * The import modal's apply step with every decision made up front: the same + * analysis, the same default destinations (templates land in the template + * folder) and the same apply, then the choices are stored, their commands + * synced and the settings saved. The trust gate is the caller's: check + * `requiresAcknowledgement` first. + */ +export function importPackageUnattended( + plugin: QuickAdd, + pkg: QuickAddPackage, + modes: UnattendedImportModes, +): Promise { + const run = importQueue.then(() => importNow(plugin, pkg, modes)); + // A failed import must not poison the queue for the next one. + importQueue = run.catch(() => undefined); + return run; +} + +async function importNow( + plugin: QuickAdd, + pkg: QuickAddPackage, + modes: UnattendedImportModes, +): Promise { + const { app } = plugin; + const previousChoices = settingsStore.getState().choices; + const analysis = await analysePackage(app, previousChoices, pkg); + + const choiceDecisions: ChoiceImportDecision[] = analysis.choiceConflicts.map( + (conflict) => ({ choiceId: conflict.choiceId, mode: modes.choice(conflict.exists) }), + ); + + const destinations = defaultAssetDestinations( + analysis.assetConflicts, + settingsStore.getState().templateFolderPaths, + ); + const assetDecisions: AssetImportDecision[] = []; + for (const conflict of analysis.assetConflicts) { + const destinationPath = + destinations.get(conflict.originalPath) ?? conflict.originalPath; + // applyPackageImport rejects a destination outside the vault; do not + // stat it first (the package, and so this path, is untrusted input). + const exists = + destinationPath === conflict.originalPath + ? conflict.exists + : !escapesVaultBoundary(destinationPath) && + (await app.vault.adapter.exists(destinationPath)); + assetDecisions.push({ + originalPath: conflict.originalPath, + destinationPath, + mode: modes.asset(exists), + }); + } + + const result = await applyPackageImport({ + app, + existingChoices: previousChoices, + aiProviders: settingsStore.getState().ai.providers, + pkg, + choiceDecisions, + assetDecisions, + }); + + settingsStore.setState((state) => ({ ...state, choices: result.updatedChoices })); + syncImportedChoiceCommands(plugin, previousChoices, result); + await plugin.saveSettings(); + return result; +} From 7e72da181bf121b363abf5eb49aa260c527a1f0d Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 18 Oct 2026 21:44:21 +0200 Subject: [PATCH 2/5] test(template): use a type import in the collision spec's errorUtils mock --- src/engine/TemplateChoiceEngine.collision.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/engine/TemplateChoiceEngine.collision.test.ts b/src/engine/TemplateChoiceEngine.collision.test.ts index 8d1de6dde..a923b7dab 100644 --- a/src/engine/TemplateChoiceEngine.collision.test.ts +++ b/src/engine/TemplateChoiceEngine.collision.test.ts @@ -1,6 +1,7 @@ // @vitest-environment jsdom import { createChoiceExecutor } from "../../tests/helpers/createChoiceExecutor"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import type * as ErrorUtils from "../utils/errorUtils"; vi.mock("../quickAddSettingsTab", async () => { const { engineSettingsMock } = await import("../../tests/helpers/engines/settings"); @@ -81,7 +82,7 @@ vi.mock("../utils/templaterIntegration", () => ({ jumpToNextTemplaterCursorIfPossible: vi.fn(async () => true), })); vi.mock("../utils/errorUtils", async (importOriginal) => ({ - ...(await importOriginal()), + ...(await importOriginal()), reportError: vi.fn(), })); vi.mock("../utils/vaultQueries", () => ({ From 744d45f66e0dc22e58a17b79f76ed9c11c6b45e1 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 18 Oct 2026 21:44:21 +0200 Subject: [PATCH 3/5] feat(api): pick, look up, run by id and install choices from other plugins Another plugin that runs a QuickAdd choice had to ask its user to type the choice's name, then run it by that name, which breaks when the choice is renamed or shares its name. The API now lets it use QuickAdd's own picker and keep the choice's id: - pickChoice opens the picker QuickAdd: Run opens, in a pick mode that resolves the Template, Capture or Macro the user picks and runs nothing. Typing searches nested choices whatever Search nested choices says. Escape rejects with QuickAdd's usual cancellation. - getChoice looks a choice up by name or { id } and returns its id, name, type and folder path, or null. - executeChoice also takes { id }. A name still runs the first match. - importPackage imports a package the plugin ships, with no review modal. A choice whose id is already in the vault, and a file that already exists, are skipped. It throws before changing anything for an invalid package or one that runs code. --- src/api/choiceApi.ts | 100 +++++++++++++ src/api/importPackage.test.ts | 164 ++++++++++++++++++++++ src/api/pickChoice.test.ts | 125 +++++++++++++++++ src/choiceExecutor.dateScope.test.ts | 4 +- src/choiceExecutor.reentry.test.ts | 1 - src/gui/suggesters/choiceSuggester.ts | 16 ++- src/quickAddApi.audit-api-prompts.test.ts | 26 ++-- src/quickAddApi.choiceLookup.test.ts | 129 +++++++++++++++++ src/quickAddApi.executeChoice.test.ts | 8 +- src/quickAddApi.test.ts | 17 +-- src/quickAddApi.ts | 33 +++-- tests/e2e/api-pick-choice.test.ts | 84 +++++++++++ 12 files changed, 653 insertions(+), 54 deletions(-) create mode 100644 src/api/choiceApi.ts create mode 100644 src/api/importPackage.test.ts create mode 100644 src/api/pickChoice.test.ts create mode 100644 src/quickAddApi.choiceLookup.test.ts create mode 100644 tests/e2e/api-pick-choice.test.ts diff --git a/src/api/choiceApi.ts b/src/api/choiceApi.ts new file mode 100644 index 000000000..a68ae66dc --- /dev/null +++ b/src/api/choiceApi.ts @@ -0,0 +1,100 @@ +import { Notice } from "obsidian"; +import type QuickAdd from "../main"; +import { promptCancelled } from "../errors/UserCancelError"; +import ChoiceSuggester from "../gui/suggesters/choiceSuggester"; +import { NO_CHOICES_NOTICE } from "../gui/suggesters/openChoiceLauncher"; +import { analysePackagePreview, parseQuickAddPackage } from "../services/packageImportService"; +import { importPackageUnattended } from "../services/packageImportUnattended"; +import { requiresAcknowledgement } from "../services/packagePreview"; +import { settingsStore } from "../settingsStore"; +import type IChoice from "../types/choices/IChoice"; +import type { ChoiceType } from "../types/choices/choiceType"; +import { flattenChoices, flattenChoicesWithPath, rootChoicesOf } from "../utils/choiceUtils"; + +/** A choice by its id, or by its name. Names are not unique, so a name means the first match in list order. */ +export type ChoiceRef = string | { id: string }; + +/** What the API tells another plugin about a choice. The choice's own settings stay QuickAdd's. */ +export interface ChoiceSummary { + id: string; + name: string; + type: ChoiceType; + /** Folder names from the root down, the choice's own name last. */ + path: string[]; +} + +/** What importPackage added and what it left alone because the vault already had it. */ +export interface PackageImportSummary { + addedChoiceIds: string[]; + skippedChoiceIds: string[]; + writtenAssets: string[]; + skippedAssets: string[]; +} + +/** + * The choice `ref` names, searched in the order the settings list shows them. + * Script callers can pass anything, so a ref that is neither a name nor an + * `{ id }` names nothing. + */ +export function findChoice(choices: IChoice[], ref: ChoiceRef): IChoice | null { + const [key, value]: ["name" | "id", unknown] = + typeof ref === "string" ? ["name", ref] : ["id", ref?.id]; + if (typeof value !== "string") return null; + return flattenChoices(choices).find((choice) => choice[key] === value) ?? null; +} + +/** `choice`, which sits in `choices`, as the API describes it. */ +export function summarizeChoice(choices: IChoice[], choice: IChoice): ChoiceSummary { + const path = flattenChoicesWithPath(choices).find((entry) => entry.choice === choice)?.path; + return { id: choice.id, name: choice.name, type: choice.type, path: path ?? [choice.name] }; +} + +/** + * Opens the launcher's picker in pick mode and resolves with the Template, + * Capture or Macro the user picks, running nothing. Rejects with + * `promptCancelled()` when the picker is dismissed, and, after the launcher's + * notice, when there are no choices to pick from. + */ +export function pickChoice( + plugin: QuickAdd, + options?: { placeholder?: string }, +): Promise { + const choices = rootChoicesOf(plugin.settings.choices); + if (choices.length === 0) { + new Notice(NO_CHOICES_NOTICE, 8000); + return Promise.reject(promptCancelled()); + } + return new Promise((resolve, reject) => { + ChoiceSuggester.Open(plugin, choices, { + placeholder: options?.placeholder, + onPick: (choice) => resolve(summarizeChoice(choices, choice)), + // Pick mode settles the completion only when nothing was picked. + completion: () => reject(promptCancelled()), + }); + }); +} + +/** + * Imports a package the calling plugin ships, with no review modal: a choice + * whose id is already in the vault and a file that already exists are skipped, + * never overwritten. Throws before changing anything when the package is + * invalid, or when it runs code, which only the review modal may install. + */ +export async function importPackage( + plugin: QuickAdd, + packageJson: string, +): Promise { + const pkg = parseQuickAddPackage(packageJson); + const preview = await analysePackagePreview(plugin.app, settingsStore.getState().choices, pkg); + if (requiresAcknowledgement(preview)) { + throw new Error( + "This package runs code, so it can only be imported from QuickAdd's settings, where you can review it.", + ); + } + const { addedChoiceIds, skippedChoiceIds, writtenAssets, skippedAssets } = + await importPackageUnattended(plugin, pkg, { + choice: (exists) => (exists ? "skip" : "import"), + asset: (exists) => (exists ? "skip" : "write"), + }); + return { addedChoiceIds, skippedChoiceIds, writtenAssets, skippedAssets }; +} diff --git a/src/api/importPackage.test.ts b/src/api/importPackage.test.ts new file mode 100644 index 000000000..3e7c1f285 --- /dev/null +++ b/src/api/importPackage.test.ts @@ -0,0 +1,164 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import type QuickAdd from "../main"; +import { settingsStore } from "../settingsStore"; +import type IChoice from "../types/choices/IChoice"; +import type { QuickAddPackage } from "../types/packages/QuickAddPackage"; +import { encodeToBase64 } from "../utils/base64"; +import { importPackage } from "./choiceApi"; + +function fakePlugin(files: Map) { + const app = { + vault: { + configDir: ".obsidian", + adapter: { + exists: vi.fn(async (path: string) => files.has(path)), + write: vi.fn(async (path: string, content: string) => { + files.set(path, content); + }), + }, + createFolder: vi.fn(async () => {}), + }, + } as unknown as App; + const plugin = { + app, + addCommandForChoice: vi.fn(), + removeCommandForChoice: vi.fn(), + saveSettings: vi.fn(async () => {}), + }; + return plugin as unknown as QuickAdd & typeof plugin; +} + +function capture(id: string, name: string): IChoice { + return { id, name, type: "Capture", command: true, captureTo: "{{VALUE:episodeNote}}" } as IChoice; +} + +function pkg(choices: IChoice[], assets: QuickAddPackage["assets"] = []): string { + return JSON.stringify({ + schemaVersion: 1, + quickAddVersion: "2.33.0", + createdAt: "2026-10-18T00:00:00.000Z", + rootChoiceIds: choices.map((choice) => choice.id), + choices: choices.map((choice) => ({ choice, pathHint: [choice.name], parentChoiceId: null })), + assets, + } satisfies QuickAddPackage); +} + +const template = (originalPath: string, content: string): QuickAddPackage["assets"][number] => ({ + kind: "capture-template", + originalPath, + contentEncoding: "base64", + content: encodeToBase64(content), +}); + +describe("importPackage", () => { + let files: Map; + + beforeEach(() => { + files = new Map(); + settingsStore.setState({ choices: [], templateFolderPaths: [] }); + }); + + it("adds the package's choices, registers their commands and saves", async () => { + const plugin = fakePlugin(files); + + const summary = await importPackage(plugin, pkg([capture("podnotes-capture", "Podcast capture")])); + + expect(summary).toEqual({ + addedChoiceIds: ["podnotes-capture"], + skippedChoiceIds: [], + writtenAssets: [], + skippedAssets: [], + }); + expect(settingsStore.getState().choices.map((choice) => choice.name)).toEqual(["Podcast capture"]); + expect(plugin.addCommandForChoice).toHaveBeenCalledTimes(1); + expect(plugin.saveSettings).toHaveBeenCalledTimes(1); + }); + + it("resolves only after the settings are saved", async () => { + let finishSave = () => {}; + const plugin = fakePlugin(files); + plugin.saveSettings.mockImplementation(() => new Promise((resolve) => { finishSave = resolve; })); + let resolved = false; + + const run = importPackage(plugin, pkg([capture("podnotes-capture", "Podcast capture")])).then(() => { + resolved = true; + }); + await vi.waitFor(() => expect(plugin.saveSettings).toHaveBeenCalled()); + await Promise.resolve(); + expect(resolved).toBe(false); + + finishSave(); + await run; + expect(resolved).toBe(true); + }); + + it("skips a choice whose id is already in the vault and keeps it as it is", async () => { + const mine = { ...capture("podnotes-capture", "My capture"), captureTo: "Inbox.md" } as IChoice; + settingsStore.setState({ choices: [mine] }); + const plugin = fakePlugin(files); + + const summary = await importPackage( + plugin, + pkg([capture("podnotes-capture", "Podcast capture"), capture("other", "Other")]), + ); + + expect(summary.addedChoiceIds).toEqual(["other"]); + expect(summary.skippedChoiceIds).toEqual(["podnotes-capture"]); + expect(settingsStore.getState().choices).toEqual([mine, expect.objectContaining({ id: "other" })]); + expect(plugin.removeCommandForChoice).not.toHaveBeenCalled(); + }); + + it("keeps a file that already exists", async () => { + files.set("Templates/Note.md", "mine"); + const plugin = fakePlugin(files); + + const summary = await importPackage( + plugin, + pkg([capture("new", "New")], [template("Templates/Note.md", "theirs"), template("Templates/Fresh.md", "fresh")]), + ); + + expect(summary.skippedAssets).toEqual(["Templates/Note.md"]); + expect(summary.writtenAssets).toEqual(["Templates/Fresh.md"]); + expect(files.get("Templates/Note.md")).toBe("mine"); + }); + + it.each([ + ["not JSON", "{"], + ["not a package", JSON.stringify({ choices: [] })], + ])("throws for a package that is %s and changes nothing", async (_label, packageJson) => { + const mine = capture("mine", "Mine"); + settingsStore.setState({ choices: [mine] }); + const plugin = fakePlugin(files); + + await expect(importPackage(plugin, packageJson)).rejects.toThrow(); + + expect(settingsStore.getState().choices).toEqual([mine]); + expect(plugin.app.vault.adapter.write).not.toHaveBeenCalled(); + expect(plugin.saveSettings).not.toHaveBeenCalled(); + }); + + it("refuses a package that runs code and changes nothing", async () => { + const macro = { + id: "macro", + name: "Sync", + type: "Macro", + command: false, + runOnStartup: false, + macro: { + id: "m", + name: "Sync", + commands: [{ id: "s", name: "sync", type: "UserScript", path: "scripts/sync.js", settings: {} }], + }, + } as unknown as IChoice; + const plugin = fakePlugin(files); + + await expect( + importPackage(plugin, pkg([macro], [{ ...template("scripts/sync.js", "module.exports = () => {};"), kind: "user-script" }])), + ).rejects.toThrow("This package runs code"); + + expect(settingsStore.getState().choices).toEqual([]); + expect(files.has("scripts/sync.js")).toBe(false); + expect(plugin.saveSettings).not.toHaveBeenCalled(); + }); +}); diff --git a/src/api/pickChoice.test.ts b/src/api/pickChoice.test.ts new file mode 100644 index 000000000..8c719e555 --- /dev/null +++ b/src/api/pickChoice.test.ts @@ -0,0 +1,125 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { App, Notice } from "obsidian"; +import type QuickAdd from "../main"; +import { ChoiceExecutor } from "../choiceExecutor"; +import { UserCancelError } from "../errors/UserCancelError"; +import ChoiceSuggester, { BACK_CHOICE_ID } from "../gui/suggesters/choiceSuggester"; +import { NO_CHOICES_NOTICE } from "../gui/suggesters/openChoiceLauncher"; +import { settingsStore } from "../settingsStore"; +import type IChoice from "../types/choices/IChoice"; +import type IMultiChoice from "../types/choices/IMultiChoice"; +import { pickChoice } from "./choiceApi"; + +function leaf(id: string, name: string): IChoice { + return { id, name, type: "Capture", command: false } as IChoice; +} + +function folder(id: string, name: string, choices: IChoice[]): IMultiChoice { + return { id, name, type: "Multi", command: false, choices, collapsed: false }; +} + +const pressed = new MouseEvent("click"); +const match = { score: 0, matches: [] }; + +describe("pickChoice", () => { + let opened: ChoiceSuggester[]; + let ran: string[]; + let timestamp: IChoice; + let podcasts: IMultiChoice; + let inbox: IChoice; + + const plugin = (choices: IChoice[]) => + ({ app: new App(), settings: { choices } }) as unknown as QuickAdd; + const current = () => opened[opened.length - 1]; + const choose = (item: IChoice) => current().selectSuggestion({ item, match }, pressed); + + beforeEach(() => { + opened = []; + vi.spyOn(ChoiceSuggester.prototype, "open").mockImplementation(function (this: ChoiceSuggester) { + opened.push(this); + }); + ran = []; + vi.spyOn(ChoiceExecutor.prototype, "execute").mockImplementation(async (choice) => { + ran.push(choice.name); + }); + vi.spyOn(ChoiceExecutor.prototype, "executeWithFocusedProperty").mockImplementation(async (choice) => { + ran.push(choice.name); + }); + (Notice as unknown as { instances: unknown[] }).instances = []; + settingsStore.setState({ searchNestedChoices: false }); + + timestamp = leaf("timestamp", "Log timestamp"); + podcasts = folder("podcasts", "Podcasts", [timestamp]); + inbox = leaf("inbox", "Inbox"); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("resolves the picked choice and runs nothing", async () => { + const picked = pickChoice(plugin([inbox, podcasts])); + + choose(inbox); + + await expect(picked).resolves.toEqual({ + id: "inbox", + name: "Inbox", + type: "Capture", + path: ["Inbox"], + }); + expect(ran).toEqual([]); + }); + + it("keeps pick mode through a folder and Back", async () => { + const picked = pickChoice(plugin([inbox, podcasts])); + + choose(podcasts); + const back = current().getItems().find((item) => item.id === BACK_CHOICE_ID); + choose(back as IChoice); + choose(podcasts); + choose(timestamp); + + await expect(picked).resolves.toMatchObject({ + id: "timestamp", + path: ["Podcasts", "Log timestamp"], + }); + expect(opened).toHaveLength(4); + expect(ran).toEqual([]); + }); + + it("searches nested choices even with Search nested choices off", () => { + void pickChoice(plugin([inbox, podcasts])); + + const found = current().getSuggestions("timestamp").map((result) => result.item); + + expect(found).toEqual([timestamp]); + }); + + it("rejects with a cancellation when the picker is dismissed", async () => { + const picked = pickChoice(plugin([inbox, podcasts])); + + current().close(); + + await expect(picked).rejects.toBeInstanceOf(UserCancelError); + expect(ran).toEqual([]); + }); + + it("shows the launcher's notice and opens nothing when there are no choices", async () => { + await expect(pickChoice(plugin([]))).rejects.toBeInstanceOf(UserCancelError); + + expect(opened).toEqual([]); + expect( + (Notice as unknown as { instances: Array<{ message: string }> }).instances.map( + (notice) => notice.message, + ), + ).toEqual([NO_CHOICES_NOTICE]); + }); + + it("shows the caller's placeholder", () => { + void pickChoice(plugin([inbox]), { placeholder: "Pick a capture choice" }); + + expect(current().inputEl.placeholder).toBe("Pick a capture choice"); + }); +}); diff --git a/src/choiceExecutor.dateScope.test.ts b/src/choiceExecutor.dateScope.test.ts index f460b779b..8f4744a2a 100644 --- a/src/choiceExecutor.dateScope.test.ts +++ b/src/choiceExecutor.dateScope.test.ts @@ -95,7 +95,7 @@ describe("ChoiceExecutor date scope", () => { datedTemplate("Today template"), datedTemplate("Variable template", { kind: "variable", name: "day" }), ]; - const plugin = { getChoiceByName: (name: string) => choices.find((c) => c.name === name) } as unknown as QuickAdd; + const plugin = { settings: { choices } } as unknown as QuickAdd; const api = QuickAddApi.GetApi(new App(), plugin, runner); const caller = datedTemplate("Caller"); runTemplate.mockImplementation(async (active, choice) => { @@ -124,7 +124,7 @@ describe("ChoiceExecutor date scope", () => { it("keeps the caller's day when an executeChoice call fails", async () => { const runner = executor(); const failing = datedTemplate("Failing"); - const plugin = { getChoiceByName: () => failing } as unknown as QuickAdd; + const plugin = { settings: { choices: [failing] } } as unknown as QuickAdd; const api = QuickAddApi.GetApi(new App(), plugin, runner); const caller = datedTemplate("Caller", { kind: "variable", name: "callerDay" }); runner.variables.set("callerDay", "2026-01-10"); diff --git a/src/choiceExecutor.reentry.test.ts b/src/choiceExecutor.reentry.test.ts index 0a051da0f..1f62b6776 100644 --- a/src/choiceExecutor.reentry.test.ts +++ b/src/choiceExecutor.reentry.test.ts @@ -53,7 +53,6 @@ const plugin = { if (!choice) throw new Error(`Choice ${id} not found`); return choice; }, - getChoiceByName: (name: string) => choices.find((c) => c.name === name), } as never; let ran: string[] = []; diff --git a/src/gui/suggesters/choiceSuggester.ts b/src/gui/suggesters/choiceSuggester.ts index 523fc9128..8c88c9150 100644 --- a/src/gui/suggesters/choiceSuggester.ts +++ b/src/gui/suggesters/choiceSuggester.ts @@ -188,6 +188,14 @@ type ChoiceSuggesterOptions = { * the top-level launcher. */ ancestry?: ChoiceChain; + /** + * Pick mode, for the API's pickChoice: a picked Template, Capture or Macro is + * handed here and nothing runs. Folders still open, and typing searches nested + * choices whatever Search nested choices says, since the caller wants any leaf. + * Threaded through folder levels like `completion`, which still settles a + * dismissal. + */ + onPick?: (choice: IChoice) => void; }; /** @@ -216,6 +224,7 @@ export default class ChoiceSuggester extends FuzzySuggestModal { private currentPlaceholder?: string; private readonly completion?: ChoiceSuggesterOptions["completion"]; private readonly ancestry?: ChoiceChain; + private readonly onPick?: ChoiceSuggesterOptions["onPick"]; /** * Set the moment an activation is accepted (selectSuggestion, BEFORE the * modal closes), so onClose can tell a dismissal from a pick: @@ -272,6 +281,7 @@ export default class ChoiceSuggester extends FuzzySuggestModal { this.placeholderStack = options?.placeholderStack ?? []; this.completion = options?.completion; this.ancestry = options?.ancestry; + this.onPick = options?.onPick; // `currentPlaceholder` is what a nested level pushes onto the stack so Back // can restore it, so it must hold the effective placeholder (including the // default), not just an explicitly-passed one. @@ -312,7 +322,8 @@ export default class ChoiceSuggester extends FuzzySuggestModal { getSuggestions(query: string): FuzzyMatch[] { const trimmed = query.trim(); - if (!trimmed || !settingsStore.getState().searchNestedChoices) { + const searchNested = this.onPick !== undefined || settingsStore.getState().searchNestedChoices; + if (!trimmed || !searchNested) { return super.getSuggestions(trimmed); } @@ -527,6 +538,8 @@ export default class ChoiceSuggester extends FuzzySuggestModal { if (item.type === "Multi") this.onChooseMultiType(item); + else if (this.onPick) + this.onPick(item); else { // Route through executeWithFocusedProperty whenever it exists so the // captured focusedProperty AND triggerContext (issue #1429) are both @@ -601,6 +614,7 @@ export default class ChoiceSuggester extends FuzzySuggestModal { // the terminal action settles it. completion: this.completion, ancestry: this.ancestry, + onPick: this.onPick, }); } } diff --git a/src/quickAddApi.audit-api-prompts.test.ts b/src/quickAddApi.audit-api-prompts.test.ts index 36389cd75..7cda504c5 100644 --- a/src/quickAddApi.audit-api-prompts.test.ts +++ b/src/quickAddApi.audit-api-prompts.test.ts @@ -104,11 +104,8 @@ function makeChoiceExecutor() { }; } -function makePlugin(overrides: Record = {}) { - return { - getChoiceByName: vi.fn(() => ({ name: "choice", id: "1" })), - ...overrides, - } as unknown as QuickAdd; +function makePlugin(choices: unknown[] = []) { + return { settings: { choices } } as unknown as QuickAdd; } function getApi( @@ -129,19 +126,13 @@ beforeEach(() => { }); // =========================================================================== -// executeChoice — production getChoiceByName THROWS for an unknown name. +// executeChoice on a name no choice has. // (audit: api-execute-choice) // =========================================================================== -describe("executeChoice on an unknown choice (throwing getChoiceByName)", () => { - it("reports the error and returns instead of propagating the throw", async () => { - // Mirror production main.ts:getChoiceByName, which throws (never returns falsy). - const plugin = makePlugin({ - getChoiceByName: vi.fn(() => { - throw new Error("Choice typo not found"); - }), - }); +describe("executeChoice on an unknown choice", () => { + it("reports the error and returns instead of throwing", async () => { const executor = makeChoiceExecutor(); - const { api } = getApi(makeApp(), plugin, executor); + const { api } = getApi(makeApp(), makePlugin(), executor); await expect(api.executeChoice("typo")).resolves.toBeUndefined(); expect(mocks.reportError).toHaveBeenCalled(); @@ -150,10 +141,9 @@ describe("executeChoice on an unknown choice (throwing getChoiceByName)", () => }); it("still executes a found choice and clears variables", async () => { - const choice = { name: "MyChoice", id: "id1" }; + const choice = { name: "MyChoice", id: "id1", type: "Template", command: false }; const executor = makeChoiceExecutor(); - const plugin = makePlugin({ getChoiceByName: vi.fn(() => choice) }); - const { api } = getApi(makeApp(), plugin, executor); + const { api } = getApi(makeApp(), makePlugin([choice]), executor); await api.executeChoice("MyChoice", { foo: "bar" }); diff --git a/src/quickAddApi.choiceLookup.test.ts b/src/quickAddApi.choiceLookup.test.ts new file mode 100644 index 000000000..1f3e2e7ff --- /dev/null +++ b/src/quickAddApi.choiceLookup.test.ts @@ -0,0 +1,129 @@ +import { createChoiceExecutor } from "../tests/helpers/createChoiceExecutor"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { App } from "obsidian"; +import { QuickAddApi } from "./quickAddApi"; +import type QuickAdd from "./main"; +import type { IChoiceExecutor } from "./IChoiceExecutor"; +import type IChoice from "./types/choices/IChoice"; +import type IMultiChoice from "./types/choices/IMultiChoice"; +import { reportError } from "./utils/errorUtils"; +import type * as ErrorUtils from "./utils/errorUtils"; + +vi.mock("./quickAddSettingsTab", () => ({ + DEFAULT_SETTINGS: {}, + QuickAddSettingsTab: class {}, +})); + +vi.mock("./formatters/completeFormatter", () => ({ + CompleteFormatter: class CompleteFormatterMock {}, +})); + +vi.mock("./utils/errorUtils", async (importOriginal) => ({ + ...(await importOriginal()), + reportError: vi.fn(), +})); + +function leaf(id: string, name: string, type: IChoice["type"] = "Capture"): IChoice { + return { id, name, type, command: false, captureTo: "Inbox.md" } as IChoice; +} + +function folder(id: string, name: string, choices: IChoice[]): IMultiChoice { + return { id, name, type: "Multi", command: false, choices, collapsed: false }; +} + +describe("QuickAddApi choice lookup", () => { + const app = {} as App; + let executed: IChoice[]; + let choiceExecutor: IChoiceExecutor; + let choices: IChoice[]; + + const api = () => + QuickAddApi.GetApi(app, { settings: { choices } } as unknown as QuickAdd, choiceExecutor); + + beforeEach(() => { + vi.mocked(reportError).mockClear(); + executed = []; + choiceExecutor = { + ...createChoiceExecutor(), + execute: vi.fn(async (choice: IChoice) => { + executed.push(choice); + }), + variables: new Map(), + consumeAbortSignal: vi.fn().mockReturnValue(null), + }; + choices = [ + leaf("inbox", "Log"), + folder("podcasts", "Podcasts", [ + folder("shows", "Shows", [leaf("episode", "Log", "Macro")]), + ]), + ]; + }); + + describe("getChoice", () => { + // toEqual also proves no stored setting (captureTo) crosses the boundary. + it("describes a nested choice by id with its folder path", () => { + expect(api().getChoice({ id: "episode" })).toEqual({ + id: "episode", + name: "Log", + type: "Macro", + path: ["Podcasts", "Shows", "Log"], + }); + }); + + it("takes the first choice in list order when names repeat", () => { + expect(api().getChoice("Log")).toEqual({ + id: "inbox", + name: "Log", + type: "Capture", + path: ["Log"], + }); + }); + + it("describes a folder too", () => { + expect(api().getChoice("Podcasts")).toMatchObject({ type: "Multi", path: ["Podcasts"] }); + }); + + it.each([ + ["an unknown id", { id: "gone" }], + ["an unknown name", "Gone"], + ["an id passed as a name", "inbox"], + ["null from a script", null], + ["an object without an id", {}], + ])("returns null for %s without a notice", (_label, ref) => { + expect(api().getChoice(ref as never)).toBeNull(); + expect(reportError).not.toHaveBeenCalled(); + }); + }); + + describe("executeChoice by id", () => { + it("runs the choice with that id after a rename", async () => { + const renamed = leaf("episode", "Episode timestamp", "Macro"); + choices = [leaf("inbox", "Log"), folder("podcasts", "Podcasts", [renamed])]; + + await api().executeChoice({ id: "episode" }, { timestamp: "00:12:34" }); + + expect(executed).toEqual([renamed]); + }); + + it("runs exactly that one of two choices sharing a name", async () => { + await api().executeChoice({ id: "episode" }); + + expect(executed.map((choice) => choice.id)).toEqual(["episode"]); + }); + + it("still runs a name as the first match", async () => { + await api().executeChoice("Log"); + + expect(executed.map((choice) => choice.id)).toEqual(["inbox"]); + }); + + it("reports an unknown id as a missing choice and resolves", async () => { + await expect(api().executeChoice({ id: "gone" })).resolves.toBeUndefined(); + + expect(executed).toEqual([]); + expect(reportError).toHaveBeenCalledTimes(1); + const [error] = vi.mocked(reportError).mock.calls[0]; + expect((error as Error).message).toBe("Choice with id 'gone' not found"); + }); + }); +}); diff --git a/src/quickAddApi.executeChoice.test.ts b/src/quickAddApi.executeChoice.test.ts index 4566d0342..25d497138 100644 --- a/src/quickAddApi.executeChoice.test.ts +++ b/src/quickAddApi.executeChoice.test.ts @@ -21,7 +21,7 @@ vi.mock("./formatters/completeFormatter", () => ({ describe("QuickAddApi.executeChoice", () => { const app = {} as App; - let plugin: QuickAdd & { getChoiceByName: ReturnType; }; + let plugin: QuickAdd; let choiceExecutor: IChoiceExecutor; let variables: Map; const choice: IChoice = { @@ -39,11 +39,7 @@ describe("QuickAddApi.executeChoice", () => { variables, consumeAbortSignal: vi.fn().mockReturnValue(null), }; - plugin = { - getChoiceByName: vi.fn().mockReturnValue(choice), - } as unknown as QuickAdd & { - getChoiceByName: ReturnType; - }; + plugin = { settings: { choices: [choice] } } as unknown as QuickAdd; }); it("propagates aborts from executed choices", async () => { diff --git a/src/quickAddApi.test.ts b/src/quickAddApi.test.ts index b3fa4acc8..5ed8a441c 100644 --- a/src/quickAddApi.test.ts +++ b/src/quickAddApi.test.ts @@ -190,11 +190,8 @@ function makeChoiceExecutor() { }; } -function makePlugin(overrides: Record = {}) { - return { - getChoiceByName: vi.fn(() => ({ name: "choice", id: "1" })), - ...overrides, - } as unknown as QuickAdd; +function makePlugin(choices: unknown[] = [{ name: "C", id: "1", type: "Template", command: false }]) { + return { settings: { choices } } as unknown as QuickAdd; } function getApi( @@ -967,25 +964,23 @@ describe("ai.chunkedPrompt validation", () => { // =========================================================================== describe("executeChoice", () => { it("looks up the choice, sets variables, and executes it", async () => { - const choice = { name: "MyChoice", id: "id1" }; + const choice = { name: "MyChoice", id: "id1", type: "Template", command: false }; const executor = makeChoiceExecutor(); - const plugin = makePlugin({ getChoiceByName: vi.fn(() => choice) }); - const { api } = getApi(makeApp(), plugin, executor); + const { api } = getApi(makeApp(), makePlugin([choice]), executor); await api.executeChoice("MyChoice", { foo: "bar" }); - expect(plugin.getChoiceByName).toHaveBeenCalledWith("MyChoice"); expect(executor.execute).toHaveBeenCalledWith(choice, undefined); // Variables are cleared after execution. expect(executor.variables.size).toBe(0); }); it("reports an error when the choice is not found", async () => { - const plugin = makePlugin({ getChoiceByName: vi.fn(() => undefined) }); const executor = makeChoiceExecutor(); - const { api } = getApi(makeApp(), plugin, executor); + const { api } = getApi(makeApp(), makePlugin(), executor); await api.executeChoice("Missing"); expect(mocks.reportError).toHaveBeenCalled(); + expect(executor.execute).not.toHaveBeenCalled(); }); it("throws the abort signal when the executor reports one", async () => { diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index dd19e8ad8..f952ccbb0 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -10,8 +10,9 @@ import { getActiveEditorSelection, getActiveMarkdownEditorView } from "./utils/a import { applyInvocationDate } from "./utils/resolveDateOrigin"; import { handledIfCancelled, reportError } from "./utils/errorUtils"; import { getDate } from "./utils/dates"; -import type IChoice from "./types/choices/IChoice"; import type { InputPromptOptions } from "./types/inputPrompt"; +import type { ChoiceRef, ChoiceSummary } from "./api/choiceApi"; +import { findChoice, importPackage, pickChoice, summarizeChoice } from "./api/choiceApi"; import { PromptApi } from "./api/promptApi"; import { requestInputs } from "./api/requestInputs"; import { createAiApi } from "./api/aiApi"; @@ -81,25 +82,20 @@ export class QuickAddApi { } }; const executeChoice = async ( - choiceName: string, + ref: ChoiceRef, variables?: Record, options?: { date?: string | Date }, ) => { - // getChoiceByName THROWS when the name doesn't match a choice, so - // look it up defensively: report + return (don't abort the macro) - // to honor the documented "reports an error, does not throw" - // contract. The `!choice` fallback also covers any non-throwing - // lookup that yields a falsy result. - let choice: IChoice | undefined; - try { - choice = plugin.getChoiceByName(choiceName); - } catch { - choice = undefined; - } - + // Report and return rather than throw: a missing choice must not + // abort the calling macro. + const choice = findChoice(plugin.settings.choices, ref); if (!choice) { reportError( - new Error(`Choice named '${choiceName}' not found`), + new Error( + typeof ref === "string" + ? `Choice named '${ref}' not found` + : `Choice with id '${String(ref?.id)}' not found`, + ), "API executeChoice error", ); return; @@ -240,6 +236,13 @@ export class QuickAddApi { // Escape there must not show up as an error. executeChoice: (...args: Parameters) => handledIfCancelled(executeChoice(...args)), + getChoice: (ref: ChoiceRef): ChoiceSummary | null => { + const choice = findChoice(plugin.settings.choices, ref); + return choice ? summarizeChoice(plugin.settings.choices, choice) : null; + }, + pickChoice: (options?: { placeholder?: string }) => + handledIfCancelled(pickChoice(plugin, options)), + importPackage: (packageJson: string) => importPackage(plugin, packageJson), applyTemplateToActiveFile: async ( templatePath: string, options?: { mode?: TemplateInsertModeId }, diff --git a/tests/e2e/api-pick-choice.test.ts b/tests/e2e/api-pick-choice.test.ts new file mode 100644 index 000000000..2b88c3975 --- /dev/null +++ b/tests/e2e/api-pick-choice.test.ts @@ -0,0 +1,84 @@ +import { expect, it } from "vitest"; +import { CaptureChoice } from "../../src/types/choices/CaptureChoice"; +import { MultiChoice } from "../../src/types/choices/MultiChoice"; +import type IChoice from "../../src/types/choices/IChoice"; +import { createQuickAddE2EHarness, seedVaultFile } from "./e2eVault"; +import { closeOpenPrompts, insertText, jsLiteral, POLL_OPTS, pressKey } from "./uiHelpers"; + +// Another plugin lets its user pick a QuickAdd choice, stores the id, and runs it +// later: the picker must run nothing, and the id must survive a rename. +const getContext = createQuickAddE2EHarness("api-pick-choice"); + +const SELECTED = ".qa-choice-suggester .suggestion-item.is-selected"; + +it("picks a nested choice without running it, then runs it by id after a rename", async () => { + const { obsidian, plugin, sandbox } = getContext(); + const episode = await seedVaultFile(obsidian, sandbox, "Episode.md", ""); + const decoy = new CaptureChoice("Log book"); + decoy.captureTo = episode; + const capture = new CaptureChoice("Log timestamp"); + capture.captureTo = episode; + capture.format = { enabled: true, format: "- {{VALUE:timestamp}}" }; + const podcasts = new MultiChoice("Podcasts").addChoice(capture); + await plugin.data<{ choices: IChoice[]; searchNestedChoices: boolean }>().patch((data) => { + data.choices = [decoy, podcasts]; + // Pick mode searches nested choices whatever this says. + data.searchNestedChoices = false; + }); + await plugin.reload({ waitUntilReady: true }); + await obsidian.exec("dev:errors", { clear: true }); + + try { + await obsidian.dev.evalJson(`(() => { + window.__qaPick = { status: "pending" }; + void app.plugins.plugins.quickadd.api.pickChoice().then( + (choice) => { window.__qaPick = { status: "resolved", choice }; }, + (error) => { window.__qaPick = { status: "rejected", name: error?.name }; }, + ); + return true; + })()`); + await expect.poll(() => obsidian.dev.evalJson( + `Boolean(document.activeElement?.closest(".qa-choice-suggester"))`, + ), POLL_OPTS).toBe(true); + + await insertText(obsidian, "timestamp"); + await expect.poll(() => obsidian.dev.evalJson( + `document.querySelector(${jsLiteral(SELECTED)})?.textContent ?? null`, + ), POLL_OPTS).toBe("Log timestampPodcasts"); + await pressKey(obsidian, "Enter"); + + await expect.poll(() => obsidian.dev.evalJson("window.__qaPick"), POLL_OPTS).toEqual({ + status: "resolved", + choice: { id: capture.id, name: "Log timestamp", type: "Capture", path: ["Podcasts", "Log timestamp"] }, + }); + // Running the capture would open its {{VALUE:timestamp}} prompt at once. + await obsidian.sleep(500); + expect(await obsidian.dev.evalJson( + 'Boolean(document.querySelector(".modal-container, .prompt"))', + )).toBe(false); + expect(await sandbox.read("Episode.md")).toBe(""); + + capture.name = "Episode timestamp"; + await plugin.data<{ choices: IChoice[] }>().patch((data) => { + data.choices = [decoy, podcasts]; + }); + await plugin.reload({ waitUntilReady: true }); + const renamed = await obsidian.dev.evalJsonAsync(`(async () => { + const api = app.plugins.plugins.quickadd.api; + await api.executeChoice({ id: ${jsLiteral(capture.id)} }, { timestamp: "[00:12:34]" }); + return api.getChoice({ id: ${jsLiteral(capture.id)} }); + })()`); + + expect(renamed).toEqual({ + id: capture.id, + name: "Episode timestamp", + type: "Capture", + path: ["Podcasts", "Episode timestamp"], + }); + await expect.poll(() => sandbox.read("Episode.md"), POLL_OPTS).toContain("- [00:12:34]"); + expect((await obsidian.execText("dev:errors")).trim()).toBe("No errors captured."); + } finally { + await obsidian.dev.evalJson("delete window.__qaPick; true"); + await closeOpenPrompts(obsidian); + } +}); From bd5a43dd2c879aefc8cc99d5b293642530c2ab17 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 18 Oct 2026 22:09:15 +0200 Subject: [PATCH 4/5] fix(packages): decode every bundled file before an import writes one applyPackageImport decoded each file inside its write loop, so a package whose second file was not valid base64 wrote the first file and its folders before it failed. The modal, the CLI and the API all apply imports through it. It now decodes every file while it plans the writes, so an undecodable file stops the import before anything changes. --- src/services/packageImportService.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/services/packageImportService.ts b/src/services/packageImportService.ts index 0981c92f1..c84f83821 100644 --- a/src/services/packageImportService.ts +++ b/src/services/packageImportService.ts @@ -557,10 +557,12 @@ export async function applyPackageImport( await assertWriteStaysInVault(app, destinationPath); } - // Decide every write before the first one: a bundled template's - // `{{TEMPLATE:...}}` includes must point at where the included file is - // actually going, which is only known once all destinations are settled. - const plannedWrites: Array<{ asset: QuickAddPackage["assets"][number]; destinationPath: string }> = []; + // Decide every write, and decode every file, before the first one: a bundled + // template's `{{TEMPLATE:...}}` includes must point at where the included + // file is actually going, which is only known once all destinations are + // settled, and a file that fails to decode must stop the import before it + // writes anything. + const plannedWrites: Array<{ destinationPath: string; content: string }> = []; for (const { asset, destinationPath } of resolvedAssetDestinations) { const decision = assetDecisionMap.get(asset.originalPath); const exists = await assetExists(app, destinationPath); @@ -576,12 +578,14 @@ export async function applyPackageImport( `Refusing to import: "${destinationPath}" is a folder. Choose a file path for "${asset.originalPath}".`, ); } - plannedWrites.push({ asset, destinationPath }); + plannedWrites.push({ + destinationPath, + content: importedAssetContent(asset, destinationPath, assetPathOverrides), + }); } - for (const { asset, destinationPath } of plannedWrites) { + for (const { destinationPath, content } of plannedWrites) { await ensureParentFolders(app, destinationPath); - const content = importedAssetContent(asset, destinationPath, assetPathOverrides); await app.vault.adapter.write(destinationPath, content); writtenAssets.push(destinationPath); } From 480474584cd44ab88d39e8d9b8f9b2069f84d5f3 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 18 Oct 2026 22:09:15 +0200 Subject: [PATCH 5/5] fix(api): share one choice lookup and gate unattended imports in one place - main.ts and the API now find a choice through findChoiceWithPath, so a name or id resolves the same way in the API, URIs, commands and the CLI. getChoice walks the tree once. - importPackageUnattended runs the trust gate itself and returns needsReview instead of importing, so no caller can skip it. The CLI passes acknowledge=true through. - It asks whether a file exists at the validated path it will write, so a padded path cannot overwrite a file that importPackage must skip. - pickChoice reads the picked choice from the live tree, and executeChoice names a non-object ref as a name in its error. --- src/api/choiceApi.ts | 51 ++++++++++----------- src/api/importPackage.test.ts | 35 ++++++++++++-- src/cli/importPackageCli.ts | 25 +++++----- src/main.ts | 26 ++--------- src/quickAddApi.ts | 12 ++--- src/services/packageImportService.ts | 2 +- src/services/packageImportUnattended.ts | 61 +++++++++++++++---------- src/utils/choiceUtils.ts | 13 ++++++ 8 files changed, 128 insertions(+), 97 deletions(-) diff --git a/src/api/choiceApi.ts b/src/api/choiceApi.ts index a68ae66dc..0a3e75250 100644 --- a/src/api/choiceApi.ts +++ b/src/api/choiceApi.ts @@ -3,13 +3,12 @@ import type QuickAdd from "../main"; import { promptCancelled } from "../errors/UserCancelError"; import ChoiceSuggester from "../gui/suggesters/choiceSuggester"; import { NO_CHOICES_NOTICE } from "../gui/suggesters/openChoiceLauncher"; -import { analysePackagePreview, parseQuickAddPackage } from "../services/packageImportService"; +import { parseQuickAddPackage } from "../services/packageImportService"; import { importPackageUnattended } from "../services/packageImportUnattended"; -import { requiresAcknowledgement } from "../services/packagePreview"; -import { settingsStore } from "../settingsStore"; import type IChoice from "../types/choices/IChoice"; import type { ChoiceType } from "../types/choices/choiceType"; -import { flattenChoices, flattenChoicesWithPath, rootChoicesOf } from "../utils/choiceUtils"; +import type { FlatChoicePathEntry } from "../utils/choiceUtils"; +import { findChoiceWithPath, rootChoicesOf } from "../utils/choiceUtils"; /** A choice by its id, or by its name. Names are not unique, so a name means the first match in list order. */ export type ChoiceRef = string | { id: string }; @@ -32,21 +31,17 @@ export interface PackageImportSummary { } /** - * The choice `ref` names, searched in the order the settings list shows them. - * Script callers can pass anything, so a ref that is neither a name nor an - * `{ id }` names nothing. + * The choice `ref` names, with its path. Script callers can pass anything, so a + * ref that is neither a name nor an `{ id }` names nothing. */ -export function findChoice(choices: IChoice[], ref: ChoiceRef): IChoice | null { - const [key, value]: ["name" | "id", unknown] = +export function findChoice(choices: IChoice[], ref: ChoiceRef): FlatChoicePathEntry | undefined { + const [by, value]: ["name" | "id", unknown] = typeof ref === "string" ? ["name", ref] : ["id", ref?.id]; - if (typeof value !== "string") return null; - return flattenChoices(choices).find((choice) => choice[key] === value) ?? null; + return typeof value === "string" ? findChoiceWithPath(choices, by, value) : undefined; } -/** `choice`, which sits in `choices`, as the API describes it. */ -export function summarizeChoice(choices: IChoice[], choice: IChoice): ChoiceSummary { - const path = flattenChoicesWithPath(choices).find((entry) => entry.choice === choice)?.path; - return { id: choice.id, name: choice.name, type: choice.type, path: path ?? [choice.name] }; +export function summarizeChoice({ choice, path }: FlatChoicePathEntry): ChoiceSummary { + return { id: choice.id, name: choice.name, type: choice.type, path }; } /** @@ -67,7 +62,13 @@ export function pickChoice( return new Promise((resolve, reject) => { ChoiceSuggester.Open(plugin, choices, { placeholder: options?.placeholder, - onPick: (choice) => resolve(summarizeChoice(choices, choice)), + // Looked up again in the live tree, since a sync can change the + // choices while the picker is open. + onPick: (choice) => { + const picked = findChoice(plugin.settings.choices, { id: choice.id }); + if (picked) resolve(summarizeChoice(picked)); + else reject(promptCancelled()); + }, // Pick mode settles the completion only when nothing was picked. completion: () => reject(promptCancelled()), }); @@ -78,23 +79,21 @@ export function pickChoice( * Imports a package the calling plugin ships, with no review modal: a choice * whose id is already in the vault and a file that already exists are skipped, * never overwritten. Throws before changing anything when the package is - * invalid, or when it runs code, which only the review modal may install. + * invalid, or when it needs the review modal, as a package that runs code does. */ export async function importPackage( plugin: QuickAdd, packageJson: string, ): Promise { - const pkg = parseQuickAddPackage(packageJson); - const preview = await analysePackagePreview(plugin.app, settingsStore.getState().choices, pkg); - if (requiresAcknowledgement(preview)) { + const outcome = await importPackageUnattended(plugin, parseQuickAddPackage(packageJson), { + choice: (exists) => (exists ? "skip" : "import"), + asset: (exists) => (exists ? "skip" : "write"), + }); + if ("needsReview" in outcome) { throw new Error( - "This package runs code, so it can only be imported from QuickAdd's settings, where you can review it.", + "This package needs review, so it can only be imported from QuickAdd's settings.", ); } - const { addedChoiceIds, skippedChoiceIds, writtenAssets, skippedAssets } = - await importPackageUnattended(plugin, pkg, { - choice: (exists) => (exists ? "skip" : "import"), - asset: (exists) => (exists ? "skip" : "write"), - }); + const { addedChoiceIds, skippedChoiceIds, writtenAssets, skippedAssets } = outcome.imported; return { addedChoiceIds, skippedChoiceIds, writtenAssets, skippedAssets }; } diff --git a/src/api/importPackage.test.ts b/src/api/importPackage.test.ts index 3e7c1f285..567759c5a 100644 --- a/src/api/importPackage.test.ts +++ b/src/api/importPackage.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; import type QuickAdd from "../main"; import { settingsStore } from "../settingsStore"; @@ -59,6 +59,10 @@ describe("importPackage", () => { settingsStore.setState({ choices: [], templateFolderPaths: [] }); }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + it("adds the package's choices, registers their commands and saves", async () => { const plugin = fakePlugin(files); @@ -123,6 +127,31 @@ describe("importPackage", () => { expect(files.get("Templates/Note.md")).toBe("mine"); }); + it("keeps an existing file that the package spells with padding", async () => { + files.set("Templates/Note.md", "mine"); + const plugin = fakePlugin(files); + + const summary = await importPackage(plugin, pkg([capture("new", "New")], [template(" Templates/Note.md ", "theirs")])); + + expect(summary.skippedAssets).toEqual(["Templates/Note.md"]); + expect(files.get("Templates/Note.md")).toBe("mine"); + }); + + it("writes no file when a later file in the package cannot be decoded", async () => { + // Obsidian decodes with the browser's atob, which throws on bad input. + vi.stubGlobal("window", { atob }); + const plugin = fakePlugin(files); + const broken = { ...template("Templates/Broken.md", ""), content: "%%%" }; + + await expect( + importPackage(plugin, pkg([capture("new", "New")], [template("Templates/Fresh.md", "fresh"), broken])), + ).rejects.toThrow(); + + expect(plugin.app.vault.adapter.write).not.toHaveBeenCalled(); + expect(plugin.app.vault.createFolder).not.toHaveBeenCalled(); + expect(settingsStore.getState().choices).toEqual([]); + }); + it.each([ ["not JSON", "{"], ["not a package", JSON.stringify({ choices: [] })], @@ -138,7 +167,7 @@ describe("importPackage", () => { expect(plugin.saveSettings).not.toHaveBeenCalled(); }); - it("refuses a package that runs code and changes nothing", async () => { + it("refuses a package that needs review and changes nothing", async () => { const macro = { id: "macro", name: "Sync", @@ -155,7 +184,7 @@ describe("importPackage", () => { await expect( importPackage(plugin, pkg([macro], [{ ...template("scripts/sync.js", "module.exports = () => {};"), kind: "user-script" }])), - ).rejects.toThrow("This package runs code"); + ).rejects.toThrow("This package needs review"); expect(settingsStore.getState().choices).toEqual([]); expect(files.has("scripts/sync.js")).toBe(false); diff --git a/src/cli/importPackageCli.ts b/src/cli/importPackageCli.ts index b74fb81a6..7d0ab7d05 100644 --- a/src/cli/importPackageCli.ts +++ b/src/cli/importPackageCli.ts @@ -1,11 +1,9 @@ import type { CliData, CliFlags } from "obsidian"; import type QuickAdd from "../main"; -import { settingsStore } from "../settingsStore"; import { effectiveChoiceMode, reconcileMode } from "../gui/PackageManager/importDecisions"; import type { AssetImportMode, ChoiceImportMode } from "../services/packageImportService"; -import { analysePackagePreview, readQuickAddPackage } from "../services/packageImportService"; +import { readQuickAddPackage } from "../services/packageImportService"; import { importPackageUnattended } from "../services/packageImportUnattended"; -import { requiresAcknowledgement } from "../services/packagePreview"; import type IChoice from "../types/choices/IChoice"; import { flattenChoices } from "../utils/choiceUtils"; import { isTruthy } from "./params"; @@ -66,11 +64,15 @@ export async function importPackageHandler( const choiceMode = parseMode(params.choices, CHOICE_MODES, "choices"); const assetMode = parseMode(params.files, ASSET_MODES, "files"); - const { app } = plugin; - const { pkg } = await readQuickAddPackage(app, path); - const preview = await analysePackagePreview(app, settingsStore.getState().choices, pkg); - - if (requiresAcknowledgement(preview) && !isTruthy(params.acknowledge)) { + const { pkg } = await readQuickAddPackage(plugin.app, path); + const outcome = await importPackageUnattended(plugin, pkg, { + choice: (exists) => + effectiveChoiceMode(choiceMode ?? (exists ? "overwrite" : "import"), exists), + asset: (exists) => reconcileMode(assetMode ?? (exists ? "overwrite" : "write"), exists), + acknowledged: isTruthy(params.acknowledge), + }); + if ("needsReview" in outcome) { + const preview = outcome.needsReview; return { ok: false, error: @@ -81,12 +83,7 @@ export async function importPackageHandler( .map((row) => ({ flag: row.flag, title: row.title, detail: row.detail })), }; } - - const result = await importPackageUnattended(plugin, pkg, { - choice: (exists) => - effectiveChoiceMode(choiceMode ?? (exists ? "overwrite" : "import"), exists), - asset: (exists) => reconcileMode(assetMode ?? (exists ? "overwrite" : "write"), exists), - }); + const result = outcome.imported; // Report final ids: a duplicated choice gets a fresh id during apply. const updatedById = new Map( diff --git a/src/main.ts b/src/main.ts index d231b3808..ac505ce67 100644 --- a/src/main.ts +++ b/src/main.ts @@ -38,6 +38,7 @@ import { childChoicesOf, clearEmptyFormatFlag, dedupeChoicesById, + findChoiceWithPath, isChoiceLike, resolveChoiceIcon, rootChoicesOf, @@ -635,29 +636,8 @@ export default class QuickAdd extends Plugin { return choice; } - private getChoice( - by: "name" | "id", - targetPropertyValue: string, - choices: IChoice[] = this.settings.choices, - ): IChoice | null { - for (const choice of rootChoicesOf(choices)) { - if (!isChoiceLike(choice)) continue; - if (choice[by] === targetPropertyValue) { - return choice; - } - if (choice.type === "Multi") { - const subChoice = this.getChoice( - by, - targetPropertyValue, - childChoicesOf(choice), - ); - if (subChoice) { - return subChoice; - } - } - } - - return null; + private getChoice(by: "name" | "id", value: string): IChoice | null { + return findChoiceWithPath(this.settings.choices, by, value)?.choice ?? null; } /** Count how many choices anywhere in the tree carry `name` (names aren't unique). diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index f952ccbb0..81ebd0039 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -88,13 +88,13 @@ export class QuickAddApi { ) => { // Report and return rather than throw: a missing choice must not // abort the calling macro. - const choice = findChoice(plugin.settings.choices, ref); + const choice = findChoice(plugin.settings.choices, ref)?.choice; if (!choice) { reportError( new Error( - typeof ref === "string" - ? `Choice named '${ref}' not found` - : `Choice with id '${String(ref?.id)}' not found`, + typeof ref === "object" && ref !== null + ? `Choice with id '${String(ref.id)}' not found` + : `Choice named '${String(ref)}' not found`, ), "API executeChoice error", ); @@ -237,8 +237,8 @@ export class QuickAddApi { executeChoice: (...args: Parameters) => handledIfCancelled(executeChoice(...args)), getChoice: (ref: ChoiceRef): ChoiceSummary | null => { - const choice = findChoice(plugin.settings.choices, ref); - return choice ? summarizeChoice(plugin.settings.choices, choice) : null; + const found = findChoice(plugin.settings.choices, ref); + return found ? summarizeChoice(found) : null; }, pickChoice: (options?: { placeholder?: string }) => handledIfCancelled(pickChoice(plugin, options)), diff --git a/src/services/packageImportService.ts b/src/services/packageImportService.ts index c84f83821..70756bb5a 100644 --- a/src/services/packageImportService.ts +++ b/src/services/packageImportService.ts @@ -226,7 +226,7 @@ export async function analysePackagePreview( * out-of-bounds destination so the whole import aborts (surfaced as a Notice by * ImportPackageModal). Defensive: imported packages are untrusted shared data. */ -function validateAssetDestination(rawPath: string): string { +export function validateAssetDestination(rawPath: string): string { const rawDestination = rawPath ?? ""; const trimmedDestination = rawDestination.trim(); if (!trimmedDestination) { diff --git a/src/services/packageImportUnattended.ts b/src/services/packageImportUnattended.ts index 0ebf57638..e0d974aa2 100644 --- a/src/services/packageImportUnattended.ts +++ b/src/services/packageImportUnattended.ts @@ -3,7 +3,6 @@ import { settingsStore } from "../settingsStore"; import type { AssetConflict } from "../gui/PackageManager/importDecisions"; import { defaultAssetDestinationFor } from "../gui/PackageManager/importDecisions"; import type { QuickAddPackage } from "../types/packages/QuickAddPackage"; -import { escapesVaultBoundary } from "../utils/vaultPathBoundary"; import { syncImportedChoiceCommands } from "./packageImportCommands"; import type { ApplyImportResult, @@ -12,14 +11,28 @@ import type { ChoiceImportDecision, ChoiceImportMode, } from "./packageImportService"; -import { analysePackage, applyPackageImport } from "./packageImportService"; +import { + analysePackage, + analysePackagePreview, + applyPackageImport, + validateAssetDestination, +} from "./packageImportService"; +import type { PackagePreview } from "./packagePreview"; +import { requiresAcknowledgement } from "./packagePreview"; /** What an import with nobody to ask does with each choice and file, by whether it is already in the vault. */ -export interface UnattendedImportModes { +export interface UnattendedImportOptions { choice: (exists: boolean) => ChoiceImportMode; asset: (exists: boolean) => AssetImportMode; + /** The user already reviewed the package's code another way, as the CLI's acknowledge=true says. */ + acknowledged?: boolean; } +/** The import's result, or the preview of a package that needs the review modal, in which case nothing changed. */ +export type UnattendedImportOutcome = + | { imported: ApplyImportResult } + | { needsReview: PackagePreview }; + /** * Where each bundled file lands when nobody edits the destination. The modal * lets the reader rename a colliding file; an unattended import cannot, so two @@ -59,17 +72,16 @@ let importQueue: Promise = Promise.resolve(); /** * The import modal's apply step with every decision made up front: the same - * analysis, the same default destinations (templates land in the template - * folder) and the same apply, then the choices are stored, their commands - * synced and the settings saved. The trust gate is the caller's: check - * `requiresAcknowledgement` first. + * trust gate, the same analysis, the same default destinations (templates land + * in the template folder) and the same apply, then the choices are stored, + * their commands synced and the settings saved. */ export function importPackageUnattended( plugin: QuickAdd, pkg: QuickAddPackage, - modes: UnattendedImportModes, -): Promise { - const run = importQueue.then(() => importNow(plugin, pkg, modes)); + options: UnattendedImportOptions, +): Promise { + const run = importQueue.then(() => importNow(plugin, pkg, options)); // A failed import must not poison the queue for the next one. importQueue = run.catch(() => undefined); return run; @@ -78,14 +90,18 @@ export function importPackageUnattended( async function importNow( plugin: QuickAdd, pkg: QuickAddPackage, - modes: UnattendedImportModes, -): Promise { + options: UnattendedImportOptions, +): Promise { const { app } = plugin; const previousChoices = settingsStore.getState().choices; + const preview = await analysePackagePreview(app, previousChoices, pkg); + if (requiresAcknowledgement(preview) && !options.acknowledged) { + return { needsReview: preview }; + } const analysis = await analysePackage(app, previousChoices, pkg); const choiceDecisions: ChoiceImportDecision[] = analysis.choiceConflicts.map( - (conflict) => ({ choiceId: conflict.choiceId, mode: modes.choice(conflict.exists) }), + (conflict) => ({ choiceId: conflict.choiceId, mode: options.choice(conflict.exists) }), ); const destinations = defaultAssetDestinations( @@ -94,19 +110,16 @@ async function importNow( ); const assetDecisions: AssetImportDecision[] = []; for (const conflict of analysis.assetConflicts) { - const destinationPath = - destinations.get(conflict.originalPath) ?? conflict.originalPath; - // applyPackageImport rejects a destination outside the vault; do not - // stat it first (the package, and so this path, is untrusted input). - const exists = - destinationPath === conflict.originalPath - ? conflict.exists - : !escapesVaultBoundary(destinationPath) && - (await app.vault.adapter.exists(destinationPath)); + // Ask about the path the write will use, so a padded spelling cannot + // dodge a skip. Validation also refuses a path outside the vault before + // anything stats it. + const destinationPath = validateAssetDestination( + destinations.get(conflict.originalPath) ?? conflict.originalPath, + ); assetDecisions.push({ originalPath: conflict.originalPath, destinationPath, - mode: modes.asset(exists), + mode: options.asset(await app.vault.adapter.exists(destinationPath)), }); } @@ -122,5 +135,5 @@ async function importNow( settingsStore.setState((state) => ({ ...state, choices: result.updatedChoices })); syncImportedChoiceCommands(plugin, previousChoices, result); await plugin.saveSettings(); - return result; + return { imported: result }; } diff --git a/src/utils/choiceUtils.ts b/src/utils/choiceUtils.ts index c44de35cf..14e533773 100644 --- a/src/utils/choiceUtils.ts +++ b/src/utils/choiceUtils.ts @@ -333,3 +333,16 @@ export function flattenChoicesWithPath( } return result; } + +/** + * The first choice whose `name` or `id` is `value`, in the pre-order the + * settings list shows, with its path. Names are not unique, so a name means + * the first match. + */ +export function findChoiceWithPath( + choices: IChoice[], + by: "name" | "id", + value: string, +): FlatChoicePathEntry | undefined { + return flattenChoicesWithPath(choices).find((entry) => entry.choice[by] === value); +}