fix: dismissing a Yes/No confirmation is an answer, not an error - #1573
Merged
Conversation
GenericYesNoPrompt rejected with the bare string "No answer given." when the user dismissed it (Esc, close button, click outside). Three call sites awaited it from handlers that discard the promise, so a normal cancel surfaced as "Uncaught (in promise) No answer given." in the console. Make the modal never reject. Prompt() is the confirmation helper - only an explicit Yes confirms, so a dismissal is simply "no" - and Ask() returns boolean | null for the one caller that must tell a dismissal from an explicit No: quickAddApi.yesNoPrompt, where dismissing still aborts the macro with UserCancelError exactly as before. Every confirmation call site is now correct by construction, so the hand-rolled try/catch guards in choiceService (#1562) and packageExportService are gone. Fixes #1567
|
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 (9)
📝 WalkthroughWalkthroughGenericYesNoPrompt now resolves dismissal as ChangesConfirmation dismissal handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant QuickAddApi
participant GenericYesNoPrompt
participant ObsidianModal
Caller->>QuickAddApi: yesNoPrompt()
QuickAddApi->>GenericYesNoPrompt: Ask()
GenericYesNoPrompt->>ObsidianModal: Open modal
ObsidianModal-->>GenericYesNoPrompt: Resolve true, false, or null
GenericYesNoPrompt-->>QuickAddApi: Return answer
QuickAddApi-->>Caller: Return boolean or raise cancellation
Possibly related issues
Possibly related PRs
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 06:16
Deploying quickadd with
|
| Latest commit: |
3aced7b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fc8d0847.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1567-yesno-contrac.quickadd.pages.dev |
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.
Fixes #1567.
The problem behind the issue
GenericYesNoPrompt.Promptrejected with the bare string"No answer given."when the user dismissed the dialog (Esc, the close button, a click outside). That makes the single most common user action on a confirmation - walking away from it - the exceptional path, and nothing in the signature (Promise<boolean>) says so. The result is a call site that is wrong by default: three of them loggedUncaught (in promise) No answer given.on every cancelled delete, and the next one added would have too.The issue proposed wrapping the three offenders in
try/catch. I don't think that's the fix. The repo already contains three independently hand-written copies of that same "dismiss = no" workaround, added on three different dates, and the bug still existed - the boilerplate was not converging. So this PR fixes the contract instead.The contract
The modal no longer rejects at all. Of the seven call sites, six are confirmations that want dismiss = No; they keep calling
Promptand are now correct by construction, so the hand-rolled guards inchoiceService(#1562) andpackageExportServiceare deleted rather than replicated.Exactly one caller needs the distinction:
quickAddApi.yesNoPrompt, the script-facing API. There, answering No is a value the script acts on, while dismissing must abort the macro - otherwiseif (await yesNoPrompt(...)) {...} else {...}would run its else-branch when the user meant "get me out of here". That caller usesAskand mapsnulltoUserCancelError, so the observable script behaviour is unchanged."No answer given."is removed fromisCancellationError's string list; nothing produces it any more.Why this shape, and what it doesn't do
falseand let the macro carry on. That's a real regression for the one caller that cares.AIToolConfirmModalalready documents "Dismissing (Esc / click-out) resolves to 'deny'", andModelDirectoryModal/AIAssistantCommandSettingsModalalready resolvenullon dismissal. Sentinel-on-dismiss is already the house style for confirmations;GenericYesNoPromptwas the outlier.GenericInputPrompt,GenericSuggester,GenericCheckboxPrompt, …) still reject with bare strings, and that is deliberately left alone: their return types (string,T,string[]) have no natural "cancelled" value, so rejection is a legitimate signal there.GenericYesNoPromptis genuinely the different one. Follow-ups are filed rather than folded in (listed below).Validation
All checks below were run in this worktree's own isolated Obsidian 1.13.0 e2e vault, with real DOM clicks and a real
Escapekeydown, and awindow.addEventListener("unhandledrejection", …)listener attached.Reproduced first, on the pre-fix build - Settings → QuickAdd → Configure a Macro → Delete command → Esc:
After - identical UI, identical outcome (the command is kept), no console noise:
(The panel is a test overlay printing the real captured
unhandledrejectionevents; the harness's owndev:errorsbuffer agrees -No answer given.before,No errors captured.after.)["No answer given."], command kept[], command kept["No answer given."][]["Wait"]→[])api.yesNoPrompt→ EscMacroAbortErrorMacroAbortErrorapi.yesNoPrompt→ No / Yesfalse/truefalse/truefalse, macro continuesPlus: full suite
4075 passed / 37 skipped,tsc --noEmitclean,eslintclean. The four new dismissal tests were mutation-checked - revertingonCloseto throw makes exactly those four fail and nothing else.Tests
The contract is pinned on the real modal (
GenericYesNoPrompt.test.ts), not on a mock, so a future refactor can't quietly break it while the suite stays green:Askresolvestrue/false/null,Promptresolvesfalseon dismissal, and neither rejects.quickAddApi.test.tspins the script-side contract, including that an explicit No is not an abort.Follow-ups (filed, not folded in)
RemotePromptProvider.yesNoPromptcannot express a dismissal at all, while its file docstring claims exact parity with the in-app method.throwIfPromptCancelledswallows genuine errors intoundefinedat all ninequickAddApiprompt wrappers.try/catch- which is precisely the boilerplate that failed to converge here.Summary by CodeRabbit