diff --git a/docs/src/content/docs/docs/Advanced/CLI.md b/docs/src/content/docs/docs/Advanced/CLI.md index 556d36410..a0c80bbfa 100644 --- a/docs/src/content/docs/docs/Advanced/CLI.md +++ b/docs/src/content/docs/docs/Advanced/CLI.md @@ -151,6 +151,40 @@ acknowledgement, `form` → an object mapping each field's `id` to its string value (date fields use the `@date:ISO` format). The run's outcome arrives as the `done`/`error` poll event. +### Cancelling + +`{"cancelled":true}` is how you say *the user dismissed this prompt*. It aborts +the run exactly as pressing Escape on the in-app dialog does. + +Do not use it just to close an `info` panel. `info` is the one prompt that cannot +be cancelled in the app - the dialog has no reject path - so cancelling it +remotely aborts a run that would have continued. Send a plain reply instead. It +stays cancellable because it is a client's only explicit way to bail out mid-run. + +### When a reply is rejected + +:::note[Available in the next release] +The `400`-and-retry semantics below are new. Before them, a `cancelled` flag that was not the +literal `true` was consumed as a cancellation, and a `confirm` prompt with no value was read as +"No". +::: + +`/reply` answers `400` and leaves the prompt **pending** when it cannot honour +what you sent, so you can correct the reply and POST again. Two cases: + +- `cancelled` is present but is not a boolean (`"true"`, `1`, `"no"`). QuickAdd + will neither abort on a flag it does not recognise nor quietly answer the prompt + on the user's behalf, so it asks you to fix the flag. Use the literal `true`; + `false` and omitting it both mean "this is a real answer". +- a `confirm` reply whose `value` is not `true`/`false`. The user never answered, + and QuickAdd will not invent a "No" for them. + +Every other prompt type accepts whatever you send, including an empty answer: +`""` and `[]` are things a user genuinely submits in the app (the Skip buttons, +optional fields), so they must stay legal here too. + +A `409` means nothing was waiting on that `requestId`. + Good to know: - **Desktop only.** The bridge binds to `127.0.0.1`, is gated by the per-session `token`, rejects browser (`Origin`/`Referer`) and non-loopback `Host` requests, and the server is ephemeral - it starts on the first session and stops when the last one ends. diff --git a/docs/src/content/docs/docs/QuickAddAPI.md b/docs/src/content/docs/docs/QuickAddAPI.md index 1da98ebb1..8e91917b9 100644 --- a/docs/src/content/docs/docs/QuickAddAPI.md +++ b/docs/src/content/docs/docs/QuickAddAPI.md @@ -936,29 +936,47 @@ module.exports = async (params) => { ## Error Handling Best Practices -Always wrap API calls in try-catch blocks: +:::note[Available in the next release] +Until then, a prompt that failed for a reason other than the user cancelling resolved +`undefined` instead of rejecting, so a script could not tell a failure from an empty answer. +::: + +A prompt has exactly two non-happy outcomes, and they are different things: + +- **The user dismissed it** (Escape, Cancel, closing the dialog). The promise rejects with `MacroAbortError("Input cancelled by user")`. Let it bubble and QuickAdd stops the run quietly - which is almost always what you want. +- **Something broke** (a bug in your script, a vault error). The promise rejects with that error. Let it bubble and QuickAdd reports it once, so you get a notice and a console entry to debug from. + +A prompt never resolves `undefined` to mean either of those. An empty string means the user submitted an empty answer, which is a real answer. ```javascript module.exports = async (params) => { const { quickAddApi } = params; - + + // The simplest correct script: no try/catch at all. A dismissal stops the + // macro; a real failure is reported. Only catch if you need to do something + // in between. + const input = await quickAddApi.inputPrompt("Enter value:"); + if (input === "") return; // submitted empty on purpose + + // Process input... +}; +``` + +If your script does need to react to a cancellation, tell the two apart by the error name: + +```javascript +module.exports = async (params) => { + const { quickAddApi } = params; + try { const input = await quickAddApi.inputPrompt("Enter value:"); - - if (!input) { - // User cancelled - handle gracefully - return; - } - // Process input... - } catch (error) { - console.error("Script error:", error); - - await quickAddApi.infoDialog( - "Error", - `An error occurred: ${error.message}` - ); + if (error?.name === "MacroAbortError") { + // The user backed out. Clean up and stop. + return; + } + throw error; // a real failure - let QuickAdd report it } }; ``` diff --git a/src/engine/MacroChoiceEngine.entry.test.ts b/src/engine/MacroChoiceEngine.entry.test.ts index 937f0f23d..439e63758 100644 --- a/src/engine/MacroChoiceEngine.entry.test.ts +++ b/src/engine/MacroChoiceEngine.entry.test.ts @@ -874,11 +874,14 @@ describe("QuickAddApi prompt cancellation", () => { ).rejects.toThrow(MacroAbortError); }); - it("still resolves undefined for other prompt errors", async () => { - mockInputPrompt.mockRejectedValueOnce(new Error("boom")); - - await expect( - QuickAddApi.inputPrompt(app, "Enter value"), - ).resolves.toBeUndefined(); + // A genuine failure must NOT come back as "the user gave no input" (#1575): + // the script would carry on and act on a value that was never entered. + it("propagates other prompt errors instead of resolving undefined", async () => { + const failure = new Error("boom"); + mockInputPrompt.mockRejectedValueOnce(failure); + + await expect(QuickAddApi.inputPrompt(app, "Enter value")).rejects.toBe( + failure, + ); }); }); diff --git a/src/engine/runTemplateFromFolder.test.ts b/src/engine/runTemplateFromFolder.test.ts index a7c1c6071..cc66b33a5 100644 --- a/src/engine/runTemplateFromFolder.test.ts +++ b/src/engine/runTemplateFromFolder.test.ts @@ -4,6 +4,8 @@ import type QuickAdd from "../main"; import type { IChoiceExecutor } from "../IChoiceExecutor"; import type ITemplateChoice from "../types/choices/ITemplateChoice"; import { VALUE_SYNTAX } from "../constants"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "../logger/logManager"; vi.mock("../gui/GenericSuggester/genericSuggester", () => ({ default: { Suggest: vi.fn() }, @@ -171,10 +173,16 @@ describe("runTemplateFromFolder", () => { expect(choice.templatePath).toBe("Templates/Daily.md"); }); - it("does not execute when the picker is cancelled", async () => { + // The `isCancellationError -> return null` shape, driven with what the picker + // ACTUALLY rejects with since #1577. The legacy string is kept as a second case + // because a user script can still throw one and the contract honours that. + it.each([ + ["a typed dismissal", () => promptCancelled()], + ["a legacy sentinel a user script may throw", () => "no input given."], + ])("does not execute when the picker is cancelled with %s", async (_label, make) => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); const executor = createExecutor(); - // GenericSuggester rejects with this exact reason on dismissal. - suggestMock.mockRejectedValueOnce("no input given."); + suggestMock.mockRejectedValueOnce(make()); await runTemplateFromFolder( app, createPlugin({ @@ -184,6 +192,26 @@ describe("runTemplateFromFolder", () => { { choiceExecutor: executor }, ); expect(executor.execute).not.toHaveBeenCalled(); + // Quietly: backing out of the picker is not an error to report. + expect(logError).not.toHaveBeenCalled(); + logError.mockRestore(); + }); + + it("reports a genuine picker failure instead of failing silently", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + const executor = createExecutor(); + suggestMock.mockRejectedValueOnce(new Error("index unavailable")); + await runTemplateFromFolder( + app, + createPlugin({ + templateFolderPaths: ["Templates"], + templateFiles: [tfile("Templates/Daily.md")], + }), + { choiceExecutor: executor }, + ); + expect(executor.execute).not.toHaveBeenCalled(); + expect(logError).toHaveBeenCalledTimes(1); + logError.mockRestore(); }); }); diff --git a/src/errors/UserCancelError.ts b/src/errors/UserCancelError.ts index 6947c1e96..2780ece03 100644 --- a/src/errors/UserCancelError.ts +++ b/src/errors/UserCancelError.ts @@ -14,3 +14,27 @@ import { MacroAbortError } from "./MacroAbortError"; * error name continue to recognise it. */ export class UserCancelError extends MacroAbortError {} + +/** + * The one message every prompt uses when the user dismisses it. Matches the text the + * public docs promise (`MacroAbortError("Input cancelled by user")`), so a script that + * surfaces `error.message` reads the same whether the prompt was dismissed in the app, + * dismissed remotely, or converted from a legacy sentinel on the way up. + */ +export const PROMPT_CANCELLED_MESSAGE = "Input cancelled by user"; + +/** + * The cancellation a prompt throws when the user dismisses it (Escape / Cancel / close). + * + * Prompts used to reject with a bare English sentence, recognised by exact string + * equality (see the legacy sentinel list in `errorUtils`) — so rewording one, or + * adding a prompt with a slightly different + * sentence, silently turned "the user cancelled" into "an error occurred" with no compiler + * or test signal (#1577). This is the typed replacement, and it is deliberately the SAME + * class the ~40 consumers already converted that string into, rather than a new one: a + * dismissal now arrives already classified, so a consumer that forgets to convert it still + * behaves correctly. + */ +export function promptCancelled(): UserCancelError { + return new UserCancelError(PROMPT_CANCELLED_MESSAGE); +} diff --git a/src/formatters/previewDiagnostics.ts b/src/formatters/previewDiagnostics.ts index da605ebd2..6c2b6b272 100644 --- a/src/formatters/previewDiagnostics.ts +++ b/src/formatters/previewDiagnostics.ts @@ -74,14 +74,18 @@ function stripBrandPrefix(message: string): string { export function describePreviewFailure(error: unknown): string | null { // A cancelled prompt is a user action, not an authoring mistake. The preview // formatters never prompt, but a nested resolver still can. Check the RAW - // value first: QuickAdd's modals reject with a bare string - // (`rejectPromise("No input given.")`), and `isCancellationError` only ever - // matches strings - so an `instanceof Error` gate ahead of it would let every - // cancellation through as "Preview unavailable". + // value first, before the `instanceof Error` gate below: a dismissal is a + // typed UserCancelError (#1577), and letting it reach that gate would report + // every cancellation as "Preview unavailable". if (isCancellationError(error)) return null; if (!(error instanceof Error)) return PREVIEW_FAILED_MESSAGE; const message = error.message ?? ""; - // Also covers a cancellation that was wrapped in an Error on the way up. + // Covers a USER SCRIPT that throws an Error whose message is one of the legacy + // sentinels (see `isCancellationError`) - the shape a script written against the + // pre-#1577 strings still uses. It deliberately does NOT cover + // `toError(err, context)`, which prefixes the context and so matches no sentinel; + // adding the canonical message to that set would not fix it either, and nothing + // wraps on the way into this function. if (isCancellationError(message)) return null; // By far the most frequent throw: the token autocomplete inserts `{{VALUE:}}` // with the caret between the colon and the braces, and VARIABLE_REGEX matches diff --git a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts index 228705a96..ddb13e61f 100644 --- a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts +++ b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.ts @@ -1,5 +1,6 @@ import type { App } from "obsidian"; import { beforeAll, describe, expect, it, vi } from "vitest"; +import { UserCancelError } from "../../errors/UserCancelError"; // Mock obsidian with a local Modal/ButtonComponent/ToggleComponent so // super.onClose() resolves (the shared stub's Modal omits onClose on the @@ -161,7 +162,7 @@ describe("GenericCheckboxPrompt header + cancel (audit: prompts-gui-checkbox-pro expect(() => buttonByText(prompt, "Cancel")).not.toThrow(); }); - it("Cancel rejects the promise (no input given)", async () => { + it("Cancel rejects the promise with a typed cancellation", async () => { const prompt = new GenericCheckboxPrompt(app, ["a", "b"], ["a"]); const promise = prompt.promise; @@ -169,7 +170,7 @@ describe("GenericCheckboxPrompt header + cancel (audit: prompts-gui-checkbox-pro new Event("click", { bubbles: true }), ); - await expect(promise).rejects.toBe("no input given."); + await expect(promise).rejects.toBeInstanceOf(UserCancelError); }); it("Submit resolves the selected items", async () => { diff --git a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts index d2e4d4bd2..5a8087b91 100644 --- a/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts +++ b/src/gui/GenericCheckboxPrompt/genericCheckboxPrompt.ts @@ -1,5 +1,6 @@ import type { App } from "obsidian"; import { ButtonComponent, Modal, ToggleComponent } from "obsidian"; +import { promptCancelled } from "../../errors/UserCancelError"; export default class GenericCheckboxPrompt extends Modal { private resolvePromise: (value: string[]) => void; @@ -54,7 +55,7 @@ export default class GenericCheckboxPrompt extends Modal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private addCheckboxRows() { diff --git a/src/gui/GenericInputPrompt/GenericInputPrompt.ts b/src/gui/GenericInputPrompt/GenericInputPrompt.ts index 876b99136..5d653c90b 100644 --- a/src/gui/GenericInputPrompt/GenericInputPrompt.ts +++ b/src/gui/GenericInputPrompt/GenericInputPrompt.ts @@ -8,6 +8,7 @@ import { positionInputPromptCursor } from "../inputPromptCursor"; import { renderPromptContextLine } from "../promptContextLine"; import type { ImagePasteHandle } from "../imagePasteHandler"; import { attachImagePasteHandler } from "../imagePasteHandler"; +import { promptCancelled } from "../../errors/UserCancelError"; /** * The keyboard gesture that skips an optional prompt: ctrl/cmd+shift+Enter. @@ -290,7 +291,7 @@ export default class GenericInputPrompt extends Modal { } private resolveInput() { - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(this.input); } diff --git a/src/gui/GenericSuggester/genericSuggester.ts b/src/gui/GenericSuggester/genericSuggester.ts index 7536f748a..b72662a12 100644 --- a/src/gui/GenericSuggester/genericSuggester.ts +++ b/src/gui/GenericSuggester/genericSuggester.ts @@ -6,6 +6,7 @@ import { normalizeDisplayItem, normalizeQuery, } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; type SuggestRender = (value: T, el: HTMLElement) => void; @@ -160,7 +161,7 @@ export default class GenericSuggester extends FuzzySuggestModal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private warnIfEmptyDisplay(): void { diff --git a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts index 627e1e037..a06aa9a4e 100644 --- a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts +++ b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.ts @@ -3,6 +3,7 @@ import { Modal } from "obsidian"; import type QuickAdd from "../../main"; import { setQuickAddInstance } from "../../quickAddInstance"; import GenericWideInputPrompt from "./GenericWideInputPrompt"; +import { UserCancelError } from "../../errors/UserCancelError"; // The obsidian-stub Modal does not implement onOpen/onClose; the prompt calls // super.onOpen()/super.onClose(). Provide no-ops so construction and close do not @@ -220,7 +221,7 @@ describe("image paste submit/cancel races (issue #1484)", () => { ).find((button) => button.textContent === "Cancel") as HTMLButtonElement; cancelButton.click(); - await expect(waitForClose).rejects.toBe("No input given."); + await expect(waitForClose).rejects.toBeInstanceOf(UserCancelError); // The save landing later must NOT resurrect the submit on the closed // modal (deferred submit is guarded by didClose). diff --git a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts index 12d0e39da..38fc9f48a 100644 --- a/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts +++ b/src/gui/GenericWideInputPrompt/GenericWideInputPrompt.ts @@ -10,6 +10,7 @@ import { attachTextareaIndent } from "../components/textareaIndent"; import { isSkipPromptShortcut } from "../GenericInputPrompt/GenericInputPrompt"; import type { ImagePasteHandle } from "../imagePasteHandler"; import { attachImagePasteHandler } from "../imagePasteHandler"; +import { promptCancelled } from "../../errors/UserCancelError"; export default class GenericWideInputPrompt extends Modal { public waitForClose: Promise; @@ -281,7 +282,7 @@ export default class GenericWideInputPrompt extends Modal { } private resolveInput() { - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(this.input); } diff --git a/src/gui/InputSuggester/inputSuggester.ts b/src/gui/InputSuggester/inputSuggester.ts index ee41f5001..a16bd4511 100644 --- a/src/gui/InputSuggester/inputSuggester.ts +++ b/src/gui/InputSuggester/inputSuggester.ts @@ -6,6 +6,7 @@ import { normalizeDisplayItem, normalizeQuery, } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; type SuggestRender = (value: T, el: HTMLElement) => void; @@ -275,7 +276,7 @@ export default class InputSuggester extends FuzzySuggestModal { onClose() { super.onClose(); - if (!this.resolved) this.rejectPromise("no input given."); + if (!this.resolved) this.rejectPromise(promptCancelled()); } private warnIfEmptyDisplay(): void { diff --git a/src/gui/MathModal.ts b/src/gui/MathModal.ts index fc1599189..759110f85 100644 --- a/src/gui/MathModal.ts +++ b/src/gui/MathModal.ts @@ -10,6 +10,7 @@ import { import { getQuickAddInstance } from "../quickAddInstance"; import { LATEX_CURSOR_MOVE_HERE } from "../LaTeXSymbols"; import { LaTeXSuggester } from "./suggesters/LaTeXSuggester"; +import { promptCancelled } from "../errors/UserCancelError"; export class MathModal extends Modal { public waitForClose: Promise; @@ -154,7 +155,7 @@ export class MathModal extends Modal { const output = this.inputEl.value .replace(/\\n/g, `\\\\n`) .replace(new RegExp(LATEX_CURSOR_MOVE_HERE, "g"), ""); - if (!this.didSubmit) this.rejectPromise("No input given."); + if (!this.didSubmit) this.rejectPromise(promptCancelled()); else this.resolvePromise(output); } diff --git a/src/gui/MultiChoiceSettingsModal.ts b/src/gui/MultiChoiceSettingsModal.ts index ebf9e2693..6901809d2 100644 --- a/src/gui/MultiChoiceSettingsModal.ts +++ b/src/gui/MultiChoiceSettingsModal.ts @@ -2,6 +2,7 @@ import type { App } from "obsidian"; import { ButtonComponent, Modal, Setting } from "obsidian"; import type IMultiChoice from "../types/choices/IMultiChoice"; import { addChoiceIconSetting } from "./ChoiceBuilder/components/choiceIconSetting"; +import { promptCancelled } from "../errors/UserCancelError"; export class MultiChoiceSettingsModal extends Modal { public waitForClose: Promise; @@ -88,7 +89,7 @@ export class MultiChoiceSettingsModal extends Modal { onClose() { super.onClose(); if (!this.didSubmit) { - this.rejectPromise("No input given."); + this.rejectPromise(promptCancelled()); return; } diff --git a/src/gui/MultiSuggester/multiSuggester.ts b/src/gui/MultiSuggester/multiSuggester.ts index 771d80577..25e1a40f0 100644 --- a/src/gui/MultiSuggester/multiSuggester.ts +++ b/src/gui/MultiSuggester/multiSuggester.ts @@ -1,6 +1,7 @@ import type { App } from "obsidian"; import { Modal, Notice, Setting } from "obsidian"; import { normalizeDisplayItem } from "../suggesters/utils"; +import { promptCancelled } from "../../errors/UserCancelError"; export interface MultiSuggesterOptions { /** Modal title / prompt header. */ @@ -237,7 +238,7 @@ export default class MultiSuggester extends Modal { if (this.didSubmit) { this.resolvePromise(this.skipped ? [] : this.collectResult()); } else { - this.rejectPromise("no input given."); + this.rejectPromise(promptCancelled()); } } } diff --git a/src/gui/choiceRename.test.ts b/src/gui/choiceRename.test.ts index 856069986..486c8c7e5 100644 --- a/src/gui/choiceRename.test.ts +++ b/src/gui/choiceRename.test.ts @@ -19,6 +19,8 @@ vi.mock("./GenericInputPrompt/GenericInputPrompt", () => ({ import type { App } from "obsidian"; import { promptRenameChoice } from "./choiceRename"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "src/logger/logManager"; const app = {} as App; @@ -66,4 +68,28 @@ describe("promptRenameChoice", () => { null, ); }); + + // Since #1577 a dismissal IS an Error, so the previous `instanceof Error` gate + // would report every cancelled rename as a failure - a 15s error notice plus an + // error-log entry for pressing Escape. Nothing caught that, so pin both halves. + describe("dismissal versus failure", () => { + it("returns null and logs nothing when the user dismisses the prompt", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + promptSpy.mockRejectedValue(promptCancelled()); + + await expect(promptRenameChoice(app, "Old", "Macro")).resolves.toBeNull(); + expect(logError).not.toHaveBeenCalled(); + logError.mockRestore(); + }); + + it("returns null but DOES log when the prompt genuinely fails", async () => { + const logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + promptSpy.mockRejectedValue(new Error("boom")); + + await expect(promptRenameChoice(app, "Old", "Macro")).resolves.toBeNull(); + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain("boom"); + logError.mockRestore(); + }); + }); }); diff --git a/src/gui/choiceRename.ts b/src/gui/choiceRename.ts index aaa952466..69a69f9dd 100644 --- a/src/gui/choiceRename.ts +++ b/src/gui/choiceRename.ts @@ -1,6 +1,7 @@ import type { App } from "obsidian"; import GenericInputPrompt from "./GenericInputPrompt/GenericInputPrompt"; import { log } from "src/logger/logManager"; +import { isCancellationError, toError } from "../utils/errorUtils"; import type { ChoiceType } from "../types/choices/choiceType"; import { choiceNounCapitalized } from "../utils/choiceNoun"; @@ -29,10 +30,11 @@ export async function promptRenameChoice( if (!trimmed || trimmed === currentName) return null; return trimmed; } catch (error) { - // GenericInputPrompt rejects with a string ("No input given.") when the - // user cancels (Esc/Cancel) — that is expected, not an error. Surface only - // genuine failures (Error instances) instead of swallowing them silently. - if (error instanceof Error) log.logError(error); + // A dismissal (Esc/Cancel) is expected, not an error. Ask the cancellation + // contract rather than gating on `instanceof Error`: since #1577 a dismissal + // IS an Error, so that gate would report every cancelled rename as a failure. + if (isCancellationError(error)) return null; + log.logError(toError(error, "Could not rename")); return null; } } diff --git a/src/interactive/interactivePromptServer.test.ts b/src/interactive/interactivePromptServer.test.ts index 94e3a6ae1..417befe0c 100644 --- a/src/interactive/interactivePromptServer.test.ts +++ b/src/interactive/interactivePromptServer.test.ts @@ -3,9 +3,25 @@ import { interactivePromptServer, isLoopbackClient, safeEqual, + type ReplyBody, } from "./interactivePromptServer"; import { UserCancelError } from "../errors/UserCancelError"; +/** + * A reply as it arrives from a client: validated, then delivered. Goes through the + * same method the HTTP handler calls, so the validation under test is production's. + */ +function replyOverWire( + sessionId: string, + body: ReplyBody, +): { status: number; body: { error?: string } } { + const outcome = interactivePromptServer.submitWireReply(sessionId, body); + return outcome.ok + ? { status: 200, body: {} } + : { status: outcome.status, body: { error: outcome.error } }; +} + + afterEach(() => { vi.useRealTimers(); }); @@ -172,6 +188,124 @@ describe("interactivePromptServer session multiplexing", () => { interactivePromptServer.finish(s.id, { kind: "done", result: {} }); }); + // The flag arrives from untrusted JSON. A loose truthy check let + // `"cancelled": "no"` kill a run; strict `=== true` alone would be worse - it would + // resolve a mistyped cancel as an EMPTY ANSWER (textPrompt maps undefined -> ""), + // handing the run a value the user never gave. So the wire rejects it and the prompt + // stays pending. Note the value-less shape, which is what a mistaken cancel has. + it.each([["no"], [1], [{}], ["true"], [[]], [null]])( + "rejects a non-boolean cancelled flag (%o) with 400 and keeps the prompt pending", + async (flag) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + let settled = false; + void prompt.then( + () => (settled = true), + () => (settled = true), + ); + const rid = pendingRequestId(s.id); + + const outcome = replyOverWire(s.id, { requestId: rid, cancelled: flag }); + + expect(outcome.status).toBe(400); + expect(String(outcome.body.error)).toMatch(/cancelled/i); + await Promise.resolve(); + expect(settled, "the prompt must stay pending so the client can retry").toBe( + false, + ); + + // ...and the corrected reply still works. + expect( + replyOverWire(s.id, { requestId: rid, cancelled: true }).status, + ).toBe(200); + await expect(prompt).rejects.toBeInstanceOf(UserCancelError); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + + it("accepts an explicit cancelled:false as a normal answer", async () => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + const rid = pendingRequestId(s.id); + expect( + replyOverWire(s.id, { requestId: rid, value: "Ada", cancelled: false }).status, + ).toBe(200); + await expect(prompt).resolves.toBe("Ada"); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + + // A confirm reply that is neither boolean nor a cancel is validated HERE, while the + // client is still holding the HTTP response, because deeper in the only options are + // to invent an answer or to fail the run with a message the Template/Capture path + // replaces with a generic sentence the client never sees. + it.each([[undefined], [null], ["yes"], [0], [{}]])( + "rejects a non-boolean confirm reply (%o) with 400", + async (value) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "confirm", + header: "Proceed?", + }); + const rid = pendingRequestId(s.id); + + const outcome = replyOverWire(s.id, { requestId: rid, value }); + + expect(outcome.status).toBe(400); + expect(String(outcome.body.error)).toContain("confirm prompt needs a boolean"); + expect(String(outcome.body.error)).toContain('{"cancelled": true}'); + + expect(replyOverWire(s.id, { requestId: rid, value: false }).status).toBe(200); + await expect(prompt).resolves.toBe(false); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + + it.each([[true], [false], ["true"], ["false"]])( + "accepts a boolean-ish confirm reply (%o)", + async (value) => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "confirm", + header: "Proceed?", + }); + const rid = pendingRequestId(s.id); + expect(replyOverWire(s.id, { requestId: rid, value }).status).toBe(200); + await expect(prompt).resolves.toBe(value); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }, + ); + + it("still 409s an unknown requestId", () => { + const s = interactivePromptServer.createSession(); + const outcome = replyOverWire(s.id, { requestId: "nope", value: "x" }); + expect(outcome.status).toBe(409); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + + // Leniency preserved on purpose: "" and [] are answers a user really gives in-app + // via the Skip affordances and optional fields, so an empty reply must stay legal or + // optional prompts break on remote runs. + it("leaves an empty answer to a non-confirm prompt alone", async () => { + const s = interactivePromptServer.createSession(); + const prompt = interactivePromptServer.emitPrompt(s.id, { + type: "input", + header: "Name", + multiline: false, + }); + const rid = pendingRequestId(s.id); + expect(replyOverWire(s.id, { requestId: rid, value: null }).status).toBe(200); + await expect(prompt).resolves.toBeNull(); + interactivePromptServer.finish(s.id, { kind: "done", result: {} }); + }); + it("caps the number of concurrent sessions", () => { const created: string[] = []; try { diff --git a/src/interactive/interactivePromptServer.ts b/src/interactive/interactivePromptServer.ts index d54243445..795d504b1 100644 --- a/src/interactive/interactivePromptServer.ts +++ b/src/interactive/interactivePromptServer.ts @@ -17,7 +17,10 @@ * session is cleaned up, so nothing listens when no interactive run is active. */ -import { UserCancelError } from "../errors/UserCancelError"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, +} from "../errors/UserCancelError"; // Minimal structural types for the slice of Node's `http` we use, declared // locally so this module never statically imports a Node builtin (keeps the @@ -139,7 +142,12 @@ interface Session { waiterTimer: number | null; pending: Map< string, - { resolve: (value: unknown) => void; reject: (error: Error) => void } + { + resolve: (value: unknown) => void; + reject: (error: Error) => void; + /** What kind of reply this prompt accepts, for wire-level validation. */ + promptType: PromptSpec["type"]; + } >; finished: boolean; cleanupTimer: number | null; @@ -212,6 +220,67 @@ function randomId(): string { ); } +/** Body of a `POST /reply`. Every field is untrusted JSON, hence the `unknown`s. */ +export interface ReplyBody { + requestId?: string; + value?: unknown; + cancelled?: unknown; +} + +export type ReplyOutcome = + | { ok: true } + | { ok: false; status: 400 | 409; error: string }; + +const NO_PENDING_PROMPT = "No pending prompt for that requestId"; + +/** + * Why this reply cannot be honoured, or null if it can. + * + * Validating here rather than deeper in is what makes a malformed reply safe: the + * client is still waiting on the HTTP response, so it gets an actionable 400 and the + * prompt stays pending. Deeper in, the only options are to invent an answer (which + * is the bug this seam had) or to fail the whole run with a message the client may + * never see - the Template/Capture path replaces it with a generic sentence. + * + * Only two things are checked, both because a wrong answer there is + * indistinguishable from a real one. Empty answers stay legal everywhere else: + * `""` and `[]` are things a user genuinely submits in-app via the Skip + * affordances and optional fields, so rejecting them would break optional prompts + * on remote runs. + */ +function describeReplyProblem( + promptType: PromptSpec["type"], + body: ReplyBody, +): string | null { + if (body.cancelled !== undefined && typeof body.cancelled !== "boolean") { + return 'The "cancelled" flag must be the literal true (or omitted). A cancel that is not recognised would otherwise be answered on the user\'s behalf.'; + } + if (body.cancelled === true) return null; + + if (promptType === "confirm") { + const value = body.value; + const isBoolean = + value === true || + value === false || + value === "true" || + value === "false"; + if (!isBoolean) { + return `A confirm prompt needs a boolean reply, or {"cancelled": true} if the user dismissed it. Got ${describeValue(value)}.`; + } + } + return null; +} + +/** A short, safe rendering of a bad reply value for an error message. */ +function describeValue(value: unknown): string { + if (value === undefined) return "no value"; + if (value === null) return "null"; + if (typeof value === "string") return JSON.stringify(value.slice(0, 40)); + if (typeof value === "number" || typeof value === "boolean") + return String(value); + return Array.isArray(value) ? "an array" : typeof value; +} + class InteractivePromptServer { private server: HttpServer | null = null; private port = 0; @@ -305,7 +374,11 @@ class InteractivePromptServer { } const requestId = randomId(); return new Promise((resolve, reject) => { - session.pending.set(requestId, { resolve, reject }); + session.pending.set(requestId, { + resolve, + reject, + promptType: prompt.type, + }); this.push(session, { kind: "prompt", requestId, prompt }); }); } @@ -426,7 +499,7 @@ class InteractivePromptServer { sessionId: string, requestId: string, value: unknown, - cancelled = false, + cancelled: unknown = false, ): boolean { const session = this.sessions.get(sessionId); if (!session) return false; @@ -436,12 +509,25 @@ class InteractivePromptServer { // A remote cancel must abort the run exactly like dismissing the Obsidian // modal: reject with UserCancelError so downstream abort handling classifies // it as a user cancellation (x-cancel, suppressed notice) rather than an error. - if (cancelled) - pending.reject(new UserCancelError("Input cancelled by user")); + // + // Strict `=== true`. A malformed flag must never reach here as a plain answer - + // `describeReplyProblem` turns it into a 400 at the wire, leaving the prompt + // pending - because resolving it would hand the run an empty answer the user + // never gave, which is worse than the spurious abort a loose truthy check caused. + if (cancelled === true) + pending.reject(new UserCancelError(PROMPT_CANCELLED_MESSAGE)); else pending.resolve(value); return true; } + /** The pending prompt's type, or undefined if nothing is waiting on that id. */ + private pendingPromptType( + session: Session, + requestId: string, + ): PromptSpec["type"] | undefined { + return session.pending.get(requestId)?.promptType; + } + private send(res: HttpServerResponse, status: number, body: unknown): void { const payload = JSON.stringify(body); res.writeHead(status, { @@ -487,20 +573,17 @@ class InteractivePromptServer { return; } if (req.method === "POST" && url.pathname === "/reply") { - const body = (await this.readBody(req)) as { - requestId?: string; - value?: unknown; - cancelled?: boolean; - }; - const accepted = this.handleReply(session, body); - // A missing/stale requestId matched no pending prompt: tell the - // client so it doesn't believe a blocked run was answered. - if (accepted) this.send(res, 200, { ok: true }); - else - this.send(res, 409, { - ok: false, - error: "No pending prompt for that requestId", - }); + const body = (await this.readBody(req)) as ReplyBody; + const outcome = this.submitWireReply(session.id, body); + if (outcome.ok) { + this.send(res, 200, { ok: true }); + } else { + // 400: the reply itself is malformed, and the prompt is STILL + // pending so the client can correct itself and try again. + // 409: nothing was waiting on that requestId, so the client + // doesn't believe a blocked run was answered. + this.send(res, outcome.status, { ok: false, error: outcome.error }); + } return; } @@ -564,17 +647,37 @@ class InteractivePromptServer { }); } - private handleReply( - session: Session, - body: { requestId?: string; value?: unknown; cancelled?: boolean }, - ): boolean { - if (!body.requestId) return false; - return this.submitReply( + /** + * Validate and deliver a reply that arrived over the wire. Public so the HTTP layer + * and tests share ONE path - the validation is the interesting part, and a test that + * bypassed it would be testing a shape production never sees. + */ + submitWireReply(sessionId: string, body: ReplyBody): ReplyOutcome { + const session = this.sessions.get(sessionId); + if (!session) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + if (!body.requestId) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + const promptType = this.pendingPromptType(session, body.requestId); + if (!promptType) { + return { ok: false, status: 409, error: NO_PENDING_PROMPT }; + } + // Validate BEFORE settling. A reply we cannot honour must leave the prompt + // pending and say why, never resolve it with something the user did not send. + const problem = describeReplyProblem(promptType, body); + if (problem) return { ok: false, status: 400, error: problem }; + + const accepted = this.submitReply( session.id, body.requestId, body.value, body.cancelled, ); + return accepted + ? { ok: true } + : { ok: false, status: 409, error: NO_PENDING_PROMPT }; } } diff --git a/src/interactive/promptProvider.test.ts b/src/interactive/promptProvider.test.ts index 159a50193..a83402fc3 100644 --- a/src/interactive/promptProvider.test.ts +++ b/src/interactive/promptProvider.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { RemotePromptProvider } from "./promptProvider"; import type { FieldRequirement } from "../preflight/RequirementCollector"; import type { interactivePromptServer } from "./interactivePromptServer"; +import { promptCancelled } from "../errors/UserCancelError"; type ServerLike = typeof interactivePromptServer; @@ -12,6 +13,19 @@ function fakeServer(answer: unknown): ServerLike { } as unknown as ServerLike; } +/** + * A server stub that REJECTS every emitPrompt, which is what a client cancel does + * (`submitReply(..., cancelled: true)`). The resolving stub above cannot express + * that, which is why nothing pinned cancel propagation through the provider before. + */ +function rejectingServer(error: unknown): ServerLike { + return { + emitPrompt: vi.fn(async () => { + throw error; + }), + } as unknown as ServerLike; +} + function dateField(id: string): FieldRequirement { return { id, label: id, type: "date" }; } @@ -178,3 +192,74 @@ describe("RemotePromptProvider suggester marshaling", () => { expect(result).toEqual(["custom-x"]); }); }); + +// --------------------------------------------------------------------------- +// Cancel propagation (#1574) +// --------------------------------------------------------------------------- + +describe("RemotePromptProvider cancel propagation", () => { + // A client cancel must abort the run exactly like dismissing the Obsidian modal. + // The file's own contract ("returns exactly what its in-app counterpart returns") + // only holds if this is true for EVERY method, so every method is covered. + const calls: Array<[string, (p: RemotePromptProvider) => Promise]> = [ + ["suggester", (p) => p.suggester(["a"], ["a"])], + ["suggesterMulti", (p) => p.suggesterMulti(["a"], ["a"])], + ["inputPrompt", (p) => p.inputPrompt("H")], + ["wideInputPrompt", (p) => p.wideInputPrompt("H")], + ["datePrompt", (p) => p.datePrompt("H")], + ["yesNoPrompt", (p) => p.yesNoPrompt("H")], + ["checkboxPrompt", (p) => p.checkboxPrompt(["a"])], + ["infoDialog", (p) => p.infoDialog("H", "t")], + ["requestInputs", (p) => p.requestInputs([dateField("d")])], + ]; + + for (const [name, call] of calls) { + it(`${name} propagates a client cancel as UserCancelError`, async () => { + const cancelled = promptCancelled(); + const provider = new RemotePromptProvider("s", rejectingServer(cancelled)); + await expect(call(provider)).rejects.toBe(cancelled); + }); + + it(`${name} propagates a session failure untouched`, async () => { + const failure = new Error("Interactive session ended"); + const provider = new RemotePromptProvider("s", rejectingServer(failure)); + await expect(call(provider)).rejects.toBe(failure); + }); + } +}); + +// --------------------------------------------------------------------------- +// confirm replies (#1574) +// --------------------------------------------------------------------------- + +describe("RemotePromptProvider yesNoPrompt reply validation", () => { + it.each([ + [true, true], + ["true", true], + [false, false], + ["false", false], + ])("accepts %o as %s", async (answer, expected) => { + const provider = new RemotePromptProvider("s", fakeServer(answer)); + expect(await provider.yesNoPrompt("H")).toBe(expected); + }); + + // In-app, `false` can ONLY come from clicking No. A reply that is neither a + // boolean nor a cancel used to be silently read as No, so the script took its + // else-branch on an answer the user never gave (#1574). + it.each([[null], [undefined], ["nope"], [0], [1], [{}], [[]]])( + "rejects %o instead of fabricating a No", + async (answer) => { + const provider = new RemotePromptProvider("s", fakeServer(answer)); + await expect(provider.yesNoPrompt("H")).rejects.toThrow( + /confirm prompt needs a boolean reply/, + ); + }, + ); + + it("names the documented cancel form in the error so a client can fix itself", async () => { + const provider = new RemotePromptProvider("s", fakeServer(null)); + await expect(provider.yesNoPrompt("H")).rejects.toThrow( + /\{"cancelled": true\}/, + ); + }); +}); diff --git a/src/interactive/promptProvider.ts b/src/interactive/promptProvider.ts index 3eee5df89..d4ff9e224 100644 --- a/src/interactive/promptProvider.ts +++ b/src/interactive/promptProvider.ts @@ -7,6 +7,19 @@ * datePrompt / yesNoPrompt / checkboxPrompt / infoDialog. Each method returns * exactly what its in-app counterpart returns, so a script cannot tell it was * driven remotely. + * + * A dismissal is part of that contract, not an exception to it: a client replies + * `{"cancelled": true}` (see `submitReply`), the pending prompt rejects with + * `UserCancelError`, and the run aborts exactly as it does when the Obsidian modal + * is dismissed - same class, same message. `promptProvider.test.ts` pins that for + * every method. + * + * One genuine asymmetry, kept deliberately: cancelling an `info` prompt aborts the + * run, while `GenericInfoDialog` in-app has no reject path and can never abort + * anything. Removing it would take away a remote client's only way to bail out + * mid-run, so it stays and is documented at the wire (docs/.../Advanced/CLI.md) + * instead: a client that just wants to close an info panel should send a plain + * reply, not a cancel. */ import { formatISODate } from "../utils/dateParser"; @@ -23,6 +36,16 @@ import { */ const SUGGESTER_INDEX_PREFIX = "\u0000qa-idx:"; +/** A short, safe rendering of a bad reply for a protocol error message. */ +function describeReply(answer: unknown): string { + if (answer === undefined) return "no value"; + if (answer === null) return "null"; + if (typeof answer === "string") return JSON.stringify(answer.slice(0, 40)); + if (typeof answer === "number" || typeof answer === "boolean") + return String(answer); + return Array.isArray(answer) ? "an array" : typeof answer; +} + export interface PromptProvider { /** Batch multi-field prompt (`quickAddApi.requestInputs`). Returns id -> value. */ requestInputs(fields: FieldRequirement[]): Promise>; @@ -238,13 +261,41 @@ export class RemotePromptProvider implements PromptProvider { return formatted ?? iso; } + /** + * Yes/No is the one prompt where a malformed reply is indistinguishable from a + * real answer, so it is the one prompt that validates. + * + * In-app, `false` can only come from clicking No: dismissing resolves a + * three-state `null` that aborts the run. This used to collapse every non-`true` + * reply to `false`, so a client that omitted `value`, sent `null` (the shape + * #1574 proposed for a dismissal), or sent a typo silently answered "No" and the + * script walked its else-branch (#1574). A dismissal already has a wire + * representation - `{"cancelled": true}`, documented and rejecting with + * UserCancelError - so anything that is neither a boolean nor a cancel is a + * client bug, and failing beats inventing an answer the user never gave. + * + * A live client never sees this throw: the same rule is enforced at `/reply` + * (`describeReplyProblem`), where a 400 reaches the client while it is still + * holding the response and the prompt stays pending. This is the backstop for + * every other caller of the provider, and the reason it is not the primary check + * is that on the Template/Capture path a thrown message is replaced by a generic + * sentence before the client ever polls for it. + * + * The other prompt types stay lenient on purpose: `""` and `[]` are answers a + * user really can give in-app (the Skip affordances, optional fields), so + * tightening them would break optional prompts on remote runs. + */ async yesNoPrompt(header: string, text?: string): Promise { const answer = await this.server.emitPrompt(this.sessionId, { type: "confirm", header, text, }); - return answer === true || answer === "true"; + if (answer === true || answer === "true") return true; + if (answer === false || answer === "false") return false; + throw new Error( + `A confirm prompt needs a boolean reply, or {"cancelled": true} if the user dismissed it. Got ${describeReply(answer)}.`, + ); } async checkboxPrompt( diff --git a/src/main.ts b/src/main.ts index e65576e48..2c465d772 100644 --- a/src/main.ts +++ b/src/main.ts @@ -9,6 +9,7 @@ import { ConsoleErrorLogger } from "./logger/consoleErrorLogger"; import { GuiLogger } from "./logger/guiLogger"; import { LogManager } from "./logger/logManager"; import { reportError, withErrorHandling } from "./utils/errorUtils"; +import { registerUnhandledRejectionReporter } from "./utils/unhandledRejectionReporter"; import { openQuickAddSettings } from "./utils/openPluginSettings"; import { StartupMacroEngine } from "./engine/StartupMacroEngine"; import { ChoiceExecutor } from "./choiceExecutor"; @@ -84,10 +85,18 @@ export default class QuickAdd extends Plugin { private unsubscribeSettingsStore: () => void; // Debounced disk write for the store subscriber. saveSettings() stays immediate // (migrations await it) and cancels this; onunload flushes it. - private requestSave: Debouncer<[], void> = debounce( - () => void this.saveData(this.settings), - SETTINGS_SAVE_DEBOUNCE_MS, - ); + // + // The async IIFE is load-bearing, not style: floating `saveData()` straight from a + // non-async arrow leaves NO QuickAdd frame on the rejection's stack (it is + // constructed inside Obsidian's own FS plumbing, after an await), so a failed + // settings write would be invisible to the unhandled-rejection reporter - which is + // the one failure here the user most needs to hear about, since their settings + // silently did not persist. Awaiting inside a QuickAdd frame puts us on the stack. + private requestSave: Debouncer<[], void> = debounce(() => { + void (async () => { + await this.saveData(this.settings); + })(); + }, SETTINGS_SAVE_DEBOUNCE_MS); get api(): ReturnType { return QuickAddApi.GetApi( @@ -311,6 +320,12 @@ export default class QuickAdd extends Plugin { log.register(new ConsoleErrorLogger()).register(new GuiLogger(this)); + // Must come after the loggers: a QuickAdd promise that rejects with nobody + // awaiting it (a settings click handler, a floated call) used to leave the user + // with nothing but a console line. Now it reports through the same channel as + // every other failure (#1576). + registerUnhandledRejectionReporter(this); + if (this.settings.enableRibbonIcon) { this.addRibbonIcon("file-plus", "QuickAdd", () => { openChoiceLauncher(this); diff --git a/src/preflight/OnePageInputModal.test.ts b/src/preflight/OnePageInputModal.test.ts index f3bd7c267..d4be308bd 100644 --- a/src/preflight/OnePageInputModal.test.ts +++ b/src/preflight/OnePageInputModal.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; import type { FieldRequirement } from "./RequirementCollector"; import { OnePageInputModal } from "./OnePageInputModal"; +import { UserCancelError } from "../errors/UserCancelError"; import { buildValueVariableKey } from "src/utils/valueSyntax"; const { attachImagePasteHandlerMock } = vi.hoisted(() => ({ @@ -745,13 +746,13 @@ describe("OnePageInputModal", () => { }); describe("Esc settles the modal promise (issue #1259 rider)", () => { - it("rejects with 'cancelled' when closed without submitting", async () => { + it("rejects with a typed cancellation when closed without submitting", async () => { const requirements: FieldRequirement[] = [ { id: "note", label: "note", type: "text" }, ]; const modal = new OnePageInputModal({} as App, requirements, new Map()); modal.onClose(); - await expect(modal.waitForClose).rejects.toBe("cancelled"); + await expect(modal.waitForClose).rejects.toBeInstanceOf(UserCancelError); }); it("does not double-settle after a submit", async () => { diff --git a/src/preflight/OnePageInputModal.ts b/src/preflight/OnePageInputModal.ts index 498a47d82..b363c03e3 100644 --- a/src/preflight/OnePageInputModal.ts +++ b/src/preflight/OnePageInputModal.ts @@ -33,6 +33,7 @@ import { normalizeNumericValue, normalizeSliderValue, } from "src/utils/valueSyntax"; +import { promptCancelled } from "../errors/UserCancelError"; type PreviewComputer = ( values: Record, @@ -685,7 +686,7 @@ export class OnePageInputModal extends Modal { private cancel() { this.settled = true; this.close(); - this.rejectPromise("cancelled"); + this.rejectPromise(promptCancelled()); } onClose() { @@ -695,7 +696,7 @@ export class OnePageInputModal extends Modal { // otherwise the choice execution hangs forever on waitForClose. if (!this.settled) { this.settled = true; - this.rejectPromise("cancelled"); + this.rejectPromise(promptCancelled()); } } diff --git a/src/preflight/runOnePagePreflight.fallback.test.ts b/src/preflight/runOnePagePreflight.fallback.test.ts index f29a6182a..19818879c 100644 --- a/src/preflight/runOnePagePreflight.fallback.test.ts +++ b/src/preflight/runOnePagePreflight.fallback.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { App } from "obsidian"; import { runOnePagePreflight } from "./runOnePagePreflight"; import { MacroAbortError } from "../errors/MacroAbortError"; +import { promptCancelled } from "../errors/UserCancelError"; import type ICaptureChoice from "../types/choices/ICaptureChoice"; import type { IChoiceExecutor } from "../IChoiceExecutor"; @@ -146,9 +147,10 @@ describe("runOnePagePreflight fallback warning", () => { expect(logWarningMock).not.toHaveBeenCalled(); }); - it("rethrows the modal's 'cancelled' rejection without warning", async () => { - modalOutcome = () => Promise.reject("cancelled"); - await expect(run()).rejects.toBe("cancelled"); + it("rethrows the modal's dismissal without warning", async () => { + const cancelled = promptCancelled(); + modalOutcome = () => Promise.reject(cancelled); + await expect(run()).rejects.toBe(cancelled); expect(logWarningMock).not.toHaveBeenCalled(); }); diff --git a/src/preflight/runOnePagePreflight.ts b/src/preflight/runOnePagePreflight.ts index 1973f3544..78dd7972f 100644 --- a/src/preflight/runOnePagePreflight.ts +++ b/src/preflight/runOnePagePreflight.ts @@ -244,10 +244,10 @@ export async function runOnePagePreflight( return true; } catch (error) { // Propagate an explicit cancellation/abort so the run stops instead of - // continuing with the inputs missing. The native modal throws the string - // "cancelled"; a remote provider rejects with UserCancelError (a - // MacroAbortError subclass). - if (error === "cancelled" || error instanceof MacroAbortError) { + // continuing with the inputs missing. Both the native modal and a remote + // provider signal a dismissal with UserCancelError, a MacroAbortError + // subclass (#1577), so one check covers both. + if (error instanceof MacroAbortError) { throw error; } // Any other error degrades to the sequential runtime prompts. That diff --git a/src/quickAddApi.test.ts b/src/quickAddApi.test.ts index c3730462a..5366c4b29 100644 --- a/src/quickAddApi.test.ts +++ b/src/quickAddApi.test.ts @@ -152,6 +152,7 @@ vi.mock("./utils/errorUtils", async () => { const { QuickAddApi } = await import("./quickAddApi"); const { MacroAbortError } = await import("./errors/MacroAbortError"); +const { promptCancelled } = await import("./errors/UserCancelError"); // --------------------------------------------------------------------------- // Test doubles for app / plugin / choiceExecutor @@ -248,10 +249,12 @@ describe("static prompt wrappers", () => { ); }); - it("returns undefined for a generic (non-cancellation) error", async () => { - mocks.genericInputPrompt.mockRejectedValue(new Error("boom")); - const out = await QuickAddApi.inputPrompt(app, "Header"); - expect(out).toBeUndefined(); + it("propagates a generic (non-cancellation) error", async () => { + const failure = new Error("boom"); + mocks.genericInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.inputPrompt(app, "Header")).rejects.toBe( + failure, + ); }); it("converts a cancellation string into a MacroAbortError", async () => { @@ -300,9 +303,10 @@ describe("static prompt wrappers", () => { ); }); - it("swallows generic errors as undefined", async () => { - mocks.genericWideInputPrompt.mockRejectedValue(new Error("x")); - expect(await QuickAddApi.wideInputPrompt(app, "H")).toBeUndefined(); + it("propagates generic errors", async () => { + const failure = new Error("x"); + mocks.genericWideInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.wideInputPrompt(app, "H")).rejects.toBe(failure); }); }); @@ -327,9 +331,12 @@ describe("static prompt wrappers", () => { ); }); - it("swallows generic errors as undefined", async () => { - mocks.genericYesNoPromptAsk.mockRejectedValue(new Error("x")); - expect(await QuickAddApi.yesNoPrompt(app, "H")).toBeUndefined(); + // The worst-typed swallow of all: `undefined` is falsy, so a defect used to + // read to the script as "the user answered No". + it("propagates generic errors instead of reading as a falsy No", async () => { + const failure = new Error("x"); + mocks.genericYesNoPromptAsk.mockRejectedValue(failure); + await expect(QuickAddApi.yesNoPrompt(app, "H")).rejects.toBe(failure); }); }); @@ -339,6 +346,21 @@ describe("static prompt wrappers", () => { await QuickAddApi.infoDialog(app, "H", ["a", "b"]); expect(mocks.genericInfoDialog).toHaveBeenCalledWith(app, "H", ["a", "b"]); }); + + // Declared `Promise`, so a swallowed failure was indistinguishable + // from success - the error was not merely unlogged, it was unrepresentable. + it("propagates a generic error", async () => { + const failure = new Error("dom exploded"); + mocks.genericInfoDialog.mockRejectedValue(failure); + await expect(QuickAddApi.infoDialog(app, "H", "x")).rejects.toBe(failure); + }); + + it("converts a dismissal into a MacroAbortError", async () => { + mocks.genericInfoDialog.mockRejectedValue(promptCancelled()); + await expect(QuickAddApi.infoDialog(app, "H", "x")).rejects.toBeInstanceOf( + MacroAbortError, + ); + }); }); describe("checkboxPrompt", () => { @@ -353,9 +375,10 @@ describe("static prompt wrappers", () => { ); }); - it("returns undefined on a generic error", async () => { - mocks.genericCheckboxPrompt.mockRejectedValue(new Error("nope")); - expect(await QuickAddApi.checkboxPrompt(app, [])).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("nope"); + mocks.genericCheckboxPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.checkboxPrompt(app, [])).rejects.toBe(failure); }); }); }); @@ -403,9 +426,10 @@ describe("datePrompt", () => { ); }); - it("returns undefined on a generic error", async () => { - mocks.vDateInputPrompt.mockRejectedValue(new Error("fail")); - expect(await QuickAddApi.datePrompt(app, "H")).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("fail"); + mocks.vDateInputPrompt.mockRejectedValue(failure); + await expect(QuickAddApi.datePrompt(app, "H")).rejects.toBe(failure); }); it("converts a cancellation into a MacroAbortError", async () => { @@ -501,9 +525,23 @@ describe("suggester", () => { }); }); - it("returns undefined on a generic error", async () => { - mocks.genericSuggester.mockRejectedValue(new Error("boom")); - expect(await QuickAddApi.suggester(app, ["a"], ["a"])).toBeUndefined(); + it("propagates a generic error", async () => { + const failure = new Error("boom"); + mocks.genericSuggester.mockRejectedValue(failure); + await expect(QuickAddApi.suggester(app, ["a"], ["a"])).rejects.toBe(failure); + }); + + // The try also spans the caller-supplied display mapping, so a buggy display + // callback used to be swallowed too - the exact #1575 field report. + it("propagates a throwing display callback instead of returning undefined", async () => { + mocks.genericSuggester.mockResolvedValue("a"); + await expect( + QuickAddApi.suggester( + app, + (value: string) => (value as unknown as { nope: string }).nope.trim(), + ["a"], + ), + ).rejects.toBeInstanceOf(TypeError); }); }); diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index c4b367945..324533ece 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -52,7 +52,10 @@ import { FieldSuggestionCache } from "./utils/FieldSuggestionCache"; import { FieldSuggestionFileFilter } from "./utils/FieldSuggestionFileFilter"; import { InlineFieldParser } from "./utils/InlineFieldParser"; import { MacroAbortError } from "./errors/MacroAbortError"; -import { UserCancelError } from "./errors/UserCancelError"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, +} from "./errors/UserCancelError"; import { formatISODate } from "./utils/dateParser"; import type { InputPromptOptions } from "./types/inputPrompt"; import type { NumericInputConfig, SliderConfig } from "./utils/valueSyntax"; @@ -229,8 +232,7 @@ export class QuickAddApi { try { collected = await provider.requestInputs(missing); } catch (error) { - throwIfPromptCancelled(error); - throw error; + rethrowPromptError(error); } } else { const modal = new OnePageInputModal( @@ -241,8 +243,7 @@ export class QuickAddApi { try { collected = await modal.waitForClose; } catch (error) { - throwIfPromptCancelled(error); - throw error; + rethrowPromptError(error); } } } @@ -874,8 +875,7 @@ export class QuickAddApi { options, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -905,8 +905,7 @@ export class QuickAddApi { } return value; } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -927,8 +926,7 @@ export class QuickAddApi { options, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -940,11 +938,10 @@ export class QuickAddApi { try { answer = await GenericYesNoPrompt.Ask(app, header, text); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } - if (answer === null) throw new UserCancelError("Input cancelled by user"); + if (answer === null) throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); return answer; } @@ -956,8 +953,7 @@ export class QuickAddApi { try { return await GenericInfoDialog.Show(app, header, text); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -1004,8 +1000,7 @@ export class QuickAddApi { options?.renderItem, ); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } @@ -1022,17 +1017,38 @@ export class QuickAddApi { ? GenericCheckboxPrompt.Open(app, items, selectedItems) : GenericCheckboxPrompt.Open(app, items, selectedItems, header)); } catch (error) { - throwIfPromptCancelled(error); - return undefined; + rethrowPromptError(error); } } } -function throwIfPromptCancelled(error: unknown): void { +/** + * The single error policy for every prompt on the script API surface. + * + * A dismissal becomes a `UserCancelError` (which the macro engine turns into a + * quiet abort); anything else propagates untouched. It returns `never`, which is + * the point: these wrappers used to call a `void` helper and then + * `return undefined`, so a genuine failure inside a modal - a throwing display + * callback, a bad argument, an Obsidian API error - was handed back to the script + * as "the user gave no input", with nothing logged and nothing shown (#1575). + * `never` makes that omission a compile error rather than a silent one. + * + * Not reported here on purpose: every path that runs a script already reports a + * propagating error (MacroChoiceEngine for user scripts, TemplateChoiceEngine for + * inline scripts, main.ts / choiceSuggester at the top), and `reportError` raises a + * 15-second Notice - so reporting here too would stack another one. + * + * "Already reports" is at least once, not exactly once: on the command-palette macro + * path MacroChoiceEngine reports and rethrows into main.ts, which reports again, so a + * user script error raises TWO notices today. That is pre-existing and tracked + * separately; adding a third here would make it worse, not better. + */ +function rethrowPromptError(error: unknown): never { if (error instanceof MacroAbortError) { throw error; } if (isCancellationError(error)) { - throw new UserCancelError("Input cancelled by user"); + throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); } + throw error; } diff --git a/src/utils/cancellationContract.test.ts b/src/utils/cancellationContract.test.ts new file mode 100644 index 000000000..2d289e4c8 --- /dev/null +++ b/src/utils/cancellationContract.test.ts @@ -0,0 +1,216 @@ +import { describe, expect, it } from "vitest"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; +import { isCancellationError } from "./errorUtils"; +import { + PROMPT_CANCELLED_MESSAGE, + UserCancelError, + promptCancelled, +} from "../errors/UserCancelError"; +import { MacroAbortError } from "../errors/MacroAbortError"; + +/** + * The cancellation contract (#1577): a dismissed prompt throws a typed + * `UserCancelError`, never a bare English sentence. The string list in + * `isCancellationError` exists only for user scripts that still throw a legacy + * sentinel - if QuickAdd's own code starts producing one again, the fragility the + * types removed is back, and the compiler will not say a word. Hence the ratchet. + */ + +const SRC = join(__dirname, ".."); + +function walk(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + walk(full, out); + continue; + } + if (!/\.(ts|svelte)$/.test(entry)) continue; + if (/\.test\.ts$/.test(entry)) continue; + out.push(full); + } + return out; +} + +/** + * Comments talk ABOUT the shapes below ("would throw \"Unknown location\""), so scanning + * raw text produces false positives. Blank them, preserving offsets so reported line + * numbers still point at the real line. + */ +function stripComments(text: string): string { + return text + .replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, " ")) + .replace(/(^|[^:])\/\/[^\n]*/g, (m, lead: string) => lead + " ".repeat(m.length - lead.length)); +} + +/** + * Any `reject(...)` / `rejectPromise(...)` CALL, capturing its argument up to the first + * `)`. Matching the call rather than a bare-string argument is deliberate: the shape that + * would actually reintroduce #1577 is not only `reject("dismissed")` but also + * `reject(new Error("dismissed"))` - an Error `isCancellationError` cannot recognise, + * so a dismissal reads as a failure again with nothing to catch it. + * + * `[\s\S]` rather than `[^\n]`: an argument split across lines would otherwise capture + * as empty and be waved through as an executor binding. + */ +const REJECT_CALL = /\b(?:reject|rejectPromise)(?:!|\?\.|\?)?\s*\(\s*([\s\S]*?)\)/g; +/** A `throw` of a string literal, i.e. a bare-string rejection inside an async method. */ +const THROW_STRING = /\bthrow\s+["'`]/g; + +/** Directories whose modules are prompt surfaces: a rejection there means a dismissal. */ +const PROMPT_DIRS = [join(SRC, "gui"), join(SRC, "preflight")]; + +/** + * Every rejection in `text` that is not `promptCancelled()`, as `line — snippet`. + * Extracted from the directory scan so the detector itself is testable: this guard is + * load-bearing, and a guard that silently stops matching is worse than none. + */ +export function findNonCancelledRejections(text: string): string[] { + const found: string[] = []; + const stripped = stripComments(text); + for (const match of stripped.matchAll(REJECT_CALL)) { + const arg = (match[1] ?? "").trim(); + // `new Promise((resolve, reject) => …)`: the binding, not a call on it. + if (arg === "" || arg.startsWith("=>")) continue; + if (arg.startsWith("promptCancelled(")) continue; + const line = stripped.slice(0, match.index).split("\n").length; + found.push(`${line} — ${match[0].trim().replace(/\s+/g, " ").slice(0, 60)}`); + } + return found; +} + +describe("cancellation contract (#1577)", () => { + // The positive invariant, not a negative syntax shape: inside the prompt + // surfaces, the ONLY thing a promise may be rejected with is promptCancelled(). + it("every prompt rejection is promptCancelled()", () => { + const offenders: string[] = []; + for (const dir of PROMPT_DIRS) { + for (const file of walk(dir)) { + for (const hit of findNonCancelledRejections(readFileSync(file, "utf8"))) { + offenders.push(`${file.slice(SRC.length + 1)}:${hit}`); + } + } + } + expect( + offenders, + "A dismissed prompt must reject with promptCancelled(). A bare string is " + + "unrecognisable and untraceable; a plain Error reads as a genuine failure.", + ).toEqual([]); + }); + + it("no source file throws a bare string literal", () => { + const offenders: string[] = []; + for (const file of walk(SRC)) { + const text = stripComments(readFileSync(file, "utf8")); + for (const match of text.matchAll(THROW_STRING)) { + const line = text.slice(0, match.index).split("\n").length; + offenders.push(`${file.slice(SRC.length + 1)}:${line} — ${match[0].trim()}…`); + } + } + expect( + offenders, + "Throw an Error so the value carries a stack and can be classified.", + ).toEqual([]); + }); + + it("no source file re-introduces a legacy cancellation sentinel", () => { + // Unquoted so single-quoted and template forms count too. OnePageInputModal's + // old sentinel, "cancelled", is deliberately absent: as a bare substring it is + // far too common a word in src/ to scan for. The reject-call invariant above is + // what actually guards that modal. + const sentinels = ["No input given.", "no input given."]; + const offenders: string[] = []; + for (const file of walk(SRC)) { + // errorUtils owns the compatibility list by design. + if (file.endsWith(join("utils", "errorUtils.ts"))) continue; + const text = readFileSync(file, "utf8"); + for (const sentinel of sentinels) { + if (text.includes(sentinel)) { + offenders.push(`${file.slice(SRC.length + 1)} — ${sentinel}`); + } + } + } + expect(offenders).toEqual([]); + }); + + // Guards the guard: these are the shapes that would reintroduce #1577, and a + // detector that stops seeing one of them fails silently. + describe("the rejection detector", () => { + it.each([ + ["a bare string", 'this.rejectPromise("dismissed");'], + ["an unrecognisable Error", 'this.rejectPromise(new Error("dismissed"));'], + ["an indirected constant", "this.rejectPromise(CANCEL_MESSAGE);"], + ["an optional call", 'this.rejectPromise?.("dismissed");'], + ["the raw reject binding", 'reject("dismissed");'], + // The multiline forms: captured as EMPTY by a line-bounded pattern, which + // would then wave them through as an executor binding. + ["a multiline string", 'this.rejectPromise(\n\t"dismissed",\n);'], + [ + "a multiline Error", + 'this.rejectPromise(\n\tnew Error("dismissed"),\n);', + ], + ])("flags %s", (_label, source) => { + expect(findNonCancelledRejections(source)).not.toEqual([]); + }); + + it.each([ + ["the contract", "this.rejectPromise(promptCancelled());"], + ["the contract, multiline", "this.rejectPromise(\n\tpromptCancelled(),\n);"], + [ + "an executor binding", + "this.waitForClose = new Promise((resolve, reject) => {\n\t\tthis.rejectPromise = reject;\n\t});", + ], + [ + "a comment about the old shape", + '// this.rejectPromise("dismissed") - the pre-#1577 shape', + ], + ])("allows %s", (_label, source) => { + expect(findNonCancelledRejections(source)).toEqual([]); + }); + }); + + // If the walk stops finding files (a moved directory, a changed extension filter), + // every scan above passes vacuously. + it("the scan actually reads the prompt sources", () => { + const scanned = PROMPT_DIRS.flatMap((dir) => walk(dir)); + expect(scanned.length).toBeGreaterThan(50); + expect( + scanned.some((f) => f.endsWith(join("GenericSuggester", "genericSuggester.ts"))), + ).toBe(true); + expect( + scanned.some((f) => f.endsWith(join("preflight", "OnePageInputModal.ts"))), + ).toBe(true); + }); + + it("recognises a typed prompt dismissal", () => { + expect(isCancellationError(promptCancelled())).toBe(true); + expect(isCancellationError(new UserCancelError("anything"))).toBe(true); + }); + + it("still recognises the legacy sentinels a user script may throw", () => { + expect(isCancellationError("No input given.")).toBe(true); + expect(isCancellationError("no input given.")).toBe(true); + expect(isCancellationError("cancelled")).toBe(true); + }); + + it("does not mistake a real failure for a cancellation", () => { + expect(isCancellationError(new Error("ENOENT"))).toBe(false); + expect(isCancellationError(new MacroAbortError("target missing"))).toBe( + false, + ); + expect(isCancellationError("Cancelled")).toBe(false); + expect(isCancellationError(undefined)).toBe(false); + expect(isCancellationError(null)).toBe(false); + }); + + it("carries the message the public docs promise", () => { + const error = promptCancelled(); + // docs/.../QuickAddAPI.md tells scripts to expect + // MacroAbortError("Input cancelled by user"). + expect(error.message).toBe(PROMPT_CANCELLED_MESSAGE); + expect(error.message).toBe("Input cancelled by user"); + expect(error.name).toBe("MacroAbortError"); + expect(error).toBeInstanceOf(MacroAbortError); + }); +}); diff --git a/src/utils/errorUtils.ts b/src/utils/errorUtils.ts index 404186f91..bef2eb886 100644 --- a/src/utils/errorUtils.ts +++ b/src/utils/errorUtils.ts @@ -1,6 +1,7 @@ import { log } from "../logger/logManager"; import type { ErrorLevel } from "../logger/errorLevel"; import { ErrorLevel as ErrorLevelEnum } from "../logger/errorLevel"; +import { UserCancelError } from "../errors/UserCancelError"; /** * Maximum number of errors to keep in the error log @@ -54,39 +55,51 @@ export function toError(err: unknown, contextMessage?: string): Error { } /** - * Checks if an error indicates user cancellation rather than a real error. - * Used to distinguish between intentional user cancellations (Escape key, Cancel button) - * and actual errors (network failures, file system errors, etc.) - * + * Checks if a caught value means "the user backed out" rather than "something broke". + * + * Every QuickAdd prompt signals a dismissal by throwing {@link UserCancelError} + * (see `promptCancelled()`), so this is an `instanceof` check. Because + * `UserCancelError extends MacroAbortError`, a dismissal that nobody classifies still + * aborts the run quietly instead of being reported as a failure. + * * @param error - The error to check * @returns true if the error indicates user cancellation, false otherwise - * + * * @example * ```ts * try { * const result = await promptUser(); * } catch (error) { - * if (isCancellationError(error)) { - * throw new MacroAbortError("Input cancelled by user"); - * } - * throw error; // Re-throw actual errors + * if (isCancellationError(error)) return null; // the user backed out + * throw error; // a real failure * } * ``` */ export function isCancellationError(error: unknown): boolean { - if (typeof error !== "string") return false; - - const cancellationMessages = [ - "no input given.", // GenericSuggester, InputSuggester, GenericCheckboxPrompt - "No input given.", // GenericInputPrompt, MathModal - // GenericYesNoPrompt is deliberately absent: it resolves a dismissal - // instead of rejecting, so it never reaches this check (#1567). - "cancelled" // OnePagePreflight - ]; - - return cancellationMessages.includes(error); + // QuickAdd's own prompts throw a typed cancellation (#1577). This is the whole + // check for every in-plugin prompt; the sentinels below are compatibility only. + if (error instanceof UserCancelError) return true; + + // Legacy sentinels. QuickAdd's prompts used to reject with one of these bare + // English sentences and nothing else, so a *user script* may still throw one + // (or re-throw one it caught), and MacroChoiceEngine has honoured that for as + // long as the sentinels existed. Kept for that reason alone - no QuickAdd code + // produces them any more, which `errorUtils.cancellationContract.test.ts` + // pins. Do not add to this list: a new prompt throws `promptCancelled()`. + return typeof error === "string" && LEGACY_CANCELLATION_SENTINELS.has(error); } +/** + * Bare-string cancellations QuickAdd's prompts rejected with before #1577. + * Note the two case variants of one sentence - that inconsistency is exactly why + * string matching was the wrong signal. + */ +const LEGACY_CANCELLATION_SENTINELS: ReadonlySet = new Set([ + "no input given.", // GenericSuggester, InputSuggester, GenericCheckboxPrompt, MultiSuggester + "No input given.", // GenericInputPrompt, GenericWideInputPrompt, MathModal, MultiChoiceSettingsModal + "cancelled", // OnePageInputModal +]); + /** * Reports an error to the logging system with additional context * Converts any error type to a proper Error object and logs it with the appropriate level diff --git a/src/utils/unhandledRejectionReporter.test.ts b/src/utils/unhandledRejectionReporter.test.ts new file mode 100644 index 000000000..96eea7df6 --- /dev/null +++ b/src/utils/unhandledRejectionReporter.test.ts @@ -0,0 +1,228 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerUnhandledRejectionReporter } from "./unhandledRejectionReporter"; +import { ChoiceAbortError } from "../errors/ChoiceAbortError"; +import { promptCancelled } from "../errors/UserCancelError"; +import { log } from "../logger/logManager"; + +/** + * Errors get attributed to QuickAdd by the `plugin:` marker Obsidian puts in a + * plugin bundle's stack frames (verified live in Obsidian 1.13.0). Build one, with a + * controllable throw site so the site-based dedupe can be exercised. + */ +function pluginError(message: string, site = "doThing", name = "Error"): Error { + const error = new Error(message); + error.name = name; + error.stack = `${name}: ${message}\n at mS.${site} (plugin:quickadd:414:30553)`; + return error; +} + +function foreignError(message: string): Error { + const error = new Error(message); + error.stack = `Error: ${message}\n at x (plugin:some-other-plugin:1:1)`; + return error; +} + +describe("unhandled rejection reporter (#1576)", () => { + let listener: ((event: PromiseRejectionEvent) => void) | null = null; + let logError: ReturnType; + // A hand-cranked clock, injected. Stubbing the global `Date.now` here deadlocks + // vitest, which reads it for its own timeouts. + let clock = 1_000_000; + + const host = { + manifest: { id: "quickadd" }, + registerDomEvent( + _el: Window, + _type: "unhandledrejection", + callback: (event: PromiseRejectionEvent) => void, + ) { + listener = callback; + }, + }; + + /** + * A minimal stand-in for PromiseRejectionEvent (jsdom's needs a real promise). + * Non-optional call on purpose: if the reporter ever stops registering a listener, + * the negative tests below would otherwise pass vacuously - a dead listener + * produces exactly the values they assert. + */ + function fire(reason: unknown) { + if (!listener) throw new Error("no unhandledrejection listener was registered"); + let defaultPrevented = false; + listener({ + reason, + preventDefault() { + defaultPrevented = true; + }, + } as unknown as PromiseRejectionEvent); + return { defaultPrevented }; + } + + beforeEach(() => { + listener = null; + clock = 1_000_000; + logError = vi.spyOn(log, "logError").mockImplementation(() => {}); + registerUnhandledRejectionReporter(host, () => clock); + expect(listener, "the reporter must register a listener").not.toBeNull(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("reports a QuickAdd failure and suppresses the console default", () => { + const { defaultPrevented } = fire(pluginError("Invalid choice type")); + + expect(defaultPrevented).toBe(true); + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain("Invalid choice type"); + }); + + it("leaves another plugin's rejection completely alone", () => { + const { defaultPrevented } = fire(foreignError("not ours")); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + + it("ignores a rejection it cannot attribute (a bare string has no stack)", () => { + const { defaultPrevented } = fire("no input given."); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + + it("silences a dismissed prompt instead of reporting it", () => { + const cancelled = promptCancelled(); + cancelled.stack = "MacroAbortError: x\n at a (plugin:quickadd:1:1)"; + + const { defaultPrevented } = fire(cancelled); + + // Claimed (so the console stays quiet) but never reported. + expect(defaultPrevented).toBe(true); + expect(logError).not.toHaveBeenCalled(); + }); + + // Not every MacroAbortError is a user cancellation. ChoiceAbortError carries copy + // the user needs ("Selected folder not allowed."), and the rest of the plugin keeps + // the two apart - so swallowing it here would leave a floated involuntary abort with + // LESS signal than the console line this replaces. + it("reports an involuntary abort, which is not a cancellation", () => { + const abort = new ChoiceAbortError("Selected folder not allowed."); + abort.stack = "MacroAbortError: x\n at a (plugin:quickadd:1:1)"; + + fire(abort); + + expect(logError).toHaveBeenCalledTimes(1); + expect(String(logError.mock.calls[0]?.[0])).toContain( + "Selected folder not allowed.", + ); + }); + + it("reports a repeated failure once per window, not once per occurrence", () => { + // A validator that throws on every keystroke must not bury the user. + for (let i = 0; i < 20; i++) fire(pluginError("validator exploded")); + expect(logError).toHaveBeenCalledTimes(1); + + clock += 9_000; + fire(pluginError("validator exploded")); + expect(logError).toHaveBeenCalledTimes(1); + }); + + // The window SLIDES on occurrences, it does not restart on reports. Stamping only + // on report would let a continuously failing site raise a notice every 10 seconds + // indefinitely - six 15-second notices a minute, worse than the silence it replaced. + it("stays quiet while a failure keeps recurring inside the window", () => { + fire(pluginError("stuck loop")); + expect(logError).toHaveBeenCalledTimes(1); + + // Failing every 9s for two minutes: still one report. + for (let i = 0; i < 14; i++) { + clock += 9_000; + fire(pluginError("stuck loop")); + } + expect(logError).toHaveBeenCalledTimes(1); + }); + + it("reports again once the failure has stopped and recurs", () => { + fire(pluginError("intermittent")); + expect(logError).toHaveBeenCalledTimes(1); + + clock += 11_000; // quiet for longer than the window + fire(pluginError("intermittent")); + expect(logError).toHaveBeenCalledTimes(2); + }); + + // Silencing the notice must never leave an occurrence with LESS evidence than + // before this reporter existed, so a suppressed repeat keeps the browser's own + // unhandled-rejection line. + it("leaves the browser default alone for a suppressed repeat", () => { + expect(fire(pluginError("noisy")).defaultPrevented).toBe(true); + expect(fire(pluginError("noisy")).defaultPrevented).toBe(false); + }); + + it("collapses one broken loop that fails on many different values", () => { + // Same throw site, message embeds the note path: one bug, one notice. + for (let i = 0; i < 200; i++) { + fire(pluginError(`Could not read note ${i}.md`, "readNote")); + } + expect(logError).toHaveBeenCalledTimes(1); + }); + + // `plugin:quickadd` is a PREFIX of `plugin:quickadd-beta`, so an undelimited match + // would claim a fork's rejections and suppress their only console line. + it("does not claim a plugin whose id merely starts with ours", () => { + const error = new Error("beta bug"); + error.stack = "Error: beta bug\n at f (plugin:quickadd-beta:1:1)"; + + const { defaultPrevented } = fire(error); + + expect(defaultPrevented).toBe(false); + expect(logError).not.toHaveBeenCalled(); + }); + + it("does not let one noisy failure mask a different one", () => { + fire(pluginError("first", "siteA")); + fire(pluginError("second", "siteB")); + fire(pluginError("first", "siteA")); + + expect(logError).toHaveBeenCalledTimes(2); + }); + + it("reports every distinct throw site, however many there are", () => { + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + expect(logError).toHaveBeenCalledTimes(200); + }); + + // The map is bounded (MAX_TRACKED), so past that many LIVE sites the "one report + // per site per window" guarantee degrades to reporting again. That is the deliberate + // trade against unbounded growth; pin it so the behaviour is a decision, not a + // surprise. Fewer sites than the bound must still collapse within the window. + it("keeps collapsing a repeat while the site is still tracked", () => { + for (let i = 0; i < 10; i++) fire(pluginError("boom", `site${i}`)); + expect(logError).toHaveBeenCalledTimes(10); + + fire(pluginError("boom", "site0")); + expect(logError).toHaveBeenCalledTimes(10); + }); + + it("evicts under pressure rather than growing without limit", () => { + // Far more live sites than the bound; the oldest entries get dropped, so an + // early site is reported a second time instead of being remembered forever. + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + logError.mockClear(); + fire(pluginError("boom", "site0")); + expect(logError).toHaveBeenCalledTimes(1); + }); + + // Eviction must never drop the key it just stamped, or the site currently failing + // would report on every single occurrence. + it("does not evict the site it is currently handling", () => { + for (let i = 0; i < 200; i++) fire(pluginError("boom", `site${i}`)); + logError.mockClear(); + fire(pluginError("boom", "hot")); + expect(logError).toHaveBeenCalledTimes(1); + fire(pluginError("boom", "hot")); + expect(logError).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/utils/unhandledRejectionReporter.ts b/src/utils/unhandledRejectionReporter.ts new file mode 100644 index 000000000..0537d8347 --- /dev/null +++ b/src/utils/unhandledRejectionReporter.ts @@ -0,0 +1,140 @@ +import { isCancellationError, reportError } from "./errorUtils"; + +/** + * Reports a promise rejection that escaped QuickAdd unhandled, instead of letting it + * die in the console. + * + * Across `src/` there are roughly forty `onClick(async …)` / `onclick={…}` handlers that + * hand Obsidian or Svelte a promise it discards, plus a long tail of deliberately + * fire-and-forget `void someAsyncCall()`. When one of those fails the user gets nothing: + * no notice, no entry in QuickAdd's error log, only an `Uncaught (in promise) …` line + * they will never see. Clicking the gear on a choice whose `type` is corrupt, for + * instance, threw `Invalid choice type` and simply did nothing (#1576). + * + * This is one seam instead of forty `try/catch` blocks. A per-handler wrapper is exactly + * the boilerplate #1567 failed to converge on: it cannot cover Svelte prop handlers or + * floated calls, and it rots the moment someone adds handler forty-one. The trade-off + * accepted is that the notice carries no per-action context, only the error itself. + */ + +/** + * Suppress a repeat of the same failure for this long. Several of the floated calls run + * at high frequency - a validator on every keystroke, a reindex on every vault change, + * a Svelte unmount on every modal close - so without this a single broken one would bury + * the user under 15-second notices. + */ +const REPEAT_WINDOW_MS = 10_000; +/** Bound the dedupe map so a pathological run cannot grow it without limit. */ +const MAX_TRACKED = 50; + +export interface UnhandledRejectionReporterHost { + manifest: { id: string }; + registerDomEvent( + el: Window, + type: "unhandledrejection", + callback: (event: PromiseRejectionEvent) => void, + ): void; +} + +/** + * Dedupe on the throw SITE, not the message. + * + * A message often embeds a varying value ("Could not read note "), so keying on it + * would let one broken loop over 500 notes raise 500 notices. The first `plugin:` + * frame is where the Error was constructed, so the same bug collapses to one report + * however many values it fails on, while two genuinely different bugs stay distinct. + */ +function dedupeKey(error: Error, pluginId: string): string { + const frame = (error.stack ?? "") + .split("\n") + .find((line) => line.includes(`plugin:${pluginId}:`)); + return `${error.name}@${frame?.trim() ?? error.message}`; +} + +/** + * True only when the rejection demonstrably came from QuickAdd's own bundle. + * + * Obsidian evaluates a plugin's `main.js` with a `sourceURL` of `plugin:`, so any + * Error constructed inside QuickAdd carries frames like + * `at mS.getChoiceByName (plugin:quickadd:414:30553)`. Requiring that keeps QuickAdd from + * claiming another plugin's bug, and is why a non-Error rejection (a bare string has no + * stack) is left alone: with nothing to attribute it to, reporting it would be a guess. + */ +function isFromPlugin(reason: unknown, pluginId: string): boolean { + return ( + reason instanceof Error && + typeof reason.stack === "string" && + // Trailing colon: the frame is `plugin:::`, so an undelimited + // match would claim `plugin:quickadd-beta` - a fork's or beta's rejections, + // whose only console line we would then suppress on their behalf. + reason.stack.includes(`plugin:${pluginId}:`) + ); +} + +export function registerUnhandledRejectionReporter( + plugin: UnhandledRejectionReporterHost, + /** + * Injected so the dedupe window can be tested without stubbing the global + * `Date.now` - vitest itself reads it for timeouts, and freezing it deadlocks the + * runner. + */ + now: () => number = Date.now, +): void { + const pluginId = plugin.manifest.id; + const recentlySeen = new Map(); + + plugin.registerDomEvent(window, "unhandledrejection", (event) => { + const reason = event.reason; + if (!isFromPlugin(reason, pluginId)) return; + + // A dismissed prompt is not a failure. It reaches here when a handler floats a + // prompt (e.g. the macro editor's script picker), and telling the user "an + // error occurred" because they pressed Escape would be worse than the silence + // this replaces. + // + // Only a USER cancellation is silenced, not every MacroAbortError. Its other + // subclass, ChoiceAbortError, is how QuickAdd reports involuntary aborts that + // carry copy the user needs ("Selected folder not allowed.", "…re-run with the + // ui flag."), and the rest of the plugin keeps the two apart everywhere it + // matters - choiceExecutor's cancelKind, macroAbortHandler's notice + // suppression, x-cancel vs x-error. Swallowing those here would leave a floated + // involuntary abort with LESS signal than the console line it replaces. + if (isCancellationError(reason)) { + // Claimed: a dismissal is not something the console should shout about. + event.preventDefault(); + return; + } + + const error = reason as Error; + const key = dedupeKey(error, pluginId); + const at = now(); + const last = recentlySeen.get(key); + // Stamp every OCCURRENCE, not every report. Stamping only on report restarts + // the window each time, so a site failing continuously would raise a notice + // every REPEAT_WINDOW_MS forever; stamping here makes the window slide, so a + // sustained failure reports once and then stays quiet until it stops and recurs. + recentlySeen.set(key, at); + if (last !== undefined && at - last < REPEAT_WINDOW_MS) { + // Deliberately NOT preventDefault: a suppressed repeat keeps the browser's + // own unhandled-rejection line, so silencing the notice never leaves the + // occurrence with less evidence than before this reporter existed. + return; + } + + if (recentlySeen.size >= MAX_TRACKED) { + for (const [tracked, seenAt] of recentlySeen) { + if (at - seenAt >= REPEAT_WINDOW_MS) recentlySeen.delete(tracked); + } + // Still full: every entry is fresh, so drop the oldest to make room. + if (recentlySeen.size >= MAX_TRACKED) { + const oldest = recentlySeen.keys().next(); + if (!oldest.done && oldest.value !== key) { + recentlySeen.delete(oldest.value); + } + } + } + + event.preventDefault(); + reportError(error, "A QuickAdd action failed"); + }); +}