fix: stop macros and APIs immediately when cancellations occur#976
fix: stop macros and APIs immediately when cancellations occur#976
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a centralized abort signaling flow: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PromptUI
participant QuickAddApi
participant ChoiceExecutor
participant Engine
User->>PromptUI: Cancel prompt
PromptUI-->>QuickAddApi: rejection/cancel
QuickAddApi->>QuickAddApi: throwIfPromptCancelled(err) -> MacroAbortError
QuickAddApi->>ChoiceExecutor: signalAbort(MacroAbortError)
ChoiceExecutor->>ChoiceExecutor: store pendingAbort
Engine->>ChoiceExecutor: consumeAbortSignal()
ChoiceExecutor-->>Engine: MacroAbortError or null
alt MacroAbortError returned
Engine->>Engine: throw MacroAbortError (halt macro)
else
Engine->>Engine: continue execution
end
sequenceDiagram
participant SingleMacroEngine
participant ChoiceExecutor
participant SubEngine
participant NestedChoice
SingleMacroEngine->>ChoiceExecutor: consumeAbortSignal() %% ensureNotAborted
ChoiceExecutor-->>SingleMacroEngine: null
SingleMacroEngine->>SubEngine: run()
SubEngine->>NestedChoice: execute choice
alt Nested choice cancels
NestedChoice-->>SubEngine: MacroAbortError
SubEngine->>ChoiceExecutor: signalAbort(MacroAbortError)
ChoiceExecutor->>ChoiceExecutor: store pendingAbort
else success
NestedChoice-->>SubEngine: Result
end
SubEngine-->>SingleMacroEngine: return
SingleMacroEngine->>ChoiceExecutor: consumeAbortSignal()
ChoiceExecutor-->>SingleMacroEngine: MacroAbortError or null
alt MacroAbortError
SingleMacroEngine->>SingleMacroEngine: throw (stop further commands)
else
SingleMacroEngine->>SingleMacroEngine: proceed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/engine/MacroChoiceEngine.entry.test.ts (1)
247-277: LGTM! Test correctly verifies cancellation wrapping for executeNestedChoice.The test properly validates the nested choice cancellation path. Minor observation: Line 267 uses
"no input given."(lowercase) while test 1 uses"No input given."(uppercase). Consider using consistent error strings across tests for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/engine/MacroChoiceEngine.entry.test.ts(2 hunks)src/engine/MacroChoiceEngine.ts(2 hunks)
🔇 Additional comments (5)
src/engine/MacroChoiceEngine.entry.test.ts (3)
10-13: LGTM! Imports are appropriate.The new imports support the cancellation test suite and are all utilized in the tests below.
184-212: LGTM! Test setup is well-structured.The
beforeEachhook properly initializes all required mocks and fixtures for the cancellation tests.
214-245: LGTM! Test correctly verifies cancellation wrapping for executeChoice.The test appropriately:
- Mocks the cancellation scenario by rejecting with a cancellation error
- Asserts the error is wrapped in
MacroAbortError- Verifies the executor is called with the correct choice
src/engine/MacroChoiceEngine.ts (2)
352-362: LGTM! Cancellation handling is correctly implemented.The try-catch block properly converts cancellation errors to
MacroAbortErrorwhile preserving existingMacroAbortErrorinstances and re-throwing other errors. This pattern is consistent with the error handling inonExportIsObject(lines 326-334) andexecuteAIAssistant(lines 422-427).
372-382: LGTM! Cancellation handling mirrors executeChoice.The error handling logic is identical to
executeChoice(lines 352-362), ensuring consistent cancellation behavior across both choice execution paths. This addresses the issue where Escape wasn't aborting macros.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/MacroChoiceEngine.entry.test.ts (1)
93-103: Fix mock typing forQuickAddApi.GetApi.
mockGetApiis currently typed as() => ({}), so the assignment back ontoQuickAddApi.GetApitriggers TS2352 and breaks the lint pipeline. Please type the mock with the real signature (and re-export it without unsafe casts) so tsc accepts the override.Apply this diff to align the mock with the production signature:
+type QuickAddGetApi = (typeof import("../quickAddApi"))["QuickAddApi"]["GetApi"]; + const { mockGetUserScript, mockInitializeUserScriptSettings, mockSuggest, mockGetApi, mockInputPrompt } = vi.hoisted(() => ({ mockGetUserScript: vi.fn(), mockInitializeUserScriptSettings: vi.fn(), mockSuggest: vi.fn(), - mockGetApi: vi.fn(() => ({})), + mockGetApi: vi.fn<QuickAddGetApi>(), mockInputPrompt: vi.fn(), })); @@ - actual.QuickAddApi.GetApi = mockGetApi as typeof actual.QuickAddApi.GetApi; - return actual; + return { + ...actual, + QuickAddApi: { + ...actual.QuickAddApi, + GetApi: mockGetApi, + }, + };src/engine/TemplateChoiceEngine.notice.test.ts (1)
43-60: Mock signature mismatch causes spread error.
formatFileContentMockis declared as a zero-arg mock, but you forward...args, which is why tsc reports “A spread argument must either have a tuple type”. Update the mock type to accept variadic args so the spread is legal and the lint job can pass.Apply this diff:
-const { formatFileNameMock, formatFileContentMock } = vi.hoisted(() => { - return { - formatFileNameMock: vi.fn<(format: string, prompt: string) => Promise<string>>(), - formatFileContentMock: vi.fn<() => Promise<string>>().mockResolvedValue(""), - }; -}); +const { formatFileNameMock, formatFileContentMock } = vi.hoisted(() => ({ + formatFileNameMock: vi.fn<(format: string, prompt: string) => Promise<string>>(), + formatFileContentMock: vi + .fn<(...args: unknown[]) => Promise<string>>() + .mockResolvedValue(""), +}));
🧹 Nitpick comments (1)
src/choiceExecutor.ts (1)
20-32: Minor: Redundant null coalescing in consumeAbortSignal.Line 31 uses
return abort ?? null, butabortis already typed asMacroAbortError | null, making the?? nullunnecessary. The implementation is functionally correct, but simplifying toreturn abort;would be cleaner.Apply this diff to simplify:
consumeAbortSignal(): MacroAbortError | null { const abort = this.pendingAbort; this.pendingAbort = null; - return abort ?? null; + return abort; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
EXECPLAN.md(1 hunks)src/IChoiceExecutor.ts(1 hunks)src/choiceExecutor.ts(1 hunks)src/engine/CaptureChoiceEngine.ts(1 hunks)src/engine/MacroChoiceEngine.entry.test.ts(4 hunks)src/engine/MacroChoiceEngine.notice.test.ts(2 hunks)src/engine/MacroChoiceEngine.ts(3 hunks)src/engine/SingleMacroEngine.ts(2 hunks)src/engine/TemplateChoiceEngine.notice.test.ts(9 hunks)src/engine/TemplateChoiceEngine.ts(3 hunks)src/engine/TemplateEngine.ts(4 hunks)src/quickAddApi.ts(7 hunks)vitest.config.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/MacroChoiceEngine.ts
🧰 Additional context used
🧬 Code graph analysis (10)
src/engine/TemplateChoiceEngine.ts (2)
src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/TemplateEngine.ts (1)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/quickAddApi.ts (4)
src/preflight/OnePageInputModal.ts (1)
OnePageInputModal(19-337)src/gui/GenericYesNoPrompt/GenericYesNoPrompt.ts (1)
GenericYesNoPrompt(4-72)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/utils/errorUtils.ts (1)
isCancellationError(69-80)
src/IChoiceExecutor.ts (2)
src/types/choices/IChoice.ts (1)
IChoice(3-10)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/SingleMacroEngine.ts (1)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/CaptureChoiceEngine.ts (1)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/TemplateChoiceEngine.notice.test.ts (2)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/settingsStore.ts (1)
settingsStore(7-20)
src/engine/MacroChoiceEngine.notice.test.ts (6)
src/types/choices/IChoice.ts (1)
IChoice(3-10)src/types/macros/IMacro.ts (1)
IMacro(3-7)src/types/choices/IMacroChoice.ts (1)
IMacroChoice(4-7)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/MacroChoiceEngine.ts (1)
MacroChoiceEngine(56-610)
src/choiceExecutor.ts (1)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/MacroChoiceEngine.entry.test.ts (5)
src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/types/choices/IChoice.ts (1)
IChoice(3-10)src/types/macros/IChoiceCommand.ts (1)
IChoiceCommand(3-5)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/types/macros/QuickCommands/INestedChoiceCommand.ts (1)
INestedChoiceCommand(4-6)
🪛 GitHub Actions: Build With Lint
src/engine/MacroChoiceEngine.entry.test.ts
[error] 97-97: TS2352: Conversion of type 'Mock<() => {}>' to type '(app: App, plugin: QuickAdd, choiceExecutor: IChoiceExecutor) => { requestInputs: (inputs: { id: string; label?: string | undefined; type: "textarea" | ... 4 more ... | "suggester"; ... 5 more ...; suggesterConfig?: { ...; } | undefined; }[]) => Promise<...>; ... 11 more ...; fieldSuggestions: { ...; }; }' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. (Command: tsc -noEmit -skipLibCheck && bun lint && node esbuild.config.mjs production)
🪛 GitHub Actions: Test
src/engine/MacroChoiceEngine.entry.test.ts
[error] 338-338: Test failed: promise resolved "undefined" instead of rejecting. Expected MacroAbortError to be thrown when input prompt is cancelled.
🪛 GitHub Check: Build With Lint
src/engine/TemplateChoiceEngine.notice.test.ts
[failure] 59-59:
A spread argument must either have a tuple type or be passed to a rest parameter.
src/engine/MacroChoiceEngine.entry.test.ts
[failure] 97-97:
Conversion of type 'Mock<() => {}>' to type '(app: App, plugin: QuickAdd, choiceExecutor: IChoiceExecutor) => { requestInputs: (inputs: { id: string; label?: string | undefined; type: "textarea" | ... 4 more ... | "suggester"; ... 5 more ...; suggesterConfig?: { ...; } | undefined; }[]) => Promise<...>; ... 11 more ...; fieldSuggestions: { ...; }; }' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
🪛 GitHub Check: Test (20)
src/engine/MacroChoiceEngine.entry.test.ts
[failure] 338-338: src/engine/MacroChoiceEngine.entry.test.ts > QuickAddApi prompt cancellation > throws MacroAbortError when input prompt is cancelled
AssertionError: promise resolved "undefined" instead of rejecting
- Expected:
Error {
"message": "rejected promise",
}
- Received:
undefined
❯ src/engine/MacroChoiceEngine.entry.test.ts:338:3
🔇 Additional comments (11)
vitest.config.mts (1)
24-28: Configuration correctly uses modern Vitest optimizer approach.The change from
deps.inlinetodeps.optimizer.web.includeis correct and aligns with current Vitest recommendations. The optimizer-based approach usingdeps.optimizer.web.includeis the recommended path for web transform mode, offering better test performance and browser-like behavior by bundling packages with esbuild, while the legacydeps.inlineapproach may emit deprecation warnings in newer Vitest versions.src/IChoiceExecutor.ts (1)
7-17: LGTM: Well-designed abort signaling API.The optional methods maintain backward compatibility while providing a clean synchronization mechanism for nested choice cancellations. The JSDoc clearly documents when engines should call each method.
src/quickAddApi.ts (2)
91-104: LGTM: One-page input cancellation properly handled.The try-catch correctly wraps the modal invocation and uses the new
throwIfPromptCancelledhelper to convert cancellations intoMacroAbortError, ensuring macros abort on user cancellation.
612-619: LGTM: Consistent cancellation conversion.The helper correctly distinguishes between
MacroAbortError(rethrow as-is), cancellation errors (convert toMacroAbortError), and other errors (fall through to return undefined). This ensures macros abort on user cancellation while maintaining backward compatibility for non-cancellation errors.EXECPLAN.md (1)
1-126: Comprehensive execution plan.The ExecPlan thoroughly documents the implementation strategy, progress, decisions, and interfaces. This provides excellent context for understanding the abort signaling mechanism across the codebase.
src/engine/SingleMacroEngine.ts (1)
243-243: LGTM: Proper abort signal propagation.The
signalAbortcall is correctly placed afterhandleMacroAbortconfirms the error is aMacroAbortError, allowing orchestrators to detect that the export execution was aborted.src/engine/CaptureChoiceEngine.ts (1)
187-187: LGTM: Consistent abort propagation.The abort signal is correctly propagated after
handleMacroAbortconfirms the error, maintaining consistency with other engines in the codebase.src/engine/TemplateEngine.ts (2)
20-28: LGTM: Defensive type guard for MacroAbortError.The helper uses both
instanceofand duck typing (checking thenameproperty) to reliably identifyMacroAbortErrorinstances, even across module boundaries. This is a robust approach for error identification.
183-185: LGTM: MacroAbortError propagation in template operations.All three template operations (
createFileWithTemplate,overwriteFileWithTemplate,appendToFileWithTemplate) now correctly rethrowMacroAbortErrorbefore falling through to error reporting, ensuring cancellations propagate up the call stack.src/choiceExecutor.ts (1)
35-35: LGTM: Proper abort signal lifecycle management.Resetting
pendingAbortat the start of eachexecute()call ensures a clean slate for each choice execution, preventing stale abort signals from affecting subsequent operations.src/engine/MacroChoiceEngine.notice.test.ts (1)
174-254: LGTM: Comprehensive nested choice cancellation test.The test properly validates the abort signaling flow:
- Simulates a nested choice that cancels (line 223)
- Verifies
signalAbortis called with aMacroAbortError(lines 249-250)- Verifies
consumeAbortSignalis invoked exactly once (line 251)- Confirms subsequent commands are skipped (line 252:
obsidianExecutions === 0)This effectively demonstrates that macros halt when nested choices cancel.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/quickAddApi.executeChoice.test.ts (3)
27-32: Consider moving choice to beforeEach for test flexibility.The
choiceobject is currently defined at the module level. If future tests need different choice configurations, moving it tobeforeEachwould provide more flexibility without affecting current tests.
48-58: Consider passing variables through the parameter and verify execute() was called.The test correctly validates abort propagation and cleanup, but could be more realistic:
- Line 53 manually sets a variable on the map. Consider passing variables via the
executeChoiceparameter instead to better match actual usage patterns.- Add an assertion to verify that
choiceExecutor.executewas called to ensure the full execution flow occurred before the abort.Example improvement:
it("propagates aborts from executed choices", async () => { const abortError = new MacroAbortError("Input cancelled by user"); (choiceExecutor.consumeAbortSignal as ReturnType<typeof vi.fn>).mockReturnValueOnce(abortError); const api = QuickAddApi.GetApi(app, plugin, choiceExecutor); - variables.set("foo", "bar"); - await expect(api.executeChoice("My Template")) + await expect(api.executeChoice("My Template", { foo: "bar" })) .rejects.toBe(abortError); + expect(choiceExecutor.execute).toHaveBeenCalledTimes(1); expect(choiceExecutor.consumeAbortSignal).toHaveBeenCalledTimes(1); expect(variables.size).toBe(0); });
60-67: Add assertion to verify execute() was called.The test correctly validates the success path and variable cleanup. Consider adding an assertion to verify that
choiceExecutor.executewas called to ensure the full execution flow is tested.Apply this diff:
it("clears variables and resolves when no abort is signalled", async () => { const api = QuickAddApi.GetApi(app, plugin, choiceExecutor); await expect( api.executeChoice("My Template", { project: "QA" }), ).resolves.toBeUndefined(); + expect(choiceExecutor.execute).toHaveBeenCalledTimes(1); expect(choiceExecutor.consumeAbortSignal).toHaveBeenCalledTimes(1); expect(variables.size).toBe(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/quickAddApi.executeChoice.test.ts(1 hunks)src/quickAddApi.ts(8 hunks)tests/obsidian-stub.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/quickAddApi.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/quickAddApi.executeChoice.test.ts (5)
tests/obsidian-stub.ts (1)
App(23-51)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/types/choices/IChoice.ts (1)
IChoice(3-10)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/quickAddApi.ts (1)
QuickAddApi(32-614)
🪛 GitHub Check: Build With Lint
tests/obsidian-stub.ts
[failure] 54-54:
'App' refers to a value, but is being used as a type here. Did you mean 'typeof App'?
🔇 Additional comments (5)
tests/obsidian-stub.ts (2)
53-70: Plugin stub implementation looks good, pending type fix.The Plugin class provides appropriate stub implementations for testing:
- Constructor properly initializes
appandmanifest- Stub methods return sensible values (objects with expected shapes or no-ops)
- Consistent with other stubs in this file
Note: This approval assumes the type annotation issue on line 54 will be fixed as noted in the previous comment.
218-218: LGTM!The addition of
Pluginto the default export is correct and consistent with the existing export structure.src/quickAddApi.executeChoice.test.ts (3)
1-7: LGTM!All necessary imports are present and correctly structured for the test scenarios.
9-20: LGTM!The dependency mocks appropriately isolate the test from external modules.
34-46: LGTM!The
beforeEachhook properly initializes clean mocks for each test, with sensible defaults that represent successful execution.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/TemplateChoiceEngine.notice.test.ts (1)
43-68: Fix the hoisting issue causing the pipeline failure.The pipeline failure indicates that
formatFileNameMockcannot be accessed before initialization. This occurs becausevi.mockis hoisted by Vitest and runs before top-level variable declarations. Variables used inside mock factories must be defined withvi.hoisted().Apply this diff to wrap the mock variable declarations:
-const formatFileNameMock = vi.fn<(format: string, prompt: string) => Promise<string>>(); -const formatFileContentMock = vi - .fn<(...args: unknown[]) => Promise<string>>() - .mockResolvedValue(""); +const { formatFileNameMock, formatFileContentMock } = vi.hoisted(() => { + return { + formatFileNameMock: vi.fn<(format: string, prompt: string) => Promise<string>>(), + formatFileContentMock: vi + .fn<(...args: unknown[]) => Promise<string>>() + .mockResolvedValue(""), + }; +});
♻️ Duplicate comments (1)
src/engine/MacroChoiceEngine.entry.test.ts (1)
328-334: Test fails—implementation must convert prompt cancellation to MacroAbortError.The test correctly expects
QuickAddApi.inputPromptto throwMacroAbortErrorwhen the prompt is cancelled, but the pipeline shows it currently resolvesundefined. The implementation inquickAddApi.tsneedsthrowIfPromptCancelledto recognize"No input given."as a cancellation sentinel and throwMacroAbortError.Verify the implementation with:
#!/bin/bash # Check throwIfPromptCancelled implementation ast-grep --pattern $'function throwIfPromptCancelled($$$) { $$$ }' # Also check if it handles "No input given." sentinel rg -nP --type=ts 'No input given\.' -C3
🧹 Nitpick comments (4)
src/engine/TemplateChoiceEngine.notice.test.ts (2)
137-191: Well-structured test helper with flexible options.The updated
createEnginefunction provides good flexibility for testing different scenarios. The addition ofsignalAbortandconsumeAbortSignalto the mock choiceExecutor aligns with the new abort propagation API, and returning both the engine and executor enables verification of the abort signaling behavior.The type casting at lines 175-179 works but is verbose. Consider adding a test utility type if this pattern repeats:
type TemplateEngineWithStubs = TemplateChoiceEngine & { getTemplateContent: () => Promise<string>; };Then use:
(engine as TemplateEngineWithStubs).getTemplateContent = vi.fn()...
256-268: Good coverage for content formatting abort path.This test effectively verifies that aborts during template content formatting are also signaled to the choice executor, complementing the file name formatting test.
For consistency with the previous test, consider also verifying the error type:
expect(choiceExecutor.signalAbort).toHaveBeenCalledTimes(1); const [[error]] = (choiceExecutor.signalAbort as ReturnType<typeof vi.fn>).mock.calls; expect(error).toBeInstanceOf(MacroAbortError);src/engine/MacroChoiceEngine.entry.test.ts (2)
42-75: Consider adding a comment to document the GUI mock block.The extensive GUI component mocks are necessary for isolating the
QuickAddApitests, but a brief comment explaining their purpose would improve maintainability.
247-248: Remove unused spy.The
getApiSpyis mocked but never asserted in the test. This code can be removed.Apply this diff:
- const getApiSpy = vi.spyOn(QuickAddApi, "GetApi"); - getApiSpy.mockReturnValueOnce({} as any); - const engine = new MacroChoiceEngine(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/engine/MacroChoiceEngine.entry.test.ts(3 hunks)src/engine/SingleMacroEngine.ts(2 hunks)src/engine/TemplateChoiceEngine.notice.test.ts(9 hunks)tests/obsidian-stub.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/SingleMacroEngine.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/engine/MacroChoiceEngine.entry.test.ts (3)
src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/MacroChoiceEngine.ts (1)
MacroChoiceEngine(56-610)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/TemplateChoiceEngine.notice.test.ts (2)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/settingsStore.ts (1)
settingsStore(7-20)
🪛 GitHub Actions: Test
src/engine/MacroChoiceEngine.entry.test.ts
[error] 333-333: AssertionError: promise resolved "undefined" instead of rejecting. Expected QuickAddApi prompt cancellation to reject with MacroAbortError.
src/engine/TemplateChoiceEngine.notice.test.ts
[error] 1-1: Vitest mocking error: There was an error when mocking a module. Possible top-level variable in vi.mock factory causing hoisting issue (Cannot access 'formatFileNameMock' before initialization).
🪛 GitHub Check: Test (20)
src/engine/MacroChoiceEngine.entry.test.ts
[failure] 333-333: src/engine/MacroChoiceEngine.entry.test.ts > QuickAddApi prompt cancellation > throws MacroAbortError when input prompt is cancelled
AssertionError: promise resolved "undefined" instead of rejecting
- Expected:
Error {
"message": "rejected promise",
}
- Received:
undefined
❯ src/engine/MacroChoiceEngine.entry.test.ts:333:3
🔇 Additional comments (10)
src/engine/TemplateChoiceEngine.notice.test.ts (3)
246-254: Excellent test coverage for abort signaling.This test correctly verifies that MacroAbortError is propagated to the choice executor via
signalAbort. The verification approach is thorough—checking both call count and error type.
198-199: Proper mock initialization in beforeEach.The addition of
formatFileContentMockreset and default value ensures clean test state between runs, consistent with howformatFileNameMockis managed.
202-244: Existing tests properly updated for new createEngine API.All existing tests have been correctly updated to destructure
enginefrom the newcreateEnginereturn value, maintaining test functionality while adapting to the API change.tests/obsidian-stub.ts (4)
23-51: TypeScript error resolved correctly.The conversion from class expression to proper class declaration fixes the previously flagged TypeScript error. The
Appidentifier now provides both a type and a value, enabling correct type annotations throughout the codebase. This matches the preferred solution from the previous review.
53-70: Plugin stub implementation looks correct.The Plugin class correctly uses
app: Appas a type annotation (now valid after the App class refactoring). The constructor and stub methods provide appropriate minimal implementations for testing, matching the Obsidian Plugin API surface.
72-181: Class declaration conversions applied consistently.All class expressions have been successfully converted to proper class declarations (TFile, TFolder, MarkdownView, FileView, WorkspaceLeaf, FuzzySuggestModal). The FuzzySuggestModal correctly includes a generic parameter, and all conversions follow the same pattern established by the App class refactoring.
218-218: Default export correctly includes Plugin.The Plugin class has been appropriately added to the default export, maintaining consistency with other exported stub classes.
src/engine/MacroChoiceEngine.entry.test.ts (3)
250-283: Well-structured abort propagation test.The test properly verifies that
executeChoiceconsumes the abort signal from the executor and propagatesMacroAbortErrorto halt macro execution. The mock setup and assertions align with the abort signaling contract defined inIChoiceExecutor.
286-318: Well-structured nested choice cancellation test.This test completes coverage of both choice execution paths (executeChoice + executeNestedChoice) as stated in the PR objectives. The abort propagation logic is verified consistently across both paths.
336-342: Good edge case coverage.This test ensures that non-cancellation prompt errors gracefully return
undefinedrather than halting macro execution. Once the cancellation detection is fixed, this distinction will be important for correct error handling.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/engine/SingleMacroEngine.member-access.test.ts(1 hunks)src/engine/SingleMacroEngine.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/SingleMacroEngine.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/engine/SingleMacroEngine.member-access.test.ts (6)
src/types/macros/ICommand.ts (1)
ICommand(3-7)src/types/macros/IUserScript.ts (1)
IUserScript(3-6)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/choiceExecutor.ts (1)
consumeAbortSignal(28-32)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/SingleMacroEngine.ts (1)
SingleMacroEngine(17-400)
🪛 GitHub Actions: Test
src/engine/SingleMacroEngine.member-access.test.ts
[error] 397-397: Test failure: propagates abort signals when export pre-commands cancel. Expected promise to reject but received a resolved value (''), indicating the promise did not reject as required.
🪛 GitHub Check: Test (20)
src/engine/SingleMacroEngine.member-access.test.ts
[failure] 397-397: src/engine/SingleMacroEngine.member-access.test.ts > SingleMacroEngine member access > propagates abort signals when export pre-commands cancel
AssertionError: promise resolved "''" instead of rejecting
- Expected:
Error {
"message": "rejected promise",
}
- Received:
""
❯ src/engine/SingleMacroEngine.member-access.test.ts:397:55
🔇 Additional comments (1)
src/engine/SingleMacroEngine.member-access.test.ts (1)
405-442: No actionable changes needed—test logic is sound.After examining the implementation, the test should pass. When
runAndGetOutput("My Macro")is called with no member access, it skipstryExecuteExportentirely and goes directly to line 115-116:
await engine.run()executes and resolvesthis.ensureNotAborted()immediately callsconsumeAbortSignal(), which returnsabortErroron the first invocationensureNotAborted()throws the abort (line 397)- The promise rejects before
engine.getOutput()is reached (line 117)Unlike
tryExecuteExportwhich wraps its logic in a try-catch and swallows abort signals viahandleMacroAbort, the direct run path has no error handler—the abort propagates correctly. The test expectations are met:engine.runcalled once,engine.getOutputnever called, and the promise rejects withabortError.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/engine/SingleMacroEngine.ts (1)
241-254: Consider removing redundantsignalAbortcall when re-throwing.The throw at line 250 correctly fixes the abort propagation issue identified in the past review. However, the
signalAbortcall at line 249 appears redundant since:
- The error is thrown immediately after signaling
- Line 107's
ensureNotAborted()is never reached when an error propagates- The signal set at line 249 will not be consumed by any caller
- The abort information is already carried by the thrown error itself
Consider simplifying to just re-throw without signaling:
if ( handleMacroAbort(error, { logPrefix: "Macro execution aborted", noticePrefix: "Macro execution aborted", defaultReason: "Macro execution aborted", }) ) { - this.choiceExecutor.signalAbort?.(error as MacroAbortError); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/engine/SingleMacroEngine.member-access.test.ts(2 hunks)src/engine/SingleMacroEngine.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/engine/SingleMacroEngine.ts (1)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/SingleMacroEngine.member-access.test.ts (4)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/choiceExecutor.ts (1)
consumeAbortSignal(28-32)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/SingleMacroEngine.ts (1)
SingleMacroEngine(17-397)
🔇 Additional comments (6)
src/engine/SingleMacroEngine.ts (3)
391-396: LGTM: Clean abort signal consumer.The helper method correctly consumes and propagates abort signals, with optional chaining ensuring backward compatibility.
107-108: LGTM: Strategic abort checkpoint placement.The
ensureNotAborted()calls are correctly placed after async operations where user cancellations might occur (command executions, macro runs, export executions). This ensures prompt abort detection and propagation throughout the execution flow.Also applies to: 116-116, 162-162, 227-227, 234-234, 315-315
15-15: LGTM: Type import for abort error handling.Clean type-only import used correctly for type assertion in the catch block.
src/engine/SingleMacroEngine.member-access.test.ts (3)
320-359: LGTM: Test updated to verify abort propagation.The renamed test and updated expectation (rejection instead of empty string) correctly validate that aborts from exports now propagate as
MacroAbortErrorrather than being swallowed. This aligns with the PR objectives.
361-404: Test correctly validates pre-command abort propagation.This test verifies that cancellations during pre-command execution propagate correctly. With the implementation fix in
SingleMacroEngine.ts(line 250 now throws instead of returning), this test should pass.The test correctly:
- Mocks
consumeAbortSignalto simulate an abort after pre-commands- Expects rejection with the specific
MacroAbortErrorinstance- Verifies that execution stops early (no user script loading)
Note: This addresses the past review comment that identified the abort propagation issue in
tryExecuteExport.
406-443: LGTM: Comprehensive coverage of full macro abort path.This test complements the pre-command test by validating abort propagation in the simpler execution path (without member access). It correctly verifies that:
- The macro runs to completion before abort is detected
ensureNotAborted()at line 116 of the implementation catches the signal- The promise rejects with the abort error
- Execution stops before retrieving output
Together with the pre-command test, these provide strong coverage of abort propagation scenarios.
be36120 to
138f0d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/engine/SingleMacroEngine.member-access.test.ts (1)
406-443: LGTM: Well-structured test for full macro run abort propagation.This test properly verifies the abort propagation behavior for the non-member-access path (full macro run). The test correctly simulates an abort signal after the macro execution completes and confirms that the error propagates while halting further processing.
Optional: Consider adding a test for post-command abort propagation.
The current test suite covers abort scenarios during export execution, pre-commands, and full macro runs. For completeness, you might consider adding a test that verifies abort propagation when the signal is set during post-command execution (after the export function succeeds but before post-commands finish). However, given the symmetric handling of pre/post commands in the implementation, this is a nice-to-have rather than essential.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/docs/Advanced/onePageInputs.md(1 hunks)docs/docs/QuickAddAPI.md(5 hunks)docs/docs/UserScripts.md(2 hunks)src/IChoiceExecutor.ts(1 hunks)src/choiceExecutor.ts(1 hunks)src/engine/CaptureChoiceEngine.ts(1 hunks)src/engine/MacroChoiceEngine.entry.test.ts(3 hunks)src/engine/MacroChoiceEngine.notice.test.ts(2 hunks)src/engine/MacroChoiceEngine.ts(3 hunks)src/engine/SingleMacroEngine.member-access.test.ts(2 hunks)src/engine/SingleMacroEngine.ts(7 hunks)src/engine/TemplateChoiceEngine.notice.test.ts(9 hunks)src/engine/TemplateChoiceEngine.ts(3 hunks)src/engine/TemplateEngine.ts(4 hunks)src/quickAddApi.executeChoice.test.ts(1 hunks)src/quickAddApi.ts(8 hunks)tests/obsidian-stub.ts(3 hunks)vitest.config.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- vitest.config.mts
- src/engine/TemplateEngine.ts
- src/engine/SingleMacroEngine.ts
- src/engine/CaptureChoiceEngine.ts
- src/quickAddApi.executeChoice.test.ts
- src/engine/MacroChoiceEngine.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/quickAddApi.ts (4)
src/preflight/OnePageInputModal.ts (1)
OnePageInputModal(19-337)src/gui/GenericYesNoPrompt/GenericYesNoPrompt.ts (1)
GenericYesNoPrompt(4-72)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/utils/errorUtils.ts (1)
isCancellationError(69-80)
src/choiceExecutor.ts (3)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/main.ts (1)
QuickAdd(34-302)src/types/choices/IChoice.ts (1)
IChoice(3-10)
src/engine/MacroChoiceEngine.entry.test.ts (4)
src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/quickAddApi.ts (1)
QuickAddApi(32-614)src/engine/MacroChoiceEngine.ts (1)
MacroChoiceEngine(56-610)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/MacroChoiceEngine.notice.test.ts (7)
src/types/choices/IChoice.ts (1)
IChoice(3-10)src/types/macros/IMacro.ts (1)
IMacro(3-7)src/types/choices/IMacroChoice.ts (1)
IMacroChoice(4-7)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/choiceExecutor.ts (2)
signalAbort(24-26)consumeAbortSignal(28-32)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/MacroChoiceEngine.ts (1)
MacroChoiceEngine(56-610)
src/engine/TemplateChoiceEngine.ts (2)
src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
src/engine/SingleMacroEngine.member-access.test.ts (4)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/choiceExecutor.ts (1)
consumeAbortSignal(28-32)src/IChoiceExecutor.ts (1)
IChoiceExecutor(4-18)src/engine/SingleMacroEngine.ts (1)
SingleMacroEngine(17-397)
src/engine/TemplateChoiceEngine.notice.test.ts (2)
src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)src/settingsStore.ts (1)
settingsStore(7-20)
src/IChoiceExecutor.ts (2)
src/types/choices/IChoice.ts (1)
IChoice(3-10)src/errors/MacroAbortError.ts (1)
MacroAbortError(1-6)
🪛 LanguageTool
docs/docs/UserScripts.md
[style] ~320-~320: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d only call abort() yourself when you want to provide a custom message or stop execut...
(REP_WANT_TO_VB)
🔇 Additional comments (16)
src/engine/SingleMacroEngine.member-access.test.ts (2)
320-359: LGTM: Test correctly verifies export abort propagation.The test has been updated to expect strict abort propagation instead of graceful handling. This aligns with the PR objective to stop macro execution immediately when cancellations occur. The assertions properly verify that the abort propagates and execution halts.
361-404: LGTM: Comprehensive test for pre-command abort signal propagation.This test properly verifies that when a pre-command triggers an abort signal (via
consumeAbortSignal), the execution halts immediately and theMacroAbortErrorpropagates correctly. The mock setup accurately simulates the abort signal consumption pattern, and the assertions confirm that execution stops before loading the user script.src/choiceExecutor.ts (1)
20-32: Clean abort signaling implementation.The state management pattern is straightforward: store on signal, consume and clear on retrieval, reset on new execution. This prevents stale abort signals from affecting subsequent executions.
docs/docs/Advanced/onePageInputs.md (1)
187-187: Clear cancellation semantics documented.The documentation accurately describes the new behavior where closing the modal without submitting triggers
MacroAbortError, providing users with clear expectations.src/IChoiceExecutor.ts (1)
2-17: Well-designed backward-compatible interface extension.The optional method signatures with comprehensive JSDoc comments provide clear guidance for implementers while maintaining backward compatibility with existing code.
src/engine/TemplateChoiceEngine.ts (2)
32-43: Proper integration of choice executor.The choiceExecutor is correctly stored as an instance member and passed to the parent class, enabling abort signaling in error paths.
191-191: Correct abort signaling in error path.The abort signal is properly propagated when
handleMacroAbortconfirms a cancellation, following the documented pattern from the interface.src/engine/MacroChoiceEngine.entry.test.ts (3)
16-76: Comprehensive mock setup for cancellation testing.The mocks properly cover all GUI components needed to test the cancellation flows, including the newly added
mockInputPromptfor testing input cancellation scenarios.
214-319: Thorough cancellation propagation tests.The tests properly verify that cancellation errors from nested choices are wrapped in
MacroAbortErrorand propagated through the executor's abort signaling mechanism.
328-343: Implementation should now satisfy test requirements.The
throwIfPromptCancelledhelper inquickAddApi.ts(lines 616-623) now properly converts cancellation errors (including"No input given.") intoMacroAbortError, which should resolve the test failure noted in the past review. The test correctly distinguishes between cancellation (throwsMacroAbortError) and other errors (returnsundefined).src/engine/MacroChoiceEngine.notice.test.ts (1)
174-254: Comprehensive nested choice cancellation test.The test thoroughly verifies that when a nested choice cancels, subsequent macro commands are properly halted. The manual mock of the abort signaling mechanism accurately reflects the real implementation's behavior, and the assertions comprehensively verify the expected flow.
docs/docs/QuickAddAPI.md (1)
41-41: Excellent documentation of cancellation semantics.The documentation clearly and consistently describes the new
MacroAbortErrorbehavior across all prompt methods, with practical examples showing optional error handling. The try/catch pattern example is particularly helpful for users who want to handle cancellations gracefully.Also applies to: 119-139, 146-146, 164-164, 207-207
src/quickAddApi.ts (4)
91-104: Proper cancellation handling in requestInputs.The try/catch correctly routes cancellation errors through
throwIfPromptCancelled, ensuring consistentMacroAbortErrorsemantics while preserving other error types.
167-171: Correct abort signal consumption pattern.The code properly consumes the abort signal after execution and rethrows if present, ensuring that cancellations in nested choices propagate up the call stack as expected.
507-613: Consistent cancellation handling across all prompt methods.All prompt methods follow the same pattern: cancellations are converted to
MacroAbortErrorand thrown, while other errors result inundefinedbeing returned. This provides uniform behavior across the API surface.
616-623: Well-designed cancellation conversion helper.The
throwIfPromptCancelledhelper elegantly centralizes the conversion logic, preserving existingMacroAbortErrorinstances while wrapping raw cancellation errors. This ensures consistent error handling across all prompt methods.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/UserScripts.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/UserScripts.md
[style] ~322-~322: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d only call abort() yourself when you want to provide a custom message or stop execut...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/UserScripts.md
312-312: Hard tabs
Column: 1
(MD010, no-hard-tabs)
314-314: Hard tabs
Column: 1
(MD010, no-hard-tabs)
315-315: Hard tabs
Column: 1
(MD010, no-hard-tabs)
316-316: Hard tabs
Column: 1
(MD010, no-hard-tabs)
317-317: Hard tabs
Column: 1
(MD010, no-hard-tabs)
318-318: Hard tabs
Column: 1
(MD010, no-hard-tabs)
326-326: Hard tabs
Column: 1
(MD010, no-hard-tabs)
327-327: Hard tabs
Column: 1
(MD010, no-hard-tabs)
328-328: Hard tabs
Column: 1
(MD010, no-hard-tabs)
329-329: Hard tabs
Column: 1
(MD010, no-hard-tabs)
330-330: Hard tabs
Column: 1
(MD010, no-hard-tabs)
331-331: Hard tabs
Column: 1
(MD010, no-hard-tabs)
332-332: Hard tabs
Column: 1
(MD010, no-hard-tabs)
333-333: Hard tabs
Column: 1
(MD010, no-hard-tabs)
334-334: Hard tabs
Column: 1
(MD010, no-hard-tabs)
335-335: Hard tabs
Column: 1
(MD010, no-hard-tabs)
336-336: Hard tabs
Column: 1
(MD010, no-hard-tabs)
337-337: Hard tabs
Column: 1
(MD010, no-hard-tabs)
338-338: Hard tabs
Column: 1
(MD010, no-hard-tabs)
339-339: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (3)
docs/docs/UserScripts.md (3)
287-293: Past review concern resolved: abort() guidance now reflects MacroAbortError behavior.The documentation has been appropriately updated to remove the conflicting bullet about user prompt cancellations and now explicitly clarifies that "Prompt cancellations already throw
MacroAbortErrorand halt macros automatically." This directly addresses the prior concern about misleading script authors. Users are now correctly directed to callabort()only when providing a custom message or handling non-prompt scenarios.
324-341: Example code pattern is clear and well-demonstrates abort handling.The example correctly shows the recommended pattern: wrap prompt calls in try/catch, check for MacroAbortError, optionally abort() with a custom message, and re-throw other errors. This provides script authors with a practical reference for handling cancellations.
301-323: MacroAbortError documentation is comprehensive and well-integrated.The new section clearly documents which API methods throw on cancellation, explains the behavior change from undefined returns to MacroAbortError, and provides practical try/catch patterns. The progression from listing cancellable methods → showing error handling → emphasizing the Important point is logical and user-friendly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/docs/UserScripts.md (1)
322-322: Minor: Vary phrasing to reduce repetition.Line 322 reuses "want to provide" from line 290 in the same section. Consider a slight rewording for variety, e.g.:
-**Important:** Because cancellations now throw, you should only call `abort()` yourself when you want to provide a custom message or stop execution for a non-prompt reason. +**Important:** Because cancellations now throw, call `abort()` only when you need to surface a custom message or stop for a non-prompt reason.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/UserScripts.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/UserScripts.md
[style] ~322-~322: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d only call abort() yourself when you want to provide a custom message or stop execut...
(REP_WANT_TO_VB)
🔇 Additional comments (1)
docs/docs/UserScripts.md (1)
287-322: Excellent update—abort guidance conflict is resolved.The previous review flagged a conflict between old guidance (telling users to call
abort()on prompt cancellation) and the new auto-cancellation behavior. This revision clearly resolves it:
- Old problematic bullet removed
- Lines 293, 308, and 322 consistently state that cancellations throw
MacroAbortErrorautomatically- New bullet at line 290 and examples show proper
try/catchpattern- Clear distinction: cancellations throw (no action needed);
abort()is for custom messages or non-prompt reasonsThe documentation now accurately reflects the PR's design and will guide users correctly.
# [2.7.0](2.6.0...2.7.0) (2025-11-08) ### Bug Fixes * format capture tags as YAML arrays ([#974](#974)) ([bc61760](bc61760)) * remove script suffix in request inputs ([#979](#979)) ([60ec853](60ec853)) * stop macros and APIs immediately when cancellations occur ([#976](#976)) ([48ea32d](48ea32d)) ### Features * add git info display to settings for development mode ([#973](#973)) ([e3b2a8b](e3b2a8b))
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests