fix: say so when a macro step cannot be run, instead of dropping it - #1623
Open
chhoumann wants to merge 3 commits into
Open
fix: say so when a macro step cannot be run, instead of dropping it#1623chhoumann wants to merge 3 commits into
chhoumann wants to merge 3 commits into
Conversation
…ng it
`executeCommands` was a flat chain of `if (isXCommand(command))` guards with
no else, so a command type it did not know was skipped with no error, no
notice and no log - and the macro still reported success. The next step then
inherited the hole: a capture reading `{{VALUE:out}}` from the skipped step's
output variable falls through to `promptForVariable`, so the user gets a
modal titled "out" mid-macro asking them to type the missing value by hand,
and a non-interactive run aborts.
It is a switch now, with the `const exhaustiveCheck: never` idiom
`executeEditorCommand` already used, so adding a `CommandType` without a
handler is a tsc error rather than a silent hole. Unknown types are skipped
and shouted about rather than aborting the macro, matching what
`executeObsidianCommand` does for a command whose plugin is gone: the step
did nothing either way, and aborting would take the file-writing steps that
DID work with it.
Three shapes reach the new default branch, and they are not the same:
- a real type name QuickAdd does not know (a newer QuickAdd's data.json read
by an older one - which our own downgrade recipe produces - a hand-edited
data.json, or a hand-authored package) is quoted, because it is what the
user greps for;
- an entry with no usable type is described as such, never quoted, so the
notice cannot send anyone hunting for a command type called `undefined` or
`[object Object]`;
- a null entry is a hole rather than an entry, and stays silent, like the
same shape in package import.
`InfiniteAIAssistant` gets a named case saying it has never been runnable in
any released QuickAdd - it has no builder entry and no engine branch, so the
only way to hold one is a hand-edited data.json or a hand-authored package.
`executeEditorCommand`'s own default threw, which aborted the whole macro. It
has the same threat model, so it gets the same answer.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesMacro command safety
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MacroChoiceEngine
participant executeCommands
participant executeEditorCommand
participant reportUnrunnableCommand
MacroChoiceEngine->>executeCommands: execute macro steps
executeCommands->>reportUnrunnableCommand: report unknown macro command
executeCommands->>executeEditorCommand: dispatch editor command
executeEditorCommand->>reportUnrunnableCommand: report unknown editor command
reportUnrunnableCommand-->>executeCommands: continue remaining steps
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 |
Deploying quickadd with
|
| Latest commit: |
e0c022c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://df3ce7bb.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1571-exhaustive-co.quickadd.pages.dev |
chhoumann
marked this pull request as ready for review
July 27, 2026 19:57
…st one "The rest of the macro still ran" was logged BEFORE the rest ran, and when the skipped step is the last (or only) one, there is no rest. What the tail is actually for is telling the user the macro was not aborted, so what they got is partial - which is true wherever the step sits. Also pins the unnamed-command fallback. It existed to keep `'undefined'` out of a notice, and nothing failed when it was removed.
…bout A truthy non-object entry is the shape a shape-based guard would silently re-swallow. That silence is the hole this change closes, so it gets a pin.
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.
The problem
MacroChoiceEngine.executeCommandswas a flat chain ofif (isXCommand(command))guards with no else. A command type it did not know was dropped on the floor - no error, no notice, no log - and the macro still reported success."Silently skipped" understates what the user sees. Live repro in an isolated vault, macro =
Wait→InfiniteAIAssistant(outputVariableName: "out")→Capturewith formatAI said: {{VALUE:out}}:The skipped step never sets
out, so the capture's{{VALUE:out}}falls through toensureValueVariableResolved→promptForVariable, and a modal titled "out" opens mid-macro asking the user to type the AI result by hand. Non-interactive runs abort withChoiceAbortError. Nothing on screen said why.InfiniteAIAssistant(#1571) is one instance of a class. The same hole swallows any unrecognised type from:data.json,QuickAddPackage.tsvalidates only thatid/name/typeare strings),data.jsonwritten by a newer QuickAdd and read by an older one - which our own downgrade recipe produces.…including inside
Conditionalbranches, which recurse through the same loop.The change
A
switchoncommand.typewith theconst exhaustiveCheck: neveridiom this file already used inexecuteEditorCommand. Adding aCommandTypewithout a handler is now a tsc error, so the hole cannot come back:Skip and shout, not abort - matching
executeObsidianCommandfor a command whose plugin is gone. The step did nothing either way, and aborting would take the file-writing steps that did work with it.Three shapes reach the default branch and they are deliberately not treated alike:
data.json{name: "x"}, a non-stringtype)undefinedor[object Object]null/undefinedpackageImportService, and a red 15s notice per hole would bury the #1583 resilience work in noiseInfiniteAIAssistantgets a named case (thenevercheck forces one) with an honest message. Verified absolutely:git log -S"InfiniteAIAssistant" --all -- src/engine/is empty across all 202 tags, so "no released QuickAdd has ever been able to run one" is accurate, not rhetoric.Also fixed:
executeEditorCommand's owndefaultthrew, which aborted the entire macro. Same threat model (a newer QuickAdd adding anEditorCommandType), so it gets the same answer. Leaving it would have made PR-adjacent behaviour contradict itself: unknownCommandTypeskips politely, unknownEditorCommandTypekills the run.Deliberate non-changes
InfiniteAIAssistantto a plainAIAssistantis not inert: withpromptTemplate.enable: falseit opens a template picker and fires a billable request (or throws underdisableOnlineFeatures, the default). Dropping it deletes user data to fix a hazard nobody demonstrably has. A migration also runs once, while hand-editing and package import keep working afterwards - this fix covers the hazard permanently and generically.quickadd:runstill answersok: true. The macro ran; one step was skipped. The envelope already carriesverified: falsewith a comment explaining that a resolvingexecute()does not guarantee a file was written. Enumerating skipped commands in the envelope is a reasonable future addition, not this PR.executeObsidianCommandalready stacks one per missing command), and a macro holding several is exactly when seeing each matters.Evidence (isolated worktree vault,
pnpm run obsidian:e2e)Seeded macro with
[null, {name: "Half a command"} /* no type */, EditorCommand("FoldEverything"), Wait], thenquickadd:run:The
nullentry produced no notice; theWaitafter both skips still ran (662 ms).Unknown
CommandType, with a later capture step proving the macro carried on:Tests
New
src/engine/MacroChoiceEngine.unknownCommand.test.ts(10 cases): unknown type shouts and names all three origins; the next command still runs;InfiniteAIAssistantshouts; a shout from inside a conditionalthenbranch;runSubsetgets the same guard; a typeless/non-string type is never quoted;null/undefinedstay silent; known types still run exactly once each.tsc -noEmitclean,eslintclean,svelte-check0 errors, full suite 4658 passed.Part 1 of 2 for #1571. Part 2 removes the
InfiniteAIAssistanttype outright; this one is independently mergeable and is the part that generalises.Summary by CodeRabbit