diff --git a/src/engine/MacroChoiceEngine.entry.test.ts b/src/engine/MacroChoiceEngine.entry.test.ts index 5a944b34..1604f8b1 100644 --- a/src/engine/MacroChoiceEngine.entry.test.ts +++ b/src/engine/MacroChoiceEngine.entry.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { MacroChoiceEngine } from "./MacroChoiceEngine"; import type QuickAdd from "../main"; import type { IChoiceExecutor } from "../IChoiceExecutor"; @@ -211,6 +211,122 @@ describe("MacroChoiceEngine user script entry handling", () => { }); }); +describe("MacroChoiceEngine user script variable propagation", () => { + const app = {} as App; + const plugin = {} as unknown as QuickAdd; + + let choiceExecutor: IChoiceExecutor; + let variables: Map; + let macroChoice: IMacroChoice; + let logs: Array; + let getApiSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + mockGetUserScript.mockReset(); + mockInitializeUserScriptSettings.mockReset(); + mockSuggest.mockReset(); + + getApiSpy = vi + .spyOn(QuickAddApi, "GetApi") + .mockReturnValue({} as unknown as ReturnType); + + logs = []; + variables = new Map(); + choiceExecutor = { + execute: vi.fn(), + variables, + }; + + const makeScript = (index: number): IUserScript => ({ + id: `script-${index}`, + name: `Script ${index}`, + type: CommandType.UserScript, + path: `script-${index}.js`, + settings: {}, + }); + + macroChoice = { + id: "macro-sequence", + name: "Macro sequence", + type: "Macro", + command: false, + runOnStartup: false, + macro: { + id: "macro-sequence", + name: "Macro sequence", + commands: [ + makeScript(1), + makeScript(2), + makeScript(3), + makeScript(4), + ], + } as IMacro, + }; + + mockGetUserScript.mockImplementation((command: IUserScript) => { + const nextValueByPath: Record = { + "script-1.js": 1, + "script-2.js": 2, + "script-3.js": 3, + "script-4.js": 3, + }; + + const nextValue = nextValueByPath[command.path ?? ""] ?? 0; + + return Promise.resolve(async (params: { variables: Record }) => { + logs.push(params.variables.target as number | undefined); + if (nextValue !== 0) { + params.variables.target = nextValue; + } + }); + }); + }); + + afterEach(() => { + getApiSpy.mockRestore(); + }); + + it("propagates variable mutations across sequential user scripts", async () => { + const engine = new MacroChoiceEngine( + app, + plugin, + macroChoice, + choiceExecutor, + variables, + ); + + await engine.run(); + + expect(logs).toEqual([undefined, 1, 2, 3]); + expect(choiceExecutor.variables.get("target")).toBe(3); + }); + + it("merges existing executor variables into provided map without losing state", async () => { + const executorVariables = new Map([ + ["keep", "executor"], + ]); + const providedVariables = new Map([ + ["override", 1], + ]); + + const engine = new MacroChoiceEngine( + app, + plugin, + macroChoice, + { + ...choiceExecutor, + variables: executorVariables, + }, + providedVariables, + ); + + expect(engine["params"].variables.keep).toBe("executor"); + expect(engine["params"].variables.override).toBe(1); + expect(engine["choiceExecutor"].variables).toBe(providedVariables); + }); +}); + describe("MacroChoiceEngine choice command cancellation", () => { const app = {} as App; let plugin: QuickAdd & { getChoiceById: ReturnType }; diff --git a/src/engine/MacroChoiceEngine.ts b/src/engine/MacroChoiceEngine.ts index 529571f1..7f809b0e 100644 --- a/src/engine/MacroChoiceEngine.ts +++ b/src/engine/MacroChoiceEngine.ts @@ -47,6 +47,7 @@ import type { ScriptCondition } from "../types/macros/Conditional/types"; import { evaluateCondition } from "./helpers/conditionalEvaluator"; import { handleMacroAbort } from "../utils/macroAbortHandler"; import { buildOpenFileOptions } from "./helpers/openFileOptions"; +import { createVariablesProxy } from "../utils/variablesProxy"; type ConditionalScriptRunner = () => Promise; @@ -78,6 +79,42 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { private userScriptCommand: IUserScript | null; private conditionalScriptCache = new Map(); private readonly preloadedUserScripts: Map; + private buildParams( + app: App, + plugin: QuickAdd, + choiceExecutor: IChoiceExecutor, + sharedVariables: Map + ) { + return { + app, + quickAddApi: QuickAddApi.GetApi(app, plugin, choiceExecutor), + variables: createVariablesProxy(sharedVariables), + obsidian, + abort: (message?: string) => { + throw new MacroAbortError(message); + }, + }; + } + + private initSharedVariables( + choiceExecutor: IChoiceExecutor, + providedVariables?: Map + ): Map { + const existingVariables = choiceExecutor.variables; + + if (providedVariables) { + if (existingVariables && providedVariables !== existingVariables) { + existingVariables.forEach((value, key) => { + if (!providedVariables.has(key)) { + providedVariables.set(key, value); + } + }); + } + return providedVariables; + } + + return existingVariables ?? new Map(); + } constructor( app: App, @@ -93,19 +130,12 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { this.macro = choice?.macro; this.choiceExecutor = choiceExecutor; this.preloadedUserScripts = preloadedUserScripts ?? new Map(); - this.params = { - app: this.app, - quickAddApi: QuickAddApi.GetApi(app, plugin, choiceExecutor), - variables: {}, - obsidian, - abort: (message?: string) => { - throw new MacroAbortError(message); - }, - }; - - variables?.forEach((value, key) => { - this.params.variables[key] = value; - }); + const sharedVariables = this.initSharedVariables( + choiceExecutor, + variables + ); + this.choiceExecutor.variables = sharedVariables; + this.params = this.buildParams(app, plugin, choiceExecutor, sharedVariables); } async run(): Promise { @@ -151,14 +181,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { if (command?.type === CommandType.Conditional) { await this.executeConditional(command as IConditionalCommand); } - - this.pullExecutorVariablesIntoParams(); - Object.keys(this.params.variables).forEach((key) => { - this.choiceExecutor.variables.set( - key, - this.params.variables[key] - ); - }); } } catch (error) { if ( @@ -462,7 +484,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { } private async executeConditional(command: IConditionalCommand) { - this.pullExecutorVariablesIntoParams(); const shouldRunThenBranch = await evaluateCondition(command.condition, { variables: this.params.variables, evaluateScriptCondition: async (condition: ScriptCondition) => @@ -489,12 +510,6 @@ export class MacroChoiceEngine extends QuickAddChoiceEngine { this.output = value; } - private pullExecutorVariablesIntoParams() { - this.choiceExecutor.variables.forEach((value, key) => { - this.params.variables[key] = value; - }); - } - private async evaluateScriptCondition( condition: ScriptCondition ): Promise { diff --git a/src/engine/SingleMacroEngine.ts b/src/engine/SingleMacroEngine.ts index 43c27f77..fafb4b60 100644 --- a/src/engine/SingleMacroEngine.ts +++ b/src/engine/SingleMacroEngine.ts @@ -288,11 +288,6 @@ export class SingleMacroEngine { engine: MacroChoiceEngine, settings: Record, ): Promise { - // Ensure params reflect latest shared variables before executing - this.choiceExecutor.variables.forEach((value, key) => { - engine.params.variables[key] = value; - }); - if (typeof member === "function") { return await ( member as ( diff --git a/src/quickAddApi.ts b/src/quickAddApi.ts index 946fcfcd..c704f5b4 100644 --- a/src/quickAddApi.ts +++ b/src/quickAddApi.ts @@ -273,8 +273,10 @@ export class QuickAddApi { } if (settings?.shouldAssignVariables) { - // Copy over `output` and `output-quoted` to the variables (if 'outout' is variable name) - Object.assign(choiceExecutor.variables, assistantRes); + // Copy over `output` and `output-quoted` to the variables (if 'output' is variable name) + Object.entries(assistantRes).forEach(([key, value]) => { + choiceExecutor.variables.set(key, value); + }); } return assistantRes; @@ -372,8 +374,10 @@ export class QuickAddApi { } if (settings?.shouldAssignVariables) { - // Copy over `output` and `output-quoted` to the variables (if 'outout' is variable name) - Object.assign(choiceExecutor.variables, assistantRes); + // Copy over `output` and `output-quoted` to the variables (if 'output' is variable name) + Object.entries(assistantRes).forEach(([key, value]) => { + choiceExecutor.variables.set(key, value); + }); } return assistantRes; diff --git a/src/utils/variablesProxy.test.ts b/src/utils/variablesProxy.test.ts new file mode 100644 index 00000000..c6f44621 --- /dev/null +++ b/src/utils/variablesProxy.test.ts @@ -0,0 +1,133 @@ +import { describe, expect, it } from "vitest"; +import { createVariablesProxy } from "./variablesProxy"; + +describe("createVariablesProxy", () => { + it("reads and writes through to the backing map", () => { + const backing = new Map([["a", 1]]); + const proxy = createVariablesProxy(backing); + + expect(proxy.a).toBe(1); + proxy.b = 2; + expect(backing.get("b")).toBe(2); + + delete proxy.a; + expect(backing.has("a")).toBe(false); + }); + + it("enumerates keys like a plain object", () => { + const backing = new Map([ + ["foo", "bar"], + ["num", 3], + ]); + const proxy = createVariablesProxy(backing); + + expect(Object.keys(proxy)).toEqual(["foo", "num"]); + expect(JSON.stringify(proxy)).toBe('{"foo":"bar","num":3}'); + }); + + it("reflects external map mutations immediately", () => { + const backing = new Map(); + const proxy = createVariablesProxy(backing); + + backing.set("outside", 42); + expect(proxy.outside).toBe(42); + + backing.delete("outside"); + expect(proxy.outside).toBeUndefined(); + expect(Object.keys(proxy)).toEqual([]); + }); + + it("ignores symbol property access and does not throw", () => { + const backing = new Map(); + const proxy = createVariablesProxy(backing); + const sym = Symbol("test"); + + // @ts-expect-error accessing symbol + expect(proxy[sym]).toBeUndefined(); + // @ts-expect-error setting symbol + proxy[sym] = "value"; + expect(backing.size).toBe(0); + }); + + it("works with for...in and Object.assign", () => { + const backing = new Map([ + ["x", 1], + ["y", 2], + ]); + const proxy = createVariablesProxy(backing); + + const seen: string[] = []; + for (const key in proxy) { + seen.push(key); + } + expect(seen).toEqual(["x", "y"]); + + const copy = Object.assign({}, proxy); + expect(copy).toEqual({ x: 1, y: 2 }); + }); + + it("supports hasOwnProperty shim without exposing full prototype", () => { + const backing = new Map([["foo", "bar"]]); + const proxy = createVariablesProxy(backing); + + expect(typeof proxy.hasOwnProperty).toBe("function"); + expect(proxy.hasOwnProperty("foo")).toBe(true); + expect(proxy.hasOwnProperty("missing")).toBe(false); + // Prototype helpers are still absent + expect((proxy as Record).toString).toBeUndefined(); + }); + + it("enumerates via Object.entries and Object.values", () => { + const backing = new Map([ + ["a", 1], + ["b", "two"], + ]); + const proxy = createVariablesProxy(backing); + + expect(Object.entries(proxy)).toEqual([ + ["a", 1], + ["b", "two"], + ]); + expect(Object.values(proxy)).toEqual([1, "two"]); + }); + + it("handles empty string and numeric string keys", () => { + const backing = new Map(); + const proxy = createVariablesProxy(backing); + + proxy[""] = "empty"; + proxy["123"] = 123; + + expect(backing.get("")).toBe("empty"); + expect(backing.get("123")).toBe(123); + expect(Object.keys(proxy)).toEqual(["", "123"]); + }); + + it("distinguishes setting undefined vs delete", () => { + const backing = new Map(); + const proxy = createVariablesProxy(backing); + + proxy.flag = undefined; + expect(backing.has("flag")).toBe(true); + expect(backing.get("flag")).toBeUndefined(); + + delete proxy.flag; + expect(backing.has("flag")).toBe(false); + }); + + it("keeps multiple proxies over the same map in sync", () => { + const backing = new Map(); + const proxyA = createVariablesProxy(backing); + const proxyB = createVariablesProxy(backing); + + proxyA.shared = "yes"; + expect(proxyB.shared).toBe("yes"); + + backing.set("other", 42); + expect(proxyA.other).toBe(42); + expect(proxyB.other).toBe(42); + + delete proxyB.shared; + expect(backing.has("shared")).toBe(false); + }); +}); diff --git a/src/utils/variablesProxy.ts b/src/utils/variablesProxy.ts new file mode 100644 index 00000000..f9210699 --- /dev/null +++ b/src/utils/variablesProxy.ts @@ -0,0 +1,54 @@ +/** + * Creates a plain-object style proxy over a Map so scripts can read/write + * `params.variables.foo` while the underlying source of truth remains the Map. + * Includes a hasOwnProperty shim for backward compatibility without exposing + * the full Object prototype chain. + */ +export function createVariablesProxy( + store: Map +): Record { + const target = { + hasOwnProperty: (key: string) => store.has(key), + }; + + return new Proxy(target, { + get(_t, prop: string | symbol) { + if (prop === "hasOwnProperty") return target.hasOwnProperty; + if (typeof prop !== "string") return undefined; + return store.get(prop); + }, + set(_t, prop: string | symbol, value) { + if (typeof prop !== "string") return true; + store.set(prop, value); + return true; + }, + deleteProperty(_t, prop: string | symbol) { + if (typeof prop !== "string") return true; + store.delete(prop); + return true; + }, + has(_t, prop: string | symbol) { + return typeof prop === "string" && store.has(prop); + }, + ownKeys() { + return Array.from(store.keys()); + }, + getOwnPropertyDescriptor(_t, prop: string | symbol) { + if (prop === "hasOwnProperty") { + return { + value: target.hasOwnProperty, + writable: false, + configurable: false, + enumerable: false, + }; + } + if (typeof prop !== "string" || !store.has(prop)) return undefined; + return { + value: store.get(prop), + writable: true, + configurable: true, + enumerable: true, + }; + }, + }); +}