fix: tell the user when a QuickAdd action fails, and stop signalling cancellation with English strings - #1606
Merged
Merged
Conversation
…glish sentence A dismissed QuickAdd prompt used to reject with a bare English string, and `isCancellationError` decided "did the user cancel?" by comparing the caught value against three literals - two of which were case variants of the same sentence. Rewording any of them, or adding a prompt with a slightly different one, silently reclassified "the user backed out" as "an error occurred", with no compiler or test signal (#1577). All nine reject sites now throw `promptCancelled()`, a `UserCancelError`. That is deliberately the SAME class the ~40 consumers already converted the string into, rather than a new one, for two reasons: it is what the public docs already promise (`MacroAbortError("Input cancelled by user")` - `UserCancelError` inherits that name), and because `UserCancelError extends MacroAbortError` a dismissal now arrives already classified, so a consumer that forgets to convert it still aborts quietly instead of reporting a failure. Two consumers needed real changes rather than none: - `choiceRename` gated its error logging on `instanceof Error`, which was a fine proxy for "not a cancellation" while cancellations were strings. It would now log every cancelled rename as an error, so it asks the contract instead. - `runOnePagePreflight` compared against the literal `"cancelled"` alongside its `instanceof MacroAbortError` check; the instanceof check alone now covers both the native modal and a remote provider. The legacy sentinel list stays in `isCancellationError`, narrowed to its real purpose: a *user script* can still throw one, and MacroChoiceEngine has honoured that for as long as the sentinels existed. `cancellationContract.test.ts` ratchets it - no source file may reject with a bare string literal or re-introduce a sentinel - so the fragility cannot come back by copy-paste. Verified in an isolated vault (Obsidian 1.13.0): dismissing the script picker in the macro editor now floats `Input cancelled by user` with a real stack instead of the bare string `no input given.` (which had no stack at all, and so could not be attributed or classified by anything downstream), and dismissing a `{{VALUE:}}` prompt still aborts the template run quietly with no note created and no error notice. Refs #1577
Seven of the nine prompt wrappers on the script API surface did
catch (error) { throwIfPromptCancelled(error); return undefined; }
and `throwIfPromptCancelled` returned normally for anything that was not a
MacroAbortError or a cancellation string. So every genuine failure - a throwing
display callback, a bad argument, an Obsidian API error - was handed back to the
script as `undefined`: nothing logged, nothing shown, and a value the script then
acted on. `yesNoPrompt` reported a defect as "the user answered No"; `infoDialog`
is declared `Promise<void>`, so there it was not merely unlogged but
unrepresentable (#1575).
The helper is now `rethrowPromptError(error): never` with a trailing
`throw error`, and all nine sites are identical - including the two in
`requestInputs` that already rethrew and were the model for this. `never` is the
load-bearing part: the swallow existed because a `void` helper let a wrapper
forget the rethrow, and now that omission is a compile error.
Deliberately NOT reported inside the wrapper. Every path that runs a script
already reports a propagating error exactly once (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 two or three notices for one failure.
Nothing documented `undefined`: every signature in QuickAddAPI.md is
non-optional, cancellation is documented as a MacroAbortError rejection, and
UserScripts.md already tells scripts `throw error; // real errors should still
bubble up` - which these seven methods made impossible. The one doc example that
contradicted this ("if (!input) { // User cancelled") has been wrong since #976
and is rewritten to state the two-outcome contract.
Verified in an isolated vault with a user script whose suggester display callback
has a real TypeError. Before: QuickAdd swallowed it, the macro carried on, and
wrote a note reading "the script carried on with: undefined" - no notice, nothing
in the console. After: no note, and exactly one notice naming the script and the
actual TypeError.
Refs #1575
…he console
Around forty `onClick(async …)` / `onclick={…}` handlers hand Obsidian or Svelte a
promise it discards, plus a long tail of deliberately fire-and-forget
`void someAsyncCall()`. When one failed the user got nothing: no notice, no entry
in QuickAdd's error log, only an unhandled rejection they will never see (#1576).
One seam, not forty try/catch blocks. `registerUnhandledRejectionReporter`
listens once on `window` (via `registerDomEvent`, so it unregisters on unload)
and reports through the same channel as every other failure. 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.
Three properties make it safe rather than noisy:
- Attribution. Obsidian evaluates a plugin's main.js with a `sourceURL` of
`plugin:<id>`, so an Error built inside QuickAdd carries frames like
`at mS.getChoiceByName (plugin:quickadd:414:30553)`. Verified live in Obsidian
1.13.0. Anything without that frame is left completely alone - including a
non-Error rejection, which has no stack to attribute at all.
- Cancellations stay silent. A floated prompt (the macro editor's script picker
floats one) must not tell the user "an error occurred" because they pressed
Escape. This is why the typed cancellation had to land first: the dismissal used
to be a bare string with no stack, so it was neither attributable nor
classifiable here.
- Dedupe by throw SITE, not message, within a 10s window. Several floated calls
run at high frequency - a validator on every keystroke, a reindex on every vault
change, a Svelte unmount on every modal close - and messages often embed a
varying value, so one loop failing over 500 notes would otherwise raise 500
fifteen-second notices. Same site collapses to one report; distinct sites stay
distinct.
We claim the event (`preventDefault`) for rejections we recognise so the browser
does not also report what we just reported; the stack is not lost, because
ConsoleErrorLogger passes the original Error to `console.error`.
Verified in an isolated vault, comparing a build of 234a349 against this one in
the same instance, on a real user path: a choice whose `type` is corrupt (the
#1566 shape) made the settings gear button do absolutely nothing - `Invalid
choice type` escaped as an unhandled rejection with zero notices. Now it raises
"A QuickAdd action failed: Invalid choice type". Dismissing the macro editor's
script picker still raises no notice, and where it used to escape as the opaque
string `no input given.` it is now a typed `MacroAbortError: Input cancelled by
user` with a full stack.
The reporter takes an injected clock: stubbing the global `Date.now` in its test
deadlocks vitest, which reads it for its own timeouts.
Refs #1576
#1574's headline claim - that a remote client cannot express a dismissed Yes/No - is wrong, and this commit does not "add" a cancel channel. `submitReply` has taken a per-prompt `cancelled` flag since the seam landed (b280fcd / #1471), it rejects that one prompt with UserCancelError, it is documented at CLI.md, and it is tested. The issue cites interactivePromptServer.ts:440, which is inside `submitReply`, not `finish()` (`finish` rejects with a plain "Interactive session ended"). Proved end-to-end over real loopback HTTP against the isolated vault: replying `{"cancelled":true}` to a `confirm` prompt ends the run with `{"kind":"error","error":"Input cancelled by user"}` and the script's else-branch never runs - the same class and message the in-app path throws. What IS real on that seam is the leniency underneath the claim. `RemotePromptProvider.yesNoPrompt` collapsed every non-`true` reply to `false`. So a client that omitted `value`, sent `null` (exactly the shape #1574 proposed for a dismissal), or made a typo silently answered "No", and the script walked its else-branch on an answer the user never gave. Yes/No is the one prompt where a malformed reply is indistinguishable from a real one - in-app, `false` can only come from clicking No - so it is the one prompt that now validates, and a dismissal already has a wire form to use instead. The other types stay lenient on purpose: `""` and `[]` are answers a user really can give (Skip affordances, optional fields), so tightening them would break optional prompts on remote runs. `cancelled` is also now strictly `=== true`. It arrives from untrusted JSON, and the loose truthy check meant `"cancelled":"no"` killed the run, and a cancel beat a real `value` sent alongside it. The documented wire form has always been `true`. Live matrix over real HTTP, one session per row: {"cancelled":true} -> error "Input cancelled by user", no file written {"value":true} -> done, script took the Yes branch {"value":false} -> done, script took the No branch {"value":null} -> error naming the fix, was a silent "No" {"cancelled":"no","value":true} -> done on the real value, was a spurious abort Also pins what was structurally untestable: the provider's test stub could only resolve, so nothing asserted that a client cancel propagates. There is now a rejecting stub and a case per method - including `requestInputs` - plus a wire-level case for the strict flag. Two honest notes rather than code changes. The file's "a script cannot tell it was driven remotely" claim now states how a dismissal fits it, and records the one genuine asymmetry: cancelling an `info` prompt aborts the run, while GenericInfoDialog in-app has no reject path at all. That stays, because it is a remote client's only way to bail out mid-run; CLI.md now tells clients to send a plain reply when they only mean to close an info panel. Closes #1574
…still fix it
Adversarial review found that the previous commit's strict `cancelled === true`
traded one invented answer for another. Any non-literal flag fell through to
`pending.resolve(value)`, and with no `value` present - which is exactly the shape
a mistaken cancel has - `textPrompt` maps `undefined` to `""`, `requestInputs` to
`{}`, `checkboxPrompt` to `[]`. So a client that meant "the user dismissed this"
but wrote `"cancelled":"true"` got a run that completed with empty inputs and a
`200 {"ok":true}` telling it everything was fine. That is the same harm the commit
set out to remove, moved from `confirm.value` to the flag. Two reviewers found it
independently.
The fix is to validate at `/reply` instead of deeper in. There the client is still
holding the HTTP response, so it gets a `400` naming the problem and the prompt
stays PENDING - it can correct itself and POST again. Deeper in, the only options
were to invent an answer or to fail the whole run with a message the client may
never see: on the Template/Capture path a thrown message is replaced by "Choice
execution failed; no file was created" before it reaches the poll stream, so the
actionable sentence became a desktop notice nobody is looking at. That was the
second blocker, and moving validation to the wire closes both.
Two rules, both where a wrong answer is indistinguishable from a real one:
a present-but-non-boolean `cancelled`, and a `confirm` whose value is not
boolean-ish. Everything else stays lenient - `""` and `[]` are answers a user
genuinely gives in-app via the Skip affordances and optional fields, so rejecting
them would break optional prompts on remote runs. `submitWireReply` is the single
path the HTTP layer and the tests share, so no test can pass through a shape
production never sees.
The provider-level throw stays as the backstop for non-HTTP callers, now
documented as such rather than as the primary check.
Live matrix over real loopback HTTP, one session per row:
{"cancelled":true} -> 200, run ends "Input cancelled by user", nothing written
{"value":true} -> 200, Yes branch
{"value":false} -> 200, No branch
{"value":null} -> 400 "…needs a boolean reply, or {"cancelled": true}…",
then the corrected {"value":false} is accepted, 200
{"cancelled":"true"} -> 400 "…must be the literal true…", then corrected, 200
Also from the same review:
- The unhandled-rejection reporter silenced every MacroAbortError. Only a USER
cancellation is silenced now: ChoiceAbortError carries copy the user needs
("Selected folder not allowed."), and the rest of the plugin keeps the two apart
everywhere it matters, so swallowing it left a floated involuntary abort with
LESS signal than the console line it replaced.
- The #1577 ratchet missed the shape most likely to reintroduce the bug -
`rejectPromise(new Error("dismissed"))`, an Error `isCancellationError` cannot
recognise. It now asserts the positive invariant (inside the prompt surfaces the
only permitted rejection is `promptCancelled()`), strips comments before scanning
so it stops matching prose about throws, catches a bare `throw "…"`, and has a
guard-the-guard case so a moved directory cannot make it pass vacuously. Verified
by hand that it now fails for the Error, bare-string and indirected-constant
shapes.
- The reporter's negative tests passed against an implementation that registered
nothing: `listener?.()` on a null listener produces exactly the values they
asserted. The listener is now required, and the "bounds the map" test - which
asserted nothing about bounding - is replaced by two that pin the real collapse
and eviction behaviour.
- `choiceRename`, the one regression this PR identified, had no test: the whole
suite stayed green with the fix reverted. Covered both ways, and verified the new
cases fail when reverted.
- `runTemplateFromFolder` was still stubbing the picker with the old bare string,
so nothing fed a typed cancellation to a `return null` consumer. Both forms are
covered now, plus the genuine-failure case.
- CLI.md's new section contradicted itself (it promised a bad flag was "ignored",
which was true for no prompt type after this change) and is rewritten around the
400.
Refs #1574 #1576 #1577
…ming them Second adversarial pass, all four findings verified live in the isolated vault. - The dedupe window restarted on every REPORT instead of sliding on every OCCURRENCE, so a continuously failing site raised a 15-second notice every 10 seconds forever - six a minute, worse than the silence it replaced. The timestamp is now stamped on each occurrence, so a sustained failure reports once and goes quiet until it stops and recurs. Verified: five rapid occurrences -> one notice. - `preventDefault()` ran before the report decision, so a SUPPRESSED repeat lost the browser's unhandled-rejection line as well as the notice - strictly less evidence than master for occurrences 2..n. It now only claims the event when it actually handles it (report, or a silenced cancellation). - `plugin:quickadd` is a prefix of `plugin:quickadd-beta`, so an undelimited match claimed a fork's or beta's rejections and suppressed their only console line. Matching `plugin:<id>:` fixes both this and the dedupe key. Verified: a `plugin:quickadd-beta` rejection now produces nothing. - The eviction path could drop the key it had just stamped, which would make the currently-failing site report on every occurrence. It skips that key now. Also: the debounced settings write floated `saveData()` from a NON-async arrow, so the rejection's stack carried no QuickAdd frame at all (it is constructed inside Obsidian's FS plumbing after an await) and the reporter could not attribute it. That is the one float where the user most needs to hear about a failure - their settings silently did not persist. Awaiting inside an async IIFE puts a QuickAdd frame on the stack. Verified: a throwing `saveData` now raises "A QuickAdd action failed: disk full", where before it produced nothing. Three comments corrected, because a comment that justifies a decision on a false premise is worse than no comment: - `rethrowPromptError` claimed every path reports a propagating error "exactly once". It is at least once: measured on the real command-palette path, a user script error raises TWO notices (MacroChoiceEngine reports and rethrows into main.ts, which reports again). That is pre-existing, is now filed separately, and it strengthens rather than weakens the reason not to add a third report here. My earlier verification of #1575 said "exactly one notice" - it went through `api.executeChoice`, which skips main.ts's handler. Corrected. - `previewDiagnostics`' message fallback does not cover a `toError(err, context)` wrap (the context prefix matches no sentinel). What it covers is a user script throwing an Error whose message IS a legacy sentinel. Said so, so nobody re-attempts the reverted commit. - The sentinel scan's comment claimed it included "cancelled"; it does not, and should not - as a bare substring it is far too common in src/. The reject-call invariant is what guards that modal. Refs #1576
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR standardizes typed prompt cancellation, propagates genuine prompt failures, validates remote replies, and adds deduplicated reporting for unhandled QuickAdd rejections. Documentation and tests define the updated cancellation, reply, and error-handling contracts. ChangesPrompt contracts and propagation
Remote prompt validation
Unhandled rejection reporting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chhoumann
marked this pull request as ready for review
July 27, 2026 13:09
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c63f35cd80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Docs deploy from master immediately while plugin releases are cut manually, so a page describing behaviour nobody has installed yet misleads current users - AGENTS.md requires an unreleased marker for exactly that window. Flagged in review on #1606. The two windows that matter: the interactive seam's 400-and-retry semantics (the old server consumed `cancelled: "true"` as a cancellation and read a value-less confirm as "No"), and the script API's two-outcome contract (a non-cancellation failure used to resolve `undefined`).
Deploying quickadd with
|
| Latest commit: |
69f600a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5158658a.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1574-cancel-contra.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/interactive/interactivePromptServer.ts (2)
223-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm-reply boolean validation and its error-message helper are duplicated across the wire layer and the provider layer. Both
describeReplyProblem/describeValueininteractivePromptServer.tsanddescribeReply/yesNoPrompt's inline check inpromptProvider.tsindependently encode the same "accepttrue/false/"true"/"false", else reject" rule and the same error text — an invariant the code comments say must hold ("the same rule is enforced at/reply"), but nothing enforces it beyond manual sync.
src/interactive/interactivePromptServer.ts#L223-L283: extract the confirm-boolean check and the value-describing logic (describeValue) into a small shared helper module.src/interactive/promptProvider.ts#L39-L48: replacedescribeReplywith the shared value-describing helper.src/interactive/promptProvider.ts#L264-L299: replace the inlinetrue/"true"/false/"false"check and duplicated error string inyesNoPromptwith a call into the same shared confirm-validation helper used bydescribeReplyProblem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interactive/interactivePromptServer.ts` around lines 223 - 283, Extract the shared confirm-boolean validation and safe value-description logic from describeReplyProblem and describeValue in src/interactive/interactivePromptServer.ts into a helper module. Update src/interactive/promptProvider.ts at lines 39-48 to use the shared value-describing helper, and at lines 264-299 replace yesNoPrompt’s inline validation and error text with the shared confirm-validation helper, preserving acceptance of true, false, "true", and "false".
493-521: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUpdate
submitReply’s JSDoc from stale shared-path language.
submitReplyis the internal resolver path; wire replies always go throughsubmitWireReplyfirst, and the tests now usesubmitWireReply. Remove or change the JSDoc claim that it is the shared HTTP/test path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interactive/interactivePromptServer.ts` around lines 493 - 521, Update the JSDoc for submitReply to describe it as the internal pending-prompt resolver, removing the stale claim that it is shared by the HTTP layer and tests. Preserve the existing behavior and the documentation that it returns true when a matching pending prompt is found.src/quickAddApi.ts (1)
933-946: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: reuse the
promptCancelled()factory instead of duplicating its construction.Both
yesNoPrompt(line 944) andrethrowPromptError(line 1051) manually buildnew UserCancelError(PROMPT_CANCELLED_MESSAGE)rather than calling the existingpromptCancelled()factory that exists precisely for this. Purely cosmetic today (no constructor logic differs), but centralizing it avoids the two literal construction sites drifting if the cancellation value ever gains metadata.♻️ Optional consolidation
-import { - PROMPT_CANCELLED_MESSAGE, - UserCancelError, -} from "./errors/UserCancelError"; +import { + promptCancelled, + UserCancelError, +} from "./errors/UserCancelError"; ... - if (answer === null) throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); + if (answer === null) throw promptCancelled(); ... - if (isCancellationError(error)) { - throw new UserCancelError(PROMPT_CANCELLED_MESSAGE); - } + if (isCancellationError(error)) { + throw promptCancelled(); + }Also applies to: 1046-1054
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickAddApi.ts` around lines 933 - 946, Replace the direct UserCancelError construction in yesNoPrompt and rethrowPromptError with the existing promptCancelled() factory. Preserve the current cancellation behavior while centralizing creation through that factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/cancellationContract.test.ts`:
- Around line 54-73: Update the REJECT_CALL pattern and its handling in the
“every prompt rejection is promptCancelled()” test so rejection arguments
spanning multiple lines are captured and validated instead of skipped. Preserve
the existing exemption for reject callback bindings, and add regression coverage
for multiline string and Error rejection arguments that must be reported as
offenders.
---
Nitpick comments:
In `@src/interactive/interactivePromptServer.ts`:
- Around line 223-283: Extract the shared confirm-boolean validation and safe
value-description logic from describeReplyProblem and describeValue in
src/interactive/interactivePromptServer.ts into a helper module. Update
src/interactive/promptProvider.ts at lines 39-48 to use the shared
value-describing helper, and at lines 264-299 replace yesNoPrompt’s inline
validation and error text with the shared confirm-validation helper, preserving
acceptance of true, false, "true", and "false".
- Around line 493-521: Update the JSDoc for submitReply to describe it as the
internal pending-prompt resolver, removing the stale claim that it is shared by
the HTTP layer and tests. Preserve the existing behavior and the documentation
that it returns true when a matching pending prompt is found.
In `@src/quickAddApi.ts`:
- Around line 933-946: Replace the direct UserCancelError construction in
yesNoPrompt and rethrowPromptError with the existing promptCancelled() factory.
Preserve the current cancellation behavior while centralizing creation through
that factory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 371b1328-9b0a-4f61-ae71-cf704d260150
📒 Files selected for processing (33)
docs/src/content/docs/docs/Advanced/CLI.mddocs/src/content/docs/docs/QuickAddAPI.mdsrc/engine/MacroChoiceEngine.entry.test.tssrc/engine/runTemplateFromFolder.test.tssrc/errors/UserCancelError.tssrc/formatters/previewDiagnostics.tssrc/gui/GenericCheckboxPrompt/genericCheckboxPrompt.audit-api-prompts.test.tssrc/gui/GenericCheckboxPrompt/genericCheckboxPrompt.tssrc/gui/GenericInputPrompt/GenericInputPrompt.tssrc/gui/GenericSuggester/genericSuggester.tssrc/gui/GenericWideInputPrompt/GenericWideInputPrompt.test.tssrc/gui/GenericWideInputPrompt/GenericWideInputPrompt.tssrc/gui/InputSuggester/inputSuggester.tssrc/gui/MathModal.tssrc/gui/MultiChoiceSettingsModal.tssrc/gui/MultiSuggester/multiSuggester.tssrc/gui/choiceRename.test.tssrc/gui/choiceRename.tssrc/interactive/interactivePromptServer.test.tssrc/interactive/interactivePromptServer.tssrc/interactive/promptProvider.test.tssrc/interactive/promptProvider.tssrc/main.tssrc/preflight/OnePageInputModal.test.tssrc/preflight/OnePageInputModal.tssrc/preflight/runOnePagePreflight.fallback.test.tssrc/preflight/runOnePagePreflight.tssrc/quickAddApi.test.tssrc/quickAddApi.tssrc/utils/cancellationContract.test.tssrc/utils/errorUtils.tssrc/utils/unhandledRejectionReporter.test.tssrc/utils/unhandledRejectionReporter.ts
The reject-call pattern was line-bounded, so an argument split across lines captured as EMPTY - and the empty capture is the exemption for an executor binding (`new Promise((resolve, reject) => …)`), so a multiline `rejectPromise(\n "dismissed",\n)` was waved straight through the guard that exists to catch it. Flagged in review on #1606. The pattern now spans lines and stops at the closing paren, the exemption keys on the binding shape rather than on emptiness, and it accepts optional-call syntax (`rejectPromise?.(…)`), which it also silently missed. Guard the guard: the detector is extracted from the directory walk and tested against the seven shapes that would reintroduce #1577 (bare string, unrecognisable Error, indirected constant, optional call, raw binding, and both multiline forms) plus the four it must allow (the contract, the contract multiline, an executor binding, and a comment about the old shape). A ratchet that silently stops matching is worse than no ratchet, and this one had already been wrong twice.
This was referenced Jul 27, 2026
chhoumann
added a commit
that referenced
this pull request
Jul 27, 2026
…essage that helps (#1617) * fix(ui): a broken renderItem no longer grows its own message or buries the user `toError(err)` with no context returns the CALLER's Error unchanged, and both suggesters then assigned to `err.message`. `renderSuggestion` runs once per visible row and re-runs on every keystroke, so a `renderItem` that rethrows one cached Error grew the prefix without bound - and because `log.logWarning` raises a Notice, it did so once per row per keypress. Measured live in the isolated vault: three items over three renders put NINE stacked notices on screen, the last carrying nine copies of "Custom renderItem threw an error; falling back to default rendering. " (#1604). Both halves fixed at one seam, `createRenderFallbackWarner`: the context goes to `toError`, which builds a fresh Error preserving name and stack - exactly what `toError`'s own docstring says it stopped doing for this reason - and the warner fires once per call site. One broken callback is one defect, and the fallback rendering it triggers is identical for every row. Also deletes the second `toError` in logManager. Its existence is why this happened: the two suggesters imported THAT one, a context-less twin, so the safe helper's "do NOT mutate the caller's Error" contract did not apply to the call sites that most needed it. One helper now. Refs #1604 * fix: report a QuickAdd failure once, and never report a dismissal as one One user-script failure raised TWO 15-second notices for one bug (#1601): QuickAdd: (ERROR) Failed to run user script probe.js: Cannot read properties… QuickAdd: (ERROR) Error executing choice probe-1601: Cannot read properties… MacroChoiceEngine reports and re-throws, and the command-palette handler reports again. Both layers are right to report - neither can know whether anything above it will - so "report once" now belongs to the function they both call. `reportError` remembers the values it has shown, keyed on identity rather than text, so the same failure through five layers reports once while two independent failures that happen to read alike still both report. It walks the `cause` chain, because not every layer re-throws the same instance: the AI request path reports the provider error and then throws a wrapper carrying it as `cause` (both its sites report through `reportError` now, so that pair collapses too). The user-script LOAD path had the same doubling and reached neither reporting layer - `getUserScript` throws straight past MacroChoiceEngine's catch - so a typo'd `require` stacked two notices carrying the same 300-character message. It reports through `reportError` now. Also fixes what the outermost handlers did with a dismissal. Escape on the one-page input modal raised, live: QuickAdd: (ERROR) Error executing choice onepage-s2: One-page input cancelled by user - a red 15-second notice for a deliberate Escape, which is exactly what PR #1606's first rule exists to prevent. `reportUnlessCancelled` is that rule at the three sites that catch a whole run: the command callback, the legacy URI path, and the picker. Those handlers now name the choice instead of its UUID, and the picker's two template-stringified `log.logError` calls became real reports - interpolating an error throws its stack away, and only a value passed through `reportError` can take part in report-once. Refs #1601 * fix: attribute an unhandled rejection to the plugin that constructed it The reporter claimed a rejection if `plugin:quickadd:` appeared ANYWHERE in the stack. `Error.stack` is captured at construction with the whole live call stack, so a foreign caller's bug that merely ran through QuickAdd was claimed as ours - and `preventDefault` took away the console line naming the real culprit (#1602). Reproduced with a second plugin calling `quickadd.api.suggester(v => v.nope.trim(), items)` and floating it: TypeError: Cannot read properties of undefined (reading 'trim') at eval (plugin:probe-foreign:19:43) <- construction site: THEIRS at eval (plugin:quickadd:88:3004) at r.suggester (plugin:quickadd:88:2988) at Object.callback (plugin:probe-foreign:19:19) The topmost frame naming ANY plugin bundle now decides. The issue proposed requiring the first frame to be ours and expected worse false negatives; measured, there are none. An Error constructed inside Obsidian's own async plumbing (`vault.create` into a missing folder, awaited from a plugin) carries no plugin frame at all - not even the caller's - so "any frame" was never catching that class either: Error: ENOENT: no such file or directory, open '…/definitely/missing/folder/x.md' at async open (node:internal/original-fs/promises:636:25) The only thing "any frame" caught and this does not is a foreign frame above ours, which is the false positive itself. QuickAdd's own bug reached through another plugin still reports, verified live. Two details the shape depends on: - The `Name: message` header is stripped instead of filtering for `at ` frames. QuickAdd is `isDesktopOnly: false` and Obsidian mobile runs JavaScriptCore, whose frames are `fn@url:line:col` with no `at ` and no header - an `at `-keyed filter would have left this whole reporter dead on iOS with every desktop-shaped test still green. Stripping the header still stops an Error naming a plugin in its own message from dictating the blame. Both shapes are pinned by tests. - The plugin id is read without requiring a trailing `:line:col`, because an eval'd frame names the bundle as an origin with no position: `eval at exports.load (plugin:quickadd)` is a user script's own code and `eval at <anonymous> (plugin:dataview)` is a dataviewjs snippet. Those are exactly the frames attribution must not skip past. `plugin:quickadd-beta` still reads as a different plugin. `preventDefault` is now gated on a report actually going out, since `reportError` can drop a failure a lower layer already showed the user - claiming the event anyway would kill the browser's async trace and put nothing in its place, the opposite of the guarantee the repeat-window branch keeps. Refs #1602 * fix(cli): tell a headless caller why a Template or Capture run failed On an interactive run - the seam whose entire premise is that nobody is at the desktop - a genuine failure reached the client as: {"kind":"error","error":"Choice execution failed; no file was created."} while the actionable sentence (`Template file not found at path "templates/x.md".`) went to a notice on a screen nobody was watching (#1603). The Template and Capture engines report a failure and return WITHOUT recording an outcome, so `executeWithOutcome` produced a reason-less `{status:"error"}` and the CLI substituted a fixed sentence. The Macro branch already forwarded `error.message`; only Template and Capture lost it. `ChoiceOutcome`'s error variant now carries `reason`, exactly as the abort path already does for `cancelKind:"aborted"`, and both CLI sites forward it - the interactive bridge and `runResolvedChoice`, which serves `quickadd:run-template` and `quickadd:run verify=…` and had the identical bug through a different command. The URI x-callback handler keeps ignoring it on both variants, so no vault detail leaks to an external callback URL. The reason is recorded at EVERY failure exit, not just the top-level catch. Five of the six were silent returns - a missing append-link destination, a target path occupied by a non-markdown file, an unresolvable file-exists behaviour, a failed create - each producing the same reason-less outcome without any exception, so fixing only the catch would have left #1603 reproducible on the more common paths. The fixed sentence is now a fallback for an outcome that carries nothing, which nothing in the engines produces. Two details worth naming: - Success is recorded at each engine's commit point precisely so a post-commit link or open-file failure cannot make an automation retry and duplicate the side effect. That terminality lives in the recorder, not in `run()`: Capture commits from two methods, and the canvas path keeps going into steps whose throws unwind into `run()`'s catch. - `createFileWithTemplate` reports the real cause and returns null, so its caller knew only THAT creation failed. It now hands the cause up, and the caller records it without logging a second, vaguer line. Verified live in the isolated vault - a Template choice pointing at a missing template, over real loopback HTTP and via the verified CLI path - both now answer `Template file not found at path "templates/does-not-exist.md".`, and the desktop shows one notice saying the same thing. Refs #1603 * feat(cli): closing a remote info panel no longer ends the run; POST /abort does `promptProvider.ts` opens by claiming each method "returns exactly what its in-app counterpart returns, so a script cannot tell it was driven remotely". `infoDialog` broke that. `GenericInfoDialog` resolves on EVERY close path and has no reject path at all, so it can never abort anything - but remotely, replying `{"cancelled":true}` to an `info` prompt rejected with `UserCancelError` and ended the run. Escape is the only gesture an info panel affords, and a client maps it to a cancel, so the same choice that finishes in the app died remotely (#1605). Measured both ways in the isolated vault before the fix: in-app Escape ran the macro to completion; the remote cancel returned `{"kind":"error","error":"Input cancelled by user"}` with the remaining steps never run. #1574 documented this instead of changing it, because cancelling was a client's only explicit way out mid-run. So the protocol gets a real one first: POST /abort?session=…&token=… -> {"ok":true,"interrupted":<n>} It rejects every pending prompt with `UserCancelError` and marks the session, so a prompt raised afterwards rejects immediately rather than parking - a run that was mid-work unwinds at its next prompt. It pushes no final event: the run unwinds through the ordinary cancellation path and delivers its REAL outcome, which is more truthful than a fabricated one. `interrupted` is there because the abort is not omnipotent: a Template/Capture run still opens some prompts in Obsidian itself (the file-exists chooser, the folder picker, the capture-target picker) which never travel over this bridge, so `0` tells a client it stopped nothing. That limit is documented at the wire rather than glossed. The info exception is decided in `submitReply`, which already tracks each prompt's type for reply validation - not in `RemotePromptProvider.infoDialog`. A provider-side swallow cannot tell a per-prompt cancel from a session abort without a second error class, and would leave `/abort` unable to end a run blocked on an info panel, the one case it exists for. The handshake now carries `"capabilities":["abort"]`. Both halves of this are observable behaviour changes to a documented wire, and without a marker a client could only feature-detect by string-matching a 404 body - an unknown path and an unauthed session answer the same shape. Verified over real loopback HTTP: info cancel -> `200`, panel closes, run finishes `done`; `/abort` -> `200 {"interrupted":1}` then `error: Input cancelled by user`; `/abort` on an ended session -> `409`; `GET /abort` -> `404`; bad token -> `404`; browser `Origin` -> `403`. Closes #1605 * fix: report the message that helps, not just the first one Adversarial review of the four commits above found three ways the report-once rule picked the wrong survivor, and one way `/abort` misled its client. **The AI request path lost its remedy.** `OpenAIRequest` reported the raw provider error and then threw a wrapper carrying the provider name and QuickAdd's own context-window guidance ("shorten it, choose a model with a larger context, or use the chunked AI prompt API"). Report-once then dropped the wrapper as an already-reported cause, so a context-overflow failure showed only the bare provider string - the least informative half of a pair that used to show both. It now reports the WRAPPER, whose message is a strict superset, and the test asserts the logged message rather than a call count. **A macro failure did not say which macro.** The surviving report is the innermost one, so it has to carry enough context on its own. `MacroChoiceEngine` now names the choice alongside the script. That matters most where the outer handler is not there to help: a run-on-startup macro fails with no user action to correlate it with, and "Failed to run user script sync.js" alone does not say which of two startup macros broke. **#1603 was only fixed for one file-exists branch.** `lastTemplateFileFailure` was set by `createFileWithTemplate` alone, so Overwrite and Append - the paths taken whenever the target note already exists - still answered `Could not resolve file exists behavior for 'x.md'.` and still raised a second, vaguer notice. All three write helpers record their cause now. Verified live: a Template choice set to Overwrite with a missing template now answers `Template file not found at path "templates/gone.md".` with exactly one notice, where it previously gave the vague sentence plus two notices. **`/abort` could hand back a prompt it had just cancelled.** A prompt raised while no poll was parked sits in `session.queue`; aborting rejected the pending map but left the queue, so the next poll delivered a dialog the client had just asked to cancel, for a dead requestId, in front of the run's real terminal event. Queued prompts are dropped now; a queued done/error is kept, because that is the outcome the client is waiting for. Also from the review: - `/abort` is now driven through the real router in tests - POST 200 with `interrupted`, 409 on an ended session, 404 for GET and for a bad token - instead of calling `abortSession` directly, so the routing and status mapping this PR documents are actually pinned. The handshake's `capabilities` marker is asserted too. - The docs no longer promise `/abort` produces an `error` event: a run with nothing left to interrupt finishes normally and delivers `done`, side effects committed. `/abort`'s 409 and 404 responses are documented, and the `409` sentence further down is scoped to `/reply`. - `promptProvider`'s opening paragraph still claimed every method rejects on a client cancel, contradicting the paragraph below it. - The interactive server's tests leaked sessions (they live for 60s after `finish`), so the file was three tests away from failing on MAX_SESSIONS as a capacity error attributed to whichever test came 33rd. - Dead surface removed: Capture's `failRun` level parameter no caller passes, and `dedupeKey`'s message fallback, unreachable since attribution requires a frame. Refs #1601, #1602, #1603, #1605 * fix: expire a dedupe mark, and carry the canvas append reason Two review findings from Codex on the PR, both real. **Report-once must not be forever.** A long-lived user-script module that re-throws one cached `Error` on every invocation was reported the first time and then silently suppressed on every subsequent run - no notice, no console entry, a command that simply does nothing. That is the exact failure the reporting seam exists to remove, so suppression now expires after 10 seconds: the same window, for the same reason, as the unhandled-rejection reporter's. One propagation unwinds in microseconds, so the stacked notices in #1601 still collapse while separate runs stay separate. **The canvas/base append guard lost its reason.** Appending a template to a `.canvas` or `.base` file would splice raw text into structured JSON, so it is refused - with the most actionable sentence any of these exits produces, since it names the fix ("Use the Overwrite file-exists option instead"). It was the last report-and-return-null exit still handing a CLI or interactive caller "Could not resolve file exists behavior". Both are pinned by tests verified to fail against the unfixed source. Refs #1601, #1603 * fix(cli): drain the /abort request body before answering /abort takes no body, but a client may still send one, and it answered without reading - the only route that did. Unread bytes left on a keep-alive socket become the next request's problem. Node drains an unconsumed request itself when the response finishes, so this was not reachable in practice; reading it here makes the route independent of that internal and symmetric with /reply. Verified over a real keep-alive connection with one socket: `/abort` carrying a 2 KB body answers `200 {"ok":true,"interrupted":1}`, the next request on the SAME socket parses correctly, and the final poll still delivers the run's real outcome. The router test's request stand-in is now async-iterable, which is what a real IncomingMessage is - it 400'd against the drain otherwise, so the test was quietly looser than production. Refs #1605
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1574. Fixes #1575, #1576, #1577.
The problem
QuickAdd could not reliably tell "the user backed out" from "something broke", and when
something broke it often could not tell the user at all. The four issues are four faces of that:
against three literals - two of which were case variants of the same sentence. Rewording one, or
adding a prompt with a slightly different one, silently reclassified a cancellation as an error
with no compiler or test signal.
undefined, which reads to a script as "the user gave no input".onClick(async …)/onclick={…}handlers hand Obsidian or Svelte a promise itdiscards. When one failed the user got nothing: no notice, no log entry.
and I have closed it as such (evidence below) - but the leniency underneath it was real.
What a user sees
A settings action that silently did nothing now says what went wrong. A choice whose
typeiscorrupt (the #1566 shape - hand-edited or partially-migrated
data.json) made the gear buttoncompletely inert:
Invalid choice typeescaped as an unhandled rejection, zero notices.A script bug stops being reported as "the user gave no input". A user script whose
suggesterdisplay callback has a real
TypeError. Before: QuickAdd swallowed it, the macro carried on, andwrote a note reading
the script carried on with: undefined- nothing on screen, nothing in theconsole. After: no note, and the actual
TypeErrornamed alongside the script.undefinedThe contract
Three rules, applied across the call sites:
promptCancelled();isCancellationErroris aninstanceofcheck.user cancellation, which is silenced.
Design note: no new error class
#1577 proposes a
PromptCancelledError. I did not add one, becauseUserCancelErroralready isthat class: it is what all ~40 consumers already converted the string into, it is what the published
docs promise (
MacroAbortError("Input cancelled by user")-UserCancelErrorinherits that name, sothe documented
error?.name === "MacroAbortError"pattern keeps working), and it is what the remoteseam already rejects with. A third name for one concept is abstraction for a need that does not exist.
The consequence I accepted: a dismissal is now a
MacroAbortErrorsubclass at the throw site, soa consumer that forgets to classify it aborts quietly instead of reporting a failure. I audited every
transitive caller of the nine prompts; two needed real changes:
choiceRenamegated its logging oninstanceof Error, a fine proxy for "not a cancellation" whilecancellations were strings. It would have logged every cancelled rename as an error.
runOnePagePreflightcompared against the literal"cancelled"; itsinstanceof MacroAbortErrorcheck now covers both the native modal and a remote provider.
The legacy string list stays in
isCancellationError, narrowed to its real purpose: a user scriptcan still throw one, and
MacroChoiceEnginehas honoured that for as long as the sentinels existed.Design note: one seam, not forty try/catch blocks
For #1576 I chose a global
unhandledrejectionreporter over arunAction(fn)wrapper. The wrapperis exactly the boilerplate #1567 failed to converge on: it cannot cover Svelte prop handlers or
void-floated calls, and it rots the moment someone adds handler forty-one. Attribution is real, notspeculative - Obsidian evaluates a plugin's
main.jswith asourceURLofplugin:<id>, verifiedlive:
Anything without that frame is left completely alone. Trade-off accepted: the notice carries no
per-action context, only the error itself.
This is why the typed cancellation had to land first. The dismissal used to be a bare string with no
stack at all, so it was neither attributable nor classifiable here - measured before and after:
#1574: the headline claim is refuted
submitReplyhas taken a per-promptcancelledflag since the seam landed (b280fcd / #1471);it rejects that one prompt with
UserCancelError, it is documented atCLI.md, and it is tested. Theissue cites
interactivePromptServer.ts:440, which is insidesubmitReply, notfinish()(finishrejects with a plain
"Interactive session ended").What was real is the leniency underneath:
yesNoPromptcollapsed every non-truereply tofalse,so a client that omitted
value, sentnull(the shape #1574 itself proposes for a dismissal), ormade a typo silently answered "No" and the script walked its else-branch.
Malformed replies are now caught at
/replywith a400, while the client still holds the responseand the prompt stays pending so it can correct itself. Validating deeper in was my first attempt
and adversarial review killed it: on the Template/Capture path a thrown message is replaced by a
generic sentence before the client ever polls for it (now #1603).
Live matrix over real loopback HTTP, one session per row:
{"cancelled":true}200; run endsInput cancelled by user, nothing written{"value":true}200; Yes branch{"value":false}200; No branch{"value":null}400naming the fix, then the corrected reply is accepted{"cancelled":"true"}400naming the fix, then the corrected reply is acceptedOnly two things validate, both where a wrong answer is indistinguishable from a real one. Everything
else stays lenient on purpose:
""and[]are answers a user genuinely gives in-app via the Skipaffordances and optional fields, so rejecting them would break optional prompts on remote runs.
Validation
Everything below was run in this worktree's own isolated vault (Obsidian 1.13.0), comparing a build of
234a3493against this branch in the same instance.Invalid choice typeescaped with zero notices; after,one notice. Screenshots above.
the script carried on with: undefinedand no notice; after, no note and the realTypeError.no input given.in
dev:errors; after, a typedMacroAbortError: Input cancelled by userwith a full stack, and nonotice. Dismissing a
{{VALUE:}}prompt still aborts the template run quietly with no note created.before - see below); a
plugin:quickadd-betarejection is left alone; five rapid occurrences of onefailure raise exactly one notice.
tsc,eslint, and 4384 unit tests green.Adversarial review changed the design
Worth flagging, since two of these were self-inflicted:
cancelledflag resolve as an empty answer - the same harmit set out to remove, moved from
confirm.valueto the flag. Two reviewers found it independently.Hence the
400-at-the-wire redesign.so a continuously failing site would have raised a notice every 10 seconds forever.
preventDefault()ed before deciding, so a suppressed repeat lost the browser's console linetoo - strictly less evidence than master. It now only claims the event when it handles it.
plugin:quickaddis a prefix ofplugin:quickadd-beta; the match is delimited now.#1577ratchet missed the shape most likely to reintroduce the bug -rejectPromise(new Error("dismissed")). It now asserts the positive invariant (inside the promptsurfaces the only permitted rejection is
promptCancelled()), and I verified by hand that it failsfor the Error, bare-string and indirected-constant shapes.
choiceRename- the one regression this PR identified - had no test: the whole suite stayedgreen with the fix reverted. Covered both ways now, and verified the new cases fail on revert.
toErrorwitha context prefix matches no sentinel either, and nothing wraps on the way into that function).
A correction to my own earlier claim: I said the #1575 fix produces "exactly one notice". It does
not. On the real command-palette path a user-script failure raises two, because
MacroChoiceEnginereports and rethrows intomain.ts, which reports again. My first measurement wentthrough
api.executeChoice, which skips that handler. This is pre-existing, filed as #1601, and it iswhy
rethrowPromptErrordeliberately does not add a third report.Follow-ups filed rather than folded in
#1601 (two stacked notices) · #1602 (attribution on any frame, not the construction frame) · #1603
(interactive Template/Capture hides the real error) · #1604 (
renderItemhandler mutates the caughtError) · #1605 (cancelling a remote
infoprompt aborts a run the in-app dialog cannot).Known limitations
inside Obsidian's own async plumbing and then floated is still missed.
main.ts'srequestSaveisfixed by awaiting inside an async IIFE so a QuickAdd frame is on the stack; the general case is [BUG] An unhandled rejection is attributed to QuickAdd on any frame, not the one that constructed it #1602.
reporting again. That is the deliberate trade against unbounded growth, and it is pinned by a test.
infoprompt still aborts the run ([BUG] Cancelling a remote info prompt aborts a run the in-app dialog cannot abort #1605). Left as-is because it is a client'sonly explicit mid-run bail-out; documented at the wire instead of silently changed.
Summary by CodeRabbit
/replywire replies.