codex: address PR review feedback (#1155)

Author: chhoumann

src/engine/SingleMacroEngine.member-access.test.ts

@@ -250,7 +250,7 @@ describe("SingleMacroEngine member access", () => {
 		expect(engineInstance.runSubset).toHaveBeenNthCalledWith(2, [postCommand]);
 		expect(engineInstance.setOutput).toHaveBeenCalledWith("export-result");
 		expect(result).toBe("export-result");
-		expect(mockGetUserScript).toHaveBeenCalledTimes(2);
+		expect(mockGetUserScript).toHaveBeenCalledTimes(3);
 		expect(mockGetUserScript).toHaveBeenCalledWith(firstScript, app);
 		expect(mockGetUserScript).toHaveBeenCalledWith(secondScript, app);
 	});
@@ -328,6 +328,52 @@ describe("SingleMacroEngine member access", () => {
 		expect(mockGetUserScript).toHaveBeenCalledWith(scriptB, app);
 	});
 
+	it("loads a selector-targeted script after pre-commands run", async () => {
+		const preCommand = {
+			id: "wait-1",
+			name: "Wait",
+			type: CommandType.Wait,
+		} as ICommand;
+		const scriptA = createUserScript("script-a", "a.js", {
+			name: "Alpha Script",
+		});
+		const scriptB = createUserScript("script-b", "b.js", {
+			name: "Beta Script",
+		});
+
+		let ready = false;
+		const engineInstance = macroEngineFactory();
+		engineInstance.runSubset = vi.fn().mockImplementation(async () => {
+			ready = true;
+		});
+		macroEngineFactory = () => engineInstance;
+
+		mockGetUserScript.mockImplementation(async (command: IUserScript) => {
+			if (command.id !== "script-b") {
+				return { alpha: vi.fn() };
+			}
+
+			return ready
+				? { beta: () => "late-bound-result" }
+				: { alpha: vi.fn() };
+		});
+
+		const engine = new SingleMacroEngine(
+			app,
+			plugin,
+			[baseMacroChoice([preCommand, scriptA, scriptB])],
+			choiceExecutor,
+		);
+
+		const result = await engine.runAndGetOutput(
+			"My Macro::Beta Script::beta",
+		);
+
+		expect(result).toBe("late-bound-result");
+		expect(mockGetUserScript).toHaveBeenCalledTimes(1);
+		expect(mockGetUserScript).toHaveBeenCalledWith(scriptB, app);
+	});
+
 	it("aborts when a selector matches duplicate script names", async () => {
 		const scriptA = createUserScript("script-a", "a.js", {
 			name: "Shared Script",
@@ -383,7 +429,7 @@ describe("SingleMacroEngine member access", () => {
 		const result = await engine.runAndGetOutput("My Macro::NotAScript::beta");
 
 		expect(result).toBe("nested-result");
-		expect(mockGetUserScript).toHaveBeenCalledTimes(2);
+		expect(mockGetUserScript).toHaveBeenCalledTimes(3);
 	});
 
 	it("aborts when a selected script does not export the requested member", async () => {
@@ -411,7 +457,7 @@ describe("SingleMacroEngine member access", () => {
 
 		await expect(
 			engine.runAndGetOutput("My Macro::Alpha Script::beta"),
-		).rejects.toThrow("targeted script 'Alpha Script'");
+		).rejects.toThrow("routes member access to 'Alpha Script'");
 	});
 
 	it("propagates aborts when the export aborts", async () => {

src/engine/SingleMacroEngine.ts

@@ -16,8 +16,6 @@ import { MacroAbortError } from "../errors/MacroAbortError";
 type UserScriptCandidate = {
 	command: IUserScript;
 	index: number;
-	exportsRef?: unknown;
-	resolvedMember: { found: boolean; value?: unknown };
 };
 
 type MemberAccessSelection = {
@@ -98,16 +96,14 @@ export class SingleMacroEngine {
 			throw new Error(`macro '${macroName}' does not exist.`);
 		}
 
-		const preloadedScripts = new Map<string, unknown>();
-
 		// Create a dedicated engine for this macro
 		const engine = new MacroChoiceEngine(
 			this.app,
 			this.plugin,
 			macroChoice,
 			this.choiceExecutor,
 			this.variables,
-			preloadedScripts,
+			undefined,
 			context?.label,
 		);
 
@@ -116,7 +112,6 @@ export class SingleMacroEngine {
 				engine,
 				macroChoice,
 				memberAccess,
-				preloadedScripts,
 			);
 
 			this.ensureNotAborted();
@@ -154,7 +149,6 @@ export class SingleMacroEngine {
 		engine: MacroChoiceEngine,
 		macroChoice: IMacroChoice,
 		memberAccess: string[],
-		preloadedScripts: Map<string, unknown>,
 	): Promise<{ executed: boolean; result?: unknown }> {
 		const originalCommands = macroChoice.macro?.commands;
 		if (!originalCommands?.length) {
@@ -176,7 +170,6 @@ export class SingleMacroEngine {
 			macroChoice,
 			userScriptCommands,
 			memberAccess,
-			preloadedScripts,
 		);
 		const preCommands = originalCommands.slice(0, selection.candidate.index);
 
@@ -199,22 +192,14 @@ export class SingleMacroEngine {
 				userScriptCommand.settings = {};
 			}
 
-			const exportsRef =
-				selection.candidate.exportsRef !== undefined
-					? selection.candidate.exportsRef
-					: await getUserScript(userScriptCommand, this.app);
+			const exportsRef = await getUserScript(userScriptCommand, this.app);
 
 			if (exportsRef === undefined || exportsRef === null) {
 				throw new MacroAbortError(
 					`Macro '${macroChoice.name}' could not load '${userScriptCommand.name}' for member access.`,
 				);
 			}
 
-			const cacheKey = userScriptCommand.path ?? userScriptCommand.id;
-			if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
-				preloadedScripts.set(cacheKey, exportsRef);
-			}
-
 			const settingsExport =
 				typeof exportsRef === "object" || typeof exportsRef === "function"
 					? (exportsRef as Record<string, unknown>).settings
@@ -227,10 +212,10 @@ export class SingleMacroEngine {
 				);
 			}
 
-			const resolvedMember =
-				selection.candidate.exportsRef !== undefined
-					? selection.candidate.resolvedMember
-					: this.resolveMemberAccess(exportsRef, selection.memberAccess);
+			const resolvedMember = this.resolveMemberAccess(
+				exportsRef,
+				selection.memberAccess,
+			);
 
 			if (!resolvedMember.found) {
 				throw new MacroAbortError(
@@ -327,14 +312,12 @@ export class SingleMacroEngine {
 		macroChoice: IMacroChoice,
 		userScriptCommands: Array<{ command: IUserScript; index: number }>,
 		memberAccess: string[],
-		preloadedScripts: Map<string, unknown>,
 	): Promise<MemberAccessSelection> {
 		if (userScriptCommands.length === 1) {
 			return {
 				candidate: {
 					command: userScriptCommands[0].command,
 					index: userScriptCommands[0].index,
-					resolvedMember: { found: false },
 				},
 				memberAccess,
 			};
@@ -347,48 +330,24 @@ export class SingleMacroEngine {
 		);
 
 		if (selectorMatch) {
-			const exportsRef = await getUserScript(selectorMatch.command, this.app);
-			const cacheKey = selectorMatch.command.path ?? selectorMatch.command.id;
-			if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
-				preloadedScripts.set(cacheKey, exportsRef);
-			}
-
-			const resolvedMember = this.resolveMemberAccess(
-				exportsRef,
-				selectorMatch.memberAccess,
-			);
-			if (!resolvedMember.found) {
-				throw new MacroAbortError(
-					`Macro '${macroChoice.name}' targeted script '${selectorMatch.command.name}', but that script does not export '${selectorMatch.memberAccess.join(
-						"::",
-					)}'.`,
-				);
-			}
-
 			return {
 				candidate: {
 					command: selectorMatch.command,
 					index: selectorMatch.index,
-					exportsRef,
-					resolvedMember,
 				},
 				memberAccess: selectorMatch.memberAccess,
 			};
 		}
 
-		const candidates: UserScriptCandidate[] = [];
+		const candidates: Array<
+			UserScriptCandidate & { resolvedMember: { found: boolean; value?: unknown } }
+		> = [];
 
 		for (const entry of userScriptCommands) {
 			const exportsRef = await getUserScript(entry.command, this.app);
-			const cacheKey = entry.command.path ?? entry.command.id;
-			if (cacheKey && exportsRef !== undefined && exportsRef !== null) {
-				preloadedScripts.set(cacheKey, exportsRef);
-			}
-
 			candidates.push({
 				command: entry.command,
 				index: entry.index,
-				exportsRef,
 				resolvedMember: this.resolveMemberAccess(exportsRef, memberAccess),
 			});
 		}

tests/e2e/macro-member-access.test.ts

@@ -15,7 +15,8 @@ import type {
 
 const VAULT = "dev";
 const PLUGIN_ID = "quickadd";
-const WAIT_OPTS = { timeoutMs: 10_000, intervalMs: 200 };
+const waitTimeoutMs = Number(process.env.E2E_TIMEOUT_MS) || 15_000;
+const WAIT_OPTS = { timeoutMs: waitTimeoutMs, intervalMs: 200 };
 const TEST_PREFIX = "__qa-test-964-";
 
 let obsidian: ObsidianClient;